Standardize loading state patterns and components #9520
Conversation
- Created `PlanetLoadingSpinnerComponent` for page-level loading. - Refactored `DialogsLoadingService` to use reference counting and added `wrap` utility. - Created `LoadingState` utility with `load` operator. - Standardized loading patterns in Dashboard, Courses, Resources, and Community components. - Added documentation in `src/app/shared/loading-patterns.md`. - Ensured `isLoading` is correctly reset using `finalize` or error handling in observables.
|
Refactored the PR to standardize the loader patterns and their usage in specific situations The 
Note: A new translation round is needed to reflect the new loading texts |
There was a problem hiding this comment.
Pull request overview
This PR standardizes loading state patterns across the application by introducing a reusable PlanetLoadingSpinnerComponent and refactoring loading logic to use consistent patterns with proper cleanup via finalize operators.
Changes:
- Created
PlanetLoadingSpinnerComponentfor consistent loading UI across all pages - Refactored
DialogsLoadingServiceto use reference counting for nested loading states - Standardized loading state management in components using
finalizeand error handling
Reviewed changes
Copilot reviewed 47 out of 48 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/app/shared/planet-loading-spinner.component.ts | New reusable loading spinner component with customizable text |
| src/app/shared/shared-components.module.ts | Registered the new loading spinner component |
| src/app/shared/dialogs/dialogs-loading.service.ts | Added reference counting to prevent premature dialog closure |
| src/app/users/users-table.component.html | Replaced inline loading text with spinner component |
| src/app/teams/teams-view.component.ts | Split loading states and added proper error handling |
| src/app/teams/teams-view.component.html | Added loading spinners for news and member data |
| src/app/surveys/surveys.component.ts | Added conditional dialog loading based on route context |
| src/app/resources/view-resources/resources-view.component.ts | Removed dialog loading service and added error handling |
| src/app/manager-dashboard/reports/reports-detail.component.ts | Added loading states and computed property for summary loading |
| src/app/dashboard/dashboard.component.ts | Refactored to use finalize for loading state cleanup |
| src/app/exams/exams-view.component.ts | Replaced custom spinner state with dialog loading service |
| src/app/courses/courses.component.ts | Added error handler for loading state cleanup |
| src/app/community/community.component.ts | Split single loading state into multiple granular states |
| src/app/chat/chat-sidebar/chat-sidebar.component.ts | Added loading state with proper initialization and cleanup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ).subscribe(tasks => { | ||
| this.tasks = tasks; | ||
| this.setTasks(tasks); |
There was a problem hiding this comment.
The subscription lacks error handling. If any observable in the pipe chain fails, the loading states may not be properly reset. Add an error handler to the subscribe call.
| ).subscribe(tasks => { | |
| this.tasks = tasks; | |
| this.setTasks(tasks); | |
| ).subscribe({ | |
| next: (tasks) => { | |
| this.tasks = tasks; | |
| this.setTasks(tasks); | |
| }, | |
| error: (error) => { | |
| console.error('Error while loading tasks in TeamsViewComponent:', error); | |
| } |
| return of([]); | ||
| } | ||
| this.financesLoading = true; | ||
| this.teamDataLoading = true; |
There was a problem hiding this comment.
The loading state is set to true here but there's no corresponding error handler in the chain starting at line 213 to reset it to false if the observable fails. This could leave the UI in a perpetual loading state.
| get summaryLoading() { | ||
| return this.loginLoading || this.resourcesLoading || | ||
| this.coursesLoading || this.chatLoading || this.voicesLoading || this.progressLoading; | ||
| } |
There was a problem hiding this comment.
This getter is called on every change detection cycle, recalculating the boolean expression each time. Consider computing this value once when the individual loading states change, or use memoization.
| ? this.dateQueryParams.startDate : new Date(new Date().setMonth(new Date().getMonth() - 12)) | ||
| ); | ||
| this.setLoginActivities(); | ||
| this.loginLoading = false; |
There was a problem hiding this comment.
The subscription starting at line 268 lacks an error handler. If the observable fails, loginLoading will remain true, potentially blocking UI updates.
| ).subscribe((responses) => { | ||
| this.setMembers(responses); | ||
| this.isLoading = false; |
There was a problem hiding this comment.
The subscription starting at line 54 lacks error handling. If the observable fails, isLoading will remain true.
| ).subscribe((responses) => { | |
| this.setMembers(responses); | |
| this.isLoading = false; | |
| ).subscribe({ | |
| next: (responses) => { | |
| this.setMembers(responses); | |
| this.isLoading = false; | |
| }, | |
| error: () => { | |
| this.isLoading = false; | |
| } |
| </ng-template> | ||
| </mat-tab> | ||
| <mat-tab [label]="leadersTabLabel" *ngIf="isLoggedIn"> | ||
| <p *ngIf="councillors.length === 0" i18n>No leaders available.</p> |
There was a problem hiding this comment.
This message appears unconditionally when the array is empty, even during initial loading. Consider adding a loading state check to avoid showing 'No leaders available' while data is still being fetched.
| <p *ngIf="councillors.length === 0" i18n>No leaders available.</p> | |
| <p *ngIf="!communityDataLoading && (!councillors || councillors.length === 0)" i18n>No leaders available.</p> |
PlanetLoadingSpinnerComponentfor page-level loading.DialogsLoadingServiceto use reference counting and addedwraputility.LoadingStateutility withloadoperator.src/app/shared/loading-patterns.md.isLoadingis correctly reset usingfinalizeor error handling in observables.https://jules.google.com/session/11740011107361944408