Skip to content

Newsletter: gate placement "Preview and edit" link on saved-enabled state#49532

Open
CGastrell wants to merge 2 commits into
change/newsletter-settings-spinnerfrom
change/newsletter-preview-edit-gating
Open

Newsletter: gate placement "Preview and edit" link on saved-enabled state#49532
CGastrell wants to merge 2 commits into
change/newsletter-settings-spinnerfrom
change/newsletter-preview-edit-gating

Conversation

@CGastrell

@CGastrell CGastrell commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Part of #49485 - NL08

Stacked on #49530 (the Settings-spinner cache). This PR is based on that branch and shares one settings-state model — review/merge #49530 first; GitHub will retarget this to trunk automatically once it lands. The diff here is just the placement-link change.

Proposed changes

  • In the modernized Newsletter Settings, the subscription-form placement "Preview and edit" link previously rendered whenever the site supported the Site Editor link — regardless of whether that placement was actually enabled. Clicking it for a disabled (or freshly-toggled-but-unsaved) placement opened an empty Site Editor preview.
  • The link is now shown only when the placement is enabled in the saved state and still displayed as checked. Gating is done against a persisted saved baseline (savedData) rather than "untouched since save", which keeps the link reversible: toggle a saved-on placement off and back on and the link returns immediately, without a round-trip to the server. A freshly enabled-but-unsaved placement still stays linkless until the save lands. The existing site-capability gating (block theme / theme stylesheet / subscription-site-edit support) is preserved.
  • Why a saved baseline: the Subscriptions section stages toggles for a manual Save — data is updated optimistically while the key is tracked in a pending changeset that's cleared on save. The page previously kept no record of the last-persisted values, so the first version of this fix had to use "key is in the pending changeset" as a proxy for "unsaved". That proxy is touch-based, not value-based: toggling a saved-on placement off then on left the key marked dirty, so the link stayed hidden until an actual save — even though the saved state never changed. This version introduces savedData, a persisted baseline set on load and advanced on every successful save, and gates the link on data[ key ] && savedData[ key ]. The baseline is module-cached (parallel to Newsletter: Stop the modernized Settings spinner from flashing on every visit #49530's optimistic cache) so it survives the tab unmount/remount without the link blinking off on return.

This only touches the placement-card grid. The Navigation subgroup's "Preview and edit" links are intentionally out of scope here (they share the same "empty preview when unsaved" critique — a reasonable follow-up).

Related product discussion/links

  • See linked issue.

Does this pull request change what data or activity we track or use?

No.

Testing instructions

Prerequisite: a site with the modernized Newsletter dashboard enabled (modernization filter on) and a block theme so the Site Editor links are eligible.

  1. Go to Jetpack → Newsletter → Settings, and find the Subscriptions → Homepage and posts placement grid (the 2×2 set of selectable cards: overlay, pop-up, end-of-post block, floating button).
  2. For a placement whose checkbox is off: confirm no "Preview and edit" link appears under its title.
  3. Toggle a previously-off placement on but do not click Save: confirm the link does not appear while the change is unsaved.
  4. Click Save: confirm the link now appears and opens the correct template/part in the Site Editor (not an empty preview).
  5. Reversibility check (the fix): with that placement saved-on, toggle it off (link disappears), then back on without saving — confirm the link returns immediately.
  6. Toggle a saved-on placement off and Save: confirm the link stays gone.
  7. Switch to the Subscribers tab and back to Settings: confirm the link state is correct on return with no flicker (the saved baseline is module-cached).
  8. Regression: on a site that does not support the Site Editor links (non-block theme / no subscription-site-edit support), confirm no placement shows the link regardless of enabled/saved state.

Automated coverage: tests/subscriptions-section.test.tsx asserts the link shows for enabled-and-saved placements (including the reverted-to-saved case) and is hidden for disabled, enabled-but-unsaved, and capability-ineligible cases (jp test js packages/newsletter).

…pinner flash

