Skip to content

Conversation

@AlexVelezLl
Copy link
Member

@AlexVelezLl AlexVelezLl commented May 13, 2025

Summary

EPubJs was registering some ResizeObserver listeners when rendering the epub books, and somehow these listeners caused an infinite resize observer loop when the EPubRenderer component was unmounted. This PR destroys de epub book, which cleans these listeners, before EPubRenderer is unmounted.

Before:

Screenshare.-.2025-05-13.4_30_37.PM.mp4

After:

Screenshare.-.2025-05-13.4_31_37.PM.mp4

References

learningequality/kolibri-design-system#960

Reviewer guidance

  1. Open a channel
  2. Click to add/upload a file
  3. Upload an EPUB file
  4. Click 'Finish'
  5. Check that no errors are triggered

@AlexVelezLl AlexVelezLl requested review from akolson and bjester May 13, 2025 21:33
Copy link
Member

@bjester bjester left a comment

Choose a reason for hiding this comment

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

I think this is a sensible change and reduces the errors. Although, I still feel there is a more systematic issue with the KDS composable. Therefore I don't think we should close the issue-- the problems are still valid:

I think the callback being passed to the ResizeObserver in KDS should be wrapped in a requestAnimationFrame or throttle (from frame-throttle). It's already using frame-throttle but only at the target. The frame throttling should be moved higher up in the chain.

We should follow the recommendations of the MDN https://developer.mozilla.org/en-US/docs/Web/API/ResizeObserver#observation_errors

The resize observer in useKResponsiveElement triggers the listener, which is attached to an element, but that listener is throttled. By the time the throttled function inside is invoked, the element could have been removed or changed. If we move the throttling closer to the observer, then it can be defensive against changes between the resize and invocation directly at that point in time. That would also align multiple resize events into a frame paint, which should give better visual consistency if there many resize listeners. Although, the smoothness of the transition would be dependent on how often animation frames are allowed, but that's the whole point of using them.

At the very least, the KDS onBeforeUnmount should cancel any pending calls to the resize listener, but overall it would be best to follow the structure in MDN's docs.

@bjester bjester merged commit d916071 into learningequality:unstable May 14, 2025
14 checks passed
@AlexVelezLl AlexVelezLl deleted the fix-resize-observer-errors branch May 14, 2025 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants