Reviewed 53 of 64 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @matthewtodd)

pkg/ui/cluster-ui/src/databasePage/databasePage.tsx, line 1 at r1 (raw file):

// Copyright 2021 The Cockroach Authors.

nit: file name databaseDetailsPage

pkg/ui/cluster-ui/src/databasePage/databasePage.tsx, line 42 at r1 (raw file):

const sortableTableCx = classNames.bind(sortableTableStyles);

export interface DatabasePageDataTableDetails {

nit: add comments for all data interfaces

pkg/ui/cluster-ui/src/databasePage/databasePage.tsx, line 141 at r1 (raw file):

  }

  private changeView(view: ViewOption) {

nit: maybe a comment for now (or think of a clear name)

pkg/ui/cluster-ui/src/databasePage/databasePage.tsx, line 145 at r1 (raw file):

  }

  private columns(): ColumnDescriptor<DatabasePageDataTable>[] {

nit: separate between table and grant

pkg/ui/cluster-ui/src/databasePage/databasePage.tsx, line 274 at r1 (raw file):

  }

  private viewSelectionTitle(): string {

nit: inline this function

pkg/ui/cluster-ui/src/databasePage/databasePage.module.scss, line 1 at r1 (raw file):

// Copyright 2021 The Cockroach Authors.

nit: file name databaseDetailsPage

pkg/ui/cluster-ui/src/databasesPage/databasesPage.tsx, line 37 at r1 (raw file):

const sortableTableCx = classNames.bind(sortableTableStyles);

export interface DatabasesPageDataMissingTable {

nit: add comments for interfaces

pkg/ui/cluster-ui/src/databaseTablePage/databaseTablePage.tsx, line 16 at r1 (raw file):

import _ from "lodash";

import { AlignedIcon } from "src/icon/alignedIcon";

nit: group with icon import

pkg/ui/cluster-ui/src/databaseTablePage/databaseTablePage.tsx, line 35 at r1 (raw file):

}

export interface DatabaseTablePageDataDetails {

nit: add comments for interfaces

pkg/ui/cluster-ui/src/icon/alignedIcon.tsx, line 17 at r1 (raw file):

const cx = classNames.bind(styles);

export interface AlignedIconProps {

check titleWithIcon component

pkg/ui/cluster-ui/src/icon/icon.tsx, line 1 at r1 (raw file):

// Copyright 2021 The Cockroach Authors.

we might want to create a task for refactor current usage of icons, so they're all in this same place

pkg/ui/cluster-ui/src/storybook/fixtures/randomName.ts, line 1 at r1 (raw file):

// Copyright 2021 The Cockroach Authors.

check if these files are being added to the build

pkg/ui/src/util/fakeApi.ts, line 11 at r1 (raw file):

// licenses/APL.txt.

import * as $protobuf from "protobufjs";

nit: add comment about usage of this file (for testing)
add examples

pkg/ui/src/views/databases/databasePage/redux.spec.ts, line 196 at r1 (raw file):

    await driver.refreshDatabaseDetails();
    await driver.refreshTableDetails("foo");

create a test for checking two tables with different values

pkg/ui/src/views/databases/databasePage/redux.spec.ts, line 216 at r1 (raw file):

    fakeApi.stubTableDetails("things", "foo", {
      grants: [
        { user: "bloblaw", privileges: ["ALL"] },

nit: add another role to test the alphabetical order

pkg/ui/src/views/databases/databasesPage/redux.ts, line 27 at r1 (raw file):

const { DatabaseDetailsRequest, TableStatsRequest } = cockroach.server.serverpb;

export const mapStateToProps = createSelector(

consider extracting this outside of this redux to a place that can be used by crdb and cloud db (and serverless )

pkg/ui/src/views/databases/databasesPage/redux.ts, line 55 at r1 (raw file):

        // all of their individual stats.

        const possiblyMissingTables = stats

consider a different visual for the user when something is missing. Discuss with Anne possible solutions. We want the user to know that some data might be missing.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag)

pkg/ui/cluster-ui/src/icon/icon.tsx, line 1 at r1 (raw file):

Done, #68501

pkg/ui/cluster-ui/src/storybook/fixtures/randomName.ts, line 1 at r1 (raw file):

All clear!

$ grep randomName pkg/ui/cluster-ui/dist/main.js
$ grep randomRole pkg/ui/cluster-ui/dist/main.js
$ grep randomTablePrivilege pkg/ui/cluster-ui/dist/main.js
$

pkg/ui/src/views/databases/databasesPage/redux.ts, line 27 at r1 (raw file):

Good idea, I think it'll make sense to do this extraction as we're starting to support cloud / serverless.

pkg/ui/src/views/databases/databasesPage/redux.ts, line 55 at r1 (raw file):

Good idea, I've captured a note to follow up with Anne when she gets back. It is unclear to me how common this case is and how much we need to design for it; just that's it's possible. In the meantime, per our brief conversation in today's standup, we'll look into it as part of a follow-on task, not for this PR.

pkg/ui/cluster-ui/src/icon/alignedIcon.tsx, line 17 at r1 (raw file):

Done, merged with the Icon component!

pkg/ui/src/views/databases/databasePage/redux.spec.ts, line 196 at r1 (raw file):

Done.

Reviewed 10 of 64 files at r1, 12 of 19 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag and @matthewtodd)

pkg/ui/cluster-ui/src/databasesPage/databasesPage.tsx, line 83 at r2 (raw file):

// those properties for the table directly.
export interface DatabasesPageDataMissingTable {
  loading: boolean;

why there is no need for loaded field here?

pkg/ui/cluster-ui/src/databasesPage/databasesPage.tsx, line 134 at r2 (raw file):

    }

    _.forEach(this.props.databases, database => {

Do we have a preference on when to use lodash vs the builtin functions of js array ?

pkg/ui/src/redux/analytics.ts, line 58 at r2 (raw file):

  },
  {
    match: new RegExp("/database/.+"),

Do matchers here run in sequential orders as they are defined? It seems like /database/.+ would also match /database/.+/grants

pkg/ui/src/views/databases/databaseTablePage/redux.spec.ts, line 107 at r2 (raw file):

    driver = new TestDriver(
      createAdminUIStore(createMemoryHistory()),
      "things",

super nit: got thrown off a bit reading through this, perhaps s/thing/fakeDbName/ and s/foo/fakeTableName/

pkg/ui/src/views/databases/databaseDetailsPage/redux.ts, line 34 at r2 (raw file):

} = cockroach.server.serverpb;

function normalizeRoles(raw: string[]): string[] {

nit: perhaps normalizeRolesOrdering ?

pkg/ui/src/views/databases/databaseDetailsPage/redux.ts, line 82 at r2 (raw file):

  ): DatabaseDetailsPageData => {
    return {
      loading: !!databaseDetails[database]?.inFlight,

what is this !! operator?

pkg/ui/src/views/databases/databaseDetailsPage/redux.ts, line 86 at r2 (raw file):

      name: database,
      tables: _.map(databaseDetails[database]?.data?.table_names, (table) => {
        const details = tableDetails[generateTableID(database, table)];

nit: save the tableID to avoid repeatedly calling generateTableID ?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng and @maryliag)

pkg/ui/cluster-ui/src/databasesPage/databasesPage.tsx, line 83 at r2 (raw file):

Good call, this one breaks the pattern. I'll add a comment.

pkg/ui/cluster-ui/src/databasesPage/databasesPage.tsx, line 134 at r2 (raw file):

Oh, I didn't realize most of what I wanted to do was already on Array -- it's been a while for me! I think it makes sense to lean less on an external dependency where we can, so I'll be happy to remove lodash where possible.

pkg/ui/src/redux/analytics.ts, line 58 at r2 (raw file):

Yes, the redaction function runs through these until it finds the first match; that's why the more specific matches come first in the list.

pkg/ui/src/views/databases/databaseTablePage/redux.spec.ts, line 107 at r2 (raw file):

Done.

pkg/ui/src/views/databases/databaseDetailsPage/redux.ts, line 34 at r2 (raw file):

This function (and normalizePrivileges, below) does a little more than just ordering, so I kind of like normalize as it stands. Did it seem unclear?

pkg/ui/src/views/databases/databaseDetailsPage/redux.ts, line 82 at r2 (raw file):

Ooh! It's a double-not -- you can pronounce it "bang bang!" -- good for turning "truthy" and "falsy" (false, null, undefined) into true and false. I'm using it here because databaseDetails[database] might be undefined, so inFlight might also be, and having a reliable boolean in the props made me and typescript a little happier. :-) Is there a more modern way to do this I don't know about?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @maryliag, and @matthewtodd)

pkg/ui/cluster-ui/src/databasesPage/databasesPage.tsx, line 134 at r2 (raw file):

👍

pkg/ui/src/redux/analytics.ts, line 58 at r2 (raw file):

👍

pkg/ui/src/views/databases/databaseDetailsPage/redux.ts, line 34 at r2 (raw file):

👍

pkg/ui/src/views/databases/databaseDetailsPage/redux.ts, line 82 at r2 (raw file):

Ah I see. I guess this is the price we pay for dynamic typing lol.

Reviewed 1 of 19 files at r2, 5 of 10 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @maryliag, and @matthewtodd)

pkg/ui/cluster-ui/src/databaseTablePage/databaseTablePage.tsx, line 215 at r3 (raw file):

                    <SummaryCardItem
                      label="Indexes"
                      value={_.join(this.props.details.indexNames, ", ")}

nit: similar message as the others, you can join without the use of lodash

pkg/ui/cluster-ui/src/icon/icon.module.scss, line 9 at r3 (raw file):

// the Business Source License, use of this software will be governed
// by the Apache License, Version 2.0, included in the file
// licenses/APL.txt.

did you made a check on all places using icon to make sure none of them misaligned?

pkg/ui/src/views/databases/databasesPage/redux.ts, line 36 at r3 (raw file):

      loading: databases.inFlight,
      loaded: databases.valid,
      databases: _.map(databases.data?.databases, (database) => {

nit: use map directly on the array (other example on this same file, same for .each)

pkg/ui/cluster-ui/src/databaseDetailsPage/databaseDetailsPage.tsx, line 156 at r3 (raw file):

    }

    _.forEach(this.props.tables, table => {

nit: no need for lodash here either, all the forEach can be directly on the array

pkg/ui/cluster-ui/src/databaseDetailsPage/databaseDetailsPage.tsx, line 300 at r3 (raw file):

      {
        title: "Roles",
        cell: table => _.join(table.details.roles, ", "),

nit for all joins: table.details.roles.join(", ")

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @maryliag, and @matthewtodd)

pkg/ui/cluster-ui/src/databasesPage/databasesPage.tsx, line 134 at r2 (raw file):

#68820

pkg/ui/cluster-ui/src/icon/icon.module.scss, line 9 at r3 (raw file):

This was a new file that only the new components used, and I checked them all. I've since unrolled it because its strategy of using data-uris for svgs prevented us from using css to size and color the icons.

pkg/ui/cluster-ui/src/databaseDetailsPage/databaseDetailsPage.tsx, line 156 at r3 (raw file):

Agreed, pushed to #68820 since there are some semantic differences between _.forEach and Array.forEach when undefined is involved.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @maryliag, and @matthewtodd)

pkg/ui/cluster-ui/src/databaseTablePage/databaseTablePage.tsx, line 215 at r3 (raw file):

Bumped to #68820

pkg/ui/src/views/databases/databasesPage/redux.ts, line 36 at r3 (raw file):

Bumped to #68820

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng and @maryliag)

pkg/ui/cluster-ui/src/databaseDetailsPage/databaseDetailsPage.tsx, line 300 at r3 (raw file):

Bumped to #68820

Great work on this, new page looks awesome!
As long as all comments are addressed (and necessary new issues created):

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @Azhng and @maryliag)

Thanks, @maryliag!

Follow-on issues are:

• #68820
• #68821
• #68823
• #68824
• #68825
• #68826
• #68828

I've tagged many of these as quick wins so we / I can find them easily on our next quick win day.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @Azhng and @maryliag)