chore: incoming webhook events UI #6317

nunogois · 2024-02-22T14:32:45Z

https://linear.app/unleash/issue/2-1937/incoming-webhook-events-ui

This PR implements the UI for incoming webhook events.

We're also introducing a new SidePanelList component that we'll be able to reuse when we tackle action set events. This PR also promotes ReactJSONEditor to a common component and adapts it slightly for this use case.

We're refreshing the events view every 5s, so if you're monitoring events for a specific incoming webhook you can see the latest ones coming in.
We load 20 (configurable through the hook) events by default. Everytime you reach the end of the list you can load 20 more events until you reach the end of the event list.

vercel · 2024-02-22T14:32:52Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
unleash-monorepo-frontend	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Feb 23, 2024 10:23am

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
unleash-docs	⬜️ Ignored (Inspect)	Visit Preview		Feb 23, 2024 10:23am

codescene-delta-analysis

❌ Code Health Quality Gates: FAILED

Declining Code Health: 2 findings(s) 🚩
Improving Code Health: 0 findings(s) ✅
Affected Hotspots: 0 files(s) 🔥

Recommended Review Level: Detailed -- Inspect the code that degrades in code health.
View detailed results in CodeScene

🚩 Declining Code Health (highest to lowest):

Large Method IncomingWebhooksTable.tsx: IncomingWebhooksTable
Complex Conditional useIncomingWebhookEvents.ts: getKey:SWRInfiniteKeyLoader

codescene-delta-analysis · 2024-02-22T14:33:43Z

frontend/src/component/incomingWebhooks/IncomingWebhooksTable/IncomingWebhooksTable.tsx

+    const [eventsModalOpen, setEventsModalOpen] = useState(false);
+


❌ Getting worse: Large Method
IncomingWebhooksTable increases from 221 to 239 lines of code, threshold = 120

Why does this problem occur?

Large functions with many lines of code are generally harder to understand and lower the code health. Avoid adding more lines to this function. Read more.

_{To ignore this warning click here.}

codescene-delta-analysis · 2024-02-22T14:33:44Z

frontend/src/hooks/api/getters/useIncomingWebhookEvents/useIncomingWebhookEvents.ts

