Ensure Product Collection Block's `queryId` Uniqueness by ObliviousHarmony · Pull Request #44522 · woocommerce/woocommerce

ObliviousHarmony · 2024-02-09T19:48:29Z

Submission Review Guidelines:

I have followed the WooCommerce Contributing Guidelines and the WordPress Coding Standards.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have reviewed my code for security best practices.
Following the above guidelines will result in quick merges and clear and detailed feedback when appropriate.

Changes proposed in this Pull Request:

One of the caveats to block duplication is that all attributes are copied. This has caused some trouble because the queryId are expected to be unique. This PR makes some changes to the block initialization in able to ensure that both attributes have a unique value.

queryId: This problem was also present in Gutenberg and I have applied the same fix to both this block and the Query Loop block. Since we only need the ID to remain stable across frontend page loads, we can just set instanceId each time. It will only actually do anything if it changes so there's no problem with doing it this way.

Closes #43311

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

Create a new post.
Add a new "Product Collection" block and note the queryId in the Code Editor.
Add a new "Product Collection" block and confirm that the queryId are different in the Code Editor.
Duplicate the first "Product Collection" block and note that the queryId is different.
Refresh the page, the queryId should re-index and still ensure uniqueness.

Note: The queryId tracks the number of instances that have been created on the page. Switching between the code editor and visual editor will not reset the counter and so after switching back you'll see the IDs have a weird offset. This is fixed on the next editor reload but doesn't actually hurt anything.

Changelog entry

Automatically create a changelog entry from the details below.

Details

Significance

Patch
Minor
Major

Type

Fix - Fixes an existing bug
Add - Adds functionality
Update - Update existing functionality
Dev - Development related task
Tweak - A minor adjustment to the codebase
Performance - Address performance issues
Enhancement - Improvement to existing functionality

Message

Comment

Since the `clientId` is already using a uuid we can just lean into it and avoid the extra dependency. The `clientId` is not persistent across page reloads but that's okay since we only need it on init.

Since we don't need it to be _perfectly_ stable, we can just set the `queryId` to the instance ID on every component mounting. This is fine and won't mark the attribute as dirty unless a collection block is added, deleted, or moved in just the right way.

When product collection blocks are copied the attributes are copied too. This includes the unique ID which then becomes not unique. This commit adds a check to ensure that the ID is always unique.

github-actions · 2024-02-09T19:49:05Z

Hi @Aljullu, @imanish003, @kmanijak,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

github-actions · 2024-02-09T20:12:58Z

Test Results Summary

Commit SHA: e4bfe6c

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	259	0	0	2	0	261	0m 38s
E2E Tests	0	0	0	363	0	363	0m 0s

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

imanish003

I found one edge case where it doesn’t seem to be working as expected. I added “Featured” collection & made it a sync pattern. Here is a quick demo of what’s happening if I try to add two sync patterns on the same post. The post becomes unresponsive.

Screen.Recording.2024-02-12.at.12.22.46.pm.mov

In the Code Editor, the sync patterns are represented as follows:

<!-- wp:block {"ref":219} /-->

This pattern includes a 'ref' attribute, which corresponds to a post ID. As far as I understand, sync patterns are stored as posts in the wp_posts table, as illustrated in the screenshot below. This situation seems to cause a race condition or an infinite loop. Can you please check if it also leads to race condition in Gutenberg PR?

ObliviousHarmony · 2024-02-12T18:01:08Z

Thanks @imanish003, I'll have to add patterns to the list of things I test blocks code with. I was also able to replicate this in the Gutenberg PR so I've returned that one to a draft to make sure it doesn't get merged and introduce problems. I'll have to dive into how these pattern blocks work, I'm not sure what causes the failure.

spencerfinnell · 2024-02-12T18:20:44Z

@ObliviousHarmony getBlocks() does not include nested blocks, which is likely why it's failing in patterns:

https://github.com/WordPress/gutenberg/blob/cf18b3bd947be8b250cd2c18b0c496d6dbe85930/packages/block-editor/src/store/selectors.js#L177-L187

