SF-3743 Fix checking slowdown when a project has many questions

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking-overview/checking-overview.component.spec.ts

@@ -1,6 +1,7 @@
 import { Location } from '@angular/common';
 import { DebugElement, NgZone } from '@angular/core';
 import { ComponentFixture, discardPeriodicTasks, fakeAsync, flush, TestBed, tick } from '@angular/core/testing';
+import { MatDialogRef } from '@angular/material/dialog';
 import { MatExpansionPanel } from '@angular/material/expansion';
 import { By } from '@angular/platform-browser';
 import { ActivatedRoute } from '@angular/router';
@@ -26,7 +27,7 @@ import { createTestProjectUserConfig } from 'realtime-server/lib/esm/scripturefo
 import { TextInfo } from 'realtime-server/lib/esm/scriptureforge/models/text-info';
 import { VerseRefData } from 'realtime-server/lib/esm/scriptureforge/models/verse-ref-data';
 import { of } from 'rxjs';
-import { anything, mock, resetCalls, verify, when } from 'ts-mockito';
+import { anything, instance, mock, resetCalls, verify, when } from 'ts-mockito';
 import { DialogService } from 'xforge-common/dialog.service';
 import { NoticeService } from 'xforge-common/notice.service';
 import { OnlineStatusService } from 'xforge-common/online-status.service';
@@ -51,6 +52,7 @@ import { CheckingOverviewComponent } from './checking-overview.component';
 
 const mockedActivatedRoute = mock(ActivatedRoute);
 const mockedDialogService = mock(DialogService);
+const mockedImportQuestionsDialogRef = mock(MatDialogRef);
 const mockedNoticeService = mock(NoticeService);
 const mockedProjectService = mock(SFProjectService);
 const mockedQuestionsService = mock(CheckingQuestionsService);
