[WS-2600]: Account Promotional Banner dismissal functionality failing#14009
Open
LukasFrm wants to merge 2 commits into
Open
[WS-2600]: Account Promotional Banner dismissal functionality failing#14009LukasFrm wants to merge 2 commits into
LukasFrm wants to merge 2 commits into
Conversation
…torage for dismissal tracking and remove cookie dependencies
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reverts the Account Promotional Banner dismissal mechanism from cookie/SSR-driven visibility to a client-side localStorage approach, to avoid the performance impact of forcing private caching when passing personalised dismissal cookies through Belfrage.
Changes:
- Removed server-computed
initialIsAccountPromoBannerVisiblefrom IDCTA config andAccountContext. - Deleted cookie-based dismissal utilities/tests and moved dismissal tracking into the banner component using
localStorage. - Updated banner docs, component tests, and Storybook stories to align with the new dismissal storage.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/app/models/types/account.ts | Removes promo banner visibility fields from IDCTA/account context types. |
| src/app/lib/idcta/getIdctaConfig/index.ts | Stops computing/passing initial promo banner visibility from request cookies. |
| src/app/lib/idcta/getIdctaConfig/index.test.ts | Removes tests for the removed config field. |
| src/app/contexts/AccountContext/index.tsx | Drops promo banner visibility from context value. |
| src/app/components/SaveButton/index.stories.tsx | Updates mocked AccountContext provider value shape. |
| src/app/components/Account/AccountPromotionalBanner/utilities/index.ts | Deletes cookie-based dismissal implementation. |
| src/app/components/Account/AccountPromotionalBanner/utilities/index.test.ts | Deletes tests for removed cookie-based utilities. |
| src/app/components/Account/AccountPromotionalBanner/README.md | Updates documentation to describe localStorage-based dismissal. |
| src/app/components/Account/AccountPromotionalBanner/index.tsx | Implements localStorage-based dismissal/visibility logic. |
| src/app/components/Account/AccountPromotionalBanner/index.test.tsx | Updates tests to assert localStorage behaviour. |
| src/app/components/Account/AccountPromotionalBanner/index.stories.tsx | Updates mocked AccountContext provider value shape. |
| src/app/components/Account/AccountHeader/index.stories.tsx | Updates mocked AccountContext provider value shape. |
Comments suppressed due to low confidence (3)
src/app/components/Account/AccountPromotionalBanner/index.tsx:35
getBannerDismissals()/getBannerLastDismissed()can returnNaNwhen localStorage contains malformed data (e.g. user/devtools edits). That propagates intosetBannerDismissed()(NaN + 1) and can effectively break dismissal persistence by writing"NaN". Consider normalising parsed values to0whenNumber.isNaN(...)/ not finite, similar to the previousparseIntOrZerobehaviour that existed for cookie parsing.
const getBannerDismissals = () => {
const accountBannerDismissValue = localStorage.getItem(
ACCOUNT_BANNER_DISMISS_KEY,
);
return parseInt(accountBannerDismissValue ?? '0', 10);
};
const getBannerLastDismissed = () => {
const accountBannerLastDismissValue = localStorage.getItem(
ACCOUNT_BANNER_LAST_DISMISS_KEY,
);
return parseInt(accountBannerLastDismissValue ?? '0', 10);
};
const setBannerDismissed = () => {
const dismissals = getBannerDismissals() + 1;
localStorage.setItem(ACCOUNT_BANNER_DISMISS_KEY, String(dismissals));
localStorage.setItem(ACCOUNT_BANNER_LAST_DISMISS_KEY, String(Date.now()));
};
src/app/components/Account/AccountPromotionalBanner/index.tsx:45
- All localStorage reads/writes here assume storage access is always available. In some environments (Safari private mode, strict privacy settings, embedded contexts)
localStorage.getItem/setItemcan throw and crash the component. Please add atypeof window !== 'undefined'guard and/or wrap storage access intry/catch, falling back to a safe default (e.g. treat as visible and ignore persistence) on error.
const getBannerDismissals = () => {
const accountBannerDismissValue = localStorage.getItem(
ACCOUNT_BANNER_DISMISS_KEY,
);
return parseInt(accountBannerDismissValue ?? '0', 10);
};
const getBannerLastDismissed = () => {
const accountBannerLastDismissValue = localStorage.getItem(
ACCOUNT_BANNER_LAST_DISMISS_KEY,
);
return parseInt(accountBannerLastDismissValue ?? '0', 10);
};
const setBannerDismissed = () => {
const dismissals = getBannerDismissals() + 1;
localStorage.setItem(ACCOUNT_BANNER_DISMISS_KEY, String(dismissals));
localStorage.setItem(ACCOUNT_BANNER_LAST_DISMISS_KEY, String(Date.now()));
};
const isBannerVisible = () => {
const dismissals = getBannerDismissals();
const lastDismissed = getBannerLastDismissed();
const now = Date.now();
if (dismissals >= ACCOUNT_BANNER_MAX_DISMISSALS) return false;
if (lastDismissed && now - lastDismissed < ACCOUNT_BANNER_DISMISS_INTERVAL_MS)
return false;
return true;
};
src/app/components/Account/AccountPromotionalBanner/index.tsx:61
isDismissedinitialises tofalseand dismissal is only applied in auseEffect. This means previously-dismissed users will still get the banner in the initial render/SSR HTML (and it may disappear only after hydration), which undermines the dismissal behaviour and can cause a noticeable flash. Consider deferring render until after a client-side visibility check has run, or otherwise ensuring dismissal state is respected before the banner is first painted.
const [isDismissed, setIsDismissed] = useState(false);
useEffect(() => {
if (!isBannerVisible()) {
setIsDismissed(true);
}
}, []);
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.
Resolves JIRA: https://bbc.atlassian.net/browse/WS-2600
Summary
Passing personalized dismissal cookies through Belfrage would introduce a performance overhead due to private cache being hit instead of a public one.
This PR reverts the Promotional Banner dismissal logic to localStorage.
To be followed up with PS's client-side-conditions-script pre-CSR injection approach.
Code changes
Testing
Useful Links