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

pkg/ui/src/views/statements/statementsPage.tsx, line 232 at r6 (raw file):

      // We currently don't have the support for determining the bounds
      // of persisted statement data, aside from issuing a get all.
      validStatementsDateRange: [

I don't we should limit one year or trying to find the oldest data. The picker itself can go back as far as possible and the user selects what makes more sense. Is common to have a selector going back more than you have data for, we simply show what we have
(make that change on the other statementsPage too, so have older data range)

A tour de force, nice work!

I am wondering whether we want to expose this "combined" concept up through the API, or whether we see it more as an implementation detail of The Future way to view statements, in which case maybe we take over the existing /_status/statements endpoint, making the "combined" behavior opt-in with a query parameter for now, but eventually making "combined" the default?

What do you all think?

Reviewed 1 of 19 files at r3, 11 of 34 files at r5, 35 of 35 files at r6, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @xinhaoz)

Reviewed 1 of 35 files at r6.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @matthewtodd and @xinhaoz)

pkg/ui/cluster-ui/src/statementsPage/statementsPage.selectors.ts, line 49 at r7 (raw file):

export const statementsSelector = createSelector(
  adminUISelector,
  adminUiState => adminUiState.combinedStatements,

you may need to change back to .statements, otherwise it may not work as expected on the cases you're calling the statements api

pkg/ui/cluster-ui/src/statementsPage/statementsPageConnected.tsx, line 59 at r7 (raw file):

    }),
    (dispatch: Dispatch) => ({
      refreshStatements: () => dispatch(combinedStatementActions.refresh()),

similar to previous comments, we won't be refreshing combined on all cases, so might need to handle here too

pkg/ui/src/redux/statements/statementsSelectors.ts, line 18 at r7 (raw file):

export const selectStatementByFingerprint = createSelector(
  (state: AdminUIState) => state.cachedData.combinedStatements.data?.statements,

similar to the other comments

pkg/ui/src/views/statements/statementDetails.tsx, line 153 at r7 (raw file):

export const selectStatement = createSelector(
  (state: AdminUIState) => state.cachedData.combinedStatements,

same comment as other cases

pkg/ui/src/views/statements/statementsPage.tsx, line 67 at r7 (raw file):

// StatementsPage, based on if the appAttr route parameter is set.
export const selectStatements = createSelector(
  (state: AdminUIState) => state.cachedData.combinedStatements,

same as others

Nice. Great work on this. Just some quick drive-by comments.

Also, do you mind put some screenshots into the PR description ?

Reviewed 1 of 19 files at r3, 11 of 34 files at r5, 4 of 35 files at r6, 3 of 14 files at r7.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @matthewtodd and @xinhaoz)

-- commits, line 42 at r6:
release justification?

pkg/ui/cluster-ui/src/api/fetchData.ts, line 38 at r6 (raw file):

// - non-string values will be toString'd
export function propsToQueryString(props: { [k: string]: any }) {
  return _.compact(

in other pages we are trying to move from using lodash to using vanilla js array functions (#68820). Do we really need lodash here?

pkg/ui/cluster-ui/src/store/localStorage/localStorage.reducer.ts, line 42 at r7 (raw file):

const initialState: LocalStorageState = {
  "adminUi/showDiagnosticsModal":
    Boolean(JSON.parse(localStorage.getItem("adminUi/showDiagnosticsModal"))) ||

This is interesting. Why do we need to JSON.parse it ?

the phrasing sounds a little weird, can you change to “Date interval not valid” if the before happens after the end, and “Select a start and end date” when one of them is not selected

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

Reviewed 1 of 67 files at r8, 4 of 5 files at r15, 1 of 1 files at r16, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @Azhng, @maryliag, @matthewtodd, and @xinhaoz)

Reviewed 41 of 47 files at r17.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @Azhng, @maryliag, @matthewtodd, and @xinhaoz)

-- commits, line 28 at r17:
nit: and transaction page

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx, line 168 at r17 (raw file):

  if (props.dateRange == null) return null;
  return new cockroach.server.serverpb.StatementsRequest({
    combined: true,

you need to use this value from the props too, since it will be false for tenants
I added the props value on a PR currently being merged, you can rebase yours with that one and use the value from there
#69444

pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPageConnected.tsx, line 50 at r17 (raw file):

    (state: AppState, props: StatementsPageProps) => ({
      statements: selectStatements(state, props),
      dateRange: state.adminUI.uiConfig.isTenant

once you merge with the PR I mention, change this to
dateRange: selectIsTenant(state) ? null : selectDateRange(state)
just to be on the safe side, so we have one single place of source for that value

pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPage.tsx, line 90 at r17 (raw file):

  if (props.dateRange == null) return null;
  return new protos.cockroach.server.serverpb.StatementsRequest({
    combined: true,

similar to the other case, the combined value must check the isTenant value

pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPageConnected.tsx, line 43 at r17 (raw file):

      nodeRegions: nodeRegionsByIDSelector(state),
      error: selectTransactionsLastError(state),
      dateRange: state.adminUI.uiConfig.isTenant

similar to the other message, use the selector from the other PR

pkg/ui/workspaces/db-console/src/redux/statements/statementsSelectors.ts, line 22 at r17 (raw file):

  (statements, statementFingerprint) => {
    if (statements == null) {
      return null;

why you want to return null here? using an empty array is safer. If you're using this value to decide on using statements vs combined statements, you should be using the value of isTenant

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

): Promise<StatementsResponseMessage> {
  const queryStr = propsToQueryString({
    combined: true,

this value should be based on value of isTenant

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

pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPage.tsx, line 90 at r17 (raw file):

Done.

pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPageConnected.tsx, line 43 at r17 (raw file):

Done.

pkg/ui/workspaces/db-console/src/redux/statements/statementsSelectors.ts, line 22 at r17 (raw file):

Done.

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

This is only used for db console, so we'll always be using combined api.

Reviewed 4 of 11 files at r18, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @Azhng, @maryliag, @matthewtodd, and @xinhaoz)

pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPage.tsx, line 90 at r18 (raw file):

  props: TransactionsPageProps,
): protos.cockroach.server.serverpb.StatementsRequest | null {
  if (props.isTenant || props.dateRange == null) return null;

on the previous refresh the in-memory was being updated, now your new refresh call this function and this only updates the combined data, so when it's a tenant or the user didn't make any date selection is not doing any refreshes. We still want tenant values to be refreshed, so you still return a request when is tenant or there is not data range, the difference is that you will make the call without passing any of those values.

Thank you for adding changes to transaction too. I had just 3 small nits and then

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

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.selectors.ts, line 123 at r18 (raw file):

  (statementsState, props) => {
    const statements = statementsState.data?.statements;

nit: remove blank line

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetailsConnected.ts, line 46 at r18 (raw file):

    statement,
    statementsError: state.adminUI.statements.lastError,
    dateRange: state.adminUI.uiConfig.isTenant ? null : selectDateRange(state),

nit: can you change this one too to use the selector instead of the parameter directly?

pkg/ui/workspaces/db-console/src/redux/statements/statementsSelectors.ts, line 25 at r18 (raw file):

    );
  },
);

you can remove the return now, since you retracted the other changes you made here

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

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.selectors.ts, line 123 at r18 (raw file):

Done.

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetailsConnected.ts, line 46 at r18 (raw file):

Done.

pkg/ui/workspaces/db-console/src/redux/statements/statementsSelectors.ts, line 25 at r18 (raw file):

Done.