google
Improve rendering time of newly refactored subtle notifications
Feature Description
As per this comment thread, there is an increased lag when loading the newly refactored RRM Setup Success Subtle Notification compared to the legacy implementation. This could be because refactored Subtle Notifications are now not rendered immediately simultaneously in the React DOM. They are evaluated along with all other registered Error and Banner notifications and only after all the checkRequirements callbacks are run, then the topmost notification in the queue is rendered.
One additional way of solving this would be to introduce the concept of "ad-hoc" notifications which are registered and added to the queue instantaneously without the entire queue of notifications being (re)evaluated. Issue #9453 has been created to explore this possibility. Hence that issue will block this one to ensure we don't create multiple solutions unnecessarily.
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
- Setup success subtle notifications within the plugin should be rendered without any delay on page load after the setup of the feature they notify about is completed (i.e. the same effect before these notifications were refactored).
Implementation Brief
- [ ]
Test Coverage
QA Brief
Changelog entry
Setup success subtle notifications within the plugin should be rendered without any delay on page load after the setup of the feature they notify about is completed.
@jimmymadon I don't think we should do it this way. I think we need to reconsider our main resolver to be split into area specific resolvers to make sure subtle notifications don't need to wait for other notification checks to be resolved.
@eugene-manuilov Yes - sounds good to me. That was the original problem's solution - the only reason I mentioned the "ad-hoc" approach is because that is being investigated in #9453 as well.
Anyway, the AC here would not change though - as it is generic and just says that there should be no delay between us doing a setup and the notification being displayed.
@aaemnnosttv @jimmymadon, I had the POC branch for this checked out while investigating another issue (https://github.com/google/site-kit-wp/issues/9261) and noticed a couple of issues that needed to be fixed. Evan I hope you don't mind, I've pushed the fix directly to the branch, see https://github.com/google/site-kit-wp/commit/5331d191d21105347febe19362d24c988c701ece.
The tests will of also need updating to reflect the change, but that's something that can no doubt be tackled during implementation.
Thanks @techanvil – I'd like to keep the async util more of a generic utility that we could potentially use elsewhere so I've reverted the addition of the registry there (we can simply apply it before that's called).
I'll pick this up to move forward as I think it's quite close as it is 👍
IB ✅
QA Update ⚠
I tested this and I have 2 queries:
ITEM 1: ⚠ I tested on audience segmentation subtle notification and it looks like there is a delay there. Refer to 0:51 second around in the attached video.
https://github.com/user-attachments/assets/8a9f842d-d2d5-48fa-a742-39ae90dcdf07
ITEM 2: ⚠ I tried to test the linkage of GA4 and Adsense but was not successful. The way I did it is as follows:
- Set up SK with Analytics and Adsense
- I ran the following in console :
googlesitekit.data.dispatch('modules/analytics-4').setAdSenseLinked(true)(Source: https://docs.google.com/document/d/1FtdDt1_AMe9sE-sEkA_ZjWSEhhthkOJi_aLRdr5vHD0/edit?tab=t.0) - However, the subtle notification did not appear. Video is attached for reference. Let me know in case i missed any steps.
- I tried after resetting site kit. I tried changing the analytics property but in vain.
Expected notification:
Video of step by step test:
https://github.com/user-attachments/assets/802a13b1-25f5-49f4-97ba-7d08ce05106f
Other than than, the following subtle notifications were tested good: ✅
-
RRM notifications - these were quite snappy.
https://github.com/user-attachments/assets/1b1adce6-e6b5-498b-8127-f478c4a2bcb9
https://github.com/user-attachments/assets/7081f179-d355-4bd4-b7f6-99f166d453cb
-
Ads Pax Setup
https://github.com/user-attachments/assets/b0eda765-d4f9-4ba2-90e0-4cc946cfee9e
-
Ads conversion ID
https://github.com/user-attachments/assets/fa0a9b99-e670-49be-a012-3e06cb3ee4b5
QA Update: ❌
@aaemnnosttv sending this back to Execution for this point:
I tested on audience segmentation subtle notification and it looks like there is a delay there. See Kelvin's screencast or mine below.
https://github.com/user-attachments/assets/54e26df1-2cd4-4324-9c7c-7874bd994d08
@kelvinballoo For issue 2, did you follow these instructions, re running the cron job and updating the database? I went through the steps and I was able to get the subtle notification and it loaded perfectly quick. Screenshots below.
FYI - it takes 24 hours for the cron job to run. I used my live site to test this. Since we have a few tickets to get into approval lets chat about this on our sync tomorrow. 👍
Screenshots
@kelvinballoo
I tested on audience segmentation subtle notification and it looks like there is a delay there. Refer to 0:51 second around in the attached video.
Are you referring to the delay around 18 seconds? There probably isn't anything we can do about this delay for now.
I tried to test the linkage of GA4 and Adsense but was not successful.
Making a change in the console after notifications have been queued will no longer cause the notification to appear on-demand since the active state of notifications is evaluated once on page load and again in response to specific actions (e.g. the AS setup success as above).
If you change the setting related to the linking instead of trying to set it in the browser you should see the result you are trying to get. As @wpdarren mentioned above, if the linking is set in the DB as usual then it sounds like the notification is working as expected.
I tested on audience segmentation subtle notification and it looks like there is a delay there. See Kelvin's screencast or mine below.
@wpdarren as mentioned above, I'm not sure there is anything we can do about this right now. The reason there is likely a delay is because we need to re-evaluate all other higher priority notifications before we can show this one to avoid the case of one notification flashing first and then being replaced by another. I'm not saying we can't do anything, but it's something we'll work on improving as this work goes on. The important thing for now is that there aren't different notifications visibly competing as described before. This may sound strange given the title of this issue, but the changes here are more fundamental to the way notifications are handled internally to make sure we always show the highest priority notifications as soon as possible, followed by the next, etc. We probably should have made this issue less specific to subtle notifications.
Any other thoughts here @jimmymadon ?
QA Update ✅
Thanks for the update @aaemnnosttv and @wpdarren .
With that, it's good to move to approval.
The following were tested good: ✅
-
RRM notifications - these were quite snappy.
https://github.com/user-attachments/assets/1b1adce6-e6b5-498b-8127-f478c4a2bcb9
https://github.com/user-attachments/assets/7081f179-d355-4bd4-b7f6-99f166d453cb
-
Ads Pax Setup - Snappy too
https://github.com/user-attachments/assets/b0eda765-d4f9-4ba2-90e0-4cc946cfee9e
-
Ads conversion ID - Snappy
https://github.com/user-attachments/assets/fa0a9b99-e670-49be-a012-3e06cb3ee4b5
-
GA4 linked to adsense - loaded perfectly quick from Darren's test.
-
Audience segmentation, we've established that there isn't much we can do right now and it will be part of the ongoing work.
https://github.com/user-attachments/assets/8a9f842d-d2d5-48fa-a742-39ae90dcdf07