-
Notifications
You must be signed in to change notification settings - Fork 365
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
refactor: [M3-6571] - RQ-ify Events - Part 2: Event Handlers #9293
refactor: [M3-6571] - RQ-ify Events - Part 2: Event Handlers #9293
Conversation
import { storeFactory } from './store'; | ||
|
||
const store = storeFactory(queryClientFactory()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We no longer need to associate the Redux store with the queryClient.
Why this was done before: event handlers used to be registered in the Redux store (via combineEventsMiddleware
). Some of these handlers needed access to the queryClient (e.g., to invalidate queries when entities got updates).
Since event handlers are now handled through a hook, they can receive the queryClient using the useQueryClient
hook.
@@ -354,7 +353,6 @@ const MainContent = (props: CombinedProps) => { | |||
</> | |||
</NotificationProvider> | |||
<Footer desktopMenuIsOpen={desktopMenuIsOpen} /> | |||
<ToastNotifications /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored this to be a hook since it doesn't render anything.
@@ -84,9 +83,6 @@ export class AuthenticationWrapper extends React.Component<CombinedProps> { | |||
this.props.checkAccountSize(), | |||
]; | |||
|
|||
// Start events polling | |||
startEventsInterval(this.props.store, this.props.queryClient); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was used to initiate the Redux-based events architecture. Now polling is automatically handled by the RQ hook.
@@ -101,6 +101,8 @@ export const CreateImageTab: React.FC<Props> = (props) => { | |||
const { mutateAsync: createImage } = useCreateImageMutation(); | |||
const hasMetadataCustomerTag = useMetadataCustomerTag(); | |||
|
|||
const { resetEventsPolling } = useEventsInfiniteQuery({ enabled: false }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resetEventsPolling
is now exposed by the hook instead of being a global function.
packages/manager/src/features/Linodes/LinodesDetail/LinodeAdvanced/LinodeDiskRow.tsx
Outdated
Show resolved
Hide resolved
@@ -37,7 +38,6 @@ import { | |||
} from 'src/store/linodes/disk/disk.requests'; | |||
import { updateLinode as _updateLinode } from 'src/store/linodes/linode.requests'; | |||
import { ThunkDispatch } from 'src/store/types'; | |||
import { ExtendedLinode } from './types'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are 5 definitions of ExtendedLinode
in the codebase -- combining these two since they are identical.
|
||
export const isEventRelevantToLinode = (event: Event, linodeId: number) => | ||
isPrimaryEntity(event, linodeId) || | ||
(isSecondaryEntity(event, linodeId) && | ||
isEventRelevantToLinodeAsSecondaryEntity(event)); | ||
|
||
export const isPrimaryEntity = (event: Event, linodeId: number) => | ||
event?.entity?.type === 'linode' && event?.entity?.id === linodeId; | ||
|
||
export const isSecondaryEntity = (event: Event, linodeId: number) => | ||
event?.secondary_entity?.type === 'linode' && | ||
event?.secondary_entity?.id === linodeId; | ||
|
||
// Some event types include a Linode as a `secondary_entity`. A subset of these | ||
// events should be included in the `eventsForLinode` selector since they are | ||
// relevant to that Linode. | ||
// | ||
// An example: `clone_linode` events include the source Linode as the `entity` | ||
// and the target Linode as the `secondary_entity`. In this case, we want the | ||
// consumer of the `eventsForLinode` selector to have access to these events so | ||
// it can do things like display progress bars. | ||
export const eventActionsForLinodeAsSecondaryEntity: EventAction[] = [ | ||
'linode_clone', | ||
]; | ||
export const isEventRelevantToLinodeAsSecondaryEntity = (event: Event) => | ||
eventActionsForLinodeAsSecondaryEntity.includes(event?.action); | ||
|
||
export const isEntityEvent = (e: Event): e is EntityEvent => Boolean(e.entity); | ||
|
||
export const isEventInProgressDiskImagize = (event: Event): boolean => { | ||
return ( | ||
event.action === 'disk_imagize' && | ||
Boolean(event.secondary_entity) && | ||
isInProgressEvent(event) | ||
); | ||
}; | ||
|
||
export const isEventImageUpload = (event: Event): boolean => { | ||
return event.action === 'image_upload'; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Migrated over from event.helpers.ts
.
Noticing a couple of issues while testing:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I haven't looked through all 3000 lines of code, but I have tested functionality.
Notifications in two separate tabs seem to be updating across both tabs for seen/unseen and in progress events. In progress events update until completion with page refreshes. Toasts look good and are consistent across tabs.
The only thing I've noticed is that the Events Landing page seems to no longer display in progress events at the top of the list. In progress events are still displayed at the top of the notification menu, as expected.
Steps to replicate:
- Kick off a migration, since it takes some time to complete
- Perform some actions that will also trigger events while the migration is in progress
- Check the landings page and observe that the in-progress migration event is farther down the table than it is in prod, where it is at the top
This branch:
Good eye @mjac0bs. Added sorting in-progress events to the top in |
PR is looking solid but I don't quite understand why the events load into the dom in batches less then 25... What am I not seeing in the code that causes this? Screen.Recording.2023-07-17.at.9.50.45.AM.mov |
Line 169 in
|
packages/manager/src/features/Linodes/LinodesDetail/LinodeStorage/LinodeDiskRow.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job 🎉 Not seeing any major issues!
So happy to see rxjs
gone. Also impressed by the much needed clean up you were able to include in the PR (useToastNotifications, etc..)
I will do further testing when this is merged into the feature branch
enqueueSnackbar, | ||
eventStatus: event.status, | ||
successMessage: `Linode ${label} migrated successfully.`, | ||
failureMessage: `Linode ${label} migration failed.`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the whole idea of the toastSuccessAndFailure
could be cleaned up. I think @hkhalil-akamai made some great clean up here but I think we can go further.
Rather than maintaining a switch statment, maybe we could have some kind of map?
const toasts = {
volume_attach: {
eventStatus: event.status,
failureMessage: `Error attaching Volume ${label}.`,
successMessage: `Volume ${label} successfully attached.`,
},
volume_detach: {
eventStatus: event.status,
failureMessage: `Error detaching Volume ${label}.`,
successMessage: `Volume ${label} successfully detached.`,
},
} as const;
const toast = toasts[event.action as keyof typeof toasts];
toastSuccessAndFailure({
enqueueSnackbar,
...toast,
});
* refactor: [M3-6570] - RQ-ify Events - Part 1: NotificationMenu, EventsLanding (#9046) * Initial idea for RQ events Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Initial working version of useEventsPolling hook Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Re-implementation of useEventsPolling hook Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Completed initial implementation of useEvents hook Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Invalidate events cache when new events are fetched Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Use read instead of seen flag for polling Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Implement polling interval Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Switch to RQ hook for notifications menu Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Switch EventsLanding to RQ hook and enhance top-menu Fixes notification menu button background color disappearing on hover in dark mode Fix Notification menu doesn't close when navigating to EventsLanding Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Implement markEventsAsSeen mutation Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Temporarily re-enable redux polling Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Track previously existing in-progress events Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Remove old test Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Move NotificationMenu styled component to dedicated file * Follow style guide in EventsLanding * Rename TopMenuIconWrapper * fix: Re-add `fs` import to Cypress config (#9199) * add `fs` import * use cleaner import --------- Co-authored-by: Banks Nussman <banks@nussman.us> * refactor: [M3-6382] - MUI v5 - `Components > Radio` (#9185) * move styles to theme and update stories * remove incorrect color --------- Co-authored-by: Banks Nussman <banks@nussman.us> * refactor: [M3-6411] - MUI v5 - `Components > Tag` (#9164) ## Description 📝 Migrate styles for the Tag component ## How to test 🧪 Confirm that there have been no Tag regressions across the app Co-authored-by: Jaalah Ramos <jaalah.ramos@gmail.com> * refactor: [M3-6412, M3-6413, M3-6650] - MUI v5 - `Components > TagCell, Tags, AddTag` (#9186) ## Description 📝 Migrate styles for the TagCell and AddTag component Update Tags component to match our current patterns ## How to test 🧪 - Check tags around the app for regressions - Ensure the Tags story renders in Storybook --------- Co-authored-by: Hussain Khalil <hussain@sanpilot.co> Co-authored-by: Banks Nussman <115251059+bnussman-akamai@users.noreply.github.com> Co-authored-by: Banks Nussman <banks@nussman.us> Co-authored-by: Hana Xu <115299789+hana-linode@users.noreply.github.com> Co-authored-by: Jaalah Ramos <jaalah.ramos@gmail.com> * Undo no-longer needed changes to top menu icons * refactor: [M3-6571] - RQ-ify Events - Part 2: Event Handlers (#9293) * Make polling part of EventsInfiniteQuery * Move event utilities to utilities folder * Create events query container * Migrate App events handlers to new events hook * Migrate toast notifications to new hook * Move app event handlers to hook * Finish migrating event handlers * Remove events redux store * Fix failing test * Fix issue causing multiple pages of events to refresh * implement page zero for new events * Optimize/reduce calls to events endpoint * Implement rolling-time polling * Fix events being duplicated * Fix unsupported API filter * Fix index out of bounds bug * Fix broken in-progress events polling * Get rid of separate page zero cache and store new events in first page instead * Undo accidental changes to cachedData * remove leftover pageZero stuff * Fix undefined recent event * Start from page 1 * Feedback @jaalah-akamai * New hook for synchronizing polling intervals * Remove unused exports * Custom implementation of partition util * Sort in-progress events to top * Use events instead of data * Use React.useMemo * Fix state accidental deletion * Clean up useToastNotifications * Add changesets * Remove Redux connector in EventsLanding * Rename to NotificationMenu.styles.ts * nodebalanacer -> nodebalancer * Import from src/components instead of MUI * Replace CombinedProps with EventsLandingProps --------- Co-authored-by: Hussain Khalil <hussain@sanpilot.co> Co-authored-by: Banks Nussman <115251059+bnussman-akamai@users.noreply.github.com> Co-authored-by: Banks Nussman <banks@nussman.us> Co-authored-by: Hana Xu <115299789+hana-linode@users.noreply.github.com> Co-authored-by: Jaalah Ramos <jaalah.ramos@gmail.com>
* refactor: [M3-6570] - RQ-ify Events - Part 1: NotificationMenu, EventsLanding (linode#9046) * Initial idea for RQ events Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Initial working version of useEventsPolling hook Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Re-implementation of useEventsPolling hook Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Completed initial implementation of useEvents hook Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Invalidate events cache when new events are fetched Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Use read instead of seen flag for polling Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Implement polling interval Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Switch to RQ hook for notifications menu Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Switch EventsLanding to RQ hook and enhance top-menu Fixes notification menu button background color disappearing on hover in dark mode Fix Notification menu doesn't close when navigating to EventsLanding Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Implement markEventsAsSeen mutation Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Temporarily re-enable redux polling Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Track previously existing in-progress events Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Remove old test Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Move NotificationMenu styled component to dedicated file * Follow style guide in EventsLanding * Rename TopMenuIconWrapper * fix: Re-add `fs` import to Cypress config (linode#9199) * add `fs` import * use cleaner import --------- Co-authored-by: Banks Nussman <banks@nussman.us> * refactor: [M3-6382] - MUI v5 - `Components > Radio` (linode#9185) * move styles to theme and update stories * remove incorrect color --------- Co-authored-by: Banks Nussman <banks@nussman.us> * refactor: [M3-6411] - MUI v5 - `Components > Tag` (linode#9164) ## Description 📝 Migrate styles for the Tag component ## How to test 🧪 Confirm that there have been no Tag regressions across the app Co-authored-by: Jaalah Ramos <jaalah.ramos@gmail.com> * refactor: [M3-6412, M3-6413, M3-6650] - MUI v5 - `Components > TagCell, Tags, AddTag` (linode#9186) ## Description 📝 Migrate styles for the TagCell and AddTag component Update Tags component to match our current patterns ## How to test 🧪 - Check tags around the app for regressions - Ensure the Tags story renders in Storybook --------- Co-authored-by: Hussain Khalil <hussain@sanpilot.co> Co-authored-by: Banks Nussman <115251059+bnussman-akamai@users.noreply.github.com> Co-authored-by: Banks Nussman <banks@nussman.us> Co-authored-by: Hana Xu <115299789+hana-linode@users.noreply.github.com> Co-authored-by: Jaalah Ramos <jaalah.ramos@gmail.com> * Undo no-longer needed changes to top menu icons * refactor: [M3-6571] - RQ-ify Events - Part 2: Event Handlers (linode#9293) * Make polling part of EventsInfiniteQuery * Move event utilities to utilities folder * Create events query container * Migrate App events handlers to new events hook * Migrate toast notifications to new hook * Move app event handlers to hook * Finish migrating event handlers * Remove events redux store * Fix failing test * Fix issue causing multiple pages of events to refresh * implement page zero for new events * Optimize/reduce calls to events endpoint * Implement rolling-time polling * Fix events being duplicated * Fix unsupported API filter * Fix index out of bounds bug * Fix broken in-progress events polling * Get rid of separate page zero cache and store new events in first page instead * Undo accidental changes to cachedData * remove leftover pageZero stuff * Fix undefined recent event * Start from page 1 * Feedback @jaalah-akamai * New hook for synchronizing polling intervals * Remove unused exports * Custom implementation of partition util * Sort in-progress events to top * Use events instead of data * Use React.useMemo * Fix state accidental deletion * Clean up useToastNotifications * Add changesets * Remove Redux connector in EventsLanding * Rename to NotificationMenu.styles.ts * nodebalanacer -> nodebalancer * Import from src/components instead of MUI * Replace CombinedProps with EventsLandingProps --------- Co-authored-by: Hussain Khalil <hussain@sanpilot.co> Co-authored-by: Banks Nussman <115251059+bnussman-akamai@users.noreply.github.com> Co-authored-by: Banks Nussman <banks@nussman.us> Co-authored-by: Hana Xu <115299789+hana-linode@users.noreply.github.com> Co-authored-by: Jaalah Ramos <jaalah.ramos@gmail.com>
* refactor: [M3-6570] - RQ-ify Events - Part 1: NotificationMenu, EventsLanding (linode#9046) * Initial idea for RQ events Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Initial working version of useEventsPolling hook Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Re-implementation of useEventsPolling hook Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Completed initial implementation of useEvents hook Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Invalidate events cache when new events are fetched Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Use read instead of seen flag for polling Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Implement polling interval Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Switch to RQ hook for notifications menu Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Switch EventsLanding to RQ hook and enhance top-menu Fixes notification menu button background color disappearing on hover in dark mode Fix Notification menu doesn't close when navigating to EventsLanding Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Implement markEventsAsSeen mutation Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Temporarily re-enable redux polling Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Track previously existing in-progress events Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Remove old test Co-authored-by: Hussain Khalil <hussain@sanpilot.co> * Move NotificationMenu styled component to dedicated file * Follow style guide in EventsLanding * Rename TopMenuIconWrapper * fix: Re-add `fs` import to Cypress config (linode#9199) * add `fs` import * use cleaner import --------- Co-authored-by: Banks Nussman <banks@nussman.us> * refactor: [M3-6382] - MUI v5 - `Components > Radio` (linode#9185) * move styles to theme and update stories * remove incorrect color --------- Co-authored-by: Banks Nussman <banks@nussman.us> * refactor: [M3-6411] - MUI v5 - `Components > Tag` (linode#9164) ## Description 📝 Migrate styles for the Tag component ## How to test 🧪 Confirm that there have been no Tag regressions across the app Co-authored-by: Jaalah Ramos <jaalah.ramos@gmail.com> * refactor: [M3-6412, M3-6413, M3-6650] - MUI v5 - `Components > TagCell, Tags, AddTag` (linode#9186) ## Description 📝 Migrate styles for the TagCell and AddTag component Update Tags component to match our current patterns ## How to test 🧪 - Check tags around the app for regressions - Ensure the Tags story renders in Storybook --------- Co-authored-by: Hussain Khalil <hussain@sanpilot.co> Co-authored-by: Banks Nussman <115251059+bnussman-akamai@users.noreply.github.com> Co-authored-by: Banks Nussman <banks@nussman.us> Co-authored-by: Hana Xu <115299789+hana-linode@users.noreply.github.com> Co-authored-by: Jaalah Ramos <jaalah.ramos@gmail.com> * Undo no-longer needed changes to top menu icons * refactor: [M3-6571] - RQ-ify Events - Part 2: Event Handlers (linode#9293) * Make polling part of EventsInfiniteQuery * Move event utilities to utilities folder * Create events query container * Migrate App events handlers to new events hook * Migrate toast notifications to new hook * Move app event handlers to hook * Finish migrating event handlers * Remove events redux store * Fix failing test * Fix issue causing multiple pages of events to refresh * implement page zero for new events * Optimize/reduce calls to events endpoint * Implement rolling-time polling * Fix events being duplicated * Fix unsupported API filter * Fix index out of bounds bug * Fix broken in-progress events polling * Get rid of separate page zero cache and store new events in first page instead * Undo accidental changes to cachedData * remove leftover pageZero stuff * Fix undefined recent event * Start from page 1 * Feedback @jaalah-akamai * New hook for synchronizing polling intervals * Remove unused exports * Custom implementation of partition util * Sort in-progress events to top * Use events instead of data * Use React.useMemo * Fix state accidental deletion * Clean up useToastNotifications * Add changesets * Remove Redux connector in EventsLanding * Rename to NotificationMenu.styles.ts * nodebalanacer -> nodebalancer * Import from src/components instead of MUI * Replace CombinedProps with EventsLandingProps --------- Co-authored-by: Hussain Khalil <hussain@sanpilot.co> Co-authored-by: Banks Nussman <115251059+bnussman-akamai@users.noreply.github.com> Co-authored-by: Banks Nussman <banks@nussman.us> Co-authored-by: Hana Xu <115299789+hana-linode@users.noreply.github.com> Co-authored-by: Jaalah Ramos <jaalah.ramos@gmail.com>
Description 📝
See Part 1 here: #9046.
This PR finishes off the RQ Events migration, moving all event handlers over from the old system (Redux middleware + rxjs observable) to a unified events "mega-hook".
The 2 hooks from the last PR are combined into a single hook
useEventsInfiniteQuery
, which handles both fetching events and allows for registering event handlers for new/updated events. It was necessary to combine the polling hook into this one because we use previously-fetched events to create a list of in-progresseventIds
to poll for updates.The event handlers themselves have been moved to two new hooks,
useAppEventHandlers
anduseToastNotifications
, from<App />
where most of them used to live (a few others were scattered around the codebase -- they are all now in one place). I decided to move them out ofApp
to reduce clutter in that file; moving them to hooks instead of components seemed appropriate because they don't render any content.This PR also introduces a container,
withEventsInfiniteQuery
that allows our class-based components to use events.What exactly do Events do ❓
Events have a few different functions in Cloud Manager:
Major Changes 🔄
useEventsInfiniteQuery
, for retrieving events and registering event handlersuseAppEventHandlers
anduseToastNotifications
rxjs
as a dependencyHow to test 🧪
useToastNotifications.tsx
for examples of this, such as attaching a volume, resizing a disk, or migrating a Linode). Ensure this toast actually appears in both tabs.Additional things to check:
useAppEventHandlers
are registered and invoked as expecteduseToastNotifications
work as expectedresetPollingInterval
) is correctly reset when adding/updating/deleting/performing actions on entities