Repository navigation
Wrap all the makeActions functions in useCallback - #2112
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
david-crespo
force-pushed
the
use-callbacks-everywhere
branch
from
March 29, 2024 21:16
b830b14 to
4b65ae5
Compare
david-crespo
enabled auto-merge (squash)
March 29, 2024 21:17
Collaborator
Author
|
Still flaking. It’s probably time to really figure this out, or if that takes too long, just change the test. |
david-crespo
added a commit
to oxidecomputer/omicron
that referenced
this pull request
Apr 3, 2024
oxidecomputer/console@156c082...b22ca1d * [b22ca1dc](oxidecomputer/console@b22ca1dc) add loop comment to scp-assets * [99173b92](oxidecomputer/console@99173b92) bump omicron script: automatically run gh run watch when assets aren't ready * [2cfc8ee7](oxidecomputer/console@2cfc8ee7) oxidecomputer/console#2076 * [11411bb8](oxidecomputer/console@11411bb8) oxidecomputer/console#2121 * [1f8b25d7](oxidecomputer/console@1f8b25d7) oxidecomputer/console#2119 * [95f2e49e](oxidecomputer/console@95f2e49e) oxidecomputer/console#2108 * [8e3a2005](oxidecomputer/console@8e3a2005) oxidecomputer/console#2116 * [bf592a31](oxidecomputer/console@bf592a31) oxidecomputer/console#2105 * [b63c81ea](oxidecomputer/console@b63c81ea) oxidecomputer/console#2115 * [d5d70bd7](oxidecomputer/console@d5d70bd7) oxidecomputer/console#2113 * [1954709e](oxidecomputer/console@1954709e) oxidecomputer/console#2112 * [4db8d830](oxidecomputer/console@4db8d830) oxidecomputer/console#2111 * [9485ca23](oxidecomputer/console@9485ca23) Revert "Revert "Change all uses of RHF `<Controller>` to `useController` (oxidecomputer/console#2102)""
david-crespo
added a commit
to oxidecomputer/omicron
that referenced
this pull request
Apr 3, 2024
oxidecomputer/console@156c082...b22ca1d * [b22ca1dc](oxidecomputer/console@b22ca1dc) add loop comment to scp-assets * [99173b92](oxidecomputer/console@99173b92) bump omicron script: automatically run gh run watch when assets aren't ready * [2cfc8ee7](oxidecomputer/console@2cfc8ee7) oxidecomputer/console#2076 * [11411bb8](oxidecomputer/console@11411bb8) oxidecomputer/console#2121 * [1f8b25d7](oxidecomputer/console@1f8b25d7) oxidecomputer/console#2119 * [95f2e49e](oxidecomputer/console@95f2e49e) oxidecomputer/console#2108 * [8e3a2005](oxidecomputer/console@8e3a2005) oxidecomputer/console#2116 * [bf592a31](oxidecomputer/console@bf592a31) oxidecomputer/console#2105 * [b63c81ea](oxidecomputer/console@b63c81ea) oxidecomputer/console#2115 * [d5d70bd7](oxidecomputer/console@d5d70bd7) oxidecomputer/console#2113 * [1954709e](oxidecomputer/console@1954709e) oxidecomputer/console#2112 * [4db8d830](oxidecomputer/console@4db8d830) oxidecomputer/console#2111 * [9485ca23](oxidecomputer/console@9485ca23) Revert "Revert "Change all uses of RHF `<Controller>` to `useController` (oxidecomputer/console#2102)""
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.
This is a very noisy but empty change, so we're pulling it out, so the rest of our
useQueryTableconversions will not be as noisy as #2111.