+        previousPageData: IncomingWebhookEventsResponse,
+    ) => {
+        // Does not meet conditions
+        if (!incomingWebhookId || !isEnterprise || !incomingWebhooksEnabled)


❌ New issue: Complex Conditional
getKey:SWRInfiniteKeyLoader has 1 complex conditionals with 2 branches, threshold = 2

Why does this problem occur?

A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell. Read more.

_{To ignore this warning click here.}

codescene-delta-analysis

❌ Code Health Quality Gates: FAILED

Declining Code Health: 2 findings(s) 🚩
Improving Code Health: 0 findings(s) ✅
Affected Hotspots: 0 files(s) 🔥

Recommended Review Level: Detailed -- Inspect the code that degrades in code health.
View detailed results in CodeScene

🚩 Declining Code Health (highest to lowest):

Large Method IncomingWebhooksTable.tsx: IncomingWebhooksTable
Complex Conditional useIncomingWebhookEvents.ts: getKey:SWRInfiniteKeyLoader

daveleek · 2024-02-23T08:52:26Z

frontend/src/component/common/ReactJSONEditor/ReactJSONEditor.tsx

+    ...(editorStyle === 'sidePanel' && {
+        '&&&': {
+            '& .jse-main': {
+                minHeight: 0,
+            },
+            '--jse-main-border': 0,
+            '& > div': {
+                height: '100%',
+            },
+            '& .jse-focus': {
+                '--jse-main-border': 0,
+            },
+            '& .cm-gutters': {
+                '--jse-panel-background': 'transparent',
+                '--jse-panel-border': 'transparent',
+            },
+            '& .cm-gutter-lint': {
+                width: 0,
+            },
+            '& .jse-text-mode': {
+                borderBottomRightRadius: theme.shape.borderRadiusMedium,
+            },
+            '& .cm-scroller': {
+                '--jse-delimiter-color': theme.palette.text.primary,
+            },
+        },
+    }),


This felt a tad noisy, but I guess this is where it belongs

Yep, can't think of a better way of doing this right now, as I want this specific style for this component when used on the new side panel component.

daveleek · 2024-02-23T09:08:28Z

frontend/src/component/incomingWebhooks/IncomingWebhooksEvents/IncomingWebhooksEventsModal.tsx

+const LazyReactJSONEditor = lazy(
+    () => import('component/common/ReactJSONEditor/ReactJSONEditor'),
+);


Could the component be exported this way? so it's always lazily imported? Or is that an implementation detail more than what we should do every time?

If I understand correctly you're asking whether we can export the lazy import, so that it is always lazy imported. I think it should always be up to the parent to decide how to import the component, but we can discuss this in the team.

Yes exactly. I saw it being lazily imported twice in the same PR, so figured it was worth asking if this should be the standard way of doing this

daveleek · 2024-02-23T09:12:42Z

frontend/src/component/incomingWebhooks/IncomingWebhooksModal/IncomingWebhooksModal.tsx

@@ -137,12 +151,16 @@ export const IncomingWebhooksModal = ({
            <FormTemplate
                loading={loading}
                modal
-                title={title}
+                title=''


Could we make this property/parameter optional? Or did you want to show that it's intentionally omitted?

Seems like it's already optional so we can just omit it. Nice catch! f35add8

daveleek

Couple of questions around things like potentially undefined parameters meaning we return null etc. But apart from that LGTM!

daveleek · 2024-02-23T09:39:51Z

frontend/src/component/incomingWebhooks/IncomingWebhooksEvents/IncomingWebhooksEventsModal.tsx

+            refreshInterval: 5000,
+        });
+
+    if (!incomingWebhook) {


under which circumstances would this occur?

incomingWebhook is optional on the props, so this saves us from adding the optional chaining operator (?.) everywhere we access incomingWebhook down the line. If there's no incomingWebhook, we simply don't render the component.

However to address your question, if we look at:
https://github.com/Unleash/unleash/pull/6317/files#diff-825b19367b6b8fc7939b8bc023222583d4778b4afef02559b16c7523a3fc68a8R263-R270
We'll see that we're passing down the selectedIncomingWebhook to this prop. Which means it is going to be undefined in case no incoming webhook has been selected, which is why the prop is optional.

Cool, thanks for the explanation!

What would the drawbacks of conditionally rendering the Modal be?
Edit: asking to be enlightened, not a blocking question!

That could also work, but if we're using ConditionallyRender to accomplish that, the component should be returned in a callback, so something like this:

<ConditionallyRender condition={Boolean(selectedIncomingWebhook)} show={() => ( <IncomingWebhooksModal incomingWebhook={selectedIncomingWebhook!} open={modalOpen} setOpen={setModalOpen} newToken={(token: string) => { setNewToken(token); setTokenDialog(true); }} onOpenEvents={() => { setModalOpen(false); setEventsModalOpen(true); }} /> )} />

TypeScript also doesn't know that show only happens when the condition is true, so you also need to add the ! in selectedIncomingWebhook!. But if you do this, I believe it's safe to assume that the incomingWebhook prop is no longer optional since it's always going to be present.

While this could be a valid option and sometimes easier, the main drawbacks for me at this stage are noisy code at the parent level and less consistency with the rest of our code base.

Effectively, what we have now, checking for if (!incomingWebhook) return null inside the component, is the same as a conditional render, but shifting the responsibility of handling this gracefully to the component itself and not the parent.

daveleek · 2024-02-23T09:40:04Z

frontend/src/hooks/api/getters/useIncomingWebhookEvents/useIncomingWebhookEvents.ts

+        previousPageData: IncomingWebhookEventsResponse,
+    ) => {
+        // Does not meet conditions
+        if (!incomingWebhookId || !isEnterprise || !incomingWebhooksEnabled)


!incomingWebhookId
Is this a UI that can be opened without having an existing inc.webhook?

I think this is related to: #6317 (comment)

The behavior we want for this hook is that you can call it whenever, but if you're not passing an incomingWebhookId, it should not fetch data.

Since you cannot conditionally call hooks in React, this matches the previous comment well, because you can just call this hook every time and it will gracefully handle undefined cases.

codescene-delta-analysis

❌ Code Health Quality Gates: FAILED

Declining Code Health: 2 findings(s) 🚩
Improving Code Health: 0 findings(s) ✅
Affected Hotspots: 0 files(s) 🔥

Recommended Review Level: Detailed -- Inspect the code that degrades in code health.
View detailed results in CodeScene

🚩 Declining Code Health (highest to lowest):

Large Method IncomingWebhooksTable.tsx: IncomingWebhooksTable
Complex Conditional useIncomingWebhookEvents.ts: getKey:SWRInfiniteKeyLoader

nunogois requested review from chriswk, gastonfournier, daveleek and nicolaesocaciu February 22, 2024 14:32

github-actions bot assigned nunogois Feb 22, 2024

vercel bot deployed to Preview – unleash-monorepo-frontend February 22, 2024 14:33 View deployment

codescene-delta-analysis bot reviewed Feb 22, 2024

View reviewed changes

nunogois added 5 commits February 22, 2024 14:35

chore: incoming webhook events UI

12d196b

ongoing implementation of incoming webhook events UI

ae141a4

chore: ongoing development of SidePanelList

34afdef

chore: finish first implementation of SidePanelList

3337976

chore: implement load more

e09f279

nunogois force-pushed the chore-incoming-webhook-events-ui branch from f4093ca to e09f279 Compare February 22, 2024 14:35

codescene-delta-analysis bot reviewed Feb 22, 2024

View reviewed changes

vercel bot deployed to Preview – unleash-monorepo-frontend February 22, 2024 14:39 View deployment

daveleek reviewed Feb 23, 2024

View reviewed changes

daveleek approved these changes Feb 23, 2024

View reviewed changes

fix: remove empty title prop

f35add8

codescene-delta-analysis bot reviewed Feb 23, 2024

View reviewed changes

vercel bot deployed to Preview – unleash-monorepo-frontend February 23, 2024 10:23 View deployment

nunogois merged commit 12ff4ab into main Feb 23, 2024
12 of 13 checks passed

nunogois deleted the chore-incoming-webhook-events-ui branch February 23, 2024 11:01

nunogois mentioned this pull request Feb 27, 2024

chore: action events UI #6358

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: incoming webhook events UI #6317

chore: incoming webhook events UI #6317

nunogois commented Feb 22, 2024 •

edited

Loading

vercel bot commented Feb 22, 2024 •

edited

Loading

codescene-delta-analysis bot left a comment

codescene-delta-analysis bot Feb 22, 2024

codescene-delta-analysis bot Feb 22, 2024

codescene-delta-analysis bot left a comment

daveleek Feb 23, 2024

nunogois Feb 23, 2024

daveleek Feb 23, 2024

nunogois Feb 23, 2024

daveleek Feb 23, 2024

daveleek Feb 23, 2024

nunogois Feb 23, 2024

daveleek left a comment

daveleek Feb 23, 2024

nunogois Feb 23, 2024

daveleek Feb 23, 2024

daveleek Feb 23, 2024 •

edited

Loading

nunogois Feb 23, 2024 •

edited

Loading

daveleek Feb 23, 2024

nunogois Feb 23, 2024

codescene-delta-analysis bot left a comment

		const [eventsModalOpen, setEventsModalOpen] = useState(false);

chore: incoming webhook events UI #6317

chore: incoming webhook events UI #6317

Conversation

nunogois commented Feb 22, 2024 • edited Loading

vercel bot commented Feb 22, 2024 • edited Loading

codescene-delta-analysis bot left a comment

Choose a reason for hiding this comment

🚩 Declining Code Health (highest to lowest):

codescene-delta-analysis bot Feb 22, 2024

Choose a reason for hiding this comment

Why does this problem occur?

codescene-delta-analysis bot Feb 22, 2024

Choose a reason for hiding this comment

Why does this problem occur?

codescene-delta-analysis bot left a comment

Choose a reason for hiding this comment

🚩 Declining Code Health (highest to lowest):

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

daveleek 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

daveleek Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

nunogois Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codescene-delta-analysis bot left a comment

Choose a reason for hiding this comment

🚩 Declining Code Health (highest to lowest):

nunogois commented Feb 22, 2024 •

edited

Loading

vercel bot commented Feb 22, 2024 •

edited

Loading

daveleek Feb 23, 2024 •

edited

Loading

nunogois Feb 23, 2024 •

edited

Loading