-
Couldn't load subscription status.
- Fork 834
Forms: Add useConfigValue hook and start using it in the forms dashboard #45472
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
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 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new centralized configuration store using Redux for managing Forms configuration data with better performance and caching. It replaces scattered config access patterns with a unified useConfigValue hook that automatically fetches data from /wp/v2/feedback/config only once.
- Introduces
useConfigValuehook and complete Redux store implementation for Forms config management - Migrates dashboard and block editor components from
window.JetpackScriptData.formsanduseFormsConfig()to the new hook - Removes server-side config injection for dashboard pages to improve performance
Reviewed Changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/store/config/ |
Complete Redux store implementation with actions, reducer, selectors, resolvers, and TypeScript types |
src/hooks/use-config-value.ts |
New hook providing simple access to config values with automatic fetching |
tests/js/store/config.test.js |
Comprehensive store tests covering actions, reducer, selectors, and integration scenarios |
tests/js/hooks/use-config-value.test.js |
Hook tests covering various usage patterns and edge cases |
src/dashboard/components/layout/index.tsx |
Migrated from useFormsConfig() to useConfigValue() with loading state handling |
src/dashboard/inbox/index.js |
Updated to use new hook for hasFeedback checks with proper navigation logic |
src/dashboard/inbox/response.js |
Simplified config access for emptyTrashDays value |
src/dashboard/inbox/empty-responses.tsx |
Updated to use new hook for trash configuration |
src/dashboard/inbox/export-responses/google-drive.tsx |
Migrated Google Drive support URL access to new hook |
src/dashboard/hooks/use-export-responses.ts |
Updated export nonce handling to use new config system |
src/dashboard/hooks/use-create-form.ts |
Migrated form creation nonce to new hook with proper dependency tracking |
src/dashboard/about/index.tsx |
Updated plugin assets URL access to use new config hook |
src/dashboard/index.tsx |
Removed config data parsing and direct navigation logic |
src/dashboard/class-dashboard.php |
Removed server-side config injection for dashboard pages |
src/blocks/contact-form/edit.tsx |
Migrated integrations feature flag to new hook |
src/blocks/contact-form/components/jetpack-integrations-modal/integration-card/plugin-action-button.tsx |
Updated plugin permission checks to use new config system |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
projects/packages/forms/src/dashboard/components/layout/index.tsx
Outdated
Show resolved
Hide resolved
148f98b to
d4040b7
Compare
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Code Coverage SummaryCoverage changed in 11 files. Only the first 5 are listed here.
9 files are newly checked for coverage. Only the first 5 are listed here.
|
13840ed to
0c86bd4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been testing this for a while, but it was just rebased. Going to post these results on pre-rebase version for the record, and will re-pull and re-test.
- Dashboard: loads as expected, no errors, pre-loading still works
- Dashboard: if no responses, it redirects to About as expected
- Dashboard: About page images with ASSETS_URL load correctly
- Dashboard: integrations are hidden if isIntegrationsEnabled is false
- Dashboard: CSV and Google Drive exports work as expected
- Dashboard: correct EmptyResponses message show when inbox, spam, trash, or search are empty
- Dashboard: Updating EMPTY_TRASH_DAYS shows correct value in trash inbox and single responses views
- Block: loads as expected without errors, no errors, pre-loading still works
- Block: integrations aer hidden if isIntegrationsEnabled is false
- PluginActionButton: installing/activating plugins, including loading states, works as before
- Tests: new tests pass
projects/packages/forms/src/dashboard/components/response-view/body.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Retested this again after latest rebase. Works great!
- Dashboard: loads as expected, no errors, pre-loading still works
- Dashboard: if no responses, it redirects to About as expected
- Dashboard: About page images with ASSETS_URL load correctly
- Dashboard: integrations are hidden if isIntegrationsEnabled is false
- Dashboard: CSV and Google Drive exports work as expected
- Dashboard: correct EmptyResponses message show when inbox, spam, trash, or search are empty
- Dashboard: Updating EMPTY_TRASH_DAYS shows correct value in trash inbox and single responses views
- Block: loads as expected without errors, no errors, pre-loading still works
- Block: integrations aer hidden if isIntegrationsEnabled is false
- PluginActionButton: installing/activating plugins, including loading states, works as before
- Tests: new tests pass
As an extra check, I also searched the codebase to confirm we do not have any remaining uses of the old config object on the frontend.
…ard (#45472) * Forms: Add new config hook * Add tests * Update to useConfigValue instead of useFormsConfig and config * Remove the on page config for the dashboard * changelog * simply the resolver * Update the readme with example to add a new config * update code to be more readable * Add unknown error constant * Update getConfig resolver * Update the includes to be at the right spot * Add form response type * fix lint error * remove comment
This PR does the following.
Introduces a new hook
useConfigValuethat is responsible for fetching the/wp/v2/feedback/configendpoint only once unless it is already called. (if the hook is preloaded) then it doesn't get called at all. (which is the case for the feedback dashboad and the editor)The new hook is easier to use.
Proposed changes.
New Files
Modified Files
useConfigValue()
rd/plugin-action-button.tsx
JetpackScriptData.forms into dashboard pages
Breaking Changes
None - This is a pure refactor. The old useFormsConfig() hook remains available
for backward compatibility.
Other information:
Jetpack product discussion
This is an performance improvement because we don't call slow function multiple times any more.
Does this pull request change what data or activity we track or use?
No
Testing instructions:
Do the tests pass?
Visit the Feedback dashboard.
Notice that it loads fast. The feedback/config endpoint should be preloaded so notice that there are no requests to the config endpoint happening.
Change the config endpoint and notice that it still works as expected when you refresh the page.
Things to test.