Add expansion with animation with gpui#2323
Conversation
✅ Deploy Preview for hyprnote ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for hyprnote-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughThe notification2 crate was modularized into animation, constants, event, and toast modules. A new StatusToast UI component with builder-style configuration, expand/collapse animation, and event emission was added. Example usage was updated to use the new API and event messages. Changes
Sequence DiagramsequenceDiagram
participant User
participant StatusToast as StatusToast (Component)
participant Animation as AnimationState
participant Emitter as EventEmitter
User->>StatusToast: Click action button
activate StatusToast
StatusToast->>Animation: Start expand animation
Animation->>Animation: Interpolate height (ease_out_quint)
StatusToast->>StatusToast: Render expanded content (clip/opacity)
StatusToast->>Emitter: Emit Accepted
deactivate StatusToast
Emitter-->>User: Delivered event callback
User->>StatusToast: Click dismiss
activate StatusToast
StatusToast->>Animation: Start collapse animation
Animation->>Animation: Interpolate height
StatusToast->>Emitter: Emit Dismissed
deactivate StatusToast
Emitter-->>User: Delivered event callback
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Code Review:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/notification2/src/event.rs (1)
1-4: Add standard trait derives for better usability.The
NotificationEventenum lacks common derives that are typically useful for event types. Consider addingDebugfor logging/debugging,Clonefor event handling patterns, andPartialEq/Eqfor testing and matching.Apply this diff:
+#[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum NotificationEvent { Accepted, Dismissed, }crates/notification2/src/animation.rs (2)
5-7: Consider adding input validation and documentation.The
ease_out_quintfunction lacks documentation and input bounds checking. While the caller intoast.rsline 77 clamps progress with.min(1.0), documenting the expected range (0.0 to 1.0) would improve clarity.Apply this diff:
+/// Applies an ease-out quintic curve to the input. +/// Input `t` is expected to be in the range [0.0, 1.0]. pub(crate) fn ease_out_quint(t: f32) -> f32 { 1.0 - (1.0 - t).powi(5) }
9-13: Consider adding documentation for the animation state.The
AnimationStatestruct would benefit from documentation explaining its purpose and usage in the height animation interpolation.Apply this diff:
+/// Tracks the state of an ongoing height animation. pub(crate) struct AnimationState { pub start_time: Instant, pub from_height: Pixels, pub to_height: Pixels, }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
crates/notification2/examples/test_notification.rs(1 hunks)crates/notification2/src/animation.rs(1 hunks)crates/notification2/src/constants.rs(1 hunks)crates/notification2/src/event.rs(1 hunks)crates/notification2/src/lib.rs(1 hunks)crates/notification2/src/toast.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-26T06:09:32.213Z
Learnt from: CR
Repo: fastrepl/hyprnote PR: 0
File: crates/notification-linux/AGENTS.md:0-0
Timestamp: 2025-11-26T06:09:32.213Z
Learning: Applies to crates/notification-linux/**/*notification-linux*/**/*.{rs,toml} : Use gtk3 (not gtk4) because webkit2gtk-4.1 links to gtk3
Applied to files:
crates/notification2/src/lib.rs
🧬 Code graph analysis (2)
crates/notification2/src/toast.rs (1)
crates/notification2/src/animation.rs (1)
ease_out_quint(5-7)
crates/notification2/src/lib.rs (1)
extensions/shared/types/hypr-extension.d.ts (1)
toast(462-462)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: desktop_ci (linux, depot-ubuntu-24.04-8)
- GitHub Check: desktop_ci (macos, depot-macos-14)
- GitHub Check: desktop_ci (linux, depot-ubuntu-22.04-8)
🔇 Additional comments (13)
crates/notification2/examples/test_notification.rs (2)
8-18: LGTM! Clear demonstration of the builder pattern API.The example effectively demonstrates the new
StatusToastbuilder pattern withproject_name,subtitle,action_label, andexpanded_content. The text updates make the use case more concrete and realistic.
20-33: LGTM! Event handling is correctly implemented.The event subscription pattern correctly handles both
AcceptedandDismissedevents, with appropriate logging messages that reflect the updated action label.crates/notification2/src/constants.rs (1)
5-11: LGTM! Consider whether 200ms is appropriate for the large height change.The constants are well-defined. The height expands from 72px to 500px (~7x increase), which is a substantial change. A 200ms animation duration might feel quite fast for this transition. Consider testing whether a slightly longer duration (e.g., 250-300ms) provides a smoother user experience.
crates/notification2/src/lib.rs (1)
1-9: LGTM! Clean modularization of the crate structure.The refactoring into separate modules (
animation,constants,event,toast) improves code organization while maintaining the public API surface through re-exports. This follows standard Rust practices.crates/notification2/src/toast.rs (9)
13-54: LGTM! Well-designed builder pattern API.The struct and builder methods follow gpui idioms correctly, using
Into<SharedString>for flexible string input and providing sensible defaults. The separation ofexpanded_contentas optional enables both simple and complex notification use cases.
56-71: LGTM! Native-looking shadow implementation.The two-layer shadow approach effectively creates depth and follows platform conventions for notification shadows.
73-98: LGTM! Correct animation interpolation logic.The height calculation methods properly handle both animated and static states. The
expanded_content_heightcorrectly accounts for the collapsed height plus the 24px toggle bar, andcurrent_content_clip_heightsafely handles the transition with.max(0.0).
100-129: LGTM! Proper window positioning for notification toast.The
window_optionscorrectly positions the toast in the top-right corner with appropriate margins. The window configuration (PopUpkind, no focus, not movable, transparent background) is suitable for a non-intrusive notification.
131-177: LGTM! Robust animation loop with proper interruption handling.The animation implementation correctly handles:
- Interruption mid-animation by capturing
current_animated_height()as the starting point (line 132)- Progressive window resizing during the animation (line 164-167)
- Cleanup when animation completes (line 169-170)
- Frame scheduling with
cx.on_next_framepattern
179-179: LGTM! Standard gpui EventEmitter implementation.
285-291: Action button behavior may be confusing when expanded.The action button has dual behavior:
- When collapsed with expandable content: expands the notification
- When expanded: emits
AcceptedeventThis means the button changes function after expansion. Users might expect it to perform the same action (expand) again or become disabled. Consider whether this is the intended UX, as it could be confusing.
If this behavior is intentional, consider adding a visual cue (e.g., changing the button label or icon) when the notification is expanded to indicate the changed behavior.
309-372: LGTM! Smooth expand/collapse animation with proper clipping.The expanded content rendering correctly implements:
- Conditional rendering based on expansion state or active animation
- Opacity fade-in calculated from clip progress (lines 314-320)
- Height clipping to create smooth expansion effect (line 356)
- Interactive collapse toggle with clear affordance (lines 330-349)
376-391: LGTM! Clean content rendering helper.The
render_expanded_contentmethod correctly styles the expanded content with appropriate visual separation from the header. Theunwrap_or_default()is safe since this is only called whenexpanded_content.is_some().
Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/notification2/src/toast.rs (1)
105-107: Consider extracting magic number to a constant.The
px(24.)represents the expand toggle bar height. For maintainability, consider adding this toconstants.rs.+// In constants.rs +pub(crate) const EXPAND_TOGGLE_HEIGHT: Pixels = px(24.); // In toast.rs fn expanded_content_height(&self) -> Pixels { - NOTIFICATION_HEIGHT_EXPANDED - NOTIFICATION_HEIGHT_COLLAPSED - px(24.) + NOTIFICATION_HEIGHT_EXPANDED - NOTIFICATION_HEIGHT_COLLAPSED - EXPAND_TOGGLE_HEIGHT }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/notification2/src/lib.rs(1 hunks)crates/notification2/src/toast.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-26T06:09:32.213Z
Learnt from: CR
Repo: fastrepl/hyprnote PR: 0
File: crates/notification-linux/AGENTS.md:0-0
Timestamp: 2025-11-26T06:09:32.213Z
Learning: Applies to crates/notification-linux/**/*notification-linux*/**/*.{rs,toml} : Use gtk3 (not gtk4) because webkit2gtk-4.1 links to gtk3
Applied to files:
crates/notification2/src/lib.rs
🧬 Code graph analysis (2)
crates/notification2/src/lib.rs (1)
extensions/shared/types/hypr-extension.d.ts (1)
toast(462-462)
crates/notification2/src/toast.rs (1)
crates/notification2/src/animation.rs (1)
ease_out_quint(5-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: Pages changed - hyprnote
- GitHub Check: desktop_ci (linux, depot-ubuntu-24.04-8)
- GitHub Check: desktop_ci (linux, depot-ubuntu-22.04-8)
- GitHub Check: desktop_ci (macos, depot-macos-14)
🔇 Additional comments (8)
crates/notification2/src/lib.rs (1)
1-9: Clean module organization.The refactoring from inline definitions to a modular structure is well-executed. The public API surface is explicit with re-exports for
NotificationEvent,NotificationTheme, andStatusToast, while keepinganimationandconstantsas internal implementation details.crates/notification2/src/toast.rs (7)
1-19: Imports and theme enum look good.The
NotificationThemeenum is well-designed with appropriate derives. UsingSystemas the default is a sensible choice for respecting user preferences.
21-69: Well-designed builder API.The builder pattern with
impl Into<SharedString>provides a flexible and ergonomic API. Internal state (is_expanded,animation) is correctly kept private.
115-144: Window options are well-configured for a notification popup.The positioning logic correctly places the notification at the top-right corner. The transparent background and popup window kind are appropriate for this use case.
146-191: Solid animation implementation.The animation state machine is correctly structured with proper frame scheduling. The window resizes smoothly during animation, and the loop correctly terminates when complete. Good use of
cx.on_next_frame()for frame-perfect animation.
198-201:Systemtheme always resolves to dark mode.The current implementation treats
NotificationTheme::Systemthe same asDark. If the intent is to respect the actual system appearance, this would need to query the system's dark/light mode setting.Is this intentional, or should
Systemdetect the actual OS appearance? If detection is needed, gpui may provide an API to query this.
319-325: Action button behavior is clear.The click handler correctly handles the two-stage interaction: first expand (if content exists), then accept. This provides a natural flow for users to preview content before accepting.
434-448: Clean content rendering helper.The
render_expanded_contentmethod is well-structured and uses safe defaults withunwrap_or_default().
1 participant
Summary
Refactors the
notification2crate to support expandable notifications with smooth animations. Key changes:cx.on_next_frame()patternNotificationThemeenum with System/Light/Dark variants and a.theme()builder method to force light or dark appearanceNote: The
Systemtheme currently defaults to dark becausecx.theme()isn't available in the notification window context. Use.theme(NotificationTheme::Light)to force light theme.Review & Testing Checklist for Human
cargo run -p notification2 --example test_notificationand verify the expand/collapse animation is smooth (200ms ease-out-quint)Acceptedevent - confirm this is the intended UX.theme(NotificationTheme::Light)to the example and verify the frosted glass effect looks correctRecommended test plan: Run the example notification, click the action button to expand, verify animation smoothness, click "Show less" to collapse, then click action button again to confirm it emits the Accepted event.
Notes