Fix advanced data breakdowns state when switching Analytics properties#12924
Merged
tofumatt merged 15 commits intoJun 19, 2026
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the Advanced Data Breakdowns settings in the Analytics 4 module to store a per-property map of enabled flags instead of a single global flag, allowing each GA4 property to maintain its own state. It introduces a new GET:custom-dimensions datapoint to fetch custom dimensions for any property and updates the JS datastore, React components, and PHP settings classes accordingly. The review feedback is highly constructive and focuses on improving robustness and security. Key recommendations include adding defensive checks for failed custom dimension requests, validating and sanitizing GA4 property IDs in both JS and PHP to prevent malformed keys, and avoiding fragile magic property access with wp_list_pluck on Google API Client model objects.
Change the setting from a single `enabled` flag to a map of property ID to boolean, so each Analytics property keeps its own state. Default to an empty map, sanitize every value to a boolean, return the stored property IDs as the view-only keys, take a property ID in `is_enabled()`, and merge with `array_replace` to keep the numeric keys.
Validate that every value in the `settings` map is a boolean, then pass the whole map to `merge`, so saving one property keeps the others.
The GET datapoint returns the stored option, now a per-property map. Update its test to expect the map for both admin and view-only users.
Add a datapoint that lists a property's Site Kit custom dimensions from the property ID on the request, so the settings row can read a property that is not yet saved. Return the `googlesitekit_`-prefixed parameter names, or an empty array when the property has none.
Register `GET:custom-dimensions` with the `analyticsadmin` service, next to the sync datapoint, and assert it in the datapoint list.
Change `AdvancedDataBreakdownsSettings` to a map of property ID to boolean. Take a property ID in `setAdvancedDataBreakdownsEnabled` and `isAdvancedDataBreakdownsEnabled`, and validate that every value is a boolean.
Add `getCustomDimensions` and `hasCustomDimensionsForProperty` to read any property's custom dimensions through the new datapoint. In `createCustomDimensions`, create the dimensions the selected property is missing, and create nothing while a new property is being set up.
Read the selected property's state with `getPropertyID`, `hasCustomDimensionsForProperty`, and `isAdvancedDataBreakdownsEnabled`. Show the green check when the property has every Site Goals custom dimension, the loading state while they resolve, and a disabled Enable button while a new property is being set up.
Add a story for the row with a new property selected, where the Enable button is disabled, with the Backstop reference images that snapshot it at the small, medium, and large widths.
`createCustomDimensions` now reads `getCustomDimensions`. Seed it in the Site Goals tests so they do not fetch the custom-dimensions endpoint.
`createCustomDimensions` now reads `getCustomDimensions`. Seed it in the custom-dimensions hooks tests so they do not fetch the endpoint.
shervElmi
force-pushed
the
bug/12892-advanced-data-breakdowns-property-switch
branch
from
ada072a to
419abbb
Compare
Mark the custom dimensions as not yet resolved to ensure the loading state is displayed correctly in the SettingsAdvancedDataBreakdowns component during tests.
|
Size Change: 0 B Total Size: 2.92 MB ℹ️ View Unchanged
|
The "setting is loading" test rendered without waiting, so its frozen request settled during the next test and React logged an act() warning there. Wait for the registry in both loading tests so each one settles inside act() and does not affect the next.
Collaborator
Author
|
If the E2E or VRT tests fail in this PR, the failures are not caused by this PR. |
tofumatt
requested changes
Jun 19, 2026
tofumatt
left a comment
tofumatt
left a comment
Collaborator
There was a problem hiding this comment.
Looks good! I think mostly minor comments here and a few questions… only main issue is the UI of the error message and margins for the Web Data Stream name text field.
Please let me know what you think 🙂
Switch `setAdvancedDataBreakdownsEnabled` to a property-to-boolean map and update the reducer, the caller, and the datastore tests. The action validates the keys, so the saved map only holds valid property IDs. Add bottom margin below the web data stream name error so it no longer crowds the toggle. Add the `--analytics` modifier to the settings story so the row renders as it does in production. Add an error-state variant to the Text Field story. Name the "enabled" flag in the custom dimensions tests, add whitespace between the steps, and use a double-quoted view-only assertion message.
shervElmi
force-pushed
the
bug/12892-advanced-data-breakdowns-property-switch
branch
from
393157b to
86b19b7
Compare
tofumatt
approved these changes
Jun 19, 2026
tofumatt
left a comment
tofumatt
left a comment
Collaborator
There was a problem hiding this comment.
Awesome, looks great, thanks! 👍🏻
Comment on lines
+78
to
+90
| <TextField | ||
| className="mdc-text-field--error" | ||
| label="With Error" | ||
| name="textfield" | ||
| value="https://www.sitekitbygoogle.com" | ||
| helperText="This is the error message." | ||
| trailingIcon={ | ||
| <span className="googlesitekit-text-field-icon--error"> | ||
| <WarningIcon width={ 14 } height={ 12 } /> | ||
| </span> | ||
| } | ||
| outlined | ||
| /> |
Collaborator
There was a problem hiding this comment.
We use this pattern in four places in Site Kit, and it's kinda awkward/verbose/error-prone:
Even labelling it "helper text" is a bit disingenuous, as it's really an error message.
Normally I'd say we update the API of this component while we're here adding more instances, but instead I've created a follow-up issue for this: #12935
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
2 participants
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.
Summary
Addresses issue:
Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist