jetpack icon indicating copy to clipboard operation
jetpack copied to clipboard

Tiled Gallery Block: Fix block crashing on high volume gallery (> 50 images).

Open yansern opened this issue 3 years ago • 5 comments

Fixes https://github.com/Automattic/jetpack/issues/25017 https://github.com/Automattic/wp-calypso/issues/60335

Changes proposed in this Pull Request:

Fixes the Tiled Gallery Block crashing when user updates a high volume gallery (> 50 images), e.g. adding images, remove images or simplying updating without changing anything.

Why did it break? React throws a "maximum call stack depth" error. This is because of excessive calls to this.props.setAttributes causing unnecessary re-rendering.

How was it fixed? Add additional checks to ensure this.props.setAttributes is only called when necessary.

Jetpack product discussion

Maintenance work.

Does this pull request change what data or activity we track or use?

No.

Testing instructions:

How to replicate the crash:

  • Add a Tiled Gallery block with more than 50 images.
  • Click Edit Gallery from the block toolbar.
  • Add images, remove images or just do nothing and click Update in the media modal.
  • It should crash.
  • Save this post where you can consistently replicate the crash.

How to test the fix:

  • Load the post where you can consistently replicate that crash.
  • Click Edit Gallery from the toolbar.
  • Add images, remove images or just do nothing and click Update in the media modal.
  • It should NOT crash.

Here are some videos to show the before & after:

https://user-images.githubusercontent.com/1287077/162379721-1d955629-dd8a-4989-8c29-5b52200461c5.mp4

https://user-images.githubusercontent.com/1287077/162379526-a3b2cfcc-02f8-4650-a476-09db4fa4a94f.mp4

yansern avatar Apr 08 '22 06:04 yansern

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • :white_check_mark: Include a description of your PR changes.
  • :warning: All commits were linted before commit.
  • :white_check_mark: Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • :white_check_mark: Add testing instructions.
  • :white_check_mark: Specify whether this PR includes any changes to data or privacy.
  • :white_check_mark: Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation :robot:


The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.


Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped. Then, add the "[Status] Needs Team review" label and ask someone from your team review the code. Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.


Jetpack plugin:

  • Next scheduled release: August 2, 2022.
  • Scheduled code freeze: July 25, 2022.

github-actions[bot] avatar Apr 08 '22 06:04 github-actions[bot]

Just want to add a note about this one. Also try testing on a small tiled gallery, like 2-3 images, and see if the attributes are updated/stored properly after adding/removing/swapping images. It should be fine I think but who knows maybe the checking is too aggressive.

yansern avatar Apr 08 '22 08:04 yansern

It looks like issue can be reproduced on Simple and WoA sites only, right? I couldn't reproduce it on a standalone Jetpack site.

ivan-ottinger avatar Apr 08 '22 10:04 ivan-ottinger

It looks like issue can be reproduced on Simple and WoA sites only, right? I couldn't reproduce it on a standalone Jetpack site.

Applies to all places. I was reproducing it on the standalone Jetpack sites. Interesting that you can't reproduce it on standalone.

yansern avatar Apr 08 '22 11:04 yansern

The PR fix needs to be reworked as it is missing data-* attributes (technically not necessary for Edit's view but will rethink solution).

yansern avatar Apr 11 '22 06:04 yansern