WEB-377-fix/content-render-timing #2722
Conversation
WalkthroughAdds a reusable SkeletonLoaderComponent (profile/table/list/card/custom) and integrates it into the profile view. Profile now shows the skeleton while translations load, using a 600ms minimum display timer before revealing the final content. SCSS updated with skeleton styles and dark-theme overrides. Changes
Sequence Diagram(s)sequenceDiagram
participant Profile as ProfileComponent
participant Transl as TranslateService
participant Skeleton as SkeletonLoaderComponent
participant UI as Profile UI
Profile->>Profile: set isLoadingTranslations = true\nstart 600ms minimum timer
Profile->>Skeleton: render skeleton (type=profile)
Skeleton-->>UI: show loading placeholders
par Translation fetch
Profile->>Transl: request translation key(s)
Transl-->>Profile: returns translation(s)
and Minimum timer
Profile->>Profile: wait until 600ms elapsed
end
Profile->>Profile: set isLoadingTranslations = false
Skeleton-->>UI: unmount skeleton
Profile-->>UI: render actual profile content
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/profile/profile.component.ts (1)
31-49: Component usesimportsbut lacksstandalone: true.This is a compile-time error in Angular 15+. Add
standalone: true.@Component({ selector: 'mifosx-profile', templateUrl: './profile.component.html', styleUrls: ['./profile.component.scss'], + standalone: true, imports: [Optionally add
changeDetection: ChangeDetectionStrategy.OnPushfor perf.
🧹 Nitpick comments (9)
src/app/profile/profile.component.scss (2)
6-10: Scope generic table borders to the component container.To avoid unintended styling on nested tables, anchor to
.container table th, .container table td.-table th, -table td { +.container table th, +.container table td { border-top: 1px solid var(--border-color, rgb(0 0 0 / 12%)); }
112-137: Dark theme button overrides may fight Material theming.Using explicit colors on
.mat-raised-buttoncan override palette/theming. Prefer Material tokens or CSS vars bound to theme.- button { - &.mat-raised-button { - background-color: var(--primary-color, #1976d2); - color: #fff; - &:hover { background-color: var(--primary-hover-color, #1565c0); } - } - } + .mat-raised-button { + background-color: var(--mifos-primary, var(--primary-color, #1976d2)); + color: var(--mifos-on-primary, #fff); + }src/app/profile/profile.component.html (1)
2-4: Add basic a11y: announce loading state.Expose status to AT and mark busy.
-<div *ngIf="isLoadingTranslations" class="skeleton-wrapper"> - <mifosx-skeleton-loader type="profile" [showButtons]="true" [buttonCount]="2" [tableRows]="3" [tableColumns]="2"> +<div *ngIf="isLoadingTranslations" class="skeleton-wrapper" role="status" aria-live="polite" aria-busy="true"> + <mifosx-skeleton-loader type="profile" [showButtons]="true" [buttonCount]="2" [tableRows]="3" [tableColumns]="2" aria-hidden="true"> </mifosx-skeleton-loader>src/app/shared/skeleton-loader/skeleton-loader.component.scss (1)
291-361: Prefer CSS variables over class overrides for dark mode + reduced motion.You already set CSS vars; with the placeholder change above, you can drop most duplication. Also respect
prefers-reduced-motion.:host-context(.dark-theme) { --skeleton-base-bg: #3a3a3a; --skeleton-card-bg: #2d2d2d; --skeleton-info-box-bg: #383838; --skeleton-border-color: #444; --skeleton-table-header-bg: #333; --skeleton-shimmer-color: rgb(255 255 255 / 10%); - .skeleton-base { ... } - .skeleton-buttons .skeleton-button { ... } + /* With %skeleton-base using vars, most overrides above become unnecessary */ } +@media (prefers-reduced-motion: reduce) { + .skeleton-base::after { animation: none; } +}src/app/shared/skeleton-loader/skeleton-loader.component.ts (1)
10-18: Consider stricter typing and perf defaults.
- Make
typea string-literal union via a reusabletypealias orenum.- Add
ChangeDetectionStrategy.OnPush.-import { Component, Input } from '@angular/core'; +import { Component, Input, ChangeDetectionStrategy } from '@angular/core'; +export type SkeletonType = 'profile' | 'table' | 'list' | 'card' | 'custom'; ... export class SkeletonLoaderComponent { - @Input() type: 'profile' | 'table' | 'list' | 'card' | 'custom' = 'profile'; + @Input() type: SkeletonType = 'profile'; @Input() items: number = 1; @Input() cssClass: string = ''; @Input() showButtons: boolean = true; @Input() buttonCount: number = 2; @Input() tableRows: number = 3; @Input() tableColumns: number = 2; }Also update
@ComponentwithchangeDetection: ChangeDetectionStrategy.OnPush.src/app/profile/profile.component.ts (3)
65-67: Prefer explicit typing for state.Add explicit type to aid readability and strictness.
- isLoadingTranslations = true; + isLoadingTranslations: boolean = true;
85-103: Simplify min-delay + translation readiness with RxJS and ensure completion.
get()completes, but the timer pattern can be expressed declaratively and guards against errors.-// Show skeleton for at least 600ms and wait for translations -const startTime = Date.now(); -const minDisplayTime = 600; -this.translateService.get('labels.inputs.Tenant Id').subscribe(() => { - const elapsedTime = Date.now() - startTime; - const remainingTime = Math.max(0, minDisplayTime - elapsedTime); - if (remainingTime > 0) { - setTimeout(() => { this.isLoadingTranslations = false; }, remainingTime); - } else { - this.isLoadingTranslations = false; - } -}); +// Wait for translation readiness and a minimum 600ms display +const minDisplayTime = 600; +combineLatest([ + this.translateService.get('labels.inputs.Tenant Id').pipe(take(1), catchError(() => of('labels.inputs.Tenant Id'))), + timer(minDisplayTime) +]).subscribe(() => { this.isLoadingTranslations = false; });Add imports:
combineLatest, timer, offromrxjs, andtake, catchErrorfromrxjs/operators.
57-63: Type the table data source.If you have a Role model, use it to improve type safety.
- dataSource = new MatTableDataSource(); + dataSource = new MatTableDataSource<Role>();src/app/shared/skeleton-loader/skeleton-loader.component.html (1)
11-11: Make the info boxes count configurable via component input.Currently, the info boxes count is hardcoded to 6 (line 11). This should be an input property to match the flexibility of other display modes.
Add an input property to the component:
// In skeleton-loader.component.ts @Input() infoBoxes: number = 6;Then update the template:
- <div class="skeleton-info-box" *ngFor="let i of [].constructor(6); let idx = index"> + <div class="skeleton-info-box" *ngFor="let i of [].constructor(infoBoxes); let idx = index">
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/app/profile/profile.component.html(1 hunks)src/app/profile/profile.component.scss(2 hunks)src/app/profile/profile.component.ts(3 hunks)src/app/shared/skeleton-loader/index.ts(1 hunks)src/app/shared/skeleton-loader/skeleton-loader.component.html(1 hunks)src/app/shared/skeleton-loader/skeleton-loader.component.scss(1 hunks)src/app/shared/skeleton-loader/skeleton-loader.component.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/shared/skeleton-loader/skeleton-loader.component.scsssrc/app/shared/skeleton-loader/index.tssrc/app/shared/skeleton-loader/skeleton-loader.component.tssrc/app/profile/profile.component.htmlsrc/app/shared/skeleton-loader/skeleton-loader.component.htmlsrc/app/profile/profile.component.tssrc/app/profile/profile.component.scss
🧬 Code graph analysis (2)
src/app/shared/skeleton-loader/skeleton-loader.component.ts (2)
src/app/profile/profile.component.ts (1)
Component(31-129)src/app/shared/skeleton-loader/index.ts (1)
SkeletonLoaderComponent(1-1)
src/app/profile/profile.component.ts (1)
src/app/shared/skeleton-loader/index.ts (1)
SkeletonLoaderComponent(1-1)
🔇 Additional comments (2)
src/app/shared/skeleton-loader/index.ts (1)
1-1: LGTM: clean barrel export.Re-export looks correct and matches usage
app/shared/skeleton-loader.src/app/shared/skeleton-loader/skeleton-loader.component.html (1)
70-73: Custom type implementation is clean.The passthrough content area via
ng-contentwith CSS class bindings is well-structured and provides good extensibility.
fix/i18n-and-darkmode-skeleton-profile fix: content rendering timing and i18n dark mode skeleton issue in profile
shubhamkumar9199
force-pushed
the
fix/content-render-timing
branch
from
1ab9e2f to
0b8fa32
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/app/shared/skeleton-loader/skeleton-loader.component.html (1)
1-88: Excellent! Previous feedback addressed. Optional: clean up unused index variables.The template now correctly uses
createArray()helper andtrackBy: trackByIndexon all*ngForloops, addressing the previous review feedback.As an optional improvement, you can remove the unused
idxandidx2variables throughout the template since they're declared but never referenced:- *ngFor="let i of createArray(buttonCount); let idx = index; trackBy: trackByIndex" + *ngFor="let i of createArray(buttonCount); trackBy: trackByIndex" - <div class="skeleton-info-box" *ngFor="let i of createArray(6); let idx = index; trackBy: trackByIndex"> + <div class="skeleton-info-box" *ngFor="let i of createArray(6); trackBy: trackByIndex"> - *ngFor="let i of createArray(tableColumns); let idx = index; trackBy: trackByIndex" + *ngFor="let i of createArray(tableColumns); trackBy: trackByIndex" - <div class="skeleton-table-row" *ngFor="let i of createArray(tableRows); let idx = index; trackBy: trackByIndex"> + <div class="skeleton-table-row" *ngFor="let i of createArray(tableRows); trackBy: trackByIndex"> - *ngFor="let j of createArray(tableColumns); let idx2 = index; trackBy: trackByIndex" + *ngFor="let j of createArray(tableColumns); trackBy: trackByIndex" - *ngFor="let i of createArray(tableColumns); let idx = index; trackBy: trackByIndex" + *ngFor="let i of createArray(tableColumns); trackBy: trackByIndex" - <div class="skeleton-table-row" *ngFor="let i of createArray(tableRows); let idx = index; trackBy: trackByIndex"> + <div class="skeleton-table-row" *ngFor="let i of createArray(tableRows); trackBy: trackByIndex"> - *ngFor="let j of createArray(tableColumns); let idx2 = index; trackBy: trackByIndex" + *ngFor="let j of createArray(tableColumns); trackBy: trackByIndex" - <div class="skeleton-card-item" *ngFor="let i of createArray(items); let idx = index; trackBy: trackByIndex"> + <div class="skeleton-card-item" *ngFor="let i of createArray(items); trackBy: trackByIndex"> - <div class="skeleton-list-item" *ngFor="let i of createArray(items); let idx = index; trackBy: trackByIndex"> + <div class="skeleton-list-item" *ngFor="let i of createArray(items); trackBy: trackByIndex">
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/app/profile/profile.component.html(1 hunks)src/app/profile/profile.component.scss(2 hunks)src/app/profile/profile.component.ts(3 hunks)src/app/shared/skeleton-loader/index.ts(1 hunks)src/app/shared/skeleton-loader/skeleton-loader.component.html(1 hunks)src/app/shared/skeleton-loader/skeleton-loader.component.scss(1 hunks)src/app/shared/skeleton-loader/skeleton-loader.component.ts(1 hunks)src/assets/translations/en-US.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/app/shared/skeleton-loader/index.ts
- src/app/shared/skeleton-loader/skeleton-loader.component.scss
- src/app/profile/profile.component.html
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/profile/profile.component.scsssrc/app/shared/skeleton-loader/skeleton-loader.component.tssrc/app/profile/profile.component.tssrc/app/shared/skeleton-loader/skeleton-loader.component.html
🧬 Code graph analysis (2)
src/app/shared/skeleton-loader/skeleton-loader.component.ts (2)
src/app/profile/profile.component.ts (1)
Component(31-129)src/app/shared/skeleton-loader/index.ts (1)
SkeletonLoaderComponent(1-1)
src/app/profile/profile.component.ts (1)
src/app/shared/skeleton-loader/index.ts (1)
SkeletonLoaderComponent(1-1)
🔇 Additional comments (6)
src/assets/translations/en-US.json (1)
2547-2550: Translation keys for authentication states added appropriately.The new
"states"object with"Authenticated"and"Not Authenticated"entries is well-placed and provides necessary i18n support for the skeleton loader's profile view. The JSON structure is valid, key naming is clear and consistent with existing conventions, and the object is properly positioned in the file hierarchy.src/app/profile/profile.component.scss (1)
1-138: LGTM! Clean styling with proper dark theme support.The SCSS changes are well-structured with appropriate use of CSS variables, fallbacks, and dark theme overrides. The skeleton-wrapper styling integrates nicely with the existing profile styles.
src/app/profile/profile.component.ts (3)
25-26: LGTM! Appropriate imports for the new functionality.The TranslateService and SkeletonLoaderComponent imports are correctly added to support the translation-aware loading state.
47-48: LGTM! SkeletonLoaderComponent correctly added to standalone imports.The component is properly added to the imports array since ProfileComponent is standalone.
65-66: LGTM! Loading state property appropriately declared.The public boolean property is correctly initialized and suitable for template binding.
src/app/shared/skeleton-loader/skeleton-loader.component.ts (1)
1-27: LGTM! Well-structured reusable skeleton loader component.The component is correctly implemented as standalone, has properly typed inputs with sensible defaults, and includes the helper methods (
trackByIndex,createArray) that support the template's*ngForloops. The previous feedback regardingstandalone: truehas been addressed.
|
|
||
| // Show skeleton for at least 600ms and wait for translations | ||
| const startTime = Date.now(); | ||
| const minDisplayTime = 600; | ||
|
|
||
| this.translateService.get('labels.inputs.Tenant Id').subscribe(() => { | ||
| const elapsedTime = Date.now() - startTime; | ||
| const remainingTime = Math.max(0, minDisplayTime - elapsedTime); | ||
|
|
||
| if (remainingTime > 0) { | ||
| setTimeout(() => { | ||
| this.isLoadingTranslations = false; | ||
| }, remainingTime); | ||
| } else { | ||
| this.isLoadingTranslations = false; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Clean up subscription and timeout on component destroy.
The translation subscription and setTimeout are not cleaned up, which can cause memory leaks and errors if the component is destroyed before they complete.
Apply this diff to add proper cleanup:
+import { Component, OnInit, OnDestroy } from '@angular/core';
+import { Subject, takeUntil } from 'rxjs';
-export class ProfileComponent implements OnInit {
+export class ProfileComponent implements OnInit, OnDestroy {
+ private destroy$ = new Subject<void>();
+ private loadingTimeout?: number;
/** Loading state for translations */
isLoadingTranslations = true;
ngOnInit() {
this.dataSource = new MatTableDataSource(this.profileData.roles);
// Show skeleton for at least 600ms and wait for translations
const startTime = Date.now();
const minDisplayTime = 600;
- this.translateService.get('labels.inputs.Tenant Id').subscribe(() => {
+ this.translateService.get('labels.inputs.Tenant Id')
+ .pipe(takeUntil(this.destroy$))
+ .subscribe(() => {
const elapsedTime = Date.now() - startTime;
const remainingTime = Math.max(0, minDisplayTime - elapsedTime);
if (remainingTime > 0) {
- setTimeout(() => {
+ this.loadingTimeout = window.setTimeout(() => {
this.isLoadingTranslations = false;
}, remainingTime);
} else {
this.isLoadingTranslations = false;
}
});
}
+ ngOnDestroy() {
+ this.destroy$.next();
+ this.destroy$.complete();
+ if (this.loadingTimeout) {
+ clearTimeout(this.loadingTimeout);
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/app/profile/profile.component.ts around lines 87 to 103, the
translateService subscription and the setTimeout timer are never cleaned up
which can leak memory or trigger after the component is destroyed; store the
Subscription returned from translateService.get(...) and store the timeout id
returned by setTimeout (if any), implement ngOnDestroy to call
subscription.unsubscribe() and clearTimeout(timeoutId) (guarding for undefined),
and ensure isLoadingTranslations is not toggled after destroy by checking a
destroyed flag or relying on the unsubscribe/clearTimeout to prevent callbacks.
2 participants
Description
Implemented a skeleton loader to ensure that the page content displays only after the required data has been fully loaded.
Previously, the content rendered before the data was fetched, causing layout shifts and a poor user experience.
The loader provides a smooth transition while data is being retrieved, making the interface feel more consistent and responsive.
#{Issue Number}
Screenshots, if any
WEB-377
Checklist
before:
https://github.com/user-attachments/assets/d0b39e93-1ad0-4dba-8d89-d0d33311b119
after:
https://github.com/user-attachments/assets/920527a0-bc48-47ee-84b0-58c64d0fec94
Please make sure these boxes are checked before submitting your pull request - thanks!
[ X ] If you have multiple commits please combine them into one commit by squashing them.
[ X ] Read and understood the contribution guidelines at
web-app/.github/CONTRIBUTING.md.Summary by CodeRabbit
New Features
Style
Localization