Repository navigation
test(sandbox-panel): wait for the loaded panel, not for the load to start - #6064
Merged
senamakel merged 1 commit intoSep 12, 2026
Merged
Conversation
…tart `SandboxSettingsPanel.validation.test.tsx` fails intermittently on CI with "Unable to find an element with the display value: 512" and a DOM dump still showing the loading paragraph. renderLoaded's readiness signal is `waitFor(() => expect(mockGet) .toHaveBeenCalled())`. The panel calls openhumanGetSandboxSettings synchronously inside its mount effect, so that assertion already holds on waitFor's first tick; isLoading only flips in the promise's finally block, one microtask and a re-render later. The synchronous getBy* accessors that follow can therefore query a DOM with no fields in it. The file already documents this exact trap in flushPersist's docstring -- "mockGet fires on mount, so the condition is already true and waitFor resolves on its first tick" -- but the same non-signal was left in place as the readiness gate. The sibling SandboxSettingsPanel.test.tsx does not trip on it because it reaches for the loaded DOM with findBy*, which retries. Awaiting the backend combobox instead: it is rendered only by the loaded branch, and by every override these tests pass, including the null-limits case. Verified by making the load land on a macrotask instead of a microtask, which is what a loaded runner does. Against origin/main that fails with the CI error and the same loading paragraph; with this change the file passes 14/14. It also passes 14/14 unmodified, and tsc --noEmit and prettier --check are clean.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe validation test now waits for the backend combobox to render before it queries fields, replacing an earlier wait that could complete while the panel remained in its loading state. ChangesSandbox settings validation
Estimated code review effort: 2 (Simple) | ~5 minutes Merge Risk: ⚪ Minimal · up to Sandbox settings validation tests now wait for the loaded panel before querying fields, removing an intermittent loading-state race without changing product behavior. No current merge-blocking risk remains. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
tinysweeper
Bot
added
the
priority: p3
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.
SandboxSettingsPanel.validation.test.tsxfails intermittently on CI:…and the DOM dump printed with it still shows the loading paragraph:
Why
renderLoadedtakes its readiness signal from the wrong event:SandboxSettingsPanelcallsopenhumanGetSandboxSettings()synchronously inside its mount effect (SandboxSettingsPanel.tsx:55), so that assertion is already true whenwaitForfirst checks it.isLoadingonly flips in thefinally(:69) — one microtask and a re-render later.renderLoadedtherefore returns while the panel is still on its loading branch, and the synchronousgetBy*accessors below it (memoryInput,cpuInput,imageInput) query a DOM that has no fields in it yet.Usually the tick
waitForcosts is enough to cover the microtask. On a loaded runner with coverage instrumentation it is not.Two things worth noting:
flushPersist's docstring: "await waitFor(() => expect(mockGet).toHaveBeenCalled())is NOT a readiness signal here:mockGetfires on mount, so the condition is already true andwaitForresolves on its first tick." It was applied to the negative assertions but the same non-signal was left as the readiness gate.SandboxSettingsPanel.test.tsxdoes not trip on it — it reaches for the loaded DOM withfindByRole/findByText, which retry. Only this file uses synchronous accessors.The change
Await the backend combobox. It is rendered only by the loaded branch, and by every override these tests pass — including the
docker_memory_limit_mb: null, docker_cpu_limit: nullcase, where waiting on a specific field value would not work.Verification
The failure is timing-dependent, so I made it deterministic instead of running it until it flaked: have the mock resolve on a macrotask rather than a microtask, which is what a contended runner does to it.
origin/mainUnable to find…+Loading…in the DOMAlso clean in
app/:npx tsc --noEmit,npx prettier --check.No production code is touched, and no assertion is weakened — the tests still query the same elements and make the same claims; they just wait for the panel to exist first.
Summary by CodeRabbit