-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pd 1481 build new main product landing page #1460
Pd 1481 build new main product landing page #1460
Conversation
WalkthroughThe pull request introduces various updates across multiple components in the Ember.js application. Key changes include the addition of new components such as Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
277e042
to
b7390be
Compare
Deploying irenestaging with Cloudflare Pages
|
b7390be
to
a63db13
Compare
31eaf00
to
229f5f8
Compare
229f5f8
to
8c3a0a1
Compare
8c3a0a1
to
2bca84c
Compare
2bca84c
to
913eec8
Compare
913eec8
to
e0e7e9f
Compare
e0e7e9f
to
5c11fc4
Compare
5c11fc4
to
a47ec7d
Compare
a47ec7d
to
0c653a0
Compare
0c653a0
to
491d1c6
Compare
491d1c6
to
cba7279
Compare
ef0e063
to
ff82cb6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (13)
app/components/home-page/index.ts (1)
37-40
: Consider using a translation key enum for type safety.The translation keys are currently hardcoded strings. This could lead to runtime errors if the keys are mistyped or changed in the translations file.
+enum TranslationKeys { + VAPT = 'vapt', + APPKNOX = 'appknox', + APPKNOX_DESC = 'appknoxDesc', + APP_MONITORING = 'appMonitoring', + STOREKNOX = 'storeknox', + STOREKNOX_DESC = 'storeknoxDesc' +} title: this.orgIsAnEnterprise - ? this.intl.t('vapt') - : this.intl.t('appknox'), - desc: this.intl.t('appknoxDesc'), + ? this.intl.t(TranslationKeys.VAPT) + : this.intl.t(TranslationKeys.APPKNOX), + desc: this.intl.t(TranslationKeys.APPKNOX_DESC),Also applies to: 46-49
app/components/side-nav/index.scss (3)
62-65
: Consider reducing the padding for visual consistencyThe 3em padding-top in the
.no-switcher
class creates a significantly larger gap compared to other spacing values in the component (most are between 0.5em to 1.5em). Consider reducing this to maintain consistent vertical rhythm.&.no-switcher { - padding-top: 3em; + padding-top: 1.5em; }
153-156
: Consider using currentColor for SVG pathsInstead of explicitly setting the stroke color, consider using
currentColor
to inherit the color from the parent element. This would reduce redundancy and make the styling more maintainable..menu-item-icon-svg { path { - stroke: var(--side-nav-inherit-background-text-color) !important; + stroke: currentColor; } }
Line range hint
1-195
: Consider enhancing accessibility featuresThe side navigation could benefit from additional accessibility improvements:
- Add appropriate ARIA labels and roles
- Ensure proper focus management
- Add keyboard navigation support between menu items
- Consider adding a skip link for keyboard users
Would you like me to provide specific implementation details for these accessibility enhancements?
app/components/side-nav/index.ts (3)
73-73
: Consider abstracting window service for better testabilityInstead of directly injecting the window service, consider creating a wrapper service that encapsulates browser APIs. This would make the component more testable and maintainable.
// services/browser.ts export default class BrowserService extends Service { get window(): Window { return window; } } // In component @service declare browser: BrowserService;
85-93
: Optimizeclasses
getter with memoizationThe
classes
getter creates a new object on every access. Consider memoizing the result to prevent unnecessary object creation.private _classes?: Record<string, string>; get classes() { if (!this._classes) { this._classes = { menuItemText: styles['menu-item-text'], menuItemLink: styles['menu-item-link'], menuItemLinkActive: styles['active'], menuItemTooltip: styles['menu-item-tooltip'], menuItemIcon: styles['menu-item-icon'], }; } return this._classes; }
126-130
: Enhance Glint registry with blocks typingConsider adding explicit typing for the component's blocks in the registry for better type safety.
declare module '@glint/environment-ember-loose/registry' { export default interface Registry { SideNav: typeof SideNavComponent & { Blocks: SideNavSignature['Blocks']; }; } }tests/integration/components/top-nav-test.js (3)
10-20
: Consider enhancing NotificationsStub with message clearing.The NotificationsStub could benefit from a
clear()
method to reset messages between tests, improving test isolation.class NotificationsStub extends Service { errorMsg = null; successMsg = null; error(msg) { this.errorMsg = msg; } success(msg) { this.successMsg = msg; } + clear() { + this.errorMsg = null; + this.successMsg = null; + } }
49-64
: Consider extracting organization setup into a helper method.The organization setup logic could be moved to a separate helper method to improve readability and reusability.
+ async function setupTestOrganization(testContext) { + testContext.server.createList('organization', 1); + const store = testContext.owner.lookup('service:store'); + const organizationMe = store.createRecord('organization-me', { + is_owner: true, + is_admin: true, + }); + class OrganizationMeStub extends Service { + org = organizationMe; + } + await testContext.owner.lookup('service:organization').load(); + testContext.owner.register('service:me', OrganizationMeStub); + return organizationMe; + } hooks.beforeEach(async function () { - this.server.createList('organization', 1); - const store = this.owner.lookup('service:store'); - const organizationMe = store.createRecord('organization-me', { - is_owner: true, - is_admin: true, - }); - class OrganizationMeStub extends Service { - org = organizationMe; - } - await this.owner.lookup('service:organization').load(); - this.owner.register('service:me', OrganizationMeStub); + await setupTestOrganization(this);
91-101
: Enhance assertion messages with more descriptive text.The assertions would be more maintainable with explicit messages describing the expected behavior.
- assert.dom('[data-test-topNav]').exists(); + assert.dom('[data-test-topNav]').exists('Top navigation bar should be rendered'); - assert.dom('[data-test-topNav-title]').hasText(this.title); + assert.dom('[data-test-topNav-title]').hasText(this.title, 'Title should match the provided text'); - assert.dom('[data-test-bell-icon]').isNotDisabled(); + assert.dom('[data-test-bell-icon]').isNotDisabled('Notification bell should be clickable'); assert .dom('[data-test-topNav-profileBtn]') .isNotDisabled() - .hasText(this.user.username); + .hasText(this.user.username, 'Profile button should display username');app/components/appknox-wrapper/index.ts (1)
79-81
: Add return type annotations to getters.The getters lack explicit return type annotations, which would improve type safety and code documentation.
Example fix for a few getters:
- get isShowAnalytics() { + get isShowAnalytics(): boolean { return this.me.org?.is_member === false; } - get showBilling() { + get showBilling(): boolean { const orgShowBilling = this.organization?.selected?.showBilling; const isOwner = this.me.org?.is_owner; return orgShowBilling && isOwner; }Also applies to: 83-88, 90-92, 94-96, 98-100, 102-107
app/styles/_component-variables.scss (2)
352-352
: Consider using a dedicated z-index variable.Instead of calculating the z-index inline, consider defining a dedicated z-index variable for the side navigation. This makes it easier to manage z-index hierarchy across components.
- --side-nav-zIndex: calc(var(--zIndex-appbar) + 1); + --zIndex-side-nav: calc(var(--zIndex-appbar) + 1); + --side-nav-zIndex: var(--zIndex-side-nav);
1916-1923
: Consider more semantic variable names.The current variable names are very generic. Consider using more semantic names that reflect their specific usage context.
- --home-page-button-background-color: var(--common-white); + --home-page-action-button-background-color: var(--common-white); - --home-page-product-card-background-color: var(--common-white); + --home-page-product-card-surface-color: var(--common-white);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.json
is excluded by!**/package-lock.json
📒 Files selected for processing (78)
app/components/ak-popover/index.hbs
(1 hunks)app/components/ak-popover/index.ts
(1 hunks)app/components/ak-svg/sm-indicator.hbs
(1 hunks)app/components/ak-svg/vp-indicator.hbs
(1 hunks)app/components/appknox-wrapper/index.hbs
(1 hunks)app/components/appknox-wrapper/index.scss
(2 hunks)app/components/appknox-wrapper/index.ts
(4 hunks)app/components/appknox-wrapper/onboarding-guide/index.scss
(1 hunks)app/components/appknox-wrapper/onboarding-guide/index.ts
(3 hunks)app/components/file-compare/index.ts
(2 hunks)app/components/file-compare/table/index.ts
(3 hunks)app/components/file-compare/vulnerability-details/index.ts
(2 hunks)app/components/home-page/index.hbs
(1 hunks)app/components/home-page/index.js
(0 hunks)app/components/home-page/index.scss
(1 hunks)app/components/home-page/index.ts
(1 hunks)app/components/home-page/organization-dashboard/header/index.hbs
(0 hunks)app/components/home-page/organization-dashboard/header/index.scss
(0 hunks)app/components/home-page/organization-dashboard/index.hbs
(0 hunks)app/components/home-page/organization-dashboard/index.ts
(0 hunks)app/components/home-page/organization-dashboard/side-nav/security-menu-item/index.ts
(0 hunks)app/components/home-page/product-card/index.hbs
(1 hunks)app/components/home-page/product-card/index.scss
(1 hunks)app/components/home-page/product-card/index.ts
(1 hunks)app/components/home-page/security-dashboard-nav/index.hbs
(0 hunks)app/components/home-page/security-dashboard-nav/index.scss
(0 hunks)app/components/home-page/security-dashboard-nav/index.ts
(0 hunks)app/components/partner/template.hbs
(1 hunks)app/components/security-wrapper/index.hbs
(1 hunks)app/components/security-wrapper/index.scss
(1 hunks)app/components/security-wrapper/index.ts
(1 hunks)app/components/security/nav-menu/index.hbs
(0 hunks)app/components/security/nav-menu/index.scss
(1 hunks)app/components/side-nav/index.hbs
(4 hunks)app/components/side-nav/index.scss
(7 hunks)app/components/side-nav/index.ts
(1 hunks)app/components/side-nav/product-switcher/index.hbs
(1 hunks)app/components/side-nav/product-switcher/index.scss
(1 hunks)app/components/side-nav/product-switcher/index.ts
(1 hunks)app/components/side-nav/security-menu-item/index.ts
(1 hunks)app/components/storeknox-wrapper/index.hbs
(1 hunks)app/components/storeknox-wrapper/index.scss
(1 hunks)app/components/storeknox-wrapper/index.ts
(1 hunks)app/components/top-nav/index.hbs
(1 hunks)app/components/top-nav/index.scss
(1 hunks)app/components/top-nav/index.ts
(4 hunks)app/router.ts
(3 hunks)app/routes/authenticated/dashboard.ts
(1 hunks)app/routes/authenticated/home.ts
(1 hunks)app/routes/authenticated/index.ts
(1 hunks)app/routes/authenticated/partner.ts
(2 hunks)app/routes/authenticated/storeknox.ts
(1 hunks)app/services/user-auth.ts
(1 hunks)app/styles/_component-variables.scss
(3 hunks)app/styles/_icons.scss
(1 hunks)app/styles/_theme.scss
(1 hunks)app/templates/authenticated.hbs
(1 hunks)app/templates/authenticated/dashboard.hbs
(1 hunks)app/templates/authenticated/home.hbs
(1 hunks)app/templates/authenticated/partner.hbs
(1 hunks)app/templates/authenticated/security.hbs
(1 hunks)app/templates/authenticated/storeknox.hbs
(1 hunks)app/templates/authenticated/storeknox/inventory.hbs
(1 hunks)app/utils/scroll-to-top.ts
(2 hunks)tests/acceptance/home-page-test.js
(1 hunks)tests/acceptance/side-nav-test.js
(1 hunks)tests/integration/components/appknox-wrapper-test.js
(1 hunks)tests/integration/components/appknox-wrapper/onboarding-guide-test.js
(1 hunks)tests/integration/components/home-page/organization-dashboard/header-test.js
(0 hunks)tests/integration/components/home-page/organization-dashboard/onboarding-guide-test.js
(0 hunks)tests/integration/components/home-page/organization-dashboard/side-nav-test.js
(0 hunks)tests/integration/components/home-page/security-dashboard-nav-test.js
(0 hunks)tests/integration/components/security-wrapper-test.js
(1 hunks)tests/integration/components/storeknox-wrapper-test.js
(1 hunks)tests/integration/components/top-nav-test.js
(1 hunks)translations/en.json
(10 hunks)translations/ja.json
(10 hunks)types/ak-svg.d.ts
(1 hunks)
💤 Files with no reviewable changes (14)
- app/components/home-page/index.js
- app/components/home-page/organization-dashboard/header/index.hbs
- app/components/home-page/organization-dashboard/header/index.scss
- app/components/home-page/organization-dashboard/index.hbs
- app/components/home-page/organization-dashboard/index.ts
- app/components/home-page/organization-dashboard/side-nav/security-menu-item/index.ts
- app/components/home-page/security-dashboard-nav/index.hbs
- app/components/home-page/security-dashboard-nav/index.scss
- app/components/home-page/security-dashboard-nav/index.ts
- app/components/security/nav-menu/index.hbs
- tests/integration/components/home-page/organization-dashboard/header-test.js
- tests/integration/components/home-page/organization-dashboard/onboarding-guide-test.js
- tests/integration/components/home-page/organization-dashboard/side-nav-test.js
- tests/integration/components/home-page/security-dashboard-nav-test.js
🚧 Files skipped from review as they are similar to previous changes (54)
- app/components/ak-popover/index.hbs
- app/components/ak-popover/index.ts
- app/components/ak-svg/sm-indicator.hbs
- app/components/ak-svg/vp-indicator.hbs
- app/components/appknox-wrapper/index.hbs
- app/components/appknox-wrapper/index.scss
- app/components/appknox-wrapper/onboarding-guide/index.scss
- app/components/appknox-wrapper/onboarding-guide/index.ts
- app/components/file-compare/index.ts
- app/components/file-compare/table/index.ts
- app/components/file-compare/vulnerability-details/index.ts
- app/components/home-page/index.scss
- app/components/home-page/product-card/index.scss
- app/components/home-page/product-card/index.ts
- app/components/partner/template.hbs
- app/components/security-wrapper/index.hbs
- app/components/security-wrapper/index.scss
- app/components/security-wrapper/index.ts
- app/components/security/nav-menu/index.scss
- app/components/side-nav/index.hbs
- app/components/side-nav/product-switcher/index.hbs
- app/components/side-nav/product-switcher/index.scss
- app/components/side-nav/product-switcher/index.ts
- app/components/side-nav/security-menu-item/index.ts
- app/components/storeknox-wrapper/index.hbs
- app/components/storeknox-wrapper/index.scss
- app/components/storeknox-wrapper/index.ts
- app/components/top-nav/index.hbs
- app/components/top-nav/index.scss
- app/components/top-nav/index.ts
- app/router.ts
- app/routes/authenticated/dashboard.ts
- app/routes/authenticated/home.ts
- app/routes/authenticated/index.ts
- app/routes/authenticated/partner.ts
- app/routes/authenticated/storeknox.ts
- app/services/user-auth.ts
- app/styles/_icons.scss
- app/styles/_theme.scss
- app/templates/authenticated.hbs
- app/templates/authenticated/dashboard.hbs
- app/templates/authenticated/home.hbs
- app/templates/authenticated/partner.hbs
- app/templates/authenticated/security.hbs
- app/templates/authenticated/storeknox.hbs
- app/templates/authenticated/storeknox/inventory.hbs
- app/utils/scroll-to-top.ts
- tests/acceptance/home-page-test.js
- tests/acceptance/side-nav-test.js
- tests/integration/components/appknox-wrapper-test.js
- tests/integration/components/appknox-wrapper/onboarding-guide-test.js
- tests/integration/components/security-wrapper-test.js
- tests/integration/components/storeknox-wrapper-test.js
- types/ak-svg.d.ts
🔇 Additional comments (26)
app/components/home-page/product-card/index.hbs (5)
1-5
: LGTM! Well-structured root container.
The component follows best practices with proper test attributes and CSS class naming.
6-8
: LGTM! Good implementation of dynamic component.
The background image implementation follows the suggested pattern of passing SVG as a component argument.
10-10
: LGTM! Clean content container structure.
11-30
: 🛠️ Refactor suggestion
Enhance accessibility for screen readers.
While the typography and structure are well implemented, consider adding ARIA attributes for better accessibility:
- <AkStack @direction='column' @spacing='1'>
+ <AkStack @direction='column' @spacing='1' role="article">
{{#let (component @indicatorSvg) as |SvgIcon|}}
<SvgIcon
data-test-home-page-product-card-indicator-icon
local-class='product-icon'
+ role="img"
+ aria-label={{@name}}
/>
{{/let}}
<AkTypography
data-test-home-page-product-card-title
@variant='h6'
@fontWeight='bold'
+ role="heading"
+ aria-level="6"
>
Likely invalid or redundant comment.
32-43
: 🛠️ Refactor suggestion
Verify route binding and enhance icon accessibility.
Two concerns need to be addressed:
- The
this.route
property needs to be verified as properly initialized in the component's class. - The icon should have appropriate ARIA attributes.
<AkIcon
@iconName='north-east'
@size='small'
@color='primary'
+ role="img"
+ aria-label="Navigate to {{@name}}"
/>
✅ Verification successful
Route binding is correctly implemented, but icon accessibility can be improved
The route
property is properly implemented as a getter that returns this.args.route
, which is correctly typed and passed as an argument to the component. However, the icon's accessibility attributes suggestion remains valid.
- Add
role="img"
and descriptivearia-label
to the icon for better screen reader support.
<AkIcon
@iconName='north-east'
@size='small'
@color='primary'
+ role="img"
+ aria-label="External link to {{@name}}"
/>
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify route property initialization
# Search for route property definition in component's class
rg "route\s*=|route:" app/components/home-page/product-card/
Length of output: 206
Script:
#!/bin/bash
# Let's check the full component class implementation to understand route initialization
ast-grep --pattern 'class $_Component {
$$$
route: string;
$$$
}' app/components/home-page/product-card/index.ts
Length of output: 120
Script:
#!/bin/bash
# Let's try a different pattern to find the component class and see its implementation
cat app/components/home-page/product-card/index.ts
Length of output: 893
app/components/home-page/index.hbs (5)
1-14
: LGTM! Well-structured layout implementation.
The layout structure is well-organized using nested AkStack
components with proper alignment and spacing. Good use of data test attributes and local classes.
18-18
: Translation keys need to be added to translation files.
The translation keys 'selectThePath', 'toSecureYourMobileApps', and 'logout' are still missing from the translation files.
Also applies to: 33-33, 48-48
36-50
: Add error handling for logout action.
The logout functionality should handle potential failures gracefully. Consider implementing the suggested error handling from the previous review.
56-67
: Add empty state handling for product cards.
The product cards section should still implement the empty state handling as suggested in the previous review.
62-62
: Verify translation key existence.
Please ensure the translation key 'takeMeToDashboard' exists in the translation files.
✅ Verification successful
Translation key 'takeMeToDashboard' is properly defined in both English and Japanese locale files
The key exists in:
- translations/en.json
- translations/ja.json
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify translation key existence
# Test: Search for the translation key in locale files
fd -e json -e yaml -e yml | xargs rg '"takeMeToDashboard"'
Length of output: 204
app/components/home-page/index.ts (3)
1-9
: LGTM! Well-organized imports.
The imports are properly structured with clear separation between core Ember imports, types, and application-specific services.
10-16
: LGTM! Well-defined interface with strict types.
The use of literal types for indicator
and coverImage
helps prevent errors by restricting possible values.
58-62
: LGTM! Proper Glint type registration.
The component is correctly registered in the Glint registry, ensuring proper type checking in templates.
app/components/side-nav/index.scss (1)
7-10
: LGTM: Variable naming improvements
The simplified variable naming convention improves readability while maintaining functionality.
app/components/side-nav/index.ts (2)
1-65
: Well-structured type definitions and interfaces!
The TypeScript interfaces and type definitions are comprehensive and follow best practices. The component contract is well-defined through SideNavSignature
, and the interfaces (MenuItem
, LowerMenuItem
, SwitcherMenuItem
) provide clear type safety.
21-34
: Verify integration with related navigation components
The component seems to be part of a larger navigation system. Let's verify the integration with related components.
✅ Verification successful
Let me gather more information about the integration by checking the actual implementations and usage patterns.
Let me check one final aspect about the integration by looking at the main SideNav component implementation and its relationship with the product switcher.
Integration is properly implemented and working as expected
The verification shows that:
- SideNav is correctly integrated in both appknox and storeknox wrappers with proper props
- ProductSwitcher is conditionally rendered based on
showAppMonitoringDashboard
feature flag - The
productSwitcherFilterKey
is properly passed through the component hierarchy - The component handles both collapsed and expanded states with appropriate UI elements
- All required interfaces and types are properly defined
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for related navigation components and their usage patterns
# Check for related navigation components
echo "Checking for related navigation components..."
rg -l "SideNav" --type-add 'template:*.{hbs,handlebars}' -g '!node_modules' -t template
# Check for product switcher usage
echo "Checking for product switcher implementations..."
rg "productSwitcherFilterKey" --type ts
Length of output: 726
Script:
#!/bin/bash
# Check the implementation and usage of SideNav in the wrapper components
echo "Checking SideNav usage in wrapper components..."
rg -A 5 "SideNav" app/components/storeknox-wrapper/index.hbs app/components/appknox-wrapper/index.hbs
# Check the product switcher component implementation
echo "Checking product switcher component implementation..."
cat app/components/side-nav/product-switcher/index.ts
# Check if there are any tests for these integrations
echo "Checking for integration tests..."
fd -e ts -e js test --exec rg -l "SideNav"
Length of output: 3721
Script:
#!/bin/bash
# Check the main SideNav component implementation
echo "Checking main SideNav component implementation..."
cat app/components/side-nav/index.ts
# Check the template to see how ProductSwitcher is integrated
echo "Checking SideNav template..."
cat app/components/side-nav/index.hbs
Length of output: 8713
app/components/appknox-wrapper/index.ts (4)
18-26
: LGTM! Well-structured interface declaration.
The AppknoxWrapperSignature
interface is well-defined with proper typing for both Args and Blocks.
33-33
: LGTM! Properly typed RouterService injection.
The RouterService is correctly imported and injected with proper typing.
191-194
: LGTM! Conditional security menu item.
The security menu item is properly integrated with the existing menu structure and correctly uses the tracked isSecurityEnabled
property.
293-295
: LGTM! Proper Glint type registration.
The component is correctly registered in the Glint registry for proper type checking in templates.
translations/ja.json (1)
Line range hint 80-1625
: All translation issues have been previously identified
The untranslated English text entries have already been flagged in previous reviews. Please address those existing comments to provide Japanese translations for:
- Basic UI elements:
- "App Switcher" (アプリスイッチャー)
- "Discovery" (発見)
- "Inventory" (インベントリ)
- "Switch to" (切り替える)
- Product descriptions:
- Appknox description
- Storeknox description
- "To Secure Your Mobile Applications" (モバイルアプリケーションを安全に保つために)
- Navigation elements:
- "Take me to the dashboard" (ダッシュボードに移動)
app/styles/_component-variables.scss (1)
376-383
: LGTM!
The product switcher variables are well-organized and consistently follow the existing patterns.
translations/en.json (4)
80-81
: Maintain consistent product name capitalization.
This issue was previously identified. The product names should maintain consistent capitalization ("Appknox" and "StoreKnox") and descriptions should follow parallel structure.
Also applies to: 1505-1506
366-366
: LGTM: New feature translations are clear and concise.
The added translations for "discovery" and "inventory" features are appropriate.
Also applies to: 648-648
593-593
: LGTM: Navigation and UI translations are clear.
The added translations for navigation and UI elements are appropriate and follow consistent patterns.
Also applies to: 1082-1082, 1521-1521, 1529-1529, 1562-1562
1625-1625
: Expand VAPT acronym for clarity.
This issue was previously identified. For consistency with other security-related terms in the codebase, VAPT should be expanded to "Vulnerability Assessment and Penetration Testing (VAPT)".
ff82cb6
to
3111d05
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (19)
app/components/top-nav/index.hbs (3)
8-21
: Consider handling long titles gracefully.The
@noWrap={{true}}
property on AkTypography might truncate long titles without visual indication. Consider adding a tooltip or ellipsis for truncated text.<AkTypography data-test-topNav-title local-class='navbar-title' @variant='h4' @noWrap={{true}} + @tooltip={{@title}} > {{@title}} </AkTypography>
32-46
: Consider making the click handler more generic.The handler name
handleProfileBtnClick
is implementation-specific. Consider a more generic name likeonProfileAction
to improve reusability.<AkButton class={{this.classes.navbarBtn}} - {{on 'click' this.handleProfileBtnClick}} + {{on 'click' this.onProfileAction}} data-test-topNav-profileBtn >
63-71
: Consider DRYing up the color fallback logic.The
or pmi.color 'textPrimary'
pattern is repeated. Consider extracting it to a computed property or helper.+ {{#let (or pmi.color "textPrimary") as |itemColor|}} <AkIcon class='mr-1' - @color={{or pmi.color 'textPrimary'}} + @color={{itemColor}} @iconName={{pmi.iconName}} /> - <AkTypography @color={{or pmi.color 'textPrimary'}}> + <AkTypography @color={{itemColor}}> {{pmi.label}} </AkTypography> + {{/let}}app/components/home-page/index.hbs (3)
1-6
: Add aria-label to main container for better accessibilityThe main container should have an aria-label to improve screen reader navigation.
<AkStack data-test-home-page @direction='column' @alignItems='center' local-class='home-page-container' + aria-label={{t "homePageMainContent"}} >
36-42
: Add aria-label to logout button for better accessibilityThe logout button should have an aria-label to improve screen reader experience.
<AkButton @variant='outlined' @color='neutral' local-class='logout-button' {{on 'click' this.userAuth.invalidateSession}} data-test-home-page-logoutBtn + aria-label={{t "logoutButton"}} >
56-56
: Consider adding responsive spacing for product cards gridThe current fixed spacing (
@spacing='4'
) might not work well on all screen sizes. Consider using responsive spacing classes.-<AkStack class='py-1' @direction='row' @spacing='4' @flexWrap='wrap'> +<AkStack class='py-1' @direction='row' @spacing={{this.responsiveSpacing}} @flexWrap='wrap'>Add to the component class:
get responsiveSpacing() { return window.innerWidth < 768 ? '2' : '4'; }app/components/ak-tabs/item/index.scss (1)
48-50
: Consider adding transition for smoother state changesWhile the hover and focus states are properly defined, the color change might feel abrupt to users.
Consider adding a transition for smoother state changes:
.active-line { position: relative; color: var(--ak-tabs-text-color-primary); font-weight: var(--ak-tabs-font-weight-bold); + transition: color 0.2s ease-in-out; &:hover, &:focus { background-color: unset; color: var(--ak-tabs-text-color-primary); }
app/components/side-nav/product-switcher/index.ts (1)
43-68
: Consider enhancing type safety in switcherMenuItems.The implementation correctly addresses previous review comments by using an array with filter method. However, we could further improve type safety.
Consider this enhancement:
+type ProductKey = 'appknox' | 'storeknox'; + get switcherMenuItems() { - const allMenuItems: SwitcherMenuItem[] = [ + const allMenuItems: (SwitcherMenuItem & { key: ProductKey })[] = [ { id: 'vp-svg', svg: 'ak-svg/vp-indicator',This change would:
- Make the product keys type-safe
- Ensure the filter key comparison is type-checked
app/components/side-nav/index.scss (2)
62-65
: Consider standardizing vertical spacingThe 3em padding-top in
.no-switcher
creates a large gap. Consider:
- Using a more consistent spacing value (1.5-2em) to align with the application's spacing patterns
- Using CSS custom properties for standardized spacing values
&.no-switcher { - padding-top: 3em; + padding-top: var(--side-nav-spacing-large, 2em); }
153-156
: Consider using CSS custom properties for SVG stylingThe current approach tightly couples the SVG styling to the implementation. Consider using CSS custom properties to make the styling more flexible and maintainable.
.menu-item-icon-svg { path { - stroke: var(--side-nav-inherit-background-text-color) !important; + stroke: currentColor; } }app/components/side-nav/index.ts (1)
117-119
: Simplify the isSidebarExpanded getterThe getter can be simplified by directly returning the negation of isCollapsed.
get isSidebarExpanded() { - return !this.args.isCollapsed; + return !this.args.isCollapsed; }tests/integration/components/top-nav-test.js (2)
39-41
: Consider makingisCrispEnabled
more realisticThe
isCrispEnabled
method always returnstrue
without considering any conditions. Consider making it more realistic by:
- Taking into account user preferences
- Checking configuration settings
- Reflecting actual integration state
- isCrispEnabled() { - return true; - } + isCrispEnabled() { + return this.currentUser?.preferences?.crispEnabled ?? false; + }
133-135
: Move service setup to beforeEachThe Freshdesk service configuration should be moved to the beforeEach hook to ensure consistent test setup:
- const freshdesk = this.owner.lookup('service:freshdesk'); - freshdesk.freshchatEnabled = chatSupport; - freshdesk.logUserOutOfSupportWidget = () => {}; + this.set('freshdesk.freshchatEnabled', chatSupport);Then add to beforeEach:
hooks.beforeEach(function() { // ... existing setup ... const freshdesk = this.owner.lookup('service:freshdesk'); freshdesk.logUserOutOfSupportWidget = () => {}; });tests/integration/components/appknox-wrapper-test.js (4)
13-23
: Enhance NotificationsStub for better test assertionsConsider tracking message history to enable assertions on multiple notifications:
class NotificationsStub extends Service { - errorMsg = null; - successMsg = null; + errorMessages = []; + successMessages = []; error(msg) { - this.errorMsg = msg; + this.errorMessages.push(msg); } success(msg) { - this.successMsg = msg; + this.successMessages.push(msg); } }
51-82
: Simplify menuItems function parametersConsider using a single configuration object parameter to improve maintainability:
-const menuItems = ({ - appMonitoring, - sbom, - analytics, - publicApis, - market, - billing, - partner, - security, -}) => +const menuItems = (config) => [ { label: t('allProjects'), icon: 'folder', hasBadge: true, }, - appMonitoring && { label: t('appMonitoring'), icon: 'inventory-2' }, - sbom && { label: t('SBOM'), icon: 'receipt-long' }, + config.appMonitoring && { label: t('appMonitoring'), icon: 'inventory-2' }, + config.sbom && { label: t('SBOM'), icon: 'receipt-long' }, // ... rest of the items ].filter(Boolean);
214-240
: Improve test assertions specificityThe test expectations could be more specific:
test('test knowledge base click', async function (assert) { - assert.expect(5); + assert.expect(6); const freshdesk = this.owner.lookup('service:freshdesk'); freshdesk.supportWidgetIsEnabled = true; + let supportWidgetOpened = false; freshdesk.openSupportWidget = function () { - assert.ok('Knowledge base clicked'); + supportWidgetOpened = true; assert.strictEqual(arguments.length, 0); }; // ... rest of the test await click('[data-test-topNav-KnowledgeBaseBtn]'); + assert.true(supportWidgetOpened, 'Support widget should be opened on click'); });
299-305
: Extract HTTP status codes to constantsReplace magic numbers with named constants for better maintainability:
+ const HTTP_STATUS = { + OK: 200, + NOT_FOUND: 404 + }; + this.server.get('/hudson-api/projects', () => { if (sectionConfig.security) { - return new Response(200); + return new Response(HTTP_STATUS.OK); } else { - return new Response(404); + return new Response(HTTP_STATUS.NOT_FOUND); } });app/styles/_component-variables.scss (1)
376-382
: Consider adding dark theme variations for product switcher.While the light theme implementation is solid, consider adding dark theme-specific variables for better contrast and user experience.
Consider adding these theme-specific variables:
// variables for side-nav/product-switcher --side-nav-product-switcher-modal-item-hover-background: var(--neutral-grey-100); --side-nav-product-switcher-modal-title-background: var(--neutral-grey-100); --side-nav-product-switcher-border: 1px solid var(--border-color-1); --side-nav-product-switcher-bg-main: var(--background-main); --side-nav-product-switcher-box-shadow: var(--box-shadow-light); --side-nav-product-switcher-item-hover-background: var(--hover-light-background); + &.theme-dark { + --side-nav-product-switcher-modal-item-hover-background: var(--hover-dark-background); + --side-nav-product-switcher-modal-title-background: var(--background-dark); + --side-nav-product-switcher-box-shadow: var(--box-shadow-dark); + --side-nav-product-switcher-item-hover-background: var(--hover-dark-background); + }translations/en.json (1)
1562-1562
: Improve clarity of product value proposition.The key message "To Secure Your Mobile Applications" should be more specific about the value proposition:
- "toSecureYourMobileApps": "To Secure Your Mobile Applications", + "toSecureYourMobileApps": "To Secure and Protect Your Mobile Applications",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.json
is excluded by!**/package-lock.json
📒 Files selected for processing (79)
app/components/ak-popover/index.hbs
(1 hunks)app/components/ak-popover/index.ts
(1 hunks)app/components/ak-svg/sm-indicator.hbs
(1 hunks)app/components/ak-svg/vp-indicator.hbs
(1 hunks)app/components/ak-tabs/item/index.scss
(1 hunks)app/components/appknox-wrapper/index.hbs
(1 hunks)app/components/appknox-wrapper/index.scss
(2 hunks)app/components/appknox-wrapper/index.ts
(4 hunks)app/components/appknox-wrapper/onboarding-guide/index.scss
(1 hunks)app/components/appknox-wrapper/onboarding-guide/index.ts
(3 hunks)app/components/file-compare/index.ts
(2 hunks)app/components/file-compare/table/index.ts
(3 hunks)app/components/file-compare/vulnerability-details/index.ts
(2 hunks)app/components/home-page/index.hbs
(1 hunks)app/components/home-page/index.js
(0 hunks)app/components/home-page/index.scss
(1 hunks)app/components/home-page/index.ts
(1 hunks)app/components/home-page/organization-dashboard/header/index.hbs
(0 hunks)app/components/home-page/organization-dashboard/header/index.scss
(0 hunks)app/components/home-page/organization-dashboard/index.hbs
(0 hunks)app/components/home-page/organization-dashboard/index.ts
(0 hunks)app/components/home-page/organization-dashboard/side-nav/security-menu-item/index.ts
(0 hunks)app/components/home-page/product-card/index.hbs
(1 hunks)app/components/home-page/product-card/index.scss
(1 hunks)app/components/home-page/product-card/index.ts
(1 hunks)app/components/home-page/security-dashboard-nav/index.hbs
(0 hunks)app/components/home-page/security-dashboard-nav/index.scss
(0 hunks)app/components/home-page/security-dashboard-nav/index.ts
(0 hunks)app/components/partner/template.hbs
(1 hunks)app/components/security-wrapper/index.hbs
(1 hunks)app/components/security-wrapper/index.scss
(1 hunks)app/components/security-wrapper/index.ts
(1 hunks)app/components/security/nav-menu/index.hbs
(1 hunks)app/components/security/nav-menu/index.scss
(1 hunks)app/components/side-nav/index.hbs
(4 hunks)app/components/side-nav/index.scss
(7 hunks)app/components/side-nav/index.ts
(1 hunks)app/components/side-nav/product-switcher/index.hbs
(1 hunks)app/components/side-nav/product-switcher/index.scss
(1 hunks)app/components/side-nav/product-switcher/index.ts
(1 hunks)app/components/side-nav/security-menu-item/index.ts
(1 hunks)app/components/storeknox-wrapper/index.hbs
(1 hunks)app/components/storeknox-wrapper/index.scss
(1 hunks)app/components/storeknox-wrapper/index.ts
(1 hunks)app/components/top-nav/index.hbs
(1 hunks)app/components/top-nav/index.scss
(1 hunks)app/components/top-nav/index.ts
(4 hunks)app/router.ts
(3 hunks)app/routes/authenticated/dashboard.ts
(1 hunks)app/routes/authenticated/home.ts
(1 hunks)app/routes/authenticated/index.ts
(1 hunks)app/routes/authenticated/partner.ts
(2 hunks)app/routes/authenticated/storeknox.ts
(1 hunks)app/services/user-auth.ts
(1 hunks)app/styles/_component-variables.scss
(3 hunks)app/styles/_icons.scss
(1 hunks)app/styles/_theme.scss
(1 hunks)app/templates/authenticated.hbs
(1 hunks)app/templates/authenticated/dashboard.hbs
(1 hunks)app/templates/authenticated/home.hbs
(1 hunks)app/templates/authenticated/partner.hbs
(1 hunks)app/templates/authenticated/security.hbs
(1 hunks)app/templates/authenticated/storeknox.hbs
(1 hunks)app/templates/authenticated/storeknox/inventory.hbs
(1 hunks)app/utils/scroll-to-top.ts
(2 hunks)tests/acceptance/home-page-test.js
(1 hunks)tests/acceptance/side-nav-test.js
(1 hunks)tests/integration/components/appknox-wrapper-test.js
(1 hunks)tests/integration/components/appknox-wrapper/onboarding-guide-test.js
(1 hunks)tests/integration/components/home-page/organization-dashboard/header-test.js
(0 hunks)tests/integration/components/home-page/organization-dashboard/onboarding-guide-test.js
(0 hunks)tests/integration/components/home-page/organization-dashboard/side-nav-test.js
(0 hunks)tests/integration/components/home-page/security-dashboard-nav-test.js
(0 hunks)tests/integration/components/security-wrapper-test.js
(1 hunks)tests/integration/components/storeknox-wrapper-test.js
(1 hunks)tests/integration/components/top-nav-test.js
(1 hunks)translations/en.json
(10 hunks)translations/ja.json
(10 hunks)types/ak-svg.d.ts
(1 hunks)
💤 Files with no reviewable changes (13)
- app/components/home-page/index.js
- app/components/home-page/organization-dashboard/header/index.hbs
- app/components/home-page/organization-dashboard/header/index.scss
- app/components/home-page/organization-dashboard/index.hbs
- app/components/home-page/organization-dashboard/index.ts
- app/components/home-page/organization-dashboard/side-nav/security-menu-item/index.ts
- app/components/home-page/security-dashboard-nav/index.hbs
- app/components/home-page/security-dashboard-nav/index.scss
- app/components/home-page/security-dashboard-nav/index.ts
- tests/integration/components/home-page/organization-dashboard/header-test.js
- tests/integration/components/home-page/organization-dashboard/onboarding-guide-test.js
- tests/integration/components/home-page/organization-dashboard/side-nav-test.js
- tests/integration/components/home-page/security-dashboard-nav-test.js
🚧 Files skipped from review as they are similar to previous changes (54)
- app/components/ak-popover/index.hbs
- app/components/ak-popover/index.ts
- app/components/ak-svg/sm-indicator.hbs
- app/components/ak-svg/vp-indicator.hbs
- app/components/appknox-wrapper/index.hbs
- app/components/appknox-wrapper/index.scss
- app/components/appknox-wrapper/onboarding-guide/index.scss
- app/components/appknox-wrapper/onboarding-guide/index.ts
- app/components/file-compare/index.ts
- app/components/file-compare/table/index.ts
- app/components/file-compare/vulnerability-details/index.ts
- app/components/home-page/index.scss
- app/components/home-page/index.ts
- app/components/home-page/product-card/index.hbs
- app/components/home-page/product-card/index.scss
- app/components/home-page/product-card/index.ts
- app/components/partner/template.hbs
- app/components/security-wrapper/index.hbs
- app/components/security-wrapper/index.scss
- app/components/security-wrapper/index.ts
- app/components/security/nav-menu/index.hbs
- app/components/security/nav-menu/index.scss
- app/components/side-nav/index.hbs
- app/components/side-nav/product-switcher/index.hbs
- app/components/side-nav/product-switcher/index.scss
- app/components/side-nav/security-menu-item/index.ts
- app/components/storeknox-wrapper/index.hbs
- app/components/storeknox-wrapper/index.scss
- app/components/storeknox-wrapper/index.ts
- app/components/top-nav/index.scss
- app/components/top-nav/index.ts
- app/router.ts
- app/routes/authenticated/dashboard.ts
- app/routes/authenticated/home.ts
- app/routes/authenticated/index.ts
- app/routes/authenticated/partner.ts
- app/routes/authenticated/storeknox.ts
- app/services/user-auth.ts
- app/styles/_icons.scss
- app/styles/_theme.scss
- app/templates/authenticated.hbs
- app/templates/authenticated/dashboard.hbs
- app/templates/authenticated/home.hbs
- app/templates/authenticated/partner.hbs
- app/templates/authenticated/security.hbs
- app/templates/authenticated/storeknox.hbs
- app/templates/authenticated/storeknox/inventory.hbs
- app/utils/scroll-to-top.ts
- tests/acceptance/home-page-test.js
- tests/acceptance/side-nav-test.js
- tests/integration/components/appknox-wrapper/onboarding-guide-test.js
- tests/integration/components/security-wrapper-test.js
- tests/integration/components/storeknox-wrapper-test.js
- types/ak-svg.d.ts
🔇 Additional comments (25)
app/components/top-nav/index.hbs (3)
1-6
: LGTM! Well-structured component setup.
The AkAppbar component follows best practices with proper attribute spreading and test selectors.
23-25
: LGTM! Clean implementation of flexible content area.
The spacer with yielded classes provides good flexibility for content customization.
61-61
: Previous review comment is still applicable.
app/components/ak-tabs/item/index.scss (1)
43-43
: Good use of CSS custom properties!
The switch to using --ak-tabs-text-color-primary
variable improves maintainability and consistency across the application. The removal of !important
is also a positive change as it maintains proper CSS specificity.
app/components/side-nav/product-switcher/index.ts (5)
1-21
: LGTM! Well-structured imports and type definitions.
The imports and interface definitions are comprehensive and follow TypeScript best practices.
23-26
: LGTM! Proper component setup with typed service injections.
The component class is well-structured with properly typed service injections.
27-41
: LGTM! Well-structured reactive properties and simple getters.
The tracked property and basic getters are implemented correctly and follow Ember.js best practices.
70-76
: LGTM! Well-implemented action methods.
The actions are properly typed and handle the anchorRef state correctly.
79-84
: LGTM! Comprehensive type registry declarations.
The Glint registry declarations properly support both component invocation styles.
app/components/side-nav/index.scss (1)
7-10
: LGTM: Variable renaming improves consistency
The variable renaming from --organization-dashboard-side-nav-*
to --side-nav-*
improves naming consistency while maintaining the same functionality.
app/components/side-nav/index.ts (4)
11-65
: Well-structured TypeScript interfaces!
The type definitions are well-organized with proper TypeScript practices:
- Good use of literal types for SVG values
- Clear interface segregation for different menu item types
- Proper optional properties marking
67-83
: Reference existing review comment about image handling
A previous review comment already addresses the improvements needed for image handling.
126-130
: LGTM: Proper Glint type registry declaration
The Glint registry declaration ensures proper type checking in templates.
21-34
: Verify integration with related components
Let's verify the integration with related components and proper event handling.
✅ Verification successful
Let me gather more specific information about the component's integration.
Component integration is properly implemented
The verification shows that the SideNav component is well-integrated within the application:
- Properly used in both
appknox-wrapper
andstoreknox-wrapper
components with consistent prop passing toggleSidebar
implementation exists in both wrapper components with correct state management- Product switcher integration is properly handled with filtering mechanism
- Menu items are correctly typed and implemented across the application
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify component integration and event handling
# Check for proper event handler implementations in related components
echo "Checking event handler implementations..."
rg -l "toggleSidebar.*=.*function" app/components/
# Verify proper usage of the component
echo "Checking component usage..."
rg -l "<SideNav" app/templates/
# Check for proper route definitions for menu items
echo "Checking route definitions..."
rg -l "path:.*" app/router.ts
Length of output: 359
Script:
#!/bin/bash
# Check for SideNav component usage with different patterns
echo "Checking SideNav usage patterns..."
rg -A 5 "<SideNav" --type ts --type hbs
# Look for toggleSidebar implementations
echo "Checking toggleSidebar implementations..."
rg -A 3 "toggleSidebar" --type ts
# Check MenuItem type usage and implementation
echo "Checking MenuItem related code..."
rg -A 2 "MenuItem" --type ts
# Check for product switcher related code
echo "Checking product switcher implementation..."
rg -A 3 "productSwitcherFilterKey" --type ts
Length of output: 11613
app/components/appknox-wrapper/index.ts (3)
18-26
: LGTM! Well-structured interface definition.
The interface is clear, properly typed, and follows TypeScript best practices.
191-194
: LGTM! Clean implementation of security menu item.
The conditional rendering and component integration are well implemented.
291-296
: LGTM! Proper type registry update.
The Glint registry is correctly updated with the new component name.
translations/ja.json (4)
80-81
: Skip commenting on product descriptions
These translation issues were already identified in past review comments.
Also applies to: 1505-1506
1521-1521
: Skip commenting on action phrases
These translation issues were already identified in past review comments.
Also applies to: 1529-1529
366-366
: Skip commenting on navigation items
These translation issues were already identified in past review comments.
Also applies to: 648-648, 593-593, 1082-1082
1562-1562
: Skip commenting on marketing phrases
These translation issues were already identified in past review comments.
Also applies to: 1625-1625
app/styles/_component-variables.scss (2)
346-373
: LGTM! Well-structured side navigation variables.
The implementation follows best practices with proper theme scoping, semantic color variables, and good z-index management.
1492-1502
: Skip comment - Issue already identified.
A past review comment already addresses the incorrect use of background-main for text color in dark theme.
translations/en.json (2)
80-81
: Maintain consistent product name capitalization and branding.
The product names and descriptions should follow consistent capitalization:
Previous review already identified this issue. The suggestion to update "Storeknox" to "StoreKnox" for consistent branding with "Appknox" is still valid.
Also applies to: 1505-1506
1625-1625
: Expand VAPT acronym for better clarity.
Previous review already identified this issue. The suggestion to expand "VAPT" to "Vulnerability Assessment and Penetration Testing (VAPT)" for consistency with other security terms in the file is still valid.
3111d05
to
fe9cbeb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (10)
app/components/home-page/index.hbs (1)
36-50
: Add aria-label to improve accessibilityThe logout button should have an aria-label for better screen reader support.
<AkButton @variant='outlined' @color='neutral' local-class='logout-button' {{on 'click' this.userAuth.invalidateSession}} data-test-home-page-logoutBtn + aria-label={{t 'logout'}} >
tests/integration/components/security-wrapper-test.js (1)
82-105
: Improve test readability by grouping related assertions.The assertions could be better organized by grouping them based on the UI section they're testing.
Consider restructuring the test:
test('it renders main top-nav with logo', async function (assert) { + assert.expect(7); + await render(hbs` <SecurityWrapper @user={{this.user}} /> `); + // Test main navigation structure assert.dom('[data-test-topNav]').exists(); assert.dom('[data-test-topNav-logo]').exists(); assert.dom('[data-test-topNav-title]').doesNotExist(); + + // Test menu components assert.dom('[data-test-security-nav-menu]').exists(); assert.dom('[data-test-side-nav]').doesNotExist(); assert.dom('[data-test-bell-icon]').doesNotExist(); + + // Test profile section assert .dom('[data-test-topNav-profileBtn]') .isNotDisabled() .hasText(this.user.username); });app/components/side-nav/index.scss (2)
153-156
: Consider browser compatibility for SVG stylingThe direct path styling might not work in older browsers. Consider adding a fallback or using a more compatible approach.
.menu-item-icon-svg { + fill: var(--side-nav-inherit-background-text-color) !important; path { stroke: var(--side-nav-inherit-background-text-color) !important; } }
Line range hint
1-195
: Consider enhancing accessibility supportThe side navigation could benefit from additional accessibility improvements:
- Add
aria-current="page"
to active menu items- Consider adding support for Windows High Contrast Mode using forced-colors media query
@media (forced-colors: active) { .side-menu-root { border: 1px solid CanvasText; } .menu-item-link.active::before { background-color: Highlight; } }app/components/side-nav/index.ts (1)
85-93
: Consider memoizing the classes getterThe
classes
getter creates a new object on every access. Consider memoizing this object to prevent unnecessary object creation and potential re-renders.+ private _memoizedClasses: Record<string, string> | null = null; + get classes() { + if (!this._memoizedClasses) { + this._memoizedClasses = { - return { menuItemText: styles['menu-item-text'], menuItemLink: styles['menu-item-link'], menuItemLinkActive: styles['active'], menuItemTooltip: styles['menu-item-tooltip'], menuItemIcon: styles['menu-item-icon'], - }; + }; + } + return this._memoizedClasses; }tests/integration/components/top-nav-test.js (2)
34-42
: Add error handling to IntegrationStub.configureThe configure method should handle potential errors to make tests more robust.
class IntegrationStub extends Service { + configureError = null; + async configure(user) { + try { this.currentUser = user; + } catch (error) { + this.configureError = error; + throw error; + } } isCrispEnabled() { return true; } }
79-81
: Make submissions API mock more specificThe current mock returns an empty array for all cases. Consider adding specific test data to verify different scenarios.
- // handle submissions api for each test - this.server.get('/submissions', () => []); + // Mock submissions API with specific test data + this.server.get('/submissions', (schema, request) => { + const { queryParams } = request; + // Return different data based on query parameters + return schema.submissions.where(queryParams); + });app/components/appknox-wrapper/index.ts (1)
Line range hint
123-195
: Consider extracting menu item configurationsThe
menuItems
getter contains complex conditional logic and a large array of menu items. Consider extracting these configurations into separate methods or a configuration file for better maintainability.Example refactor:
+ private getProjectMenuItem(): MenuItem { + return { + label: this.intl.t('allProjects'), + icon: 'folder', + route: 'authenticated.dashboard.projects', + hasBadge: true, + badgeLabel: this.organization.selected?.projectsCount, + currentWhen: 'authenticated.dashboard.projects authenticated.dashboard.project.files ...' + }; + } get menuItems() { return [ - { - label: this.intl.t('allProjects'), - icon: 'folder', - // ... other properties - }, + this.getProjectMenuItem(), // ... other menu items ].filter(Boolean) as MenuItem[]; }app/styles/_component-variables.scss (1)
1916-1923
: Consider using a more contrasting color for button background.Using
--background-main
for button background might not provide sufficient contrast with the page's light background. Consider using a more distinct color to improve visibility and user experience.// variables for home-page --home-page-background-color: var(--background-light); - --home-page-button-background-color: var(--background-main); + --home-page-button-background-color: var(--primary-main);translations/en.json (1)
1625-1625
: Expand security testing acronym for clarity.For consistency with other expanded security terms in the file (like DAST), expand VAPT:
- "vapt": "VAPT", + "vapt": "Vulnerability Assessment and Penetration Testing (VAPT)",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.json
is excluded by!**/package-lock.json
📒 Files selected for processing (79)
app/components/ak-popover/index.hbs
(1 hunks)app/components/ak-popover/index.ts
(1 hunks)app/components/ak-svg/sm-indicator.hbs
(1 hunks)app/components/ak-svg/vp-indicator.hbs
(1 hunks)app/components/ak-tabs/item/index.scss
(1 hunks)app/components/appknox-wrapper/index.hbs
(1 hunks)app/components/appknox-wrapper/index.scss
(2 hunks)app/components/appknox-wrapper/index.ts
(4 hunks)app/components/appknox-wrapper/onboarding-guide/index.scss
(1 hunks)app/components/appknox-wrapper/onboarding-guide/index.ts
(3 hunks)app/components/file-compare/index.ts
(2 hunks)app/components/file-compare/table/index.ts
(3 hunks)app/components/file-compare/vulnerability-details/index.ts
(2 hunks)app/components/home-page/index.hbs
(1 hunks)app/components/home-page/index.js
(0 hunks)app/components/home-page/index.scss
(1 hunks)app/components/home-page/index.ts
(1 hunks)app/components/home-page/organization-dashboard/header/index.hbs
(0 hunks)app/components/home-page/organization-dashboard/header/index.scss
(0 hunks)app/components/home-page/organization-dashboard/index.hbs
(0 hunks)app/components/home-page/organization-dashboard/index.ts
(0 hunks)app/components/home-page/organization-dashboard/side-nav/security-menu-item/index.ts
(0 hunks)app/components/home-page/product-card/index.hbs
(1 hunks)app/components/home-page/product-card/index.scss
(1 hunks)app/components/home-page/product-card/index.ts
(1 hunks)app/components/home-page/security-dashboard-nav/index.hbs
(0 hunks)app/components/home-page/security-dashboard-nav/index.scss
(0 hunks)app/components/home-page/security-dashboard-nav/index.ts
(0 hunks)app/components/partner/template.hbs
(1 hunks)app/components/security-wrapper/index.hbs
(1 hunks)app/components/security-wrapper/index.scss
(1 hunks)app/components/security-wrapper/index.ts
(1 hunks)app/components/security/nav-menu/index.hbs
(1 hunks)app/components/security/nav-menu/index.scss
(1 hunks)app/components/side-nav/index.hbs
(4 hunks)app/components/side-nav/index.scss
(7 hunks)app/components/side-nav/index.ts
(1 hunks)app/components/side-nav/product-switcher/index.hbs
(1 hunks)app/components/side-nav/product-switcher/index.scss
(1 hunks)app/components/side-nav/product-switcher/index.ts
(1 hunks)app/components/side-nav/security-menu-item/index.ts
(1 hunks)app/components/storeknox-wrapper/index.hbs
(1 hunks)app/components/storeknox-wrapper/index.scss
(1 hunks)app/components/storeknox-wrapper/index.ts
(1 hunks)app/components/top-nav/index.hbs
(1 hunks)app/components/top-nav/index.scss
(1 hunks)app/components/top-nav/index.ts
(4 hunks)app/router.ts
(3 hunks)app/routes/authenticated/dashboard.ts
(1 hunks)app/routes/authenticated/home.ts
(1 hunks)app/routes/authenticated/index.ts
(1 hunks)app/routes/authenticated/partner.ts
(2 hunks)app/routes/authenticated/storeknox.ts
(1 hunks)app/services/user-auth.ts
(1 hunks)app/styles/_component-variables.scss
(3 hunks)app/styles/_icons.scss
(1 hunks)app/styles/_theme.scss
(1 hunks)app/templates/authenticated.hbs
(1 hunks)app/templates/authenticated/dashboard.hbs
(1 hunks)app/templates/authenticated/home.hbs
(1 hunks)app/templates/authenticated/partner.hbs
(1 hunks)app/templates/authenticated/security.hbs
(1 hunks)app/templates/authenticated/storeknox.hbs
(1 hunks)app/templates/authenticated/storeknox/inventory.hbs
(1 hunks)app/utils/scroll-to-top.ts
(2 hunks)tests/acceptance/home-page-test.js
(1 hunks)tests/acceptance/side-nav-test.js
(1 hunks)tests/integration/components/appknox-wrapper-test.js
(1 hunks)tests/integration/components/appknox-wrapper/onboarding-guide-test.js
(1 hunks)tests/integration/components/home-page/organization-dashboard/header-test.js
(0 hunks)tests/integration/components/home-page/organization-dashboard/onboarding-guide-test.js
(0 hunks)tests/integration/components/home-page/organization-dashboard/side-nav-test.js
(0 hunks)tests/integration/components/home-page/security-dashboard-nav-test.js
(0 hunks)tests/integration/components/security-wrapper-test.js
(1 hunks)tests/integration/components/storeknox-wrapper-test.js
(1 hunks)tests/integration/components/top-nav-test.js
(1 hunks)translations/en.json
(10 hunks)translations/ja.json
(10 hunks)types/ak-svg.d.ts
(1 hunks)
💤 Files with no reviewable changes (13)
- app/components/home-page/index.js
- app/components/home-page/organization-dashboard/header/index.hbs
- app/components/home-page/organization-dashboard/header/index.scss
- app/components/home-page/organization-dashboard/index.hbs
- app/components/home-page/organization-dashboard/index.ts
- app/components/home-page/organization-dashboard/side-nav/security-menu-item/index.ts
- app/components/home-page/security-dashboard-nav/index.hbs
- app/components/home-page/security-dashboard-nav/index.scss
- app/components/home-page/security-dashboard-nav/index.ts
- tests/integration/components/home-page/organization-dashboard/header-test.js
- tests/integration/components/home-page/organization-dashboard/onboarding-guide-test.js
- tests/integration/components/home-page/organization-dashboard/side-nav-test.js
- tests/integration/components/home-page/security-dashboard-nav-test.js
🚧 Files skipped from review as they are similar to previous changes (55)
- app/components/ak-popover/index.hbs
- app/components/ak-popover/index.ts
- app/components/ak-svg/sm-indicator.hbs
- app/components/ak-svg/vp-indicator.hbs
- app/components/ak-tabs/item/index.scss
- app/components/appknox-wrapper/index.hbs
- app/components/appknox-wrapper/index.scss
- app/components/appknox-wrapper/onboarding-guide/index.scss
- app/components/appknox-wrapper/onboarding-guide/index.ts
- app/components/file-compare/index.ts
- app/components/file-compare/table/index.ts
- app/components/file-compare/vulnerability-details/index.ts
- app/components/home-page/index.scss
- app/components/home-page/index.ts
- app/components/home-page/product-card/index.scss
- app/components/home-page/product-card/index.ts
- app/components/partner/template.hbs
- app/components/security-wrapper/index.hbs
- app/components/security-wrapper/index.scss
- app/components/security-wrapper/index.ts
- app/components/security/nav-menu/index.hbs
- app/components/security/nav-menu/index.scss
- app/components/side-nav/index.hbs
- app/components/side-nav/product-switcher/index.hbs
- app/components/side-nav/product-switcher/index.scss
- app/components/side-nav/product-switcher/index.ts
- app/components/side-nav/security-menu-item/index.ts
- app/components/storeknox-wrapper/index.hbs
- app/components/storeknox-wrapper/index.scss
- app/components/top-nav/index.hbs
- app/components/top-nav/index.scss
- app/components/top-nav/index.ts
- app/router.ts
- app/routes/authenticated/dashboard.ts
- app/routes/authenticated/home.ts
- app/routes/authenticated/index.ts
- app/routes/authenticated/partner.ts
- app/routes/authenticated/storeknox.ts
- app/services/user-auth.ts
- app/styles/_icons.scss
- app/styles/_theme.scss
- app/templates/authenticated.hbs
- app/templates/authenticated/dashboard.hbs
- app/templates/authenticated/home.hbs
- app/templates/authenticated/partner.hbs
- app/templates/authenticated/security.hbs
- app/templates/authenticated/storeknox.hbs
- app/templates/authenticated/storeknox/inventory.hbs
- app/utils/scroll-to-top.ts
- tests/acceptance/home-page-test.js
- tests/acceptance/side-nav-test.js
- tests/integration/components/appknox-wrapper-test.js
- tests/integration/components/appknox-wrapper/onboarding-guide-test.js
- tests/integration/components/storeknox-wrapper-test.js
- types/ak-svg.d.ts
🔇 Additional comments (40)
app/components/home-page/product-card/index.hbs (4)
1-5
: LGTM! Well-structured container setup.
The root container is properly structured with appropriate test selectors and styling classes.
6-8
: LGTM! Good use of dynamic component rendering.
The implementation follows best practices by accepting the background image as a component argument, making it flexible and reusable.
10-30
: 🛠️ Refactor suggestion
Enhance accessibility for better screen reader support.
While the structure and styling are well-implemented, consider adding ARIA attributes for better accessibility:
- <AkStack @direction='column' @spacing='1'>
+ <AkStack @direction='column' @spacing='1' role="article">
{{#let (component @indicatorSvg) as |SvgIcon|}}
<SvgIcon
data-test-home-page-product-card-indicator-icon
local-class='product-icon'
+ role="img"
+ aria-label={{@name}}
/>
{{/let}}
<AkTypography
data-test-home-page-product-card-title
@variant='h6'
@fontWeight='bold'
+ role="heading"
+ aria-level="6"
>
Likely invalid or redundant comment.
32-43
: 🛠️ Refactor suggestion
Verify route binding and enhance icon accessibility.
The link section needs two improvements:
- Ensure
this.route
is properly defined in the component's class - Add accessibility attributes to the icon
<AkIcon
@iconName='north-east'
@size='small'
@color='primary'
+ role="img"
+ aria-label="Navigate to {{@name}}"
/>
✅ Verification successful
Route binding is correct, but icon accessibility needs improvement
The route property is properly implemented:
- It's defined in the component's interface as a required string argument
- There's a getter that correctly returns the route from component args
- The template usage matches the implementation
The icon still needs accessibility improvements as suggested:
<AkIcon
@iconName='north-east'
@size='small'
@color='primary'
+ role="img"
+ aria-label="Navigate to {{@name}}"
/>
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify route property definition in component class
# Search for route property definition
rg "route\s*=|route:" app/components/home-page/product-card/
Length of output: 206
Script:
#!/bin/bash
# Check the component class implementation to verify route property initialization
ast-grep --pattern 'class $_Component {
$$$
route$_
$$$
}'
# Also check for any route property assignments
rg "route\s*=" -A 3 -B 3 app/components/home-page/product-card/index.ts
Length of output: 140
Script:
#!/bin/bash
# Let's try a different approach to check the component implementation
cat app/components/home-page/product-card/index.ts
# Also check for any route-related imports or decorators
rg "@route|import.*route" app/components/home-page/product-card/index.ts
Length of output: 968
app/components/home-page/index.hbs (3)
1-69
: Well-structured template with proper component hierarchy
The template follows a clean and maintainable structure with:
- Consistent use of AkStack for layout
- Proper data test attributes for testing
- Responsive design with flex wrapping
- Clear component hierarchy
13-13
: Verify AuthAssets component implementation
The template references the AuthAssets
component. Let's verify its implementation and any required properties.
#!/bin/bash
# Search for AuthAssets component definition
ast-grep --pattern 'class AuthAssets extends Component {
$$$
}'
# Search for AuthAssets template
fd "auth-assets" --type f
58-65
: Verify ProductCard component properties
Let's verify the implementation of the ProductCard component and its required properties.
✅ Verification successful
Let me try another search pattern to find the ProductCard component.
Let me check the template file for the ProductCard component to complete the verification.
All ProductCard component properties are correctly implemented and used
The verification confirms that:
- All required properties (@name, @description, @route, @linkText, @coverBackgroundImage, @indicatorSvg) match the component's interface definition
- The 'takeMeToDashboard' translation key exists in both en.json and ja.json
- The template properly renders all passed properties with correct types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ProductCard component definition and its properties
ast-grep --pattern 'class ProductCard extends Component {
$$$
}'
# Search for the takeMeToDashboard translation key
fd -e json -e yaml -e yml | xargs rg '"takeMeToDashboard"'
# Search for ProductCard template
fd "product-card" --type f
Length of output: 465
Script:
#!/bin/bash
# Search for ProductCard component with a more specific pattern
ast-grep --pattern 'class ProductCard {
$$$
}'
# Also try finding the template file with a different pattern
fd -g "*product-card*.hbs"
# Search for any file containing ProductCard
rg "ProductCard" -A 5
Length of output: 1701
Script:
#!/bin/bash
# Get the content of the ProductCard template
cat app/components/home-page/product-card/index.hbs
# Get the full interface definition
rg -A 15 "interface ProductCardComponentSignature"
Length of output: 2473
app/components/storeknox-wrapper/index.ts (6)
1-18
: LGTM! Well-structured imports and interface definition.
The imports are properly typed and the component interface is well-defined with appropriate type constraints.
27-37
: Duplicate feedback: Extract localStorage key to a constant.
56-58
: LGTM! Clear and focused enterprise status check.
The getter provides a clean abstraction for checking enterprise status.
60-68
: LGTM! Clean toggle implementation.
The action properly manages state updates and persistence.
71-75
: LGTM! Proper Glint type registration.
The component is correctly registered in the Glint registry for proper type checking.
20-24
: Verify the window service injection path.
The window service is injected using a custom path 'browser/window'. Let's verify this service exists and is registered correctly.
app/components/side-nav/index.scss (5)
7-10
: LGTM: Variable renaming is consistent
The updated variable names follow the new naming convention while maintaining the same functionality.
31-31
: Referencing previous review comment about logical properties
62-65
: Referencing previous review comment about padding value
149-150
: Referencing previous review comment about !important declarations
178-178
: Referencing previous review comment about !important declarations
app/components/side-nav/index.ts (4)
1-65
: Well-structured type definitions and interfaces!
The type definitions are comprehensive and follow TypeScript best practices. The interfaces are well-documented with proper optional properties and type safety (e.g., union types for svg property in SwitcherMenuItem).
75-83
: Improve image instance handling
The current image handling could be improved as per the previous review comment.
126-130
: LGTM: Proper Glint type registration
The Glint registry is correctly implemented, providing proper type safety for the component in templates.
67-74
: Verify window service mock in tests
The @service('browser/window')
injection might need special handling in tests. Ensure that the window service is properly mocked in your test environment to avoid any testing issues.
✅ Verification successful
The search results show extensive usage and proper mocking of the browser/window
service across various test files. Let me verify if there are any specific tests for the SideNavComponent
that properly handle the window service.
Let me try a different search pattern to locate the side nav tests.
Based on the search results, I can now generate the final response:
Window service is properly mocked in tests
The window service is well-handled in the test environment:
- The side-nav test uses
setupBrowserFakes
from 'ember-browser-services/test-support' - The window service is properly looked up in the test:
this.owner.lookup('service:browser/window')
- Multiple test files across the codebase demonstrate consistent mocking patterns for the window service
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for window service mocks in test files
rg -t js -t ts "browser/window|windowService" tests/
Length of output: 3884
Script:
#!/bin/bash
# Search for SideNav component tests and their window service handling
rg -t js -t ts -A 5 "SideNavComponent|module\('.*side-nav.*'" tests/
Length of output: 76
Script:
#!/bin/bash
# Search for side nav related test files
fd -t f "side-nav.*test" tests/
# If found, show the content of the test file
rg -t js -t ts "describe|test|it" tests/acceptance/side-nav-test.js
Length of output: 1832
tests/integration/components/top-nav-test.js (3)
22-25
:
Add missing logUserOutOfSupportWidget method to FreshdeskStub
The logUserOutOfSupportWidget
method is used in the tests (line 135) but not defined in the stub class.
class FreshdeskStub extends Service {
supportWidgetIsEnabled = false;
freshchatEnabled = false;
+ logUserOutOfSupportWidget() {
+ // Track if method was called for assertions
+ this.logoutCalled = true;
+ }
}
Likely invalid or redundant comment.
83-101
: 🛠️ Refactor suggestion
Add accessibility testing to main navigation test
The test verifies visual elements but lacks accessibility checks.
test('it renders main top-nav with title', async function (assert) {
await render(hbs`
<TopNav
@user={{this.user}}
@title={{this.title}}
/>
`);
assert.dom('[data-test-topNav]').exists();
+ assert.dom('[data-test-topNav]').hasAttribute('role', 'navigation');
+ assert.dom('[data-test-topNav]').hasAttribute('aria-label', t('main_navigation'));
assert.dom('[data-test-topNav-title]').hasText(this.title);
+ assert.dom('[data-test-topNav-title]').hasAttribute('aria-label', this.title);
assert.dom('[data-test-bell-icon]').isNotDisabled();
+ assert.dom('[data-test-bell-icon]').hasAttribute('aria-label', t('notifications'));
+ assert.dom('[data-test-bell-icon]').hasAttribute('role', 'button');
assert
.dom('[data-test-topNav-profileBtn]')
.isNotDisabled()
- .hasText(this.user.username);
+ .hasText(this.user.username)
+ .hasAttribute('aria-haspopup', 'true')
+ .hasAttribute('aria-expanded', 'false');
});
Likely invalid or redundant comment.
122-160
:
Replace test.each with standard QUnit approach
QUnit doesn't support test.each
. Use a standard approach for parameterized testing.
- test.each(
- 'test profile btn menu items',
- [{ chatSupport: true }, { chatSupport: false }],
- async function (assert, { chatSupport }) {
+ const testCases = [
+ { chatSupport: true },
+ { chatSupport: false }
+ ];
+
+ testCases.forEach(({ chatSupport }) => {
+ test(`test profile btn menu items with chatSupport=${chatSupport}`, async function(assert) {
// ... rest of the test code ...
});
+ });
Likely invalid or redundant comment.
translations/ja.json (7)
80-81
: Skip comment as issue already reported
The missing Japanese translations for "appknox" and "appknoxDesc" were already reported in past reviews.
83-83
: Skip comment as issue already reported
The missing Japanese translation for "appSwitcher" was already reported in past reviews.
1505-1506
: Skip comment as issue already reported
The missing Japanese translations for "storeknox" and "storeknoxDesc" were already reported in past reviews.
1521-1521
: Skip comment as issue already reported
The missing Japanese translation for "switchTo" was already reported in past reviews.
1529-1529
: Skip comment as issue already reported
The missing Japanese translation for "takeMeToDashboard" was already reported in past reviews.
1562-1562
: Skip comment as issue already reported
The missing Japanese translation for "toSecureYourMobileApps" was already reported in past reviews.
1625-1625
: Skip comment as issue already reported
The missing Japanese translation for "vapt" was already reported in past reviews.
app/styles/_component-variables.scss (4)
329-342
: Skip comment about dark theme color choices.
A previous review already identified potential contrast issues with the dark theme configuration.
346-373
: Well-structured side navigation variables!
The implementation follows best practices:
- Consistent naming conventions
- Proper theme handling
- Semantic color variables
- Comprehensive styling coverage
376-382
: Clean implementation of product switcher variables!
The variables are well-organized and follow the established patterns for modal and interactive elements.
1492-1505
: Skip comment about text color in dark theme.
A previous review already addressed the use of secondary-contrast-text in dark theme.
translations/en.json (4)
83-83
: LGTM: App switcher translation key added.
The key for app switcher functionality is properly defined.
366-366
: Navigation menu items follow proper capitalization.
The navigation items "discovery", "home", and "inventory" are properly capitalized in their translations.
Also applies to: 593-593, 648-648
1521-1521
: LGTM: Landing page navigation strings added.
The strings for navigation ("switchTo", "takeMeToDashboard") and product value proposition ("toSecureYourMobileApps") are properly defined.
Also applies to: 1529-1529, 1562-1562
80-81
: 🛠️ Refactor suggestion
Maintain consistent product branding.
The product name "Appknox" and its description follow good practices, but for consistency with the brand identity, consider:
- "appknox": "Appknox",
- "appknoxDesc": "Assess your mobile applications for vulnerabilities",
+ "appknox": "AppKnox",
+ "appknoxDesc": "Assess your mobile applications for vulnerabilities"
Likely invalid or redundant comment.
fe9cbeb
to
f68f87b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (17)
app/components/home-page/product-card/index.hbs (1)
1-8
: Enhance container accessibilityAdd appropriate ARIA attributes to improve accessibility for screen readers.
<AkStack data-test-home-page-product-card @direction='column' local-class='product-card-container' + role="article" + aria-label={{@name}} >app/components/home-page/index.hbs (1)
1-69
: Overall structure looks clean and well-organizedThe new layout implementation follows good practices with:
- Proper component hierarchy
- Consistent use of data test attributes
- Clean separation of sections
Consider breaking down the template into smaller sub-components if this component grows larger, to maintain maintainability.
app/components/side-nav/product-switcher/index.ts (1)
43-68
: Consider optimizing menu items definition and label handlingThe implementation correctly filters out the current product as suggested. However, there are opportunities for optimization:
- Move the menu items array outside the getter to avoid recreation on each access
- Simplify the enterprise/non-enterprise label logic using a mapping
Here's a suggested optimization:
// Define outside the class const PRODUCT_LABELS = { enterprise: { appknox: 'vapt', storeknox: 'appMonitoring' }, standard: { appknox: 'appknox', storeknox: 'storeknox' } } as const; const MENU_ITEMS_BASE = [ { id: 'vp-svg', svg: 'ak-svg/vp-indicator', route: 'authenticated.dashboard.projects', key: 'appknox' }, { id: 'sm-svg', svg: 'ak-svg/sm-indicator', route: 'authenticated.storeknox.discover', key: 'storeknox' } ] as const; // In the component get switcherMenuItems() { const labelType = this.orgIsAnEnterprise ? 'enterprise' : 'standard'; return MENU_ITEMS_BASE .filter((item) => item.key !== this.args.productSwitcherFilterKey) .map((item) => ({ ...item, label: this.intl.t(PRODUCT_LABELS[labelType][item.key as keyof typeof PRODUCT_LABELS.standard]) })); }tests/integration/components/security-wrapper-test.js (1)
89-104
: Improve assertion organization.Group related assertions and add expect count for better test organization:
test('it renders main top-nav with logo', async function (assert) { + assert.expect(7); + await render(hbs` <SecurityWrapper @user={{this.user}} /> `); + // Test top navigation presence assert.dom('[data-test-topNav]').exists(); assert.dom('[data-test-topNav-logo]').exists(); assert.dom('[data-test-topNav-title]').doesNotExist(); + // Test navigation menu assert.dom('[data-test-security-nav-menu]').exists(); assert.dom('[data-test-side-nav]').doesNotExist(); + // Test notification elements assert.dom('[data-test-bell-icon]').doesNotExist(); + // Test profile button assert .dom('[data-test-topNav-profileBtn]') .isNotDisabled() .hasText(this.user.username); });app/components/side-nav/index.scss (1)
153-156
: Consider using CSS custom properties for SVG colorsInstead of using !important for SVG path colors, consider using currentColor or CSS custom properties for better maintainability:
- .menu-item-icon-svg { - path { - stroke: var(--side-nav-inherit-background-text-color) !important; - } - } + .menu-item-icon-svg { + color: var(--side-nav-inherit-background-text-color); + path { + stroke: currentColor; + } + }app/components/side-nav/index.ts (2)
1-65
: LGTM! Consider adding JSDoc comments for better documentation.The type definitions and interfaces are well-structured and provide good type safety. Consider adding JSDoc comments to document the purpose and usage of each interface, especially for public exports.
Example documentation:
/** * Interface for menu items in the side navigation. * @interface MenuItem * @property {string} label - The display text for the menu item * @property {string} [icon] - Optional icon identifier * ... */ export interface MenuItem { // ... existing properties }
67-74
: Consider adding service documentation and initialization checks.The services are correctly injected, but it would be beneficial to:
- Document the purpose of each service
- Add initialization checks in the constructor to ensure required services are available
/** * @service * Handles user-related operations and state */ @service declare me: MeService; constructor(owner: unknown, args: SideNavSignature['Args']) { super(owner, args); // Ensure required services are available if (!this.organization || !this.whitelabel) { throw new Error('Required services not initialized'); } // ... rest of the constructor }tests/integration/components/top-nav-test.js (2)
83-101
: Add accessibility testing to rendering testThe test verifies visual elements but lacks accessibility checks.
Add assertions for ARIA attributes:
test('it renders main top-nav with title', async function (assert) { await render(hbs`<TopNav @user={{this.user}} @title={{this.title}} />`); assert.dom('[data-test-topNav]').exists(); + assert.dom('[data-test-topNav]').hasAttribute('role', 'navigation'); assert.dom('[data-test-topNav-title]').hasText(this.title); + assert.dom('[data-test-topNav-title]').hasAttribute('aria-label', this.title); assert.dom('[data-test-bell-icon]').isNotDisabled(); + assert.dom('[data-test-bell-icon]') + .hasAttribute('aria-label', t('notifications')) + .hasAttribute('role', 'button'); assert .dom('[data-test-topNav-profileBtn]') .isNotDisabled() - .hasText(this.user.username); + .hasText(this.user.username) + .hasAttribute('aria-haspopup', 'true') + .hasAttribute('aria-expanded', 'false'); });
122-160
: Add error case tests for menu interactionsThe profile menu tests only cover the happy path. Consider adding error scenarios.
Add test cases for:
- Support widget failure
- Menu keyboard navigation
- Session handling
Example test:
test('handles support widget failure', async function (assert) { const freshdesk = this.owner.lookup('service:freshdesk'); freshdesk.freshchatEnabled = true; freshdesk.logUserOutOfSupportWidget = () => { throw new Error('Widget failed'); }; await render(hbs`<TopNav @user={{this.user}} />`); await click('[data-test-topNav-profileBtn]'); await click('[data-test-support-button]'); assert.dom('[data-test-error-message]').exists(); assert.strictEqual( this.owner.lookup('service:notifications').errorMsg, t('errors.support_widget_failed') ); });app/router.ts (1)
271-282
: LGTM! Route structure follows established patterns.The Storeknox routes are well-organized with a logical hierarchy and follow the dashboard path prefix pattern.
Consider adding JSDoc comments to document the purpose of each route, similar to other major route sections. This would help developers understand the navigation flow and feature relationships. Example:
/** * Storeknox routes - Handles store monitoring and inventory management * - /dashboard/storeknox: Main entry point * - /dashboard/storeknox/discover: Store discovery and analysis * - /dashboard/storeknox/inventory: Store inventory management */app/components/appknox-wrapper/index.ts (2)
191-195
: Improve type safety in menuItems getterThe security menu item's component property is cast as a const assertion, but the filter operation might affect type inference. Consider using a type guard or explicit typing.
- ].filter(Boolean) as MenuItem[]; + ].filter((item): item is MenuItem => Boolean(item));
Line range hint
232-246
: Improve type safety and error handling in showGuideThe function accesses
window.pendo
without type checking, and the error handling could be more informative.@action async showGuide() { if (this.enablePendo) { try { + // Type check for pendo + if (!this.window.pendo?.getActiveGuides) { + throw new Error('Pendo not initialized'); + } + const guide = this.window.pendo .getActiveGuides() .find(function (element) { return element.launchMethod === 'auto-badge'; }); await guide?.show(); } catch (e) { - console.error(e); + this.notifications.error( + this.intl.t('errors.pendo_guide_failed') + ); } } }tests/integration/components/appknox-wrapper-test.js (5)
13-23
: Add parameter validation to NotificationsStub methodsThe error and success methods should validate their message parameters to prevent potential issues with undefined or non-string messages.
class NotificationsStub extends Service { errorMsg = null; successMsg = null; error(msg) { + if (typeof msg !== 'string') { + throw new TypeError('Message must be a string'); + } this.errorMsg = msg; } success(msg) { + if (typeof msg !== 'string') { + throw new TypeError('Message must be a string'); + } this.successMsg = msg; } }
25-28
: Make FreshdeskStub properties configurableThe hardcoded boolean values make the stub less flexible for different test scenarios.
class FreshdeskStub extends Service { - supportWidgetIsEnabled = false; - freshchatEnabled = true; + supportWidgetIsEnabled; + freshchatEnabled; + + constructor() { + super(...arguments); + this.supportWidgetIsEnabled = false; + this.freshchatEnabled = true; + } }
95-110
: Make lowerMenuItems more configurableThe hardcoded
enabled
values limit test flexibility. Consider making these configurable through parameters.-const lowerMenuItems = [ +const lowerMenuItems = ({ chatEnabled = false, versionEnabled = false } = {}) => [ { title: () => t('chatSupport'), icon: 'chat-bubble', - enabled: false, + enabled: chatEnabled, }, { title: () => t('version'), icon: 'info', - enabled: false, + enabled: versionEnabled, }, { title: () => t('collapse'), icon: 'keyboard-tab', }, ];
152-154
: Add error handling to submissions API mockThe current API mock doesn't handle potential error scenarios. Consider adding error cases for better test coverage.
- // handle submissions api for each test - this.server.get('/submissions', () => []); + // handle submissions api for each test with error scenarios + this.server.get('/submissions', (schema, request) => { + if (!request.queryParams) { + return new Response(400, {}, { errors: ['Invalid request'] }); + } + return []; + });
214-240
: Enhance test coverage for knowledge base functionalityThe knowledge base test should include error scenarios and verify the behavior when the support widget fails to open.
test('test knowledge base click', async function (assert) { - assert.expect(5); + assert.expect(7); const freshdesk = this.owner.lookup('service:freshdesk'); freshdesk.supportWidgetIsEnabled = true; + // Test success scenario freshdesk.openSupportWidget = function () { assert.ok('Knowledge base clicked'); assert.strictEqual(arguments.length, 0); }; await render(hbs` <AppknoxWrapper @user={{this.user}} /> `); assert.dom('[data-test-topNav]').exists(); assert .dom('[data-test-topNav-KnowledgeBaseBtn]') .isNotDisabled() .hasText(t('knowledgeBase')); await click('[data-test-topNav-KnowledgeBaseBtn]'); + + // Test error scenario + freshdesk.openSupportWidget = function () { + throw new Error('Failed to open support widget'); + }; + + await click('[data-test-topNav-KnowledgeBaseBtn]'); + + const notifications = this.owner.lookup('service:notifications'); + assert.strictEqual( + notifications.errorMsg, + 'Failed to open knowledge base' + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.json
is excluded by!**/package-lock.json
📒 Files selected for processing (79)
app/components/ak-popover/index.hbs
(1 hunks)app/components/ak-popover/index.ts
(1 hunks)app/components/ak-svg/sm-indicator.hbs
(1 hunks)app/components/ak-svg/vp-indicator.hbs
(1 hunks)app/components/ak-tabs/item/index.scss
(1 hunks)app/components/appknox-wrapper/index.hbs
(1 hunks)app/components/appknox-wrapper/index.scss
(2 hunks)app/components/appknox-wrapper/index.ts
(4 hunks)app/components/appknox-wrapper/onboarding-guide/index.scss
(1 hunks)app/components/appknox-wrapper/onboarding-guide/index.ts
(3 hunks)app/components/file-compare/index.ts
(2 hunks)app/components/file-compare/table/index.ts
(3 hunks)app/components/file-compare/vulnerability-details/index.ts
(2 hunks)app/components/home-page/index.hbs
(1 hunks)app/components/home-page/index.js
(0 hunks)app/components/home-page/index.scss
(1 hunks)app/components/home-page/index.ts
(1 hunks)app/components/home-page/organization-dashboard/header/index.hbs
(0 hunks)app/components/home-page/organization-dashboard/header/index.scss
(0 hunks)app/components/home-page/organization-dashboard/index.hbs
(0 hunks)app/components/home-page/organization-dashboard/index.ts
(0 hunks)app/components/home-page/organization-dashboard/side-nav/security-menu-item/index.ts
(0 hunks)app/components/home-page/product-card/index.hbs
(1 hunks)app/components/home-page/product-card/index.scss
(1 hunks)app/components/home-page/product-card/index.ts
(1 hunks)app/components/home-page/security-dashboard-nav/index.hbs
(0 hunks)app/components/home-page/security-dashboard-nav/index.scss
(0 hunks)app/components/home-page/security-dashboard-nav/index.ts
(0 hunks)app/components/partner/template.hbs
(1 hunks)app/components/security-wrapper/index.hbs
(1 hunks)app/components/security-wrapper/index.scss
(1 hunks)app/components/security-wrapper/index.ts
(1 hunks)app/components/security/nav-menu/index.hbs
(1 hunks)app/components/security/nav-menu/index.scss
(1 hunks)app/components/side-nav/index.hbs
(4 hunks)app/components/side-nav/index.scss
(7 hunks)app/components/side-nav/index.ts
(1 hunks)app/components/side-nav/product-switcher/index.hbs
(1 hunks)app/components/side-nav/product-switcher/index.scss
(1 hunks)app/components/side-nav/product-switcher/index.ts
(1 hunks)app/components/side-nav/security-menu-item/index.ts
(1 hunks)app/components/storeknox-wrapper/index.hbs
(1 hunks)app/components/storeknox-wrapper/index.scss
(1 hunks)app/components/storeknox-wrapper/index.ts
(1 hunks)app/components/top-nav/index.hbs
(1 hunks)app/components/top-nav/index.scss
(1 hunks)app/components/top-nav/index.ts
(4 hunks)app/router.ts
(3 hunks)app/routes/authenticated/dashboard.ts
(1 hunks)app/routes/authenticated/home.ts
(1 hunks)app/routes/authenticated/index.ts
(1 hunks)app/routes/authenticated/partner.ts
(2 hunks)app/routes/authenticated/storeknox.ts
(1 hunks)app/services/user-auth.ts
(1 hunks)app/styles/_component-variables.scss
(3 hunks)app/styles/_icons.scss
(1 hunks)app/styles/_theme.scss
(1 hunks)app/templates/authenticated.hbs
(1 hunks)app/templates/authenticated/dashboard.hbs
(1 hunks)app/templates/authenticated/home.hbs
(1 hunks)app/templates/authenticated/partner.hbs
(1 hunks)app/templates/authenticated/security.hbs
(1 hunks)app/templates/authenticated/storeknox.hbs
(1 hunks)app/templates/authenticated/storeknox/inventory.hbs
(1 hunks)app/utils/scroll-to-top.ts
(2 hunks)tests/acceptance/home-page-test.js
(1 hunks)tests/acceptance/side-nav-test.js
(1 hunks)tests/integration/components/appknox-wrapper-test.js
(1 hunks)tests/integration/components/appknox-wrapper/onboarding-guide-test.js
(1 hunks)tests/integration/components/home-page/organization-dashboard/header-test.js
(0 hunks)tests/integration/components/home-page/organization-dashboard/onboarding-guide-test.js
(0 hunks)tests/integration/components/home-page/organization-dashboard/side-nav-test.js
(0 hunks)tests/integration/components/home-page/security-dashboard-nav-test.js
(0 hunks)tests/integration/components/security-wrapper-test.js
(1 hunks)tests/integration/components/storeknox-wrapper-test.js
(1 hunks)tests/integration/components/top-nav-test.js
(1 hunks)translations/en.json
(10 hunks)translations/ja.json
(10 hunks)types/ak-svg.d.ts
(1 hunks)
💤 Files with no reviewable changes (13)
- app/components/home-page/index.js
- app/components/home-page/organization-dashboard/header/index.hbs
- app/components/home-page/organization-dashboard/header/index.scss
- app/components/home-page/organization-dashboard/index.hbs
- app/components/home-page/organization-dashboard/index.ts
- app/components/home-page/organization-dashboard/side-nav/security-menu-item/index.ts
- app/components/home-page/security-dashboard-nav/index.hbs
- app/components/home-page/security-dashboard-nav/index.scss
- app/components/home-page/security-dashboard-nav/index.ts
- tests/integration/components/home-page/organization-dashboard/header-test.js
- tests/integration/components/home-page/organization-dashboard/onboarding-guide-test.js
- tests/integration/components/home-page/organization-dashboard/side-nav-test.js
- tests/integration/components/home-page/security-dashboard-nav-test.js
🚧 Files skipped from review as they are similar to previous changes (54)
- app/components/ak-popover/index.hbs
- app/components/ak-popover/index.ts
- app/components/ak-svg/sm-indicator.hbs
- app/components/ak-svg/vp-indicator.hbs
- app/components/ak-tabs/item/index.scss
- app/components/appknox-wrapper/index.hbs
- app/components/appknox-wrapper/index.scss
- app/components/appknox-wrapper/onboarding-guide/index.scss
- app/components/appknox-wrapper/onboarding-guide/index.ts
- app/components/file-compare/index.ts
- app/components/file-compare/table/index.ts
- app/components/file-compare/vulnerability-details/index.ts
- app/components/home-page/index.scss
- app/components/home-page/index.ts
- app/components/home-page/product-card/index.scss
- app/components/home-page/product-card/index.ts
- app/components/partner/template.hbs
- app/components/security-wrapper/index.hbs
- app/components/security-wrapper/index.scss
- app/components/security-wrapper/index.ts
- app/components/security/nav-menu/index.hbs
- app/components/security/nav-menu/index.scss
- app/components/side-nav/index.hbs
- app/components/side-nav/product-switcher/index.hbs
- app/components/side-nav/product-switcher/index.scss
- app/components/side-nav/security-menu-item/index.ts
- app/components/storeknox-wrapper/index.hbs
- app/components/storeknox-wrapper/index.scss
- app/components/storeknox-wrapper/index.ts
- app/components/top-nav/index.hbs
- app/components/top-nav/index.scss
- app/components/top-nav/index.ts
- app/routes/authenticated/dashboard.ts
- app/routes/authenticated/home.ts
- app/routes/authenticated/index.ts
- app/routes/authenticated/partner.ts
- app/routes/authenticated/storeknox.ts
- app/services/user-auth.ts
- app/styles/_component-variables.scss
- app/styles/_icons.scss
- app/styles/_theme.scss
- app/templates/authenticated.hbs
- app/templates/authenticated/dashboard.hbs
- app/templates/authenticated/home.hbs
- app/templates/authenticated/partner.hbs
- app/templates/authenticated/security.hbs
- app/templates/authenticated/storeknox.hbs
- app/templates/authenticated/storeknox/inventory.hbs
- app/utils/scroll-to-top.ts
- tests/acceptance/home-page-test.js
- tests/acceptance/side-nav-test.js
- tests/integration/components/appknox-wrapper/onboarding-guide-test.js
- tests/integration/components/storeknox-wrapper-test.js
- types/ak-svg.d.ts
🔇 Additional comments (26)
app/components/home-page/product-card/index.hbs (2)
12-17
: LGTM: Icon implementation follows best practices
The implementation correctly uses:
- Dynamic component rendering
- Suggested
product-icon
class - Data test selector for testing
33-40
: 🛠️ Refactor suggestion
Verify route binding and enhance link accessibility
The link implementation needs attention in two areas:
- Ensure
this.route
is properly defined in the component's class - Add appropriate ARIA attributes for better accessibility
Let's verify the route implementation:
<AkLink
data-test-home-page-product-card-link
@route={{this.route}}
@underline='always'
@fontWeight='medium'
+ aria-label="Learn more about {{@name}}"
>
✅ Verification successful
Route binding is correctly implemented, accessibility can be improved
The route property is properly defined and used:
- It's correctly typed in the component's signature
- Implemented as a getter that returns
args.route
- Properly passed from parent component via
productCardDetails.link
Consider the accessibility enhancement:
<AkLink
data-test-home-page-product-card-link
@route={{this.route}}
@underline='always'
@fontWeight='medium'
+ aria-label="Learn more about {{@name}}"
>
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify route property definition and usage
# Check component's class file
fd "product-card/index" --type f --extension ts,js
# Look for route property definition
rg "route\s*=|route:" --type ts --type js app/components/home-page/product-card/
Length of output: 199
Script:
#!/bin/bash
# Let's verify how the route property is used and set in the component
# Check the full implementation of the component class
rg -A 10 -B 10 "class" app/components/home-page/product-card/index.ts
# Check if there are any tests that verify the route property
fd "product-card-test" --type f
rg "route" --type test
Length of output: 718
Script:
#!/bin/bash
# Let's check for tests with proper file extensions
fd "product-card" --type f --extension test.ts,test.js
# Check template usage of the component
rg -A 5 -B 5 "ProductCard" app/templates/ app/components/
Length of output: 2961
app/components/home-page/index.hbs (4)
16-34
: LGTM! Clean and well-structured header section
The header section uses proper typography variants and maintains a clean layout structure.
Note: The translation key issue has been previously identified and is still applicable.
36-50
: LGTM with previously noted concerns
The logout button implementation follows component guidelines but retains the previously identified error handling concerns.
Note: The error handling improvement suggestion from the previous review is still applicable.
56-67
: Verify type safety of productCardDetails
While the product cards implementation looks clean, let's ensure type safety for the productCardDetails properties.
#!/bin/bash
# Description: Check type definitions for ProductCard component and productCardDetails
# Test: Look for type definitions and interfaces
# Check for ProductCard component type definitions
ast-grep --pattern 'interface ProductCardArgs {
$$$
}'
# Check for productCardDetails type definition
ast-grep --pattern 'productCardDetails: $_ = $$$'
Note: The loading state improvement suggestion from the previous review is still applicable.
1-14
: Verify AuthAssets component implementation
The layout structure looks clean and well-organized. However, let's verify the AuthAssets component implementation.
✅ Verification successful
AuthAssets component implementation is correct and properly used
The AuthAssets component is a simple wrapper that renders the ImgLogo component, which in turn displays a whitelabel-aware logo with proper routing. The component is consistently used across authentication-related pages and the home page for branding purposes. The implementation follows Ember/Glimmer component patterns and is properly typed with Glint support.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check AuthAssets component implementation
# Test: Look for AuthAssets component definition and its usage
ast-grep --pattern 'class AuthAssets extends Component {
$$$
}'
# Check for template file
fd "auth-assets" --type f
Length of output: 142
Script:
#!/bin/bash
# Let's check the actual implementation and template of AuthAssets
rg -l "AuthAssets" --type-add 'hbs:*.hbs' --type hbs
rg -l "AuthAssets" --type js
# Check the test file content to understand the component's purpose
cat tests/integration/components/auth-assets-test.js
Length of output: 1045
Script:
#!/bin/bash
# Let's find the actual AuthAssets component implementation
fd "auth-assets" --type f --exclude "test.js"
# Search for any template file for AuthAssets
fd "auth-assets.hbs"
# Let's check if it's a Glimmer component
rg -g "*.ts" -g "*.js" "class AuthAssets"
# Check if it's an addon component
fd "auth-assets" --type f --full-path --hidden
Length of output: 424
Script:
#!/bin/bash
# Let's check the actual template content and component implementation
cat app/components/auth-assets/index.hbs
cat app/components/auth-assets/index.ts
# Let's also check where this component is used to understand its context
rg -C 2 "<AuthAssets" --type-add 'hbs:*.hbs' --type hbs
Length of output: 3205
Script:
#!/bin/bash
# Let's check the ImgLogo component since AuthAssets uses it
cat app/components/img-logo/index.{ts,hbs} 2>/dev/null || true
rg -l "ImgLogo" --type-add 'hbs:*.hbs' --type hbs --type ts --type js
Length of output: 833
app/components/side-nav/product-switcher/index.ts (4)
1-21
: LGTM! Well-structured TypeScript definitions
The imports and interface definitions are clean, properly typed, and follow TypeScript best practices.
23-28
: LGTM! Clean component structure
The component follows Ember Octane patterns with proper service injections and state tracking.
70-76
: LGTM! Well-implemented actions
The actions are focused, properly typed, and follow Ember's modern patterns.
79-84
: LGTM! Proper Glint type declarations
The registry declarations properly support both kebab-case and nested component invocation styles.
tests/integration/components/security-wrapper-test.js (4)
10-42
: Service stubs need enhancement.
The existing review comment about enhancing service stubs with reset methods, error handling, and configurable flags is still applicable.
63-63
: Add error handling for organization load.
The existing review comment about adding error handling for organization load is still applicable.
78-80
: Enhance submissions API mock.
The existing review comment about improving the submissions API mock to handle different scenarios is still applicable.
82-105
: Expand test coverage.
The existing review comment about adding tests for error states, loading states, and different user scenarios is still applicable.
app/components/side-nav/index.scss (3)
7-10
: LGTM: Variable renaming is consistent and improves maintainability
The transition from --organization-dashboard-side-nav-
to --side-nav-
prefix makes the code more concise while maintaining clarity.
89-89
: LGTM: Consistent variable renaming in menu item styles
The variable renaming maintains consistency with the new --side-nav-
prefix pattern.
Also applies to: 96-96, 105-105
113-114
: LGTM: Consistent updates to text and badge styling variables
The variable renaming maintains consistency across text and badge styling properties.
Also applies to: 120-120, 125-125, 128-128, 138-138, 142-143
app/components/side-nav/index.ts (1)
126-130
: LGTM! Proper Glint type registration.
The Glint registry declaration is correctly implemented, ensuring proper type checking for the component in templates.
tests/integration/components/top-nav-test.js (2)
22-25
:
Add missing logUserOutOfSupportWidget method to FreshdeskStub
The logUserOutOfSupportWidget
method is used in tests (line 135) but not defined in the stub class. This could lead to runtime errors.
Apply this diff to fix the issue:
class FreshdeskStub extends Service {
supportWidgetIsEnabled = false;
freshchatEnabled = false;
+ logUserOutOfSupportWidget() {}
}
Likely invalid or redundant comment.
122-160
:
Replace test.each with QUnit-compatible approach
The test.each
method is not part of QUnit's API and will cause runtime errors.
Replace with a QUnit-compatible approach:
-test.each(
- 'test profile btn menu items',
- [{ chatSupport: true }, { chatSupport: false }],
- async function (assert, { chatSupport }) {
+[{ chatSupport: true }, { chatSupport: false }].forEach(({ chatSupport }) => {
+ test(`test profile btn menu items with chatSupport=${chatSupport}`, async function (assert) {
// ... existing test code ...
});
+});
Likely invalid or redundant comment.
app/router.ts (1)
46-47
: LGTM! Route placement and path are correct.
The home route is properly placed under the authenticated section and follows the established path pattern.
translations/en.json (5)
80-81
: Maintain consistent product name capitalization and description style.
This issue was already identified in a past review comment. The product names and descriptions should maintain consistent capitalization and parallel structure.
Also applies to: 1505-1506
366-366
: Navigation menu items need proper hierarchy.
This issue was already identified in a past review comment. The navigation items should follow proper capitalization and hierarchy.
Also applies to: 648-648
1082-1082
: LGTM: New feature strings are properly formatted.
The new strings for partner features and navigation actions follow consistent formatting and are properly structured.
Also applies to: 1521-1521, 1529-1529
1625-1625
: Expand VAPT acronym for clarity.
This issue was already identified in a past review comment. For consistency with other security-related terms in the codebase, VAPT should be expanded to "Vulnerability Assessment and Penetration Testing (VAPT)".
1562-1562
: LGTM: Product value proposition is clear and consistent.
The string "toSecureYourMobileApps" effectively communicates the product's core value proposition and maintains consistent capitalization.
f68f87b
to
45bf690
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (14)
app/components/home-page/product-card/index.hbs (2)
1-8
: Add accessibility attributes to the cover image.The cover image component should have appropriate ARIA attributes for screen readers.
{{#let (component @coverBackgroundImage) as |Cover|}} - <Cover /> + <Cover + role="img" + aria-label={{concat @name " background"}} + /> {{/let}}
32-43
: Enhance keyboard navigation and focus management.While the link and icon structure is good, consider improving keyboard navigation and focus management.
<AkStack @direction='row' @spacing='1' @alignItems='center'> <AkLink data-test-home-page-product-card-link @route={{this.route}} @underline='always' @fontWeight='medium' + tabindex="0" + {{on "keydown" this.handleKeyDown}} > {{@linkText}} </AkLink> - <AkIcon @iconName='north-east' @size='small' @color='primary' /> + <AkIcon + @iconName='north-east' + @size='small' + @color='primary' + aria-hidden="true" + /> </AkStack>Consider adding keyboard event handling in the component's class:
// in the component's class @action handleKeyDown(event: KeyboardEvent) { if (event.key === 'Enter' || event.key === ' ') { event.preventDefault(); // Trigger the same action as clicking this.navigate(); } }app/components/home-page/index.hbs (1)
13-13
: Add error handling for AuthAssets component.Consider adding error boundaries and loading states for the AuthAssets component to handle potential authentication-related issues gracefully.
- <AuthAssets /> + {{#let (component 'error-boundary') as |ErrorBoundary|}} + <ErrorBoundary> + {{#if this.isAuthLoading}} + <AkSpinner /> + {{else}} + <AuthAssets /> + {{/if}} + </ErrorBoundary> + {{/let}}app/components/side-nav/index.scss (1)
153-156
: Consider using currentColor for SVG path stylingInstead of explicitly setting the stroke color, consider using
currentColor
to inherit the color from the parent element. This would reduce redundancy and make the code more maintainable..menu-item-icon-svg { path { - stroke: var(--side-nav-inherit-background-text-color) !important; + stroke: currentColor; } }app/components/side-nav/index.ts (2)
21-65
: Consider enhancing type safety in interfacesWhile the interfaces are well-structured, consider these type safety improvements:
- Make required properties explicit in MenuItem interface
- Consider using an enum for the svg property in SwitcherMenuItem
- Add validation for the query parameter values in MenuItem
export interface MenuItem { label: string; icon?: string; route?: string; - query?: Record<string, string | number>; + query?: Record<string, string | number | undefined>; currentWhen?: string; hasBadge?: boolean; badgeLabel?: string; component?: 'side-nav/security-menu-item'; customIconComponent?: 'ak-svg/public-api-icon'; } +export enum SwitcherSvgType { + SmIndicator = 'ak-svg/sm-indicator', + VpIndicator = 'ak-svg/vp-indicator' +} export interface SwitcherMenuItem { id: string; - svg: 'ak-svg/sm-indicator' | 'ak-svg/vp-indicator'; + svg: SwitcherSvgType; label: string; route: string; key: string; }
67-124
: Consider splitting the component for better separation of concernsThe SideNavComponent currently handles multiple responsibilities:
- Navigation management
- Product switching
- Image management
- Sidebar state management
Consider splitting this into smaller, more focused components:
- ProductSwitcher
- NavigationMenu
- SidebarController
This would improve maintainability and make the code more testable.
app/components/appknox-wrapper/index.ts (1)
193-196
: Consider handling loading state for security menu itemThe security menu item's visibility depends on
isSecurityEnabled
, which is set asynchronously. This could lead to a flash of incorrect content while the security status is being determined.Consider adding a loading state:
- this.isSecurityEnabled && { + this.isSecurityEnabled !== undefined && { label: this.intl.t('security'), - component: 'side-nav/security-menu-item' as const, + component: this.isSecurityEnabled + ? ('side-nav/security-menu-item' as const) + : ('side-nav/loading-menu-item' as const), },tests/integration/components/appknox-wrapper-test.js (5)
30-35
: Consider making ConfigurationStub more configurableThe stub has hardcoded values which might make it difficult to test different scenarios. Consider making it more flexible by accepting configuration through constructor or setters.
class ConfigurationStub extends Service { - frontendData = {}; - themeData = {}; - imageData = {}; - serverData = { urlUploadAllowed: true, enterprise: false }; + frontendData; + themeData; + imageData; + serverData; + + constructor() { + super(...arguments); + this.reset(); + } + + reset() { + this.frontendData = {}; + this.themeData = {}; + this.imageData = {}; + this.serverData = { urlUploadAllowed: true, enterprise: false }; + } + + configure(config = {}) { + Object.assign(this, config); + }
51-82
: Add documentation and type safety to menuItems functionThe function accepts multiple boolean parameters but lacks documentation explaining their purpose. Consider adding JSDoc or TypeScript types.
+/** + * Generates menu items based on feature flags + * @param {Object} options + * @param {boolean} options.appMonitoring - Enable app monitoring menu + * @param {boolean} options.sbom - Enable SBOM menu + * @param {boolean} options.analytics - Enable analytics menu + * @param {boolean} options.publicApis - Enable public APIs menu + * @param {boolean} options.market - Enable marketplace menu + * @param {boolean} options.billing - Enable billing menu + * @param {boolean} options.partner - Enable partner menu + * @param {boolean} options.security - Enable security menu + * @returns {Array<{label: string, icon?: string, hasBadge?: boolean, badgeLabel?: string}>} + */ const menuItems = ({ appMonitoring, sbom, // ...
95-110
: Standardize enabled flag usage in lowerMenuItemsSome items have the
enabled
flag while others don't. This inconsistency could lead to confusion. Consider standardizing the usage.const lowerMenuItems = [ { title: () => t('chatSupport'), icon: 'chat-bubble', - enabled: false, + enabled: () => false, }, { title: () => t('version'), icon: 'info', - enabled: false, + enabled: () => false, }, { title: () => t('collapse'), icon: 'keyboard-tab', + enabled: () => true, }, ];
152-154
: Extract submissions API setup to a helper functionThe submissions API setup is mixed with other setup code. Consider extracting it to a separate helper function for better organization.
+ function setupSubmissionsApi(server) { + server.get('/submissions', () => []); + } + hooks.beforeEach(async function () { // ... other setup code ... - // handle submissions api for each test - this.server.get('/submissions', () => []); + setupSubmissionsApi(this.server); });
214-215
: Document the expected assertion countThe magic number
5
inassert.expect(5)
should be documented to explain what assertions are expected.test('test knowledge base click', async function (assert) { - assert.expect(5); + // Expect 5 assertions: + // 1. Top nav exists + // 2. Knowledge base button exists + // 3. Knowledge base button has correct text + // 4. Knowledge base click handler called + // 5. Knowledge base click handler called with no arguments + assert.expect(5);translations/en.json (1)
1626-1626
: Expand VAPT to "Vulnerability Assessment and Penetration Testing (VAPT)" for consistencyBased on the search results, the codebase follows a pattern of expanding security-related acronyms, as seen with "Dynamic Application Security Testing (DAST)" and other terms. For consistency and clarity, VAPT should also be expanded.
Apply this diff:
- "vapt": "VAPT", + "vapt": "Vulnerability Assessment and Penetration Testing (VAPT)",app/components/storeknox-wrapper/index.ts (1)
21-24
: Consider adding type annotations for improved clarityExplicitly typing the services enhances readability and helps with type checking. Ensure that all injected services have appropriate type annotations.
Apply this diff to add missing type annotations:
@service('browser/window') declare window: Window; - @service declare intl: IntlService; - @service declare configuration: ConfigurationService; + @service declare intl: IntlService; + @service declare configuration: ConfigurationService;Note: If the types are already inferred correctly, this can be considered optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.json
is excluded by!**/package-lock.json
📒 Files selected for processing (80)
app/components/ak-popover/index.hbs
(1 hunks)app/components/ak-popover/index.ts
(1 hunks)app/components/ak-svg/sm-indicator.hbs
(1 hunks)app/components/ak-svg/vp-indicator.hbs
(1 hunks)app/components/ak-tabs/item/index.scss
(1 hunks)app/components/appknox-wrapper/index.hbs
(1 hunks)app/components/appknox-wrapper/index.scss
(2 hunks)app/components/appknox-wrapper/index.ts
(4 hunks)app/components/appknox-wrapper/onboarding-guide/index.scss
(1 hunks)app/components/appknox-wrapper/onboarding-guide/index.ts
(3 hunks)app/components/file-compare/index.ts
(2 hunks)app/components/file-compare/table/index.ts
(3 hunks)app/components/file-compare/vulnerability-details/index.ts
(2 hunks)app/components/home-page/index.hbs
(1 hunks)app/components/home-page/index.js
(0 hunks)app/components/home-page/index.scss
(1 hunks)app/components/home-page/index.ts
(1 hunks)app/components/home-page/organization-dashboard/header/index.hbs
(0 hunks)app/components/home-page/organization-dashboard/header/index.scss
(0 hunks)app/components/home-page/organization-dashboard/index.hbs
(0 hunks)app/components/home-page/organization-dashboard/index.ts
(0 hunks)app/components/home-page/organization-dashboard/side-nav/security-menu-item/index.ts
(0 hunks)app/components/home-page/product-card/index.hbs
(1 hunks)app/components/home-page/product-card/index.scss
(1 hunks)app/components/home-page/product-card/index.ts
(1 hunks)app/components/home-page/security-dashboard-nav/index.hbs
(0 hunks)app/components/home-page/security-dashboard-nav/index.scss
(0 hunks)app/components/home-page/security-dashboard-nav/index.ts
(0 hunks)app/components/partner/template.hbs
(1 hunks)app/components/security-wrapper/index.hbs
(1 hunks)app/components/security-wrapper/index.scss
(1 hunks)app/components/security-wrapper/index.ts
(1 hunks)app/components/security/nav-menu/index.hbs
(1 hunks)app/components/security/nav-menu/index.scss
(1 hunks)app/components/side-nav/index.hbs
(4 hunks)app/components/side-nav/index.scss
(7 hunks)app/components/side-nav/index.ts
(1 hunks)app/components/side-nav/product-switcher/index.hbs
(1 hunks)app/components/side-nav/product-switcher/index.scss
(1 hunks)app/components/side-nav/product-switcher/index.ts
(1 hunks)app/components/side-nav/security-menu-item/index.ts
(1 hunks)app/components/storeknox-wrapper/index.hbs
(1 hunks)app/components/storeknox-wrapper/index.scss
(1 hunks)app/components/storeknox-wrapper/index.ts
(1 hunks)app/components/top-nav/index.hbs
(1 hunks)app/components/top-nav/index.scss
(1 hunks)app/components/top-nav/index.ts
(4 hunks)app/models/organization.ts
(1 hunks)app/router.ts
(3 hunks)app/routes/authenticated/dashboard.ts
(1 hunks)app/routes/authenticated/home.ts
(1 hunks)app/routes/authenticated/index.ts
(1 hunks)app/routes/authenticated/partner.ts
(2 hunks)app/routes/authenticated/storeknox.ts
(1 hunks)app/services/user-auth.ts
(1 hunks)app/styles/_component-variables.scss
(3 hunks)app/styles/_icons.scss
(1 hunks)app/styles/_theme.scss
(1 hunks)app/templates/authenticated.hbs
(1 hunks)app/templates/authenticated/dashboard.hbs
(1 hunks)app/templates/authenticated/home.hbs
(1 hunks)app/templates/authenticated/partner.hbs
(1 hunks)app/templates/authenticated/security.hbs
(1 hunks)app/templates/authenticated/storeknox.hbs
(1 hunks)app/templates/authenticated/storeknox/inventory.hbs
(1 hunks)app/utils/scroll-to-top.ts
(2 hunks)tests/acceptance/home-page-test.js
(1 hunks)tests/acceptance/side-nav-test.js
(1 hunks)tests/integration/components/appknox-wrapper-test.js
(1 hunks)tests/integration/components/appknox-wrapper/onboarding-guide-test.js
(1 hunks)tests/integration/components/home-page/organization-dashboard/header-test.js
(0 hunks)tests/integration/components/home-page/organization-dashboard/onboarding-guide-test.js
(0 hunks)tests/integration/components/home-page/organization-dashboard/side-nav-test.js
(0 hunks)tests/integration/components/home-page/security-dashboard-nav-test.js
(0 hunks)tests/integration/components/security-wrapper-test.js
(1 hunks)tests/integration/components/storeknox-wrapper-test.js
(1 hunks)tests/integration/components/top-nav-test.js
(1 hunks)translations/en.json
(10 hunks)translations/ja.json
(10 hunks)types/ak-svg.d.ts
(1 hunks)
💤 Files with no reviewable changes (13)
- app/components/home-page/index.js
- app/components/home-page/organization-dashboard/header/index.hbs
- app/components/home-page/organization-dashboard/header/index.scss
- app/components/home-page/organization-dashboard/index.hbs
- app/components/home-page/organization-dashboard/index.ts
- app/components/home-page/organization-dashboard/side-nav/security-menu-item/index.ts
- app/components/home-page/security-dashboard-nav/index.hbs
- app/components/home-page/security-dashboard-nav/index.scss
- app/components/home-page/security-dashboard-nav/index.ts
- tests/integration/components/home-page/organization-dashboard/header-test.js
- tests/integration/components/home-page/organization-dashboard/onboarding-guide-test.js
- tests/integration/components/home-page/organization-dashboard/side-nav-test.js
- tests/integration/components/home-page/security-dashboard-nav-test.js
🚧 Files skipped from review as they are similar to previous changes (55)
- app/components/ak-popover/index.hbs
- app/components/ak-popover/index.ts
- app/components/ak-svg/sm-indicator.hbs
- app/components/ak-svg/vp-indicator.hbs
- app/components/ak-tabs/item/index.scss
- app/components/appknox-wrapper/index.hbs
- app/components/appknox-wrapper/index.scss
- app/components/appknox-wrapper/onboarding-guide/index.scss
- app/components/appknox-wrapper/onboarding-guide/index.ts
- app/components/file-compare/index.ts
- app/components/file-compare/table/index.ts
- app/components/file-compare/vulnerability-details/index.ts
- app/components/home-page/index.scss
- app/components/home-page/index.ts
- app/components/home-page/product-card/index.scss
- app/components/home-page/product-card/index.ts
- app/components/partner/template.hbs
- app/components/security-wrapper/index.hbs
- app/components/security-wrapper/index.scss
- app/components/security-wrapper/index.ts
- app/components/security/nav-menu/index.hbs
- app/components/security/nav-menu/index.scss
- app/components/side-nav/index.hbs
- app/components/side-nav/product-switcher/index.hbs
- app/components/side-nav/product-switcher/index.scss
- app/components/side-nav/security-menu-item/index.ts
- app/components/storeknox-wrapper/index.hbs
- app/components/storeknox-wrapper/index.scss
- app/components/top-nav/index.hbs
- app/components/top-nav/index.scss
- app/components/top-nav/index.ts
- app/router.ts
- app/routes/authenticated/dashboard.ts
- app/routes/authenticated/home.ts
- app/routes/authenticated/index.ts
- app/routes/authenticated/partner.ts
- app/routes/authenticated/storeknox.ts
- app/services/user-auth.ts
- app/styles/_icons.scss
- app/styles/_theme.scss
- app/templates/authenticated.hbs
- app/templates/authenticated/dashboard.hbs
- app/templates/authenticated/home.hbs
- app/templates/authenticated/partner.hbs
- app/templates/authenticated/security.hbs
- app/templates/authenticated/storeknox.hbs
- app/templates/authenticated/storeknox/inventory.hbs
- app/utils/scroll-to-top.ts
- tests/acceptance/home-page-test.js
- tests/acceptance/side-nav-test.js
- tests/integration/components/appknox-wrapper/onboarding-guide-test.js
- tests/integration/components/storeknox-wrapper-test.js
- tests/integration/components/top-nav-test.js
- translations/ja.json
- types/ak-svg.d.ts
🔇 Additional comments (28)
app/components/home-page/product-card/index.hbs (2)
10-30
: LGTM with previous suggestions.
The content structure is well-organized. Please refer to the previous review comments about accessibility improvements and icon handling.
1-45
: Overall structure looks good!
The component follows a clean, modular structure with appropriate use of layout and typography components. Please ensure to address:
- Previous suggestions about using enums for product names
- Accessibility improvements for various elements
- Route binding verification
- New keyboard navigation enhancements
app/components/home-page/index.hbs (3)
18-18
: Missing translation keys detected.
The translation keys 'selectThePath', 'toSecureYourMobileApps', and 'logout' are still missing from the translations files.
Also applies to: 33-33, 48-48
36-50
: Add error handling for logout action.
The logout button directly calls invalidateSession
without error handling.
56-67
: Add loading state for product cards.
Consider adding a loading state while the product cards data is being fetched.
app/models/organization.ts (2)
Line range hint 63-68
: Consider addressing the TODO comment in membersCount getter.
There's an existing TODO comment suggesting this getter might be unused. Consider either removing it if truly unused or documenting its purpose if it's needed.
Let's verify if this getter is actually used:
16-16
: LGTM! Verify feature flag usage across the application.
The addition of the storeknox
feature flag follows the existing pattern and is type-safe.
Let's verify the feature flag usage across the codebase:
✅ Verification successful
Feature flag implementation is complete and properly guarded
The storeknox
feature flag is well-integrated across the application:
- Route guard in
app/routes/authenticated/storeknox.ts
prevents unauthorized access - Feature flag is used in tests (
home-page-test.js
,side-nav-test.js
) - Dedicated templates and components exist with proper feature-specific naming
- Implementation follows the established patterns for feature flags in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of storeknox feature flag in templates, components, and routes
# Check template usage
echo "Checking template usage:"
rg -t hbs "storeknox" -A 2
# Check TypeScript/JavaScript usage
echo "Checking TS/JS usage:"
rg "features\.storeknox|storeknox:" -t ts -t js -A 2
# Check for any potential feature guard implementations
echo "Checking feature guards:"
ast-grep --pattern 'if ($x.features.storeknox) { $$$ }'
Length of output: 2657
app/components/side-nav/product-switcher/index.ts (6)
1-10
: LGTM! Dependencies and imports are well-organized.
The imports are properly structured with a clear separation between core framework imports, type imports, and local dependencies.
11-21
: LGTM! Well-defined component signature interface.
The interface provides clear type definitions for component arguments and proper HTML element binding.
23-41
: LGTM! Clean component implementation with proper state management.
The component follows Ember best practices with proper service injection, state tracking, and computed properties.
43-68
: LGTM! Improved implementation addressing previous review comments.
The switcherMenuItems
getter has been enhanced with:
- Clear array structure for menu items
- Proper filtering to exclude the current product
- Conditional labels based on enterprise status
70-76
: LGTM! Well-typed and focused action handlers.
The actions are properly typed and maintain clear responsibilities for handling switcher interactions.
79-84
: LGTM! Proper type registry setup for template type checking.
The component is correctly registered in the Glint registry with both namespaced and path-based signatures.
tests/integration/components/security-wrapper-test.js (2)
59-62
: Move OrganizationMeStub class definition to module scope.
The class definition inside beforeEach creates unnecessary overhead. This is a duplicate of a past review comment that's still valid.
78-80
: Enhance submissions API mock with realistic scenarios.
The current mock is oversimplified and doesn't test various response scenarios. This is a duplicate of a past review comment that's still valid.
app/components/side-nav/index.scss (4)
7-10
: LGTM! Variable naming updates improve consistency
The renaming from --organization-dashboard-side-nav-
to --side-nav-
prefix makes the code more maintainable and consistent.
31-31
: Skipping comment as it was already addressed in a past review
62-65
: Skipping comment as it was already addressed in a past review
178-178
: Skipping comment as it was already addressed in a past review
app/components/side-nav/index.ts (2)
1-19
: LGTM! Well-structured imports and type definitions.
The imports are properly organized and the DefaultBlock type is well-defined with appropriate optional properties.
126-130
: LGTM! Proper Glint type registry implementation.
The component is correctly registered in the Glint registry for proper type checking in templates.
app/components/appknox-wrapper/index.ts (1)
294-297
: LGTM: Proper Glint registry declaration
The component is correctly registered in the Glint registry, which ensures proper type checking in templates.
app/styles/_component-variables.scss (3)
346-373
: Well-structured side navigation variables implementation!
The side navigation variables are well-organized with:
- Clear semantic naming
- Proper theme separation
- Consistent naming patterns
- Appropriate z-index handling
376-383
: LGTM: Consistent product switcher variable implementation.
The product switcher variables maintain consistency with the established patterns and use appropriate base variables.
1916-1923
: LGTM: Clean home page variable implementation.
The home page variables follow best practices with:
- Appropriate use of semantic variables
- Consistent naming patterns
- Logical grouping of related variables
translations/en.json (2)
366-366
: Navigation menu items need proper hierarchy.
The new navigation items "discovery", "home", and "inventory" appear to be part of the main menu structure. Ensure they follow proper capitalization and hierarchy:
- "discovery": "Discovery",
- "home": "Home",
- "inventory": "Inventory",
+ "discovery": "Discovery",
+ "home": "Home",
+ "inventory": "Inventory"
Also applies to: 593-593, 648-648
83-83
: LGTM! New entries follow translation conventions.
The new entries for app switcher, partner, switch action, dashboard navigation, and product tagline follow proper translation conventions and maintain consistent casing.
Also applies to: 1083-1083, 1522-1522, 1530-1530, 1563-1563
app/components/storeknox-wrapper/index.ts (1)
56-58
: Ensure proper typing of configuration properties
Accessing this.configuration.serverData.enterprise
assumes that enterprise
exists and is of a specific type. Verify that enterprise
is correctly typed in ConfigurationService
to ensure type safety.
Run the following script to check the type of enterprise
in ConfigurationService
:
✅ Verification successful
Property enterprise
is properly typed and used consistently
The property is correctly typed in ConfigurationService
as enterprise: string | boolean
and initialized as an empty string. The property is set with a boolean value through type coercion (data.enterprise == true
) and is consistently used across multiple components as a boolean flag. The implementation in the reviewed code aligns with the typing and usage pattern throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the type of 'enterprise' in ConfigurationService.
# Search for the declaration of 'enterprise' in ConfigurationService.
fd -I -t f 'configuration.ts' | xargs grep -n 'enterprise'
# Inspect the type of 'enterprise' property.
Length of output: 270
Script:
#!/bin/bash
# Let's verify the usage of enterprise property in the codebase
rg -n "serverData\.enterprise" -A 2
Length of output: 1761
45bf690
to
6de9b12
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (8)
app/components/home-page/index.hbs (1)
58-65
: Add type checking for productCardDetails propertiesConsider adding runtime type checking for the productCardDetails properties to prevent potential rendering issues.
<HomePage::ProductCard + {{! template-lint-disable no-unused-block-params }} + {{#let + (and + (is-string productCardDetails.title) + (is-string productCardDetails.desc) + (is-string productCardDetails.link) + (is-string productCardDetails.coverImage) + (is-string productCardDetails.indicator) + ) as |isValid| + }} + {{#if isValid}} @name={{productCardDetails.title}} @description={{productCardDetails.desc}} @route={{productCardDetails.link}} @linkText={{t 'takeMeToDashboard'}} @coverBackgroundImage={{productCardDetails.coverImage}} @indicatorSvg={{productCardDetails.indicator}} + {{else}} + {{log "Invalid product card details format"}} + {{/if}} + {{/let}} />app/components/side-nav/index.ts (2)
11-65
: LGTM! Consider adding JSDoc comments for better documentation.The type definitions are well-structured and provide good type safety. Consider adding JSDoc comments to document the purpose and usage of each interface.
Example documentation:
/** * Configuration for the default template block. * Provides class names for various menu item elements. */ type DefaultBlock = { // ... }; /** * Component signature defining the expected arguments and blocks. */ export interface SideNavSignature { // ... }
85-93
: Consider using CSS modules type safety.The
classes
getter could benefit from TypeScript's type safety by using the CSS modules types.import { type ClassesType } from 'ember-css-modules'; import styles from './index.scss'; declare module './index.scss' { export interface IIndexStyles { 'menu-item-text': string; 'menu-item-link': string; 'active': string; 'menu-item-tooltip': string; 'menu-item-icon': string; } } // In the component get classes(): ClassesType<IIndexStyles> { return { menuItemText: styles['menu-item-text'], menuItemLink: styles['menu-item-link'], menuItemLinkActive: styles['active'], menuItemTooltip: styles['menu-item-tooltip'], menuItemIcon: styles['menu-item-icon'], }; }app/components/appknox-wrapper/index.ts (2)
Line range hint
123-197
: Consider using type predicate for menuItems filter.The current filter removes falsy values but TypeScript doesn't know that the resulting array only contains MenuItem objects.
Consider using a type predicate:
- ].filter(Boolean) as MenuItem[]; + ].filter((item): item is MenuItem => Boolean(item));
Line range hint
28-291
: Consider splitting the component into smaller, focused components.The AppknoxWrapper component has grown to handle multiple responsibilities:
- Security status management
- Sidebar state management
- Menu items management
- Chat/Support widget integration
- Organization management
This could make the component harder to maintain and test.
Consider:
- Extract menu-related logic into a separate service or component
- Create a dedicated service for support/chat widget management
- Move security status management to a service
- Use composition to combine these smaller, focused components
This would improve:
- Testability
- Maintainability
- Code reusability
- Separation of concerns
tests/integration/components/appknox-wrapper-test.js (2)
13-23
: Add type checking to NotificationsStub methodsConsider adding type checking to ensure message parameters are strings.
class NotificationsStub extends Service { errorMsg = null; successMsg = null; error(msg) { + if (typeof msg !== 'string') { + throw new TypeError('Error message must be a string'); + } this.errorMsg = msg; } success(msg) { + if (typeof msg !== 'string') { + throw new TypeError('Success message must be a string'); + } this.successMsg = msg; } }
51-82
: Add JSDoc documentation to menuItems functionConsider adding documentation to improve code maintainability and type safety.
+/** + * Generates menu items based on enabled features + * @param {Object} options + * @param {boolean} options.appMonitoring - Whether app monitoring is enabled + * @param {boolean} options.sbom - Whether SBOM is enabled + * @param {boolean} options.analytics - Whether analytics is enabled + * @param {boolean} options.publicApis - Whether public APIs are enabled + * @param {boolean} options.market - Whether marketplace is enabled + * @param {boolean} options.billing - Whether billing is enabled + * @param {boolean} options.partner - Whether partner features are enabled + * @param {boolean} options.security - Whether security features are enabled + * @returns {Array<Object>} Array of menu items + */ const menuItems = ({ appMonitoring, sbom, // ...translations/en.json (1)
81-81
: Maintain consistent terminology in product messagingThe product descriptions and messaging use inconsistent terminology:
- "mobile applications" vs "mobile apps"
- "To Secure Your Mobile Applications" uses a different style than product descriptions
Consider standardizing to either "mobile applications" or "mobile apps" consistently:
- "appknoxDesc": "Assess your mobile applications for vulnerabilities", - "storeknoxDesc": "Monitor your mobile apps on Play Store and App Store", - "toSecureYourMobileApps": "To Secure Your Mobile Applications", + "appknoxDesc": "Assess your mobile applications for vulnerabilities", + "storeknoxDesc": "Monitor your mobile applications on Play Store and App Store", + "toSecureYourMobileApps": "To Secure Your Mobile Applications"Also applies to: 1507-1507, 1563-1563
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.json
is excluded by!**/package-lock.json
📒 Files selected for processing (80)
app/components/ak-popover/index.hbs
(1 hunks)app/components/ak-popover/index.ts
(1 hunks)app/components/ak-svg/sm-indicator.hbs
(1 hunks)app/components/ak-svg/vp-indicator.hbs
(1 hunks)app/components/ak-tabs/item/index.scss
(1 hunks)app/components/appknox-wrapper/index.hbs
(1 hunks)app/components/appknox-wrapper/index.scss
(2 hunks)app/components/appknox-wrapper/index.ts
(4 hunks)app/components/appknox-wrapper/onboarding-guide/index.scss
(1 hunks)app/components/appknox-wrapper/onboarding-guide/index.ts
(3 hunks)app/components/file-compare/index.ts
(2 hunks)app/components/file-compare/table/index.ts
(3 hunks)app/components/file-compare/vulnerability-details/index.ts
(2 hunks)app/components/home-page/index.hbs
(1 hunks)app/components/home-page/index.js
(0 hunks)app/components/home-page/index.scss
(1 hunks)app/components/home-page/index.ts
(1 hunks)app/components/home-page/organization-dashboard/header/index.hbs
(0 hunks)app/components/home-page/organization-dashboard/header/index.scss
(0 hunks)app/components/home-page/organization-dashboard/index.hbs
(0 hunks)app/components/home-page/organization-dashboard/index.ts
(0 hunks)app/components/home-page/organization-dashboard/side-nav/security-menu-item/index.ts
(0 hunks)app/components/home-page/product-card/index.hbs
(1 hunks)app/components/home-page/product-card/index.scss
(1 hunks)app/components/home-page/product-card/index.ts
(1 hunks)app/components/home-page/security-dashboard-nav/index.hbs
(0 hunks)app/components/home-page/security-dashboard-nav/index.scss
(0 hunks)app/components/home-page/security-dashboard-nav/index.ts
(0 hunks)app/components/partner/template.hbs
(1 hunks)app/components/security-wrapper/index.hbs
(1 hunks)app/components/security-wrapper/index.scss
(1 hunks)app/components/security-wrapper/index.ts
(1 hunks)app/components/security/nav-menu/index.hbs
(1 hunks)app/components/security/nav-menu/index.scss
(1 hunks)app/components/side-nav/index.hbs
(4 hunks)app/components/side-nav/index.scss
(7 hunks)app/components/side-nav/index.ts
(1 hunks)app/components/side-nav/product-switcher/index.hbs
(1 hunks)app/components/side-nav/product-switcher/index.scss
(1 hunks)app/components/side-nav/product-switcher/index.ts
(1 hunks)app/components/side-nav/security-menu-item/index.ts
(1 hunks)app/components/storeknox-wrapper/index.hbs
(1 hunks)app/components/storeknox-wrapper/index.scss
(1 hunks)app/components/storeknox-wrapper/index.ts
(1 hunks)app/components/top-nav/index.hbs
(1 hunks)app/components/top-nav/index.scss
(1 hunks)app/components/top-nav/index.ts
(4 hunks)app/models/organization.ts
(1 hunks)app/router.ts
(3 hunks)app/routes/authenticated/dashboard.ts
(1 hunks)app/routes/authenticated/home.ts
(1 hunks)app/routes/authenticated/index.ts
(1 hunks)app/routes/authenticated/partner.ts
(2 hunks)app/routes/authenticated/storeknox.ts
(1 hunks)app/services/user-auth.ts
(1 hunks)app/styles/_component-variables.scss
(4 hunks)app/styles/_icons.scss
(1 hunks)app/styles/_theme.scss
(1 hunks)app/templates/authenticated.hbs
(1 hunks)app/templates/authenticated/dashboard.hbs
(1 hunks)app/templates/authenticated/home.hbs
(1 hunks)app/templates/authenticated/partner.hbs
(1 hunks)app/templates/authenticated/security.hbs
(1 hunks)app/templates/authenticated/storeknox.hbs
(1 hunks)app/templates/authenticated/storeknox/inventory.hbs
(1 hunks)app/utils/scroll-to-top.ts
(2 hunks)tests/acceptance/home-page-test.js
(1 hunks)tests/acceptance/side-nav-test.js
(1 hunks)tests/integration/components/appknox-wrapper-test.js
(1 hunks)tests/integration/components/appknox-wrapper/onboarding-guide-test.js
(1 hunks)tests/integration/components/home-page/organization-dashboard/header-test.js
(0 hunks)tests/integration/components/home-page/organization-dashboard/onboarding-guide-test.js
(0 hunks)tests/integration/components/home-page/organization-dashboard/side-nav-test.js
(0 hunks)tests/integration/components/home-page/security-dashboard-nav-test.js
(0 hunks)tests/integration/components/security-wrapper-test.js
(1 hunks)tests/integration/components/storeknox-wrapper-test.js
(1 hunks)tests/integration/components/top-nav-test.js
(1 hunks)translations/en.json
(10 hunks)translations/ja.json
(10 hunks)types/ak-svg.d.ts
(1 hunks)
💤 Files with no reviewable changes (13)
- app/components/home-page/index.js
- app/components/home-page/organization-dashboard/header/index.hbs
- app/components/home-page/organization-dashboard/header/index.scss
- app/components/home-page/organization-dashboard/index.hbs
- app/components/home-page/organization-dashboard/index.ts
- app/components/home-page/organization-dashboard/side-nav/security-menu-item/index.ts
- app/components/home-page/security-dashboard-nav/index.hbs
- app/components/home-page/security-dashboard-nav/index.scss
- app/components/home-page/security-dashboard-nav/index.ts
- tests/integration/components/home-page/organization-dashboard/header-test.js
- tests/integration/components/home-page/organization-dashboard/onboarding-guide-test.js
- tests/integration/components/home-page/organization-dashboard/side-nav-test.js
- tests/integration/components/home-page/security-dashboard-nav-test.js
🚧 Files skipped from review as they are similar to previous changes (59)
- app/components/ak-popover/index.hbs
- app/components/ak-popover/index.ts
- app/components/ak-svg/sm-indicator.hbs
- app/components/ak-svg/vp-indicator.hbs
- app/components/ak-tabs/item/index.scss
- app/components/appknox-wrapper/index.hbs
- app/components/appknox-wrapper/index.scss
- app/components/appknox-wrapper/onboarding-guide/index.scss
- app/components/appknox-wrapper/onboarding-guide/index.ts
- app/components/file-compare/index.ts
- app/components/file-compare/table/index.ts
- app/components/file-compare/vulnerability-details/index.ts
- app/components/home-page/index.scss
- app/components/home-page/index.ts
- app/components/home-page/product-card/index.hbs
- app/components/home-page/product-card/index.scss
- app/components/home-page/product-card/index.ts
- app/components/partner/template.hbs
- app/components/security-wrapper/index.hbs
- app/components/security-wrapper/index.scss
- app/components/security-wrapper/index.ts
- app/components/security/nav-menu/index.hbs
- app/components/security/nav-menu/index.scss
- app/components/side-nav/index.hbs
- app/components/side-nav/index.scss
- app/components/side-nav/product-switcher/index.hbs
- app/components/side-nav/product-switcher/index.scss
- app/components/side-nav/product-switcher/index.ts
- app/components/side-nav/security-menu-item/index.ts
- app/components/storeknox-wrapper/index.hbs
- app/components/storeknox-wrapper/index.scss
- app/components/storeknox-wrapper/index.ts
- app/components/top-nav/index.hbs
- app/components/top-nav/index.scss
- app/components/top-nav/index.ts
- app/models/organization.ts
- app/routes/authenticated/dashboard.ts
- app/routes/authenticated/home.ts
- app/routes/authenticated/index.ts
- app/routes/authenticated/partner.ts
- app/routes/authenticated/storeknox.ts
- app/services/user-auth.ts
- app/styles/_component-variables.scss
- app/styles/_icons.scss
- app/styles/_theme.scss
- app/templates/authenticated.hbs
- app/templates/authenticated/dashboard.hbs
- app/templates/authenticated/home.hbs
- app/templates/authenticated/partner.hbs
- app/templates/authenticated/security.hbs
- app/templates/authenticated/storeknox.hbs
- app/templates/authenticated/storeknox/inventory.hbs
- app/utils/scroll-to-top.ts
- tests/acceptance/home-page-test.js
- tests/acceptance/side-nav-test.js
- tests/integration/components/appknox-wrapper/onboarding-guide-test.js
- tests/integration/components/storeknox-wrapper-test.js
- tests/integration/components/top-nav-test.js
- types/ak-svg.d.ts
🔇 Additional comments (20)
app/components/home-page/index.hbs (5)
1-6
: LGTM! Well-structured container layout
The main container layout is well-organized using AkStack with proper test attributes and semantic class naming.
18-18
: Translation keys still missing
Also applies to: 33-33, 48-48
36-50
: Add error handling for logout action
The logout functionality should handle potential failures gracefully.
56-67
: Add loading state and null checks for product cards
7-14
: Verify AuthAssets component integration
The logo section structure looks good, but let's verify the AuthAssets component integration.
✅ Verification successful
AuthAssets component integration is properly implemented
The verification shows that:
- The AuthAssets component exists and is properly defined in
app/components/auth-assets/index.ts
- It's consistently used across multiple authentication-related components including home-page, login, registration, and system status pages
- Integration tests exist in
tests/integration/components/auth-assets-test.js
- The usage in home-page follows the same pattern as other implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify AuthAssets component existence and its interface
# Test: Search for AuthAssets component definition and its usage
ast-grep --pattern 'class AuthAssets extends Component {
$$$
}'
# Check for other usages of AuthAssets
rg -A 5 "AuthAssets"
Length of output: 4323
tests/integration/components/security-wrapper-test.js (4)
1-9
: LGTM! All necessary testing utilities are imported.
The imports include all required testing utilities for Ember integration tests, including QUnit, Mirage for API mocking, and Intl for internationalization support.
44-48
: LGTM! Test module setup follows best practices.
The test module correctly sets up all required test helpers for rendering, API mocking, and internationalization.
78-80
: 🛠️ Refactor suggestion
Enhance submissions API mock to handle different scenarios.
The current mock is too simplistic and doesn't test how the component handles different API responses.
Improve the mock implementation:
- this.server.get('/submissions', () => []);
+ this.server.get('/submissions', (schema, request) => {
+ const { queryParams } = request;
+
+ if (queryParams.error) {
+ return new Response(500, {}, { error: 'Internal Server Error' });
+ }
+
+ if (queryParams.empty) {
+ return [];
+ }
+
+ return schema.submissions.all();
+ });
Likely invalid or redundant comment.
59-62
: 🛠️ Refactor suggestion
Move OrganizationMeStub class definition outside beforeEach hook.
Defining the class inside the hook creates a new class definition on every test run. This could lead to memory leaks and reduced maintainability.
Move the class definition to module scope:
+class OrganizationMeStub extends Service {
+ org = null;
+
+ setOrg(org) {
+ this.org = org;
+ }
+}
module('Integration | Component | security-wrapper', function (hooks) {
// ... existing setup ...
hooks.beforeEach(async function () {
// ... existing setup ...
- class OrganizationMeStub extends Service {
- org = organizationMe;
- }
+ const organizationMeService = this.owner.lookup('service:me');
+ organizationMeService.setOrg(organizationMe);
// ... rest of setup ...
});
Likely invalid or redundant comment.
app/components/side-nav/index.ts (2)
126-130
: LGTM! Proper Glint type registration.
The Glint registry declaration is correctly implemented, providing proper type checking for the component in templates.
1-130
: Verify component integration and usage.
Let's verify how this component is being used across the application and ensure all required services are properly initialized.
✅ Verification successful
Component integration is properly implemented and services are correctly initialized
The verification shows that:
- The SideNav component is correctly used in
appknox-wrapper
andstoreknox-wrapper
components - All required services (
me
,organization
,integration
,intl
,whitelabel
,window
) are properly initialized and used across the application - No circular dependencies are found between the side-nav component and the services it depends on
- The component's integration follows the application's architectural patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check component usage and service initialization
# Find component usage in templates
echo "Checking component usage in templates..."
rg -t hbs "SideNav"
# Check if all required services are properly initialized
echo "Checking service initialization..."
rg -t ts "(@service\s+(?:declare\s+)?(?:me|organization|integration|intl|whitelabel)|service\('browser/window'\))"
# Check for potential circular dependencies
echo "Checking for circular dependencies..."
rg -t ts "import.*from.*side-nav.*" --files-with-matches | xargs -I {} rg -t ts "import.*from.*irene/services" {}
Length of output: 35414
app/router.ts (1)
46-46
: LGTM! Route path follows established pattern.
The home route is correctly placed under the authenticated section with the standard '/dashboard' prefix.
app/components/appknox-wrapper/index.ts (2)
18-27
: LGTM! Clean interface definition.
The interface is well-structured with proper typing for both Args and Blocks.
254-261
:
Add error handling for localStorage operations.
The toggleSidebar method should handle potential localStorage access failures, which can occur in private browsing mode or when storage is full.
Add try-catch block:
@action
toggleSidebar() {
this.isSidebarCollapsed = !this.isSidebarCollapsed;
+ try {
this.window.localStorage.setItem(
'sidebarState',
this.isSidebarCollapsed ? 'collapsed' : 'expanded'
);
+ } catch (error) {
+ this.notifications.error(this.intl.t('errors.localStorage'));
+ }
}
Likely invalid or redundant comment.
tests/integration/components/appknox-wrapper-test.js (3)
37-41
: Add error handling to IntegrationStub.configure
The configure method should validate its input parameter.
118-154
: Modularize test setup for better maintainability
The beforeEach hook handles too many responsibilities. Consider extracting setup logic into separate helper functions.
242-269
:
Replace remaining test.each instances with proper QUnit tests
Multiple instances of test.each need to be replaced with proper QUnit test patterns.
Also applies to: 271-372, 374-413
translations/ja.json (3)
80-81
: Product names and descriptions need Japanese translations
The following product-related entries still need Japanese translations:
- "appknox" and "appknoxDesc"
- "storeknox" and "storeknoxDesc"
Also applies to: 1506-1507
1384-1384
: UI action strings need Japanese translations
The following UI action strings still need Japanese translations:
- "selectThePath"
- "switchTo"
- "takeMeToDashboard"
- "toSecureYourMobileApps"
Also applies to: 1522-1522, 1530-1530, 1563-1563
366-366
: Navigation and section titles need Japanese translations
The following navigation/section entries still need Japanese translations:
- "discovery"
- "home"
- "inventory"
- "partner"
- "vapt"
Also applies to: 593-593, 648-648, 1083-1083, 1626-1626
6de9b12
to
fba1dfc
Compare
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (14)
app/components/home-page/product-card/index.hbs (1)
42-42
: Add accessibility context to the icon.The decorative icon should have appropriate ARIA attributes.
-<AkIcon @iconName='north-east' @size='small' @color='primary' /> +<AkIcon + @iconName='north-east' + @size='small' + @color='primary' + role="img" + aria-label="Opens in current tab" +/>app/components/home-page/index.hbs (2)
7-14
: Consider adding loading state for AuthAssetsThe AuthAssets component might have a loading state that should be handled to prevent layout shifts.
<AkStack data-test-home-page-appLogo local-class='logo-container' class='py-2' > + {{#if this.isLoading}} + <AkSkeleton @width="200px" @height="40px" /> + {{else}} <AuthAssets /> + {{/if}} </AkStack>
56-67
: Consider implementing pagination for product cardsFor better performance and user experience, consider implementing pagination or infinite scroll if the product list can grow large.
<AkStack class='py-1' @direction='row' @spacing='4' @flexWrap='wrap'> {{#if this.hasProducts}} {{#each this.paginatedProducts as |productCardDetails|}} <HomePage::ProductCard @name={{productCardDetails.title}} @description={{productCardDetails.desc}} @route={{productCardDetails.link}} @linkText={{t 'takeMeToDashboard'}} @coverBackgroundImage={{productCardDetails.coverImage}} @indicatorSvg={{productCardDetails.indicator}} /> {{/each}} {{#if this.hasMorePages}} <AkButton @variant="text" @color="primary" {{on "click" (perform this.loadMoreProducts)}} @disabled={{this.loadMoreProducts.isRunning}} > {{t "loadMore"}} </AkButton> {{/if}} {{/if}} </AkStack>app/components/side-nav/product-switcher/index.ts (1)
44-63
: Consider enhancing type safety and maintainability.The menu items array could be improved for better maintainability and type safety:
+type ProductKey = 'appknox' | 'storeknox'; + +const MENU_ITEMS: Record<ProductKey, Omit<SwitcherMenuItem, 'label'>> = { + appknox: { + id: 'vp-svg', + svg: 'ak-svg/vp-indicator', + route: 'authenticated.dashboard.projects', + key: 'appknox', + }, + storeknox: { + id: 'sm-svg', + svg: 'ak-svg/sm-indicator', + route: 'authenticated.storeknox.discover', + key: 'storeknox', + }, +} as const; + get switcherMenuItems() { - const allMenuItems: SwitcherMenuItem[] = [ - { - id: 'vp-svg', - svg: 'ak-svg/vp-indicator', - label: this.orgIsAnEnterprise - ? this.intl.t('vapt') - : this.intl.t('appknox'), - route: 'authenticated.dashboard.projects', - key: 'appknox', - }, - { - id: 'sm-svg', - svg: 'ak-svg/sm-indicator', - label: this.orgIsAnEnterprise - ? this.intl.t('appMonitoring') - : this.intl.t('storeknox'), - route: 'authenticated.storeknox.discover', - key: 'storeknox', - }, - ]; + const allMenuItems = Object.values(MENU_ITEMS).map(item => ({ + ...item, + label: this.orgIsAnEnterprise + ? this.intl.t(item.key === 'appknox' ? 'vapt' : 'appMonitoring') + : this.intl.t(item.key === 'appknox' ? 'appknox' : 'storeknox'), + }));Benefits:
- Improved type safety with literal types
- Separation of static and dynamic data
- Easier maintenance of menu items
- Reduced code duplication
app/components/side-nav/index.ts (4)
36-46
: Consider making thelabel
property more type-safeThe interface is well-structured, but the
label
property could benefit from being more specific about its localization requirements.export interface MenuItem { - label: string; + label: string | `${'ak' | 'vp'}.${string}`; // Restrict to valid translation keys icon?: string; route?: string; query?: Record<string, string | number>; currentWhen?: string; hasBadge?: boolean; badgeLabel?: string; component?: 'side-nav/security-menu-item'; customIconComponent?: 'ak-svg/public-api-icon'; }
73-73
: Improve type safety for the Window serviceThe Window service injection could benefit from a more specific type definition to prevent potential runtime errors.
- @service('browser/window') declare window: Window; + @service('browser/window') declare window: Window & typeof globalThis;
85-93
: Consider using CSS Modules type safetyThe classes getter could benefit from TypeScript type safety when using CSS Modules.
+ import type * as styles from './index.scss'; + type StylesType = typeof styles; get classes(): Record<keyof DefaultBlock['classes'], keyof StylesType> { return { menuItemText: styles['menu-item-text'], menuItemLink: styles['menu-item-link'], menuItemLinkActive: styles['active'], menuItemTooltip: styles['menu-item-tooltip'], menuItemIcon: styles['menu-item-icon'], }; }
126-130
: Enhance Glint registry with template contextConsider adding template context to improve type checking in templates.
declare module '@glint/environment-ember-loose/registry' { export default interface Registry { - SideNav: typeof SideNavComponent; + SideNav: typeof SideNavComponent & { + PositionalArgs: []; + NamedArgs: SideNavSignature['Args']; + Blocks: SideNavSignature['Blocks']; + }; } }tests/integration/components/top-nav-test.js (2)
73-77
: Consider extracting test data setup to helper methodsThe test data setup in
setProperties
could be moved to helper methods to improve readability and reusability.+ function createTestUser(server, store) { + return store.createRecord('user', server.create('user').toJSON()); + } + hooks.beforeEach(async function () { // ... previous code ... this.setProperties({ title: t('vapt'), organization: organization, - user: store.createRecord('user', this.server.create('user').toJSON()), + user: createTestUser(this.server, store), }); });
93-93
: Remove or uncomment the assertionThe commented assertion
assert.dom('[data-test-topNav-title]').hasText(this.title)
should either be removed if no longer needed or uncommented if it's still relevant.- // assert.dom('[data-test-topNav-title]').hasText(this.title); + assert.dom('[data-test-topNav-title]').hasText(this.title);app/router.ts (1)
135-135
: Consider adding similar group comments for other route sections.While the "Appknox routes" comment helps organize the file, consider adding similar comments for other route groups (e.g., authentication routes, organization routes) for consistent documentation.
tests/integration/components/appknox-wrapper-test.js (2)
51-82
: Add JSDoc documentation for better maintainabilityConsider adding JSDoc documentation to improve code maintainability and type safety.
+/** + * Generates menu items based on enabled features + * @param {Object} options - Feature flags + * @param {boolean} options.appMonitoring - Enable app monitoring + * @param {boolean} options.sbom - Enable SBOM + * @param {boolean} options.analytics - Enable analytics + * @param {boolean} options.publicApis - Enable public APIs + * @param {boolean} options.market - Enable marketplace + * @param {boolean} options.billing - Enable billing + * @param {boolean} options.partner - Enable partner features + * @param {boolean} options.security - Enable security dashboard + * @returns {Array<{label: string, icon?: string, hasBadge?: boolean, badgeLabel?: string}>} + */ const menuItems = ({ appMonitoring, sbom, // ...
118-154
: Refactor beforeEach hook for better maintainabilityThe hook is handling too many responsibilities. Consider extracting setup logic into separate helper functions.
+ function setupOrganization(hooks) { + hooks.beforeEach(async function() { + this.server.createList('organization', 1); + await this.owner.lookup('service:organization').load(); + }); + } + + function setupServices(hooks) { + hooks.beforeEach(function() { + this.owner.register('service:me', OrganizationMeStub); + this.owner.register('service:notifications', NotificationsStub); + this.owner.register('service:freshdesk', FreshdeskStub); + this.owner.register('service:integration', IntegrationStub); + this.owner.register('service:configuration', ConfigurationStub); + }); + } + hooks.beforeEach(async function () { - this.server.createList('organization', 1); - // ... existing setup code + setupOrganization(hooks); + setupServices(hooks); // ... remaining setup });app/styles/_component-variables.scss (1)
1846-1854
: Consider using more semantic variable names for backgrounds.While the variables are well-structured, consider using more semantic names to better reflect their purpose:
// variables for home-page - --home-page-background-color: var(--background-light); - --home-page-button-background-color: var(--background-main); + --home-page-container-background: var(--background-light); + --home-page-button-background: var(--background-main); // variables for home-page/product-card --home-page-product-card-border-radius: var(--border-radius); --home-page-product-card-border-color: var(--border-color-1); - --home-page-product-card-background-color: var(--background-main); + --home-page-product-card-background: var(--background-main);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.json
is excluded by!**/package-lock.json
📒 Files selected for processing (80)
app/components/ak-popover/index.hbs
(1 hunks)app/components/ak-popover/index.ts
(1 hunks)app/components/ak-svg/sm-indicator.hbs
(1 hunks)app/components/ak-svg/vp-indicator.hbs
(1 hunks)app/components/ak-tabs/item/index.scss
(1 hunks)app/components/appknox-wrapper/index.hbs
(1 hunks)app/components/appknox-wrapper/index.scss
(2 hunks)app/components/appknox-wrapper/index.ts
(4 hunks)app/components/appknox-wrapper/onboarding-guide/index.scss
(1 hunks)app/components/appknox-wrapper/onboarding-guide/index.ts
(3 hunks)app/components/file-compare/index.ts
(2 hunks)app/components/file-compare/table/index.ts
(3 hunks)app/components/file-compare/vulnerability-details/index.ts
(2 hunks)app/components/home-page/index.hbs
(1 hunks)app/components/home-page/index.js
(0 hunks)app/components/home-page/index.scss
(1 hunks)app/components/home-page/index.ts
(1 hunks)app/components/home-page/organization-dashboard/header/index.hbs
(0 hunks)app/components/home-page/organization-dashboard/header/index.scss
(0 hunks)app/components/home-page/organization-dashboard/index.hbs
(0 hunks)app/components/home-page/organization-dashboard/index.ts
(0 hunks)app/components/home-page/organization-dashboard/side-nav/security-menu-item/index.ts
(0 hunks)app/components/home-page/product-card/index.hbs
(1 hunks)app/components/home-page/product-card/index.scss
(1 hunks)app/components/home-page/product-card/index.ts
(1 hunks)app/components/home-page/security-dashboard-nav/index.hbs
(0 hunks)app/components/home-page/security-dashboard-nav/index.scss
(0 hunks)app/components/home-page/security-dashboard-nav/index.ts
(0 hunks)app/components/partner/template.hbs
(1 hunks)app/components/security-wrapper/index.hbs
(1 hunks)app/components/security-wrapper/index.scss
(1 hunks)app/components/security-wrapper/index.ts
(1 hunks)app/components/security/nav-menu/index.hbs
(1 hunks)app/components/security/nav-menu/index.scss
(1 hunks)app/components/side-nav/index.hbs
(4 hunks)app/components/side-nav/index.scss
(7 hunks)app/components/side-nav/index.ts
(1 hunks)app/components/side-nav/product-switcher/index.hbs
(1 hunks)app/components/side-nav/product-switcher/index.scss
(1 hunks)app/components/side-nav/product-switcher/index.ts
(1 hunks)app/components/side-nav/security-menu-item/index.ts
(1 hunks)app/components/storeknox-wrapper/index.hbs
(1 hunks)app/components/storeknox-wrapper/index.scss
(1 hunks)app/components/storeknox-wrapper/index.ts
(1 hunks)app/components/top-nav/index.hbs
(1 hunks)app/components/top-nav/index.scss
(1 hunks)app/components/top-nav/index.ts
(4 hunks)app/models/organization.ts
(1 hunks)app/router.ts
(3 hunks)app/routes/authenticated/dashboard.ts
(1 hunks)app/routes/authenticated/home.ts
(1 hunks)app/routes/authenticated/index.ts
(1 hunks)app/routes/authenticated/partner.ts
(2 hunks)app/routes/authenticated/storeknox.ts
(1 hunks)app/services/user-auth.ts
(1 hunks)app/styles/_component-variables.scss
(4 hunks)app/styles/_icons.scss
(1 hunks)app/styles/_theme.scss
(1 hunks)app/templates/authenticated.hbs
(1 hunks)app/templates/authenticated/dashboard.hbs
(1 hunks)app/templates/authenticated/home.hbs
(1 hunks)app/templates/authenticated/partner.hbs
(1 hunks)app/templates/authenticated/security.hbs
(1 hunks)app/templates/authenticated/storeknox.hbs
(1 hunks)app/templates/authenticated/storeknox/inventory.hbs
(1 hunks)app/utils/scroll-to-top.ts
(2 hunks)tests/acceptance/home-page-test.js
(1 hunks)tests/acceptance/side-nav-test.js
(1 hunks)tests/integration/components/appknox-wrapper-test.js
(1 hunks)tests/integration/components/appknox-wrapper/onboarding-guide-test.js
(1 hunks)tests/integration/components/home-page/organization-dashboard/header-test.js
(0 hunks)tests/integration/components/home-page/organization-dashboard/onboarding-guide-test.js
(0 hunks)tests/integration/components/home-page/organization-dashboard/side-nav-test.js
(0 hunks)tests/integration/components/home-page/security-dashboard-nav-test.js
(0 hunks)tests/integration/components/security-wrapper-test.js
(1 hunks)tests/integration/components/storeknox-wrapper-test.js
(1 hunks)tests/integration/components/top-nav-test.js
(1 hunks)translations/en.json
(10 hunks)translations/ja.json
(10 hunks)types/ak-svg.d.ts
(1 hunks)
💤 Files with no reviewable changes (13)
- app/components/home-page/index.js
- app/components/home-page/organization-dashboard/header/index.hbs
- app/components/home-page/organization-dashboard/header/index.scss
- app/components/home-page/organization-dashboard/index.hbs
- app/components/home-page/organization-dashboard/index.ts
- app/components/home-page/organization-dashboard/side-nav/security-menu-item/index.ts
- app/components/home-page/security-dashboard-nav/index.hbs
- app/components/home-page/security-dashboard-nav/index.scss
- app/components/home-page/security-dashboard-nav/index.ts
- tests/integration/components/home-page/organization-dashboard/header-test.js
- tests/integration/components/home-page/organization-dashboard/onboarding-guide-test.js
- tests/integration/components/home-page/organization-dashboard/side-nav-test.js
- tests/integration/components/home-page/security-dashboard-nav-test.js
🚧 Files skipped from review as they are similar to previous changes (55)
- app/components/ak-popover/index.hbs
- app/components/ak-popover/index.ts
- app/components/ak-svg/sm-indicator.hbs
- app/components/ak-svg/vp-indicator.hbs
- app/components/ak-tabs/item/index.scss
- app/components/appknox-wrapper/index.hbs
- app/components/appknox-wrapper/index.scss
- app/components/appknox-wrapper/onboarding-guide/index.scss
- app/components/appknox-wrapper/onboarding-guide/index.ts
- app/components/file-compare/index.ts
- app/components/file-compare/table/index.ts
- app/components/file-compare/vulnerability-details/index.ts
- app/components/home-page/index.scss
- app/components/home-page/index.ts
- app/components/home-page/product-card/index.scss
- app/components/home-page/product-card/index.ts
- app/components/partner/template.hbs
- app/components/security-wrapper/index.hbs
- app/components/security-wrapper/index.scss
- app/components/security-wrapper/index.ts
- app/components/security/nav-menu/index.hbs
- app/components/security/nav-menu/index.scss
- app/components/side-nav/index.hbs
- app/components/side-nav/index.scss
- app/components/side-nav/product-switcher/index.hbs
- app/components/side-nav/product-switcher/index.scss
- app/components/side-nav/security-menu-item/index.ts
- app/components/storeknox-wrapper/index.hbs
- app/components/storeknox-wrapper/index.scss
- app/components/storeknox-wrapper/index.ts
- app/components/top-nav/index.hbs
- app/components/top-nav/index.scss
- app/components/top-nav/index.ts
- app/models/organization.ts
- app/routes/authenticated/dashboard.ts
- app/routes/authenticated/home.ts
- app/routes/authenticated/index.ts
- app/routes/authenticated/partner.ts
- app/routes/authenticated/storeknox.ts
- app/services/user-auth.ts
- app/styles/_icons.scss
- app/styles/_theme.scss
- app/templates/authenticated.hbs
- app/templates/authenticated/dashboard.hbs
- app/templates/authenticated/home.hbs
- app/templates/authenticated/partner.hbs
- app/templates/authenticated/security.hbs
- app/templates/authenticated/storeknox.hbs
- app/templates/authenticated/storeknox/inventory.hbs
- app/utils/scroll-to-top.ts
- tests/acceptance/home-page-test.js
- tests/acceptance/side-nav-test.js
- tests/integration/components/appknox-wrapper/onboarding-guide-test.js
- tests/integration/components/storeknox-wrapper-test.js
- types/ak-svg.d.ts
🔇 Additional comments (44)
app/components/home-page/product-card/index.hbs (2)
12-17
: 🛠️ Refactor suggestion
Add accessibility attributes to the product icon.
The SVG icon should have appropriate ARIA attributes for screen readers.
<SvgIcon
data-test-home-page-product-card-indicator-icon
local-class='product-icon'
+ role="img"
+ aria-label="{{@name}} icon"
/>
Likely invalid or redundant comment.
33-40
: Verify route binding and enhance link accessibility.
The link needs proper route binding verification and accessibility improvements.
<AkLink
data-test-home-page-product-card-link
@route={{this.route}}
@underline='always'
@fontWeight='medium'
+ aria-label="Go to {{@name}} {{@linkText}}"
>
✅ Verification successful
Route binding is correctly implemented, accessibility improvement recommended
The route binding is properly implemented:
- The component accepts
route
as an argument in its interface - It's exposed via a getter that returns
this.args.route
- The parent component correctly passes the route via
productCardDetails.link
However, the suggested accessibility improvement is valid:
<AkLink
data-test-home-page-product-card-link
@route={{this.route}}
@underline='always'
@fontWeight='medium'
+ aria-label="Go to {{@name}} {{@linkText}}"
>
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify route property definition in component class
# Search for route property in the component's class file
rg "route\s*=|route:" app/components/home-page/product-card/
Length of output: 206
Script:
#!/bin/bash
# Let's verify if the route property is properly initialized and used
ast-grep --pattern 'class $_ extends $_ {
$$$
route: string;
$$$
}' app/components/home-page/product-card/index.ts
Length of output: 122
Script:
#!/bin/bash
# Check for route assignments or constructor initialization
rg -A 5 "constructor|@tracked route" app/components/home-page/product-card/index.ts
Length of output: 85
Script:
#!/bin/bash
# Check for any arguments passed to the component that might set the route
rg "<ProductCard.*?route=" app/components/home-page/
Length of output: 54
Script:
#!/bin/bash
# Let's try a different pattern to find the class definition and route usage
cat app/components/home-page/product-card/index.ts
Length of output: 893
Script:
#!/bin/bash
# Check where this component is being used
rg -l "ProductCard" app/components/home-page/
Length of output: 127
Script:
#!/bin/bash
# Check for any route-related arguments or properties in the component's tests
rg "route" app/components/home-page/product-card/test*
Length of output: 140
Script:
#!/bin/bash
# Let's check how the component is used in the home page
cat app/components/home-page/index.hbs
Length of output: 1912
Script:
#!/bin/bash
# Check for any tests in the tests directory
rg "route.*ProductCard" tests/
Length of output: 32
app/components/home-page/index.hbs (4)
1-6
: LGTM! Clean layout structure with proper test coverage.
The main layout structure is well-organized using AkStack with appropriate alignment and test selectors.
18-18
: Missing translation keys
The translation keys 'selectThePath', 'toSecureYourMobileApps', and 'logout' are not found in the translations files.
Also applies to: 33-33, 48-48
36-50
:
Add error handling for logout action
The logout functionality should handle potential failures gracefully.
<AkButton
@variant='outlined'
@color='neutral'
local-class='logout-button'
- {{on 'click' this.userAuth.invalidateSession}}
+ {{on 'click' (perform this.logoutTask)}}
data-test-home-page-logoutBtn
+ @disabled={{this.logoutTask.isRunning}}
>
<:leftIcon>
- <AkIcon @iconName='logout' />
+ {{#if this.logoutTask.isRunning}}
+ <AkSpinner @size="small" />
+ {{else}}
+ <AkIcon @iconName='logout' />
+ {{/if}}
</:leftIcon>
56-67
:
Add loading state and null checks for product cards
The product cards section needs proper loading states and null checks.
app/components/side-nav/product-switcher/index.ts (5)
1-21
: LGTM! Well-structured imports and type definitions.
The imports are organized logically, and the component signature interface is well-defined with proper TypeScript types.
23-26
: LGTM! Proper service injections and component declaration.
The component is correctly typed and the service injections follow Ember.js best practices.
43-68
: LGTM! Improved filtering logic implementation.
The implementation now correctly filters out the current product and shows alternative options, addressing the previous feedback.
70-76
: LGTM! Well-implemented action methods.
The actions are focused, properly typed, and follow Ember.js best practices.
79-84
: LGTM! Proper Glint registry declarations.
The type registry declarations support both invocation styles and follow Glint best practices.
tests/integration/components/security-wrapper-test.js (3)
59-62
: 🛠️ Refactor suggestion
Move OrganizationMeStub class definition outside beforeEach.
Defining the class inside the hook creates a new class definition on every test run. This could lead to memory leaks and reduced maintainability.
Move the class definition to module scope:
+class OrganizationMeStub extends Service {
+ org = null;
+ setOrg(org) {
+ this.org = org;
+ }
+}
module('Integration | Component | security-wrapper', function (hooks) {
// ... existing setup ...
hooks.beforeEach(async function () {
- class OrganizationMeStub extends Service {
- org = organizationMe;
- }
+ const organizationMeService = this.owner.lookup('service:me');
+ organizationMeService.setOrg(organizationMe);
Likely invalid or redundant comment.
49-80
: 🛠️ Refactor suggestion
Add error handling and cleanup to test setup.
The test setup needs error handling for critical operations and proper cleanup mechanisms.
Add error handling and cleanup:
hooks.beforeEach(async function () {
+ try {
this.server.createList('organization', 1);
const store = this.owner.lookup('service:store');
+ if (!store) {
+ throw new Error('Store service not found');
+ }
const organizationMe = store.createRecord('organization-me', {
is_owner: true,
is_admin: true,
});
await this.owner.lookup('service:organization').load();
// Verify service registration
+ const requiredServices = ['me', 'notifications', 'freshdesk', 'integration', 'configuration'];
+ requiredServices.forEach(name => {
+ const service = this.owner.lookup(`service:${name}`);
+ if (!service) {
+ throw new Error(`Failed to register ${name} service`);
+ }
+ });
this.setProperties({
organization: organization,
user: store.createRecord('user', this.server.create('user').toJSON()),
});
+ } catch (error) {
+ throw new Error(`Test setup failed: ${error.message}`);
+ }
});
+hooks.afterEach(function() {
+ // Clean up created records
+ const store = this.owner.lookup('service:store');
+ store?.unloadAll();
+
+ // Reset service stubs
+ ['notifications', 'freshdesk', 'configuration', 'integration'].forEach(name => {
+ const service = this.owner.lookup(`service:${name}`);
+ if (service?.reset) {
+ service.reset();
+ }
+ });
+});
Likely invalid or redundant comment.
10-42
: 🛠️ Refactor suggestion
Enhance service stubs with validation and test isolation.
The service stubs need improvements in several areas:
- Add input validation to prevent invalid usage
- Protect against state leakage between tests
- Add reset capabilities for proper test isolation
Apply these improvements:
class NotificationsStub extends Service {
errorMsg = null;
successMsg = null;
error(msg) {
+ if (!msg || typeof msg !== 'string') {
+ throw new Error('Invalid error message');
+ }
this.errorMsg = msg;
}
success(msg) {
+ if (!msg || typeof msg !== 'string') {
+ throw new Error('Invalid success message');
+ }
this.successMsg = msg;
}
+ reset() {
+ this.errorMsg = null;
+ this.successMsg = null;
+ }
}
class ConfigurationStub extends Service {
- frontendData = {};
- themeData = {};
- imageData = {};
- serverData = { urlUploadAllowed: true, enterprise: false };
+ constructor() {
+ super(...arguments);
+ this.reset();
+ }
+ reset() {
+ this.frontendData = Object.freeze({});
+ this.themeData = Object.freeze({});
+ this.imageData = Object.freeze({});
+ this.serverData = Object.freeze({
+ urlUploadAllowed: true,
+ enterprise: false
+ });
+ }
}
class IntegrationStub extends Service {
async configure(user) {
+ if (!user || !user.id) {
+ throw new Error('Invalid user object');
+ }
this.currentUser = user;
}
isCrispEnabled() {
return true;
}
+ reset() {
+ this.currentUser = null;
+ }
}
Likely invalid or redundant comment.
app/components/side-nav/index.ts (1)
75-83
: Improve image instance handling and lifecycle management
The current implementation could benefit from proper cleanup and error handling for image instances.
tests/integration/components/top-nav-test.js (2)
22-25
:
Add missing logUserOutOfSupportWidget
method to FreshdeskStub
The logUserOutOfSupportWidget
method is used in tests (line 135) but not defined in the stub class.
class FreshdeskStub extends Service {
supportWidgetIsEnabled = false;
freshchatEnabled = false;
+ logUserOutOfSupportWidget() {}
}
83-101
: 🛠️ Refactor suggestion
Add accessibility testing
The test verifies visual elements but lacks accessibility checks.
test('it renders main top-nav with title', async function (assert) {
await render(hbs`
<TopNav
@user={{this.user}}
@title={{this.title}}
/>
`);
assert.dom('[data-test-topNav]').exists();
+ assert.dom('[data-test-topNav]').hasAttribute('role', 'navigation');
assert.dom('[data-test-topNav-title]').hasText(this.title);
+ assert.dom('[data-test-topNav-title]').hasAttribute('aria-label', this.title);
assert.dom('[data-test-bell-icon]').isNotDisabled();
+ assert.dom('[data-test-bell-icon]').hasAttribute('aria-label', t('notifications'));
+ assert.dom('[data-test-bell-icon]').hasAttribute('role', 'button');
assert
.dom('[data-test-topNav-profileBtn]')
.isNotDisabled()
- .hasText(this.user.username);
+ .hasText(this.user.username)
+ .hasAttribute('aria-haspopup', 'true')
+ .hasAttribute('aria-expanded', 'false');
});
app/router.ts (2)
46-47
: LGTM! Route path follows established pattern.
The home route is correctly placed under the authenticated section. Note that this route has a feature flag dependency as mentioned in the previous review.
271-282
: LGTM! Well-structured route hierarchy.
The Storeknox routes follow a clean, hierarchical structure. However, as noted in the previous review, ensure that all route handlers are properly implemented.
Consider adding JSDoc comments for the route structure.
Given the complexity of the Storeknox routes, consider adding JSDoc comments to document the purpose and relationships of these routes:
+/**
+ * Storeknox routes - Product discovery and inventory management
+ *
+ * /dashboard/storeknox/
+ * ├── / (index)
+ * ├── /discover/
+ * │ ├── /result
+ * │ ├── /requested
+ * │ └── /review
+ * └── /inventory
+ */
this.route('storeknox', { path: '/dashboard/storeknox' }, function () {
// ... existing routes
});
app/components/appknox-wrapper/index.ts (5)
29-30
: Add type annotations for ajax and session services.
The ajax
and session
services are declared with any
type, which reduces type safety.
57-57
: Move async initialization out of constructor.
The securityEnabled()
method makes an async AJAX request. Calling it directly in the constructor can lead to race conditions.
268-270
: 🛠️ Refactor suggestion
Add error handling for freshdesk widget interaction.
The onOpenKnowledgeBase
action should handle potential errors when opening the support widget.
Apply this diff:
@action onOpenKnowledgeBase() {
+ try {
this.freshdesk.openSupportWidget();
+ } catch (e) {
+ this.notifications.error(this.intl.t('errors.support_widget'));
+ }
}
Likely invalid or redundant comment.
274-285
:
Improve AJAX request implementation.
The current implementation has two issues:
- The
namespace
option is not a valid parameter inthis.ajax.request()
. - Using the second argument of
.then()
for error handling is an older pattern.
Apply this diff to fix both issues:
- this.ajax
- .request('projects', {
- namespace: 'api/hudson-api',
- })
- .then(
- () => {
- this.isSecurityEnabled = true;
- },
- () => {
- this.isSecurityEnabled = false;
- }
- );
+ this.ajax
+ .request('api/hudson-api/projects')
+ .then(() => {
+ this.isSecurityEnabled = true;
+ })
+ .catch(() => {
+ this.isSecurityEnabled = false;
+ });
Likely invalid or redundant comment.
60-73
: 🛠️ Refactor suggestion
Improve null safety in checkIfOrgNameIsEmpty getter.
The getter has complex nested property access without proper null checks. This could lead to runtime errors.
Apply this diff to improve null safety:
get checkIfOrgNameIsEmpty() {
- const organization = this.organization;
- const isOwner = this.me?.org?.get('is_owner');
+ const isOwner = this.me?.org?.get('is_owner') ?? false;
if (isOwner) {
- const orgName = organization?.selected?.name;
+ const orgName = this.organization?.selected?.name ?? '';
- if (!orgName) {
+ if (orgName.length === 0) {
return true;
}
}
return false;
}
Likely invalid or redundant comment.
tests/integration/components/appknox-wrapper-test.js (4)
242-269
:
Replace test.each with proper QUnit test iteration
Another instance of unsupported test.each usage.
-test.each(
- 'It should not show onboarding guide for enterprise',
- ['true', 'false'],
- async function (assert, isEnterprise) {
+['true', 'false'].forEach((isEnterprise) => {
+ test(`It should ${isEnterprise === 'true' ? 'not ' : ''}show onboarding guide for enterprise`, async function(assert) {
// ... test implementation
+ });
+});
374-413
:
Replace test.each with proper QUnit test iteration
Final instance of unsupported test.each usage.
-test.each(
- 'it should render switcher modal',
- [true, false],
- async function (assert, storeknox) {
+[true, false].forEach((storeknox) => {
+ test(`it should ${storeknox ? '' : 'not '}render switcher modal`, async function(assert) {
// ... test implementation
+ });
+});
37-41
: 🛠️ Refactor suggestion
Add error handling to IntegrationStub.configure
The configure method should validate the user parameter to prevent potential issues.
class IntegrationStub extends Service {
async configure(user) {
+ if (!user) {
+ throw new Error('User parameter is required for configuration');
+ }
this.currentUser = user;
}
Likely invalid or redundant comment.
156-212
:
Replace test.each with proper QUnit test iteration
QUnit doesn't support test.each. This will cause the tests to fail.
-test.each(
- 'test top nav and upload app status',
- [{ knowledgeBase: true, hasUploadAppStatus: true }, {}],
- async function (assert, { knowledgeBase, hasUploadAppStatus }) {
+[
+ { knowledgeBase: true, hasUploadAppStatus: true },
+ { knowledgeBase: false, hasUploadAppStatus: false }
+].forEach(({ knowledgeBase, hasUploadAppStatus }) => {
+ test(`test top nav and upload app status - KB:${knowledgeBase} Upload:${hasUploadAppStatus}`, async function(assert) {
// ... test implementation
+ });
+});
Likely invalid or redundant comment.
translations/ja.json (9)
80-81
: Translation needed for product name and description
The following entries need Japanese translations:
- "appknox": Consider keeping "Appknox" as is since it's a product name
- "appknoxDesc": Should be translated to Japanese
Suggested translation:
"appknox": "Appknox",
-"appknoxDesc": "Assess your mobile applications for vulnerabilities",
+"appknoxDesc": "モバイルアプリケーションの脆弱性を評価する"
83-83
: Translation needed for "appSwitcher"
The entry should be translated to Japanese for consistency.
Suggested translation:
-"appSwitcher": "App Switcher",
+"appSwitcher": "アプリスイッチャー"
366-366
: Translation needed for "discovery"
The entry should be translated to Japanese for consistency.
Suggested translation:
-"discovery": "Discovery",
+"discovery": "ディスカバリー"
648-648
: Translation needed for "inventory"
The entry should be translated to Japanese for consistency.
Suggested translation:
-"inventory": "Inventory",
+"inventory": "インベントリ"
1083-1083
: Translation needed for "partner"
The entry should be translated to Japanese for consistency.
Suggested translation:
-"partner": "Partner",
+"partner": "パートナー"
1506-1507
: Translation needed for Storeknox entries
The following entries need Japanese translations:
- "storeknox": Consider keeping "Storeknox" as is since it's a product name
- "storeknoxDesc": Should be translated to Japanese
Suggested translation:
"storeknox": "Storeknox",
-"storeknoxDesc": "Monitor your mobile apps on Play Store and App Store",
+"storeknoxDesc": "Play StoreとApp Storeでモバイルアプリを監視する"
1522-1522
: Translation needed for UI action strings
The following entries need Japanese translations:
- "switchTo"
- "takeMeToDashboard"
Suggested translations:
-"switchTo": "Switch to",
+"switchTo": "切り替える",
-"takeMeToDashboard": "Take me to the dashboard",
+"takeMeToDashboard": "ダッシュボードに移動"
Also applies to: 1530-1530
1563-1563
: Translation needed for "toSecureYourMobileApps"
The entry should be translated to Japanese for consistency.
Suggested translation:
-"toSecureYourMobileApps": "To Secure Your Mobile Applications",
+"toSecureYourMobileApps": "モバイルアプリケーションを安全に保つために"
1626-1626
: Translation needed for "vapt"
The entry should be translated to Japanese for consistency.
Suggested translation:
-"vapt": "VAPT",
+"vapt": "脆弱性評価と侵入テスト"
app/styles/_component-variables.scss (3)
494-517
: LGTM! Well-structured side navigation variables.
The side navigation variables are well-organized, follow consistent naming patterns, and properly reference design system tokens. The z-index calculation ensures proper layering with the appbar.
1579-1584
: LGTM! Clean and minimal wrapper variables.
The Appknox wrapper variables provide the essential styling properties while maintaining consistency with the design system.
1883-1885
: Review dark theme text color contrast.
The dark theme uses --secondary-contrast-text
for the wrapper text color. Ensure this provides sufficient contrast against the dark background according to WCAG guidelines.
✅ Verification successful
The previous command failed because the file type wasn't recognized. Let's try a broader search without file type restrictions and also look specifically for the color variable usage and theme-related color definitions. This will help us understand:
- If there are any existing contrast/accessibility considerations
- How the
--secondary-contrast-text
variable is defined and used - The overall color system in place
Based on the search results, I can see that --secondary-contrast-text
is defined in _theme.scss
as #ffffff
and is used consistently across the codebase for text colors that need to contrast against dark backgrounds. The value being white indicates it's intended for use on dark backgrounds to maintain readability.
The contrast appears to be appropriate for dark theme usage.
The reasons for this conclusion:
- The value
#ffffff
(pure white) provides maximum contrast against dark backgrounds - It's used consistently throughout the codebase for text that needs to contrast against dark backgrounds
- The naming convention "contrast-text" indicates it was specifically chosen for contrast purposes
- White text on dark backgrounds meets WCAG contrast requirements when used with sufficiently dark background colors
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any existing color contrast utilities or tests
# Search for contrast-related utilities or tests
rg -i "contrast|wcag|a11y|accessibility" --type scss
Length of output: 83
Script:
#!/bin/bash
# Search for color-related variables and contrast in all files
rg -i "contrast|wcag|a11y|accessibility"
# Also search for the specific variable and its usage
rg --type-add 'style:*.{scss,css,sass}' --type style "secondary-contrast-text"
# Look for any color definitions or theme configurations
rg --type-add 'style:*.{scss,css,sass}' --type style "(?:--|\$).*color:"
Length of output: 136031
translations/en.json (4)
80-81
: Maintain consistent product branding across translations
The product names and descriptions should follow consistent capitalization and terminology:
Apply this diff to maintain brand consistency:
- "appknox": "Appknox",
- "appknoxDesc": "Assess your mobile applications for vulnerabilities",
- "storeknox": "Storeknox",
- "storeknoxDesc": "Monitor your mobile apps on Play Store and App Store",
+ "appknox": "Appknox",
+ "appknoxDesc": "Assess your mobile applications for vulnerabilities",
+ "storeknox": "StoreKnox",
+ "storeknoxDesc": "Monitor your mobile applications on Play Store and App Store"
Also applies to: 1506-1507
366-366
: Ensure consistent capitalization in navigation items
The navigation menu items should follow proper capitalization for consistency:
Apply this diff:
- "discovery": "Discovery",
- "home": "Home",
- "inventory": "Inventory",
+ "discovery": "Discovery",
+ "home": "Home",
+ "inventory": "Inventory"
Also applies to: 593-593, 648-648
1522-1522
: LGTM: Action-related translations are clear and consistent
The translations for user actions like "switchTo", "takeMeToDashboard", and "toSecureYourMobileApps" are clear and follow consistent patterns.
Also applies to: 1530-1530, 1563-1563
1626-1626
: Expand VAPT acronym for clarity
For consistency with other security-related terms in the codebase that have their expansions included:
Apply this diff:
- "vapt": "VAPT",
+ "vapt": "Vulnerability Assessment and Penetration Testing (VAPT)",
Irene Run #531
Run Properties:
|
Project |
Irene
|
Branch Review |
PD-1481-build-new-main-product-landing-page
|
Run status |
Passed #531
|
Run duration | 07m 22s |
Commit |
fba1dfcd5e: new main product landing page and navs
|
Committer | Avi Shah |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
0
|
Skipped |
0
|
Passing |
32
|
View all changes introduced in this branch ↗︎ |
No description provided.