Repository navigation
Conversation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Settings API utilities and migration popup src/util/admin.js, src/plugins/v2-migration-popup/*, src/deprecated/v2/init.php, src/init.php, src/welcome/index.php, jest.config.js, src/test/wp-api-stub.js |
fetchSettings retrieves settings through apiFetch, and the new saveSettings function posts settings through apiFetch. The migration popup also retrieves settings through apiFetch and ignores failed requests. Script dependencies and the Jest mapping are updated to remove the WordPress API model dependency. |
Global settings persistence src/plugins/global-settings/* |
Global block styles, color schemes, colors, typography, and block layout settings use saveSettings. Existing debounce timing and described store updates remain. |
Component and welcome settings src/components/base-control2/label-tooltip.js, src/components/guided-modal-tour/index.js, src/deprecated/v2/welcome/admin.js, src/lazy-components/cimo/index.js, src/welcome/admin.js, src/welcome/import-export/index.js |
Tooltip, guided-tour, Cimo notice, optimization, welcome, and imported settings use saveSettings instead of WordPress Settings models. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix
Suggested reviewers: bfintal, kaeizen
Merge Risk: 
9efd6
-
No actionable regression remains in the reviewed settings migration; it is ready for normal checks.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 20 files. Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title "fix: migrate wp-api to wp-api-fetch" matches the main change. The PR replaces the deprecated @wordpress/apimodels (wp-api) with apiFetch-based calls (wp-api-fetch) across many files. It ad…Linked Issues check The changes satisfy the coding requirements for #3768. Settings reads and writes now useapiFetchthroughfetchSettingsandsaveSettings. The migration popup reads and saves/wp/v2/settingswi…Out of Scope Changes check The changes stay within #3768. The shared settings helper, migrated save call sites,wp-api-fetchdependencies, migration-popup error handling, and removal of the obsolete API test stub all support …Full details: Title check
-
Explanation
The title "fix: migrate wp-api to wp-api-fetch" matches the main change. The PR replaces the deprecated
@wordpress/apimodels (wp-api) with apiFetch-based calls (wp-api-fetch) across many files. It adds a saveSettings helper in src/util/admin.js and removes wp-api from the script dependencies. The title is short and names the primary change. It does not mention the saveSettings helper or the failed-save handling, but the task says it need not cover every detail.
Full details: Linked Issues check
-
Explanation
The changes satisfy the coding requirements for
#3768. Settings reads and writes now useapiFetchthroughfetchSettingsandsaveSettings. The migration popup reads and saves/wp/v2/settingswith the same permission model. Its rejected-save path clears the busy state. The changed call sites remove the old@wordpress/apimodel usage. No additional automated-test requirement is stated.
Full details: Out of Scope Changes check
-
Explanation
The changes stay within
#3768. The shared settings helper, migrated save call sites,wp-api-fetchdependencies, migration-popup error handling, and removal of the obsolete API test stub all support the settings API migration. No unrelated change is identified.
-
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
🤖 Pull request artifacts
|
|
Size Change: -112 B (0%) Total Size: 2.64 MB 📦 View Changed
ℹ️ View Unchanged
|
Contributor
|
Changes requested. Details are on #3768. The v2 migration popup in Please keep the read and the save on the same permission, and turn the spinner off when the save fails. Also add one |
bfintal
reviewed
Nov 4, 2026
bfintal
left a comment
bfintal
left a comment
Contributor
There was a problem hiding this comment.
Thanks, this is a clean change.
Every new models.Settings( ... ).save() is now a single saveSettings() helper that uses apiFetch, and no @wordpress/api usage is left in src/.
Settings save to the same /wp/v2/settings endpoint as before, so nothing changes for users.
All CI passes here and in the premium PR bfintal/Stackable-Premium#412.
Two small suggestions, neither blocks this:
[Minor] Failed saves are silently ignored
Most calls now end with .catch( () => {} ).
That matches the old behavior (Backbone also failed quietly), so this isn't a regression.
But if a save fails, the user changes a global color and it's gone after a reload, with no message.
Later, it would help to log the error (console.error) or show a notice in one place inside saveSettings().
[Nit] One call has no .catch()
See the inline comment.
Because this touches every place that saves a setting, QA should quickly check a few of them: global colors, typography, block styles, the settings page, and import/export.
| saveTimeout = setTimeout( () => { | ||
| const settings = new models.Settings( { [ optionName ]: newSettings } ) // eslint-disable-line camelcase | ||
| settings.save() | ||
| saveWPSettings( { [ optionName ]: newSettings } ) |
Contributor
There was a problem hiding this comment.
[Nit] This is the only saveSettings call here without .catch().
If the request fails, the browser console shows an "Uncaught (in promise)" error.
Add .catch( () => {} ) like the other calls, so they all behave the same.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes #3768
Summary by CodeRabbit
Improvements
Bug Fixes