fix: React Query Events `seen` and other optimizations #10588

bnussman-akamai · 2024-06-17T21:44:40Z

Description 📝

Fixes cache not updating when marking events as seen 🔧
Fixes possible duplicate event bug on Linode Details page activity feed 🔧
Optimizes Events Polling System ⏲️
- Cuts down the number of fetches to /v4/events on page load
- Only calls POST /v4/account/events/:id/seen when needed (previously it would make this POST every time the Event Menu was closed even if the event was already seen)
- Excludes profile_update events from polling because we don't use them

Preview 📷

Screen.Recording.2024-06-17.at.5.40.57.PM.mov

How to test 🧪

Prerequisites

You need to test with both Events v2 on and off. This PR affects both.

Verification steps

Verify there are no regressions with events polling and all of the items in the description are true

As an Author I have considered 🤔

bnussman-akamai · 2024-06-17T21:45:26Z

packages/manager/src/features/Events/EventsLanding.tsx

@@ -32,7 +32,7 @@ export const EventsLanding = (props: Props) => {
  const { emptyMessage, entityId } = props;
  const flags = useFlags();

-  const filter = EVENTS_LIST_FILTER;
+  const filter = { ...EVENTS_LIST_FILTER };


I was having issues with EVENTS_LIST_FILTER getting mutated and causing issues elsewhere.

How is it getting mutated??

3 lines down we do

filter['entity.id'] = entityId;

I think this is a bug caused by introducing EVENTS_LIST_FILTER. I'm seeing the issue in develop.

Steps to reproduce:

Go to a Linode details page

Go to the Activity Feed tab

Observe the Events Menu is broken

Maybe we can just Object.freeze EVENTS_LIST_FILTER?

You're right. am thinking doing

let filter = { ...EVENTS_LIST_FILTER }; if (entityId) { filter = { ...filter, 'entity.id': entityId, 'entity.type': 'linode', }; }

would be fine, but it won't prevent people from doing that mistake in the future. not sure we can eslint that either. So either that or we don't do this through a sharable const

Don't we have object constants in other places in the app? Maybe a comment would suffice? Otherwise, +1 for Object.freeze, it's also friendly with Typescript.

A code comment re: the mutation danger and why we're doing this is warranted

So either that or we don't do this through a sharable const

I like the idea of not using a shareable const, but do others object to that?

For now I pushed a comment + Object.freeze

bnussman-akamai · 2024-06-17T21:46:29Z

packages/manager/src/features/TopMenu/NotificationMenu/NotificationMenuV2.tsx

-    (notificationItem) => notificationItem.showProgress
-  );
+
+  const showInProgressEventIcon = events?.some(isInProgressEvent);


This might not be what we want. I think previously it is being determined by the factory result

bnussman-akamai · 2024-06-17T21:47:09Z

packages/manager/src/features/TopMenu/NotificationMenu/NotificationMenuV2.tsx

