Skip to content

test: clean up React trees before Vitest teardown - #84

Closed
waterbro-8 wants to merge 1 commit into
bytefolk:mainfrom
waterbro-8:fix/vitest-react-cleanup
Closed

waterbro-8 wants to merge 1 commit into
bytefolk:mainfrom
waterbro-8:fix/vitest-react-cleanup

Conversation

@waterbro-8

Copy link
Copy Markdown
Contributor

Outcome

Fixes the post-merge CI regression on current main after the Next 16 / React 19 migration.

The main push run https://github.com/bytefolk/doc/actions/runs/35496401031 completed all 350 assertions, then failed with an unhandled React commit from share-doc-button.test.tsx after the JSDOM environment had started tearing down.

Root cause

Vitest globals are disabled, so React Testing Library cannot auto-register its cleanup hook. Several component tests rely on that implicit hook. React 19 transitions can still be pending after an assertion succeeds, leaving a mounted tree to commit against a disposed JSDOM window.

Change

Register afterEach(cleanup) once in vitestSetup.ts so every rendered tree is unmounted before environment teardown. This also covers the same leak class across component tests instead of special-casing one test.

Verification

  • Existing red evidence: main CI run 35496401031, 67 files and 350 tests passed but one unhandled error failed the job.
  • git diff --check passes.
  • Hosted PR CI is the green verification because this workspace intentionally did not use npm CLI.

Refs #51. This PR only restores the migration's post-merge test signal; it does not close #51 until CI is green on this head.

@waterbro-8

Copy link
Copy Markdown
Contributor Author

@PeterGuy326 hosted verification is complete on exact head dcfb0d2: test, docker-build, Dependency review, and Scorecard pass; fork CodeQL is skipped. The current account lacks RequestReviewsByLogin permission, so I cannot add you through the reviewer picker. Please perform the independent review and merge if the one-file cleanup change is acceptable; this unblocks the final #51 close.

@waterbro-8

Copy link
Copy Markdown
Contributor Author

当前账号是本 PR 作者,不能自审。Exact head dcfb0d2a5fc547a87afbdac5448f5ea2f35d845a 的 test/docker-build/dependency/scorecard 已绿;@Bindy-lbb @PeterGuy326 请完成独立 CODEOWNER review。若安全门禁要求 exact-head CodeQL,请先由 origin write 维护者镜像该 fork head 到 bytefolk/doc 同源分支运行 CodeQL,再合入。

@waterbro-8

Copy link
Copy Markdown
Contributor Author

作者是 waterbro-8,我不能对本 PR 做 CODEOWNER 自审,也不能合。

exact head dcfb0d2a5fc547a87afbdac5448f5ea2f35d845a。hosted test/docker 绿。fork CodeQL SKIPPED。closingIssuesReferences 含 #51。

请独立 CODEOWNER(胡奕舟 / 李彬彬)review。若坚持同源 CodeQL 门禁,需要有 origin write 的人把 exact head 镜像到 bytefolk/doc。不是产品验收。

@sun-970 sun-970 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review of exact head. Not product验收. I am not merging.

Blocker

This PR is a fork (waterbro-8/doc). Required CodeQL is SKIPPED. Fail-closed: I cannot APPROVE while CodeQL did not run on this SHA.

The afterEach(cleanup) change in vitestSetup.ts is the right fix for Vitest-without-globals + React 19 teardown. Re-review after this SHA is on bytefolk/doc so CodeQL actually runs, or after a same-repo PR with CodeQL green.

I am not merging. Do not Close/Fixes linked issues from this comment.

@PeterGuy326

Copy link
Copy Markdown
Contributor

Same-repository replacement opened at #105 on exact head dcfb0d2a5fc547a87afbdac5448f5ea2f35d845a, so CodeQL can run. Keep this fork PR open until #105 finishes CI and exact-head review.

@PeterGuy326

Copy link
Copy Markdown
Contributor

Closing as superseded. The same-repository replacement opened at #105 carries exactly this change on exact head dcfb0d2a5f (so CodeQL could run), finished CI and was merged on 2026-09-28 as c298143e. main's vitestSetup.ts already contains the afterEach(cleanup) registration added here, so this fork branch has nothing left to land. The branch stays available if anything needs to be re-picked.

@PeterGuy326 PeterGuy326 closed this Oct 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Maintenance] Migrate Next.js 14 → 16 (supersedes dependabot PR #49)

3 participants