Repository navigation
CNS-120 In Console, cluster list now pulls data from SUBSCRIBE - #38014
Conversation
…ow via Jotai store. This is a prerequisite for adding replica usage matrics to the list display.
leedqin
left a comment
leedqin
left a comment
There was a problem hiding this comment.
Thanks for the migration! looks pretty good to me; just had some suggestions. Would you mind updating the verification section in the PR description and also adding a Fixes- in the Motivation section in the PR description. This would directly sync this PR to the Linear issue
| ...c, | ||
| // Treat an in-flight owners query as non-owner so owner-only menu items | ||
| // stay hidden until ownership is known. | ||
| isOwner: !isOwnersPending && (ownersById?.get(c.ownerId) ?? false), |
There was a problem hiding this comment.
do you think we could move this check into useOwners hook and share this check across ClustersList.tsx, ClusterDetail and ClusterReplica?
There was a problem hiding this comment.
Addressed in commit 16412a
| if (!clusters) return []; | ||
| const systemClusters = clusters.filter((c) => isSystemCluster(c.id)); | ||
| const nonSystemClusters = clusters | ||
| const visibleClusters = clusters |
There was a problem hiding this comment.
Same filter + sort already exists in ClusterDetailBreadcrumbs. Maybe pull it into a helper in store/allClusters.ts, e.g. visibleClusters(clusters, showSystemObjects)
export function visibleClusters(clusters: Cluster[], showSystemObjects: boolean) {
return clusters
.filter((c) => showSystemObjects || !isSystemCluster(c.id))
.sort((a, b) => a.name.localeCompare(b.name));
}
There was a problem hiding this comment.
There are a couple of minor differences between the two screens. On the cluster list page, in the default sort state, we push system clusters to the bottom of the list. In the menu on the detail page, we sort by name without separating the 2 cluster types.
Also, on the List page, we filter out clusters that aren't owned by the user. We don't filter on the Detail page.
I suggest we update the detail page menu to include the filtering and to separate the 2 types of cluster, but that's just my uninformed opinion. For that matter, maybe we should move the Unowned filter higher up in the chain so that we use it in the query or when writing to the store?
There was a problem hiding this comment.
A couple of clarifications:
The list doesn't actually filter unowned clusters out. getOwners() returns every role with an isOwner boolean, so no rows are dropped, isOwner only gates the Alter/Delete items in the row's overflow menu. So there's no unowned filter to move up the chain.
Moving ownership into the subscribe also isn't possible: has_role() and mz_is_superuser() can't run in a dataflow, which is why ownership is a separate polled query joined client-side. The allClusters atom is also shared by route validation, this breadcrumb menu, and the Shell dropdown, which all need every cluster regardless of ownership.
On sorting, the difference is smaller than it looks: the table applies initialSorting: name asc, so in the default state rows are re-sorted by name and the list's system-last grouping only shows once a user cycles sorting off. Given that, I'd keep the helper to the shared core (system-visibility filter + name sort) and let the list layer its grouping on top. Grouping system clusters last in this menu too seems fine if you want it, no strong opinion from me either.
There was a problem hiding this comment.
Ahhh gotcha OK. I see. I think I was conflating owned and system clusters. And yep, I realized that filtering higher in the chain didn't make sense after I re-read my comment. Thanks!
| expect(await screen.findByText(CLUSTERS_FETCH_ERROR_MESSAGE)).toBeVisible(); | ||
| }); | ||
|
|
||
| it("renders the cluster list", async () => { |
There was a problem hiding this comment.
The system-cluster filtering is client-side logic now, so it'd be good to cover it: seed the atom with an s-id cluster plus user clusters and assert it's hidden by default / shown with the toggle.
…order to eliminate redundant code. Added unit tests.
leedqin
left a comment
leedqin
left a comment
There was a problem hiding this comment.
LGTM! I think the tests look solid. Thank you for this change
### Motivation Closes: [CNS-192](https://linear.app/materializeinc/issue/CNS-192) On a cold load the cluster list shows its loading spinner and renders the rows once the clusters SUBSCRIBE snapshot arrives. It then swaps them for a second spinner before the table finally shows. Both the classic table and the per-replica preview table are affected. Each row's overflow menu renders its items while the menu is closed. For clusters the user owns that are managed and not system clusters, that includes `AlterClusterMenuItem`, which read the cluster sizes through `useAvailableClusterSizes`, a `useSuspenseQuery`. On a cold cache the first render of the rows suspends, and React hides them behind the `ClustersListPage` Suspense fallback until the sizes return. Before #38014 the list itself loaded through a suspending query, so both waits sat behind one fallback. Since the list moved to the subscribe-fed atom with its own loading state, they show up as two separate spinners. ### Description - `useAvailableClusterSizesWithoutSuspense` reads the same query, with the same key and fetcher, without suspending. `AlterClusterMenuItem` uses it. - "Alter cluster" is disabled until the sizes load. Without that, a click during loading closes the menu with no feedback, and the modal opens later on its own once the sizes arrive. - `useAvailableClusterSizes` keeps suspending for `NewClusterForm` and `NewReplicaModal`. Switching `NewClusterForm` would break its default size: the size `<select>` is uncontrolled with a `defaultValue`, so options that arrive after mount leave the first size selected instead of the default. - Behavior change on error: a failed sizes query used to throw to the page's error boundary. It now leaves "Alter cluster" disabled and the rest of the page usable. - The cluster detail header mounts the same menu item, so it no longer suspends there either. Not in this PR: - `NewReplicaModal` is mounted while closed on the cluster Replicas tab and still suspends on the sizes. That serializes the sizes request after the replicas query. - `OverflowMenu` mounts every item's hooks while the menu is closed. Making it lazy would remove the per-row fetches entirely, but it changes every overflow menu in the console. ### Verification New tests in `ClustersList.test.tsx` hold the sizes request open and check that the per-replica rows and the classic table's rows still render, and that "Alter cluster" is disabled until the sizes load and enabled after. The loading tests fail against the previous `AlterClusterMenuItem`. I also reproduced the bug with a temporary harness, not committed, that renders the real route tree and subscribe stack against a mocked SUBSCRIBE WebSocket. Before: spinner, snapshot, second spinner with the table hidden, table. After: spinner, snapshot, table. Not yet checked in a browser against production. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
In Console, cluster list data is now sourced from SUBSCRIBE via Jotai store. This moves one more consumer away from the deprecated polling model and it's a prerequisite for adding replica usage metrics to the list display.
Motivation
Fixes CNS-120
Verification