+      if (events && events.length >= 1 && !events[0].seen) {
+        markEventsAsSeen(events[0].id);


Only make the POST if the event is not already seen 🧹

bnussman-akamai · 2024-06-17T21:51:10Z

packages/manager/src/features/TopMenu/NotificationMenu/NotificationMenuV2.tsx

+        <Box>
+          <Box display="flex" justifyContent="space-between" px={2}>
+            <Typography variant="h3">Events</Typography>
+            <LinkButton
+              onClick={() => {
+                history.push('/events');
+                handleClose();
+              }}
+            >
+              View all events
+            </LinkButton>
+          </Box>
+          <Divider spacingBottom={0} />
+          {data?.pages[0].data.slice(0, 20).map((event) => (
+            <RenderEventV2 event={event} key={event.id} onClose={handleClose} />
+          ))}
+        </Box>


This might be too soon, but just let me know @abailly-akamai . Feels much better from a simplicity standpoint. Needs an empty state and maybe other things..

I went this route because I needed to check events[0].seen above and eventNotifications doesn't have seen exposed. I can revert this is you want to handle this separately

Chances are NotificationMenuV2 won't remain like this after the cleanup/refactor and will be further refactored. This is a fine first step 👍

Should we at least move this Box into a new component that receives events?

I'm down unless @abailly-akamai would rather save that for an upcoming PR.

I'll take care of it, no worries. This is still very much in development - I selfishly want to do the cleanup cause it will be so very healing 🤣

bnussman-akamai · 2024-06-17T21:52:14Z

packages/manager/src/queries/events/event.helpers.ts

-  return filter;
+  return {
+    ...filter,
+    ...EVENTS_LIST_FILTER,


This excludes profile_update events when polling because we never use it. This just results in less data over the wire and less cache updates

I am also on the fence about this. On one end we get less cache updates, but those are super cheap. On the other we know how often profile_update is being called so there's value. Is garbage collection a factor here?

Actually, I think profile_update never gets written to the cache so the only benefit here would be less data over the wire

I think excluding it is fine if we never use it

bnussman-akamai · 2024-06-17T21:53:10Z

packages/manager/src/queries/events/event.helpers.ts

+    '+order': 'desc',
+    '+order_by': 'id',


Just being explicit about what order we want from the API to prevent unexpected breakages

bnussman-akamai · 2024-06-17T21:53:30Z

packages/manager/src/queries/events/events.ts

+          '+order': 'desc',
+          '+order_by': 'id',


Just being explicit about what order we want from the API to prevent unexpected breakages

bnussman-akamai · 2024-06-17T21:54:16Z

packages/manager/src/queries/events/events.ts

+        queryClient.setQueriesData<InfiniteData<ResourcePage<Event>>>(
+          ['events', 'infinite'],


This is the primary fix for the seen issue. This now updates all infinite stores rather than just ['events', 'infinite', undefined]

github-actions · 2024-06-18T00:17:50Z

Coverage Report: ✅
Base Coverage: 82.85%
Current Coverage: 82.85%

hkhalil-akamai

✅ seen bug has been resolved. Tested and found no regressions.

hkhalil-akamai · 2024-06-18T15:11:04Z

packages/manager/src/features/TopMenu/NotificationMenu/NotificationMenuV2.tsx

  const { dismissNotifications } = useDismissibleNotifications();
  const { data: notifications } = useNotificationsQuery();
  const formattedNotifications = useFormattedNotifications();
-  const eventNotifications = useEventNotificationsV2();


We are no longer using this hook anywhere

hkhalil-akamai · 2024-06-18T15:14:03Z

packages/manager/src/features/TopMenu/NotificationMenu/NotificationMenuV2.tsx

+        <Box>
+          <Box display="flex" justifyContent="space-between" px={2}>
+            <Typography variant="h3">Events</Typography>
+            <LinkButton
+              onClick={() => {
+                history.push('/events');
+                handleClose();
+              }}
+            >
+              View all events
+            </LinkButton>
+          </Box>
+          <Divider spacingBottom={0} />
+          {data?.pages[0].data.slice(0, 20).map((event) => (
+            <RenderEventV2 event={event} key={event.id} onClose={handleClose} />
+          ))}
+        </Box>


Should we at least move this Box into a new component that receives events?

hkhalil-akamai · 2024-06-18T15:16:44Z

packages/manager/src/features/TopMenu/NotificationMenu/NotificationMenuV2.tsx

+            <LinkButton
+              onClick={() => {
+                history.push('/events');
+                handleClose();
+              }}
+            >
+              View all events
+            </LinkButton>


Should this be a Link instead? Also slight visual regression, in develop this is wrapped in a <strong>

@hkhalil-akamai

Should we at least move this Box into a new component that receives events?

Should this be a Link instead? Also slight visual regression, in develop this is wrapped in a

I will take care of those when I clean up the whole feature once the flag goes away

hkhalil-akamai · 2024-06-18T15:20:31Z

packages/manager/src/features/Events/EventsLanding.tsx

@@ -32,7 +32,7 @@ export const EventsLanding = (props: Props) => {
  const { emptyMessage, entityId } = props;
  const flags = useFlags();

-  const filter = EVENTS_LIST_FILTER;
+  const filter = { ...EVENTS_LIST_FILTER };


Don't we have object constants in other places in the app? Maybe a comment would suffice? Otherwise, +1 for Object.freeze, it's also friendly with Typescript.

dwiley-akamai

Functionality and optimizations look good with no regressions observed ✅

dwiley-akamai · 2024-06-18T15:13:34Z

packages/manager/src/queries/events/event.helpers.ts

-  return filter;
+  return {
+    ...filter,
+    ...EVENTS_LIST_FILTER,


I think excluding it is fine if we never use it

dwiley-akamai · 2024-06-18T15:23:33Z

packages/manager/src/features/Events/EventsLanding.tsx

@@ -32,7 +32,7 @@ export const EventsLanding = (props: Props) => {
  const { emptyMessage, entityId } = props;
  const flags = useFlags();

-  const filter = EVENTS_LIST_FILTER;
+  const filter = { ...EVENTS_LIST_FILTER };


A code comment re: the mutation danger and why we're doing this is warranted

…ations

* initial work * don't poll for `profile_update` events * fixes * update unit test * freeze constant, add comment, delete unused code * Added changeset: React Query Events `seen` behavior and other optimizations --------- Co-authored-by: Banks Nussman <[email protected]>

bnussman added 3 commits June 17, 2024 15:48

initial work

c6e4995

don't poll for profile_update events

811f9e7

fixes

9b7baa7

bnussman-akamai added the React Query Relating to the transition to use React Query label Jun 17, 2024

bnussman-akamai self-assigned this Jun 17, 2024

bnussman-akamai requested a review from a team as a code owner June 17, 2024 21:44

bnussman-akamai requested review from dwiley-akamai and cpathipa and removed request for a team June 17, 2024 21:44

bnussman-akamai commented Jun 17, 2024

View reviewed changes

bnussman-akamai changed the title ~~fix: React Query Events Polling~~ fix: React Query Events seen and other optimizations Jun 17, 2024

update unit test

5728553

hkhalil-akamai approved these changes Jun 18, 2024

View reviewed changes

dwiley-akamai approved these changes Jun 18, 2024

View reviewed changes

freeze constant, add comment, delete unused code

1291441

abailly-akamai self-requested a review June 18, 2024 17:00

abailly-akamai approved these changes Jun 18, 2024

View reviewed changes

Added changeset: React Query Events seen behavior and other optimiz…

e6a6e7d

…ations

bnussman-akamai merged commit 9939b41 into linode:develop Jun 18, 2024
17 of 18 checks passed

This was referenced Jun 21, 2024

Release v1.122.0 - release → staging #10601

Merged

Release v1.122.0 - staging → master #10608

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: React Query Events `seen` and other optimizations #10588

fix: React Query Events `seen` and other optimizations #10588

bnussman-akamai commented Jun 17, 2024

bnussman-akamai Jun 17, 2024

abailly-akamai Jun 17, 2024

bnussman-akamai Jun 18, 2024

bnussman-akamai Jun 18, 2024

abailly-akamai Jun 18, 2024

hkhalil-akamai Jun 18, 2024

dwiley-akamai Jun 18, 2024

bnussman-akamai Jun 18, 2024

bnussman-akamai Jun 17, 2024

bnussman-akamai Jun 17, 2024

bnussman-akamai Jun 17, 2024

abailly-akamai Jun 17, 2024

hkhalil-akamai Jun 18, 2024

bnussman-akamai Jun 18, 2024

abailly-akamai Jun 18, 2024

bnussman-akamai Jun 17, 2024

abailly-akamai Jun 17, 2024

bnussman-akamai Jun 18, 2024

dwiley-akamai Jun 18, 2024

bnussman-akamai Jun 17, 2024

bnussman-akamai Jun 17, 2024

bnussman-akamai Jun 17, 2024

github-actions bot commented Jun 18, 2024 •

edited

Loading

hkhalil-akamai left a comment

hkhalil-akamai Jun 18, 2024

hkhalil-akamai Jun 18, 2024

hkhalil-akamai Jun 18, 2024

abailly-akamai Jun 18, 2024

hkhalil-akamai Jun 18, 2024

dwiley-akamai left a comment

dwiley-akamai Jun 18, 2024

dwiley-akamai Jun 18, 2024

		if (events && events.length >= 1 && !events[0].seen) {
		markEventsAsSeen(events[0].id);

		queryClient.setQueriesData<InfiniteData<ResourcePage<Event>>>(
		['events', 'infinite'],

fix: React Query Events seen and other optimizations #10588

fix: React Query Events seen and other optimizations #10588

Conversation

bnussman-akamai commented Jun 17, 2024

Description 📝

Preview 📷

How to test 🧪

Prerequisites

Verification steps

As an Author I have considered 🤔

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 18, 2024 • edited Loading

hkhalil-akamai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwiley-akamai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fix: React Query Events `seen` and other optimizations #10588

fix: React Query Events `seen` and other optimizations #10588

github-actions bot commented Jun 18, 2024 •

edited

Loading