fix/14 editor show template #15
Open
Conversation
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
…hanged id; wait for anchor tracking
Contributor
📝 WalkthroughWalkthroughIntroduces rendering-mode–aware initialization and persistent starting styles in the timeline runner hook, and refactors anchor tracking to remove duplicates by tracking the previous anchor reference and cleaning up accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Hook as useTimelineRunnerRef
participant Store as WP Data Store
participant RAF as requestAnimationFrame
participant Runner as Runner/init logic
participant Anchors as Anchor replacements
Hook->>Store: read renderingMode
alt renderingMode changed
Hook->>RAF: requestAnimationFrame(callback)
RAF-->>Hook: callback
Hook->>Anchors: replace anchors
Hook->>Runner: configure & init
Runner-->>Hook: starting styles
else renderingMode unchanged
Hook->>Anchors: replace anchors
Hook->>Runner: configure & init
Runner-->>Hook: starting styles
end
Hook-->>Hook: set initialStyles state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/editor/components/timeline/runner.js`:
- Around line 67-76: The current code returns
runnerRef.current.getStartingActionStyles() synchronously even when initRunner
is deferred, producing stale/empty styles; move the renderingMode change
handling out of this synchronous block into a useEffect that watches
renderingMode and calls initRunner (using setTimeout if needed), then update
local state (e.g., startingStyles) or trigger a re-render when initRunner
completes; change the return to read from that state (instead of calling
getStartingActionStyles() directly) so styles are computed after initRunner
finishes—refer to initRunner, prevRenderingMode.current, renderingMode, and
runnerRef.current.getStartingActionStyles when locating where to apply this
change.
In `@src/editor/components/timeline/with-tracked-anchors.js`:
- Around line 15-43: The effect is missing cleanup and skips removing a previous
anchor when the current anchor becomes undefined; update the useEffect
(referencing useEffect, prevAnchorRef.current, ANCHORS, CLIENT_IDS,
props.clientId, props.attributes?.anchor) so that before the early return you
still remove any prevAnchorRef.current entry from ANCHORS/CLIENT_IDS, and return
a cleanup function that removes the current anchor (or prevAnchorRef.current if
undefined) from ANCHORS and CLIENT_IDS on unmount to avoid stale entries; ensure
you update prevAnchorRef.current appropriately after removals.
🧹 Nitpick comments (1)
src/editor/components/timeline/runner.js (1)
70-70: Consider cleanup for the setTimeout.If the component re-renders or unmounts before the timeout fires,
initRunnerwould still execute with potentially stale references. Consider storing the timeout ID and clearing it on cleanup or subsequent renders.Suggestion using a ref to track timeout
const runnerRef = useRef( null ) + const initTimeoutRef = useRef( null ) const renderingMode = select( 'core/editor' ).getRenderingMode() const prevRenderingMode = useRef( renderingMode )Then in the useMemo:
if ( prevRenderingMode.current !== renderingMode ) { + if ( initTimeoutRef.current ) { + clearTimeout( initTimeoutRef.current ) + } - setTimeout( initRunner, 0 ) + initTimeoutRef.current = setTimeout( initRunner, 0 ) } else {
Comment on lines
15
to
43
| useEffect( () => { | ||
| if ( props.attributes.anchor ) { | ||
| // Remove any previous anchor selector for this clientId. | ||
| const index = CLIENT_IDS.indexOf( props.clientId ) | ||
| if ( index !== -1 ) { | ||
| CLIENT_IDS.splice( index, 1 ) | ||
| ANCHORS.splice( index, 1 ) | ||
| const anchor = props.attributes?.anchor | ||
| if ( ! anchor ) { | ||
| return | ||
| } | ||
|
|
||
| // If this block had a previous anchor, remove it. | ||
| if ( prevAnchorRef.current ) { | ||
| const oldIndex = ANCHORS.indexOf( prevAnchorRef.current ) | ||
| if ( oldIndex !== -1 ) { | ||
| ANCHORS.splice( oldIndex, 1 ) | ||
| CLIENT_IDS.splice( oldIndex, 1 ) | ||
| } | ||
| // Keep track of the clientId-anchor pair | ||
| CLIENT_IDS.push( props.clientId ) | ||
| ANCHORS.push( props.attributes.anchor ) | ||
| } | ||
| }, [ | ||
| props.clientId, | ||
| props.attributes?.anchor, | ||
| ] ) | ||
|
|
||
| // Remove any existing entry for this anchor. | ||
| // This ensures no duplication when the blocks are re-rendered. | ||
| const existingIndex = ANCHORS.indexOf( anchor ) | ||
| if ( existingIndex !== -1 ) { | ||
| ANCHORS.splice( existingIndex, 1 ) | ||
| CLIENT_IDS.splice( existingIndex, 1 ) | ||
| } | ||
|
|
||
| ANCHORS.push( anchor ) | ||
| CLIENT_IDS.push( props.clientId ) | ||
|
|
||
| // Store anchor for next render | ||
| prevAnchorRef.current = anchor | ||
| }, [ props.clientId, props.attributes?.anchor ] ) |
Contributor
There was a problem hiding this comment.
Missing cleanup on unmount and when anchor is removed.
Two issues with the effect:
-
No cleanup function: When the component unmounts, the anchor entry remains in the global arrays, causing stale data.
-
Early return doesn't clean up previous anchor: If an anchor is removed from a block (becomes undefined), the early return at line 17-19 skips removing the old entry stored in
prevAnchorRef.current.
Proposed fix
useEffect( () => {
const anchor = props.attributes?.anchor
+
+ // If anchor was removed, clean up the previous entry
if ( ! anchor ) {
+ if ( prevAnchorRef.current ) {
+ const oldIndex = ANCHORS.indexOf( prevAnchorRef.current )
+ if ( oldIndex !== -1 ) {
+ ANCHORS.splice( oldIndex, 1 )
+ CLIENT_IDS.splice( oldIndex, 1 )
+ }
+ prevAnchorRef.current = undefined
+ }
return
}
// If this block had a previous anchor, remove it.
if ( prevAnchorRef.current ) {
const oldIndex = ANCHORS.indexOf( prevAnchorRef.current )
if ( oldIndex !== -1 ) {
ANCHORS.splice( oldIndex, 1 )
CLIENT_IDS.splice( oldIndex, 1 )
}
}
// Remove any existing entry for this anchor.
// This ensures no duplication when the blocks are re-rendered.
const existingIndex = ANCHORS.indexOf( anchor )
if ( existingIndex !== -1 ) {
ANCHORS.splice( existingIndex, 1 )
CLIENT_IDS.splice( existingIndex, 1 )
}
ANCHORS.push( anchor )
CLIENT_IDS.push( props.clientId )
// Store anchor for next render
prevAnchorRef.current = anchor
+
+ // Cleanup on unmount
+ return () => {
+ const index = ANCHORS.indexOf( anchor )
+ if ( index !== -1 ) {
+ ANCHORS.splice( index, 1 )
+ CLIENT_IDS.splice( index, 1 )
+ }
+ }
}, [ props.clientId, props.attributes?.anchor ] )🤖 Prompt for AI Agents
In `@src/editor/components/timeline/with-tracked-anchors.js` around lines 15 - 43,
The effect is missing cleanup and skips removing a previous anchor when the
current anchor becomes undefined; update the useEffect (referencing useEffect,
prevAnchorRef.current, ANCHORS, CLIENT_IDS, props.clientId,
props.attributes?.anchor) so that before the early return you still remove any
prevAnchorRef.current entry from ANCHORS/CLIENT_IDS, and return a cleanup
function that removes the current anchor (or prevAnchorRef.current if undefined)
from ANCHORS and CLIENT_IDS on unmount to avoid stale entries; ensure you update
prevAnchorRef.current appropriately after removals.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
fixes #14
Observations:
Issues:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.