I've had luck finding existing blocks with getBlocksByName() which finds nested blocks as well.

https://github.com/WordPress/gutenberg/blob/cf18b3bd947be8b250cd2c18b0c496d6dbe85930/packages/block-editor/src/store/selectors.js#L330-L338

Then when the block is mounted, check other found instances of the block and determine if a new unique value needs to be set for the attribute.

ObliviousHarmony · 2024-02-12T19:14:59Z

That's good to know @spencerfinnell, however, in this case the problem lies in the very nature of sync blocks. I documented my findings in the Gutenberg PR. Unfortunately, I think fixing it there will be much easier than fixing it here; even if fixing it there requires more discussion. Since we have a support policy with WordPress versions, if this requires an upstream change, we will have to wait a long time to take advantage of it.

I think our best bet would be to lock the queryId of blocks in a sync pattern. Assuming there's a way to detect whether a block is in one (I'm sure there is, right?), we can just keep it set to whatever it already is. This would require a tiny bit of additional effort to avoid queryId re-use but it would get us the uniqueness that we're looking for.

When a sync pattern's block is updated it causes other instances on the page to re-insert the block. This changes the ID and causes it to update it again, leading to a loop.

Co-authored-by: Karol Manijak <20098064+kmanijak@users.noreply.github.com>

Rather than using an instanceId for blocks in patterns, we will use a randomized number with an offset. This stops collisions in both instances on the page and instances in sync patterns.

kmanijak

I tested the solution and it solves the problem of unique query IDs! I tested multiple scenarios and looks good!

You mentioned:

however, this won't cause any problems since the queryid only has to be stable across client renders.

But I see one problem. Every time merchant edits the page the query IDs may change or replace. The pagination is based on the query IDs and links with specific page number may get quickly outdated.

I may be overthinking it but that affects sharing links including page number. I can't assess if it's a blocker (probably not?) but still wanted to raise that for discussion. What do you think?

There's a better selector that we can use and a const variable for the offset would be easier to follow.

ObliviousHarmony · 2024-03-14T17:01:47Z

Thanks for the review @kmanijak!

Every time merchant edits the page the query IDs may change or replace. The pagination is based on the query IDs and links with specific page number may get quickly outdated.

This is mostly true! The queryId is set based on where the collection block is on the page relative to any others. As long as they aren't re-ordering collection blocks or adding new ones between existing ones there won't be any issues. If they make large changes to the page then any links would either break or point at something different.

I may be overthinking it but that affects sharing links including page number. I can't assess if it's a blocker (probably not?) but still wanted to raise that for discussion. What do you think?

The crux of this argument would be why people share links to specific result pages. I would argue that, if someone was to do this at all, it would be with the intention that the recipient look at it shortly after receiving it. It's incredibly unlikely that It would change in that span of time, or for most folks, probably at all after the merchant has a store page they're happy with. Keep in mind that people can do whatever they want to the page as long as they don't change the collection blocks.

Ultimately, I think we're always going to have some possible instability here. As we've discussed elsewhere, Gutenberg isn't really built with stable block identification in mind. While it's something that could probably be added without too much trouble, as it stands now, it's not really achievable without caveats like this one. I don't think this is a problem though in the current case.

kmanijak

The crux of this argument would be why people share links to specific result pages. I would argue that, if someone was to do this at all, it would be with the intention that the recipient look at it shortly after receiving it. It's incredibly unlikely that It would change in that span of time, or for most folks, probably at all after the merchant has a store page they're happy with. Keep in mind that people can do whatever they want to the page as long as they don't change the collection blocks.

Yeah, agree with you!

Testing steps work as expected! And I'm preapproving.

However, I tried to use the logic related to synced patterns and usage of offsets but I must admit I didn't make it. I saw low query IDs (like instance IDs) within synced patterns and the query ID was duplicated on the frontend. Here's a video:

syncedpatterns.mov

The query param was query-6-page=5 which means query ID was 6.

Personally, I think it's ok as I accepted synced patterns work that way. If you agree as well we should remove the logic with the offset. Or if the behavior from video is not expected and IDs should be unique in synced patterns then logic requires revisiting 😅 Unless I'm doing something wrong!

