ui: Add runs overview page for admins #1238
Conversation
ui/src/routes/_layout/route.tsx
Outdated
| {['/', '/runs', '/create-organization'].includes(location.pathname) ? ( | ||
| <PageLayout sections={sections}> | ||
| <Outlet /> | ||
| </PageLayout> | ||
| ) : ( | ||
| <Outlet /> | ||
| )} |
There was a problem hiding this comment.
This should be done with the file-based routing instead.
ui/src/routes/_layout/runs/index.tsx
Outdated
| const columns: ColumnDef<OrtRunSummary>[] = [ | ||
| { |
There was a problem hiding this comment.
Old tables use this syntax, but for new tables I'd use the column helper function which provides better type-safety.
ui/src/routes/_layout/runs/index.tsx
Outdated
| <div> | ||
| {new Date(row.original.createdAt).toLocaleString(navigator.language)} | ||
| </div> |
ui/src/routes/_layout/runs/index.tsx
Outdated
| <div> | ||
| {new Date(row.original.finishedAt).toLocaleString(navigator.language)} | ||
| </div> | ||
| ) : ( | ||
| <div className='italic'>Not finished yet</div> |
ui/src/routes/_layout/runs/index.tsx
Outdated
| {new Date(row.original.createdAt).toLocaleString(navigator.language)} | ||
| </div> | ||
| ), | ||
| size: 100, |
There was a problem hiding this comment.
What's the reason for manually specifying the sizes?
There was a problem hiding this comment.
The default min-width for a column is 150 px so it lowers that, so that some of the columns are a bit narrower and the columns fit on the current width that we have on the layout. I still changed it for a couple of the columns cause I noticed the table was actually still overflowing even though it looked ok.
ui/src/routes/_layout/runs/index.tsx
Outdated
| size: 100, | ||
| }, | ||
| { | ||
| accessorKey: 'duration', |
There was a problem hiding this comment.
This should probably error with the column helpers mentioned, as I believe there is no duration in the returned object. I suggest reading https://tanstack.com/table/latest/docs/guide/column-defs#creating-accessor-columns for how the accessors work.
ui/src/routes/_layout/runs/index.tsx
Outdated
| await Promise.allSettled([ | ||
| prefetchUseRunsServiceGetOrtRuns(context.queryClient, { | ||
| limit: pageSize || defaultPageSize, | ||
| offset: page ? (page - 1) * (pageSize || defaultPageSize) : 0, | ||
| status: status?.join(','), | ||
| }), | ||
| ]); |
There was a problem hiding this comment.
No need to use allSettled for one promise.
ui/src/routes/_layout/runs/route.tsx
Outdated
| export const Route = createFileRoute('/_layout/runs')({ | ||
| component: () => <Outlet />, | ||
| }); |
There was a problem hiding this comment.
Is this required to be added?
lamppu
force-pushed
the
801-add-runs-overview-page-for-admins
branch
from
ba4cfb0
to
27cb98c
Compare
Only render the separator after a section if some of the remaining sections are visible to the user. Signed-off-by: Johanna Lamppu <johanna.lamppu@doubleopen.org>
Add toolbar that can be used with a data table to show filter and reset buttons, and later on can be extended to include other possible options to modify a table with. Signed-off-by: Johanna Lamppu <johanna.lamppu@doubleopen.org>
Add multivalue filter that can be used in the table toolbar to allow filtering the data with provided options. Signed-off-by: Johanna Lamppu <johanna.lamppu@doubleopen.org>
lamppu
force-pushed
the
801-add-runs-overview-page-for-admins
branch
from
27cb98c
to
796a3aa
Compare
| <PageLayout sections={rootSections(user.hasRole(['superuser']))}> | ||
| <CreateOrganizationPage /> | ||
| </PageLayout> |
There was a problem hiding this comment.
Generally this sort of layout sharing should be done by creating a _adminSidebarLayout etc. directory in the current _layout directory, add a route.tsx there with the shared PageLayout with the same sections and move create-organization.tsx and index.tsx there (or handling it with file names like _adminSidebarLayout.route.tsx, _adminSidebarLayout.index.tsx and _adminSidebarLayout.create-organization.tsx directly in _layout directory). That ensures that all shared layouts are handled the same way, so no need to go through and process what rootSections() etc. do.
Here I however wonder whether we could and should just omit the sidebar sections from create-organization page? I think with the current amount of links in the sidebar, it's not really important to be able to select from those in there. As runs/index is also available only for superusers, should it maybe be under the admn route and have a link to it there?
There was a problem hiding this comment.
Ah ok, I understood it wrong then. Well, the thing is, if you omit it from the create organization page, then there will be this jump in the layout when accessing that page, so that's mainly why I would add it there.
As runs/index is also available only for superusers, should it maybe be under the admin route and have a link to it there?
So to move it to the admin dashboard? I figured this might be something that would be good to be more easily accessible from the front page, but I'm not sure. Or did you mean just change the link?
There was a problem hiding this comment.
Ah ok, I understood it wrong then. Well, the thing is, if you omit it from the create organization page, then there will be this jump in the layout when accessing that page, so that's mainly why I would add it there.
As runs/index is also available only for superusers, should it maybe be under the admin route and have a link to it there?
So to move it to the admin dashboard? I figured this might be something that would be good to be more easily accessible from the front page, but I'm not sure. Or did you mean just change the link?
Yeah exactly, move it to the admin dashboard, probably as a sub-page there. I think as it's a superuser functionality it doesn't really need to be that easily accessible from the front page.
Some layout jumping is totally fine if the pages have different layout. I think it also maybe doesn't make sense to show the sidebar on the front-page, as normal users see only one item there for the page that they're already on, so it provides no value for the regular user.
There was a problem hiding this comment.
Yeah exactly, move it to the admin dashboard, probably as a sub-page there.
Alright, I guess I'll change this to a draft for a bit and do some refactoring on the admin dashboard layout first to align it with the rest of the ui before adding this there.
Add a runs overview page to allow superusers to get a quick overview of all runs on the server instance. Signed-off-by: Johanna Lamppu <johanna.lamppu@doubleopen.org>
lamppu
force-pushed
the
801-add-runs-overview-page-for-admins
branch
from
796a3aa
to
6e8687c
Compare
Add runs overview page for superusers to get a quick overview of the runs on the server instance.