Skip to content
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

JS console error appears when pressing "Set up now" on the GA4 Activation Banner #5760

Closed
techanvil opened this issue Aug 26, 2022 · 4 comments
Labels
P1 Medium priority Type: Bug Something isn't working

Comments

@techanvil
Copy link
Collaborator

techanvil commented Aug 26, 2022

Bug Description

When pressing the Set up now button on the GA4 Activation Banner an error appears in the JS console. See screenshots below.

Steps to reproduce

  1. Ensure the GA4 Activation Banner is visible on the Dashboard. One way to achieve this is to connect Analytics and then delete the googlesitekit_analytics-4_settings WordPress option. Session storage may also need to be cleared.
  2. Click on Set up now.
  3. Observe the JS error in the console.

Screenshots

1. Press Set up now.
image.png

2. The banner successfully moves to the next stage, but an error appears in the console:
image.png

Additional Context

  • PHP Version: any
  • OS: any
  • Browser: any
  • Plugin Version: spotted on 1.82.0
  • Device: any

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • No JS console error should appear when pressing Set up now on the GA4 Activation Banner.

Implementation Brief

  • Being a trivial commit (3 line change), I have just pushed my changes to the attached PR which can be moved straight to QA if it looks good.
  • In BannerNotification/index.js, use the useMountedState() hook from react-use which returns a function that will check if the component is mounted or not.
  • In handleCTAClick(), use this function to wrap the state update setIsAwaitingCTAResponse( false ); which should only happen if the component is mounted. (The error occurs on this line as the line before changes the step state variable in the ActivationBanner component causing the child components to re-render).

Test Coverage

  • No new tests required.

QA Brief

  • Follow the steps to reproduce and verify the console error does not show.
  • Also verify existing Banner Notifications, especially the Module Recovery Alert notification which should all still work normally (dismissing, clicking on CTAs, etc.).

Changelog entry

  • Prevent updating Banner Notification component state when unmounted.
@techanvil techanvil added P1 Medium priority Type: Bug Something isn't working labels Aug 26, 2022
@jimmymadon jimmymadon assigned jimmymadon and unassigned jimmymadon Aug 31, 2022
@techanvil techanvil self-assigned this Sep 5, 2022
@techanvil
Copy link
Collaborator Author

techanvil commented Sep 5, 2022

Hey @jimmymadon - good shout on the use of useMountedState rather than rolling our own hook.

I've spotted another place where we could conceivably see the same error occurring. As dismissNotification is called after onCTAClick, this call to setIsDismissed could also be called when the component is no longer mounted. Maybe we should also wrap this in a check to isMounted(). Not super elegant having to call it in two places but I don't really see an obviously better refactor. What do you think?

I mean it's slighlty preemptive as it's not the specific call that causes the error, but would save us detective work if this one does come and bite us in future...

@techanvil techanvil assigned jimmymadon and unassigned techanvil Sep 5, 2022
@jimmymadon
Copy link
Collaborator

@techanvil Nice catch - have updated the PR!

@jimmymadon jimmymadon assigned techanvil and unassigned jimmymadon Sep 5, 2022
@techanvil
Copy link
Collaborator Author

Nice one @jimmymadon.

IB ✅

@techanvil techanvil removed their assignment Sep 6, 2022
@wpdarren wpdarren self-assigned this Sep 6, 2022
@wpdarren
Copy link
Collaborator

wpdarren commented Sep 6, 2022

QA Update: ✅

Verified:

  • The console error does not show when you click on the Set up now CTA button.
  • Existing Banner Notifications, i.e. Module Recovery Alert notification should continue to work (see note)

Note: Currently, the zero and gathering data state notifications are not working due to the bug in #5148 which is currently being worked on. I checked to make sure the other banner notifications appeared.

image

image

@wpdarren wpdarren removed their assignment Sep 6, 2022
@felixarntz felixarntz assigned felixarntz and unassigned felixarntz Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Medium priority Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants