feat: richer empty states #239

alex-mcgovern · 2025-01-31T12:51:50Z

build's on @kantord's work in feat: implement empty state for tables #232 and expands the range of empty states to handle
- search queries
- filters applied
- loading state
- error state
makes use of ts-pattern to do the pattern matching across a range of scenarios

Screenshots

This is not an exhaustive list

…ts/tabs-for-table

…er-empty-states

stacklok-cloud-staging · 2025-01-31T12:51:53Z

Minder Vulnerability Report ✅

Minder analyzed this PR and found it does not add any new vulnerable dependencies.

Vulnerability scan of f50729e5:

🐞 vulnerable packages: 0

🛠 fixes available for: 0

stacklok-cloud-staging

Dependency Information

Minder analyzed the dependencies introduced in this pull request and detected that some dependencies do not meet your security profile.

📦 Dependency: ts-pattern

Trusty Score: 0

Scoring details

Component	Score
Package activity	7.3
Repository activity	5.7
User activity	8.9
Provenance	historical_provenance_match

Proof of Origin (Provenance)

This package can be linked back to its source code using a historical provenance map.

We were able to correlate a significant number of git tags and tagged releases in this package’s source code to versions of the published package. This mapping creates a strong link from the package back to its source code repository, verifying proof of origin.


Published package versions	155
Number of git tags or releases	63
Versions matched to tags or releases	55

coveralls · 2025-01-31T12:54:38Z

Pull Request Test Coverage Report for Build 13109301751

Details

23 of 25 (92.0%) changed or added relevant lines in 7 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.4%) to 74.167%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/features/alerts/components/table-alerts-empty-state.tsx	11	12	91.67%
src/features/alerts/components/table-alerts.tsx	4	5	80.0%

Totals
Change from base Build 13109294004:	0.4%
Covered Lines:	800
Relevant Lines:	1013

💛 - Coveralls

alex-mcgovern · 2025-01-31T12:55:34Z

src/features/alerts/components/__tests__/table-alerts.empty-state.test.tsx

+    if (testCase.expected.actions) {
+      for (const action of testCase.expected.actions) {
+        const actionButton = getByRole(action.role, { name: action.name });
+        expect(actionButton).toBeVisible();
+        if (action.href) {
+          expect(actionButton).toHaveAttribute("href", action.href);
+        }
+      }
+    }


Breaking the rules of testing here a bit, but this module would be a lot longer otherwise.

if you are referring to the if statement I think it's fine, but it looks like the if statement is actually kinda redundant, no?

I mean iterating over 0 items without condition has the same effect as not iterating over 0 items based on a condition 🤔

for (const action of testCase.expected.actions ?? []) { const actionButton = getByRole(action.role, { name: action.name }); expect(actionButton).toBeVisible(); if (action.href) { expect(actionButton).toHaveAttribute("href", action.href); } }

on the other hand, I think there is a slightly different way to organize the code that does not even require the for loop 🤔

you could make a function like this:

function expectAction({role = "button", name, href}) { const actionElement = screen.getByRole(role, { name }) expect(actionElement).toBeVisible() expect(actionElement).toHaveAttribute("href", href) }

Then you could pass in the action assertions as an arrow function:

expected: { title: emptyStateStrings.title.noAlertsFoundWorkspace, body: emptyStateStrings.body.alertsWillShowUpWhenWorkspace, illustrationTestId: IllustrationTestId.DONE, expectActions: () => { expectAction({role: "link", name: /manage workspaces/i, href: etc}) } },

or you can even be fancy and make a custom assertion that can be used like this

expected: { title: emptyStateStrings.title.noAlertsFoundWorkspace, body: emptyStateStrings.body.alertsWillShowUpWhenWorkspace, illustrationTestId: IllustrationTestId.DONE, expectActions: () => { expect.hrefElement("link", { name: /manage workspaces/i, href: etc}) } },

kantord · 2025-01-31T14:31:04Z

src/features/alerts/constants/strings.ts