imanish003

I've also tested the changes and share similar concerns (as @kmanijak) regarding the use of offset for Synced patterns. I'm uncertain about their benefits. Wouldn't we still end up with duplicate query IDs on the client side with the offsets? If that's the case, what's the advantage of using it? Perhaps there's something I've overlooked 🧐

Here is a quick demo:

Edited.mov

ObliviousHarmony · 2024-03-18T21:21:45Z

Thanks for taking a look you two!

However, I tried to use the logic related to synced patterns and usage of offsets but I must admit I didn't make it. I saw low query IDs (like instance IDs) within synced patterns and the query ID was duplicated on the frontend.

Odd question @kmanijak: are you sure you ran a build? I've played around and I can't get the block in a pattern without it setting an offset ID. Did you check the data-query-id attribute on the client? Manish's video contains the expected ID re-assignment behavior.

Wouldn't we still end up with duplicate query IDs on the client side with the offsets?

Due to the nature of sync patterns it is impossible to uniquely identify them. The best we can do is make sure that there are no collisions between blocks on the page and patterns, as well as different patterns that contain Product Collection blocks.

If that's the case, what's the advantage of using it?

There are a few cases we need to consider:

On page load, every non-pattern Product Collection block needs a unique queryId.
If Product Collection blocks are added or removed uniqueness must be maintained.
Once added to a pattern the Product Collection block's queryId must become static. Changing attributes on a block in a pattern has other implications, such as re-inserting other instances on the same page, as well as cascading changes to other pages.

We are calling useInstanceId() to get query IDs but this just returns the index of a given block on a page. For example, a page with 3 Product Collection blocks will have queryIds of 0, 1, and 2. The caveat to this approach though is that it has no awareness of collisions. This means it's up to us to make sure that the queryId is unique.

This works great for the first two points since we can just re-index all of the blocks each time. These IDs only exists for the purpose of pagination and so it's okay if they aren't permanent as long as they're unique and non-volatile. The benefit of using the instance ID is that each time the block is initialized we can set the queryId and it won't change unless Product Collection blocks have had their ordering changed in some way.

The problem with this is the third point. If we lock the queryId to one that might reasonably be used by a Product Collection block on the page it will collide. Imagine a pattern with a Product Collection block that has queryId: 0. If you add this to a page that contains a Product Collection block there will be a duplicate 0 ID.

My solution to this problem was to offset the queryId of Product Collection blocks in patterns. This makes sure that they'll never collide with the output of useInstanceId(). To prevent them from colliding with other patterns I randomized the ID.

imanish003 · 2024-03-20T11:47:29Z

Odd question @kmanijak: are you sure you ran a build? I've played around and I can't get the block in a pattern without it setting an offset ID. Did you check the data-query-id attribute on the client? Manish's video contains the expected ID re-assignment behavior.

@kmanijak When I was testing today again, I also faced this issue. I ran pnpm install & pnpm --filter='@woocommerce/plugin-woocommerce' watch:build to fix it. This might be worth a try on your end as well, just in case it proves beneficial 🙂

The problem with this is the third point. If we lock the queryId to one that might reasonably be used by a Product Collection block on the page it will collide. Imagine a pattern with a Product Collection block that has queryId: 0. If you add this to a page that contains a Product Collection block there will be a duplicate 0 ID.

My solution to this problem was to offset the queryId of Product Collection blocks in patterns. This makes sure that they'll never collide with the output of useInstanceId(). To prevent them from colliding with other patterns I randomized the ID.

Aha! It all makes sense now. Thank you for the detailed explanation 🙏🏻

So now there is only one edge case related to having same sync patterns in same page/post. As you said, this is nature of sync patterns so it's not possible to fix it. I believe we need to raise it in Gutenberg because this will be problem with Query Loop block too. With that said, I am going to Approve the PR, as it's working as expected. Really appreciate your patience and detailed explanations throughout this PR 🙌🏻

ObliviousHarmony · 2024-03-20T16:45:53Z

