refactor: [M3-6570] - RQ-ify Events - Part 1: NotificationMenu, EventsLanding

[hkhalil-akamai]

Description 📝

Part 1 implements the RQ hooks in the Notifications menu and EventsLanding page. Part 2 will cover event handlers.

This PR migrates all Events endpoints to two React Query hooks (useEventsInfiniteQuery for fetching event notifications and useEventsPolling for registering event handlers) while hugely simplifying the current implementation.

The current Redux-based events system in Cloud Manager is pretty complex, making it difficult to understand and develop. Functionality is defined across several files, including events.ts, eventsPolling.ts, event.request.ts, event.reducer.ts, event.actions.ts and event.helpers.ts. Event handlers are used for important app functions like snackbar/toasts and cache invalidation but are defined via two redundant mechanisms (an rxjs subject and a custom Redux middleware).

The current system handles a lot of considerations and edge cases that must be accounted for:

• Polling rate backoff to minimize API load
• Increasing polling during background operations
• Updates to in-progress events
• Avoiding 'replaying' previously seen events
• Separate read statuses for separate users under a single account

The plan is to reduce complexity by consolidating all Events-related functionality to these two hooks, remove the singular usage of the rxjs library and leverage the API-provided seen and read flags on events to avoid the complicated logic needed for maintaining a local events store.

How to test 🧪

Verify the notifications menu, which can be opened by clicking the bell icon, works as expected:

• The icon displays the number of unseen notifications
• In-progress events are rendered with a progress bar and are continuously updated
• After closing the menu, events are marked as seen and the count disappears

Verify EventsLanding (accessed by opening the notifications menu and clicking "View all events"), works as expected:

• Initially, the last 25 events are displayed
• Scrolling down triggers additional events to be displayed
• In-progress events are rendered with a progress bar and are continuously updated
• New events are automatically displayed at the top of the list

Verify Linode Activity Feed, accessed by navigating to a linode and clicking on the "Activity Feed" tab, works as expected:

• Only events related to the selected Linode are displayed
• Same criteria as EventsLanding

Open DevTools Networking tab and filter to 'events' requests. Verify the polling interval starts at once per second and backs-off to once per 16 seconds (currently, polling is duplicated since the existing Events architecture is still enabled; it will be removed in Part 2).

Verify unit and E2E tests pass.

[cypress]

5 flaky tests on run #3978 ↗︎

0

167

3

0

5

Details:

Merge remote-tracking branch 'upstream/develop' into M3-5181-rq-events
Project: Cloud Manager E2E	Commit: 769dd52d4c
Status: Passed	Duration: 16:51 💡
Started: Jun 2, 2023 5:23 PM	Ended: Jun 2, 2023 5:39 PM

  images/machine-image-upload.spec.ts • 3 flaky tests

View Output Video

Test		Artifacts
machine image > uploads machine image, mock finish event		Output Screenshots Video
machine image > uploads machine image, mock upload canceled failed event		Output Screenshots Video
machine image > uploads machine image, mock expired upload event		Output Screenshots Video

  firewalls/update-firewall.spec.ts • 1 flaky test

View Output Video

Test		Artifacts
update firewall > updates a firewall's linodes and rules		Output Screenshots Video

  linodes/rescue-linode.spec.ts • 1 flaky test

View Output Video

Test		Artifacts
Rescue Linodes > Cannot reboot a provisioning Linode into rescue mode		Output Screenshots Video

_{This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.}

[bnussman-akamai]

I think it would be cleaner for this logic to live in its own function or in the onSuccess function of useEventsPolling. I don't think useEvents should necessarily be dependent on useEventsPolling like it is right now.

[hkhalil-akamai]

Do you anticipate any issues with the dependency? I think composing the hooks like this de-deduplicates some code.

[bnussman-akamai]

Because useEventsPolling is just a wrapper over useQuery, I think for the sake of maintainability, we should try to keep this wrapper as "React Query"-like as possible. I think it would be good if we keep the param here of type UseQueryOptions. If you think (event: Event) => void is truly better, we can keep it, but I generally like to keep things as "React Query"-like as possible.

[hkhalil-akamai]

I am more in favor of keeping the signature and usage of this hook as simple as possible. Even though it uses RQ under the hook, it's not really relevant to components that use it.