The modernized dashboard mounts NewsletterSettingsBody inside a Tabs.Panel
that unmounts on tab switch and remounts on return. With isLoading
initialized to true and no cache, every revisit re-flashed the full-page
spinner while re-fetching. Seed the loading/data state from a module-level
cache so repeat mounts paint immediately; the genuine first load still shows
the spinner, and the mount effect keeps refreshing in the background.
@github-actions

Copy link
Copy Markdown
Contributor

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ✅ Add a "[Status]" label (In Progress, Needs Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

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:

  1. Ensure all required checks appearing at the bottom of this PR are passing.
  2. Make sure to test your changes on all platforms that it applies to. You're responsible for the quality of the code you ship.
  3. You can use GitHub's Reviewers functionality to request a review.
  4. When it's reviewed and merged, you will be pinged in Slack to deploy the changes to WordPress.com simple once the build is done.

If you have questions about anything, reach out in #jetpack-developers for guidance!

@jp-launch-control

jp-launch-control Bot commented Jun 10, 2026

Copy link
Copy Markdown

Code Coverage Summary

Coverage changed in 4 files.

File Coverage Δ% Δ Uncovered
projects/packages/newsletter/src/settings/newsletter-settings.tsx 69/151 (45.70%) 3.27% 6 💔
projects/packages/newsletter/src/settings/sections/placement-card.tsx 4/6 (66.67%) 66.67% -4 💚
projects/packages/newsletter/src/settings/sections/placement-illustrations.tsx 4/4 (100.00%) 100.00% -4 💚
projects/packages/newsletter/src/settings/sections/subscriptions-section.tsx 20/30 (66.67%) 66.67% -19 💚

Full summary · PHP report · JS report

If appropriate, add one of these labels to override the failing coverage check: Covered by non-unit tests Use to ignore the Code coverage requirement check when E2Es or other non-unit tests cover the code Coverage tests to be added later Use to ignore the Code coverage requirement check when tests will be added in a follow-up PR I don't care about code coverage for this PR Use this label to ignore the check for insufficient code coveage.

…tate

The subscription placement "Preview and edit" link was gated only on site
capability, so it appeared for placements that were disabled or toggled-but-
unsaved — clicking it opened an empty Site Editor preview. Show the link only
when the placement is enabled in its saved state (enabled in data and not in
the pending changeset).
@CGastrell CGastrell force-pushed the change/newsletter-preview-edit-gating branch from fc3134f to 85ff8e9 Compare June 10, 2026 19:13
@CGastrell CGastrell changed the base branch from trunk to change/newsletter-settings-spinner June 10, 2026 19:14
@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

  • To test on WoA, go to the Plugins menu on a WoA dev site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin (Jetpack), and enable the change/newsletter-preview-edit-gating branch.
  • To test on Simple, run the following command on your sandbox:
bin/jetpack-downloader test jetpack change/newsletter-preview-edit-gating

Interested in more tips and information?

  • In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
  • Read more about our development workflow here: PCYsg-eg0-p2
  • Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

@CGastrell

Copy link
Copy Markdown
Contributor Author

Follow-up note: a pre-existing "phantom unsaved edit" issue (not fixed here)

Flagging for later — out of scope for this PR.

The Settings body caches the optimistic data across the tab unmount/remount, but the pending-changes set (subscriptionChanges, etc.) resets to empty on unmount. So if you toggle a setting and switch tabs without saving, on return the toggle still shows its new value while the page believes there are no unsaved changes — it looks saved but isn't, and the Save button won't offer to persist it.

The savedData baseline this PR introduces is exactly the piece needed to fix it: on remount, compare the cached optimistic data against savedData and rebuild the pending set from the diff. Small follow-up, didn't want to expand this PR's scope.

@CGastrell CGastrell added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Jun 10, 2026
@CGastrell CGastrell marked this pull request as ready for review June 10, 2026 20:19
@dhasilva dhasilva force-pushed the change/newsletter-settings-spinner branch from 10770ad to 1eace63 Compare June 10, 2026 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Modernized Dashboards wrap up

1 participant