Is there any way we typically upload screen recordings to PRs?

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.tsx, line 358 at r2 (raw file):

                      <Tooltip
                        placement="bottom"
                        title="Clicking this button will clear index usage stats for all tables."

I've added in this tooltip to inform users that they would be resetting ALL index stats by pressing this button, but I'm still not sure if that's clear, since they're only able to view the table's index usage stats from this page... Do you think there's anything more we should do to make this more obvious/intuitive?

You can record with any tool you're used to (e.g. quickTime) and add here or use something like Loom (I send you an invite on it).
I'm trying to test, but is failing for me

Reviewed 8 of 9 files at r1, 1 of 13 files at r3, 8 of 13 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Annebirzin, @lindseyjin, and @maryliag)

pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.tsx, line 358 at r2 (raw file):

@Annebirzin can you validate if this text is clear enough? Is the reset index usage stats
"Clicking this button will clear index usage stats for all tables."

pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.tsx, line 101 at r4 (raw file):

}

interface IndexStat {

nit: IndexStats

pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.tsx, line 381 at r4 (raw file):

                    columns={this.indexStatsColumns}
                    sortSetting={this.state.sortSetting}
                    onChangeSortSetting={this.changeSortSetting.bind(this)}

nit: you don't need the bind part, just onChangeSortSetting={this.changeSortSetting}

pkg/ui/workspaces/db-console/src/util/api.ts, line 465 at r4 (raw file):

}

// getIndexStats gets detailed stats about the current table's index usage statistics

nit: period at the end of comment

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

pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.tsx, line 71 at r4 (raw file):

//         totalReads: number;
//         lastUsedTime: Moment;
//         lastUsedString: string;

Hmm, what's the difference here for lastUsedTime and lastUsedString ?

pkg/ui/workspaces/db-console/src/redux/indexUsageStats/indexUsageStatsSagas.ts, line 41 at r4 (raw file):

  action: PayloadAction<resetIndexUsageStatsPayload>,
) {
  const resetIndexUsageStatsRequest = new ResetIndexUsageStatsRequest();

It seems like ResetIndexUsageStatsRequest() here doesn't require the database and table name stored in the resetIndexUsageStatsPayload, perhaps this sagas doesn't really need to take in an action with payload then ?

pkg/ui/workspaces/db-console/src/redux/indexUsageStats/indexUsageStatsSagas.ts, line 55 at r4 (raw file):

    yield put(refreshIndexStats(new TableIndexStatsRequest({ database, table })) as any);
  } catch (e) {
    console.log(e);

Should we remove the console log here ?

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

pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.tsx, line 71 at r4 (raw file):

I take care of string formatting in the redux file, so lastUsedString is the string that we show in the table column, while lastUsedTime is the original timestamp, used for sorting. Or would it be better in this case to only pass the timestamp and do string formatting in this file?

pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.tsx, line 101 at r4 (raw file):

Hmm this is the interface for a single Index Usage Stat (in a row), so I think it should be singular, while the array would be plural? eg) indexStats[] is a list of indexStat objects.
I know for Grant the interface is in singular form too.

pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.tsx, line 381 at r4 (raw file):

Hmm, the sort doesn't seem to work anymore if I change to that?

pkg/ui/workspaces/db-console/src/redux/indexUsageStats/indexUsageStatsSagas.ts, line 41 at r4 (raw file):

Ah, the reason why I add the database and the table in the payload is so I can refresh the index stats for that particular database and table (the page that the user is currently on). Hmm if I remove it from the payload, is there some other way I can find out what those fields are? Or should I instead refresh index stats for every page in the cache?

pkg/ui/workspaces/db-console/src/redux/indexUsageStats/indexUsageStatsSagas.ts, line 55 at r4 (raw file):

Oops yes, thanks for catching that!

Here's a Loom link! Let me know if that works for you
https://www.loom.com/share/ba1fb40f8d5340b3909e4af93095a7d4

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

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

pkg/ui/package-lock.json, line 1 at r7 (raw file):

{

Heh, this file is new? I'm a bit surprised this got checked in. Is this by accident?

pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.tsx, line 71 at r4 (raw file):

Personally, I feel Redux mostly should be used for stateful information storage, and I keep rendering-related information within the React component. Since it seems like it's very trivial to derive the lastUsedString from the lastUsedTime, it would be preferred put that logic within the React component

pkg/ui/workspaces/db-console/src/redux/indexUsageStats/indexUsageStatsSagas.ts, line 41 at r4 (raw file):

Ah good point, right you still need to refresh them.

pkg/ui/workspaces/db-console/src/redux/indexUsageStats/indexUsageStatsSagas.spec.ts, line 37 at r7 (raw file):

          [call.fn(resetIndexUsageStats), resetIndexUsageStatsResponse],
        ])
        .put(resetIndexUsageStatsCompleteAction())

Hmm shouldn't we see some invalidation actions here ?

I think it's because clearing the stats also sends a request to fetch index stats for the current page, which reloads the page. Should I be finding a way to prevent this behaviour?

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

pkg/ui/package-lock.json, line 1 at r7 (raw file):

Oops yeah, removed!

Can you add some margin at the bottom of the index stats table? It's so close to the bottom of the page that looks like there are more things there.

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

pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.tsx, line 381 at r4 (raw file):

ah okay :)

pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.tsx, line 197 at r10 (raw file):

    const lastReset = this.props.indexStats.lastReset;
    if (lastReset.isSame(this.minDate)) {
      return "Last cleared: never";

nit: Never

pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.tsx, line 218 at r10 (raw file):

        // TODO(lindseyjin): replace default case with create time after it's added to table_indexes
        if (lastReset.isSame(this.minDate)) {
          return "Last reset: never";

nit: Never

Done!

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

pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.tsx, line 71 at r4 (raw file):

Done! But since lastUsed can be either "read" or "reset" or "create" (once that gets added in), I also added another field lastUsedType to keep track of the type

pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.tsx, line 197 at r10 (raw file):

Done!

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

pkg/ui/workspaces/db-console/src/redux/indexUsageStats/indexUsageStatsSagas.spec.ts, line 37 at r7 (raw file):

Added!

Reviewed 3 of 17 files at r6, 2 of 15 files at r9, 9 of 13 files at r10, 1 of 1 files at r11, 2 of 2 files at r12, 1 of 1 files at r13, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Annebirzin, @Azhng, @lindseyjin, and @maryliag)

pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.module.scss, line 14 at r13 (raw file):

.tab-area {
  padding-bottom: 80px;

nit: we use padding for controlling space within the same element, in this case you want to add space to the next thing, so the preference is to use margin-bottom here. Also 32px should be enough :)

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

pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.module.scss, line 14 at r13 (raw file):

Fixed!

Reviewed all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Annebirzin, @Azhng, @lindseyjin, and @maryliag)

pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.module.scss, line 82 at r14 (raw file):

  &__last-cleared {
    color: "#475872";

nit: use $colors--neutral-6

pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.module.scss, line 106 at r14 (raw file):

.icon {
  &--s {
    height: 16px;

nit: $line-height--x-small

pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.module.scss, line 107 at r14 (raw file):

  &--s {
    height: 16px;
    width: 16px;

nit: $line-height--x-small

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

pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.module.scss, line 82 at r14 (raw file):

Done!

Great work! Make sure to get approval from @Azhng too, but from the changes I requested

Reviewed 1 of 1 files at r15, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @Annebirzin, @Azhng, @lindseyjin, and @maryliag)

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

pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.tsx, line 188 at r15 (raw file):

  }

  minDate = moment.utc("0001-01-01"); // minimum value as per UTC

nit: i think you can simply just do moment.utc() here?

pkg/ui/workspaces/db-console/src/redux/indexUsageStats/indexUsageStatsSagas.ts, line 25 at r15 (raw file):

  KeyedCachedDataReducerState,
  refreshIndexStats,
} from "oss/src/redux/apiReducers";

nit: i think you can just do src/redux/apiReducers here. Having the oss prefix means we are only importing from the oss build, this is probably fine here but I would prefer to not have the build information leaked into the application logic.

pkg/ui/workspaces/db-console/src/redux/indexUsageStats/indexUsageStatsSagas.spec.ts, line 41 at r15 (raw file):

        ])
        .put(resetIndexUsageStatsCompleteAction())
        .put(invalidateIndexStats(KeyToTableRequest("database/table")))

I was really surprised that this test would pass, since this saga was not attached to any Redux state, and without state, the .forEach() function would not execute, which mean it shouldn't be sending out any invalidateInDexStats actions.

But the fact that this test passed made me realized we have been using expectSaga incorrectly for a while now, (mostly its just me 🤦 )

expectSaga returns a Promise to the test runner (karma for db-console) so the test runner know when the test is finished. That means we need to return the entire chained expectSaga call. Otherwise, this entire test is also just ... no-op

You can reference #73180 to see why the current expectSaga is incorrect.

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

pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.tsx, line 358 at r2 (raw file):

I can follow up with Anne on Monday, and update this in the next PR if it's not clear enough

pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.tsx, line 188 at r15 (raw file):

Looks like moment.utc() by itself just returns the current time :')

pkg/ui/workspaces/db-console/src/redux/indexUsageStats/indexUsageStatsSagas.ts, line 25 at r15 (raw file):

Done!

pkg/ui/workspaces/db-console/src/redux/indexUsageStats/indexUsageStatsSagas.spec.ts, line 41 at r15 (raw file):

I've modified my test cases to return the chained expectSaga call. However, I'm having a bit of trouble getting invalidate or refresh checks working in my test

Even when I set an initial state or mock the select return value, I've been running into this following error: sagaTestError: put expectation unmet. I tried fiddling around for a bit but haven't gotten anything working yet. Do you have any suggestions? And is this something I can come back to later?

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

pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.tsx, line 188 at r15 (raw file):

Ah right, my bad

pkg/ui/workspaces/db-console/src/redux/indexUsageStats/indexUsageStatsSagas.spec.ts, line 41 at r15 (raw file):

Hmm yeah ThunkAction doesn't quite work with the Action type that Sagas is expecting. I couldn't find a way to get them to work either. Leave a TODO here so we can come back and revisit this later.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @Annebirzin, @Azhng, @lindseyjin, and @maryliag)

pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.tsx, line 358 at r2 (raw file):

Oops, turned out there was tooltip text provided in the mocks. Updated to match the Figma mocks!

Screen Shot 2021-11-29 at 12.17.26 PM

pkg/ui/workspaces/db-console/src/redux/indexUsageStats/indexUsageStatsSagas.spec.ts, line 41 at r15 (raw file):

Done!

Tooltips removed and colour updated!

Also created an issue to add the ability to remove dotted underline for the Sorted Table component, which i'll come back to later #73280

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @Annebirzin, @Azhng, @lindseyjin, and @maryliag)