Core: Add reentry guard to focus patch #32655
Conversation
- add a reentrancy protection mechanism to the patched getter - use a Set to track active elements within the current call stack and setTimeout to defer cleanup, preventing recursive focus calls. Fixes storybookjs#32603
📝 WalkthroughWalkthroughAdds a runtime patch in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as Test code
participant Elem as HTMLElement instance
participant Proto as HTMLElement.prototype.focus (getter wrapper)
participant Orig as originalFocus
Note over Proto: getter installed once (guarded by patchedFocus)\ntracks currentFocus and focusingElements Set
Test->>Elem: call focus(...)
activate Proto
Proto->>Proto: if focusingElements.has(Elem)?
alt Not present (first entry)
Proto->>Proto: focusingElements.add(Elem)
Proto->>Orig: call originalFocus with bound Elem
Note right of Proto: schedule focusingElements.delete(Elem) on next tick (setTimeout 0)
Orig-->>Proto: return
Proto-->>Test: return result
else Reentrant call
Proto-->>Test: call/return originalFocus directly (avoid recursion)
end
deactivate Proto
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/test/preview.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/core/src/test/preview.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/core/src/test/preview.ts
🔇 Additional comments (2)
code/core/src/test/preview.ts (2)
126-142: LGTM! Reentrancy guard correctly prevents recursive focus calls.The getter logic properly implements the reentrancy protection:
- Detects recursive calls by checking Set membership (lines 128-131)
- Returns
originalFocusto break the cycle when recursion is detected- Defers cleanup via
setTimeout(..., 0)(line 138) to ensure the protection marker persists throughout the entire synchronous call chain- The arrow function correctly captures the lexical
thisfrom the getter contextThis approach aligns with the PR objective of fixing the "Maximum call stack size exceeded" error in the Cally date picker.
111-112: LGTM! Correctly preserves the original focus method.Capturing the original
focusreference before any patching is essential for the reentrancy guard to function correctly. The comment clearly documents this requirement.
valentinpalkovic
added
addon: vitest
test utilities
bug
ci:normal
addon: pseudo-states
and removed
addon: vitest
labels
|
View your CI Pipeline Execution ↗ for commit c1cf4c0
☁️ Nx Cloud last updated this comment at |
Sidnioulz
changed the title
code/core/src/test/preview.ts
Outdated
| // Must save a real, original `focus` method outside of the patch beforehand | ||
| const originalFocus = HTMLElement.prototype.focus; | ||
| let currentFocus = HTMLElement.prototype.focus; | ||
|
|
||
| // Use a Set to track elements that are currently undergoing a focus operation | ||
| const focusingElements = new Set<HTMLElement>(); | ||
|
|
||
| if (!patchedFocus) { |
There was a problem hiding this comment.
The change looks good to me! What do you think of scoping the declarations to the if statement so that there's no chance for the Set to be reinstantiated?
| // Must save a real, original `focus` method outside of the patch beforehand | |
| const originalFocus = HTMLElement.prototype.focus; | |
| let currentFocus = HTMLElement.prototype.focus; | |
| // Use a Set to track elements that are currently undergoing a focus operation | |
| const focusingElements = new Set<HTMLElement>(); | |
| if (!patchedFocus) { | |
| if (!patchedFocus) { | |
| // Must save a real, original `focus` method outside of the patch beforehand | |
| const originalFocus = HTMLElement.prototype.focus; | |
| let currentFocus = HTMLElement.prototype.focus; | |
| // Use a Set to track elements that are currently undergoing a focus operation | |
| const focusingElements = new Set<HTMLElement>(); |
There was a problem hiding this comment.
Correct, and patchedFocus = true also needs to be placed outside, because the Set might not be invoked.
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/core/src/test/preview.ts (1)
115-120: Reentry guard initialization looks correct.The declarations properly capture the original focus method and set up tracking for recursive calls. The
focusingElementsSet and helper variables are correctly scoped for the getter closure defined below.Optional: Consider tighter scoping per yannbf's suggestion.
Moving these declarations inside the
if (!patchedFocus)block (but beforeObject.defineProperties) would limit their scope while still allowing the getter to capture them. This prevents unintended external access and clarifies that they're only used for the patch.if (!patchedFocus) { + // Must save a real, original `focus` method outside of the patch beforehand + const originalFocus = HTMLElement.prototype.focus; + let currentFocus = HTMLElement.prototype.focus; + + // Use a Set to track elements that are currently undergoing a focus operation + const focusingElements = new Set<HTMLElement>(); + Object.defineProperties(HTMLElement.prototype, {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/test/preview.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/core/src/test/preview.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/core/src/test/preview.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/core/src/test/preview.ts
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/test/preview.ts
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.197Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.197Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Keep mock implementations simple and focused
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Implement mock behaviors in beforeEach blocks
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mock implementations outside of beforeEach blocks
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid inline mock implementations within test cases
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock implementations should be placed in beforeEach blocks
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required properties and methods that the test subject uses
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use vi.mocked() to access and implement mock behaviors
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking only a subset of required dependencies
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required dependencies that the test subject uses
📚 Learning: 2025-09-17T08:11:47.197Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.197Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Keep mock implementations simple and focused
Applied to files:
code/core/src/test/preview.ts
📚 Learning: 2025-09-17T08:11:47.197Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.197Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors
Applied to files:
code/core/src/test/preview.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid inline mock implementations within test cases
Applied to files:
code/core/src/test/preview.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mock implementations outside of beforeEach blocks
Applied to files:
code/core/src/test/preview.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Implement mock behaviors in beforeEach blocks
Applied to files:
code/core/src/test/preview.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking only a subset of required dependencies
Applied to files:
code/core/src/test/preview.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required properties and methods that the test subject uses
Applied to files:
code/core/src/test/preview.ts
⏰ 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
🔇 Additional comments (2)
code/core/src/test/preview.ts (2)
128-144: Reentry detection logic correctly prevents infinite recursion.The getter properly detects recursive calls by checking Set membership and returns the original focus method to break the cycle. The setTimeout cleanup ensures protection persists for the entire synchronous call chain while allowing the guard to reset for subsequent independent focus operations.
148-148: LGTM!The flag correctly ensures the focus patch is applied only once across all loader invocations.
|
Thank you so much for your contribution @ia319 !! |
yannbf
added
the
patch:yes
Core: Add reentry guard to focus patch (cherry picked from commit 9d1fc3e)
github-actions
bot
added
the
patch:done
4 participants
Closes #32603
What I did
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
There is an error:
Manual testing
Created a sandbox and added stories for the Cally, Chakra, and React Aria Components libraries.
After testing the pages, everything worked as expected with no errors.
Sandbox link:https://github.com/ia319/storybook-react-vite-focus-sandbox
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH 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.tsMake sure this PR contains one 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: Documentation only 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/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>Summary by CodeRabbit