Firewall rules create/edit modal

[david-crespo]

Two ways to see it:

1. https://console-git-firewall-rule-crud.internal.oxide.computer/orgs/maze-war/projects/prod-online/vpcs/vpc1?tab=firewall-rules, click New rule
2. Run yarn start:msw and go to http://localhost:4000/orgs/maze-war/projects/mock-project/vpcs/mock-vpc?tab=firewall-rules and click New rule

Limitations

• Doing almost no input validation — only checking that priority is a number between 0 and 65535

Notes

I'm trying out Yup, a schema validation that Formik has a pretty tight built-in integration with. It adds 30kb to our minified vendor bundle. It's pretty cool, though it seems less powerful than Zod, which is really amazing. Zod would add 50kb instead of 30, but it's quite a bit better — does parsing as well as validation. As you can see here in zod-formik-adapter, turning a Zod schema into something that Formik accepts in the validationSchema prop takes less than 40 lines of code.

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/oxidecomputer/console-ui-storybook/AdDfPE3gNhvK1os3tqhgc3VJCpj4
✅ Preview: https://console-ui-storybook-git-firewall-rule-crud-oxidecomputer.vercel.app

[github-actions]

Preview will be deployed at https://console-git-firewall-rule-crud.internal.oxide.computer

[david-crespo]

appearance-textfield hides the up/down buttons

[zephraph]

Ah, yeah, I definitely don't miss this 😅. Looking forward to figuring out how we can iterate on the QueryTable to preserve our typesafety and hopefully maintain a less verbose API.

[david-crespo]

Honestly I don't mind it. There's a memoization issue in the QueryTable version — the columns are currently memoized with a dep array of [children] but I don't think that really works because children is not referentially stable, i.e., it gets remade on every render, which triggers unneeded re-renders in the table.

[zephraph]

Indeed. That's the biggest challenge with JSX APIs is they're ultimately kind of a nightmare for referential integrity. That said imo they greatly improve the legibility of the construct. At the minimum we'd want type safety for this which I could be content with, but broadly speaking I don't really like APIs that are verbose objects or arrays. Sometimes that's just the price of dealing with JavaScript though, so I can deal if that's what we have to do.

[david-crespo]

v8 gets you typechecking on the column definition with this kind of janky API:

const table = createTable().RowType<Disk>()

const cols = table.createColumns([
  table.createColumn('name', {
    header: 'Name',
    cell: ({ value }) => <div>{value}</div>,
  }),
  table.createColumn((d) => d.state.state, {
    id: 'status',
    header: 'Status',
    cell: ({ value }) => <DiskStatusBadge status={value} />,
  }),
])

https://github.com/oxidecomputer/console/compare/react-table-v8-test

[zephraph]

You know what that reminds me of? react.createElement. Just saying 😄

[zephraph]

If new keys are added to the VpcFiewallRule will this fail type checks?

[david-crespo]

Well, now that it's a pick this is more about accidentally pulling in keys that don't belong. So for example, if we decided a field wasn't editable anymore, like we remove description from the VpcFirewallRuleUpdate, this will fail because description is still on there and it's not supposed to be.

However, as I test this, it's not behaving as I expect in other ways. For example, if I remove 'name' from the list, it should fail because name is required on VpcFirewallRuleUpdate, and it's not there. But it does not fail. Moving the NoExtraKeys to the return type makes it fail in the expected way, so I'll make that change. Guess I have something to learn about as.

export const firewallRuleGetToPut = (
  rule: VpcFirewallRule
): NoExtraKeys<VpcFirewallRuleUpdate, VpcFirewallRule> =>
  pick(
    rule,
    'name',
    'action',
    'description',
    'direction',
    'filters',
    'priority',
    'status',
    'targets'
  )

This will probably become moot once we make our PUTs spec-compliant, which will mean they have the same shape as the GET response.

[zephraph]

When running this with msw I'm hitting an error here where values.targets is undefined after trying to edit the first item in the rules list. When logging value at the top of the form it shows allow-icmp twice before erroring

[zephraph]

This is some solid work, thanks for taking it on. There's still a lot to follow up with here, but you've got TODOs on basically all of it, ha. Looking forward to the follow up conversation on the QueryTable.