Skip to content

Conversation

@siltomato
Copy link
Collaborator

@siltomato siltomato commented Jul 29, 2025

This PR drastically improves loading speed and UI responsiveness for projects with very large numbers of insights (> 1000).

  • Added web worker to offload delta formatting operations from the main thread, improving performance and responsiveness.
    • Added npm package Comlink library to improve working with web workers.
    • Added worker named-chunk pattern to SPA routes.
    • Included fallback to main thread processing if worker creation fails.
  • Additionally, when adding the blot attributes to the editor delta, optimized by detecting the overlapping insight ranges and only calling the expensive delta.compose() after those cases and not after every insight (delta.compose() handles the case where insights have the same severity and overlapping ranges).

This change is Reviewable

@siltomato siltomato added the will require testing PR should not be merged until testers confirm testing is complete label Jul 29, 2025
@codecov
Copy link

codecov bot commented Jul 29, 2025

Codecov Report

❌ Patch coverage is 74.40000% with 32 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (master@a55b036). Learn more about missing BASE report.
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...nsights/quill-services/insight-formatting-utils.ts 82.55% 11 Missing and 4 partials ⚠️
...hts/quill-services/quill-insight-render.service.ts 58.82% 14 Missing ⚠️
src/SIL.XForge.Scripture/Startup.cs 40.00% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             master    #3334   +/-   ##
=========================================
  Coverage          ?   82.42%           
=========================================
  Files             ?      607           
  Lines             ?    35537           
  Branches          ?     5784           
=========================================
  Hits              ?    29291           
  Misses            ?     5369           
  Partials          ?      877           

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

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@siltomato siltomato force-pushed the fix/sf-3370-lynx-app-lockup-for-large-projects branch from 254b826 to afef597 Compare July 29, 2025 04:30
@siltomato siltomato marked this pull request as ready for review July 29, 2025 04:45
@RaymondLuong3 RaymondLuong3 self-assigned this Jul 29, 2025
Copy link
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.

This is neat that using a web worker has such an advantage and allows you to offload the formatInsights from the main thread. I was testing this on a project with 18000 insights and there wasn't an issue for performance. The comlink library looks fantastic, it will be good to keep in mind for other expensive operations we might do that could block the main thread.

Reviewed 11 of 11 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @siltomato)


src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/quill-services/quill-insight-render.service.ts line 38 at r1 (raw file):

    if (Worker != null) {
      try {
        const worker = new Worker(new URL('./insight-formatting.worker', import.meta.url));

The examples I see on the comlink page use the file extension. Is that optional since you are not using it here?

Code quote:

'./insight-formatting.worker'

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/quill-services/quill-insight-render.service.ts line 103 at r1 (raw file):

      this.workerApi[Comlink.releaseProxy]();
    }
  }

Nit: Move this method close to the constructor. It is often useful to see interface implementations near the beginning of the class.

Code quote:

  ngOnDestroy(): void {
    if (this.workerApi != null) {
      this.workerApi[Comlink.releaseProxy]();
    }
  }

@RaymondLuong3 RaymondLuong3 added ready to test and removed will require testing PR should not be merged until testers confirm testing is complete labels Jul 29, 2025
@siltomato siltomato force-pushed the fix/sf-3370-lynx-app-lockup-for-large-projects branch from afef597 to 44e6e8c Compare July 31, 2025 20:55
Copy link
Collaborator Author

@siltomato siltomato left a comment

Choose a reason for hiding this comment

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

Yes, I was excited about the result as well.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @siltomato)


src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/quill-services/quill-insight-render.service.ts line 38 at r1 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

The examples I see on the comlink page use the file extension. Is that optional since you are not using it here?

Yes, I believe so.


src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/lynx/insights/quill-services/quill-insight-render.service.ts line 103 at r1 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Nit: Move this method close to the constructor. It is often useful to see interface implementations near the beginning of the class.

Done.

@Nateowami Nateowami force-pushed the fix/sf-3370-lynx-app-lockup-for-large-projects branch from 44e6e8c to 706cf86 Compare August 5, 2025 13:46
@Nateowami Nateowami added testing complete Testing of PR is complete and should no longer hold up merging of the PR and removed ready to test labels Aug 5, 2025
@Nateowami Nateowami enabled auto-merge (squash) August 5, 2025 13:46
@Nateowami Nateowami merged commit c3389f3 into master Aug 5, 2025
17 of 18 checks passed
@Nateowami Nateowami deleted the fix/sf-3370-lynx-app-lockup-for-large-projects branch August 5, 2025 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testing complete Testing of PR is complete and should no longer hold up merging of the PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants