fix: change play once from being per timeline, to per trigger #11
Conversation
WalkthroughThis change refactors the "play once" interaction state tracking in the Timeline class from a single global flag to per-trigger tracking using a WeakSet, enabling "play once" to function correctly when the same interaction targets multiple elements. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![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: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/frontend/scripts/class-timeline.js
🔇 Additional comments (1)
src/frontend/scripts/class-timeline.js (1)
27-29: Good use of WeakSet for per-trigger tracking.Using a WeakSet to track played triggers is an excellent choice because it allows garbage collection of DOM elements that are removed from the page, preventing memory leaks. The comment clearly explains the purpose.
| const currentTrigger = this.interaction.getCurrentTrigger() | ||
| if ( this.getRunner().isFrontend ) { | ||
| if ( this.timelineData.onceOnly && this.timelineData._played ) { | ||
| if ( this.timelineData.onceOnly && this.timelineData._playedTriggers.has( currentTrigger ) ) { | ||
| return null | ||
| } | ||
| } | ||
|
|
||
| this.timelineData._playedTriggers.add( currentTrigger ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Finding getCurrentTrigger implementation ==="
find . -name "*.js" -type f | head -20
echo ""
echo "=== Searching for getCurrentTrigger usage and definition ==="
rg -n "getCurrentTrigger" --type=js -B2 -A5Repository: gambitph/Interactions
Length of output: 10296
🏁 Script executed:
#!/bin/bash
echo "=== Reading class-interaction.js around getCurrentTrigger ==="
sed -n '100,130p' src/frontend/scripts/class-interaction.js
echo ""
echo "=== Reading class-timeline.js around the WeakSet usage ==="
sed -n '40,60p' src/frontend/scripts/class-timeline.js
echo ""
echo "=== Checking how triggerElements is set ==="
rg -n "triggerElements" src/frontend/scripts/class-interaction.js -B2 -A2
echo ""
echo "=== Checking initialization of timelineData._playedTriggers ==="
rg -n "_playedTriggers" src/frontend/scripts/class-timeline.js -B2 -A2Repository: gambitph/Interactions
Length of output: 4174
🏁 Script executed:
#!/bin/bash
echo "=== Searching for where _currentTrigger is set ==="
rg -n "_currentTrigger" src/frontend/scripts/class-interaction.js -B2 -A2
echo ""
echo "=== Checking cloneWithTrigger implementation ==="
rg -n "cloneWithTrigger" src/frontend/scripts/class-interaction.js -B2 -A8
echo ""
echo "=== Checking if triggerElements can be undefined or empty ==="
sed -n '70,90p' src/frontend/scripts/class-interaction.jsRepository: gambitph/Interactions
Length of output: 2272
🏁 Script executed:
#!/bin/bash
echo "=== Check if getTargets can return empty array ==="
rg -n "getTargets" src/frontend/scripts/class-interaction.js -A10 | head -40
echo ""
echo "=== Verify what happens in non-cloned instance scenario ==="
rg -n "createInstance\|new Timeline" src/frontend/scripts/class-interaction.js -B2 -A2
echo ""
echo "=== Check initialization context for timelines ==="
sed -n '15,35p' src/frontend/scripts/class-timeline.jsRepository: gambitph/Interactions
Length of output: 2834
WeakSet operations will throw TypeError if getCurrentTrigger() returns undefined.
WeakSet only accepts objects as values; passing primitives or non-objects like undefined will result in a TypeError. The getCurrentTrigger() method (src/frontend/scripts/class-interaction.js:114) can return undefined when _currentTrigger is undefined and triggerElements is empty or falsy.
In class-timeline.js:48, the WeakSet .has() check only executes in frontend mode with onceOnly enabled, but line 53's .add() call executes unconditionally. If currentTrigger is undefined, line 53 will throw a TypeError regardless of the frontend mode or timeline configuration, crashing the interaction.
Additionally, confirm that non-page interactions without defined triggers should always have at least a fallback trigger element to avoid this edge case entirely.
fixes #5
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.