Search Blocks: add pre-hydration skeleton to filter-wc-attribute#48677
Search Blocks: add pre-hydration skeleton to filter-wc-attribute#48677
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
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 🤖 🔴 Action required: Please include detailed testing steps, explaining how to test your change, like so: 🔴 Action required: We would recommend that you add a section to the PR description to specify whether this PR includes any changes to data or privacy, like so: Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
Code Coverage SummaryCoverage changed in 3 files.
|
|
@claude review plz |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- Replace inline `$has_buckets` derivation with `Search_Blocks::pre_hydration_filter_view()` - Use `Search_Blocks::emit_filter_wrapper_context()` for wrapper context - Switch from `!state.hasFilterBuckets` to `context.wrapperHidden` + `syncFilterWrapperVisibility` - Include `filter-skeleton-partial.php` when `is_initial_loading` is true - Add `Filter_Wc_Attribute_Render_Test.php` with 6 render tests - Note: style.scss already had the necessary skeleton mixin includes Closes #48675 Agent-Logs-Url: https://github.com/Automattic/jetpack/sessions/b14b45fa-935e-469e-9205-88393b0e2963 Co-authored-by: kangzj <1425433+kangzj@users.noreply.github.com>
- Replace assertStringContainsString('hidden', ...) and
assertStringNotContainsString(' hidden', ...) with a strict regex on the
opening div so the assertion can't be satisfied by data-wp-bind--hidden /
aria-hidden / wrapperHidden, and isn't sensitive to indentation
whitespace before the bare hidden attribute. Mirrors the canonical
Clear_Filters_Render_Test pattern.
- Guard tearDownAfterClass via WP_Block_Type_Registry::is_registered() so
the early-return path in setUpBeforeClass (WP < 6.5) doesn't leave us
calling unregister on an unregistered block and tripping failOnNotice.
Addresses Copilot review comments #3214166225, #3214166229, #3214166232.
def9099 to
ed89c24
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
WC attribute filters are the longest-loading filter region on a product page (one aggregation per registered pa_* taxonomy, often 3-5+ on a typical shop). The other filter blocks resolve their single aggregation in one fast call, so the wrapper-hidden-until-buckets pattern they used pre-#48505 doesn't cause a layout shift the visitor would notice. - filter-checkbox / filter-date / filter-wc-rating: drop pre_hydration_filter_view + emit_filter_wrapper_context + the skeleton partial include. Restore the inline $has_buckets derivation and revert the wrapper binding to !state.hasFilterBuckets + a conditional literal hidden attribute on first paint. - filter-checkbox/style.scss, filter-date/style.scss, filter-wc-rating/style.scss: drop @use "../skeleton" and the skeleton.shimmer / skeleton.filter-rows @includes. - _skeleton.scss, filter-skeleton-partial.php: docstrings updated to reflect that only filter-wc-attribute consumes them now. - filter-wc-attribute: unchanged (keeps the skeleton plumbing this PR originally added for it).
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Agent-Logs-Url: https://github.com/Automattic/jetpack/sessions/1541066f-fab7-4691-9c22-f82d7a5483ce Co-authored-by: kangzj <1425433+kangzj@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
claude[bot] flagged the docblock on test_wrapper_uses_context_wrapper_hidden_binding as stale after the narrowing — it claimed parity with filter-checkbox / filter-date, which is no longer true since those reverted to !state.hasFilterBuckets. Rewrite it to call out filter-wc-attribute as the only block still on the wrapperHidden plumbing and to note why (single aggregation resolves fast for the others).
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
🤖 Review-cycle summary (narrowing iteration) —
|
| Source | Item | Resolution |
|---|---|---|
| claude[bot] re-review | store/index.js:1069 stale comment ("for each filter-checkbox block") |
Fixed in 775420e (s/filter-checkbox/filter-wc-attribute/) |
| claude[bot] re-review | Filter_Wc_Attribute_Render_Test.php:184–186 stale docblock claiming parity with filter-checkbox / filter-date |
Fixed in 3c6be02; now spells out filter-wc-attribute as the only block on the wrapperHidden plumbing |
| claude[bot] re-review | Retained-options visibility trade-off (active selection + zero-bucket aggregation → wrapper hides) | Documented in the PR body as an "Accepted trade-off" — pre-#48505 behavior; filter-date unaffected by definition |
| copilot-swe-agent re-review | Audit of syncFilterWrapperVisibility / wrapperHidden isolation across the JS store |
Confirmed safe (context is scoped per block instance by the Interactivity API); also caught and fixed the view.js docstring in 775420e |
Unaddressed (flagged for owner): None.
CI: all 25 required checks pass on 3c6be026c9b.
Per Jasper's clarification, the loading skeleton is needed for filter-wc-attribute *and* the other bucket-driven filter blocks (filter-checkbox, filter-date, filter-wc-rating) were already working as expected with their own skeleton plumbing — they should not have been changed. Reverts: - 3c6be02 (test docblock change predicated on the narrowing) - 775420e (copilot-swe-agent's stale-comment fixes that only made sense in the narrowed world) - 402bcd5 (the narrowing itself) Net effect: PR returns to its original goal — bring filter-wc-attribute to parity with filter-checkbox / filter-date / filter-wc-rating's existing pre-hydration skeleton plumbing. The 3 non-attribute blocks are untouched from trunk.
|
Course-correction from Jasper: the narrowing direction (commit 402bcd5 + follow-ups) was a misread of the original instruction. Reverted in 63cf38c ( PR title + body restored. All 354 PHP tests + 540 JS tests pass locally; PHPCS clean. |
filter-wc-attributewas the only bucket-driven filter block missing the shared pre-hydration skeleton plumbing. On a deep-linked URL (?pa_color[]=red), the attribute filter wrapper was hidden until JS hydration finished — showing a blank space instead of shimmer rows.Why
WC attribute filters are typically the longest-loading filter region on a product page — one aggregation per registered
pa_*taxonomy. The other bucket-driven filter blocks (filter-checkbox,filter-date,filter-wc-rating) already use the shared skeleton plumbing from #48505 / its follow-ups; this PR bringsfilter-wc-attributeto parity so deep-linked attribute filter URLs paint the same shimmer rows on first paint instead of a blank gap.Real-screen before / after
Deep-link
/jetpack-search-demo/?pa_color[]=redon the local docker env, captured with JS blocked at the network layer to freeze the pre-hydration window.On trunk both
filter-wc-attributewrappers (Color, Size) render with the literalhiddenHTML attribute and stay invisible until JS hydration finishes — so the sidebar shows only the Active-filters chip with empty space below. With this PR the wrappers render with the shared skeleton partial: COLOR/SIZE headings plus shimmer rows, and the pre-selectedredcheckbox seeded from the URL is already visible.Changes
filter-wc-attribute/render.php$has_bucketsread withSearch_Blocks::pre_hydration_filter_view()(returnshas_buckets,is_initial_loading,show_wrapper)data-wp-bind--hidden="!state.hasFilterBuckets"tocontext.wrapperHidden+data-wp-watch="callbacks.syncFilterWrapperVisibility"— the same mechanism used by filter-checkbox / filter-date / filter-wc-ratingfilter-skeleton-partial.phpwhenis_initial_loadingis true$all_selected_on_paintlogic (guards "All filters applied" state), now short-circuited behind$view['has_buckets']The SCSS was already complete —
style.scsshad@include skeleton.shimmerand@include skeleton.filter-rows.Filter_Wc_Attribute_Render_Test.php(new)Six render-integration tests including the primary regression guard:
Test plan
composer phpunitinprojects/packages/search/— 354 tests, 772 assertions, all greenpnpm test-scriptsinprojects/packages/search/— 540 tests across 47 suites, all green?pa_color[]=reddeep-link) shows the shimmer rows in the attribute filter wrapperFixes RSM-2809.