fix: early return in toolbar button to avoid unneeded calls to useInt…#24
fix: early return in toolbar button to avoid unneeded calls to useInt…#24
Conversation
📝 WalkthroughWalkthroughThe ChangesState Selection Refactoring
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Pull request artifacts
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/editor/plugins/block-toolbar-button/index.js (1)
65-65:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTypo: "Reeset" → "Reset".
- // Reeset the popover mode when the popover closes. + // Reset the popover mode when the popover closes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/editor/plugins/block-toolbar-button/index.js` at line 65, Fix the typo in the comment inside src/editor/plugins/block-toolbar-button/index.js: change "Reeset the popover mode when the popover closes." to "Reset the popover mode when the popover closes." so the inline comment above the popover-handling code (in the block-toolbar-button module) reads correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/editor/plugins/block-toolbar-button/index.js`:
- Around line 36-46: The conditional early-return after computing
hasAnchorAttribute causes hooks (useInteractions, useBlockHasInteraction,
useWithShift, useDispatch, the extra useState and useEffect calls) to be called
only when hasAnchorAttribute is true which breaks the Rules of Hooks; fix by
extracting the hook-using logic into a new inner component (e.g.,
BlockToolbarButtonInner) that accepts props and uses
isPopoverOpen/setIsPopoverOpen, useInteractions, useBlockHasInteraction,
useWithShift, useDispatch, and the useEffect hooks; keep the outer component to
only compute hasAnchorAttribute (using useSelect) and then return either
<BlockEdit {...props} /> when !hasAnchorAttribute or <BlockToolbarButtonInner
{...props} /> when true so hooks are called unconditionally within the inner
component; also correct the typo "Reeset" to "Reset".
---
Outside diff comments:
In `@src/editor/plugins/block-toolbar-button/index.js`:
- Line 65: Fix the typo in the comment inside
src/editor/plugins/block-toolbar-button/index.js: change "Reeset the popover
mode when the popover closes." to "Reset the popover mode when the popover
closes." so the inline comment above the popover-handling code (in the
block-toolbar-button module) reads correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b0d831ea-f083-4123-a16a-e30edce5c4c5
📒 Files selected for processing (1)
src/editor/plugins/block-toolbar-button/index.js
bfintal
left a comment
bfintal
left a comment
There was a problem hiding this comment.
The issue seems to be with the useInteractions hook, so let's fix the hook itself instead of returning early to avoid using the hook
…ditor re-renders and prevent Kadence inspector tab reset
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/editor/hooks/use-interactions.js`:
- Around line 256-258: The selector passed to useSelect (used to compute
interactionsFiltered) currently depends on the whole data object ([data])
causing unnecessary re-runs; change the dependency array to [data.interactions]
so the selector only re-evaluates when interactions actually change. Locate the
useSelect call that defines interactionsFiltered and update its dependency array
accordingly (the callback uses isInteractionShown and data.interactions).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5401ae40-e7b7-410f-b1db-4b7be1448380
📒 Files selected for processing (1)
src/editor/hooks/use-interactions.js
| const interactionsFiltered = useSelect( select => { | ||
| return data.interactions.filter( interaction => isInteractionShown( interaction, select ) ) | ||
| }, [ data ] ) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In @wordpress/datauseSelect, does putting an object like[data]in deps cause recalculation when object identity changes, and is[data.interactions] the recommended dependency when only that field is used?
💡 Result:
Yes, putting an object like [data] in the useSelect dependencies array causes recalculation (i.e., the mapSelect callback is rememoized via useCallback) whenever the object's reference changes, even if its contents are shallowly equal. This is because the dependencies array uses React's standard dependency comparison, which for objects is reference equality (!==), not shallow content equality.[1][2][3] The deps array memoizes the mapSelect function with useCallback(mapSelect, deps), so a new object reference [data] (if data is a new object instance) will trigger recreation of the callback function, causing useSelect to re-evaluate selectors even if store state hasn't changed.[2][4] [data.interactions] is the recommended approach when only that field is used inside the mapSelect callback. Official guidance emphasizes including in deps exactly the values referenced inside the callback (like primitive IDs, search terms, or specific fields), to ensure correctness (callback uses current values) and performance (avoids unnecessary recomputations).[5][1][6][7][3] Using a primitive or specific field avoids unnecessary triggers from unrelated object reference changes. ESLint react-hooks/exhaustive-deps is configured to validate useSelect deps.[4]
Citations:
- 1: https://developer.wordpress.org/news/2024/03/how-to-work-effectively-with-the-useselect-hook/
- 2: https://wordpress.stackexchange.com/questions/388559/useselect-second-parameter
- 3: https://developer.wordpress.org/news/2024/03/how-to-work-effectively-with-the-useselect-hook
- 4: eslint-plugin does not validate dependencies in
useSelectanduseSuspenseSelecthooks WordPress/gutenberg#49897 - 5: https://mintlify.com/wordpress/gutenberg/data/working-with-data
- 6: https://wordpress.stackexchange.com/questions/417114/when-to-use-the-second-parameter-to-useselect
- 7: https://developer.wordpress.org/block-editor/reference-guides/packages/packages-data/?output_format=md
🏁 Script executed:
# Read the file around the mentioned lines to understand the context
sed -n '240,275p' src/editor/hooks/use-interactions.jsRepository: gambitph/Interactions
Length of output: 1220
🏁 Script executed:
# Check the full function to see the first useSelect that creates `data`
sed -n '200,260p' src/editor/hooks/use-interactions.js | head -70Repository: gambitph/Interactions
Length of output: 2027
Narrow the selector dependency to data.interactions to avoid unnecessary re-evaluations.
At line 258, using [ data ] as a dependency makes this selector re-evaluate whenever the entire data object reference changes. Since this callback only accesses data.interactions, the dependency array should be narrowed to [ data.interactions ]. This follows the WordPress @wordpress/data guidance and ensures the selector runs only when the interactions data actually changes.
Suggested fix
const interactionsFiltered = useSelect( select => {
return data.interactions.filter( interaction => isInteractionShown( interaction, select ) )
- }, [ data ] )
+ }, [ data.interactions ] )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const interactionsFiltered = useSelect( select => { | |
| return data.interactions.filter( interaction => isInteractionShown( interaction, select ) ) | |
| }, [ data ] ) | |
| const interactionsFiltered = useSelect( select => { | |
| return data.interactions.filter( interaction => isInteractionShown( interaction, select ) ) | |
| }, [ data.interactions ] ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/editor/hooks/use-interactions.js` around lines 256 - 258, The selector
passed to useSelect (used to compute interactionsFiltered) currently depends on
the whole data object ([data]) causing unnecessary re-runs; change the
dependency array to [data.interactions] so the selector only re-evaluates when
interactions actually change. Locate the useSelect call that defines
interactionsFiltered and update its dependency array accordingly (the callback
uses isInteractionShown and data.interactions).
2 participants
…eractions
fixes #23
Summary by CodeRabbit