Skip to content

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

Merged
RaymondLuong3 merged 7 commits intomasterfrom
fix/SF-3743
Apr 9, 2026
Merged

SF-3743 Fix checking slowdown when a project has many questions#3754
RaymondLuong3 merged 7 commits intomasterfrom
fix/SF-3743

Conversation

@pmachapman
Copy link
Copy Markdown
Collaborator

@pmachapman pmachapman commented Mar 23, 2026

This PR addresses speed issue in community checking by adding an index to the questions in IndexedDB, and by modifying change detection to push for the checking component and checking overview component.


Open with Devin

This change is Reviewable

@pmachapman pmachapman added the will require testing PR should not be merged until testers confirm testing is complete label Mar 23, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 23, 2026

Codecov Report

❌ Patch coverage is 98.68421% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 81.30%. Comparing base (50ffeaf) to head (d652cdb).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...pp/src/app/checking/checking/checking.component.ts 94.11% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3754      +/-   ##
==========================================
+ Coverage   81.27%   81.30%   +0.03%     
==========================================
  Files         622      622              
  Lines       39322    39382      +60     
  Branches     6391     6390       -1     
==========================================
+ Hits        31958    32019      +61     
+ Misses       6379     6378       -1     
  Partials      985      985              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

devin-ai-integration[bot]

This comment was marked as resolved.

@pmachapman pmachapman changed the title SF-3743 Fix checking slowdown when a project has many questions WIP: SF-3743 Fix checking slowdown when a project has many questions Mar 23, 2026
@pmachapman pmachapman marked this pull request as draft March 23, 2026 01:07
@Nateowami Nateowami added the e2e Run e2e tests for this pull request label Mar 23, 2026
@Nateowami
Copy link
Copy Markdown
Collaborator

My testing confirms a dramatic performance improvement. Thanks for tracking this down. IndexedDB indexes weren't even on my radar.

@pmachapman pmachapman marked this pull request as ready for review March 23, 2026 20:30
@pmachapman pmachapman changed the title WIP: SF-3743 Fix checking slowdown when a project has many questions SF-3743 Fix checking slowdown when a project has many questions Mar 23, 2026
Copy link
Copy Markdown
Collaborator

@RaymondLuong3 RaymondLuong3 left a comment

Choose a reason for hiding this comment

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

I was able to see the benefit locally for importing questions. I tested some of the other functionality for community checking and it looks like change detection is not working when playing audio. When I record an play audio, the progress is not updating. Can you look into this?

@RaymondLuong3 reviewed 5 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on pmachapman).


