ListPlusCell for firewall rule filters (#2163)

* configurable numInCell on ListPlusCell, use for firewall rule filters

* use it on targets too, add mock data for that

* get clever about numInCell

* test overflow a bit more

Author: david-crespo

app/components/ListPlusCell.tsx

@@ -13,22 +13,30 @@ import { Tooltip } from '~/ui/lib/Tooltip'
 type ListPlusCellProps = {
   tooltipTitle: string
   children: React.ReactNode
+  /** The number of items to show in the cell vs. in the popup */
+  numInCell?: number
 }
 
 /**
  * Gives a count with a tooltip that expands to show details when the user hovers over it
  */
-export const ListPlusCell = ({ tooltipTitle, children }: ListPlusCellProps) => {
-  const [first, ...rest] = React.Children.toArray(children)
+export const ListPlusCell = ({
+  tooltipTitle,
+  children,
+  numInCell = 1,
+}: ListPlusCellProps) => {
+  const array = React.Children.toArray(children)
+  const inCell = array.slice(0, numInCell)
+  const rest = array.slice(numInCell)
   const content = (
     <div>
       <div className="mb-2">{tooltipTitle}</div>
-      {...rest}
+      <div className="flex flex-col items-start gap-2">{...rest}</div>
     </div>
   )
   return (
     <div className="flex items-baseline gap-2">
-      {first}
+      {inCell}
       {rest.length > 0 && (
         <Tooltip content={content} placement="bottom">
           <div className="text-mono-sm">+{rest.length}</div>

app/pages/project/vpcs/VpcPage/tabs/VpcFirewallRulesTab.tsx

@@ -15,17 +15,18 @@ import {
   type VpcFirewallRule,
 } from '@oxide/api'
 
+import { ListPlusCell } from '~/components/ListPlusCell'
 import { CreateFirewallRuleForm } from '~/forms/firewall-rules-create'
 import { EditFirewallRuleForm } from '~/forms/firewall-rules-edit'
 import { useVpcSelector } from '~/hooks'
 import { confirmDelete } from '~/stores/confirm-delete'
 import { EnabledCell } from '~/table/cells/EnabledCell'
-import { FirewallFilterCell } from '~/table/cells/FirewallFilterCell'
 import { ButtonCell } from '~/table/cells/LinkCell'
 import { TypeValueCell } from '~/table/cells/TypeValueCell'
 import { getActionsCol } from '~/table/columns/action-col'
 import { Columns } from '~/table/columns/common'
 import { Table } from '~/table/Table'
+import { Badge } from '~/ui/lib/Badge'
 import { CreateButton } from '~/ui/lib/CreateButton'
 import { EmptyMessage } from '~/ui/lib/EmptyMessage'
 import { TableEmptyBox } from '~/ui/lib/Table'
@@ -50,17 +51,41 @@ const staticColumns = [
   }),
   colHelper.accessor('targets', {
     header: 'Targets',
-    cell: (info) => (
-      <div>
-        {info.getValue().map(({ type, value }) => (
-          <TypeValueCell key={type + '|' + value} type={type} value={value} />
-        ))}
-      </div>
-    ),
+    cell: (info) => {
+      const targets = info.getValue()
+      const children = targets.map(({ type, value }) => (
+        <TypeValueCell key={type + '|' + value} type={type} value={value} />
+      ))
+      // if there's going to be overflow anyway, might as well make the cell narrow
+      const numInCell = children.length <= 2 ? 2 : 1
+      return (
+        <ListPlusCell numInCell={numInCell} tooltipTitle="Other targets">
+          {info.getValue().map(({ type, value }) => (
+            <TypeValueCell key={type + '|' + value} type={type} value={value} />
+          ))}
+        </ListPlusCell>
+      )
+    },
   }),
   colHelper.accessor('filters', {
     header: 'Filters',
-    cell: (info) => <FirewallFilterCell {...info.getValue()} />,
+    cell: (info) => {
+      const { hosts, ports, protocols } = info.getValue()
+      const children = [
+        ...(hosts || []).map((tv, i) => <TypeValueCell key={`${tv}-${i}`} {...tv} />),
+        ...(protocols || []).map((p, i) => <Badge key={`${p}-${i}`}>{p}</Badge>),
+        ...(ports || []).map((p, i) => (
+          <TypeValueCell key={`port-${p}-${i}`} type="Port" value={p} />
+        )),
+      ]
+      // if there's going to be overflow anyway, might as well make the cell narrow
+      const numInCell = children.length <= 2 ? 2 : 1
+      return (
+        <ListPlusCell numInCell={numInCell} tooltipTitle="Other filters">
+          {children}
+        </ListPlusCell>
+      )
+    },
   }),
   colHelper.accessor('status', {
     header: 'Status',
@@ -149,7 +174,7 @@ export const VpcFirewallRulesTab = () => {
           />
         )}
       </div>
-      {rules.length > 0 ? <Table table={table} rowHeight="large" /> : emptyState}
+      {rules.length > 0 ? <Table table={table} /> : emptyState}
     </>
   )
 }

app/table/cells/FirewallFilterCell.tsx

@@ -286,8 +286,8 @@ const initDb = {
   snapshots: [...mock.snapshots],
   sshKeys: [...mock.sshKeys],
   users: [...mock.users],
-  vpcFirewallRules: [...mock.defaultFirewallRules],
-  vpcs: [mock.vpc],
+  vpcFirewallRules: [...mock.firewallRules],
+  vpcs: [...mock.vpcs],
   vpcSubnets: [mock.vpcSubnet],
 }
 

mock-api/msw/db.ts

@@ -9,7 +9,7 @@
 import type { Vpc, VpcFirewallRule, VpcSubnet } from '@oxide/api'
 
 import type { Json } from './json-type'
-import { project } from './project'
+import { project, project2 } from './project'
 
 const time_created = new Date(2021, 0, 1).toISOString()
 const time_modified = new Date(2021, 0, 2).toISOString()
@@ -28,6 +28,20 @@ export const vpc: Json<Vpc> = {
   time_modified,
 }
 
+export const vpc2: Json<Vpc> = {
+  id: 'e54078df-fe72-4673-b36c-a362e3b4e38b',
+  name: 'mock-vpc-2',
+  description: 'a fake vpc',
+  dns_name: 'mock-vpc-2',
+  project_id: project2.id,
+  system_router_id: systemRouterId,
+  ipv6_prefix: 'fdf6:1818:b6e2::/48',
+  time_created,
+  time_modified,
+}
+
+export const vpcs: Json<Vpc[]> = [vpc, vpc2]
+
 export const vpcSubnet: Json<VpcSubnet> = {
   // this is supposed to be flattened into the top level. will fix in API
   id: 'd12bf934-d2bf-40e9-8596-bb42a7793749',
@@ -49,7 +63,7 @@ export const vpcSubnet2: Json<VpcSubnet> = {
   ipv4_block: '10.1.1.2/24',
 }
 
-export const defaultFirewallRules: Json<VpcFirewallRule[]> = [
+export const firewallRules: Json<VpcFirewallRule[]> = [
   {
     id: 'b74aeea8-1201-4efd-b6ec-011f10a0b176',
     name: 'allow-internal-inbound',
@@ -117,4 +131,46 @@ export const defaultFirewallRules: Json<VpcFirewallRule[]> = [
     time_modified,
     vpc_id: vpc.id,
   },
+  // second mock VPC in other project, meant to test display with lots  of
+  // targets and filters
+  {
+    id: '097c849e-68c8-43f7-9ceb-b1855c51f178',
+    name: 'lots-of-filters',
+    status: 'enabled',
+    direction: 'inbound',
+    targets: [{ type: 'vpc', value: 'default' }],
+    description: 'we just want to test with lots of filters',
+    filters: {
+      ports: ['3389', '45-89'],
+      protocols: ['TCP'],
+      hosts: [
+        { type: 'instance', value: 'hello-friend' },
+        { type: 'subnet', value: 'my-subnet' },
+        { type: 'ip', value: '148.38.89.5' },
+      ],
+    },
+    action: 'allow',
+    priority: 65534,
+    time_created,
+    time_modified,
+    vpc_id: vpc2.id,
+  },
+  {
+    id: '097c849e-68c8-43f7-9ceb-b1855c51f178',
+    name: 'lots-of-targets',
+    status: 'enabled',
+    direction: 'inbound',
+    targets: [
+      { type: 'instance', value: 'my-inst' },
+      { type: 'ip', value: '125.34.25.2' },
+      { type: 'subnet', value: 'subsubsub' },
+    ],
+    description: 'we just want to test with lots of targets',
+    filters: { ports: ['80'] },
+    action: 'allow',
+    priority: 65534,
+    time_created,
+    time_modified,
+    vpc_id: vpc2.id,
+  },
 ]

mock-api/vpc.ts

@@ -90,15 +90,52 @@ test('can create firewall rule', async ({ page }) => {
     Name: 'my-new-rule',
     Priority: '5',
     Targets: 'ip192.168.0.1',
-    Filters: 'instancehost-filter-instanceUDP123-456',
+    Filters: 'instancehost-filter-instance+2', // UDP and port filters in plus popup
   })
 
+  // scroll table sideways past the filters cell
+  await page.getByText('Enabled').first().scrollIntoViewIfNeeded()
+
+  await page.getByText('+2').hover()
+  const tooltip = page.getByRole('tooltip', { name: 'Other filters UDP Port 123-' })
+  await expect(tooltip).toBeVisible()
+
   await expect(rows).toHaveCount(5)
   for (const name of defaultRules) {
     await expect(page.locator(`text="${name}"`)).toBeVisible()
   }
 })
 
+test('firewall rule targets and filters overflow', async ({ page }) => {
+  await page.goto('/projects/other-project/vpcs/mock-vpc-2')
+
+  await expect(
+    page.getByRole('cell', { name: 'instance my-inst +2', exact: true })
+  ).toBeVisible()
+
+  await page.getByText('+2').hover()
+  await expect(
+    page.getByRole('tooltip', {
+      name: 'Other targets ip 125.34.25.2 subnet subsubsub',
+      exact: true,
+    })
+  ).toBeVisible()
+
+  await expect(
+    page.getByRole('cell', { name: 'instance hello-friend +5', exact: true })
+  ).toBeVisible()
+
+  // scroll table sideways past the filters cell
+  await page.getByText('Enabled').first().scrollIntoViewIfNeeded()
+
+  await page.getByText('+5').hover()
+  const tooltip = page.getByRole('tooltip', {
+    name: 'Other filters subnet my-subnet ip 148.38.89.5 TCP Port 3389 Port 45-89',
+    exact: true,
+  })
+  await expect(tooltip).toBeVisible()
+})
+
 test('firewall rule form targets table', async ({ page }) => {
   await page.goto('/projects/mock-project/vpcs/mock-vpc')
   await page.getByRole('tab', { name: 'Firewall Rules' }).click()