@@ -88,13 +90,13 @@ describe('CheckingOverviewComponent', () => {
     }));
 
     it('should not display loading if user is offline', fakeAsync(() => {
-      const env = new TestEnvironment();
-      env.testOnlineStatusService.setIsOnline(false);
-      tick();
-      env.fixture.detectChanges();
+      const env = new TestEnvironment(false);
+      env.onlineStatus = false;
+      expect(env.component.showQuestionsLoadingMessage).toBe(false);
+      expect(env.component.showNoQuestionsMessage).toBe(true);
+      env.waitForQuestions();
       expect(env.loadingQuestionsLabel).toBeNull();
       expect(env.noQuestionsLabel).not.toBeNull();
-      env.waitForQuestions();
     }));
 
     it('should not display "Add question" button for community checker', fakeAsync(() => {
@@ -425,10 +427,7 @@ describe('CheckingOverviewComponent', () => {
       await questionDoc.submitJson0Op(op => {
         op.set(d => d.isArchived, false);
       });
-      env.testOnlineStatusService.setIsOnline(false);
-      env.fixture.detectChanges();
-      tick();
-      env.fixture.detectChanges();
+      env.onlineStatus = false;
       expect(env.loadingArchivedQuestionsLabel).toBeNull();
       expect(env.noArchivedQuestionsLabel).not.toBeNull();
 
@@ -985,6 +984,10 @@ class TestEnvironment {
         projectDoc.submitJson0Op(op => op.set(p => p.texts[textIndex].chapters[chapterIndex].hasAudio, false), false);
       }
     );
+    when(mockedImportQuestionsDialogRef.afterClosed()).thenReturn(of(undefined));
+    when(mockedDialogService.openMatDialog(ImportQuestionsDialogComponent, anything())).thenReturn(
+      instance(mockedImportQuestionsDialogRef)
+    );
     this.setCurrentUser(this.adminUser);
     this.testOnlineStatusService.setIsOnline(true);
 
@@ -1112,6 +1115,8 @@ class TestEnvironment {
     this.testOnlineStatusService.setIsOnline(isOnline);
     tick();
     this.fixture.detectChanges();
+    tick();
+    this.fixture.detectChanges();
   }
 
   get warningSomeActionsUnavailableOffline(): DebugElement {

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking-overview/checking-overview.component.ts

@@ -1,5 +1,6 @@
 import { NgClass } from '@angular/common';
-import { Component, DestroyRef, OnDestroy, OnInit } from '@angular/core';
+import { ChangeDetectionStrategy, ChangeDetectorRef, Component, DestroyRef, OnDestroy, OnInit } from '@angular/core';
+import { takeUntilDestroyed } from '@angular/core/rxjs-interop';
 import { MatButton, MatIconButton, MatMiniFabButton } from '@angular/material/button';
 import { MatCard, MatCardContent } from '@angular/material/card';
 import {
@@ -20,8 +21,8 @@ import { Operation } from 'realtime-server/lib/esm/common/models/project-rights'
 import { SF_PROJECT_RIGHTS, SFProjectDomain } from 'realtime-server/lib/esm/scriptureforge/models/sf-project-rights';
 import { Chapter, TextInfo } from 'realtime-server/lib/esm/scriptureforge/models/text-info';
 import { toVerseRef, VerseRefData } from 'realtime-server/lib/esm/scriptureforge/models/verse-ref-data';
-import { asyncScheduler, merge, Subscription } from 'rxjs';
-import { map, tap, throttleTime } from 'rxjs/operators';
+import { asyncScheduler, combineLatest, merge, Subscription } from 'rxjs';
+import { map, startWith, tap, throttleTime } from 'rxjs/operators';
 import { DataLoadingComponent } from 'xforge-common/data-loading-component';
 import { DialogService } from 'xforge-common/dialog.service';
 import { DonutChartComponent } from 'xforge-common/donut-chart/donut-chart.component';
@@ -73,22 +74,26 @@ import { QuestionDialogService } from '../question-dialog/question-dialog.servic
     MatCard,
     MatCardContent,
     L10nNumberPipe
-  ]
+  ],
+  changeDetection: ChangeDetectionStrategy.OnPush
 })
 export class CheckingOverviewComponent extends DataLoadingComponent implements OnInit, OnDestroy {
   texts: TextInfo[] = [];
   projectId?: string;
+  questionsLoaded: boolean = false;
 
   private questionDocs = new Map<string, QuestionDoc[]>();
   private textsByBookId?: TextsByBookId;
   private projectDoc?: SFProjectProfileDoc;
   private dataChangesSub?: Subscription;
+  private questionsLoadedSub?: Subscription;
   private projectUserConfigDoc?: SFProjectUserConfigDoc;
   private questionsQuery?: RealtimeQuery<QuestionDoc>;
 
   constructor(
     private readonly destroyRef: DestroyRef,
     private readonly activatedRoute: ActivatedRoute,
+    private readonly changeDetector: ChangeDetectorRef,
     private readonly dialogService: DialogService,
     noticeService: NoticeService,
     readonly i18n: I18nService,
@@ -207,11 +212,6 @@ export class CheckingOverviewComponent extends DataLoadingComponent implements O
     return this.questionsQuery.docs.filter(qd => qd.data != null && !qd.data.isArchived);
   }
 
-  private get questionsLoaded(): boolean {
-    // if the user is offline, 'ready' will never be true, but the query will still return the offline docs
-    return !this.onlineStatusService.isOnline || this.questionsQuery?.ready === true;
-  }
-
   ngOnInit(): void {
     let projectDocPromise: Promise<SFProjectProfileDoc>;
     const projectId$ = this.activatedRoute.params.pipe(
@@ -223,6 +223,7 @@ export class CheckingOverviewComponent extends DataLoadingComponent implements O
     );
     projectId$.pipe(quietTakeUntilDestroyed(this.destroyRef)).subscribe(async projectId => {
       this.loadingStarted();
+      this.changeDetector.markForCheck();
       this.projectId = projectId;
       try {
         this.projectDoc = await projectDocPromise;
@@ -238,14 +239,13 @@ export class CheckingOverviewComponent extends DataLoadingComponent implements O
         this.loadingFinished();
       }
 
-      if (this.dataChangesSub != null) {
-        this.dataChangesSub.unsubscribe();
-      }
+      this.dataChangesSub?.unsubscribe();
       this.dataChangesSub = merge(
         this.projectDoc.remoteChanges$,
         this.questionsQuery.remoteChanges$,
         this.questionsQuery.localChanges$
       )
+        .pipe(quietTakeUntilDestroyed(this.destroyRef))
         // TODO Find a better solution than merely throttling remote changes
         .pipe(throttleTime(1000, asyncScheduler, { leading: true, trailing: true }))
         .subscribe(() => {
@@ -255,11 +255,23 @@ export class CheckingOverviewComponent extends DataLoadingComponent implements O
             }
           }
         });
+
+      this.questionsLoadedSub?.unsubscribe();
+      this.questionsLoadedSub = combineLatest([
+        this.onlineStatusService.onlineStatus$.pipe(startWith(this.onlineStatusService.isOnline)),
+        this.questionsQuery.ready$.pipe(startWith(this.questionsQuery?.ready))
+      ])
+        .pipe(quietTakeUntilDestroyed(this.destroyRef))
+        .pipe(map(([isOnline, ready]) => !isOnline || ready === true))
+        .subscribe(loaded => {
+          // if the user is offline, 'ready' will never be true, but the query will still return the offline docs
+          this.questionsLoaded = loaded;
+          this.changeDetector.markForCheck();
+        });
     });
   }
 
   ngOnDestroy(): void {
-    this.dataChangesSub?.unsubscribe();
     this.questionsQuery?.dispose();
   }
 
@@ -356,6 +368,8 @@ export class CheckingOverviewComponent extends DataLoadingComponent implements O
           if (questionDoc.data!.isArchived !== archive) this.setQuestionArchiveStatus(questionDoc, archive);
         }
       }
