Skip to content

WEB-377-fix/content-render-timing #2722

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

WEB-377-fix/content-render-timing #2722

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
167 changes: 90 additions & 77 deletions src/app/profile/profile.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,86 +1,99 @@
<div class="container m-b-10 layout-row layout-lt-md-column align-end gap-1percent">
<button mat-raised-button color="primary" class="m-r-10" [routerLink]="['/system', 'roles-and-permissions']">
<fa-icon icon="check" class="m-r-10"></fa-icon>
{{ 'labels.buttons.Permissions' | translate }}
</button>
<button mat-raised-button color="primary" class="m-r-10" (click)="changeUserPassword()">
<fa-icon icon="cog" class="m-r-10"></fa-icon>
{{ 'labels.buttons.Change Password' | translate }}
</button>
<!-- Skeleton Loader -->
<div *ngIf="isLoadingTranslations" class="skeleton-wrapper">
<mifosx-skeleton-loader type="profile" [showButtons]="true" [buttonCount]="2" [tableRows]="3" [tableColumns]="2">
</mifosx-skeleton-loader>
</div>
<div class="container layout-column gap-1percent">
<mat-card>
<div class="layout-row-wrap">
<div class="info-box">
<div class="header">
{{ 'labels.inputs.Tenant Id' | translate }}

<!-- Actual Content -->
<div *ngIf="!isLoadingTranslations">
<div class="container m-b-10 layout-row layout-lt-md-column align-end gap-1percent">
<button mat-raised-button color="primary" class="m-r-10" [routerLink]="['/system', 'roles-and-permissions']">
<fa-icon icon="check" class="m-r-10"></fa-icon>
{{ 'labels.buttons.Permissions' | translate }}
</button>
<button mat-raised-button color="primary" class="m-r-10" (click)="changeUserPassword()">
<fa-icon icon="cog" class="m-r-10"></fa-icon>
{{ 'labels.buttons.Change Password' | translate }}
</button>
</div>
<div class="container layout-column gap-1percent">
<mat-card>
<div class="layout-row-wrap">
<div class="info-box">
<div class="header">
{{ 'labels.inputs.Tenant Id' | translate }}
</div>
<div>
{{ tenantIdentifier }}
</div>
</div>
<div>
{{ tenantIdentifier }}
<div class="info-box">
<div class="header">
{{ 'labels.inputs.User Id' | translate }}
</div>
<div>
{{ profileData.userId }}
</div>
</div>
</div>
<div class="info-box">
<div class="header">
{{ 'labels.inputs.User Id' | translate }}
</div>
<div>
{{ profileData.userId }}
</div>
</div>
<div class="info-box">
<div class="header">
{{ 'labels.inputs.User Name' | translate }}
</div>
<div>
{{ profileData.username }}
<div class="info-box">
<div class="header">
{{ 'labels.inputs.User Name' | translate }}
</div>
<div>
{{ profileData.username }}
</div>
</div>
</div>
<div class="info-box">
<div class="header">
{{ 'labels.inputs.Office' | translate }}
<div class="info-box">
<div class="header">
{{ 'labels.inputs.Office' | translate }}
</div>
<div>
{{ profileData.officeName }}
</div>
</div>
<div>
{{ profileData.officeName }}
</div>
</div>
<div class="info-box">
<div class="header">
{{ 'labels.inputs.Status' | translate }}
</div>
<div>
{{ profileData.authenticated ? 'Authenticated' : 'Not Authenticated' }}
</div>
</div>
<div class="info-box">
<div class="header">
{{ 'labels.inputs.Language' | translate }}
<div class="info-box">
<div class="header">
{{ 'labels.inputs.Status' | translate }}
</div>
<div>
{{
profileData.authenticated
? ('labels.states.Authenticated' | translate)
: ('labels.states.Not Authenticated' | translate)
}}
</div>
</div>
<div>
{{ language }}
<div class="info-box">
<div class="header">
{{ 'labels.inputs.Language' | translate }}
</div>
<div>
{{ language }}
</div>
</div>
</div>
</div>
</mat-card>
<mat-card>
<table class="mat-elevation-z1" mat-table [dataSource]="dataSource">
<ng-container matColumnDef="role">
<th mat-header-cell *matHeaderCellDef>
{{ 'labels.inputs.Role' | translate }}
</th>
<td mat-cell *matCellDef="let role">
{{ role.name }}
</td>
</ng-container>
<ng-container matColumnDef="description">
<th mat-header-cell *matHeaderCellDef>
{{ 'labels.inputs.Description' | translate }}
</th>
<td mat-cell *matCellDef="let role">
{{ role.description }}
</td>
</ng-container>
<tr mat-header-row *matHeaderRowDef="displayedColumns"></tr>
<tr mat-row *matRowDef="let row; columns: displayedColumns"></tr>
</table>
</mat-card>
</mat-card>
<mat-card>
<table class="mat-elevation-z1" mat-table [dataSource]="dataSource">
<ng-container matColumnDef="role">
<th mat-header-cell *matHeaderCellDef>
{{ 'labels.inputs.Role' | translate }}
</th>
<td mat-cell *matCellDef="let role">
{{ role.name }}
</td>
</ng-container>
<ng-container matColumnDef="description">
<th mat-header-cell *matHeaderCellDef>
{{ 'labels.inputs.Description' | translate }}
</th>
<td mat-cell *matCellDef="let role">
{{ role.description }}
</td>
</ng-container>
<tr mat-header-row *matHeaderRowDef="displayedColumns"></tr>
<tr mat-row *matRowDef="let row; columns: displayedColumns"></tr>
</table>
</mat-card>
</div>
</div>
34 changes: 28 additions & 6 deletions src/app/profile/profile.component.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,14 @@
.skeleton-wrapper {
display: block;
width: 100%;
}

