Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Maintenance: Clean up timeout in a11y context component#33178

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
yannbf wants to merge1 commit intonext
base:next
Choose a base branch
Loading
fromyann/resolve-more-test-flake

Conversation

@yannbf
Copy link
Member

@yannbfyannbf commentedNov 26, 2025•
edited by coderabbitaibot
Loading

Closes #

What I did

This PR attempts to add a cleanup in the A11yContext component which seems to be causing the flake we've seen in unit tests, which throws an unhandled error trying to accesswindow when that doesn't exist. Seems to be related to the timeout in this component.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to addci:normal,ci:merged orci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.ts

  • Make sure this PR containsone of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentationonly changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the@storybookjs/core team here.

core team members can create a canary releasehere or locally withgh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>

Summary by CodeRabbit

  • Refactor
    • Internal improvements to accessibility context timing management to enhance stability and resource handling.

✏️ Tip: You can customize this high-level summary in your review settings.

@yannbfyannbf self-assigned thisNov 26, 2025
@yannbfyannbf added maintenanceUser-facing maintenance tasks ci:normal labelsNov 26, 2025
@nx-cloud
Copy link

nx-cloudbot commentedNov 26, 2025•
edited
Loading

View yourCI Pipeline Execution ↗ for commite51db96

CommandStatusDurationResult
nx run-many -t build --parallel=3✅ Succeeded47sView ↗

☁️Nx Cloud last updated this comment at2025-11-26 07:16:04 UTC

@coderabbitai
Copy link
Contributor

📝 Walkthrough

Walkthrough

AddeduseRef to manage timeout references in A11yContext component. The changes replace inlinesetTimeout calls with a stored timeout reference (timeoutRef), ensuring proper cleanup of pending timeouts before setting new ones and on component unmount via a cleanup effect.

Changes

Cohort / File(s)Summary
Timeout ref management
code/addons/a11y/src/components/A11yContext.tsx		AddeduseRef import; introducedtimeoutRef to store timeout IDs; modifiedhandleResult to clear previous timeout before setting new one and reset ref afterward; added cleanup effect to clear timeout on unmount

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Verify thatuseRef cleanup effect properly clears the timeout and won't cause memory leaks
  • Confirm the ref is correctly nullified and reused across multiplehandleResult calls
  • Check that no race conditions exist between timeout clearing and setting in rapid invocations
✨ Finishing touches
  • 📝 Generate docstrings

Comment@coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitaicoderabbitaibot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
code/addons/a11y/src/components/A11yContext.tsx (2)

2-2:Timeout ref and unmount cleanup are solid; consider a more portable timer type

The ref-based timeout management and unmount cleanup look correct and should address the flaky post-unmount callback behavior. To avoid tying this browser component to Node typings, you might prefer:

consttimeoutRef=useRef<ReturnType<typeofsetTimeout>|null>(null);

which works regardless of whether@types/node is in scope and keeps the type aligned with the actualsetTimeout implementation.

Also applies to: 129-129, 439-447

207-227:Debounced status transition + scroll logic look good; optional alignment with other timeouts

The clear‑then‑set pattern aroundsetTimeout inhandleResult is correct and ensures only the latest run schedules theready transition and scroll, while the ref cleanup prevents callbacks after unmount. If you want full consistency and to avoid any future unmount‑related flakes, you could also route the 100mssetTimeout inhandleSelect through a ref + cleanup pattern, though that’s optional and outside this PR’s main goal.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between13d4773 ande51db96.

📒 Files selected for processing (1)
  • code/addons/a11y/src/components/A11yContext.tsx (5 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,mjs}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use camelCase for variable and function names

Files:

  • code/addons/a11y/src/components/A11yContext.tsx
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Enable TypeScript strict mode
Export functions from modules for testing purposes

Files:

  • code/addons/a11y/src/components/A11yContext.tsx
**/*.{ts,tsx,js,jsx,json,html,mjs}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx,js,jsx,json,html,mjs}: Use ESLint and Prettier for code style enforcement
Run 'yarn prettier --write ' to format code after making changes
Run 'yarn lint:js:cmd ' to check for ESLint issues after making changes

Files:

  • code/addons/a11y/src/components/A11yContext.tsx
code/**/!(*.test).{ts,tsx,js,mjs}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

code/**/!(*.test).{ts,tsx,js,mjs}: Use 'logger' from 'storybook/internal/node-logger' for server-side (Node.js) logging, not console.log/console.warn/console.error
Use 'logger' from 'storybook/internal/client-logger' for client-side (browser) logging, not console.log/console.warn/console.error
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size

Files:

  • code/addons/a11y/src/components/A11yContext.tsx
🧠 Learnings (5)
📓 Common learnings
Learnt from: SidnioulzRepo: storybookjs/storybook PR: 32458File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227Timestamp: 2025-11-05T09:36:55.944ZLearning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
📚 Learning: 2025-11-05T09:36:55.944Z
Learnt from: SidnioulzRepo: storybookjs/storybook PR: 32458File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227Timestamp: 2025-11-05T09:36:55.944ZLearning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.

Applied to files:

  • code/addons/a11y/src/components/A11yContext.tsx
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: SidnioulzRepo: storybookjs/storybook PR: 32458File: code/core/src/components/components/Select/Select.tsx:200-204Timestamp: 2025-11-05T09:38:47.712ZLearning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.

Applied to files:

  • code/addons/a11y/src/components/A11yContext.tsx
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: SidnioulzRepo: storybookjs/storybook PR: 32458File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96Timestamp: 2025-11-05T09:37:25.920ZLearning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.

Applied to files:

  • code/addons/a11y/src/components/A11yContext.tsx
📚 Learning: 2025-11-24T17:49:47.649Z
Learnt from: CRRepo: storybookjs/storybook PR: 0File: .github/copilot-instructions.md:0-0Timestamp: 2025-11-24T17:49:47.649ZLearning: The Storybook repository is a TypeScript/React monorepo with main codebase in 'code/' directory and tooling in 'scripts/' directory

Applied to files:

  • code/addons/a11y/src/components/A11yContext.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: normal
  • GitHub Check: Core Unit Tests, windows-latest

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@coderabbitaicoderabbitai[bot]coderabbitai[bot] left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@yannbfyannbf

Labels

ci:normalmaintenanceUser-facing maintenance tasks

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@yannbf
[8]ページ先頭
©2009-2025 Movatter.jp