Increase Page Transition Speed #1520
Conversation
WalkthroughThe pull request introduces a comprehensive set of skeleton loader components across various sections of the application, including app monitoring, file comparison, organization settings, and SBOM (Software Bill of Materials). These new components provide enhanced loading states with structured, responsive layouts using Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (27)
🚧 Files skipped from review as they are similar to previous changes (27)
⏰ Context from checks skipped due to timeout of 90000ms (10)
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 (
|
SmitGala
force-pushed
the
PD-1636-Increase-Page-Transition-Speed
branch
from
f1218fa to
8648746
Compare
Deploying irenestaging with
|
| Latest commit: |
b34dd7b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5f0d941e.irenestaging.pages.dev |
| Branch Preview URL: | https://pd-1636-increase-page-transi.irenestaging.pages.dev |
SmitGala
force-pushed
the
PD-1636-Increase-Page-Transition-Speed
branch
from
8648746 to
47392b9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 7
🧹 Nitpick comments (18)
app/templates/authenticated/dashboard/organization-settings/index-loading.hbs (1)
1-1: Consider adding error handling and fallback content.While the skeleton loader implementation is clean, consider adding error handling and fallback content for scenarios where the loader fails to render.
+ {{#if this.hasError}} + <AkAlert @intent="danger"> + {{t "errors.loading_failed"}} + </AkAlert> + {{else}} <Organization::Settings::SkeletonLoader /> + {{/if}}app/templates/authenticated/dashboard/compare/untested-cases-loading.hbs (1)
3-3: Improve i18n handling of loading text.The loading text concatenation with "..." should be part of the translation string for better i18n support.
- <FileCompare::Loader @loadingText='{{t "loading"}}...' /> + <FileCompare::Loader @loadingText={{t "loading_with_ellipsis"}} />Add to your translations file:
// en.yml loading_with_ellipsis: "loading..."app/components/file-compare/skeleton-loader/index.ts (1)
1-3: Add JSDoc documentation for the component.Consider adding JSDoc documentation to describe the component's purpose and usage.
+/** + * Skeleton loader component for file comparison view. + * Displays a loading state while file comparison data is being fetched. + */ export default class FileCompareSkeletonLoaderComponent extends Component {}app/components/app-monitoring/skeleton-loader/index.ts (1)
3-3: Consider adding interface for component arguments.While the component is currently stateless, adding a type interface for potential future arguments would improve maintainability and type safety.
+interface AppMonitoringSkeletonLoaderArgs {} -export default class AppMonitoringSkeletonLoaderComponent extends Component {} +export default class AppMonitoringSkeletonLoaderComponent extends Component<AppMonitoringSkeletonLoaderArgs> {}app/components/sbom/scan-details/skeleton-loader/index.ts (2)
3-3: Consider adding interface for component arguments.Similar to other skeleton loaders, adding a type interface would improve maintainability and type safety.
+interface SbomScanDetailsSkeletonLoaderArgs {} -export default class SbomScanDetailsSkeletonLoaderComponent extends Component {} +export default class SbomScanDetailsSkeletonLoaderComponent extends Component<SbomScanDetailsSkeletonLoaderArgs> {}
1-9: Consider creating a base skeleton loader component.There's significant code duplication across skeleton loader components. Consider creating a base component that these specific loaders can extend.
Example implementation:
// app/components/base/skeleton-loader.ts import Component from '@glimmer/component'; export interface BaseSkeletonLoaderArgs { // Common props like animation type, colors, etc. } export default class BaseSkeletonLoaderComponent extends Component<BaseSkeletonLoaderArgs> {}Then extend it in specific loaders:
import BaseSkeletonLoaderComponent, { BaseSkeletonLoaderArgs } from 'app/components/base/skeleton-loader'; interface SbomScanDetailsSkeletonLoaderArgs extends BaseSkeletonLoaderArgs { // Additional specific props } export default class SbomScanDetailsSkeletonLoaderComponent extends BaseSkeletonLoaderComponent<SbomScanDetailsSkeletonLoaderArgs> {}app/components/sbom/component-details/skeleton-loader/index.ts (1)
1-9: Consider implementing a more robust skeleton loading system.With multiple skeleton loaders following the same pattern, consider implementing a more comprehensive solution:
- Create a base skeleton loader component with common functionality
- Define a shared interface for skeleton loader configurations
- Consider using a factory pattern or decorator for registering skeleton loaders
- Implement common animation and styling utilities
This would improve maintainability, reduce duplication, and make it easier to add new skeleton loaders in the future.
Would you like me to provide a detailed implementation plan for this architectural improvement?
app/components/organization/settings/skeleton-loader/index.ts (1)
9-11: Consider adding type safety for the org property.The implementation looks good! For additional type safety, consider adding a type guard or assertion for the
orgproperty.get isOwnerOrAdmin() { - return this.me.org?.get('is_owner') || this.me.org?.get('is_admin'); + const org = this.me.org; + if (!org) return false; + return org.get('is_owner') || org.get('is_admin'); }app/components/sbom/component-details/skeleton-loader/index.hbs (2)
1-7: Add aria-label for better accessibility.The skeleton loader should indicate its purpose to screen readers.
- <AkSkeleton @width='250px' @height='14px' class='mt-2 mb-4' /> + <AkSkeleton @width='250px' @height='14px' class='mt-2 mb-4' aria-label='Loading component details' />
26-31: Consider extracting magic numbers into named constants.The height and width values could be maintained in a constants file for better maintainability.
Consider creating a constants file:
// constants/skeleton-dimensions.ts export const LOADING_VIEW = { SVG_HEIGHT: '200px', CONTAINER_HEIGHT: '350px' };app/components/app-monitoring/skeleton-loader/index.hbs (1)
39-39: Consider moving the loading text to a dedicated translations file.The loading text template literal could be moved to a translations file for better maintainability.
- @loadingText='{{t "loading"}}...' + @loadingText={{t 'appMonitoring.loading'}}Add to translations file:
# translations/en.yaml appMonitoring: loading: 'Loading...'app/components/file-compare/skeleton-loader/index.hbs (1)
1-1: Consider using responsive widths for skeleton elements.Fixed-width skeletons (
250px,75px, etc.) might not adapt well to different screen sizes. Consider using relative units or percentage-based widths for better responsiveness.-<AkSkeleton @width='250px' @height='16px' class='mt-2 mb-3' /> +<AkSkeleton @width='25%' @height='16px' class='mt-2 mb-3' /> -<AkSkeleton @width='75px' @height='16px' /> +<AkSkeleton @width='10%' @height='16px' /> -<AkSkeleton @width='200px' @height='16px' /> +<AkSkeleton @width='20%' @height='16px' />Also applies to: 13-14, 44-46
app/components/regulatory-preference-organization/index.hbs (1)
11-18: Replace magic number with a meaningful constant.The array iteration uses a magic number (0-4). Consider using a named constant or deriving the count from actual data structure.
-{{#each (array 0 1 2 3 4)}} +{{#each (array 0 1 2 3 4) as |index|}} <AkStack @direction='row' @alignItems='center' data-test-skeleton-item={{index}}>app/components/app-monitoring/details/skeleton-loader/index.hbs (2)
3-84: Consider flattening the component structure.The deep nesting of AkStack components (4+ levels) might affect performance and maintainability. Consider breaking this into smaller, more manageable components.
Consider extracting sections into separate components:
AppMonitoring::Details::SkeletonLoader::OverviewAppMonitoring::Details::SkeletonLoader::MonitoringDetails
51-51: Standardize margin utilities.Inconsistent use of margin classes (
mr-2,ml-2). Consider using a consistent approach for spacing.-<AkSkeleton @width='145px' @height='16px' class='mr-2' /> +<AkSkeleton @width='145px' @height='16px' class='me-2' /> -<AkSkeleton @width='145px' @height='16px' class='mr-2' /> +<AkSkeleton @width='145px' @height='16px' class='me-2' /> -<AkSkeleton @width='145px' @height='16px' class='mr-2' /> +<AkSkeleton @width='145px' @height='16px' class='me-2' />Also applies to: 57-57, 71-71
app/components/organization/settings/skeleton-loader/index.hbs (2)
80-96: Extract repeated skeleton pattern into a component.The skeleton pattern inside the each loop could be extracted into a reusable component to reduce duplication and improve maintainability.
Consider creating a new component
Organization::Settings::SkeletonLoader::Rowfor the repeated pattern.
83-84: Standardize styling approach.Mixed usage of Tailwind (
w-4/12,w-8/12) and local classes. Consider standardizing the styling approach for better maintainability.Choose either Tailwind or local classes consistently throughout the component.
Also applies to: 90-91
app/styles/_component-variables.scss (1)
707-720: Consider consolidating common skeleton loader variables.The skeleton loader variables are duplicated across different components. Consider creating common base variables for shared properties:
+ // Base skeleton loader variables + --skeleton-loader-base-border-color: var(--border-color-1); + --skeleton-loader-base-border-radius: var(--border-radius); + --skeleton-loader-base-background: var(--background-main); // Component-specific overrides - --appmonitoring-skeleton-loader-header-border-color: var(--neutral-white-100); - --appmonitoring-skeleton-loader-header-border-radius: var(--border-radius); + --appmonitoring-skeleton-loader-header-border-color: var(--skeleton-loader-base-border-color); + --appmonitoring-skeleton-loader-header-border-radius: var(--skeleton-loader-base-border-radius);This would:
- Reduce duplication
- Make it easier to maintain consistent styling
- Allow for component-specific overrides when needed
Also applies to: 798-805, 849-856, 1949-1959
🛑 Comments failed to post (7)
app/components/sbom/scan-details/skeleton-loader/index.hbs (1)
1-35: 🛠️ Refactor suggestion
Consider creating a shared base skeleton loader component.
The structure is very similar to
component-details/skeleton-loader. Consider extracting common patterns into a shared base component to reduce duplication and improve maintainability.Example approach:
// components/sbom/shared/base-skeleton-loader/index.ts interface BaseSkeletonLoaderArgs { headerWidth?: string; headerHeight?: string; contentConfig: Array<{ width: string; height: string; className?: string; }>; loadingViewHeight?: string; } // Usage in both components: <Sbom::Shared::BaseSkeletonLoader @headerWidth='250px' @headerHeight='14px' @contentConfig={{array (hash width='100px' height='20px') (hash width='250px' height='14px') }} @loadingViewHeight='350px' />app/components/app-monitoring/skeleton-loader/index.hbs (1)
4-4:
⚠️ Potential issueFix typo in class names.
There's a typo in the class names: 'moitoring' should be 'monitoring'.
- local-class='header-app-moitoring-page' + local-class='header-app-monitoring-page' - local-class='header-app-moitoring-page-title' + local-class='header-app-monitoring-page-title' - local-class='header-app-moitoring-page-sorting-filter' + local-class='header-app-monitoring-page-sorting-filter'Also applies to: 11-11, 30-30
app/components/file-compare/skeleton-loader/index.hbs (1)
67-67: 🛠️ Refactor suggestion
Add aria-live region for better accessibility.
The loading state should be announced to screen readers. Consider wrapping the loader in an aria-live region.
+<div role="status" aria-live="polite"> <FileCompare::Loader @loadingText='{{t "loading"}}...' /> +</div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<div role="status" aria-live="polite"> <FileCompare::Loader @loadingText='{{t "loading"}}...' /> </div>app/components/regulatory-preference-organization/index.hbs (1)
1-19: 🛠️ Refactor suggestion
Add aria-busy attribute for better accessibility.
The loading state should be properly communicated to screen readers.
+<div aria-busy="true" role="status"> {{#if this.fetchOrganizationPreference.isRunning}} ... {{/if}} +</div>Committable suggestion skipped: line range outside the PR's diff.
app/components/organization/settings/skeleton-loader/index.hbs (1)
1-103: 🛠️ Refactor suggestion
Add loading indicator for screen readers.
The skeleton loader should properly communicate its loading state to screen readers.
+<div role="status" aria-busy="true" aria-label={{t "loading_organization_settings"}}> <div local-class='org-settings-container'> ... </div> +</div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<div role="status" aria-busy="true" aria-label={{t "loading_organization_settings"}}> <div local-class='org-settings-container'> {{#if this.me.org.is_admin}} <AkSkeleton @width='175px' @height='20px' class='mb-2' /> <AkStack @direction='column' @spacing='1'> <AkSkeleton @width='300px' @height='14px' /> <AkSkeleton @width='400px' @height='12px' /> </AkStack> <div class='mt-5'> {{#each (array 0 1 2 3 4)}} <AkStack @direction='row' @alignItems='center'> <AkSkeleton @width='18px' @height='18px' class='m-1' /> <AkSkeleton @width='75px' @height='14px' /> </AkStack> {{/each}} </div> <AkDivider class='my-3' @color='dark' /> <AkSkeleton @width='175px' @height='20px' class='mb-2' /> <AkStack @direction='column' @spacing='1'> <AkSkeleton @width='300px' @height='14px' /> <AkStack @direction='column' @spacing='0.5'> <AkSkeleton @width='400px' @height='12px' /> <AkSkeleton @width='400px' @height='12px' /> </AkStack> </AkStack> <AkStack @spacing='1' class='my-4'> <AkSkeleton @width='35px' @height='16px' /> <AkSkeleton @width='200px' @height='16px' /> </AkStack> <AkSkeleton @width='450px' @height='50px' class='mb-2' /> <AkDivider class='my-3' @color='dark' /> {{/if}} <AkSkeleton @width='175px' @height='20px' class='mb-2' /> <AkStack @direction='column' @spacing='1'> <AkSkeleton @width='300px' @height='14px' /> <AkStack @direction='column' @spacing='0.5'> <AkSkeleton @width='400px' @height='12px' /> <AkSkeleton @width='400px' @height='12px' /> </AkStack> </AkStack> <div class='mt-4'> <div local-class='bordered-box'> <div class='p-2'> <AkStack @direction='column' @spacing='1'> <AkSkeleton @width='100px' @height='14px' /> <AkSkeleton @width='300px' @height='12px' /> </AkStack> </div> <AkDivider class='my-0' @color='dark' /> <div class='m-3'> <AkStack @spacing='3'> <AkSkeleton @width='140px' @height='20px' /> <AkSkeleton @width='140px' @height='20px' /> </AkStack> </div> <div class='m-2'> <div local-class='bordered-box'> {{#each (array 0 1 2)}} <AkStack local-class='full-bordered-box-section'> <div class='w-4/12' local-class='full-bordered-box-section-column' > <AkSkeleton @width='140px' @height='18px' /> </div> <div class='w-8/12' local-class='full-bordered-box-section-column' > <AkSkeleton @width='140px' @height='18px' /> </div> </AkStack> {{/each}} </div> </div> </div> </div> <AkDivider class='my-3' @color='dark' /> </div> </div>app/components/app-monitoring/skeleton-loader/index.scss (1)
1-9:
⚠️ Potential issueFix typos in class names.
The class names contain typos that should be corrected:
-.header-app-moitoring-page { +.header-app-monitoring-page { border: 1px solid var(--appmonitoring-skeleton-loader-header-border-color); border-radius: var(--appmonitoring-skeleton-loader-header-border-radius); padding-top: 0.714em; - .header-app-moitoring-page-title { + .header-app-monitoring-page-title { padding: 1em 1.285em; } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements..header-app-monitoring-page { border: 1px solid var(--appmonitoring-skeleton-loader-header-border-color); border-radius: var(--appmonitoring-skeleton-loader-header-border-radius); padding-top: 0.714em; .header-app-monitoring-page-title { padding: 1em 1.285em; } }app/components/app-monitoring/details/skeleton-loader/index.scss (1)
27-31: 🛠️ Refactor suggestion
Consider using responsive grid columns.
The current implementation uses fixed-width columns which may not be responsive:
.monitoring-details-group { width: 100%; display: grid; - grid-template-columns: repeat(2, 400px); + grid-template-columns: repeat(2, 1fr);This change will make the columns responsive, each taking up 50% of the container width.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements..monitoring-details-group { width: 100%; display: grid; grid-template-columns: repeat(2, 1fr);
SmitGala
force-pushed
the
PD-1636-Increase-Page-Transition-Speed
branch
2 times, most recently
from
76b2d2f to
b9de3b4
Compare
SmitGala
force-pushed
the
PD-1636-Increase-Page-Transition-Speed
branch
from
b9de3b4 to
90db16d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/styles/_component-variables.scss (1)
707-710: Consider creating common skeleton loader variables.Currently, each component has its own set of skeleton loader variables with similar properties. Consider creating common skeleton loader variables that can be reused across components to improve maintainability:
+ // Common skeleton loader variables + --skeleton-loader-border-radius: var(--border-radius); + --skeleton-loader-border-color: var(--border-color-1); + --skeleton-loader-background-color: var(--background-main); + --skeleton-loader-box-shadow: var(--box-shadow-6); // Component-specific overrides only when needed --appmonitoring-skeleton-loader-header-border-color: var(--skeleton-loader-border-color); --appmonitoring-skeleton-loader-header-border-radius: var(--skeleton-loader-border-radius);This approach would:
- Reduce duplication
- Make it easier to maintain consistent styling
- Allow component-specific overrides when needed
Also applies to: 715-720, 798-805, 1319-1323, 1949-1959
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
app/components/app-monitoring/details/skeleton-loader/index.hbs(1 hunks)app/components/app-monitoring/details/skeleton-loader/index.scss(1 hunks)app/components/app-monitoring/details/skeleton-loader/index.ts(1 hunks)app/components/app-monitoring/skeleton-loader/index.hbs(1 hunks)app/components/app-monitoring/skeleton-loader/index.scss(1 hunks)app/components/app-monitoring/skeleton-loader/index.ts(1 hunks)app/components/file-compare/skeleton-loader/index.hbs(1 hunks)app/components/file-compare/skeleton-loader/index.scss(1 hunks)app/components/file-compare/skeleton-loader/index.ts(1 hunks)app/components/organization/settings/skeleton-loader/index.hbs(1 hunks)app/components/organization/settings/skeleton-loader/index.scss(1 hunks)app/components/organization/settings/skeleton-loader/index.ts(1 hunks)app/components/regulatory-preference-organization/index.hbs(1 hunks)app/components/sbom/component-details/skeleton-loader/index.hbs(1 hunks)app/components/sbom/component-details/skeleton-loader/index.scss(1 hunks)app/components/sbom/component-details/skeleton-loader/index.ts(1 hunks)app/components/sbom/scan-details/skeleton-loader/index.hbs(1 hunks)app/components/sbom/scan-details/skeleton-loader/index.scss(1 hunks)app/components/sbom/scan-details/skeleton-loader/index.ts(1 hunks)app/styles/_component-variables.scss(5 hunks)app/templates/authenticated/dashboard/app-monitoring/index-loading.hbs(1 hunks)app/templates/authenticated/dashboard/app-monitoring/monitoring-details-loading.hbs(1 hunks)app/templates/authenticated/dashboard/compare-loading.hbs(1 hunks)app/templates/authenticated/dashboard/compare/untested-cases-loading.hbs(1 hunks)app/templates/authenticated/dashboard/organization-settings/index-loading.hbs(1 hunks)app/templates/authenticated/dashboard/sbom/component-details-loading.hbs(1 hunks)app/templates/authenticated/dashboard/sbom/scan-details-loading.hbs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (26)
- app/templates/authenticated/dashboard/app-monitoring/monitoring-details-loading.hbs
- app/templates/authenticated/dashboard/organization-settings/index-loading.hbs
- app/components/sbom/scan-details/skeleton-loader/index.ts
- app/templates/authenticated/dashboard/sbom/scan-details-loading.hbs
- app/templates/authenticated/dashboard/compare-loading.hbs
- app/components/sbom/scan-details/skeleton-loader/index.scss
- app/components/sbom/component-details/skeleton-loader/index.scss
- app/components/organization/settings/skeleton-loader/index.hbs
- app/templates/authenticated/dashboard/sbom/component-details-loading.hbs
- app/components/file-compare/skeleton-loader/index.ts
- app/components/app-monitoring/skeleton-loader/index.ts
- app/components/app-monitoring/details/skeleton-loader/index.ts
- app/components/organization/settings/skeleton-loader/index.scss
- app/components/sbom/component-details/skeleton-loader/index.ts
- app/components/file-compare/skeleton-loader/index.scss
- app/components/app-monitoring/details/skeleton-loader/index.hbs
- app/templates/authenticated/dashboard/app-monitoring/index-loading.hbs
- app/components/app-monitoring/skeleton-loader/index.hbs
- app/components/app-monitoring/skeleton-loader/index.scss
- app/components/file-compare/skeleton-loader/index.hbs
- app/components/sbom/scan-details/skeleton-loader/index.hbs
- app/templates/authenticated/dashboard/compare/untested-cases-loading.hbs
- app/components/regulatory-preference-organization/index.hbs
- app/components/organization/settings/skeleton-loader/index.ts
- app/components/app-monitoring/details/skeleton-loader/index.scss
- app/components/sbom/component-details/skeleton-loader/index.hbs
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: Run Integration Tests - Part 8
- GitHub Check: Run Integration Tests - Part 7
- GitHub Check: Run Integration Tests - Part 6
- GitHub Check: Run Integration Tests - Part 5
- GitHub Check: Run Integration Tests - Part 4
- GitHub Check: Run Integration Tests - Part 3
- GitHub Check: Run Integration Tests - Part 2
- GitHub Check: Run Integration Tests - Part 1
- GitHub Check: Run Acceptance Tests
- GitHub Check: Run Unit Tests
- GitHub Check: Run Linting
- GitHub Check: Publish to Cloudflare Pages
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
app/styles/_component-variables.scss (3)
707-710: LGTM! Well-structured appmonitoring skeleton loader variables.The variables are properly organized, follow consistent naming conventions, and effectively reuse existing variables.
Also applies to: 715-720
1319-1323: LGTM! Well-structured file compare skeleton loader variables.The variables are properly organized, follow consistent naming conventions, and effectively reuse existing variables.
1949-1959: LGTM! Well-structured organization settings skeleton loader variables.The variables are properly organized, follow consistent naming conventions, and effectively reuse existing variables.
SmitGala
force-pushed
the
PD-1636-Increase-Page-Transition-Speed
branch
from
90db16d to
b34dd7b
Compare
|
No description provided.