Proof of concept: Take column logic out of QueryTable

[david-crespo]

Closes #2107
Supersedes #2109

In #2109 I had a big outstanding mystery:

Unsolved: why does unmount happen before the loader is complete? Page transition should only happen after loader is done.

To investigate, I tried to reproduce the #2107 problem on StackBlitz with a very minimal setup:

• https://stackblitz.com/edit/vitejs-vite-l4jbyr?file=src%2Fmain.tsx&terminal=dev
• https://stackblitz.com/edit/vitejs-vite-59zkej?file=src%2Fmain.tsx&terminal=dev

But it did not have the issue, so I had to go back to our code and ask myself what we're doing to cause it. The breakthrough came when I replaced the IP pools table with this and the problem went away.

<Link to={pb.ipPool({ pool: 'ip-pool-1' })}>ip-pool-1</Link>
<UtilizationCell pool="ip-pool-1" />

So most likely it was QueryTable causing the UtilizationCell to unmount and remount at a weird time. Then I remembered: QueryTable is full of shenanigans, it would totally do that. The culprit was my least favorite part of QueryTable:

console/app/table/QueryTable.tsx

Lines 132 to 167 in 44c646f

	const columns = useMemo(() => {
	const colHelper = createColumnHelper<Item>()
	const columns = React.Children.toArray(children).map((child) => {
	const column = { ...(child as ReactElement<QueryTableColumnProps<Item>>).props }
	// QueryTableColumnProps ensures `id` is passed in if and only if
	// `accessor` is not a string
	const id =
	'id' in column
	? column.id
	: typeof column.accessor === 'string'
	? column.accessor
	: undefined // should never happen because id is required if accessor is a function
	return colHelper.accessor(column.accessor, {
	id: id!, // undefined not really possible, and helper doesn't allow it
	header: typeof column.header === 'string' ? column.header : id,
	cell: (info: any) => {
	const Comp = column.cell || DefaultCell
	return <Comp value={info.getValue()} />
	},
	})
	})
	if (onSingleSelect) {
	columns.unshift(getSelectCol())
	} else if (onMultiSelect) {
	columns.unshift(getMultiSelectCol())
	}
	if (makeActions) {
	columns.push(getActionsCol(makeActions))
	}
	return columns
	}, [children, makeActions, onSingleSelect, onMultiSelect])

Turning QueryTable inside out and getting rid of all the any bits has been a long-term goal of mine, but it is pretty hard to untangle. What hadn't occurred to me until now is that we can pretty easily stop doing the columns part altogether, passing in a native React Table columns array as a prop instead of doing the <Column /> business.

So in this PR, I've introduced useQueryTable2, which is the same as useQueryTable but with the bad columns thing deleted.

I think we should convert all existing useQueryTable calls to useQueryTable2, then delete the original and rename useQueryTable2 back to useQueryTable. There are 21 useQueryTable calls (not including the IP pools one I changed here), and some will be pretty involved, so we probably don't want to do them all in one PR.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
console	✅ Ready (Inspect)	Visit Preview	Mar 29, 2024 3:56pm

[david-crespo]

The same test failed twice in a row in Safari, so that’s significant. Will take a look.

[david-crespo]

The test has now passed multiple times in a row in Safari in CI. Running it one more time to be sure. I wasn't able to repro the failure locally.

[charliepark]

Good sleuthing on this. No concerns I can come up with; stoked it's addressing the issue. We might extract some of the common columns from different tables into helper functions, though I also get it if the benefits there are minimal. Regardless, this is good.

[david-crespo]

Thanks for the quick review. Definitely with you on cleaner helper functions. I did a bit of that with defaultCell and makeLinkCell.

[david-crespo]

The test passed in Safari but now flaked in Firefox and in the same spot. Very interesting....