+
+      this.changeDetector.markForCheck();
     }
   }
 
@@ -364,6 +378,8 @@ export class CheckingOverviewComponent extends DataLoadingComponent implements O
       for (const questionDoc of this.getQuestionDocs(this.getTextDocIdType(text.bookNum, chapter.number), !archive)) {
         if (questionDoc.data!.isArchived !== archive) this.setQuestionArchiveStatus(questionDoc, archive);
       }
+
+      this.changeDetector.markForCheck();
     }
   }
 
@@ -454,7 +470,15 @@ export class CheckingOverviewComponent extends DataLoadingComponent implements O
       userId: this.userService.currentUserId,
       textsByBookId: this.textsByBookId
     };
-    this.dialogService.openMatDialog(ImportQuestionsDialogComponent, { data });
+    this.changeDetector.detach();
+    const dialogRef = this.dialogService.openMatDialog(ImportQuestionsDialogComponent, { data });
+    dialogRef
+      .afterClosed()
+      .pipe(takeUntilDestroyed(this.destroyRef))
+      .subscribe(() => {
+        this.changeDetector.reattach();
+        this.changeDetector.markForCheck();
+      });
   }
 
   getBookName(text: TextInfo): string {
@@ -511,6 +535,8 @@ export class CheckingOverviewComponent extends DataLoadingComponent implements O
     for (const questionDoc of this.questionsQuery.docs) {
       this.addQuestionDoc(questionDoc);
     }
+
+    this.changeDetector.markForCheck();
   }
 
   private addQuestionDoc(questionDoc: QuestionDoc): void {

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking.component.spec.ts

@@ -171,7 +171,7 @@
       { provide: ChapterAudioDialogService, useMock: mockedChapterAudioDialogService },
       { provide: FileService, useMock: mockedFileService },
       { provide: OnlineStatusService, useClass: TestOnlineStatusService },
       provideNoopAnimations()
     ]
   }));
 
@@ -455,15 +455,14 @@
       // Question 5 has been stored as the question to start at
       expect(env.component.questionsList!.activeQuestionDoc!.data!.dataId).toBe('q5Id');
       expect(env.questions.length).toEqual(16);
-
-      let question = env.selectQuestion(1);
       // Trigger route change that should happen when activating question from a different book/chapter
       env.setBookChapter('MAT', 1);
+      let question = env.selectQuestion(1);
       expect(env.getQuestionText(question)).toBe('Matthew question relating to chapter 1');
       expect(await env.getCurrentBookAndChapter()).toBe('Matthew 1');
 
-      question = env.selectQuestion(16);
       env.setBookChapter('JHN', 2);
+      question = env.selectQuestion(16);
       expect(env.getQuestionText(question)).toBe('John 2');
       expect(await env.getCurrentBookAndChapter()).toBe('John 2');
       env.waitForQuestionTimersToComplete();
