Repository navigation
Conversation
Ignore malformed field-group entries without dropping valid siblings or converting formatting failures into fetch errors. Keep cached fields during refetch, simplify resolution arguments, and retain explicit refresh and sticky error behavior. hasFinishedResolution includes error in @wordpress/data 10.32.0 and the WordPress 6.2 bundle (data 8.3.4); error metadata dates to data 6.3.0. Keep the resolver comment scoped to the getPostType identity rather than implying shared getEntityRecord resolution metadata. Validate the staged tree: 38 suites / 966 tests pass. Leave useSiteEditorContext and usePostEditorFields unchanged. Their existing lint findings remain for separate cleanup.
Pin React-18-compatible data 10.32.0 and core-data 7.32.0 as test dependencies. Keep the REST boundary mocked while exercising real selectors, resolutions and subscriptions. Resolve unrelated root settings configuration normally before counting post-type requests. Replace mock-only fan-out assertions with tests for 1/10/200 consumers, an already pending getPostType request, remount reuse, distinct types, field retention during refresh and shared failures without automatic retries. Supply browser encoding APIs missing from jsdom. After clean npm ci, 39 suites / 970 tests pass; new and updated tests lint cleanly; formatting passes. Full bindings lint retains seven baseline errors, including three in unchanged hooks. Mutation checks reject original per-block fetching at 10 and 200 calls and reject the previous blank-on-refresh hook. Dependency-only build is byte-identical, and wp-data/wp-core-data remain externals. React and ReactDOM stay at 18.3.1.
df9e980 to
acc1fb6
Compare
|
Resolved points that were worth addressing from Opus 5.5 code review, rest are left out from this PR for now. LLM summary: Review follow-ups
|
Restore recognizable behavior test names and explicit metadata, null-type, and empty/missing-group assertions. Retain a dedicated real-resolver request-parameter check. Add controlled pending-request tests for unmounting one shared consumer and changing the type of the same mounted consumer, including late results for its previous type. Verify the surviving consumer completes, remount reuses the result, old fields do not leak, and request counts remain bounded. Test-only change: 39 suites / 976 tests pass. All 12 real-registry cases pass in three repeated runs; both modified test files pass lint and formatting. Production code and dependencies are unchanged.
|
Reviewed tests manually, applied some changes to retain existing test naming where applicable. Current CI PHP 7.4 test failure is GH Actions runner failure, not related to the PR. |
|
Manual testing on production site looks good. Before PR changes
With PR changes
|
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
cbravobernal
left a comment
cbravobernal
left a comment
There was a problem hiding this comment.
Thanks for the PR. At the beginning seems a huge one. But is due to the package lock update. Please fix it if possible and I will be happy to review it again.
|
@cbravobernal Large lock file diff is coming from the integration test related dependencies:
These are hard to avoid if wanting to keep that test, but could be moved to a separate PR while retaining unit-tests with this PR if that could help, what do you think? |
@jasalt please, otherwise I need to review 7000 lines to avoid any mistake or weird package update, like it happened before. |
Preserve the integration coverage on a separate branch and remove its test file and environment helper from this PR. Restore package.json and package-lock.json to the upstream base, removing the added WordPress test dependencies and their large lockfile diff. Restore mock-based no-private-fetch assertions while retaining the newer hook regression coverage.
Sharing the core-data resolver also caches failed resolutions, so mounting another block no longer retries a transient schema-fetch failure. The hook exposed refresh(), but the inspector did not consume it, leaving users without a recovery action short of reloading the editor. Show a generic error notice with a Retry button that invalidates the shared resolution through refresh(). Disable retry while loading and preserve cached field options. Keep retries user-triggered to avoid reintroducing per-block request storms. Add tests for error display, explicit retry, loading state, successful recovery, and Post Editor isolation. Validation: 39 suites / 972 tests pass; build, formatting, and new test lint pass. Existing experimental ToolsPanel import lint errors are unchanged. No dependency or lockfile changes.
|
@cbravobernal sure. Removed the integration test leading to large lock file diff. Could submit it as a separate PR if wished so but for now it's saved on a backup branch. PR commits could be squashed to ignore that from the history. Also, added a manual retry mechanism in case of the SCF request failing as it would not get retried again similarly as previously without a full page refresh, commit message has more detailed description. |
There was a problem hiding this comment.
🟡 Changes recommended
Request sharing lacks the described real-registry regression coverage, and the recovery UI conflicts with the declared scope.
2 open issues
What changed in this PR
Uses core-data’s native post-type resolver to reduce duplicate Site Editor schema requests, addressing #564.
Changes:
- Shares schema resolution, status tracking, and explicit invalidation.
- Preserves stored fields during refetch and skips malformed field entries.
- Adds inspector Retry controls and updates unit tests.
| File | Description |
|---|---|
| tests/js/bindings/utils.test.js | Tests malformed field groups and entries. |
| tests/js/bindings/hooks.test.js | Adapts hook tests to mocked core-data selectors. |
| tests/js/bindings/block-editor-recovery.test.js | Tests inspector error and Retry behavior. |
| assets/src/js/bindings/utils.js | Skips invalid groups and unnamed fields. |
| assets/src/js/bindings/hooks.js | Uses shared resolution and exposes refresh. |
| assets/src/js/bindings/block-editor.js | Adds an error notice and Retry button. |
Review effort: Balanced
Fix per-block SCF schema fan-out using core-data's native
getPostTypeidentityfor selection, loading/error tracking and explicit invalidation. Preserve edit,
field definitions and context/registry isolation without a module-global cache.
The hook exposes a refresh callback for programmatic invalidation, and a user-facing editor control lets users retry failed requests. Delaying the loading and processing of SCF field definitions until a block’s settings need to be shown in the editor sidebar is outside the scope of this PR and could be considered as a separate, optional optimization.
Related issue: #564
Testing
The original hook tests were adapted to core-data while retaining recognizable behavior tests for field loading, errors, metadata caching, null/type changes, and empty or missing field groups. Added coverage for malformed entries and keeping stored fields during refetch.
The real-registry integration test (
tests/js/bindings/hooks.integration.test.js) and its added dependencies were dropped at @cbravobernal’s request in commit 18b0637 because they introduced too large a lockfile diff. The following checks were run locally before removal, but are not included in this PR’s current test suite:GET /wp/v2/types/product?context=editgetPostTyperequest; a remount reuses the resultPutting back the original per-block fetching (at 10 and 200 consumers) or the blank-on-refresh behavior made those integration tests fail. These results are historical validation, not ongoing regression coverage in this PR.
Locally, before the integration test was removed:
npm run test:unitpassed 39 suites / 976 tests. The current suite count and result need to be updated after rerunning without that test.Notes for reviewers
already had seven lint errors (three in
useSiteEditorContextandusePostEditorFields, which this PR leaves unchanged). They are left for aseparate cleanup.
The selected 57-context synthetic campaign qualifies: request counts stay
bounded as mounts increase, collections/mounts stay unchanged, and both frozen
version tracks have stated coverage: https://github.com/jasalt/scf-fanout-issue-lab
Use of AI Tools
This proposed change, fixtures, tests, measurements tooling and drafts were
substantially authored with an AI coding assistant operating in PiG through an
OpenAI API.
The follow-up code review and the resulting fix plan were produced with Claude Code (Anthropic).