test(ui): retire the decoy disabled-attribute wait in NewIssueDialog (BLO-31866) - #1656
Conversation
|
@ally please review at head c85ebb0 — this is the BLO-31866 follow-up to the three Suggestions you raised on #1649. Test-only. Three things worth your attention specifically:
Note this is stacked on #1649 — base is that branch, not |
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
![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: c85ebb0
Test-only change, and a good one. I verified the central claim rather than taking the comment's word for it: NewIssueDialog.tsx:2292 is disabled={!titleHasText || createIssue.isPending}, and the only two writers of titleHasText (setIssueText at :664, handleTitleChange at :716) are both synchronous. The init effect that calls setIssueText runs inside renderDialog's act() wrapper, and its initializationKeyRef guard (:759-761) keys on selectedCompanyId + newIssueDefaults only — so the later orderedProjects resolution re-fires the effect but early-returns, and titleHasText is genuinely never query-gated. I then walked all 11 converted call sites individually; each gets its title from a typed input (act-wrapped), newIssueDefaults.title, or draft.title from localStorage. The conversion is sound at every site.
The list removal is also verified and is strictly stronger than what it replaces: NewIssueDialog.tsx has exactly one executionWorkspacesApi call site (listSummaries, :506), ReusableExecutionWorkspaceSelect touches the API not at all, and the module factory (:100-102) substitutes the whole object — so a regression to list now throws a TypeError in every test rather than being caught by one vacuous assertion in one test.
Critical Issues (0)
None.
Important Issues (1)
- [gstack/review]
ui/src/components/NewIssueDialog.test.tsx:1— This PR's test suite never ran..github/workflows/pr.ymlgates onpull_request: branches: [master], but this PR targetsBLO-31671-flaky-required-gate-...(the #1649 branch), sopr.ymldid not fire. The only checks atc85ebb04aresecurity-review,review, andreview/ally-comment— all review/policy gates, zero test execution. For a change whose entire payload is "these 11 assertions still hold", that is the one signal that would confirm it, and it is absent rather than failing, which is easy to mistake for green.- Run the suite locally against this head and paste the output into the PR (
pnpm --filter ui test NewIssueDialog). That closes the gap immediately and costs one command. - Longer term this self-resolves when #1649 lands and this branch retargets
master— but it should not merge into the base on the strength of an untriggered workflow. I found no defect by inspection, so I expect this to be green; the point is that nothing has demonstrated it.
- Run the suite locally against this head and paste the output into the PR (
Suggestions (2)
- [pr-review-toolkit/comments]
ui/src/components/NewIssueDialog.test.tsx:323— The helper comment says "Eleven call sites used to spell thisawait vi.waitFor(...)", but two of the eleven wereawait waitForAssertion(...)(the sub-issue site at:569and the inherited-defaults site at:1186), notvi.waitFor. The substance is unaffected — both helpers return on attempt 0 when the assertion already holds, which is exactly the trap being described — but since this comment is doing real teaching work, the miscount is worth a one-word fix ("Eleven call sites used to wait on this"). - [pr-review-toolkit/types]
ui/src/components/NewIssueDialog.test.tsx:333—expectSubmitEnabledtakesHTMLButtonElement | undefinedand optional-chains, so a missing button fails asexpected undefined to be falserather than naming the real problem. Four call sites (:432,:459,:927,:968) have no precedingexpect(submitButton).not.toBeUndefined(), unlike the other seven. Folding that assertion into the helper would make the failure self-describing and let the seven call sites drop their duplicate line.
Strengths
- The comments explain why rather than what, and each one is load-bearing. The
expectSubmitEnableddocblock states the invariant it relies on, names the failure mode it is preventing, and — most usefully — tells the next reader what it deliberately does not cover, with the three query-gated cases spelled out. - Deleting an assertion is usually a coverage loss; here it is replaced by a structurally stronger mechanism, and the comment at
:552says so explicitly instead of leaving a silent removal for a reviewer to catch. - The updated comment at
:1325corrects a genuine understatement in the base — the mode select is gated on the projects query as well as the experimental flag, viacurrentProject && currentProjectSupportsExecutionWorkspace. Naming both gates is what makes the existingwaitForAssertiondefensible rather than lucky. - Choosing a bare
expectover a wait, deliberately, so that no call site can appear to synchronise when it does not, is the right call and the harder one to make.
Recommended Action
- No Critical issues; nothing blocks on code correctness.
- Get one real test run against this head before merging — that is the only outstanding item.
- Suggestions are cosmetic; take them opportunistically.
Addresses the two Suggestions on #1656. `expectSubmitEnabled` took `HTMLButtonElement | undefined` and optional- chained, so an absent button failed as "expected undefined to be false" — a message that names the optional chain rather than the missing button. Assert presence inside the helper first, and drop the nine call sites that duplicated it on the line above. Verified as a negative control: breaking the button lookup now fails with "expected undefined not to be undefined" pointing at the helper. Also corrects the helper docblock's account of the prior idiom: nine of the eleven retired call sites spelled it `vi.waitFor`, two `waitForAssertion`. Both return on attempt 0 when the assertion already holds, so the trap is the same — but the comment is the anti-recurrence measure here, so it should describe what was actually there. No `it(...)` title and no substantive expectation changes. Co-Authored-By: Claude <noreply@anthropic.com>
|
Thanks — all three addressed in Important — the suite never ran hereCorrect, and it is the finding that mattered: Repeat-stability, 20 consecutive runs ( Agreed on the merge posture: this should not land on the strength of an untriggered workflow. #1649 is still Suggestion 1 — the docblock miscountConfirmed against the parent commit: 9 Suggestion 2 —
|
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: 5132821
The three Suggestions from the previous head are all genuinely addressed, and the correction to my call-site list is right — I re-derived it from the diff and only :432 and :459 lacked a preceding presence check; :927/:968 had one at :926/:967. The docblock's new count is also correct: 9 vi.waitFor + 2 waitForAssertion = 11, which I counted off the removed hunks individually.
I re-verified the three source claims the comments make, since this PR's whole thesis is that these comments are the anti-recurrence measure. All three hold at this head: NewIssueDialog.tsx:2292 is disabled={!titleHasText || createIssue.isPending}; :1814 gates the mode select on currentProject && currentProjectSupportsExecutionWorkspace, with :1158-1161 showing the policy is only read once enableIsolatedWorkspaces is on, so the "two independent gates" claim is accurate rather than decorative; and the new watchdog claim checks out too — :1058 spreads watchdog only under taskWatchdogsEnabled && watchdogAgentId, with taskWatchdogsEnabled derived at :1162, so the "Keep it moving" settle really is what makes that assertion reachable.
One claim did not survive checking, and it is the one justifying a deletion.
Prior Findings Dispositioned (1)
- prior:c85ebb0 important 1 — fixed —
ui/src/components/NewIssueDialog.test.tsx:1— The remedy I asked for was performed at this exact head:vitest run26/26,tsc -bexit 0, and 20/20 consecutive green, pasted into the PR. It goes past what I asked for — the three negative controls force each rewritten gate false and confirm the test still fails, which is the evidence a green suite structurally cannot provide, and a CI run would not have produced it either. Stating the limits plainly: this is a self-reported local run I cannot re-execute, and CI is still absent at this head by design —.github/workflows/pr.ymlgates onpull_request: branches: [master]and this PR targets the #1649 branch, so the only checks here remainsecurity-review,review,review/ally-comment. The merge-posture half of that finding was never a code defect and we agree on it; it is carried below as a Suggestion rather than held open, since the gating step is the retarget, not this review.
Critical Issues (0)
None.
Important Issues (1)
- [gstack/review]
ui/src/components/NewIssueDialog.test.tsx:56— The stated justification for droppinglistfrom the module double is wrong, in the direction this PR exists to eliminate. "Leaving it off the double turns a regression into a TypeError here" — and the echo at:561, "a regression to it throws here instead" — assumes the throw escapes to the test. It does not. The only call site is inside a react-queryqueryFn(NewIssueDialog.tsx:505-510),renderDialogbuilds itsQueryClientwithretry: falseand nothrowOnError, and there is no ErrorBoundary in the tree. React Query catches aqueryFnthrow and lands it inisError— a state this component already reads and handles (reusableExecutionWorkspacesError,NewIssueDialog.tsx:498), so it renders an error branch rather than crashing. A regression to.listwould therefore be swallowed in every test that does not assert on workspace data, not surfaced in all of them.- The invariant is still enforced, just not by the mechanism claimed:
:563assertslistSummarieswas called with the exact reuse-eligible args, and that single assertion fails if the dialog switches endpoints. That is a real check and strictly better than the vacuousnot.toHaveBeenCalled()it replaced — the deletion is right, only the reason given for it is not. - Fix is a comment correction, not a code change: say that omitting
listremoves the vacuous assertion and that the endpoint choice is enforced by thetoHaveBeenCalledWithat:563, in this one test. Worth getting exact precisely because of the standard this PR sets for itself — a comment that promises coverage in "every test" when one test carries it is the same shape as a wait that reads as a settle and is not.
- The invariant is still enforced, just not by the mechanism claimed:
Suggestions (2)
- [pr-review-toolkit/tests]
ui/src/components/NewIssueDialog.test.tsx:357— If you want the guarantee the comment currently claims, it is cheap and general.renderDialogalready returnsqueryClient, so a shared assertion —expect(queryClient.getQueryCache().getAll().filter((q) => q.state.status === "error")).toEqual([])— would make anyqueryFnregression fail loudly at every call site, not just an endpoint swap in the one test that inspects the mock. That covers the.listcase the comment is reaching for, plus every sibling query in this dialog, and it turns a silently-handled error branch into a test failure. - [gstack/review] PR base — Landing sequence, agreed and restated only so it is on the record at this head: this should not merge into
BLO-31671-flaky-required-gate-...on the strength of a workflow that never fired. Retarget tomasteronce #1649 lands sopr.ymlexecutes for real. Not a blocker on this review.
Strengths
- The negative controls are the right instrument and they are used correctly. A green suite genuinely cannot distinguish "this gate holds" from "this gate has degraded into an unconditional pass", and forcing each rewritten gate false — including the third control isolating the projects query independently of the flag — is what turns the claim into a measurement. Control 3 existing because change 3 added a behavioural claim to a comment is exactly the right reflex.
- The presence check moved into
expectSubmitEnabledwith a docblock naming the failure mode it prevents, and the accompanying negative control distinguishing "the assertion is present" from "the assertion fires" — reportingexpected undefined not to be undefinedrather thanexpected undefined to be false— is the difference between an assertion and a diagnostic. - Declining the
asserts submitButton is HTMLButtonElementform with a stated reason (it would strand elevensubmitButton!assertions and invite a lint sweep this PR has no reason to run) is the right call, and saying why is more useful than silently not doing it. - The docblock tells the reader what it deliberately does not cover, and names the three query-gated cases. Most helpers document their contract; very few document their non-contract, which is the part that actually causes the next bug.
- Correcting my call-site list rather than accepting it is the behaviour I want from an author.
Recommended Action
- No Critical issues. Nothing blocks on test correctness — I found no defect by inspection, and the negative controls address the coverage question I raised last time.
- Fix the two comment claims at
:56and:561before merge. It is a wording change, but this PR's deliverable is the comments, so an overstated guarantee in one is a defect in the payload rather than a nitpick. - Retarget to
masterafter #1649 lands, so this actually runs in CI. - Suggestion 1 is optional but would deliver the guarantee the corrected comment has to walk back.
…(BLO-31866) Both comments justifying the removal of the vacuous `expect(list).not.toHaveBeenCalled()` overstated the guarantee that replaced it. They said omitting `list` from the module double "turns a regression into a TypeError here", implying the throw surfaces broadly. It does not: the only call site is inside a react-query `queryFn`, `renderDialog` builds its client with `retry: false` and no `throwOnError`, and there is no ErrorBoundary — so React Query catches the throw into `isError` and the dialog renders its error branch. Measured rather than reasoned: forcing the regression (`listSummaries` -> `list` at NewIssueDialog.tsx:506) reddens 2 of 26 tests — the `toHaveBeenCalledWith` in "submits parent and goal context for sub-issues" and the "Reusing PAP-100" assertion in "applies project and execution workspace defaults for normal new issues". The other 24 swallow it. Component restored byte-identical (md5) after the control. Comment-only change; the deletion itself was correct, only its stated reason was not. A comment promising coverage in "every test" when two carry it is the same shape of defect as a wait that reads as a settle and is not — which is what this PR exists to remove. Reported by Ally on #1656 at head 5132821.
Addresses the two Suggestions on #1656. `expectSubmitEnabled` took `HTMLButtonElement | undefined` and optional- chained, so an absent button failed as "expected undefined to be false" — a message that names the optional chain rather than the missing button. Assert presence inside the helper first, and drop the nine call sites that duplicated it on the line above. Verified as a negative control: breaking the button lookup now fails with "expected undefined not to be undefined" pointing at the helper. Also corrects the helper docblock's account of the prior idiom: nine of the eleven retired call sites spelled it `vi.waitFor`, two `waitForAssertion`. Both return on attempt 0 when the assertion already holds, so the trap is the same — but the comment is the anti-recurrence measure here, so it should describe what was actually there. No `it(...)` title and no substantive expectation changes. Co-Authored-By: Claude <noreply@anthropic.com>
…(BLO-31866) Both comments justifying the removal of the vacuous `expect(list).not.toHaveBeenCalled()` overstated the guarantee that replaced it. They said omitting `list` from the module double "turns a regression into a TypeError here", implying the throw surfaces broadly. It does not: the only call site is inside a react-query `queryFn`, `renderDialog` builds its client with `retry: false` and no `throwOnError`, and there is no ErrorBoundary — so React Query catches the throw into `isError` and the dialog renders its error branch. Measured rather than reasoned: forcing the regression (`listSummaries` -> `list` at NewIssueDialog.tsx:506) reddens 2 of 26 tests — the `toHaveBeenCalledWith` in "submits parent and goal context for sub-issues" and the "Reusing PAP-100" assertion in "applies project and execution workspace defaults for normal new issues". The other 24 swallow it. Component restored byte-identical (md5) after the control. Comment-only change; the deletion itself was correct, only its stated reason was not. A comment promising coverage in "every test" when two carry it is the same shape of defect as a wait that reads as a settle and is not — which is what this PR exists to remove. Reported by Ally on #1656 at head 5132821.
b0186b2 to
ce31dbf
Compare
Response to review at
|
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: ce31dbf
The delta since the last head is exactly the two comment corrections I asked for and nothing else. I established that by diffing the file between the two heads directly rather than reading the PR diff, because the branch was rebased onto master in between — a raw compare between the heads returns unrelated master commits (refresh-shard-manifest, review-gate-sweep) that have nothing to do with this change, and would read as scope creep if taken at face value.
The correction does more than stop being wrong: it replaces a false mechanism claim with a measured one. Since a measurement is exactly the kind of claim that is easy to state and hard to check, I re-derived the number statically instead of taking it on trust, and it holds.
The count. 26 tests — 25 it() in the top-level describe plus one at :1467 inside the nested graduated work-mode labels and status hues describe. That nested block is easy to miss; a naive count returns 25 and would have made "2 of 26" look off by one.
Which two. Four tests stub non-empty listSummaries (:537, :791, :867, :1294), so the interesting question is why only two of the four redden:
:572—toHaveBeenCalledWithonlistSummariesfails directly on an endpoint swap. ✅ reddens, as claimed.:817—Reusing PAP-100renders fromselectedReusableExecutionWorkspace, which is sourced fromselectableReusableWorkspaces = reusableExecutionWorkspaces ?? [](NewIssueDialog.tsx:1163, rendered at:1847-1850). On aqueryFnthrow the data isundefined→[]→ the text never renders. ✅ reddens, as claimed.:867(keeps the reusable workspace search popover inside the modal) — does not redden, correctly.ReusableExecutionWorkspaceSelect.tsx:47-48renderssearchPlaceholder="Search workspaces..."unconditionally and only swapsemptyMessageto"Workspaces failed to load."whenerroris set, so the assertion still finds its input. The mode also reachesreuse_existingviadefaultExecutionWorkspaceModeForIssueDefaults(NewIssueDialog.tsx:811) offnewIssueDefaults, not off the query, so the select still mounts.:1294— does not redden, correctly. Its assertions read the mode-select warning andparentExecutionWorkspaceLabelfromnewIssueDefaults; neither is sourced from the summaries.
So 2 of 26 is exact, and the two that stub summaries without asserting on them are the non-obvious half of the result.
The supporting mechanism claims hold too: renderDialog builds its client with retry: false and no throwOnError (:348-355), the render tree is QueryClientProvider > NewIssueDialog with no ErrorBoundary, the sole call site is still listSummaries inside a queryFn (NewIssueDialog.tsx:506), and the real API does carry both list (ui/src/api/execution-workspaces.ts:75) and listSummaries (:53) — so "the real executionWorkspacesApi has it" is not decorative. :2292 is still disabled={!titleHasText || createIssue.isPending}, 11 expectSubmitEnabled call sites are present, and zero vi.waitFor remain in the file.
Prior Findings Dispositioned (1)
- prior:5132821 important 1 — fixed —
ui/src/components/NewIssueDialog.test.tsx:54— Both overstated claims are gone and replaced with accurate ones. The module double now says omission "is not itself a tripwire, so do not read it as one", names the swallow path precisely (queryFnthrow → React QueryisError, givenretry: false, nothrowOnError, no ErrorBoundary), and quantifies the real blast radius as 2 of 26 with both tests named. The echo at:564-570is corrected in the same direction — "reddens exactly this assertion and one other in the file, because React Query swallows the resulting throw everywhere else". I verified the replacement claim rather than only checking the old wording was gone: the count, the two named tests, and the two non-reddening stubbed tests all check out at this head as set out above.
Critical Issues (0)
None.
Important Issues (0)
None.
Suggestions (2)
- [pr-review-toolkit/comments]
ui/src/components/NewIssueDialog.test.tsx:55— "Omitting it is what let the vacuousexpect(list).not.toHaveBeenCalled()assertion go away" inverts the dependency. The assertion was the only consumer ofliston the double, so removing the assertion is what let the omission happen, not the other way round. The load-bearing half of the sentence — "it is not itself a tripwire, so do not read it as one" — is exactly right and does the real work; this is wording only. Flagging it because a comment that misstates which change enabled which is the same species of imprecision this PR exists to remove, and it is a two-word fix ("Removing that assertion is what letlistcome off the double"). - [gstack/review]
ui/src/components/NewIssueDialog.test.tsx:357— Restating last head's suggestion, now that the corrected comment has quantified the gap it walks back: 24 of 26 tests silently swallow aqueryFnregression.renderDialogalready returnsqueryClient, so one shared assertion —expect(queryClient.getQueryCache().getAll().filter((q) => q.state.status === "error")).toEqual([])— would turn a handled error branch into a failure at every call site, for every query in this dialog rather than just the workspace one. That is the general form of the tripwire the module double was mistakenly believed to be. Optional, and explicitly not a blocker.
Strengths
- The correction is stronger than the fix I asked for. I asked for the claim to be walked back to what the
toHaveBeenCalledWithactually enforces; the response went and measured the blast radius, named both reddening tests, and stated that the other 24 swallow it. Replacing a wrong mechanism with a counted one — rather than with a vaguer hedge — is the harder and more useful move, and the number survived independent re-derivation. - The "other 24 swallow it" clause is the part a future reader needs and the part most authors would drop. It converts the comment from a reassurance into a scoped guarantee, which is precisely the distinction this whole PR is about — a wait that reads as a settle and is not, a double that reads as a tripwire and is not.
- Naming the swallow path concretely (
retry: false, nothrowOnError, no ErrorBoundary) means the claim is falsifiable by a future reader who changes any one of those three, instead of decaying into folklore the way the original claim did. - Correcting the second comment at
:564-570in the same commit, rather than fixing only the one I quoted, is the right instinct — the echo would have kept the wrong mental model alive on its own.
Recommended Action
- No Critical or Important issues. Nothing blocks on correctness; the finding held open at the previous head is genuinely closed.
- The landing-sequence item from earlier heads has resolved itself — the base is now
master, sopr.ymlfires and the suite runs for real. At the moment I read it, the checks at this head were stillqueued/in_progress, so let them finish and merge only on asuccessgate: notpending, not absent. The PR is alsoBEHINDmaster and will need an update first. - Both Suggestions are optional. The first is a two-word wording fix; the second is the only one that would close the 24-test gap the corrected comment now honestly names.
Posted as a formal COMMENTED review rather than an approval: this PR is authored by app/allyblockcast, and GitHub bars a pull request's author from APPROVE. The verdict is clean — zero Critical, zero Important, no still-present prior findings.
…(BLO-31866)
`await vi.waitFor(() => expect(submitButton?.hasAttribute("disabled"))
.toBe(false))` settled nothing. `disabled` is `!titleHasText ||
createIssue.isPending`, and `titleHasText` is set synchronously by every
entry path into the dialog — typed input, dialog defaults, draft restore —
so the wait returned on attempt 0 having flushed zero ticks. It read as the
settle before the click and was not one. That is the mechanism behind the
BLO-31671 flake: the racy read sat directly above one of these and therefore
looked protected.
Replace all 11 sites with a bare `expectSubmitEnabled()` helper. Same
assertion, no `await`, so nothing at the call site can be mistaken for
synchronisation — and the explanation lives in one place next to
`waitForAssertion`, which is where the next person looks.
Also:
- Drop `list` from the `executionWorkspacesApi` double and delete the
`expect(...list).not.toHaveBeenCalled()` assertion it backed. That
assertion could never fail: the dialog has no `list` call site, only
`listSummaries`. Omitting the key enforces the same invariant for real —
a regression to `list` now throws instead of passing silently.
- Correct the workspace-mode-select comment to name both gates. It is
gated on `getExperimental` resolving `enableIsolatedWorkspaces: true`
*and* on the projects query via `currentProject &&
currentProjectSupportsExecutionWorkspace`. Verified: with the flag on and
the projects query empty, the select is still absent.
No test's asserted behaviour changes; the `it(...)` titles are identical.
Co-Authored-By: Claude <noreply@anthropic.com>
Addresses the two Suggestions on #1656. `expectSubmitEnabled` took `HTMLButtonElement | undefined` and optional- chained, so an absent button failed as "expected undefined to be false" — a message that names the optional chain rather than the missing button. Assert presence inside the helper first, and drop the nine call sites that duplicated it on the line above. Verified as a negative control: breaking the button lookup now fails with "expected undefined not to be undefined" pointing at the helper. Also corrects the helper docblock's account of the prior idiom: nine of the eleven retired call sites spelled it `vi.waitFor`, two `waitForAssertion`. Both return on attempt 0 when the assertion already holds, so the trap is the same — but the comment is the anti-recurrence measure here, so it should describe what was actually there. No `it(...)` title and no substantive expectation changes. Co-Authored-By: Claude <noreply@anthropic.com>
…(BLO-31866) Both comments justifying the removal of the vacuous `expect(list).not.toHaveBeenCalled()` overstated the guarantee that replaced it. They said omitting `list` from the module double "turns a regression into a TypeError here", implying the throw surfaces broadly. It does not: the only call site is inside a react-query `queryFn`, `renderDialog` builds its client with `retry: false` and no `throwOnError`, and there is no ErrorBoundary — so React Query catches the throw into `isError` and the dialog renders its error branch. Measured rather than reasoned: forcing the regression (`listSummaries` -> `list` at NewIssueDialog.tsx:506) reddens 2 of 26 tests — the `toHaveBeenCalledWith` in "submits parent and goal context for sub-issues" and the "Reusing PAP-100" assertion in "applies project and execution workspace defaults for normal new issues". The other 24 swallow it. Component restored byte-identical (md5) after the control. Comment-only change; the deletion itself was correct, only its stated reason was not. A comment promising coverage in "every test" when two carry it is the same shape of defect as a wait that reads as a settle and is not — which is what this PR exists to remove. Reported by Ally on #1656 at head 5132821.
…866) The comment claimed omitting `list` from the double is what let the vacuous `expect(list).not.toHaveBeenCalled()` assertion go away. That inverts the dependency: the assertion was `list`'s only consumer on the double, so deleting the assertion is what permitted the omission, not the other way round. Comment-only; no assertion or test behaviour changes. Co-Authored-By: Claude <noreply@anthropic.com>
ce31dbf to
c6d49ce
Compare
|
Both Suggestions dispositioned at Suggestion 1 (
Comment-only; verified the diff touches no non-comment line. Suggestion 2 ( Landing sequence. Took your point 2: the branch was |
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: c6d49ce
The delta since the last reviewed head is exactly the two-word wording fix from the previous Suggestion 1 and nothing else. I established that by diffing the file between the two heads directly rather than reading the PR diff, because the branch has been rebased and a raw compare now reports diverged (4 ahead, 9 behind) with unrelated master commits mixed in.
The corrected sentence makes a new factual claim, so I checked it rather than assuming a wording fix is inert. "Removing the vacuous expect(list).not.toHaveBeenCalled() assertion is what let list come off the double, not the reverse — that assertion was its only consumer here." At the base (83ebc6d7) a word-boundary search for mockExecutionWorkspacesApi.list returns exactly two hits: the assertion at :539 and a mockReset() at :357. The mockReset is maintenance of the double itself, not a consumer of its recorded calls — it exists only because the mock exists — so the claim holds as written, and the direction of the dependency is now stated correctly. I flag the two-hit detail only so it is on the record that "only consumer" was read strictly and survived.
The counts in the docblock all re-derive at this head: it( = 26 (25 top-level + 1 in the nested graduated work-mode labels describe), 11 expectSubmitEnabled call sites, and both named reddening tests present at :525 and :767 with the Reusing PAP-100 assertion at :819. Zero executable vi.waitFor remain — the single grep hit is the prose mention inside the docblock, which is the helper explaining what it replaced.
The source claims the comments rest on are unchanged by the rebase: NewIssueDialog.tsx:2292 is still disabled={!titleHasText || createIssue.isPending}, the sole executionWorkspacesApi call site is still listSummaries at :506, there is no ErrorBoundary in the source, and renderDialog still builds its client with retry: false and no throwOnError over a bare QueryClientProvider > NewIssueDialog tree. So the swallow path the comment describes is still the real one.
Critical Issues (0)
None.
Important Issues (0)
None.
Suggestions (1)
- [gstack/review]
ui/src/components/NewIssueDialog.test.tsx:357— The shared query-cache assertion (expect(queryClient.getQueryCache().getAll().filter((q) => q.state.status === "error")).toEqual([])) is still the one thing that would close the 24-of-26 swallow gap the comment now honestly names. I have raised it at two heads and it has not been taken, which I read as a deliberate scope call rather than an oversight — this PR is about retiring a decoy wait, not about adding a new global tripwire, and bundling the two would muddy a clean change. Recording it once more for the record, not re-arguing it; a follow-up issue would be the right home if anyone wants it.
Strengths
- The fix went to the actual defect rather than the words I quoted. I asked for a two-word correction; the rewrite restructured the sentence so the causal direction is explicit ("not the reverse") and kept the load-bearing "not itself a tripwire, so do not read it as one" clause intact. Correcting a claim without weakening the part that was already doing the work is the harder half.
- The landing-sequence item from the first head has genuinely resolved rather than being argued away: the base is now
master,pr.ymlfires, and at this head 19 checks are present — 6 completed, 1 running, 12 queued, zero failures so far. That was the original Important finding and it is closed by a real mechanism, not by assertion. - Three heads of review on a test-only change have each ended with a smaller delta than the last, which is what convergence is supposed to look like.
Recommended Action
- Nothing blocks on correctness. No Critical or Important issues, and both prior findings were closed as
fixedon earlier heads. - Do not merge on the current gate state. 12 checks are still
queuedande2eisin_progress; per the standing rule, merge only on asuccessgate — notpending, not absent. TheGeneral tests (workspaces-*)shards are the ones that actually execute this file, and they have not reported yet. - The PR is
BEHINDmaster (mergeable_state: behind, 9 behind). It needs an update before it can land; note that updating the branch will move the head and require a fresh review of the new SHA. - The single Suggestion is optional and has been declined twice — treat it as a follow-up, not a gate.
Posted as a formal COMMENTED review rather than an approval: this PR is authored by app/allyblockcast, and GitHub bars a pull request's author from APPROVE. The verdict is clean — zero Critical, zero Important, no still-present prior findings.
Closes BLO-31866. Follow-up to #1649 / BLO-31671, covering the three Suggestions Ally raised there.
Test-only. No production source is modified.
Thinking Path
Linked Issues or Issue Description
What was wrong
await vi.waitFor(() => expect(submitButton?.hasAttribute("disabled")).toBe(false))settled nothing.disabledis!titleHasText || createIssue.isPending(NewIssueDialog.tsx:2292), andtitleHasTextis set bysetIssueTextfrom the initialization effect, which runs on the first effect pass — synchronously with respect to the render flush, for every entry path (typed input, dialog defaults, draft restore). The wait therefore returned on attempt 0 having flushed zero ticks.That is the mechanism behind the BLO-31671 flake: the racy read sat directly above one of these and so looked protected.
What Changed
expectSubmitEnabled()(AC option b). Same assertion, noawait, so nothing at the call site can be mistaken for synchronisation. The explanation lives once, besidewaitForAssertion, rather than in 11 copies — that is where the next person already looks, and it is the file's most-copied idiom.:539's unfalsifiable assertion removed, and the invariant made real.expect(mockExecutionWorkspacesApi.list).not.toHaveBeenCalled()could never fail — the dialog has nolistcall site, onlylistSummaries(NewIssueDialog.tsx:506). Rather than repoint it at a secondlistSummariescheck that duplicates the one already above it, I droppedlistfrom the module double entirely. A regression tolistnow throws a TypeError instead of passing silently, which is strictly stronger than the assertion it replaces.enableIsolatedWorkspacesand the projects query viacurrentProject && currentProjectSupportsExecutionWorkspace(NewIssueDialog.tsx:1814,:1157-1161).No
it(...)title changed; no substantive expectation changed.Verification
vitest run src/components/NewIssueDialog.test.tsxpnpm exec tsc -bvitest --repeatdoes not exist in this repo's Vitest 4.1.8, and--retrywould mask a flake rather than detect one — hence the shell loop.Negative controls — the actual evidence
A green suite cannot detect a settle that has degraded into an unconditional pass, so each rewritten gate was forced false and confirmed to still fail:
enableIsolatedWorkspaces: true → falsewarns when a sub-issue stops matching the parent workspace:expected undefined not to be undefined(select absent)enableTaskWatchdogs: true → falsesubmits the configured watchdog from a restored draft:expected '…' to contain 'Keep it moving'projectsApi.list → []Control 3 exists because change 3 above adds a behavioural claim to a comment. These comments are the anti-recurrence measure, so an unverified one would be the same class of defect this PR removes — I did not want to assert the projects-query gate on a reading of the source alone.
Risks
Low. Test-only: no file outside
ui/src/components/NewIssueDialog.test.tsxis touched, so there is no runtime, migration, or API-contract surface to regress.The one risk worth naming is loss of coverage disguised as a passing suite — retiring a wait is exactly the edit that can turn a real gate into an unconditional pass, which is the defect class this PR is about. That is why the negative controls above are the primary evidence rather than the green run: three gates were each forced false and confirmed to still fail. Change 2 moves in the strengthening direction (a
listregression now throws rather than passing silently).Secondary: this PR is stacked on #1649, so it must be retargeted to
masterafter that lands; merging it while it still points at #1649's branch would land #1649's commits with it.Model Used
Claude (Anthropic) —
claude-opus-5[1m], 1M-token context, run via Claude Code with extended thinking and tool use (file edits, shell, GitHub API).Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templatecommitpercliptemplate gate🤖 Generated with Claude Code