cluster-ui: create node regions selector #131046
Conversation
xinhaoz
requested review from
angles-n-daemons
and removed request for
a team
|
This change is |
xinhaoz
force-pushed
the
node-regions-comp
branch
from
af83f17
to
c20ef8a
Compare
xinhaoz
changed the title
|
TODO: Going to post a loom wtih a demo in a bit. |
xinhaoz
force-pushed
the
node-regions-comp
branch
4 times, most recently
from
70a6478
to
bb79273
Compare
| revalidateOnFocus: false, | ||
| }); | ||
|
|
||
| const nodeIDToRegion = useMemo(() => { |
There was a problem hiding this comment.
My react is a bit rusty, and I'm not too familiar with SWR, but why do we need to use useMemo here?
There was a problem hiding this comment.
It's so that we only recompute this value when the data changes. This hook will get called on every re-render, but since the data changes less ferquently than that we can useMemo can be used to only recompute items when necessary.
There was a problem hiding this comment.
For the SWR hook, you mostly just need to know that it caches based on a unique key, which in this case is just the nodes api route path, since we don't provide any params to it (see https://swr.vercel.app/#overview for more if you're curious).
So for now, because we're also not polling this nodes data yet (we can set that in the swr options param), these maps will only be computed once when the data loads, instead of every time the parent component re-renders (which is often).
| return result; | ||
| }, [data]); | ||
|
|
||
| const storeIDToNodeID = useMemo(() => { |
There was a problem hiding this comment.
Why do we need two separate useMemos here? Can the generation of both nodeIdToRegion and storeIdToNodeId be done in a single iteration of data.nodes?
There was a problem hiding this comment.
We could do that - I find it's sometimes a little cleaner to separate but we can consolidate here. Done.
Also, I think you meant to leave this comment and the one above on #130958. This PR should only focus on the latest commit.
| ); | ||
| }); | ||
|
|
||
| it("renders without crashing", () => { |
There was a problem hiding this comment.
nit: we probably don't need to say "without crashing". Maybe something as simple as it("should render")
xinhaoz
force-pushed
the
node-regions-comp
branch
from
bb79273
to
4044583
Compare
xinhaoz
force-pushed
the
node-regions-comp
branch
2 times, most recently
from
0227a1a
to
7d2aa9a
Compare
| @@ -41,7 +41,7 @@ export type DatabaseMetadataRequest = { | |||
| sortBy?: string; | |||
| sortOrder?: string; | |||
| pagination: SimplePaginationState; | |||
| storeId?: number; | |||
| storeId?: number[]; | |||
There was a problem hiding this comment.
nit should this be storeIds?
xinhaoz
force-pushed
the
node-regions-comp
branch
2 times, most recently
from
9b79952
to
3382ee8
Compare
Changelist by BitoThis pull request implements the following key changes.
|
This commit adds a select component which allows users to select nodes by region. Epic: CRDB-37558 Fixes: cockroachdb#131032 Release note: None
xinhaoz
force-pushed
the
node-regions-comp
branch
from
3382ee8
to
656085f
Compare
|
bors r+ |
131123: cluster-ui: add getTableMetadataApi and connect tables page r=xinhaoz a=xinhaoz Only the latest commit should be reviewed in this PR. Previous: #131046 ---------------------- This commit connects the v2 db details - tables page to the new `api/v2/table_metadata` api route. The following components are now connected to the api: - listing tables for a db - sorting by column - searching by table name - filtering by node id Epic: [CRDB-37558](https://cockroachlabs.atlassian.net/browse/CRDB-37558) Fixes: #131122 Release note: None Co-authored-by: Xin Hao Zhang <[email protected]>
Please review only the latest commit in this PR.
Previous: #130958
This commit adds a select component which allows users
to select nodes by region.
Epic: CRDB-37558
Fixes: #131032
Release note: None