fix(async-parallel): suppress in tests, browser fixtures, and ordered UI flows#270
Merged
Merged
Conversation
|
🔴 React Review — 0/100 (unchanged) · No new issues Reviewed by react-review for commit f6e8b7b. Configure here. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
cursor Bot
pushed a commit
that referenced
this pull request
May 16, 2026
Address two Bugbot findings on PR #270: - getCalleeIdentifierTrail() rejected ChainExpression at entry, so optional-chained awaits like 'await page?.click()' produced an empty trail and the UI-flow / sequencing signal was missed. Peel any wrapping ChainExpression layers before checking for the call or new node — the inner traversal already handles ChainExpression but was never reached. - '@storybook/test' as a bare prefix in TEST_LIBRARY_IMPORT_SOURCE_PREFIXES subsumed both '@storybook/test-runner' and '@storybook/testing-library' under the same catch-all, even though both are independently versioned and already enumerated in the exact set. Tighten the prefix to '@storybook/test/' and add '@storybook/test-runner/' and '@storybook/testing-library/' for genuine subpaths only. Document the trailing-slash invariant in the comment so future entries follow the same rule. Add three regressions: - 'await page?.click()' is treated as an ordered UI flow and skipped. - '@storybook/test-runner' import on a production-shaped path silences three independent sequential awaits. - '@storybook/test/spy' subpath import does the same. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f3e4ffc. Configure here.
cursor Bot
pushed a commit
that referenced
this pull request
May 16, 2026
Address two Bugbot findings on PR #270: - getCalleeIdentifierTrail() rejected ChainExpression at entry, so optional-chained awaits like 'await page?.click()' produced an empty trail and the UI-flow / sequencing signal was missed. Peel any wrapping ChainExpression layers before checking for the call or new node — the inner traversal already handles ChainExpression but was never reached. - '@storybook/test' as a bare prefix in TEST_LIBRARY_IMPORT_SOURCE_PREFIXES subsumed both '@storybook/test-runner' and '@storybook/testing-library' under the same catch-all, even though both are independently versioned and already enumerated in the exact set. Tighten the prefix to '@storybook/test/' and add '@storybook/test-runner/' and '@storybook/testing-library/' for genuine subpaths only. Document the trailing-slash invariant in the comment so future entries follow the same rule. Add three regressions: - 'await page?.click()' is treated as an ordered UI flow and skipped. - '@storybook/test-runner' import on a production-shaped path silences three independent sequential awaits. - '@storybook/test/spy' subpath import does the same. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
cursor
Bot
force-pushed
the
cursor/suppress-async-parallel-noise-3ea1
branch
from
03d1380 to
e91a18d
Compare
Add the building blocks needed to suppress async-parallel advice in files that are clearly tests or ordered UI flows: - BROWSER_TEST_FILE_PATTERN matches Vitest browser mode / Playwright Component Testing / Storybook test-runner files (.browser.[jt]sx?). - TEST_LIBRARY_IMPORT_SOURCES + TEST_LIBRARY_IMPORT_SOURCE_PREFIXES enumerate the module identifiers that prove a file is a test, story, or interaction-driving harness (Vitest, Jest, Mocha, Testing Library, Playwright, Cypress, Storybook test, Puppeteer, WebdriverIO, etc.). - isTestLibraryImportSource() is the thin predicate over those tables. - ORDERED_UI_FLOW_CALLEE_NAMES + ORDERED_UI_FLOW_CALLEE_PREFIXES name the render/expect/click/findBy*/page.goto-style callees that signal a deliberately serialized UI flow. - INTENTIONAL_SEQUENCING_CALLEE_NAMES names the sleep/delay/animation callees that signal deliberate pacing, mirroring the sister set in async-await-in-loop. - getCalleeIdentifierTrail() walks a CallExpression's callee chain (members, optional-chains, intermediate calls) and yields every identifier name leaf-first so the rule can pattern-match the rightmost identifier against any of those tables. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
The previous implementation used a narrow local TEST_FILE_PATTERN that only matched '*.test.*', '*.spec.*', and '*.stories.*' suffixes — so it false-positived on: - '*.browser.tsx' Vitest browser fixtures. - Files under 'tests/', 'test/', '__tests__/', 'e2e/', 'playwright/', 'cypress/', or fixture/mock directories that the shared isTestFilePath() heuristic already understands. - Production-co-located helpers (src/test-utils.ts, fixtures shipped next to a component) that import a test runner or driver. - Render → expect → click → expect ordered UI flows whose serialization is the whole point. - Intentional animation/demo sequencing (sleep, delay, animate, spring, fakeTimers tick/advance) that loses meaning under Promise.all(). Refit: - Tag the rule 'test-noise' so mergeAndFilterDiagnostics routes every path that isTestFilePath() recognises through the shared filter — picking up __tests__/, tests/, test/, __mocks__/, cypress/, e2e/, playwright/, *.test/spec/stories/story/fixture/fixtures.* and Windows-slashed equivalents for free. - Drop the local TEST_FILE_PATTERN early-return; the tag covers it. - Add a '*.browser.[jt]sx?' filename check for Vitest browser mode / Storybook test-runner / Playwright Component Testing fixtures that the path heuristic intentionally doesn't classify as tests. - Track ImportDeclaration sources during file traversal — any import matching isTestLibraryImportSource() flags the file as a fixture and suppresses the rule (so a 'src/seed.ts' that imports '@playwright/test' or '@testing-library/react' is treated as a harness even though its path looks like production code). - When a consecutive-await block contains at least one ordered UI-flow call (render/expect/click/locator.fill/findByRole/page.goto/…) or one intentional sequencing call (sleep/delay/animate/tick/…), skip the entire block. A single render() or sleep() in the middle of three otherwise-independent awaits proves the sequence is deliberately serialized; collapsing it into Promise.all([...]) would change observable behavior. - Authors who genuinely want serial execution without any of these signals can opt out per-block with a documented '// oxlint-disable-next-line react-doctor/async-parallel -- <reason>' comment, which mergeAndFilterDiagnostics already honors. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
Two layers of coverage for the async-parallel suppression refit: - packages/react-doctor/tests/regressions/js-performance-rules.test.ts exercises the in-rule guards end-to-end via runOxlint(): production code still flags three independent awaits; '*.browser.tsx' fixtures, '@playwright/test' / '@testing-library/react' / 'vitest/browser' imports, render → findByRole → userEvent.click → findByText flows, Playwright locator chains, intentional sleep/animate/spring sequencing, and documented inline opt-outs all stay quiet; an independent prefix of three awaits followed (after non-await statements) by a UI-flow call still flags the prefix; dependent chains where each await reads an earlier result are never flagged. - packages/react-doctor/tests/merge-and-filter-diagnostics.test.ts exercises the tag-driven path filter through mergeAndFilterDiagnostics: '*.test.tsx', '__tests__/' directories, 'playwright/' / 'cypress/' / 'e2e/' top-level directories, and Windows-slashed test paths all auto-suppress, while production paths keep firing. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
… UI flows" Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
Address two Bugbot findings on PR #270: - getCalleeIdentifierTrail() rejected ChainExpression at entry, so optional-chained awaits like 'await page?.click()' produced an empty trail and the UI-flow / sequencing signal was missed. Peel any wrapping ChainExpression layers before checking for the call or new node — the inner traversal already handles ChainExpression but was never reached. - '@storybook/test' as a bare prefix in TEST_LIBRARY_IMPORT_SOURCE_PREFIXES subsumed both '@storybook/test-runner' and '@storybook/testing-library' under the same catch-all, even though both are independently versioned and already enumerated in the exact set. Tighten the prefix to '@storybook/test/' and add '@storybook/test-runner/' and '@storybook/testing-library/' for genuine subpaths only. Document the trailing-slash invariant in the comment so future entries follow the same rule. Add three regressions: - 'await page?.click()' is treated as an ordered UI flow and skipped. - '@storybook/test-runner' import on a production-shaped path silences three independent sequential awaits. - '@storybook/test/spy' subpath import does the same. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
`async-parallel` was the only consumer of `TEST_FILE_PATTERN`, and the previous commit moved it to the `test-noise` tag plus `BROWSER_TEST_FILE_PATTERN`. `SECRET_TEST_FILE_PATTERN` in `constants/security.ts` is a distinct export with a wider suffix family and stays. No other call sites reference the bare name. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
cursor
Bot
force-pushed
the
cursor/suppress-async-parallel-noise-3ea1
branch
from
e91a18d to
f6e8b7b
Compare
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
2 participants
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.
Problem
async-parallelwas loud in places where the sequential awaits are the whole point:TEST_FILE_PATTERN(*.test.*,*.spec.*,*.stories.*) that missedtests/,test/,__tests__/,e2e/,playwright/,cypress/, fixtures, mocks, and*.browser.tsx.test-noise, so the sharedisTestFilePath()suppression inmergeAndFilterDiagnosticsdidn't apply.src/test-utils.tsimporting@playwright/testor@testing-library/reactstill got the advice.render → findByRole → click → findByText, where each await must observe the side effects of the previous one.sleep,animate, fake-timertick, …) that would lose meaning underPromise.all().Dogfood references:
async-parallelinSettingsPanels.browser.tsx, ordered test-likerender → expect → click → expectscreenshots, issues #216 and #219, related PR #238.Fix
async-parallelastest-noise. LetsmergeAndFilterDiagnosticsroute every path thatisTestFilePath()recognises through the existing shared filter —__tests__/,tests/,test/,__mocks__/,cypress/,e2e/,playwright/,*.test/spec/stories/story/fixture/fixtures.*, and Windows-slashed equivalents — for free.TEST_FILE_PATTERNearly-return — the tag now covers it.*.browser.[jt]sx?filename check for Vitest browser mode / Storybook test-runner / Playwright Component Testing fixtures that the path heuristic intentionally doesn't classify as tests.isTestLibraryImportSource()flags the file as a fixture and suppresses the rule. Coversvitest,jest,mocha,chai,node:test,bun:test,@testing-library/*,@playwright/*,playwright,cypress,@cypress/*,@storybook/test*,puppeteer,webdriverio,@nuxt/test-utils, and subpaths (vitest/browser,@vitest/spy, …).render,expect,click,fill,findByRole,findAllByText,page.goto,locator(...).click(), …) or one intentional sequencing call (sleep,delay,animate,spring,tween,tick,advanceTimersByTime, …), the entire block is treated as deliberately serial. A singlerender()orsleep()in the middle of three otherwise-independent awaits is enough — collapsing such a block intoPromise.all([...])would change observable behavior.// oxlint-disable-next-line react-doctor/async-parallel -- <reason>) still works for everything else, sincemergeAndFilterDiagnosticsalready honors it.Edge cases covered
import typefrom a test library still suppresses (a type-only import is still a strong "this is a fixture" signal).findBy*/findAllBy*are matched by prefix without enumerating every suffix.page.locator("...").click()) match because the rightmost identifier in the callee trail is checked, not just the bare callee.src\components\Button.test.tsx) are auto-suppressed because the sharedisTestFilePath()normalizes backslashes.Tests
packages/react-doctor/tests/regressions/js-performance-rules.test.tsadds 10 end-to-end regressions exercising the in-rule guards viarunOxlint():*.browser.tsxrender/findBy/click/findBy stays silent.@playwright/test,@testing-library/react, andvitest/browserimports each silence the rule on production-shaped paths.sleep/animate/fadeInsequences stay silent.await fetchProfile(user.id)chains stay silent.packages/react-doctor/tests/merge-and-filter-diagnostics.test.tsadds 5 cases exercising thetest-noisetag throughmergeAndFilterDiagnostics:*.test.tsx,__tests__/,playwright//cypress//e2e/, Windows-slashed paths, and a production path control.pnpm test,pnpm lint,pnpm typecheck,pnpm format:check) is green: 84 test files, 995 tests.Refs: TODOS.md "Suppress async parallelization advice in tests and ordered UI flows".
Eval Results
Ran against 100-repo corpus (baseline: main @ 529015d, 13,279 diagnostics).
Parity: -204 — All removals are
async-parallelfalse positives in test/e2e files. Zero regressions.async-parallelonlyBreakdown by repo
pierrecomputer/pierretest/e2e/*.pw.ts,scripts/profileFileTree.tstwentyhq/twentytest/integration/**/*.integration-spec.tsaidenybai/react-grabe2e/fixtures.tsPostHog/posthogsrc/test/insight-testing/interactions.tsPostHog/posthog.storybook/test-runner.tstwentyhq/twentyuseResetVirtualizationBecauseDataChanged.tsAll 204 removed hits were sequential awaits in Playwright tests, integration test fixtures, or browser fixtures — exactly the false positives this PR targets.