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

ADAPT-1837: Added guard code to avoid crashes on page previews. #195

Merged
merged 2 commits into from
May 14, 2021

Conversation

nplowman
Copy link

@nplowman nplowman commented May 7, 2021

READY FOR REVIEW

Summary

  • Fixes issue with Storyblok previews crashing on pages with large numbers of components. (Such as Story Overview)

Review By (Date)

  • Before end of sprint.

Criticality

  • 7/10. Affects only a small number of pages, but makes editing on these pages very frustrating.

Review Tasks

Setup tasks and/or behavior to test

  1. Log into Storyblok
  2. Make a copy of the Story Overview page (you could also edit the original, if you feel confident you won't accidentally save test changes).
  3. Set the Preview URL to use the Netlify Preview environment.
  4. Make any change on the page to trigger a preview update. (I usually test by adding a new Story Picker component to the Curated Stories list under the "Stories" tab, but any change to the page was triggering the issue).

Before Fix: Preview would crash, and eventually show an error message. (Something about "component" being undefined).
After Fix: Preview should update as expected.

Front End Validation

  • Is the markup using the appropriate semantic tags and passes HTML validation?
  • Cross-browser testing has been performed?
  • Automated accessibility scans performed?
  • Manual accessibility tests performed?
  • Design is approved by @ user?

Backend / Functional Validation

Code

  • Are the naming conventions following our standards?
  • Does the code have sufficient inline comments?
  • Is there anything in this code that would be hidden or hard to discover through the UI?
  • Are there any code smells?
  • Are tests provided? eg (unit, behat, or codeception)

Code security

General

  • Is there anything included in this PR that is not related to the problem it is trying to solve?
  • Is the approach to the problem appropriate?

Affected Projects or Products

  • N/A

Associated Issues and/or People

Resources

  • The symptoms we were seeing, was that the data coming back from the storyblok.on('input') hook was resolving most but not all relationship fields. (A few random ones were containing only the UUID field).
  • My best theory of why this was happening, was that we were exceeding the max limit of 100 relations in a single API request. https://www.storyblok.com/docs/technical-limits
  • The Storyblok Bridge API seems to eventually resolve these relationships, but adding guard code prevents React from attempting to render these unresolved fields on the initial pass.

@nplowman nplowman requested review from yvonnetangsu and sherakama May 7, 2021 16:18
Copy link
Member

@yvonnetangsu yvonnetangsu left a comment

Choose a reason for hiding this comment

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

Thanks, @nplowman ! I've tested this locally and it works well. I was still able to make changes to individual cards, rearrange them etc without the page crashing. I have one question for you, is there a reason that you use != instead of !== in the typeof story comparison?

@nplowman
Copy link
Author

nplowman commented May 7, 2021

Thanks, @nplowman ! I've tested this locally and it works well. I was still able to make changes to individual cards, rearrange them etc without the page crashing. I have one question for you, is there a reason that you use != instead of !== in the typeof story comparison?

No specific reason. I'm fine changing it to the more strict type comparison if that's preferable.

@yvonnetangsu
Copy link
Member

Thanks, @nplowman ! I've tested this locally and it works well. I was still able to make changes to individual cards, rearrange them etc without the page crashing. I have one question for you, is there a reason that you use != instead of !== in the typeof story comparison?

No specific reason. I'm fine changing it to the more strict type comparison if that's preferable.

Nah, it's fine the way it is. Thanks!

@sherakama
Copy link
Member

LINTING IS COMING! I'm working through getting prettier and airbnb standards set up to take care of questions like whether or not to use strict comparisons or not.

Copy link
Member

@sherakama sherakama left a comment

Choose a reason for hiding this comment

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

Just one question about how the uid eventually gets resolved but otherwise this seems to do the job.

@nplowman
Copy link
Author

nplowman commented May 7, 2021

Just one question about how the uid eventually gets resolved but otherwise this seems to do the job.

I spent a bit of time trying to pin this down because it was bothering me as well.
It looks like I may have declared victory too early.

After running document.getElementsByClassName('ood-story-card') to validate the number of cards getting rendered, it looks like on the first time sb.on('input') is triggered, the count dropped from 50 down to 20ish. So the guard code is just preventing a hard crash. Cards that only have the UUID are just getting skipped.

@nplowman nplowman requested a review from sherakama May 14, 2021 20:59
Copy link
Member

@sherakama sherakama left a comment

Choose a reason for hiding this comment

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

Code looks good but I am not getting an alert on https://app.storyblok.com/#!/me/spaces/78141/stories/0/0/24605778

Is this still the correct page to trigger this issue?

@nplowman
Copy link
Author

Code looks good but I am not getting an alert on https://app.storyblok.com/#!/me/spaces/78141/stories/0/0/24605778

Is this still the correct page to trigger this issue?

Yes, but it should only trigger if it receives an unresolved UUID for one of the stories. (It will also only trigger once in a session, unless you delete the cookie).

If you try to edit one of the fields on the page, it should trigger it. If not, I'll take a closer look and see what is going on.

@sherakama
Copy link
Member

Ah there it goes. I swear I did that already.

Ok. GTG.

@sherakama sherakama merged commit 9abb1e9 into main May 14, 2021
@sherakama sherakama deleted the ADAPT-1837 branch May 14, 2021 23:21
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.

3 participants