@@ -0,0 +1,25 @@
+export const emptyStateStrings = {


do we need to extract strings like this? cc. @peppescg

since it's not used for translations I don't see the added value. I think in 8 out of 10 cases putting a static text in the component directly results in more navigable code, since you don't need 2 searches to find where the text is used 🤔

also having the text directly in the component helps the readability of the code, makes it more intuitive to connect what you see in the browser with what you see in the editor

I find it useful for testing, you can just reference the same variable and it's requires less changes if you update the copy.

Also — not relevant to this implementation — but to me it is good for making the structure of jsx/html easier to parse. Although here I'm just returning it from within a function.

I actually usually use a module with top level exports, which you can scan with dead code analysis tools too.

this is interesting. I see the usefulness of that, albeit I prefer that user-facing changes actually require changes to test code, as I try to see the test code as a "representative" of the user in our codebase.

I sometimes use the approach of only matching the relevant keywords when repeating long text gets annoying:

expect(screen.getByText(/.*no.*alerts.*/i).toBeVisible()

That is fine while working with the assumptions that tests are a simulation of user behavior, because I think the user's eyes are also automatically looking for the important keywords rather than carefully reading the text letter-by-letter.

It might be still slightly more annoying that your approach - but this could be a feature, not a bug, perhaps it helps us understand that whenever we get this annoyance, an experienced user might get the same annoyance finding that the behavior or wording changed 🤔

I dunno, I think wildcard regex in tests will likely be a foot gun eventually due to fiddly nature of greedy vs lazy matching.

I dunno, I think wildcard regex in tests will likely be a foot gun eventually due to fiddly nature of greedy vs lazy matching.

it can be, but mostly if the keywords are not actually good keywords, or if the same keywords end up being used a lot more in the same page. But this would actually be a signal that they keywords no longer cognitively help the user as much as well 🤔 which might be a sign that the route/page/view is becoming overly complex

I agree with @kantord, also if we want to do this we should do it everywhere, not only for a single component. IMO the application is too small so I guess for now thinking a strategy for text is early.

Cause we are in fast development mode, we can merge it, avoiding wasting time today...but I would avoid to use this approach until we don't need to think a text/translation issue

kantord · 2025-01-31T14:37:42Z

src/features/alerts/hooks/use-alerts-empty-state.tsx

+        search: P._,
+        view: P._,
+      },
+      () => ({


we can use pattern matching here, but:

I would still leverage early-return pattern as much as possible in order to simplify the pattern matching instead of merging everything into a single pattern. For instance if I understand correctly, when isLoading = true, then none of the other values in the pattern have any relevance.

This function is very long, and has some heavy parts that are not comprehensible at first glance. I would improve that by breaking it down into multiple functions/hooks, which would allow you to replace complex blocks of code with names.

Like so:

.with( { hasWorkspaceAlerts: true, hasMultipleWorkspaces: P.any, search: P.string.select(), view: P._, }, () => useAlertsEmptyStateNoSearchResults() )

also, I'm not sure I understand why this is a hook instead of a component 🤔 I would actually expect sth like this

.with( { hasWorkspaceAlerts: true, hasMultipleWorkspaces: P.any, search: P.string.select(), view: P._, }, <AlertsEmptyStateNoSearchResults /> ) /* ...etc... */ function AlertsEmptyStateNoSearchResults() { return (<GenericTableEmptyState title="No matching alerts found" illustration={<FooBarBaz />} actions={<Button onPress={etc}>Clear search</Button> />) }

It's a hook so it can call other hooks, which it does to check the length of alerts/workspaces, and to get the query params used for filtering.

Re: early return — the first arm of the match statement is effectively an early return on isMatching: true — doesn't this achieve the same thing?

I do see some merit in using a component rather than a struct for the content

I'm not sure I understand why this is a hook instead of a component 🤔

I though more about your comment — I initially wrote this as a component, but decided to separate the logic into this hook, as was quite verbose.

I guess I am not extremely against using the hook for the logic of choosing between different values.

I hold this part of the opinion stronger: https://github.com/stacklok/codegate-ui/pull/239/files#r1937429121

about not directly binding the very specific logic with the very generic one.

kantord · 2025-01-31T14:45:48Z

src/features/alerts/components/table-alerts-empty-state.tsx

+  );
+}
+
+export function TableAlertsEmptyState({ isLoading }: { isLoading: boolean }) {


look at this suggestion: https://github.com/stacklok/codegate-ui/pull/239/files#r1937422209

I think that this component could be a completely generic one, and probably should in order to encourage the reuse of the same pattern 🤔

It feels weird to bind this very generic representation logic (with all actual content being parametric) with a very specific logic in the useAlertsEmptyState hook. Even if you prefer the hook method, I would not bind it to this component, it could be called in a more alerts table-specific context

kantord · 2025-01-31T15:01:59Z

src/features/alerts/components/__tests__/table-alerts.empty-state.test.tsx

+import { TableAlerts } from "../table-alerts";
+import { hrefs } from "@/lib/hrefs";
+
+enum IllustrationTestId {


I am on the fence about whether we even need to test a purely visual illustration that has little functional purpose. If it had an actual functional purpose, it would not need test IDs but aria-labels/alt texts, no? 🤔

I just have this feeling that these illustrations don't actually illustrate anything and are only eye candy, so it would only need to be tested using visual regression tests

You say that but I had the wrong illustration in one of the match arms :)

hahaaha, I don't doubt that, I mean, if we had a good, easy-to-use, reliable, fast and cheap visual regression testing method, then I would recommend that we write a visual regression test for each case

kantord · 2025-01-31T15:04:52Z

src/features/alerts/components/table-alerts.tsx

-    </div>
-  );
+    default:
+      return column.id satisfies never;


why not

Suggested change

return column.id satisfies never;

throw new Error('tried to render non existent column')

or use ts-pattern with .exhaustive() to get type safety guarantees that it does not happen

kantord · 2025-01-31T15:10:43Z

src/features/alerts/components/table-alerts.tsx

-      </div>
-    );
-  }
+  switch (column.id) {


I am a bit confused about this refactor, seems like it adds a lot of logic but I don't really get the benefit? 🤔 since in the end it just reimplements the hardcoded code in a more complicated way? 🤔

In a simplified way, this is what my eyes see here:

Original code

<X> <Foo /> <Bar /> </X>

Refactored code:

<X columns=[Foo, Bar]> (item) => child(item) </X> /* ... etc ... */ function child(item) { if (item == Foo) { return <Foo /> } if (item == Bar) { return <Bar /> } }

So I just see some added logic that does not change behavior 🤔 what am I missing?

in any case, if we really need this function and switch-case logic, why not use ts-pattern here as well?

I actually re-wrote it while chasing a bug, that lay elsewhere. I will separate it into it's own PR.

I'm going to leave this in — mainly because James requested some styling tweaks to the cells in the table component, which are done, and they're easiest applied when using collections to render the cells.

stacklok-cloud-staging

Dependency Information

Minder analyzed the dependencies introduced in this pull request and detected that some dependencies do not meet your security profile.

📦 Dependency: ts-pattern

Trusty Score: 0

Scoring details

Component	Score
Package activity	7.3
Repository activity	5.7
User activity	8.9
Provenance	historical_provenance_match

Proof of Origin (Provenance)

This package can be linked back to its source code using a historical provenance map.

We were able to correlate a significant number of git tags and tagged releases in this package’s source code to versions of the published package. This mapping creates a strong link from the package back to its source code repository, verifying proof of origin.


Published package versions	155
Number of git tags or releases	63
Versions matched to tags or releases	55

…er-empty-states

alex-mcgovern added 7 commits January 30, 2025 15:19

feat(alerts): tabs for filtering table

82bb986

Merge branch 'main' of github.com:stacklok/codegate-ui into feat/aler…

d819675

…ts/tabs-for-table

Merge branch 'main' of github.com:stacklok/codegate-ui into feat/aler…

fe22a4e

…ts/tabs-for-table

Merge branch 'main' of github.com:stacklok/codegate-ui into feat/aler…

5d8a6e4

…ts/tabs-for-table

fix: header spacing bug

f28b9ff

feat: richer empty states

896bec0

Merge branch 'main' of github.com:stacklok/codegate-ui into feat/rich…

ac8ff7d

…er-empty-states

stacklok-cloud-staging bot reviewed Jan 31, 2025

View reviewed changes

alex-mcgovern commented Jan 31, 2025

View reviewed changes

kantord reviewed Jan 31, 2025

View reviewed changes

refactor: tidy up pattern matching

62715ba

stacklok-cloud-staging bot reviewed Jan 31, 2025

View reviewed changes

alex-mcgovern added 3 commits February 3, 2025 08:23

chore: tidy ups

0f391b1

fix: tests

f50729e

Merge branch 'main' of github.com:stacklok/codegate-ui into feat/rich…

01a0ab0

…er-empty-states

alex-mcgovern marked this pull request as ready for review February 3, 2025 08:30

peppescg approved these changes Feb 3, 2025

View reviewed changes

alex-mcgovern merged commit c8990c2 into main Feb 3, 2025
8 checks passed

alex-mcgovern deleted the feat/richer-empty-states branch February 3, 2025 09:01

stacklokbot mentioned this pull request Feb 3, 2025

chore(main): release 0.11.0 #235

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: richer empty states #239

feat: richer empty states #239

alex-mcgovern commented Jan 31, 2025

stacklok-cloud-staging bot commented Jan 31, 2025 •

edited

Loading

Vulnerability scan of `f50729e5:`

stacklok-cloud-staging bot left a comment

coveralls commented Jan 31, 2025 •

edited

Loading

alex-mcgovern Jan 31, 2025

kantord Jan 31, 2025

kantord Jan 31, 2025

kantord Jan 31, 2025

kantord Jan 31, 2025

alex-mcgovern Jan 31, 2025

kantord Jan 31, 2025 •

edited

Loading

alex-mcgovern Jan 31, 2025

kantord Jan 31, 2025

peppescg Feb 3, 2025

kantord Jan 31, 2025

kantord Jan 31, 2025

alex-mcgovern Jan 31, 2025

alex-mcgovern Jan 31, 2025

alex-mcgovern Jan 31, 2025

kantord Jan 31, 2025

kantord Jan 31, 2025

kantord Jan 31, 2025

alex-mcgovern Jan 31, 2025

kantord Jan 31, 2025

kantord Jan 31, 2025

kantord Jan 31, 2025

kantord Jan 31, 2025

alex-mcgovern Jan 31, 2025

alex-mcgovern Feb 3, 2025

stacklok-cloud-staging bot left a comment

	return column.id satisfies never;
	throw new Error('tried to render non existent column')

feat: richer empty states #239

feat: richer empty states #239

Conversation

alex-mcgovern commented Jan 31, 2025

Screenshots

stacklok-cloud-staging bot commented Jan 31, 2025 • edited Loading

Minder Vulnerability Report ✅

Vulnerability scan of f50729e5:

stacklok-cloud-staging bot left a comment

Choose a reason for hiding this comment

Dependency Information

📦 Dependency: ts-pattern

Trusty Score: 0

coveralls commented Jan 31, 2025 • edited Loading

Pull Request Test Coverage Report for Build 13109301751

Details

💛 - Coveralls

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

kantord Jan 31, 2025 • edited Loading

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

stacklok-cloud-staging bot left a comment

Choose a reason for hiding this comment

Dependency Information

📦 Dependency: ts-pattern

Trusty Score: 0

stacklok-cloud-staging bot commented Jan 31, 2025 •

edited

Loading

Vulnerability scan of `f50729e5:`

coveralls commented Jan 31, 2025 •

edited

Loading

kantord Jan 31, 2025 •

edited

Loading