Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cta): prevent an infinate loop of fetching video details #12084

Merged

Conversation

m4olivei
Copy link
Contributor

@m4olivei m4olivei commented Oct 28, 2024

Related Ticket(s)

ADCMS-6577

Description

Prevents an infinite loop in some situations with vidoe CTA cards.

There are a few key parts to the infinite loop:

  1. Both <c4d-card-group-item> and <c4d-card-footer> use CTAMixin
  2. CTAMixin introduces an update hook that watches for changes to videoDescription, and if it's changed, the c4d-cta-request-video-data event is fired.
  3. The <c4d-video-cta-container> component listens for the c4d-cta-request-video-data event, makes a request to Kaltura, and sets the videoDescription property for the component that fired the event in step 2, either <c4d-card-group-item> or <c4d-card-footer>
  4. The <c4d-card-group-item> component transposes its attributes, including videoDescription onto the <c4d-card-footer> component. Restarting the cycle.
  5. There is a race condition mixed into this infinite loop, I believe due to the async nature of the Kaltura request. Sometimes, things settle down, but other times, we end up in an infinite loop where the two components descending from CTAMixin ping pong firing an event, triggering Kaltura request, triggering update cycle, repeat.

Here is an adjustment to the provided Carbon demo that uses the CDN preview of this PR to show the fix:

https://stackblitz.com/github/m4olivei/carbon-demo/tree/video-cta-lockup-pr-preview?file=scripts%2Fprestart.js

Changelog

Changed

  • Adjusted CTA logic to guard against Kaltura fetch when it's already been done. Like the conditional shortly before it, we use videoDuration as a guard to know whether we need to query Kaltura or not.

@m4olivei m4olivei added the test: CDN preview used for generating @carbon/ibmdotcom-web-components CDN for testing label Oct 28, 2024
@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Oct 28, 2024

Deploy preview created for package IBM.com Web Components Deploy Preview CDN:
https://ibmdotcom-cdn.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/12084/index.html

Built with commit: 3986a793d63c122377242a265f8aff658f13bb44

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Oct 28, 2024

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Oct 28, 2024

@m4olivei m4olivei marked this pull request as ready for review October 29, 2024 00:37
@m4olivei m4olivei requested a review from a team as a code owner October 29, 2024 00:37
@m4olivei m4olivei requested review from bruno-amorim, marcelojcs, andy-blum and Valentin-Sorin-Nicolae and removed request for a team October 29, 2024 00:37
Copy link

netlify bot commented Oct 30, 2024

Deploy Preview for ibm-dotcom-web-components ready!

Name Link
🔨 Latest commit 3986a79
🔍 Latest deploy log https://app.netlify.com/sites/ibm-dotcom-web-components/deploys/6723c3f8d60a680008cb3e79
😎 Deploy Preview https://deploy-preview-12084--ibm-dotcom-web-components.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Oct 30, 2024

Deploy Preview for ibm-dotcom-web-components-react-wrap ready!

Name Link
🔨 Latest commit 3986a79
🔍 Latest deploy log https://app.netlify.com/sites/ibm-dotcom-web-components-react-wrap/deploys/6723c3f8312daf0008fa87e4
😎 Deploy Preview https://deploy-preview-12084--ibm-dotcom-web-components-react-wrap.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@m4olivei m4olivei force-pushed the fix/cta-video-fetch-loop branch from 45d52a7 to 3986a79 Compare October 31, 2024 17:52
@andy-blum
Copy link
Member

The demo link from the issue is updated to use the CDN deploy previews from this PR. While the browser no longer locks up, the modal containing the media isn't popping up either.

I'd expect the card-group-item to open a modal, seed in the iframe from kaltura, and then either play the video or error on a domain restriction.

Copy link
Member

@andy-blum andy-blum left a comment

Choose a reason for hiding this comment

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

changes needed in previous comment

@m4olivei
Copy link
Contributor Author

m4olivei commented Nov 1, 2024

The demo link from the issue is updated to use the CDN deploy previews from this PR. While the browser no longer locks up, the modal containing the media isn't popping up either.

I'd expect the card-group-item to open a modal, seed in the iframe from kaltura, and then either play the video or error on a domain restriction.

Isn't that the issue that we're working on in ADCMS-6596?

@andy-blum
Copy link
Member

Ah yes it is

@andy-blum andy-blum self-requested a review November 1, 2024 14:05
@m4olivei m4olivei added the Ready to merge Label for the pull requests that are ready to merge label Nov 1, 2024
@kodiakhq kodiakhq bot merged commit 9964cf1 into carbon-design-system:main Nov 1, 2024
18 of 26 checks passed
@m4olivei m4olivei deleted the fix/cta-video-fetch-loop branch November 4, 2024 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready to merge Label for the pull requests that are ready to merge test: CDN preview used for generating @carbon/ibmdotcom-web-components CDN for testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants