From 1cbc643e4d47d072c986272608994fe56bef4dcb Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Tue, 12 Mar 2024 15:56:17 -0700 Subject: [PATCH 1/8] Highlight sidebar nav links when in create form --- app/components/Sidebar.tsx | 44 ++++++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 18 deletions(-) diff --git a/app/components/Sidebar.tsx b/app/components/Sidebar.tsx index e4b383282b..0e9674f3af 100644 --- a/app/components/Sidebar.tsx +++ b/app/components/Sidebar.tsx @@ -6,7 +6,7 @@ * Copyright Oxide Computer Company */ import cn from 'classnames' -import { NavLink } from 'react-router-dom' +import { NavLink, useLocation } from 'react-router-dom' import { Action16Icon, Document16Icon } from '@oxide/design-system/icons/react' @@ -88,20 +88,28 @@ export const NavLinkItem = (props: { children: React.ReactNode end?: boolean disabled?: boolean -}) => ( -
  • - - cn(linkStyles, { - 'text-accent !bg-accent-secondary hover:!bg-accent-secondary-hover svg:!text-accent-tertiary': - isActive, - 'pointer-events-none text-disabled': props.disabled, - }) - } - end={props.end} - > - {props.children} - -
    • -) +}) => { + // "New" resource create forms have a url that doesn't match the top-level resource name, so `isActive` won't ever fire as true. + // Determine which resource in the Sidebar should be highlighted as active when on a create-form page. + const location = useLocation() + const resourcePath = location.pathname.split('/')[3] + const resourceName = resourcePath.includes('-new') ? resourcePath.replace('-new', '') : '' + const isLinkToThisResource = resourceName.length > 0 && props.to.includes(resourceName) + return ( +
  • + + cn(linkStyles, { + 'text-accent !bg-accent-secondary hover:!bg-accent-secondary-hover svg:!text-accent-tertiary': + isActive || isLinkToThisResource, + 'pointer-events-none text-disabled': props.disabled, + }) + } + end={props.end} + > + {props.children} + +
    • + ) +} From 69a602f7b3367370bfe8928779f1100e06c96892 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Tue, 12 Mar 2024 16:05:02 -0700 Subject: [PATCH 2/8] more resilient for when path can't be parsed --- app/components/Sidebar.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/components/Sidebar.tsx b/app/components/Sidebar.tsx index 0e9674f3af..c2dc39e176 100644 --- a/app/components/Sidebar.tsx +++ b/app/components/Sidebar.tsx @@ -93,7 +93,9 @@ export const NavLinkItem = (props: { // Determine which resource in the Sidebar should be highlighted as active when on a create-form page. const location = useLocation() const resourcePath = location.pathname.split('/')[3] - const resourceName = resourcePath.includes('-new') ? resourcePath.replace('-new', '') : '' + const resourceName = resourcePath?.includes('-new') + ? resourcePath.replace('-new', '') + : '' const isLinkToThisResource = resourceName.length > 0 && props.to.includes(resourceName) return (
  • From 07c9a4ae9b81f83577896f6895206dd47f43fab6 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Tue, 12 Mar 2024 16:07:20 -0700 Subject: [PATCH 3/8] More flexible, so it works on Silo pages as well --- app/components/Sidebar.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/components/Sidebar.tsx b/app/components/Sidebar.tsx index c2dc39e176..fa7e5f4b88 100644 --- a/app/components/Sidebar.tsx +++ b/app/components/Sidebar.tsx @@ -92,7 +92,7 @@ export const NavLinkItem = (props: { // "New" resource create forms have a url that doesn't match the top-level resource name, so `isActive` won't ever fire as true. // Determine which resource in the Sidebar should be highlighted as active when on a create-form page. const location = useLocation() - const resourcePath = location.pathname.split('/')[3] + const resourcePath = location.pathname.split('/').pop() const resourceName = resourcePath?.includes('-new') ? resourcePath.replace('-new', '') : '' From 333c8884c1825c5ccdc7fe863ed8b55bd373f0c5 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Tue, 12 Mar 2024 19:57:34 -0700 Subject: [PATCH 4/8] Simpler logic for location parsing; tests still not working correctly --- app/components/Sidebar.tsx | 12 +++--------- app/util/links.spec.ts | 9 +++++++++ app/util/links.ts | 10 ++++++++++ 3 files changed, 22 insertions(+), 9 deletions(-) diff --git a/app/components/Sidebar.tsx b/app/components/Sidebar.tsx index fa7e5f4b88..9e1178b229 100644 --- a/app/components/Sidebar.tsx +++ b/app/components/Sidebar.tsx @@ -6,13 +6,14 @@ * Copyright Oxide Computer Company */ import cn from 'classnames' -import { NavLink, useLocation } from 'react-router-dom' +import { NavLink } from 'react-router-dom' import { Action16Icon, Document16Icon } from '@oxide/design-system/icons/react' import { openQuickActions } from '~/hooks' import { Button } from '~/ui/lib/Button' import { Truncate } from '~/ui/lib/Truncate' +import { useIsNewResourcePage } from '~/util/links' const linkStyles = 'flex h-7 items-center rounded px-2 text-sans-md hover:bg-hover svg:mr-2 svg:text-quinary text-secondary' @@ -89,14 +90,7 @@ export const NavLinkItem = (props: { end?: boolean disabled?: boolean }) => { - // "New" resource create forms have a url that doesn't match the top-level resource name, so `isActive` won't ever fire as true. - // Determine which resource in the Sidebar should be highlighted as active when on a create-form page. - const location = useLocation() - const resourcePath = location.pathname.split('/').pop() - const resourceName = resourcePath?.includes('-new') - ? resourcePath.replace('-new', '') - : '' - const isLinkToThisResource = resourceName.length > 0 && props.to.includes(resourceName) + const isLinkToThisResource = useIsNewResourcePage(props.to) return (
  • { }) } }) + +// describe('check useIsNewResourcePage to evaluate Sidebar navigation links', () => { +// test('should return false for non-resource creation pages', () => { +// const fakeURL = new URL('https://oxide.computer/projects/mock-project/instances-new') +// vi.spyOn(window, 'location', 'get').mockReturnValue(fakeURL) +// const isNewResourcePage = useIsNewResourcePage('projects/mock-project/instances') +// expect(isNewResourcePage).toEqual(true) +// }) +// }) diff --git a/app/util/links.ts b/app/util/links.ts index 4418c90a62..57d42f346a 100644 --- a/app/util/links.ts +++ b/app/util/links.ts @@ -1,3 +1,5 @@ +import { useLocation } from 'react-router-dom' + /* * This Source Code Form is subject to the terms of the Mozilla Public * License, v. 2.0. If a copy of the MPL was not distributed with this @@ -10,3 +12,11 @@ export const links: Record = { cloudInitExamples: 'https://cloudinit.readthedocs.io/en/latest/reference/examples.html', ipPoolsDocs: 'https://docs.oxide.computer/guides/operator/ip-pool-management', } + +// Resource creation pages (e.g. /instances-new) don't have a route that matches the top-level resource name, +// so `isActive` won't ever fire as true, and the Sidebar won't highlight the correct resource. +// Determine, for a given Sidebar URL, whether it's the matching top-level resource URL for the current page. +export const useIsNewResourcePage = (resource: string): boolean => { + const location = useLocation() + return location.pathname === `${resource}-new` +} From d6d4b3191d33b8d007e051dc221eaac5d851700e Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Tue, 12 Mar 2024 20:10:53 -0700 Subject: [PATCH 5/8] Refactor to make util pure string evaluation; fix test --- app/components/Sidebar.tsx | 9 +++++---- app/util/links.spec.ts | 24 +++++++++++++++--------- app/util/links.ts | 8 ++------ 3 files changed, 22 insertions(+), 19 deletions(-) diff --git a/app/components/Sidebar.tsx b/app/components/Sidebar.tsx index 9e1178b229..f222565d85 100644 --- a/app/components/Sidebar.tsx +++ b/app/components/Sidebar.tsx @@ -6,14 +6,14 @@ * Copyright Oxide Computer Company */ import cn from 'classnames' -import { NavLink } from 'react-router-dom' +import { NavLink, useLocation } from 'react-router-dom' import { Action16Icon, Document16Icon } from '@oxide/design-system/icons/react' import { openQuickActions } from '~/hooks' import { Button } from '~/ui/lib/Button' import { Truncate } from '~/ui/lib/Truncate' -import { useIsNewResourcePage } from '~/util/links' +import { isRootResourceLink } from '~/util/links' const linkStyles = 'flex h-7 items-center rounded px-2 text-sans-md hover:bg-hover svg:mr-2 svg:text-quinary text-secondary' @@ -90,7 +90,8 @@ export const NavLinkItem = (props: { end?: boolean disabled?: boolean }) => { - const isLinkToThisResource = useIsNewResourcePage(props.to) + const location = useLocation() + const isLinkToRootResource = isRootResourceLink(props.to, location.pathname) return (
  • cn(linkStyles, { 'text-accent !bg-accent-secondary hover:!bg-accent-secondary-hover svg:!text-accent-tertiary': - isActive || isLinkToThisResource, + isActive || isLinkToRootResource, 'pointer-events-none text-disabled': props.disabled, }) } diff --git a/app/util/links.spec.ts b/app/util/links.spec.ts index 6f1cab5b16..56dcbd91b7 100644 --- a/app/util/links.spec.ts +++ b/app/util/links.spec.ts @@ -7,7 +7,7 @@ */ import { describe, expect, test } from 'vitest' -import { links } from './links' +import { isRootResourceLink, links } from './links' describe('check links are accessible', () => { for (const [key, url] of Object.entries(links)) { @@ -19,11 +19,17 @@ describe('check links are accessible', () => { } }) -// describe('check useIsNewResourcePage to evaluate Sidebar navigation links', () => { -// test('should return false for non-resource creation pages', () => { -// const fakeURL = new URL('https://oxide.computer/projects/mock-project/instances-new') -// vi.spyOn(window, 'location', 'get').mockReturnValue(fakeURL) -// const isNewResourcePage = useIsNewResourcePage('projects/mock-project/instances') -// expect(isNewResourcePage).toEqual(true) -// }) -// }) +describe('check isRootResourceLink to evaluate Sidebar navigation links', () => { + test('should return true for resource creation pages', () => { + const navUrl = 'projects/mock-project/instances' + const locationPathname = 'projects/mock-project/instances-new' + const isLinkToRootResource = isRootResourceLink(navUrl, locationPathname) + expect(isLinkToRootResource).toEqual(true) + }) + test('should return false for non-resource creation pages', () => { + const navUrl = 'projects/mock-project/disks' + const locationPathname = 'projects/mock-project/instances-new' + const isLinkToRootResource = isRootResourceLink(navUrl, locationPathname) + expect(isLinkToRootResource).toEqual(false) + }) +}) diff --git a/app/util/links.ts b/app/util/links.ts index 57d42f346a..e7ea025140 100644 --- a/app/util/links.ts +++ b/app/util/links.ts @@ -1,5 +1,3 @@ -import { useLocation } from 'react-router-dom' - /* * This Source Code Form is subject to the terms of the Mozilla Public * License, v. 2.0. If a copy of the MPL was not distributed with this @@ -16,7 +14,5 @@ export const links: Record = { // Resource creation pages (e.g. /instances-new) don't have a route that matches the top-level resource name, // so `isActive` won't ever fire as true, and the Sidebar won't highlight the correct resource. // Determine, for a given Sidebar URL, whether it's the matching top-level resource URL for the current page. -export const useIsNewResourcePage = (resource: string): boolean => { - const location = useLocation() - return location.pathname === `${resource}-new` -} +export const isRootResourceLink = (navUrl: string, locationPathname: string): boolean => + locationPathname === `${navUrl}-new` From e1830f4c946a3b085b059c6744abeb59767dbd13 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 12 Mar 2024 18:53:18 -0500 Subject: [PATCH 6/8] Use list fetches + `setQueryData` to prefetch items for `*NameFromId` cells (#2055) get wild with prefetches for *NameFromId cells --- app/api/hooks.ts | 5 ++++ app/pages/project/disks/DisksPage.tsx | 24 +++++++++++++-- app/pages/project/snapshots/SnapshotsPage.tsx | 29 +++++++++++++++++-- app/pages/system/networking/IpPoolPage.tsx | 9 ++++++ 4 files changed, 61 insertions(+), 6 deletions(-) diff --git a/app/api/hooks.ts b/app/api/hooks.ts index a44aba237c..88c82a2053 100644 --- a/app/api/hooks.ts +++ b/app/api/hooks.ts @@ -219,6 +219,11 @@ export const wrapQueryClient = (api: A, queryClient: QueryC queryClient.invalidateQueries({ queryKey: [method], ...filters }), setQueryData: (method: M, params: Params, data: Result) => queryClient.setQueryData([method, params], data), + setQueryDataErrorsAllowed: ( + method: M, + params: Params, + data: ErrorsAllowed, ApiError> + ) => queryClient.setQueryData([method, params, ERRORS_ALLOWED], data), fetchQuery: ( method: M, params: Params, diff --git a/app/pages/project/disks/DisksPage.tsx b/app/pages/project/disks/DisksPage.tsx index 0ac9448ef1..9fe75ed560 100644 --- a/app/pages/project/disks/DisksPage.tsx +++ b/app/pages/project/disks/DisksPage.tsx @@ -44,9 +44,27 @@ const EmptyState = () => ( ) DisksPage.loader = async ({ params }: LoaderFunctionArgs) => { - await apiQueryClient.prefetchQuery('diskList', { - query: { ...getProjectSelector(params), limit: 25 }, - }) + const { project } = getProjectSelector(params) + await Promise.all([ + apiQueryClient.prefetchQuery('diskList', { + query: { project, limit: 25 }, + }), + + // fetch instances and preload into RQ cache so fetches by ID in + // InstanceLinkCell can be mostly instant yet gracefully fall back to + // fetching individually if we don't fetch them all here + apiQueryClient + .fetchQuery('instanceList', { query: { project, limit: 200 } }) + .then((instances) => { + for (const instance of instances.items) { + apiQueryClient.setQueryData( + 'instanceView', + { path: { instance: instance.id } }, + instance + ) + } + }), + ]) return null } diff --git a/app/pages/project/snapshots/SnapshotsPage.tsx b/app/pages/project/snapshots/SnapshotsPage.tsx index 13fdf3aed6..4208dee58a 100644 --- a/app/pages/project/snapshots/SnapshotsPage.tsx +++ b/app/pages/project/snapshots/SnapshotsPage.tsx @@ -50,9 +50,32 @@ const EmptyState = () => ( ) SnapshotsPage.loader = async ({ params }: LoaderFunctionArgs) => { - await apiQueryClient.prefetchQuery('snapshotList', { - query: { ...getProjectSelector(params), limit: 25 }, - }) + const { project } = getProjectSelector(params) + await Promise.all([ + apiQueryClient.prefetchQuery('snapshotList', { + query: { project, limit: 25 }, + }), + + // Fetch disks and preload into RQ cache so fetches by ID in DiskNameFromId + // can be mostly instant yet gracefully fall back to fetching individually + // if we don't fetch them all here. This has to be the *ErrorsAllowed + // version of setQueryData because the disk fetchs are also the errors + // allowed version. If we use regular setQueryData, nothing blows up; the + // data is just never found in the cache. Note that the disks that error + // (delete disks) are not prefetched here because they are (obviously) not + // in the disk list response. + apiQueryClient + .fetchQuery('diskList', { query: { project, limit: 200 } }) + .then((disks) => { + for (const disk of disks.items) { + apiQueryClient.setQueryDataErrorsAllowed( + 'diskView', + { path: { disk: disk.id } }, + { type: 'success', data: disk } + ) + } + }), + ]) return null } diff --git a/app/pages/system/networking/IpPoolPage.tsx b/app/pages/system/networking/IpPoolPage.tsx index ddaaf89ab5..b64b777dcd 100644 --- a/app/pages/system/networking/IpPoolPage.tsx +++ b/app/pages/system/networking/IpPoolPage.tsx @@ -54,6 +54,15 @@ IpPoolPage.loader = async function ({ params }: LoaderFunctionArgs) { path: { pool }, query: { limit: 25 }, // match QueryTable }), + + // fetch silos and preload into RQ cache so fetches by ID in SiloNameFromId + // can be mostly instant yet gracefully fall back to fetching individually + // if we don't fetch them all here + apiQueryClient.fetchQuery('siloList', { query: { limit: 200 } }).then((silos) => { + for (const silo of silos.items) { + apiQueryClient.setQueryData('siloView', { path: { silo: silo.id } }, silo) + } + }), ]) return null } From c439b93cd9122448037b2ea753f4d56a5fe03b18 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Wed, 13 Mar 2024 10:59:33 -0700 Subject: [PATCH 7/8] Add Type column to access table (#2061) * Add Type column to access table * Remove AccessNameCell --------- Co-authored-by: David Crespo --- app/components/AccessNameCell.tsx | 34 ------------------- app/pages/SiloAccessPage.tsx | 8 +++-- .../project/access/ProjectAccessPage.tsx | 8 +++-- app/util/access.spec.tsx | 15 ++++++++ app/util/access.ts | 11 ++++++ test/e2e/project-access.e2e.ts | 8 +++-- test/e2e/silo-access.e2e.ts | 4 ++- 7 files changed, 47 insertions(+), 41 deletions(-) delete mode 100644 app/components/AccessNameCell.tsx create mode 100644 app/util/access.spec.tsx create mode 100644 app/util/access.ts diff --git a/app/components/AccessNameCell.tsx b/app/components/AccessNameCell.tsx deleted file mode 100644 index f3d0b920be..0000000000 --- a/app/components/AccessNameCell.tsx +++ /dev/null @@ -1,34 +0,0 @@ -/* - * This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, you can obtain one at https://mozilla.org/MPL/2.0/. - * - * Copyright Oxide Computer Company - */ -import type { CellContext } from '@tanstack/react-table' - -import type { IdentityType } from '@oxide/api' - -import { Badge } from '~/ui/lib/Badge' - -/** - * Display the user or group name. If the row is for a group, add a GROUP badge. - */ -export const AccessNameCell = < - RowData extends { name: string; identityType: IdentityType }, ->( - info: CellContext -) => { - const name = info.getValue() - const identityType = info.row.original.identityType - return ( - <> - {name} - {identityType === 'silo_group' ? ( - - Group - - ) : null} - - ) -} diff --git a/app/pages/SiloAccessPage.tsx b/app/pages/SiloAccessPage.tsx index 0ba0d4f014..f331a3d877 100644 --- a/app/pages/SiloAccessPage.tsx +++ b/app/pages/SiloAccessPage.tsx @@ -22,7 +22,6 @@ import { } from '@oxide/api' import { Access24Icon } from '@oxide/design-system/icons/react' -import { AccessNameCell } from '~/components/AccessNameCell' import { HL } from '~/components/HL' import { RoleBadgeCell } from '~/components/RoleBadgeCell' import { @@ -36,6 +35,7 @@ import { Button } from '~/ui/lib/Button' import { EmptyMessage } from '~/ui/lib/EmptyMessage' import { PageHeader, PageTitle } from '~/ui/lib/PageHeader' import { TableActions, TableEmptyBox } from '~/ui/lib/Table' +import { accessTypeLabel } from '~/util/access' import { groupBy, isTruthy } from '~/util/array' const EmptyState = ({ onClick }: { onClick: () => void }) => ( @@ -111,7 +111,11 @@ export function SiloAccessPage() { const columns = useMemo( () => [ - colHelper.accessor('name', { header: 'Name', cell: AccessNameCell }), + colHelper.accessor('name', { header: 'Name' }), + colHelper.accessor('identityType', { + header: 'Type', + cell: (props) => accessTypeLabel(props.getValue()), + }), colHelper.accessor('siloRole', { header: 'Silo role', cell: RoleBadgeCell, diff --git a/app/pages/project/access/ProjectAccessPage.tsx b/app/pages/project/access/ProjectAccessPage.tsx index 08d365b291..e81c245d40 100644 --- a/app/pages/project/access/ProjectAccessPage.tsx +++ b/app/pages/project/access/ProjectAccessPage.tsx @@ -24,7 +24,6 @@ import { } from '@oxide/api' import { Access24Icon } from '@oxide/design-system/icons/react' -import { AccessNameCell } from '~/components/AccessNameCell' import { HL } from '~/components/HL' import { RoleBadgeCell } from '~/components/RoleBadgeCell' import { @@ -39,6 +38,7 @@ import { Button } from '~/ui/lib/Button' import { EmptyMessage } from '~/ui/lib/EmptyMessage' import { PageHeader, PageTitle } from '~/ui/lib/PageHeader' import { TableActions, TableEmptyBox } from '~/ui/lib/Table' +import { accessTypeLabel } from '~/util/access' import { groupBy, isTruthy } from '~/util/array' const EmptyState = ({ onClick }: { onClick: () => void }) => ( @@ -127,7 +127,11 @@ export function ProjectAccessPage() { const columns = useMemo( () => [ - colHelper.accessor('name', { header: 'Name', cell: AccessNameCell }), + colHelper.accessor('name', { header: 'Name' }), + colHelper.accessor('identityType', { + header: 'Type', + cell: (props) => accessTypeLabel(props.getValue()), + }), colHelper.accessor('siloRole', { header: 'Silo role', cell: RoleBadgeCell, diff --git a/app/util/access.spec.tsx b/app/util/access.spec.tsx new file mode 100644 index 0000000000..2f0ed47884 --- /dev/null +++ b/app/util/access.spec.tsx @@ -0,0 +1,15 @@ +/* + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, you can obtain one at https://mozilla.org/MPL/2.0/. + * + * Copyright Oxide Computer Company + */ +import { expect, test } from 'vitest' + +import { accessTypeLabel } from './access' + +test('accessTypeLabel', () => { + expect(accessTypeLabel('silo_group')).toEqual('Group') + expect(accessTypeLabel('silo_user')).toEqual('User') +}) diff --git a/app/util/access.ts b/app/util/access.ts new file mode 100644 index 0000000000..0816af6136 --- /dev/null +++ b/app/util/access.ts @@ -0,0 +1,11 @@ +/* + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, you can obtain one at https://mozilla.org/MPL/2.0/. + * + * Copyright Oxide Computer Company + */ +import type { IdentityType } from '~/api' + +export const accessTypeLabel = (identityType: IdentityType) => + identityType === 'silo_group' ? 'Group' : 'User' diff --git a/test/e2e/project-access.e2e.ts b/test/e2e/project-access.e2e.ts index a6481688bc..cc177ced09 100644 --- a/test/e2e/project-access.e2e.ts +++ b/test/e2e/project-access.e2e.ts @@ -18,22 +18,26 @@ test('Click through project access page', async ({ page }) => { const table = page.locator('table') await expectRowVisible(table, { Name: 'Hannah Arendt', + Type: 'User', 'Silo role': 'admin', 'Project role': '', }) await expectRowVisible(table, { Name: 'Jacob Klein', + Type: 'User', 'Silo role': '', 'Project role': 'collaborator', }) await expectRowVisible(table, { // no space because expectRowVisible uses textContent, not accessible name - Name: 'real-estate-devsGroup', + Name: 'real-estate-devs', + Type: 'Group', 'Silo role': 'collaborator', }) await expectRowVisible(table, { // no space because expectRowVisible uses textContent, not accessible name - Name: 'kernel-devsGroup', + Name: 'kernel-devs', + Type: 'Group', 'Silo role': '', 'Project role': 'viewer', }) diff --git a/test/e2e/silo-access.e2e.ts b/test/e2e/silo-access.e2e.ts index aaf6142c59..ec45ede7b7 100644 --- a/test/e2e/silo-access.e2e.ts +++ b/test/e2e/silo-access.e2e.ts @@ -20,11 +20,13 @@ test('Click through silo access page', async ({ page }) => { await expectVisible(page, ['role=heading[name*="Access & IAM"]']) await expectRowVisible(table, { // no space because expectRowVisible uses textContent, not accessible name - Name: 'real-estate-devsGroup', + Name: 'real-estate-devs', + Type: 'Group', 'Silo role': 'collaborator', }) await expectRowVisible(table, { Name: 'Hannah Arendt', + Type: 'User', 'Silo role': 'admin', }) await expectNotVisible(page, [`role=cell[name="${user4.display_name}"]`]) From ae8d11b045e44402988d8e7f2d65e9e0c5ca2b43 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Wed, 13 Mar 2024 16:03:14 -0700 Subject: [PATCH 8/8] Simplify logic --- app/components/Sidebar.tsx | 7 +++---- app/util/links.spec.ts | 17 +---------------- app/util/links.ts | 6 ------ 3 files changed, 4 insertions(+), 26 deletions(-) diff --git a/app/components/Sidebar.tsx b/app/components/Sidebar.tsx index f222565d85..352c3497fd 100644 --- a/app/components/Sidebar.tsx +++ b/app/components/Sidebar.tsx @@ -13,7 +13,6 @@ import { Action16Icon, Document16Icon } from '@oxide/design-system/icons/react' import { openQuickActions } from '~/hooks' import { Button } from '~/ui/lib/Button' import { Truncate } from '~/ui/lib/Truncate' -import { isRootResourceLink } from '~/util/links' const linkStyles = 'flex h-7 items-center rounded px-2 text-sans-md hover:bg-hover svg:mr-2 svg:text-quinary text-secondary' @@ -90,8 +89,8 @@ export const NavLinkItem = (props: { end?: boolean disabled?: boolean }) => { - const location = useLocation() - const isLinkToRootResource = isRootResourceLink(props.to, location.pathname) + // If the current page's URL matches the create form for this NavLink resource, we want to highlight the NavLink in the sidebar. + const currentPathIsCreateForm = useLocation().pathname === `${props.to}-new` return (
  • cn(linkStyles, { 'text-accent !bg-accent-secondary hover:!bg-accent-secondary-hover svg:!text-accent-tertiary': - isActive || isLinkToRootResource, + isActive || currentPathIsCreateForm, 'pointer-events-none text-disabled': props.disabled, }) } diff --git a/app/util/links.spec.ts b/app/util/links.spec.ts index 56dcbd91b7..a59a4d2149 100644 --- a/app/util/links.spec.ts +++ b/app/util/links.spec.ts @@ -7,7 +7,7 @@ */ import { describe, expect, test } from 'vitest' -import { isRootResourceLink, links } from './links' +import { links } from './links' describe('check links are accessible', () => { for (const [key, url] of Object.entries(links)) { @@ -18,18 +18,3 @@ describe('check links are accessible', () => { }) } }) - -describe('check isRootResourceLink to evaluate Sidebar navigation links', () => { - test('should return true for resource creation pages', () => { - const navUrl = 'projects/mock-project/instances' - const locationPathname = 'projects/mock-project/instances-new' - const isLinkToRootResource = isRootResourceLink(navUrl, locationPathname) - expect(isLinkToRootResource).toEqual(true) - }) - test('should return false for non-resource creation pages', () => { - const navUrl = 'projects/mock-project/disks' - const locationPathname = 'projects/mock-project/instances-new' - const isLinkToRootResource = isRootResourceLink(navUrl, locationPathname) - expect(isLinkToRootResource).toEqual(false) - }) -}) diff --git a/app/util/links.ts b/app/util/links.ts index e7ea025140..4418c90a62 100644 --- a/app/util/links.ts +++ b/app/util/links.ts @@ -10,9 +10,3 @@ export const links: Record = { cloudInitExamples: 'https://cloudinit.readthedocs.io/en/latest/reference/examples.html', ipPoolsDocs: 'https://docs.oxide.computer/guides/operator/ip-pool-management', } - -// Resource creation pages (e.g. /instances-new) don't have a route that matches the top-level resource name, -// so `isActive` won't ever fire as true, and the Sidebar won't highlight the correct resource. -// Determine, for a given Sidebar URL, whether it's the matching top-level resource URL for the current page. -export const isRootResourceLink = (navUrl: string, locationPathname: string): boolean => - locationPathname === `${navUrl}-new`