[bnussman-akamai]

Is reverseing needed?

[hkhalil-akamai]

This is done to ensure the callback is called in chronological order; by default, events are sorted with the most recent events coming first.

[bnussman-akamai]

Things are shaping up nicely. Awesome job so far.

[bnussman-akamai]

Also, I'm wondering if await markCompletedEventsAsRead(events); should be moved into the onSuccess. I think that would be more appropriate because we don't want this query to report an error if markCompletedEventsAsRead throws an error

[hkhalil-akamai]

This was how I did it initially but the onSuccess callback is executed multiple times if there are multiple instances of this hook mounted. Having it in the actual request avoids this. I investigated alternatives to onSuccess that only execute once but I don't think RQ has that option.

[bnussman-akamai]

Testing on my person account with thousands of unread events caused events to be continuously marked as read every time the internal runs.

This isn't that bad because it does all of this pretty gracefully but I worry we will run into a rate limit or hit the API pretty hard.

I think we talked about this before and a possible solution may be to add a { created: { '+gte': timestampMountedAt } }

[hkhalil-akamai]

Capturing our discussion here for future reference:

I implemented this idea but @bnussman-akamai pointed out that filtering out events that occur before the hook was mounted would ignore previous in-progress events.

Possible ways to mitigate this include: modifying the API to allow filtering on events' status and percent_complete fields or the updated field.

The current working solution to this is that the useEventsInfiniteQuery hook will report in-progress events to the useEventsPolling hook to poll separately from new events.

[bnussman-akamai]

makes since but I worry about multiple sources of polling. I'm starting to lean towards one more API change to solve the edge case of capturing in-progress events created before mount.

[hkhalil-akamai]

Another option is to fix filtering not working when 'read' is used within '+and' and '+or'. Then, we could use a single polling hook and OR the two conditions together -- this would be more in line with what we had before.

[hkhalil-akamai]

If you're reviewing the diff, I would recommend starting at queries/events.ts to see the new hooks and how they work.

[dwiley-akamai]

LGTM 🎉

The only issue I'm seeing right now is when you power off a Linode, the percentage starts at 100% before adjusting to 0% when it first begins. I think this popped up on another PR recently but I don't see it in prod or develop, so merging the latest develop into the feature branch after this PR gets merged should probably solve that.

[mjac0bs]

when you power off a Linode, the percentage starts at 100% before adjusting to 0% when it first begins. I think this popped up on another PR recently

This fix was in the v1.93.1 hotfix PR.

Also fixed in that PR was the regression where clicking on a secondary status (e.g. Migrating (0%)) on Linodes Landing or Linodes Detail didn't open the notification menu. We'll just want to make sure that fix is still looking good once develop is merged into this feature branch.

One thing I noticed on this branch but not prod or develop:

This Branch	Prod

When a linode is cloned, once the event is complete, the linode we're cloning from has a secondary status of Cloning (100%) that doesn't go away on the Linode Details page. I think this is most likely a bug we introduced and then fixed in the v1.93.0 (re)release process, but I hadn't noticed it before this branch, so wanted to note it.

[hkhalil-akamai]

@dwiley-akamai @mjac0bs merging in the latest develop seems to fix most of issues we were seeing.

[mjac0bs]

Tested notification functionality in various scenarios and pages and observed all the things you noted in the steps to test. Also confirmed that the issues we were seeing before (starting at 100% to power on/off, Cloning stuck at 100%, and the secondary status not opening the menu) were all resolved once develop was merged in.

Looks like you have a few other comments on the code to circle back to, but I'm going to go ahead and approve this. Thanks for your clear PR description, that was great!

[mjac0bs]

Good call. 👍

[mjac0bs]

I was trying to figure out what the inconsistency was here, but couldn't seem to spot it. 🔍 I'll trust that it's fixed and this wasn't meant to address the blue button background in dark mode, which we do still need to fix but have a ticket for.

[hkhalil-akamai]

I think I discovered the cause for the failing test ("uploads machine image, mock expired upload event"):

This test intercepts the getEvents call, but since these calls are duplicated for the time being (see here for why), it intercepts the wrong call 50% of the time. I'm going ahead with the merge since this failure should be addressed in part 2 when I remove the previous events polling.

cc @jdamore-linode