src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking-overview/checking-overview.component.ts line 476 at r2 (raw file):

    dialogRef.afterClosed().subscribe(() => {
      this.changeDetector.reattach();
      this.changeDetector.markForCheck();

I noticed this when I was importing the questions and wondered if that was part of the change. Personally I think this makes sense since the dialog is open so we do not necessary need to be updating the calling component in the background

Code quote:

    dialogRef.afterClosed().subscribe(() => {
      this.changeDetector.reattach();
      this.changeDetector.markForCheck();

src/SIL.XForge.Scripture/ClientApp/src/app/core/models/question-doc.ts line 25 at r2 (raw file):

      [obj<Question>().pathStr(n => n.projectRef)]: 1,
      [obj<Question>().pathStr(n => n.verseRef.bookNum)]: 1,
      [obj<Question>().pathStr(n => n.verseRef.chapterNum)]: 1

So this makes it easier for loading questions by book and chapter because these values are now indexed, is that correct? Does this benefit both in the context of querying for all questions in a book and for a book and chapter? I could not tell based on my testing.

Code quote:

      [obj<Question>().pathStr(n => n.projectRef)]: 1,
      [obj<Question>().pathStr(n => n.verseRef.bookNum)]: 1,
      [obj<Question>().pathStr(n => n.verseRef.chapterNum)]: 1

Copy link
Copy Markdown
Collaborator Author

@pmachapman pmachapman left a comment

Choose a reason for hiding this comment

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

When I record an play audio, the progress is not updating. Can you look into this?

Done. Thank you!

@pmachapman made 3 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on RaymondLuong3).


src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking-overview/checking-overview.component.ts line 476 at r2 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

I noticed this when I was importing the questions and wondered if that was part of the change. Personally I think this makes sense since the dialog is open so we do not necessary need to be updating the calling component in the background

It helped the speed of the import to not refresh the background list of questions so often while an import was occurring. It does still happen due to other change detection updates, but at a much lower rate.


src/SIL.XForge.Scripture/ClientApp/src/app/core/models/question-doc.ts line 25 at r2 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

So this makes it easier for loading questions by book and chapter because these values are now indexed, is that correct? Does this benefit both in the context of querying for all questions in a book and for a book and chapter? I could not tell based on my testing.

Yes. The index will be used if you query by book and chapter, or book, or chapter.

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

Copy link
Copy Markdown
Collaborator

@RaymondLuong3 RaymondLuong3 left a comment

Choose a reason for hiding this comment

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

Thanks! I am still seeing some issues. When I record an answer and stop the recording, the change detection doesn't appear to run so that the audio is not shown as recorded before saving.

@RaymondLuong3 reviewed 4 files and all commit messages, made 2 comments, and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on pmachapman).


src/SIL.XForge.Scripture/ClientApp/src/app/core/models/question-doc.ts line 25 at r2 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Yes. The index will be used if you query by book and chapter, or book, or chapter.

Excellent!

Copy link
Copy Markdown
Collaborator

@RaymondLuong3 RaymondLuong3 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on pmachapman).

Copy link
Copy Markdown
Collaborator Author

@pmachapman pmachapman left a comment

Choose a reason for hiding this comment

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

When I record an answer and stop the recording, the change detection doesn't appear to run so that the audio is not shown as recorded before saving.

I think I have isolated the cause. Does this issues still occur for you?

@pmachapman made 1 comment, resolved 1 discussion, and dismissed @RaymondLuong3 from a discussion.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on pmachapman).

Copy link
Copy Markdown
Collaborator

@RaymondLuong3 RaymondLuong3 left a comment

Choose a reason for hiding this comment

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

Thanks! That looks great!. I see another issue. When attaching scripture to an answer, the change detection doesn't run and closing the dialog does not show which scripture is selected. It seems like we should check every component that is in the component tree of checking.component and make sure change detection is run.

@RaymondLuong3 reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on pmachapman).

Copy link
Copy Markdown
Collaborator Author

@pmachapman pmachapman left a comment

Choose a reason for hiding this comment

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

When attaching scripture to an answer, the change detection doesn't run and closing the dialog does not show which scripture is selected. It seems like we should check every component that is in the component tree of checking.component and make sure change detection is run.

I couldn't recreate this exact scenario, but did notice a lag, so I have added change detection to the answer/comment input scenarios, which it looked like the UI would change.

@pmachapman made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on pmachapman).

devin-ai-integration[bot]

This comment was marked as resolved.

Copy link
Copy Markdown
Collaborator

@RaymondLuong3 RaymondLuong3 left a comment

Choose a reason for hiding this comment

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

Now it is working as I would expected. Let's see what testers spots. Thanks!

@RaymondLuong3 partially reviewed 3 files and made 1 comment.
Reviewable status: 9 of 10 files reviewed, 2 unresolved discussions (waiting on pmachapman).

@RaymondLuong3 RaymondLuong3 added ready to test and removed will require testing PR should not be merged until testers confirm testing is complete labels Mar 30, 2026
devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

Copy link
Copy Markdown
Collaborator

@RaymondLuong3 RaymondLuong3 left a comment

Choose a reason for hiding this comment

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

@RaymondLuong3 reviewed 5 files and all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on pmachapman).

@RaymondLuong3 RaymondLuong3 merged commit 5ffa804 into master Apr 9, 2026
24 checks passed
@RaymondLuong3 RaymondLuong3 deleted the fix/SF-3743 branch April 9, 2026 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

e2e Run e2e tests for this pull request ready to test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants