MotirBuilding in public
MOTIR · moooon
onMotir
You’re viewing a public project. Anyone can view it — no account needed. Sign in to submit, upvote, or comment on requests.View-only — you can’t edit work items
MOTIR-1737

Two more component tests carry the MOTIR-1736 effect-ordering race (OnboardingCanvasRoadmap, TierDocModal) — found by a scheduler-ordering audit worth keeping

Done
Description

Where this came from

Out-of-scope finding surfaced while fixing MOTIR-1736 (the ProjectRoadmapCanvas auto-reset flake). Logged per notes.html mistake #27 rather than absorbed into that PR, whose scope was one file.

The underlying condition is repo-wide

vitest.config.ts never sets IS_REACT_ACT_ENVIRONMENT, and RTL's async wrapper deliberately turns the act environment OFF for findBy* / waitFor, draining with a bare setTimeout(0). React flushes passive effects on a separate scheduler callback (setImmediate under Node). So in EVERY component test, an awaited findBy* can resolve with pending passive effects — the render landed, the effect did not. Any non-retrying assertion that depends on an effect is then a load-dependent flake. MOTIR-1736 was one instance; nothing structurally confines it to that file.

Evidence — a decisive audit instrument

Flipping the scheduler ordering makes the latent race deterministic, with no added delay (so it does not create false "too slow" failures):

// tests/helpers/__auditLateEffects.ts  (setupFiles, via a throwaway config)
const real = globalThis.setImmediate;
globalThis.setImmediate = (fn, ...args) => real(() => setTimeout(() => fn(...args), 0));

Run over all 150 component test files (1270 tests), this reproduced MOTIR-1736's exact CI failure pre-fix, passed it post-fix, and flagged two other genuine failures:

  • tests/components/OnboardingCanvasRoadmap.test.tsx"shows a 'Your plan' preview at the top level and drills into the epic roots"
  • tests/components/TierDocModal.test.tsx"fetches the pre-plan and renders the tier doc (DirectionDocView) for the clicked tier" (fails as Unable to find an element with the text: /building an internal tool for a small team/i — the effect-driven fetch/render never lands within the query budget)

Excluded as instrument artifacts, do not chase: appearance-sync.test.tsx (5 tests) uses vi.useFakeTimers(), so the shim's real setTimeout never fires — artifact, not a race. An earlier 8 ms-delay variant of the shim also flagged sprint-points-refetch.test.tsx and delete-work-item-dialog.test.tsx; both pass at zero delay, so they were "slowed past the 1 s findBy budget", not ordering races. The zero-delay form is the trustworthy instrument — start from it.

What to do

  • Fix the two named tests the same way MOTIR-1736 was fixed: assert effect-driven callbacks/renders inside a retrying waitFor, and flush with await act(async () => {}) before any negative assertion. Confirm each fails pre-fix and passes post-fix under the zero-delay shim.
  • Decide whether to make the audit permanent — this is the real value. Options, in rough order of preference: (a) a scheduled/nightly CI lane running the component suite under the shim (catches new instances without taxing PR CI); (b) setting IS_REACT_ACT_ENVIRONMENT = true in a setup file so RTL flushes effects deterministically — the root fix, but it surfaces act() warnings across ~30 files and is its own migration; (c) a lint rule banning a bare expect(<mock>) assertion in a test file that also awaits findBy*. Recommend (a) now, and evaluate (b) as a follow-up — (b) removes the whole class rather than detecting it.
  • The convention itself is already documented: motir-core/CLAUDE.md § E2E tests wait on the AUTHORITATIVE signal gained a component-test bullet in the MOTIR-1736 PR.

Acceptance criteria

  • Both named tests await an authoritative signal and pass under the zero-delay ordering shim; the full component suite shows no genuine failure under it (fake-timer files documented as excluded).
  • A decision is recorded on making the audit permanent, and whichever option is chosen is implemented (or explicitly deferred with a reason).
  • No production component change — this is a test-side race, same as MOTIR-1736.