@@ -495,6 +494,7 @@
       env.setBookChapter('JHN', 2);
       env.fixture.detectChanges();
       expect(env.component.questionsList!.activeQuestionDoc!.data!.dataId).toBe('q15Id');
+      tick();
       flush();
       discardPeriodicTasks();
     }));
@@ -654,6 +654,7 @@
       expect(env.component.answersPanel?.getFileSource(questionDoc.data?.audioUrl)).toBeDefined();
       verify(mockedFileService.findOrUpdateCache(FileType.Audio, 'questions', questionId, 'anAudioFile.mp3')).once();
       env.waitForAudioPlayer();
+      tick(100);
       flush();
       discardPeriodicTasks();
     }));
@@ -1731,6 +1732,7 @@
       expect(env.scriptureText).toBe('John 2:2-5');
       env.clickButton(env.saveAnswerButton);
       expect(env.getAnswerScriptureText(0)).toBe('…The selected text(John 2:2-5)');
+      tick(100);
       flush();
       discardPeriodicTasks();
     }));
@@ -2237,6 +2239,7 @@
     it('update answer audio cache after save', fakeAsync(() => {
       const env = new TestEnvironment({ user: CHECKER_USER });
       const questionDoc = spy(env.getQuestionDoc('q6Id'));
+      verify(questionDoc!.updateAnswerFileCache()).never();
       env.selectQuestion(6);
       env.clickButton(env.getAnswerEditButton(0));
       env.waitForSliderUpdate();
@@ -2298,6 +2301,7 @@
       expect(env.getExportAnswerButton(buttonIndex).classes['status-exportable']).toBe(true);
       const questionDoc = env.component.questionsList!.activeQuestionDoc!;
       expect(questionDoc.data!.answers[0].status).toEqual(AnswerStatus.Exportable);
+      tick(100);
       flush();
       discardPeriodicTasks();
     }));
@@ -2312,6 +2316,7 @@
       expect(env.getResolveAnswerButton(buttonIndex).classes['status-resolved']).toBe(true);
       const questionDoc = env.component.questionsList!.activeQuestionDoc!;
       expect(questionDoc.data!.answers[0].status).toEqual(AnswerStatus.Resolved);
+      tick(100);
       flush();
       discardPeriodicTasks();
     }));
@@ -2341,6 +2346,7 @@
       expect(env.getExportAnswerButton(buttonIndex).classes['status-exportable']).toBeUndefined();
       questionDoc = env.component.questionsList!.activeQuestionDoc!;
       expect(questionDoc.data!.answers[0].status).toEqual(AnswerStatus.None);
+      tick(100);
       flush();
       discardPeriodicTasks();
     }));
@@ -2400,6 +2406,7 @@
       env.waitForSliderUpdate();
       tick();
       env.fixture.detectChanges();
+      tick();
       segment = env.getVerse(1, 3);
       expect(segment.classList.contains('question-segment')).toBe(false);
       expect(segment.classList.contains('highlight-segment')).toBe(false);
@@ -2608,7 +2615,7 @@
       discardPeriodicTasks();
     }));
 
-    it('notifies admin if chapter audio is absent and hide scripture text is enabled', fakeAsync(() => {
+    it('notifies admin if chapter audio is absent and hide scripture text is enabled', fakeAsync(async () => {
       const env = new TestEnvironment({
         user: ADMIN_USER,
         projectBookRoute: 'MAT',
@@ -2628,8 +2635,9 @@
           op.set(p => p.texts[matTextIndex].chapters[0].hasAudio, true);
         });
       });
+      env.waitForQuestionTimersToComplete();
 
-      env.component.addAudioTimingData();
+      await env.component.addAudioTimingData();
       env.waitForQuestionTimersToComplete();
       env.fixture.detectChanges();
 
@@ -3454,6 +3462,9 @@
     questionDoc.submitJson0Op(op => op.set(q => q.answers[answerIndex].deleted, true));
 
     this.fixture.detectChanges();
+    tick();
+    this.fixture.detectChanges();
+    tick();
   }
 
   setQuestionFilter(filter: QuestionFilter): void {