Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ui: display closed sessions, add username and session status filter #80410

Merged
merged 1 commit into from
Apr 27, 2022

Conversation

gtr
Copy link
Contributor

@gtr gtr commented Apr 22, 2022

Fixes #67888, #79914.

Previously, the sessions page UI did not support displaying closed
sessions and did not support the ability to filter by username or
session status. This commit adds the "Closed" session status to closed
sessions and adds the ability to filter by username and session status.

Session Status:
https://user-images.githubusercontent.com/35943354/164794955-5a48d6c2-589d-4f05-b476-b30b114662ee.mov

Usernames:
https://user-images.githubusercontent.com/35943354/164797165-f00f9760-7127-4f2a-96bd-88f691395693.mov

Release note (ui change): sessions overview and session details pages now
display closed sessions; sessions overview page now has username and session
status filters

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@gtr gtr linked an issue Apr 22, 2022 that may be closed by this pull request
@gtr
Copy link
Contributor Author

gtr commented Apr 22, 2022

One thing to note about this change is that since the ListSessions endpoint now returns closed sessions, the UI now has many closed sessions opened by internal app names:
session-app-names

Additionally, these hidden closed sessions cause the pagination label to quickly increases in number:
session-pagination

While technically correct, it can be a bit confusing because those closed sessions are hidden by default. Is this a problem? I assume we still want to give the user the ability to toggle sessions created by internal app names, like we have in the past.

CC: @kevin-v-ngo @Annebirzin @maryliag

@kevin-v-ngo
Copy link

One thing to note about this change is that since the ListSessions endpoint now returns closed sessions, the UI now has many closed sessions opened by internal app names: session-app-names

Additionally, these hidden closed sessions cause the pagination label to quickly increases in number: session-pagination

While technically correct, it can be a bit confusing because those closed sessions are hidden by default. Is this a problem? CC: @kevin-v-ngo

Thanks Gerardo, do we paginate results before or after the filter is applied? From what you're saying, it seems we paginate the entire result regardless of whether the user specifies filters to hid certain row correct?

CC @Annebirzin

Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6 of 9 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @gtr and @maryliag)


pkg/ui/workspaces/cluster-ui/src/sessions/sessionsPage.tsx, line 109 at r1 (raw file):

  const uniqueAppNames = new Set(
    sessions.map(s =>
      s.session.application_name ? s.session.application_name : "(unset)",

nit: I just notice this, but we want to use just $ internal for all internal app names, so add a check of: if starts with the internal prefix, use just internal prefix


pkg/ui/workspaces/cluster-ui/src/sessions/sessionsPage.tsx, line 437 at r1 (raw file):

          pageSize={pagination.pageSize}
          current={pagination.current}
          total={sessionsData.length}

the reason why you always see the total on the pagination is because here you're using sessionsData.length instead of the correct value sessionsToDisplay.length

@gtr gtr force-pushed the gtr-sessions-ui branch from 57fc667 to d842f3e Compare April 25, 2022 15:12
Copy link
Contributor Author

@gtr gtr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


pkg/ui/workspaces/cluster-ui/src/sessions/sessionsPage.tsx line 109 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: I just notice this, but we want to use just $ internal for all internal app names, so add a check of: if starts with the internal prefix, use just internal prefix

Great catch! This also brings the number of filter options down.


pkg/ui/workspaces/cluster-ui/src/sessions/sessionsPage.tsx line 437 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

the reason why you always see the total on the pagination is because here you're using sessionsData.length instead of the correct value sessionsToDisplay.length

Thanks!

@gtr
Copy link
Contributor Author

gtr commented Apr 25, 2022

Thanks Gerardo, do we paginate results before or after the filter is applied? From what you're saying, it seems we paginate the entire result regardless of whether the user specifies filters to hid certain row correct?

CC @Annebirzin

As Marylia pointed out, the pagination total was always set to whatever the total number of sessions before we apply the filters. The correct value should be the total number of sessions after the filters are applied.

So when first loading the sessions page, this is what it now looks like:

sessions-page-empty

The filter now condenses all the $ internal app names and looks like this:

sessions-page-filter

And finally, when we select $ internal app names, we get the correct pagination:

sessions-page-internal

@Annebirzin
Copy link

@gtr 👍 Your latest update looks good to me (condense all internal app names, pagination applies after filters are applied)

Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 9 files at r1, 1 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @gtr and @maryliag)


pkg/ui/workspaces/cluster-ui/src/sessions/sessionsPage.tsx line 388 at r2 (raw file):

    // const totalSessions = sessionsData.length;
    const totalSessions = sessionsToDisplay.length;

nit: no need to create a variable just for the length, use this directly on the pagination component (also clean up the unnecessary comment)

Fixes cockroachdb#67888, cockroachdb#79914.

Previously, the sessions page UI did not support displaying closed
sessions and did not support the ability to filter by username or
session status. This commit adds the "Closed" session status to closed
sessions and adds the ability to filter by username and session status.

Release note (ui change): sessions overview and session details pages now
display closed sessions; sessions overview page now has username and session
status filters
@gtr gtr force-pushed the gtr-sessions-ui branch from d842f3e to cbb933f Compare April 26, 2022 16:57
Copy link
Contributor Author

@gtr gtr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


pkg/ui/workspaces/cluster-ui/src/sessions/sessionsPage.tsx line 388 at r2 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: no need to create a variable just for the length, use this directly on the pagination component (also clean up the unnecessary comment)

Didn't mean to leave that in there, thanks!

Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @gtr)

@gtr
Copy link
Contributor Author

gtr commented Apr 26, 2022

bors r+

@craig
Copy link
Contributor

craig bot commented Apr 27, 2022

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants