refactor: [M3-7397] - Clean up `App.tsx` by bnussman-akamai · Pull Request #9897 · linode/manager

bnussman-akamai · 2023-11-13T18:24:06Z

Description 📝

App.tsx has gotten very messy and generally hard to consume. This PR cleans up App.tsx and related pieces. Most changes are just moving code. The largest refactoring happens in useToastNotifications.

Changes 🔄

Cleans up and improves ToastNotifications and makes it useToastNotifications (inspired by refactor: [M3-5181] - RQ-ify Events #9416)
Created useAdobeAnalytics, useNewRelic, useEventHandlers, and useGlobalKeyboardListener to separate out logic

How to test 🧪

Verification steps

Perform a general check of Cloud Manager
Verify global toast notifications still work (creating or deleting a volume for example)
Verify that both Adobe Analytics and New Relic initialize
- This can be verified by looking at the preview link and inspecting the network requests. Verify you see requests to both adobe and New Relic
Verify keyboard shortcuts like Control + Shift + D still work
Verify e2es pass

As an Author I have considered 🤔

bnussman-akamai · 2023-11-13T18:28:42Z

packages/manager/src/App.tsx

-        };
-      }, [handleMigrationEvent]);
+      useAdobeAnalytics();
+      useNewRelic();


It feels kind of wrong to have hooks that don't return anything, but I think it accomplishes what we need.

In fact, useNewRelic probably doesn't even need to be a hook, but for the sake of not making a breaking change, I made it one for now.

bnussman-akamai · 2023-11-13T18:31:23Z

packages/manager/src/hooks/useToastNotifications.tsx

+const toasts: Toasts = {
+  backups_restore: {
+    failure: (e) => `Backup restoration failed for ${getLabel(e)}.`,
+    link: formatLink(
+      'Learn more about limits and considerations.',
+      'https://www.linode.com/docs/products/storage/backups/#limits-and-considerations'
+    ),
+    persistFailureMessage: true,
+  },


I rewrote ToastNotifications.tsx to be cleaner and more extendable. The messages, links, and stuff are all the same.

abailly-akamai

Great cleanup! Nice to see those entry points getting some love 👍

Left a few comments for clarification and/or cleanup

packages/manager/src/hooks/useAdobeAnalytics.ts

abailly-akamai · 2023-11-14T14:18:00Z

packages/manager/src/hooks/useAdobeAnalytics.ts

+          }
+        })
+        .catch(() => {
+          // Do nothing; a user may have analytics script requests blocked.


Am wondering if anyone would care to see events for that.

Not sure. I personally don't mind the silent failure

Silent for the user for sure. Just wondering about collecting this info on our end so we have a better idea of the sample for those events but it would be a product decision and I doubt anyone would be interested 🤷

We could capture a sentry exception, but I worry we'd just flood sentry with useless messages. I have a feeling lots of browsers with privacy features enabled will block Adobe Analytics

^^ Yes, that was the problem before, though I think the error handling was at a slightly different spot in the code at the time. We don't want to flood Sentry, and before we were creating a new Sentry alert for every user and every event they clicked where analytics were disabled. Where the catch is now, I think we'd just be triggering sentry alerts for every user blocking extensions every time they load the app (rather than every time they fire a trackable event), but that's still a lot.

packages/manager/src/hooks/useAdobeAnalytics.ts

abailly-akamai · 2023-11-14T14:20:06Z

packages/manager/src/hooks/useEventHandlers.ts

+    filter: ({ event }) => event.action.startsWith('disk') && !event._initial,
+    handler: diskEventHandler,
+  },
+];


This does not seem to be a 1/1 with the legacy code. Can you explain what happened to the linode_migrate events? They were previously a dependency for the hook below.

handleMigrationEvent was duplicated logic that also was in ToastNotifications. It now lives in useToastNotifications where it belongs

abailly-akamai · 2023-11-14T14:32:31Z

packages/manager/src/hooks/useToastNotifications.tsx

+export const getLabel = (event: Event) => event.entity?.label ?? '';
+export const getSecondaryLabel = (event: Event) =>
+  event.secondary_entity?.label ?? '';
+const formatLink = (text: string, link: string, handleClick?: any) => {


can we type this better? also, we are not testing any hooks in the codebase, so in general return types would be nice to have in those new hooks.

Oops, didn't see the any when I copied this code over!

I made the hooks explicitly return null. I don't see too much value in adding an explicit return type to the function signatures. Do you think it makes sense to be explicit here?

Ugh was commenting with my personal GH account. Yeah, agreed - not much value. I'll give it more thoughts!

tyler-akamai · 2023-11-14T17:12:02Z

Performed general check of Cloud Manager
Verified global toast notifications still work (creating or deleting a volume for example)
Verified that both Adobe Analytics and New Relic initialize
Verified keyboard shortcuts like Control + Shift + D still work
Verify e2es pass

abailly-akamai

Second pass: everything seems stable when testing in the browser. Tested a bunch of actions and notifications came through as expected. No regression spotted ✅

tyler-akamai · 2023-11-14T21:18:59Z

packages/manager/src/hooks/useToastNotifications.tsx

+ */
+const toasts: Toasts = {
+  backups_restore: {
+    failure: (e) => `Backup restoration failed for ${getLabel(e)}.`,


Very small nitpick: should we end notifications with a period. For the toast notifications ive added, they have just been a statement without a period, but I can update them.

I'm gonna leave the periods but put an item on a cafe agenda. It seems pretty inconstant throughout the app when I search for instances of enqueueSnackbar

bnussman added 4 commits November 13, 2023 10:52

break up logic into new files/hooks

56bcd85

clean up toast notification handler

72ace1b

update comments

7216571

add more comments

7670fcd

bnussman-akamai self-assigned this Nov 13, 2023

bnussman-akamai requested a review from a team as a code owner November 13, 2023 18:24

bnussman-akamai requested review from jaalah-akamai and tyler-akamai and removed request for a team November 13, 2023 18:24

bnussman-akamai commented Nov 13, 2023

View reviewed changes

bnussman added 3 commits November 13, 2023 13:32

Added changeset: Clean up App.tsx

7aa2708

Merge branch 'develop' into clean-up-app-entry-points

9e6d6c1

make logic correct to what it says

f538184

abailly-akamai reviewed Nov 14, 2023

View reviewed changes

bnussman added 2 commits November 14, 2023 10:13

feedback @abailly-akamai

c167b1e

clean up

ff78ecc

linode deleted a comment from alioso Nov 14, 2023

abailly-akamai approved these changes Nov 14, 2023

View reviewed changes

tyler-akamai reviewed Nov 14, 2023

View reviewed changes

tyler-akamai approved these changes Nov 14, 2023

View reviewed changes

bnussman-akamai merged commit 7a62480 into linode:develop Nov 14, 2023

mjac0bs mentioned this pull request Dec 11, 2023

Release v1.108.0 - staging → master #9986

Merged

Conversation

bnussman-akamai commented Nov 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description 📝

Changes 🔄

How to test 🧪

Verification steps

As an Author I have considered 🤔

Uh oh!

bnussman-akamai Nov 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abailly-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjac0bs Nov 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tyler-akamai commented Nov 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abailly-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bnussman-akamai commented Nov 13, 2023 •

edited

Loading

bnussman-akamai Nov 13, 2023 •

edited

Loading

mjac0bs Nov 14, 2023 •

edited

Loading

tyler-akamai commented Nov 14, 2023 •

edited

Loading