Repository navigation
fix: wait for the switched document, not a URL that never changes (#826) - #829
Merged
Merged
Conversation
Both specs that #826 reports red drive switchToWorkspace, and that helper waited with page.waitForURL on a pathname prefix of /console. The account-switch handler always answers with a 303 back to /console, so on the console the URL is identical before and after a switch, and Frame.waitForURL short-circuits to waitForLoadState whenever the current URL already matches the pattern. The wait therefore resolved immediately, every time, without observing the navigation it existed to wait for, and whatever the caller did next raced the in-flight POST. That is the whole of both failures: the reload in console-workspace-switch aborted mid navigation, and the goto in console-budgets landed on the budget page before the switch had taken effect, so the caps were still the ones the user owns and correctly editable. The product was never broken. Driven with a correctly awaited navigation on a stack built from main, the switch takes effect, survives a reload, and the destination workspace enforces its owner gate: the member sees disabled cap inputs, a disabled save button, and the read-only notice. Evidence, including the deployed box where the demo account has one workspace and so renders the static label branch with no interactive control at all, is in docs/proof/workspace-switch-race-826. Wait for the next load event instead. It is per document, so it cannot be satisfied by the document already open, and it fires after the replacement document is committed. Waiting on the redirect response is not enough: the bytes arrive before the browser swaps documents, and a goto issued in that window still fails with "interrupted by another navigation". Return early when the target workspace is already selected, because React does not dispatch onChange for an unchanged select value, so nothing submits and there would be no load event to wait for. Both specs also get a 120s budget. Each drives seven or eight full server-rendered navigations and the 30s default is a budget for a single page, which lets machine speed decide the result before the assertions do. Every assertion these specs rest on was broken on purpose and confirmed red: deleting the onChange handler in workspace-switcher.tsx fails both specs, setting readOnly to false on the budget page fails the member assertion while leaving the switch spec green, dropping the hive_account_id cookie fails the reload-persistence assertion with the pre-switch workspace id, and forcing the new early return to always fire fails both. No assertion was weakened and no product code is changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Owner
Author
Visual proofBoth captures are committed on this branch under 1. A real switch takes effect, survives a reload, and the destination workspace enforces its owner gateTaken against a stack booted from
Three correct behaviours in one frame:
This is the direct answer to the issue's worse alternative: a workspace member cannot edit that workspace's budget caps. 2. The deployed box, for the "is a real user affected right now" question
That account holds one workspace, so the sidebar renders a plain Neither image contains an address bar or any credential; the only identities visible are the demo fixture account and a seeded E2E fixture account. |
📝 WalkthroughWalkthroughThe workspace-switch helper now waits for the replacement document after a workspace change and returns immediately when the target workspace is already selected. Two E2E suites use 120-second timeouts. Bug evidence records persistence and role-based budget restrictions. ChangesWorkspace switch E2E flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant switchToWorkspace
participant PlaywrightPage
participant AccountSwitchNavigation
switchToWorkspace->>PlaywrightPage: register load listener
switchToWorkspace->>PlaywrightPage: select workspace option
PlaywrightPage->>AccountSwitchNavigation: submit account-switch request
AccountSwitchNavigation-->>PlaywrightPage: load replacement document
PlaywrightPage-->>switchToWorkspace: resolve load wait
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
sakibsadmanshajib
added a commit
that referenced
this pull request
Aug 10, 2026
…ly passes on retry (#838) Gate 4 of promoting `Web E2E (full stack)` to a required merge check: drive the measured 7.7 percent flake rate to zero by removing the cause, and make any future retry-pass impossible to miss. ## The two specs Identified from the artifact of CI run [31361681115](https://github.com/sakibsadmanshajib/hive/actions/runs/31361681115), by unzipping the report JSON embedded in the HTML report and reading each test's `outcome` and `results[]`. Not guessed from names. That run collected 34 tests, skipped 6, failed 2 and executed 26. Two of the 26 passed only on a retry, which is the 7.7 percent. Both are in `apps/web-console/tests/e2e/profile-completion.spec.ts`: | Spec | Test | Attempts | | --- | --- | --- | | `profile-completion.spec.ts` | `setup saves profile` | 3 | | `profile-completion.spec.ts` | `profile settings stay reachable while unverified` | 3 | ## Root cause **Category: test infrastructure. Neither is a product race.** I looked for one and did not find it; the browser behaved correctly in every attempt I examined, including the failing ones. Nine spec files copy-paste a `beforeEach` that reseeds the shared Supabase fixtures through a child process. Playwright charges `beforeEach` to the test's own timeout, and the reseed talks to a shared cloud project, so its wall time is set by conditions no spec controls. Step timings for that identical call in that one job: | Test | Attempt | `beforeEach hook` | | --- | --- | --- | | `setup saves profile` | 0 | 19894 ms | | `setup saves profile` | 1 | 19226 ms | | `setup saves profile` | 2 | 2618 ms | | `profile settings stay reachable while unverified` | 1 | **30248 ms** | Against a declared 30 second timeout, every spec in the file was running with a random slice of its budget. The second test's failing attempt spent the entire 30 seconds inside the hook and never opened a page, which is why its error reads `Test timeout of 30000ms exceeded while setting up "context"` and names no product surface at all. A second defect sat underneath. The reseed ran through `execFileSync`, which freezes the worker's event loop, and Playwright's timeout is an ordinary timer that cannot fire while the loop is blocked. `setup saves profile` attempt 1 was therefore reported as **passed at 30194 ms under a 30000 ms timeout**: a test that overran its deadline and went green anyway. That is a camouflage shape in its own right, so it is fixed here too rather than left as a footnote. ## The fix One shared helper, `apps/web-console/tests/e2e/support/fixture-reset.ts`, replacing the copy-pasted block in all nine specs (107 lines deleted). It: 1. runs the reseed as an **awaited async child**, so the worker's event loop keeps turning and Playwright's timeout accounting stays honest, 2. gives the reseed **its own explicit 120 second ceiling**, so a wedged seeder dies with its own message instead of surfacing as a UI timeout against whatever the browser happened to be doing, 3. adds the hook's **measured wall time back onto the deadline** via `testInfo.setTimeout`, so the product always gets exactly the budget the spec author declared. It composes, so a second reseeding hook in the same test adds its own elapsed time on top. Fixing it in the shared helper rather than only in `profile-completion.spec.ts` is deliberate: the other eight files carry the identical defect and are all in scope once this check is required. Nothing here raises `retries`, adds a blind `waitForTimeout`, loosens or deletes an assertion, or skips a spec. I also updated the long comment in `e2e-fixture-seed.mjs` that explicitly reasoned from "runs via execFileSync, fully synchronous". Its ordering guarantee never depended on the event loop being blocked, only on the `await` and on `workers: 1`, and the comment now says so and names the new revisit condition. ## Retries are now visible Today's job log ends with a `list` reporter tail that folds retry-passes into its pass count, so a run that needed two retries reads identically to a clean one. Finding these two meant downloading a 5 MB artifact and unzipping the HTML report's embedded JSON by hand. `flake-reporter.ts` now writes a table of every retry-pass into the GitHub job summary (and stdout locally), and fails the run when there is one at all. **The threshold is zero, and that is argued in the code.** This check is being promoted to required, and a retry-pass is precisely the failure mode that promotion has to survive: it blocks main at random and teaches everyone to press re-run until green, which is the same habit whether the rate is 8 percent or 4. Any non-zero percentage also silently loosens as the suite grows, since one flaky test scores better in a bigger denominator. Retries stay switched on, because they are what produce the second attempt, the trace and the video that identify the flake; what changes is that a run which needed them no longer reports success. Raising it is a one line diff a reviewer can see and argue with, which is why there is no environment variable for it. Not applied to the OWUI nightly config, deliberately: those specs drive real free tier provider latency, and a zero flake gate there would fail nightly for reasons unrelated to this check. ## Red proof Per `docs/TESTING-STANDARD.md`, everything added here was watched failing first. | Break | Result | | --- | --- | | `gateTripped = false` in `buildFlakeReport` | RED, 2 of 6 unit tests, both threshold cases | | `executed = entries.length` (skipped back in the denominator) | RED, 2 of 6 unit tests, the 7.7 percent case and the skipped-exclusion case | | Temporary spec that fails on attempt 0 and passes on attempt 1, run through the reporter | RED, `playwright` exit code **1**, summary named the spec and "Attempts 2" | | Same spec filtered to its always-passing test | GREEN, exit code **0**, summary read "No test needed a retry" | The unit tests cover the pure `buildFlakeReport`; the reporter's mapping from Playwright's `Suite` onto it is what the two end to end runs above prove, so neither half is only asserted against a hand copy of itself. ## Before and after, 10 runs each Both arms ran against the same booted control-plane and console, the same live Supabase project and the same seeder, one attempt per trial (`--retries=0`). Only the spec changed between arms, restored with `git stash`. **Natural seeder latency** (measured 9.6 s to 20.3 s on this box, no injection): | Test | Before | After | | --- | --- | --- | | `setup saves profile` | **0/10** | **10/10** | | `profile settings stay reachable while unverified` | 10/10 | 9/10 | The second test did not reproduce in that window: this box's seeder happened to stay fast enough for it. So I reran it with the hook latency pinned to what the failing CI attempt actually showed (about 30 s total), which is the condition that broke it in CI: | Test | Before | After | | --- | --- | --- | | `profile settings stay reachable while unverified`, CI hook latency | **0/10** | **9/10** | The one remaining failure in each after arm was an upstream Supabase problem, not the budget: a `Could not query the database for the schema cache` retry in one, and a sign-in exceeding its own 25 s timeout on a heavily loaded box in the other. Both are now reported as what they are, a seeder failure and a sign-in timeout, rather than as an unexplained UI timeout. That is the fix working as intended. The mechanism is also visible directly in the run output: the same spec on the same box reports `Test timeout of 30000ms exceeded` before and `Test timeout of 43103ms exceeded` after, the difference being the 13103 ms the hook actually took. ## Residual flake rate, and what it means for promoting this check **Roughly 10 percent per trial on one test. Not zero, and it should not be read as zero.** Across the two after arms, `profile settings stay reachable while unverified` failed once in each set of ten, so 1/10 and 1/10. Neither failure was the timeout budget this PR fixes: | Arm | Failure | Attribution | | --- | --- | --- | | natural latency | `accounts upsert failed: Could not query the database for the schema cache` | Supabase PostgREST, surfaced by the seeder | | CI hook latency | `page.waitForURL: Timeout 25000ms exceeded` in `signIn` | sign-in exceeded the helper's own 25 s cap on a loaded box | The attribution is sound in mechanism, both failures carry an explicit upstream error rather than an inferred one, but the evidence for each is a single occurrence and no artefact is linked, so treat the mechanism as established and the rate as a rough estimate rather than a measured constant. **The consequence matters for the promotion decision and should not be discovered afterwards.** With `retries: 2` and a threshold of zero, an upstream hiccup of this kind consumes a retry, and if it recurs on the retry the job goes red; if the retry rescues it, the run becomes a retry-pass, which this PR now deliberately fails. Either way a transient Supabase or sign-in stall becomes a red required check once `Web E2E (full stack)` is required. That is the intended behaviour, since the alternative is exactly the re-run-until-green habit this work exists to prevent, but it means promotion should account for a non-zero rate of reds that no code change in this repository can remove. Two things reduce it, neither in scope here: #837 makes the pooler less contended, which shortens the seeder and narrows the window; and the 25 second cap inside `signIn` is a hardcoded local timeout that does not scale with load, which is worth revisiting separately. ## Review follow-ups (second commit) **The gate had an escape hatch, now closed.** `npx playwright test --reporter=list` replaces the configured reporters outright. Under that flag the custom reporter never ran and the run exited 0 on a retry-pass, so one command line argument disabled the whole gate. `failOnFlakyTests: true` is now set alongside the reporter and survives the override. Both layers are kept on purpose: the built-in flag gives an exit code and nothing to act on, the reporter is what names the test and its attempt count. | Break | Result | | --- | --- | | retry-pass, `--reporter=list`, flag present | exit **1** | | retry-pass, `--reporter=list`, flag removed | exit **0**, and the reporter never ran at all | | clean run, `--reporter=list`, flag present | exit **0** | | retry-pass, configured reporters | exit **1**, summary table names one spec | | reseed helper driven down its failure path | logs elapsed and relays the child's error, product budget kept at 30000 | **`live-auth.ts` no longer blocks the event loop.** It is the sanctioned auth helper and its `reauthenticate` is built for mid-test use, so the first mid-test caller would have silently disabled every timeout in that file, which is precisely the defect this PR removes from the reseed. It has no callers yet, so the conversion touched nothing else. **`owui.setup.ts` keeps `execFileSync`, deliberately.** It runs once inside a setup project rather than mid-test, so the only deadline it can eat is its own, and `stdio: "inherit"` streams the installer's progress into the job log live where a buffered async child would withhold it until exit. The reasoning is now a comment in the file so the next reader does not have to rediscover it, with the condition that would change the answer. **Three corrections.** The flake table repeated the spec path in both columns, because `titlePath()` carries a leading empty root and slicing before filtering kept the file; it filters first now and the comment matches the code. `fixture-reset.ts` promised a "reseed exceeded" message it never emitted, so a timeout kill produces one instead of a bare signal. Hook elapsed time was logged only on failure, which is backwards for the one number that diagnoses this class, so it prints on every reseed alongside the preserved product budget. ## Scope added after review, third commit Flagged explicitly because it was not part of what was approved, and an earlier approval should not be stretched over it silently. An independent audit of the `Web E2E` failures on main traced them to this branch's fix, with a head to head control run on the same live Supabase three minutes apart: all three `auth-shell.spec.ts` tests green on first attempt at 0 percent flake here, against two of them failing on main at 36.3 s and 32.6 s under a 30000 ms timeout. A test overrunning its own declared timeout and being recorded at all is the event loop freeze this PR removes, so that is a direct confirmation of the mechanism from an independent run. That audit surfaced one more test of the same family, and this commit **deletes** it: `workspace switcher persists selected account` in `auth-shell.spec.ts`. Why it went rather than being repaired: - **Its core assertion cannot fail.** It called `switcher.selectOption(secondValue)` and then asserted `expect(select).toHaveValue(secondValue)` on the same element. `selectOption` sets that value client side, so the assertion was already true when it was reached, before any navigation. It read as a persistence check and was not one. That is camouflage shape 3 in `docs/TESTING-STANDARD.md`. - **It was a strictly weaker duplicate.** `console-workspace-switch.spec.ts`, `switching workspace takes effect and survives a reload`, switches by workspace name, calls `page.reload()`, and asserts the value the server rendered from the cookie the switch handler set, in both directions. **That is where the coverage lives, and it is unchanged by this PR.** It passed in 14.7 s in the very run where the deleted test failed, so deleting the weaker one loses nothing. - Two smaller faults came with it: it skipped itself whenever the account had fewer than two workspaces, so on a single workspace account it was silently absent, and it hand rolled the switch instead of calling the shared `tests/e2e/support/workspace-switch.ts`. The alternative was to rewrite it to call `switchToWorkspace`, reload, and assert the server rendered value. That would have made it a second copy of the test named above, so deletion was the smaller and honest change. A comment in `auth-shell.spec.ts` records what was removed, why, and which test retains the coverage, so nobody rediscovers the gap and writes the weak version again. `auth-shell.spec.ts` goes from three tests to two, confirmed with `--list`. Every import it had is still used by the invitation test that remains. ## Separate finding, not fixed here `profile-completion.spec.ts` contains a wait that never waits, the same shape as #826: ```ts await page.getByRole("button", { name: "Save billing details" }).click(); await page.waitForURL("**/console/settings/billing"); // already on this URL ``` Measured in the CI artifact, that step resolves in **0 ms**, and the following `toHaveValue("Acme Labs LLC")` then passes in 12 ms by reading the value the test itself typed into the unsubmitted input. `billing settings save partial business profile` therefore cannot distinguish a saved billing profile from a silently discarded one. It was not flaky and is not this PR's cause, so it is filed separately rather than changed here, because fixing the assertion honestly requires first establishing whether the save actually persists. ## Scope note for reviewers `console-budgets.spec.ts` and `console-workspace-switch.spec.ts` are also touched by #829. The overlap is only the `beforeEach` block at the top of each file, well away from the test bodies #829 edits, so whichever merges second should resolve trivially. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
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
1 participant
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.
Fixes #826.
Verdict, in one sentence
The product is not broken; the two specs were failing on their own shared helper, which waited for a URL that a same-URL redirect never changes and so never waited for the switch at all.
One root cause, not two separate defects.
console-budgets"disabled for a member" andconsole-workspace-switch"survives a reload" are the same defect seen from two angles: both callswitchToWorkspace, both then race the in-flight POST, and each fails at whatever it happened to do next. One helper is fixed and both go green, and no other caller of it exists.On the specific worry in #794: the switcher does survive a reload for a real user, so #786 did not replace a dead button with a control that silently does nothing. Screenshot proof is in the PR comment and committed under
docs/proof/workspace-switch-race-826/.Nothing was rotated to produce any of this, and no token, key, JWT, or connection string appears in the captures, the committed evidence, or this body.
Detail: the specs were wrong, the product is not
Issue #826 said the direction mattered, because the two possibilities call for
opposite fixes and one of them is an authorization hole on a money surface.
It is neither of those. The workspace switcher works, the switch survives a
reload, and a workspace member cannot edit that workspace's budget caps. The
shared spec helper both specs drive was never waiting for the switch it asked
for.
The defect
tests/e2e/support/workspace-switch.tswaited like this:/console/account-switchalways answers a switch with a 303 back to/console, so on the console the URL is identical before and after. AndFrame.waitForURLshort-circuits when the current URL already matches(
playwright-core/lib/client/frame.js:162):The page is already on
/consolewhen the helper is called, so that waitreturned immediately, every time, without ever observing the navigation it
existed to wait for. Whatever the caller did next then raced the in-flight
POST. That single cause produces both reported failures and every reported
variant of them:
page.reload: net::ERR_ABORTED; maybe frame was detached?console-workspace-switchtoHaveCountfailing withwaiting for navigation to finish...console-budgetstoBeDisabled()receivedenabled, becausepage.goto("/console/billing/budget")aborted the switch and the budget page still rendered the workspace the user ownsThe two failures share one root cause. Only these two specs call the
helper, so the fix is one function.
Proof the product is correct
Against a stack booted from
mainatf7d9293f(control-plane plus redisfrom
deploy/docker, a productionnext buildofapps/web-consolepointedat it), driving the same build with a correctly awaited navigation:
docs/proof/workspace-switch-race-826/01-member-workspace-budget-read-only.pngshows all of it in one frame: the sidebar reading
E2E Shared Workspace (current)after a reload and a further navigation, both cap inputs greyedout, Save budget disabled, and the notice "Only the workspace owner can edit
budget caps."
On the deployed box (
console-hive.scubed.co, signed in through the auditedlive-authhelper, nothing rotated) the demo account holds a singleworkspace, so the sidebar renders the static label branch and no interactive
control at all. No user is losing a workspace selection right now, and the
static label confirms the deployed build carries #786.
The fix
Wait for the next
loadevent instead of a URL. A load event is per document,so it cannot be satisfied by the document that is already open, and it fires
after the replacement document is committed. Waiting on the redirect's
response was tried first and is not enough: the bytes arrive before the
browser swaps documents, and a
page.goto()in that window still dies withinterrupted by another navigation.The helper also returns early when the target workspace is already selected.
React's change plugin does not dispatch
onChangefor an unchanged selectvalue, so no form submit and no navigation happen, and waiting for a load
event there would hang until the timeout.
The two specs are also given a 120s budget. Each drives seven or eight full
server-rendered navigations and the 30s default is a budget for a single page,
so on a slower runner the clock decides the result before the assertions do. A
timeout budget cannot turn a failing assertion green.
Every assertion proven able to fail
Four deliberate breaks, each rebuilt and re-run against the live stack. All
product-code breaks were reverted;
git diffagainstmaintouches noproduct code.
onChangehandler inworkspace-switcher.tsx, the exact regression #786 fixed and #794 said had no guardpage.waitForEvent: Timeout 25000ms exceeded while waiting for event "load"readOnly={!isOwner}toreadOnly={false}inapp/console/billing/budget/page.tsxconsole-budgetsred at line 115,toBeDisabledExpected: disabled / Received: enabled.console-workspace-switchstayed green, so the budget assertion is about the owner gate and not about the switch/console/account-switchstops setting thehive_account_idcookieconsole-workspace-switchred at line 92,toHaveValueExpected: 2cc1a48f-... / Received: 33ac1fa0-..., which is the pre-switch workspace. This is the reload-persistence guard #794 asked for, doing its jobGreen before and after on the same stack: 3 passed, including
budget page renderswhich was already green in CI.Not changed
No assertion was weakened, removed, or rewritten to match current behaviour.
The only spec-level change beyond the helper is the timeout budget. No product
code is touched.
Summary by CodeRabbit
Bug Fixes
Tests