Conversation
📝 WalkthroughWalkthroughA location selection feature is added to the confetti action type. The backend introduces a location property with nine directional options (center, top, right, bottom, left, top-left, top-right, bottom-left, bottom-right). The frontend dynamically calculates confetti origin coordinates based on the selected location and target element. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
🤖 Pull request artifacts
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/action-types/frontend/confetti.js (1)
34-41: Consider clamping origin values for off-screen elements.When the target element is partially or fully outside the viewport,
rect.leftorrect.topcan be negative (or larger than viewport), potentially producing origin values outside the[0, 1]range. Depending on how the confetti library handles out-of-bounds origins, this could cause unexpected behavior.🛡️ Optional defensive fix
if ( targetEl !== window && targetEl !== document ) { // Get the percentage of the center of the target element to the screen width const rect = targetEl.getBoundingClientRect() - origin.x = ( rect.left + ( rect.width * pos.x ) ) / window.innerWidth - origin.y = ( rect.top + ( rect.height * pos.y ) ) / window.innerHeight + origin.x = Math.max( 0, Math.min( 1, ( rect.left + ( rect.width * pos.x ) ) / window.innerWidth ) ) + origin.y = Math.max( 0, Math.min( 1, ( rect.top + ( rect.height * pos.y ) ) / window.innerHeight ) ) } else { origin = pos }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/action-types/frontend/confetti.js` around lines 34 - 41, The origin calculation can yield values outside [0,1] for off-screen targets; after computing origin.x and origin.y in the block that uses targetEl.getBoundingClientRect() (using rect, pos, origin), clamp each with Math.max(0, Math.min(1, value)) so origin.x = Math.max(0, Math.min(1, (rect.left + rect.width * pos.x) / window.innerWidth)) and similarly for origin.y; keep the fallback else branch (origin = pos) as-is or also clamp pos if desired.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/action-types/frontend/confetti.js`:
- Around line 34-41: The origin calculation can yield values outside [0,1] for
off-screen targets; after computing origin.x and origin.y in the block that uses
targetEl.getBoundingClientRect() (using rect, pos, origin), clamp each with
Math.max(0, Math.min(1, value)) so origin.x = Math.max(0, Math.min(1, (rect.left
+ rect.width * pos.x) / window.innerWidth)) and similarly for origin.y; keep the
fallback else branch (origin = pos) as-is or also clamp pos if desired.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b429a79b-a07e-4b78-952a-09bec61de478
📒 Files selected for processing (2)
src/action-types/class-action-type-confetti.phpsrc/action-types/frontend/confetti.js
fixes #17
Summary by CodeRabbit