Repository navigation
Fix affinity test flake due to missing column memoization - #2816
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
david-crespo
enabled auto-merge (squash)
May 2, 2025 20:03
david-crespo
added a commit
to oxidecomputer/omicron
that referenced
this pull request
May 7, 2025
oxidecomputer/console@99a1f00...46e244f * [46e244fd](oxidecomputer/console@46e244fd) oxidecomputer/console#2818 * [9811a9c7](oxidecomputer/console@9811a9c7) element=null on root redirect route to silence warnings * [b8e9f385](oxidecomputer/console@b8e9f385) oxidecomputer/console#2821 * [f097ec96](oxidecomputer/console@f097ec96) oxidecomputer/console#2819 * [585fe913](oxidecomputer/console@585fe913) oxidecomputer/console#2815 * [973d332a](oxidecomputer/console@973d332a) oxidecomputer/console#2817 * [ad61a081](oxidecomputer/console@ad61a081) oxidecomputer/console#2816 * [239e34b9](oxidecomputer/console@239e34b9) oxidecomputer/console#2806 * [376f172f](oxidecomputer/console@376f172f) oxidecomputer/console#2805 * [5c7c8b5b](oxidecomputer/console@5c7c8b5b) oxidecomputer/console#2746 * [1d8c3b76](oxidecomputer/console@1d8c3b76) bump playwright, use more cores to run e2es locally * [f493cb35](oxidecomputer/console@f493cb35) oxidecomputer/console#2772 * [819b99a6](oxidecomputer/console@819b99a6) chore: type error after RR 7.5. this is why we make PRs * [c1134cff](oxidecomputer/console@c1134cff) chore: react-router 7.5.0 * [c1f0d78b](oxidecomputer/console@c1f0d78b) chore: vite 6.3.0 + vitest 3.1.1 * [79c6bc53](oxidecomputer/console@79c6bc53) oxidecomputer/console#2803 * [f454f5ee](oxidecomputer/console@f454f5ee) oxidecomputer/console#2804 * [ccfa5a58](oxidecomputer/console@ccfa5a58) fix deploy-dogfood script when used with a tag * [6196fa48](oxidecomputer/console@6196fa48) invalidate anti-affinity group member lists on instance action success
david-crespo
added a commit
to oxidecomputer/omicron
that referenced
this pull request
May 7, 2025
oxidecomputer/console@99a1f00...46e244f * [46e244fd](oxidecomputer/console@46e244fd) oxidecomputer/console#2818 * [9811a9c7](oxidecomputer/console@9811a9c7) element=null on root redirect route to silence warnings * [b8e9f385](oxidecomputer/console@b8e9f385) oxidecomputer/console#2821 * [f097ec96](oxidecomputer/console@f097ec96) oxidecomputer/console#2819 * [585fe913](oxidecomputer/console@585fe913) oxidecomputer/console#2815 * [973d332a](oxidecomputer/console@973d332a) oxidecomputer/console#2817 * [ad61a081](oxidecomputer/console@ad61a081) oxidecomputer/console#2816 * [239e34b9](oxidecomputer/console@239e34b9) oxidecomputer/console#2806 * [376f172f](oxidecomputer/console@376f172f) oxidecomputer/console#2805 * [5c7c8b5b](oxidecomputer/console@5c7c8b5b) oxidecomputer/console#2746 * [1d8c3b76](oxidecomputer/console@1d8c3b76) bump playwright, use more cores to run e2es locally * [f493cb35](oxidecomputer/console@f493cb35) oxidecomputer/console#2772 * [819b99a6](oxidecomputer/console@819b99a6) chore: type error after RR 7.5. this is why we make PRs * [c1134cff](oxidecomputer/console@c1134cff) chore: react-router 7.5.0 * [c1f0d78b](oxidecomputer/console@c1f0d78b) chore: vite 6.3.0 + vitest 3.1.1 * [79c6bc53](oxidecomputer/console@79c6bc53) oxidecomputer/console#2803 * [f454f5ee](oxidecomputer/console@f454f5ee) oxidecomputer/console#2804 * [ccfa5a58](oxidecomputer/console@ccfa5a58) fix deploy-dogfood script when used with a tag * [6196fa48](oxidecomputer/console@6196fa48) invalidate anti-affinity group member lists on instance action success
david-crespo
added a commit
that referenced
this pull request
Sep 3, 2026
…ty (#3364) The Firefox run on the audit log merge to main failed `Silo IP pools` on all three attempts, each time in `clickRowAction` on the menu item, with Chrome and Safari passing: https://github.com/oxidecomputer/console/actions/runs/33767277998/attempts/1 I could not repro locally with the code as-is, but I can make it happen by adding a delay in the mock API (see below). `SiloIpPoolsTab` fetches the silo's pools twice: the paginated list that the table renders, and a second `useQuery` for all of them, which the "Make default" confirm modal uses to find the existing default for the same IP version and type so it can warn that it will be replaced. `makeActions` depends on that second result. The sequence in the test: 1. The test unlinks a pool, and the mutation's `onSuccess` invalidates both the paginated list and the all-pools list. 2. The test waits for the unlinked row to disappear, which happens as soon as the paginated refetch lands, and then opens the next row's menu. 3. If the all-pools refetch has already landed by then, nothing happens. If it lands after the menu is open, it gives `makeActions` a new identity. 4. `useColsWithActions` builds a new actions column with a new inline `cell` closure. 5. [`flexRender`](https://github.com/TanStack/table/blob/ab2819c/packages/react-table/src/index.tsx#L18-L27) renders a function `cell` as a component, so a new function is a new element type and React remounts the whole cell. 6. The open menu is unmounted along with the cell and comes back closed, so the menu item Playwright was about to click is gone. To confirm this, I added a 150ms delay to the mock all-pools response so it lands right around the time the test opens the menu. On main, the e2e test fails 5 out of 42 runs on Firefox with exactly the CI error, detached menu item and all, at the same line. With the fix and the same delay, it passes 24 out of 24. A longer delay like 1.5s makes the test pass again on main because Playwright opens the menu and clicks before the refetch lands, which is also why the test has been fine locally. With the 1.5s delay and a throwaway e2e test that holds the menu open across it, the menu closed before the fix and stayed open after. The fix is to make the more actions button cell a single module-level `ActionsCell` component and pass `makeActions` and `copyIdLabel` through column `meta` instead of closing over them in a dynamic cell definition. The element type is then stable no matter how often the callback or the columns array changes identity, so it fixes every table using `getActionsCol` without touching any call sites. The browser spec opens a row menu, rerenders with a new `makeActions`, and asserts the menu is still open. It failed in all three browsers before the fix. This is the same mechanism behind the polling-closes-the-menu problems in #2112, #2460, and #2816. Each of those was fixed by memoizing `makeActions` or the columns array carefully enough to keep the closure identity stable. With a stable cell component, that memoization is only a render-count optimization, so I softened the doc comments on `useColsWithActions`. The `instanceState` hack on NIC rows from #2460 could be unwound now, but I've left that for a followup.
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Charlie and I spent some time on this after noticing pretty frequent failure locally — guess it was just a coincidence that it worked in recent PRs and main commits. We were not memoizing the columns of the affinity groups table on the instance settings tab, so any render (caused, e.g., by instance state polling) would reset an open row actions menu. Related to #2618 and #1091.