/* Generic table styles should come before specific ones */
table th,
table td {
border-top: 1px solid var(--border-color, rgb(0 0 0 / 12%));
}

.container {
max-width: 37rem;
padding: 1rem;
Expand Down Expand Up @@ -76,11 +87,6 @@
}
}

.container table th,
.container table td {
border-top: 1px solid var(--border-color, rgb(0 0 0 / 12%));
}

th.mat-header-cell:not(:first-of-type),
td.mat-cell:not(:first-of-type) {
border-left: 1px solid var(--border-color, rgb(0 0 0 / 12%));
Expand All @@ -92,7 +98,6 @@ td.mat-cell:not(:first-of-type) {
border-radius: 6px;
}

// Dark mode styles
:host-context(.dark-theme) {
--border-color: #444;
--card-background: #2d2d2d;
Expand All @@ -107,10 +112,27 @@ td.mat-cell:not(:first-of-type) {
.container {
mat-card {
box-shadow: 0 2px 4px rgb(0 0 0 / 30%);
background-color: var(--card-background);
color: var(--text-color);
}

table {
box-shadow: 0 1px 3px rgb(0 0 0 / 30%);
}
}

.skeleton-wrapper {
background-color: transparent;
}

button {
&.mat-raised-button {
background-color: var(--primary-color, #1976d2);
color: #fff;

&:hover {
background-color: var(--primary-hover-color, #1565c0);
}
}
}
}
29 changes: 27 additions & 2 deletions src/app/profile/profile.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ import { ChangePasswordDialogComponent } from 'app/shared/change-password-dialog
import { SettingsService } from 'app/settings/settings.service';
import { FaIconComponent } from '@fortawesome/angular-fontawesome';
import { STANDALONE_SHARED_IMPORTS } from 'app/standalone-shared.module';
import { TranslateService } from '@ngx-translate/core';
import { SkeletonLoaderComponent } from 'app/shared/skeleton-loader';

/**
* Profile Component.
Expand All @@ -42,7 +44,8 @@ import { STANDALONE_SHARED_IMPORTS } from 'app/standalone-shared.module';
MatHeaderRowDef,
MatHeaderRow,
MatRowDef,
MatRow
MatRow,
SkeletonLoaderComponent
]
})
export class ProfileComponent implements OnInit {
Expand All @@ -59,23 +62,45 @@ export class ProfileComponent implements OnInit {
'description'
];

/** Loading state for translations */
isLoadingTranslations = true;

/**
* @param {AuthenticationService} authenticationService Authentication Service
* @param {UserService} userService Users Service
* @param {Router} router Router
* @param {MatDialog} dialog Mat Dialog
* @param {TranslateService} translateService Translate Service
*/
constructor(
private authenticationService: AuthenticationService,
private settingsService: SettingsService,
private router: Router,
public dialog: MatDialog
public dialog: MatDialog,
private translateService: TranslateService
) {
this.profileData = authenticationService.getCredentials();
}

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(() => {
const elapsedTime = Date.now() - startTime;
const remainingTime = Math.max(0, minDisplayTime - elapsedTime);

if (remainingTime > 0) {
setTimeout(() => {
this.isLoadingTranslations = false;
}, remainingTime);
} else {
this.isLoadingTranslations = false;
}
});
Comment on lines +87 to +103
Copy link

@coderabbitai coderabbitai bot Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

}

/**
Expand Down
1 change: 1 addition & 0 deletions src/app/shared/skeleton-loader/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { SkeletonLoaderComponent } from './skeleton-loader.component';
88 changes: 88 additions & 0 deletions src/app/shared/skeleton-loader/skeleton-loader.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
<!-- Profile Type Skeleton -->
<div *ngIf="type === 'profile'" class="skeleton-loader-profile">
<!-- Buttons -->
<div class="skeleton-buttons" *ngIf="showButtons">
<div
class="skeleton-button"
*ngFor="let i of createArray(buttonCount); let idx = index; trackBy: trackByIndex"
></div>
</div>

<!-- Info Boxes Card -->
<div class="skeleton-card">
<div class="layout-row-wrap">
<div class="skeleton-info-box" *ngFor="let i of createArray(6); let idx = index; trackBy: trackByIndex">
<div class="skeleton-header"></div>
<div class="skeleton-text"></div>
</div>
</div>
</div>

<!-- Table Card -->
<div class="skeleton-card">
<div class="skeleton-table" [style.--columns]="tableColumns">
<div class="skeleton-table-header">
<div
class="skeleton-cell"
*ngFor="let i of createArray(tableColumns); let idx = index; trackBy: trackByIndex"
></div>
</div>
<div class="skeleton-table-row" *ngFor="let i of createArray(tableRows); let idx = index; trackBy: trackByIndex">
<div
class="skeleton-cell"
*ngFor="let j of createArray(tableColumns); let idx2 = index; trackBy: trackByIndex"
></div>
</div>
</div>
</div>
</div>

<!-- Table Type Skeleton -->
<div *ngIf="type === 'table'" class="skeleton-loader-table">
<div class="skeleton-table" [style.--columns]="tableColumns">
<div class="skeleton-table-header">
<div
class="skeleton-cell"
*ngFor="let i of createArray(tableColumns); let idx = index; trackBy: trackByIndex"
></div>
</div>
<div class="skeleton-table-row" *ngFor="let i of createArray(tableRows); let idx = index; trackBy: trackByIndex">
<div
class="skeleton-cell"
*ngFor="let j of createArray(tableColumns); let idx2 = index; trackBy: trackByIndex"
></div>
</div>
</div>
</div>

<!-- Card Type Skeleton -->
<div *ngIf="type === 'card'" class="skeleton-loader-card">
<div class="skeleton-card-item" *ngFor="let i of createArray(items); let idx = index; trackBy: trackByIndex">
<div class="skeleton-card-header">
<div class="skeleton-avatar"></div>
<div class="skeleton-card-title"></div>
</div>
<div class="skeleton-card-content">
<div class="skeleton-line skeleton-line-90"></div>
<div class="skeleton-line skeleton-line-70"></div>
<div class="skeleton-line skeleton-line-85"></div>
</div>
<div class="skeleton-card-footer"></div>
</div>
</div>

<!-- List Type Skeleton -->
<div *ngIf="type === 'list'" class="skeleton-loader-list">
<div class="skeleton-list-item" *ngFor="let i of createArray(items); let idx = index; trackBy: trackByIndex">
<div class="skeleton-avatar"></div>
<div class="skeleton-list-content">
<div class="skeleton-line skeleton-line-60"></div>
<div class="skeleton-line skeleton-line-40"></div>
</div>
</div>
</div>

<!-- Custom Type Skeleton -->
<div *ngIf="type === 'custom'" class="skeleton-loader-custom" [ngClass]="cssClass">
<ng-content></ng-content>
</div>
Loading