-
Notifications
You must be signed in to change notification settings - Fork 30
642 cache for newsletter subscriptions data #15024
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
base: main
Are you sure you want to change the base?
Conversation
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 implements client-side caching for newsletter subscription data to reduce API calls to the identity service and Braze. The cache uses session storage with a 5-minute TTL and invalidates when users change or sign out.
Key changes:
- New caching module to manage newsletter subscription data in session storage
- Integration of cache checks before API calls in the subscription hook
- Cache updates after successful newsletter signups
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| dotcom-rendering/src/lib/newsletterSubscriptionCache.ts | New module implementing session storage cache with TTL and user-specific invalidation |
| dotcom-rendering/src/lib/useNewsletterSubscription.ts | Updated to check cache before fetching, cache API responses, and clear cache on signout |
| dotcom-rendering/src/lib/useNewsletterSubscription.test.ts | Added comprehensive test coverage for caching behavior and cache invalidation scenarios |
| dotcom-rendering/src/components/SecureSignup.importable.tsx | Updated to add newsletter to cache after successful signup |
| dotcom-rendering/src/components/ManyNewsletterSignUp.importable.tsx | Updated to add multiple newsletters to cache after successful signup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…com:guardian/dotcom-rendering into 642-cache-for-newsletter-subscriptions-data
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
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hello 👋! When you're ready to run Chromatic, please apply the You will need to reapply the label each time you want to run Chromatic. |
georgerichmond
left a comment
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.
Looks excellent 👍
…com:guardian/dotcom-rendering into 642-cache-for-newsletter-subscriptions-data
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
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Could we squash the commits where possible please. It would be great if each commit describes an atomic change and hence makes the history easier to read. |
What does this change?
We have recently implemented a feature to show newsletter signup components conditionally within articles. The functionality checks the list of user's current subscriptions and only displays the signup component if user is not subscribed. This change adds caching to the newsletter subscription list.
This is a short lived cache that sits in browser session storage. It gets invalidated when user closes the tab logs out. Additionally when the functionality is trying to retrieve the data, it would invalidate the cache, if it is more than 5 minutes old, user is logged out or user has changed.
It stores user
subtaken form theidToken, and list of numeric newsletter subscription ids.Why?
We want to reduce the amount of requests to the identity api and following requests to braze.
Spike in requests to identity API:

Spike in requests from identity to Braze:

Desired outcome
Drop in the amount of requests to
/users/me/newslettersNo drop in the amount of subscriptions from within articles.
Demo video