[codex] stabilize onboarding e2e persistence wait - #934
Conversation
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
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. |
|
/test |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: e4b7b58
Important Issues (1)
- [pr-review-toolkit/native-codex]
tests/e2e/conference-room-typing-intro.spec.ts:83- The new 45-second assertion does not reliably provide a 45-second persistence allowance because this spec inherits the config's 60-second timeout for the entire test. Time spent loading onboarding and completing the earlier steps consumes that same budget, and the hire, review, dashboard, and API assertions after this wait still need to run. Under the slow CI condition this PR targets, Playwright can therefore hit the enclosing timeout before this assertion reaches its new limit, or immediately afterward before validating the persisted result.- Add an explicit per-test timeout with enough headroom for the full flow, as
nux-phase4-screenshots.spec.tsalready does with 180 seconds, while retaining the scoped 45-second assertion timeout.
- Add an explicit per-test timeout with enough headroom for the full flow, as
Strengths
- The larger assertion timeout is limited to the persistence-backed transition rather than weakening every wizard assertion.
- The visual QA spec already reserves a 180-second test budget, and the current e2e CI job passes.
Recommended Action
- Address the Important issue this cycle by increasing the enclosing timeout for the conference-room onboarding spec.
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
Addressed Ally's Important finding by giving the enclosing conference-room onboarding Playwright test a 180s budget, matching the existing NUX visual QA headroom, while keeping the scoped 45s persistence assertion. Verification:
/test |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 8a6c5ae
Prior Findings Dispositioned (1)
- prior:e4b7b58 important 1 — fixed —
tests/e2e/conference-room-typing-intro.spec.ts:17— The spec now sets a 180-second enclosing timeout, leaving sufficient full-flow headroom around the scoped 45-second persistence wait.
Looks good. The persistence allowance is narrowly scoped, and both affected full-flow specs now have explicit 180-second test budgets.
Strengths
- The change extends only the persistence-backed wizard transition rather than weakening all assertions.
- The named timeout documents why this transition needs a larger CI allowance.
Recommended Action
- Merge after required CI checks pass.
allyblockcast
left a comment
allyblockcast
left a comment
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 8a6c5ae
Prior Findings Dispositioned (1)
- prior:e4b7b58 important 1 — fixed —
tests/e2e/conference-room-typing-intro.spec.ts:17— The spec now sets a 180-second enclosing timeout, leaving sufficient full-flow headroom around the scoped 45-second persistence wait.
Looks good. The persistence allowance is narrowly scoped, and both affected full-flow specs now have explicit 180-second test budgets.
Strengths
- The change extends only the persistence-backed wizard transition rather than weakening all assertions.
- The named timeout documents why this transition needs a larger CI allowance.
Recommended Action
- Merge after required CI checks pass.
Superseded duplicate Ally approval; keeping latest operative Ally approval 4837026098 for head 8a6c5ae.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: dcc1e02
Looks good. The persistence-backed wizard transition has a narrowly scoped 45-second assertion timeout, while both affected full-flow specs retain enough enclosing test headroom.
Strengths
tests/e2e/conference-room-typing-intro.spec.ts:17explicitly reserves 180 seconds for the complete flow, resolving the earlier enclosing-timeout concern.- The longer assertion timeout applies only to the persistence-sensitive transition rather than weakening unrelated wizard checks.
Recommended Action
- Merge after required CI checks pass.
allyblockcast
left a comment
allyblockcast
left a comment
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: dcc1e02
Looks good. The persistence-backed wizard transition has a narrowly scoped 45-second assertion timeout, while both affected full-flow specs retain enough enclosing test headroom.
Strengths
tests/e2e/conference-room-typing-intro.spec.ts:17explicitly reserves 180 seconds for the complete flow, resolving the earlier enclosing-timeout concern.- The longer assertion timeout applies only to the persistence-sensitive transition rather than weakening unrelated wizard checks.
Recommended Action
- Merge after required CI checks pass.
Resolve Ally review-consistency guard: stale approval attests an older head after branch update.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 3e7f527
Looks good. The persistence-backed wizard transition retains a narrowly scoped 45-second assertion allowance, while both affected full-flow tests have explicit 180-second enclosing budgets. No retries, bypasses, or broader timeout weakening were introduced, and the exact-head CI checks are green.
Strengths
- The longer assertion timeout is limited to the persistence-sensitive transition.
- Both end-to-end flows retain sufficient full-test headroom around the persistence wait.
- The prior enclosing-timeout finding remains resolved on this head.
Recommended Action
- Merge after all required approvals are present.
allyblockcast
left a comment
allyblockcast
left a comment
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 3e7f527
Looks good. The persistence-backed wizard transition retains a narrowly scoped 45-second assertion allowance, while both affected full-flow tests have explicit 180-second enclosing budgets. No retries, bypasses, or broader timeout weakening were introduced, and the exact-head CI checks are green.
Strengths
- The longer assertion timeout is limited to the persistence-sensitive transition.
- Both end-to-end flows retain sufficient full-test headroom around the persistence wait.
- The prior enclosing-timeout finding remains resolved on this head.
Recommended Action
- Merge after all required approvals are present.
Dismissed by Codex: duplicate Ally approval on the same current head 3e7f527; keeping later allyblockcast approval 4838061934 as the single operative exact-head attestation.
Conflict in tests/e2e/nux-phase4-screenshots.spec.ts, pre-existing on 6bc9890 and unrelated to the approvals fixes. Resolved as a union: both sides wanted the same thing. Master (#934) widened the onboarding persistence wait to 45s for CI headroom; this branch added the assertion that the lead-name input actually retains its value. Kept master's longer timeout AND the branch's assertion -- the longer wait is precisely what makes that assertion reliable. Both consts are live: COMPANY_NAME has three call sites, WIZARD_PERSISTENCE_TIMEOUT_MS one.
2 participants
Thinking Path
Linked Issues or Issue Description
CI failure observed across unrelated open PRs including #925, #915, #906, and #927.
Failure shape:
tests/e2e/conference-room-typing-intro.spec.tstimed out waiting for theCreate your team leadheading.tests/e2e/nux-phase4-screenshots.spec.tstimed out waiting for the same heading.Confirm mission, which waits on company and goal persistence before advancing to the next step.What Changed
WIZARD_PERSISTENCE_TIMEOUT_MSconstant to both affected e2e specs.Confirm mission->Create your team leadassertion from 15s to 45s.Verification
PATH=/opt/homebrew/bin:$PATH pnpm install --frozen-lockfilePATH=/opt/homebrew/bin:$PATH npx playwright test --config tests/e2e/playwright.config.ts tests/e2e/conference-room-typing-intro.spec.ts tests/e2e/nux-phase4-screenshots.spec.ts --listgit diff --checkPostgres init script exited with code 1.Risks
Model Used
Codex / GPT-5, tool-assisted code editing and CI log inspection.
Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template