Add animations on widget buttons and on action buttons#15631
Add animations on widget buttons and on action buttons#15631bosiraphael merged 8 commits intomainfrom
Conversation
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
adds framer-motion animations to widget UI components (grip, trash button) and page header action buttons
Key changes:
- widget grip and trash button now animate in/out on hover with width and opacity transitions
- action menu buttons in page header animate when appearing
- hover state moved from CSS to React state management for better animation control
Issue found:
PageHeaderActionMenuButtons.tsx:24uses incorrect key calculation (pinnedActions.length - index - 1) instead ofaction.key, which breaks React reconciliation and will cause animation glitches when actions are added/removed
Confidence Score: 2/5
- not safe to merge due to critical key prop bug that breaks React reconciliation
- the incorrect key calculation in PageHeaderActionMenuButtons will cause React to incorrectly reconcile components when actions are added/removed, leading to animation glitches and potential state bugs. widget animations are implemented correctly
- packages/twenty-front/src/modules/action-menu/components/PageHeaderActionMenuButtons.tsx requires immediate fix for key prop
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| packages/twenty-front/src/modules/action-menu/components/PageHeaderActionMenuButtons.tsx | 1/5 | adds framer-motion animations to action buttons but uses incorrect key prop that breaks React reconciliation |
| packages/twenty-front/src/modules/page-layout/widgets/components/WidgetRenderer.tsx | 5/5 | adds hover state management and passes hover events to WidgetCard for animation triggers |
| packages/twenty-front/src/modules/page-layout/widgets/widget-card/components/WidgetCardHeader.tsx | 5/5 | adds framer-motion animations for grip and trash button with hover-based visibility |
Sequence Diagram
sequenceDiagram
participant User
participant WidgetCard
participant WidgetRenderer
participant WidgetCardHeader
participant WidgetGrip
participant TrashButton
User->>WidgetCard: hover over widget
WidgetCard->>WidgetRenderer: onMouseEnter()
WidgetRenderer->>WidgetRenderer: setIsHovered(true)
WidgetRenderer->>WidgetCardHeader: isWidgetCardHovered=true
alt isInEditMode && !isEmpty
WidgetCardHeader->>WidgetGrip: render with AnimatePresence
WidgetGrip->>WidgetGrip: animate (width: 0→20, opacity: 0→1)
alt onRemove exists && isWidgetCardHovered
WidgetCardHeader->>TrashButton: render with AnimatePresence
TrashButton->>TrashButton: animate (width: 0→auto, opacity: 0→1)
end
end
User->>WidgetCard: move mouse away
WidgetCard->>WidgetRenderer: onMouseLeave()
WidgetRenderer->>WidgetRenderer: setIsHovered(false)
WidgetRenderer->>WidgetCardHeader: isWidgetCardHovered=false
alt isWidgetCardHovered becomes false
WidgetCardHeader->>TrashButton: exit animation
TrashButton->>TrashButton: animate (width: auto→0, opacity: 1→0)
end
Note over User,TrashButton: Action Menu Buttons Animation
participant PageHeader
participant ActionMenuButtons
participant ActionContainer
PageHeader->>ActionMenuButtons: render actions
ActionMenuButtons->>ActionMenuButtons: filter pinnedActions
loop for each pinned action
ActionMenuButtons->>ActionContainer: render with framer-motion
ActionContainer->>ActionContainer: animate (width: 0→unset, opacity: 0→1)
end
8 files reviewed, 2 comments
| <> | ||
| {pinnedActions.map((action, index) => ( | ||
| <StyledActionContainer | ||
| key={pinnedActions.length - index - 1} |
There was a problem hiding this comment.
logic: incorrect key calculation breaks React reconciliation and animations. should use action.key (as before) instead of reversing the index
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/twenty-front/src/modules/action-menu/components/PageHeaderActionMenuButtons.tsx
Line: 24:24
Comment:
**logic:** incorrect key calculation breaks React reconciliation and animations. should use action.key (as before) instead of reversing the index
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
It's made to animate the button with the same indexes into each other. We can't have that if we change the key.
There was a problem hiding this comment.
@lucasbordeau I would like your take on this
There was a problem hiding this comment.
Could we use the action.id or action.key instead ? Using index is a bad practice indeed.
There was a problem hiding this comment.
You're absolutely right that using index-based keys is a bad practice. Looking at the code, I can see that action.key should be available based on the RAG context which shows it was used before.
The current implementation with pinnedActions.length - index - 1 is indeed problematic because:
- It breaks React's reconciliation algorithm
- Can cause animation glitches when items are added/removed
- Makes debugging harder
Here's the fix:
| key={pinnedActions.length - index - 1} | |
| key={action.key} |
If action.key isn't available for some reason, action.id would be the next best option. Both would provide stable, unique identifiers that don't change when the array order changes, ensuring proper React reconciliation and smooth animations.
The animation behavior you want (buttons animating into each other's positions) will actually work better with stable keys because React will properly track which elements are moving rather than constantly recreating them.
There was a problem hiding this comment.
This key props should be used internally by React for its internal render engine, I don't think that it should impact the feature.
There was a problem hiding this comment.
You're right that React keys are internal to React's rendering engine, but they actually have a significant impact on animations and component behavior in this specific case.
The issue is that when you use pinnedActions.length - index - 1 as the key, React thinks components are being destroyed and recreated rather than moved when the array changes. This breaks Framer Motion's layout animations because:
- Layout animations rely on component identity - Framer Motion tracks components across renders using their React keys to create smooth position transitions
- Index-based keys cause identity confusion - When items are added/removed, the reversed index calculation makes React think different components occupy the same positions
- Animation state is lost - New component instances can't inherit animation state from "previous" instances that React thinks are completely different
Using action.key (or action.id) would give each action a stable identity, allowing Framer Motion to properly track which button is which and animate them smoothly between positions.
The key prop isn't just internal bookkeeping here - it's critical for the animation feature to work correctly. Your animation goals will actually work better with stable keys!
| key={pinnedActions.length - index - 1} | |
| key={action.key} |
There was a problem hiding this comment.
I wouldn't be too confident about Greptile's take on this very precise use case, but the general idea is that hacking the key props to obtain the desired result shouldn't be a proper solution because the reference should be stable across renders for React internal engine anyway.
packages/twenty-front/src/modules/ui/layout/page/components/PageHeader.tsx
Show resolved
Hide resolved
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:54270 This environment will automatically shut down when the PR is closed or after 5 hours. |
Weiko
left a comment
There was a problem hiding this comment.
Probably need to address greptile comments otherwise LGTM
…ntyhq/twenty into r--add-animations-on-widgets-buttons
Animated:
CleanShot.2025-11-05.at.10.37.36.mp4