I believe we need to raise it in Gutenberg because this will be problem with Query Loop block too.

Already done @imanish003 😄 I opened a pull request alongside this one. Now that this PR is ready to merge I can pull the changes from it into the Gutenberg one and mark it as ready for review.

ObliviousHarmony · 2024-03-20T17:52:48Z

I took another look at my Gutenberg PR and it looks like this sync block solution doesn't work with the latest version of Gutenberg @imanish003 @kmanijak. As far as I can tell, the support they're building for partial sync patterns (which will solve this correctly!) has broken the way that it changes the attributes. For now, I think it's fine to merge this PR, but we should revisit once the feature in Gutenberg is stable and we can take advantage of it.

What do you two think? It will still work perfectly for normal blocks but will break if you have Gutenberg installed. I think that's kind of problematic but there's currently no support otherwise.

kmanijak

Odd question @kmanijak: are you sure you ran a build? I've played around and I can't get the block in a pattern without it setting an offset ID. Did you check the data-query-id attribute on the client? Manish's video contains the expected ID re-assignment behavior.

So I was testing with WP 6.5 and Manish with WP 6.4. I double checked and I can see offset values in 6.4 but not in 6.5. I debugged and the offset logic is fired correctly but the updated queryID is not reflected on the frontend.
It proves your statement (although I assumed you meant the newer Gutenberg version aiming WP 6.6):

I took another look at my Gutenberg PR and it looks like this sync block solution doesn't work with the latest version of Gutenberg @imanish003 @kmanijak.

What do you two think?

Given the above, I think we should remove the offset logic already as it will only temporarily benefit those using WP 6.4 while it introduces some unnecessary complexity.

And the most important is that the remaining logic with the instance ID solves the initial problem which is ensuring unique ID when duplicating Product Collection.

Since this offset doesn't work on WordPress 6.5 we're just going to go ahead and remove it.

ObliviousHarmony · 2024-03-21T20:57:55Z

Given the above, I think we should remove the offset logic already as it will only temporarily benefit those using WP 6.4 while it introduces some unnecessary complexity.

I agree, I've gone ahead and removed the offset logic since it serves no purpose. We can explore solving this more holistically in a year or so when WordPress 6.6 (hopefully) has support for partial patterns!

kmanijak

🚀

ObliviousHarmony added 3 commits February 8, 2024 00:10

Use clientId Instead of uuid Package

894cfbd

Since the `clientId` is already using a uuid we can just lean into it and avoid the extra dependency. The `clientId` is not persistent across page reloads but that's okay since we only need it on init.

Maintain queryId Uniqueness

4b82304

Since we don't need it to be _perfectly_ stable, we can just set the `queryId` to the instance ID on every component mounting. This is fine and won't mark the attribute as dirty unless a collection block is added, deleted, or moved in just the right way.

Ensure Product Collection id Uniqueness

8fd4f5f

When product collection blocks are copied the attributes are copied too. This includes the unique ID which then becomes not unique. This commit adds a check to ensure that the ID is always unique.

woocommercebot requested review from a team and Aljullu and removed request for a team February 9, 2024 19:48

ObliviousHarmony requested a review from imanish003 February 9, 2024 19:49

imanish003 suggested changes Feb 12, 2024

View reviewed changes

Leave queryId Alone in Sync Patterns

7d7bd48

When a sync pattern's block is updated it causes other instances on the page to re-insert the block. This changes the ID and causes it to update it again, leading to a loop.

github-actions Bot added plugin: woocommerce Issues related to the WooCommerce Core plugin. focus: monorepo infrastructure Issues and PRs related to monorepo tooling. labels Feb 14, 2024

Merge branch 'trunk' into fix/43311-fix-query-id-persistence

6b978f9

kmanijak reviewed Feb 19, 2024

View reviewed changes

Comment thread ...s/woocommerce-blocks/assets/js/blocks/product-collection/edit/product-collection-content.tsx Outdated

ObliviousHarmony and others added 2 commits February 20, 2024 16:16

Removed Unnecessary Spread Operator

