Skip to content

Conversation

siltomato
Copy link
Collaborator

@siltomato siltomato commented Jun 27, 2025

This PR fixes a timing issue that caused a null reference error due to the 'setTimeout' used when calling ensureOverlayWithinEditorBounds().

  • Changed ensureOverlayWithinEditorBounds() to use the instance variable openRef instead of passing in overlay ref.
  • Added null check for overlayElement inside ensureOverlayWithinEditorBounds() method.

This change is Reviewable

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

codecov bot commented Jun 27, 2025

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 82.50%. Comparing base (59d9191) to head (943b065).
⚠️ Report is 103 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...itor/lynx/insights/lynx-insight-overlay.service.ts 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3290   +/-   ##
=======================================
  Coverage   82.50%   82.50%           
=======================================
  Files         605      605           
  Lines       35197    35198    +1     
  Branches     5724     5724           
=======================================
+ Hits        29038    29039    +1     
+ Misses       5304     5303    -1     
- Partials      855      856    +1     

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

@siltomato siltomato marked this pull request as ready for review June 27, 2025 22:31
@Nateowami Nateowami changed the title SF-3444 Prevent error when rapidly clicking lynx panel insights SF-3444 Prevent error when rapidly clicking Lynx panel insights Jun 28, 2025
@pmachapman pmachapman self-assigned this Jun 29, 2025
@pmachapman pmachapman self-requested a review June 29, 2025 19:10
Copy link
Collaborator

@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.

:lgtm: I'm marking LGTM so it can go on to testing - I do have a question though, as generally it feels hacky to use setTimeout when the issue appears to be we are waiting for an element in another component to be rendered.

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @siltomato)


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

    // When initially displayed, scroll editor if necessary to ensure overlay is displayed within editor bounds
    setTimeout(() => this.ensureOverlayWithinEditorBounds());

Is one event cycle enough - I am guessing the assumption is that is enough for the scrolling container to finish loading?

Is there no event we could have trigger this line instead?

Code quote:

setTimeout(() => this.ensureOverlayWithinEditorBounds());

@pmachapman pmachapman added ready to test and removed will require testing PR should not be merged until testers confirm testing is complete labels Jun 29, 2025
@RaymondLuong3 RaymondLuong3 force-pushed the fix/sf-3444-error-when-lynx-panel-insights-clicked-rapidly branch from ed6cd3e to 943b065 Compare July 3, 2025 22:35
@RaymondLuong3 RaymondLuong3 merged commit 88de53a into master Jul 3, 2025
18 checks passed
@RaymondLuong3 RaymondLuong3 deleted the fix/sf-3444-error-when-lynx-panel-insights-clicked-rapidly branch July 3, 2025 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants