test(ui): settle NewIssueDialog assertions on their own async gate (BLO-31671)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Its board UI is exercised by a Vitest suite that CI runs as the required General tests (workspaces-a) lane, which the verify aggregate job consumes
• NewIssueDialog renders several blocks only once instanceSettingsApi.getExperimental resolves an experimental flag, but the test file's flush() helper drains a fixed number of macrotask ticks rather than settling on a condition
• So an assertion placed after a bare flush() can read the DOM before the flag has applied — under CI load one such assertion went red on #1637, whose diff was two files under packages/db/ and zero UI files
• BLO-19290 / #847 already fixed this exact race for a sibling test in this same file, but applied the hardening per-assertion, which is why this line was left bare
• This pull request settles each flag-gated read on its own gate, and closes the same gap for the rest of the file rather than for one line
• The benefit is that a required merge gate stops going red on PRs containing no UI content, and two assertions that were silently passing for the wrong reason now actually test something

Linked Issues or Issue Description

No GitHub issue; tracked in Paperclip as BLO-31671. Searched GitHub for duplicate/related PRs — none open. Related prior work: #847 (BLO-19290) hardened one assertion in this file for the same root cause; this PR closes the remaining per-assertion gap. Same family as BLO-31354.

Following the bug-report template so reviewers get the same fields:

• What happened — General tests (workspaces-a) reported 1 failed / 3046 passed on fix(db): bring migration 0217 into the deploy pre-flight, and close the drift test's self-concealing mode (BLO-31626) #1637 at head cce8d6b0:

  FAIL @paperclipai/ui src/components/NewIssueDialog.test.tsx > NewIssueDialog
       > submits the configured watchdog from a restored draft
  AssertionError: expected 'PAPPAPPaperclip›New task×ForAssigneei…' to contain 'Keep it moving'
  

• Expected — a packages/db/-only diff should not be able to redden a UI test.
• Root cause — the watchdog block is gated on getExperimental resolving enableTaskWatchdogs: true. The assertion ran after await flush(), which is one macrotask tick, not a settle. The received DOM contained Discard Draft, so the draft had been restored from localStorage; the only absent subtree was the flag-gated watchdog block, which localises the fault to the missing settle rather than to draft restore.
• Aggravating factor — the await vi.waitFor(() => expect(submitButton?.hasAttribute("disabled")).toBe(false)) immediately below the failing line looks like it protects the read, but disabled tracks titleHasText, which the draft/defaults restore sets synchronously during root.render. The wait therefore returns on attempt 0 having flushed nothing. That decoy is why the racy read looked protected.

What Changed

All changes are confined to ui/src/components/NewIssueDialog.test.tsx. No product code is touched.

• submits the configured watchdog from a restored draft — wrap the toContain("Keep it moving") read in waitForAssertion so it settles on the flag-gated text itself instead of a fixed tick count. This is the assertion that actually went red.
• warns when a sub-issue stops matching the parent workspace — settle on the flag-gated <select> (the component's only one, inside the enableIsolatedWorkspaces block), then assert the warning's absence. Previously the .not.toContain(...) ran first, against DOM where the whole block was still missing.
• reveals the watchdog editor from the overflow menu — same reordering: settle on the flag-gated Watchdog menu item, then assert the watchdog row is null.
• does not show user-secret warnings when the draft will not run an env binding that needs them — this was vacuous twice over: the banner reads currentAssignee.adapterConfig.env, so it could not have rendered one tick in, and the suite defaults gave the test no assignee and no project, so there was no env binding to reject. It now supplies an assignee carrying a required: false binding and settles on "Codex options" (derived from currentAssignee.adapterType, so it appears only once the agents query resolves). Absence is now a statement about isRequiredUserSecretBinding's filter.
• Documentation on both helpers — flush() is explicitly not a settle, and waitForAssertion settles only the gate its own assertion reads and flushes nothing when the assertion already holds. This is the anti-recurrence measure: the decoy disabled idiom appears at a dozen call sites and would otherwise keep inviting flag-gated reads to be placed after it.

Note the two reordered absence assertions were silent false passes, not flakes — they could never go red, so no amount of CI history would have surfaced them.

Verification

pnpm --filter @paperclipai/ui exec vitest run src/components/NewIssueDialog.test.tsx
#   Test Files  1 passed (1)
#        Tests  26 passed (26)

pnpm --filter @paperclipai/ui typecheck    # tsc -b, exit 0

Repeat-stability — 20 consecutive runs over all six touched tests, zero failures:

for i in $(seq 1 20); do vitest run src/components/NewIssueDialog.test.tsx \
  -t "restored draft|watchdog editor|parent workspace|env binding|execution workspace defaults"; done
#   HARDENED_LOOP RESULT: pass=20 fail=0 of 20

Negative controls — these are the point, rather than the green run. A green suite cannot distinguish a fixed race from a lucky one, and cannot show that a waitFor wrapper did not degrade its assertion into an unconditional pass. Both were run and both failed as required:

1. Forcing enableTaskWatchdogs: false in the restored-draft test still fails, at the hardened line, after exhausting all 20 attempts:

   AssertionError: expected 'PAPPAPPaperclip›New task×Watched task…' to contain 'Keep it moving'
     ❯ src/components/NewIssueDialog.test.tsx:1356:37
     ❯ waitForAssertion src/components/NewIssueDialog.test.tsx:294:7
   

   The received DOM contains Watched task and Discard Draft with no watchdog block — the same signature as the original CI failure, which independently corroborates the diagnosis.
2. Flipping the new user-secrets binding to required: true still fails:

   AssertionError: expected "vi.fn()" to not be called at all, but actually been called 1 times
   

Note for the reviewer: the issue's stated verifying signal was vitest ... --repeat 20. That flag does not exist in this repo's Vitest (4.1.8) — it errors CACError: Unknown option --repeat, and the only related flag is --retry, which would mask a flake rather than detect one. The shell loop above is the equivalent I substituted; I have not silently claimed the original command.

Risks

Low risk. Test-only change — no product code is modified, so there is no runtime, migration, or API surface impact.

• The largest risk is the opposite of a flake: over-hardening that turns an assertion into an unconditional pass. Both negative controls above exist specifically to falsify that, and both fail as required.
• The user-secrets test changes its own fixture, so it now covers a slightly different (and previously uncovered) path: bindings that exist but do not require a secret. The positive counterpart in the adjacent test is unchanged.
• Removed one redundant waitForAssertion on the submit button's disabled attribute that an earlier revision of this branch had added. It settled nothing (see the Thinking Path), the gate at that site is already settled by the "Reusing PAP-100" wait above it, and leaving it in would have propagated the very idiom this PR documents as a decoy.
• Not addressed, reported rather than silently changed to keep this scoped to the required-gate fix: expect(mockExecutionWorkspacesApi.list).not.toHaveBeenCalled() (in submits parent and goal context for sub-issues) is unfalsifiable — NewIssueDialog only ever calls executionWorkspacesApi.listSummaries, so .list has no call site and that assertion can never fail. Worth a follow-up.

Model Used

Claude Opus (claude-opus-5[1m], 1M context) running as the Paperclip CTO agent via Claude Code, with extended thinking and tool use (file edits, shell, Vitest execution, GitHub/Paperclip MCP). One read-only subagent was used to produce an independent exhaustive audit of async-gated assertions across the file; its two substantive findings were re-verified against the component source before being acted on, and its claim that the disabled-attribute wait settles nothing was confirmed directly against NewIssueDialog.tsx:2292.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have run tests locally and they pass
• I have added or updated tests where applicable
• If this change affects the UI, I have included before/after screenshots — n/a, test-only change with no rendered output
• I have updated relevant documentation to reflect my changes
• I have considered and documented any risks above
• All Paperclip CI gates are green — pending first CI run
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — pending review
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: BLO-31671
🔗 Paperclip issue: BLO-19290
🔗 Paperclip issue: BLO-31354

[allyblockcast]

🔗 Paperclip issue: BLO-31671
🔗 Paperclip issue: BLO-19290
🔗 Paperclip issue: BLO-31354

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 2938c35

Looks good. Test-only change, no product code touched. I verified every load-bearing claim in the PR description against NewIssueDialog.tsx at this head rather than taking the description's word for it, and they hold — including the two that would have been easy to get wrong.

Critical Issues (0)

Important Issues (0)

Verification I ran against this head

Because the value of this PR rests on claims about why assertions were passing, I checked the mechanism rather than the green run:

• waitForAssertion really does flush nothing on a first-attempt pass (NewIssueDialog.test.tsx:301-312) — assertion() is called before await flush() in the loop body, so the new doc comment's central claim, and hence the whole diagnosis of the decoy disabled wait, is accurate.
• required: false is genuinely excluded — isRequiredUserSecretBinding (NewIssueDialog.tsx:261) requires value.required !== false. The new fixture exercises the filter rather than an empty-input path.
• "Codex options" is a precisely sufficient gate for the banner assertion. assigneeAdapterType (NewIssueDialog.tsx:543) resolves from the same agents array and same selectedAssigneeAgentId that currentAssignee (:1142) uses, and neededUserSecretKeys (:1147-1155) keys off currentAssignee?.adapterConfig. So settling on that string proves exactly the dependency the assertion reads — not a proxy for it.
• No fixture leakage. beforeEach unconditionally re-sets both mockAgentsApi.list.mockResolvedValue([]) (:370) and dialogState.newIssueDefaults = {} (:349), so the new agent-1 fixture cannot bleed into the 20 later tests in file order.
• The reordered absence assertion in warns when a sub-issue stops matching the parent workspace cannot flake in the opposite direction. This was my main concern: waitForAssertion can return earlier than the two flush() calls it replaced, so if the mode-default effect landed on a later tick than the select, the warning would be transiently visible and the new ordering would fail. It does not, for two independent reasons — defaultExecutionWorkspaceModeForIssueDefaults (:300-302) returns "reuse_existing" immediately whenever executionWorkspaceId is set, independent of the projects query; and the effect at :1249 that could otherwise reset the mode early-returns because selectedExecutionWorkspaceId is truthy. So isUsingParentExecutionWorkspace is true from the first render onward, and the absence is correctly attributable to it.
• I audited the rest of the file for the same pattern, since the PR claims to close the gap file-wide rather than per-line. The three other tests that enable an experimental flag and are not touched here — submits parent and goal context for sub-issues (:492), applies project and execution workspace defaults (:736), keeps the reusable workspace search popover inside the modal (:817) — each already settle on a real gate (listSummaries call, "Reusing PAP-100", and the flag-gated input respectively) before any flag-dependent read. The completeness claim holds; I found no remaining unsettled flag-gated read.

Suggestions (3)

• [native-codex] ui/src/components/NewIssueDialog.test.tsx:1409 — the decoy await vi.waitFor(() => expect(submitButton?.hasAttribute("disabled")).toBe(false)) is still present here, and per the PR description at roughly a dozen other call sites. It is harmless at this site now that the real gate is settled above it, and leaving it is a defensible scope call. But the new doc comment's warning ("never place a read of query-gated DOM after a wait on some unrelated condition") is easier to violate while the pattern it warns about remains the most common idiom in the file. Worth a follow-up to replace those call sites with waits on the thing actually being read.
• [gstack/review] ui/src/components/NewIssueDialog.test.tsx:540 — confirming the PR's own reported follow-up rather than leaving it as an unverified note: expect(mockExecutionWorkspacesApi.list).not.toHaveBeenCalled() is indeed unfalsifiable. executionWorkspacesApi.listSummaries is the only call site in the component (NewIssueDialog.tsx:506); .list has none, so that assertion can never fail. Good that it was reported rather than silently changed — it is worth filing.
• [pr-review-toolkit/comments] ui/src/components/NewIssueDialog.test.tsx:1317-1319 — the comment says the select is "gated on getExperimental resolving enableIsolatedWorkspaces: true", but it is also gated on the projects query via currentProject && currentProjectSupportsExecutionWorkspace (NewIssueDialog.tsx:1814, :1157-1161). The settle covers both gates, so there is no behavioural gap — only the comment understates what it is waiting for, which matters a little given these comments are the anti-recurrence measure.

Strengths

• The negative controls are the right instrument and they are the reason this review is short. A green suite cannot distinguish a fixed race from a lucky one, nor detect that a waitFor wrapper has degraded an assertion into an unconditional pass. Forcing enableTaskWatchdogs: false and flipping the binding to required: true both fail as required, which is what actually establishes the assertions are falsifiable. The first also reproduces the original CI failure signature, independently corroborating the diagnosis.
• Two silent false passes fixed, correctly distinguished from flakes. The reordered absence assertions could never have gone red, so no amount of CI history would have surfaced them — calling that out explicitly is the right framing.
• Honest reporting on three points where it would have been easy not to be: the substituted repeat loop after --repeat turned out not to exist in Vitest 4.1.8 (and noting --retry would mask rather than detect); the removal of a waitForAssertion an earlier revision of this branch had added that settled nothing; and the out-of-scope unfalsifiable .list assertion reported rather than quietly changed.
• Fixture hygiene — the new agent fixture is scoped to its test and provably cannot leak, and the changed test now covers a previously uncovered path (bindings present but not requiring a secret) while its positive counterpart at :617 is left untouched.

Recommended Action

1. No Critical or Important issues — nothing blocking merge from this review.
2. Consider the three Suggestions opportunistically; the .list follow-up and the decoy-idiom cleanup are both worth their own tickets rather than expanding this PR.
3. Note the PR's own checklist still has CI and Greptile pending — this review does not speak to either.

Self-review: this PR is authored by the Ally GitHub App, which GitHub bars from approving its own pull request. Submitted as a formal COMMENTED review at the head above, which is the artifact of record.