868c440

Co-authored-by: Karol Manijak <20098064+kmanijak@users.noreply.github.com>

Merge branch 'trunk' into fix/43311-fix-query-id-persistence

66d9aad

github-actions Bot removed the focus: monorepo infrastructure Issues and PRs related to monorepo tooling. label Mar 7, 2024

ObliviousHarmony changed the title ~~Ensure Product Collection Block's id and queryId Uniqueness~~ Ensure Product Collection Block's queryId Uniqueness Mar 7, 2024

Prevent Collisions in Patterns

e5bbf74

Rather than using an instanceId for blocks in patterns, we will use a randomized number with an offset. This stops collisions in both instances on the page and instances in sync patterns.

ObliviousHarmony requested review from imanish003 and kmanijak March 7, 2024 19:30

kmanijak reviewed Mar 13, 2024

View reviewed changes

ObliviousHarmony added 2 commits March 13, 2024 13:12

Removed Unnecessary Changelog

99d6790

Cleaned Up Sync Pattern Handling

6b503fa

There's a better selector that we can use and a const variable for the offset would be easier to follow.

ObliviousHarmony requested a review from kmanijak March 13, 2024 20:55

kmanijak approved these changes Mar 15, 2024

View reviewed changes

imanish003 suggested changes Mar 18, 2024

View reviewed changes

Merge branch 'trunk' into fix/43311-fix-query-id-persistence

b1395a6

ObliviousHarmony requested review from imanish003 and kmanijak March 20, 2024 04:41

imanish003 approved these changes Mar 20, 2024

View reviewed changes

kmanijak requested changes Mar 21, 2024

View reviewed changes

ObliviousHarmony added 2 commits March 21, 2024 13:37

Merge branch 'trunk' into fix/43311-fix-query-id-persistence

6f14825

Removed Sync Pattern Offset

e62630c

Since this offset doesn't work on WordPress 6.5 we're just going to go ahead and remove it.

Fixed Linting

45b049d

kmanijak approved these changes Mar 22, 2024

View reviewed changes

kmanijak added 2 commits March 22, 2024 08:46

Prevent collection different than Produyct Catalog from being filtered

59f5bc5

Revert previous commit

e4bfe6c

ObliviousHarmony merged commit b8bd480 into trunk Mar 22, 2024

ObliviousHarmony deleted the fix/43311-fix-query-id-persistence branch March 22, 2024 08:40

github-actions Bot added this to the 8.9.0 milestone Mar 22, 2024

github-actions Bot added the needs: analysis label Mar 22, 2024

alopezari added needs: external testing and removed needs: analysis labels Mar 25, 2024

Conversation

ObliviousHarmony commented Feb 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Submission Review Guidelines:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Changelog entry

Significance

Type

Message

Comment

Uh oh!

github-actions Bot commented Feb 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Feb 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results Summary

Uh oh!

imanish003 left a comment

Choose a reason for hiding this comment

Uh oh!

ObliviousHarmony commented Feb 12, 2024

Uh oh!

spencerfinnell commented Feb 12, 2024

Uh oh!

ObliviousHarmony commented Feb 12, 2024

Uh oh!

Uh oh!

kmanijak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ObliviousHarmony commented Mar 14, 2024

Uh oh!

kmanijak left a comment

Choose a reason for hiding this comment

Uh oh!

imanish003 left a comment

Choose a reason for hiding this comment

Uh oh!

ObliviousHarmony commented Mar 18, 2024

Uh oh!

imanish003 commented Mar 20, 2024

Uh oh!

ObliviousHarmony commented Mar 20, 2024

Uh oh!

ObliviousHarmony commented Mar 20, 2024

Uh oh!

kmanijak left a comment

Choose a reason for hiding this comment

Uh oh!

ObliviousHarmony commented Mar 21, 2024

Uh oh!

kmanijak left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ObliviousHarmony commented Feb 9, 2024 •

edited

Loading

github-actions Bot commented Feb 9, 2024 •

edited

Loading

github-actions Bot commented Feb 9, 2024 •

edited

Loading