Add fade-in/out animation to notification removal #650
Conversation
Enhanced the element_removal_controller to animate notifications with fade-in on mount and fade-out before removal. Updated the notification partial to include initial animation classes and transition properties for a smoother user experience.
|
Related Documentation Checked 1 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds enter/visible/exit animation lifecycle to element removal via a Stimulus controller, updates notification/CTA partials and layout to use the new animation classes and data attributes, adds animation utilities to the design system, and exposes a render_flash_cta helper in the Notifiable concern. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant DOM as NotificationElement
participant Controller as ElementRemovalController
Browser ->> DOM: mount element (data-controller="element-removal", initial classes)
DOM ->> Controller: connect()
Controller ->> DOM: requestAnimationFrame -> replace initial -> visible class
Browser ->> DOM: CSS enter transition runs
Browser ->> DOM: (optional) element animation ends -> animationend event
DOM ->> Controller: animationend -> element-removal#remove (or user click -> remove)
Controller ->> Controller: guard isRemoving -> set isRemoving = true
Controller ->> DOM: swap visible -> exit class
Controller ->> Controller: setTimeout(durationValue)
Controller ->> DOM: remove element from DOM after timeout
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Pull request overview
This PR adds fade-in and fade-out animations to notification removal for a smoother user experience. The notification now fades in when it appears and fades out before being removed from the DOM.
Changes:
- Enhanced the
element_removal_controllerwithconnect()lifecycle method to trigger fade-in animation on mount - Modified the
remove()method to animate fade-out before DOM removal using setTimeout - Updated the notification partial to include initial animation classes (
opacity-0,translate-y-[-8px]) and CSS transition properties
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| app/javascript/controllers/element_removal_controller.js | Added connect() method for fade-in animation and updated remove() method to include fade-out animation with setTimeout |
| app/views/shared/notifications/_notice.html.erb | Added transition classes, initial opacity/transform states, and notification target |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@app/views/shared/notifications/_alert.html.erb`:
- Around line 3-7: The action binding uses animationend but the element uses CSS
transitions, so change the Stimulus action from
"animationend->element-removal#remove" to
"transitionend->element-removal#remove" on the tag.div; also update or ensure
the element-removal controller's remove() method uses its isRemoving guard (or
set and check a removal flag) so duplicate transitionend events (one per
transitioned property) don't trigger removal twice, and keep the existing
setTimeout fallback intact.
In `@app/views/shared/notifications/_cta.html.erb`:
- Around line 4-8: The CTA currently listens for animationend but should mirror
the alert fix: change the data-action on the tag.div from
"animationend->element-removal#remove" to
"transitionend->element-removal#remove" and add a manual close control that
calls the same controller method (e.g., add a close button inside the div with
data-action="click->element-removal#remove") so the element-removal controller's
remove method is invoked either on transition end or via user click.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/javascript/controllers/element_removal_controller.jsapp/views/shared/notifications/_alert.html.erbapp/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_notice.html.erb
🚧 Files skipped from review as they are similar to previous changes (2)
- app/javascript/controllers/element_removal_controller.js
- app/views/shared/notifications/_notice.html.erb
🧰 Additional context used
📓 Path-based instructions (15)
{app/**/*.{erb,html},app/javascript/**/*.js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Prefer Hotwire-first approach: use Turbo + Stimulus for reactive UI without heavy JavaScript
Files:
app/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
app/views/**/_*.html.erb
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use partials only for simple, static HTML with minimal logic in specific contexts
app/views/**/_*.html.erb: Use underscore prefix for partial files (e.g., _trend_change.html.erb, _form_errors.html.erb, _sync_indicator.html.erb)
Place context-specific partials in relevant controller view directory (e.g., accounts/_account_sidebar_tabs.html.erb)
Files:
app/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
app/views/**/*.erb
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
app/views/**/*.erb: Always use theiconhelper for icons in views, never uselucide_icondirectly
Use Tailwind design tokens (e.g., text-primary, bg-container, border-secondary) instead of raw color names
Pass data to Stimulus controllers via data-*-value attributes instead of inline JavaScript
Prefer semantic HTML elements (dialog, details, summary) over JavaScript components
Use Turbo Frames for page sections instead of client-side solutions
Use server-side formatting for currencies, numbers, and dates instead of client-side
app/views/**/*.erb: Avoid heavy logic in ERB views; prefer helpers and components instead
Ensure ERB views passerb-lintchecks as configured in.erb_lint.yml
Files:
app/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
**/*.{css,erb}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use Tailwind CSS v4.x with custom design system for styling
Files:
app/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
**/*.{rb,erb}
📄 CodeRabbit inference engine (.cursor/rules/general-rules.mdc)
**/*.{rb,erb}: UseCurrent.userfor accessing the current user. Do NOT usecurrent_user
UseCurrent.familyfor accessing the current family. Do NOT usecurrent_family
Files:
app/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
**/*.html.erb
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
**/*.html.erb: Prefer native HTML elements over JavaScript-based components. Use semantic HTML: for modals,for disclosures
Leverage Turbo frames to break up pages instead of JavaScript-driven client-side solutions
Use theiconhelper from application_helper.rb for icons. Never uselucide_iconhelper directly
Files:
app/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
**/*.{rb,js,erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Format currencies, numbers, dates, and other values server-side, then pass to Stimulus controllers for display only
Files:
app/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
**/*.{rb,html.erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Use ActiveRecord validations for complex validations and business logic. Simple validations may be mirrored in ActiveRecord for form error handling convenience, but prioritize client-side form validation when possible
Files:
app/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
{app/views/**,app/helpers/**,app/javascript/controllers/**}
📄 CodeRabbit inference engine (.cursor/rules/ui-ux-design-guidelines.mdc)
{app/views/**,app/helpers/**,app/javascript/controllers/**}: Reference maybe-design-system.css for base primitives, functional tokens, and component tokens before writing styles
Prefer using functional design system tokens (e.g., text-primary, bg-container, border-primary) from maybe-design-system.css instead of raw Tailwind color values
Files:
app/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
{app/views/**,app/helpers/**}
📄 CodeRabbit inference engine (.cursor/rules/ui-ux-design-guidelines.mdc)
Always generate semantic HTML
Files:
app/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
**/*.{erb,html.erb}
📄 CodeRabbit inference engine (.cursor/rules/view_conventions.mdc)
**/*.{erb,html.erb}: Use ViewComponents when: element has complex logic or styling patterns, will be reused across multiple views/contexts, needs structured styling with variants/sizes, requires interactive behavior or Stimulus controllers, has configurable slots or complex APIs, or needs accessibility features or ARIA support
Use Partials when: element is primarily static HTML with minimal logic, used in only one or few specific contexts, is simple template content, doesn't need variants/sizes/complex configuration, or is more about content organization than reusable functionality
Prefer components over partials: if a component is available in app/components, use it; if not, look for a partial; if neither exists, decide between component or partial based on complexity and reusability criteria
Keep domain logic out of views: compute values like button classes, conditional logic, and data transformations in the component file, not the template file
Always use the declarative approach when integrating Stimulus controllers in views: the ERB template should declare what happens using data-* attributes, and the Stimulus controller should respond
Component controllers in app/components/ should only be used within their component templates; global controllers in app/javascript/controllers/ can be used across any view
Pass data from Rails to Stimulus using data-*-value attributes, not inline JavaScript
Files:
app/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
**/*.erb
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.erb: In view templates, use native HTML elements preferred over JS components: use<dialog>for modals,<details><summary>for disclosures
Leverage Turbo frames for page sections over client-side solutions in views
Always use server-side formatting for currencies, numbers, and dates in views
Always useiconhelper inapplication_helper.rb. NEVER uselucide_icondirectly in templates.
Always generate semantic HTML in view templates
Keep domain logic OUT of view templates. Logic belongs in component files, not template files.
Files:
app/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
**/*.{erb,js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{erb,js,ts,tsx}: Use query params for state management over localStorage/sessions in views
In TailwindCSS, always referenceapp/assets/tailwind/maybe-design-system.cssfor primitives and tokens
Use functional tokens defined in design system (e.g.,text-primaryinstead oftext-white,bg-containerinstead ofbg-white)
Pass data to Stimulus controllers viadata-*-valueattributes, not inline JavaScript
Files:
app/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
**/*.{erb,rb}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{erb,rb}: All user-facing strings must use localization (i18n). Always uset()helper. Update locale files for each new or changed element.
Use i18n interpolation for dynamic content:t("users.greeting", name: user.name)
Use Rails pluralization in i18n:t("transactions.count", count:@transactions.count)
Files:
app/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
**/*.{js,ts,tsx,erb}
📄 CodeRabbit inference engine (CLAUDE.md)
In Stimulus controllers, use Declarative Actions (HTML declares what happens). Example:
data-action="click->toggle#toggle"
Files:
app/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/stimulus_conventions.mdc:0-0
Timestamp: 2025-11-24T16:56:51.982Z
Learning: Applies to **/*_controller.{js,ts} : Use declarative actions in Stimulus controllers, not imperative event listeners - define actions in ERB templates using data-action attributes instead of binding events in the controller's connect() method
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/stimulus_conventions.mdc:0-0
Timestamp: 2025-09-23T22:22:00.149Z
Learning: Applies to **/*_controller.{js,ts} : In Stimulus controllers, do not imperatively bind DOM events (e.g., addEventListener in connect); expose methods and let views trigger them via actions
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-09-23T22:23:22.963Z
Learning: Applies to app/{views,components}/**/*.html.erb : Integrate Stimulus declaratively in ERB: templates declare data-controller/actions/targets; controllers respond to those declarations
📚 Learning: 2025-11-24T16:56:13.406Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-conventions.mdc:0-0
Timestamp: 2025-11-24T16:56:13.406Z
Learning: Applies to **/*.html.erb : Prefer native HTML elements over JavaScript-based components. Use semantic HTML: <dialog> for modals, <details><summary> for disclosures
Applied to files:
app/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
📚 Learning: 2026-01-10T19:11:44.566Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T19:11:44.566Z
Learning: Applies to **/*.erb : In view templates, use native HTML elements preferred over JS components: use `<dialog>` for modals, `<details><summary>` for disclosures
Applied to files:
app/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
📚 Learning: 2026-01-10T19:11:44.566Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T19:11:44.566Z
Learning: Applies to **/*.{js,ts,tsx,erb} : In Stimulus controllers, use Declarative Actions (HTML declares what happens). Example: `data-action="click->toggle#toggle"`
Applied to files:
app/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
📚 Learning: 2025-09-23T22:23:22.963Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-09-23T22:23:22.963Z
Learning: Applies to app/{views,components}/**/*.html.erb : Integrate Stimulus declaratively in ERB: templates declare data-controller/actions/targets; controllers respond to those declarations
Applied to files:
app/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
📚 Learning: 2025-11-24T16:56:51.982Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/stimulus_conventions.mdc:0-0
Timestamp: 2025-11-24T16:56:51.982Z
Learning: Applies to **/*_controller.{js,ts} : Use declarative actions in Stimulus controllers, not imperative event listeners - define actions in ERB templates using data-action attributes instead of binding events in the controller's connect() method
Applied to files:
app/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
📚 Learning: 2025-11-24T16:54:59.198Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T16:54:59.198Z
Learning: Applies to app/views/**/*.erb : Prefer semantic HTML elements (dialog, details, summary) over JavaScript components
Applied to files:
app/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
📚 Learning: 2025-09-23T22:22:00.149Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/stimulus_conventions.mdc:0-0
Timestamp: 2025-09-23T22:22:00.149Z
Learning: Applies to app/{views,components}/**/*.html.erb : In ERB views, wire Stimulus interactions declaratively using data-action to call controller methods
Applied to files:
app/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
📚 Learning: 2025-11-24T16:57:20.318Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-11-24T16:57:20.318Z
Learning: Applies to **/*.{erb,html.erb} : Always use the declarative approach when integrating Stimulus controllers in views: the ERB template should declare what happens using data-* attributes, and the Stimulus controller should respond
Applied to files:
app/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
📚 Learning: 2025-09-23T22:23:22.963Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-09-23T22:23:22.963Z
Learning: Applies to app/{views,components}/**/*.html.erb : Pass data from Rails to Stimulus using data-*-value attributes, not inline JavaScript
Applied to files:
app/views/shared/notifications/_alert.html.erb
📚 Learning: 2025-11-24T16:54:59.198Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T16:54:59.198Z
Learning: Applies to app/views/**/*.erb : Pass data to Stimulus controllers via data-*-value attributes instead of inline JavaScript
Applied to files:
app/views/shared/notifications/_alert.html.erb
⏰ 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). (1)
- GitHub Check: ci / test
🔇 Additional comments (2)
app/views/shared/notifications/_alert.html.erb (1)
16-18: LGTM on close button.The declarative Stimulus action
click->element-removal#removecorrectly follows the project's convention for wiring interactions via data attributes. Based on learnings, this is the preferred pattern over imperative event listeners.app/views/shared/notifications/_cta.html.erb (1)
9-22: Content structure follows conventions.The nested content uses the
iconhelper, semantic HTML structure, and design system tokens (text-primary,text-secondary,bg-success) as required by coding guidelines.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| <%= tag.div class: "flex gap-3 rounded-lg bg-container p-4 group w-full md:max-w-80 shadow-border-lg transition-all duration-300 ease-out opacity-0 translate-y-[-8px]", | ||
| data: { | ||
| controller: "element-removal", | ||
| action: "animationend->element-removal#remove" | ||
| } do %> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n '@keyframes' --type=cssRepository: we-promise/sure
Length of output: 133
🏁 Script executed:
fd -t f 'element_removal_controller' --exec cat {}Repository: we-promise/sure
Length of output: 902
animationend event won't fire for CSS transitions—this breaks automatic removal after fade-in.
The element uses transition-all duration-300 ease-out classes, which trigger CSS transitions. Transitions fire transitionend events, not animationend. Since no @keyframes animations are defined for notifications, the animationend->element-removal#remove binding will never trigger, so the alert won't auto-dismiss after the fade-in animation completes.
The remove() method has a setTimeout fallback, so manual dismissal via the close button works. However, automatic removal is broken.
Change the action binding to use transitionend:
data: {
controller: "element-removal",
- action: "animationend->element-removal#remove"
+ action: "transitionend->element-removal#remove"
}Note: transitionend fires once per transitioned property (opacity and transform in this case). The controller's isRemoving guard must be applied consistently to prevent duplicate removals.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <%= tag.div class: "flex gap-3 rounded-lg bg-container p-4 group w-full md:max-w-80 shadow-border-lg transition-all duration-300 ease-out opacity-0 translate-y-[-8px]", | |
| data: { | |
| controller: "element-removal", | |
| action: "animationend->element-removal#remove" | |
| } do %> | |
| <%= tag.div class: "flex gap-3 rounded-lg bg-container p-4 group w-full md:max-w-80 shadow-border-lg transition-all duration-300 ease-out opacity-0 translate-y-[-8px]", | |
| data: { | |
| controller: "element-removal", | |
| action: "transitionend->element-removal#remove" | |
| } do %> |
🤖 Prompt for AI Agents
In `@app/views/shared/notifications/_alert.html.erb` around lines 3 - 7, The
action binding uses animationend but the element uses CSS transitions, so change
the Stimulus action from "animationend->element-removal#remove" to
"transitionend->element-removal#remove" on the tag.div; also update or ensure
the element-removal controller's remove() method uses its isRemoving guard (or
set and check a removal flag) so duplicate transitionend events (one per
transitioned property) don't trigger removal twice, and keep the existing
setTimeout fallback intact.
| <%= tag.div class: "relative flex gap-3 rounded-lg bg-container p-4 group w-full md:max-w-80 shadow-border-xs transition-all duration-300 ease-out opacity-0 translate-y-[-8px]", | ||
| data: { | ||
| controller: "element-removal", | ||
| action: "animationend->element-removal#remove" | ||
| } do %> |
There was a problem hiding this comment.
Same animationend issue—CTA has no manual close fallback.
This partial has the same animationend vs transitionend mismatch flagged in _alert.html.erb. Unlike the alert, this CTA has no close button, so if animationend never fires, the notification will persist indefinitely until page navigation.
Apply the same fix:
Suggested fix
data: {
controller: "element-removal",
- action: "animationend->element-removal#remove"
+ action: "transitionend->element-removal#remove"
}🤖 Prompt for AI Agents
In `@app/views/shared/notifications/_cta.html.erb` around lines 4 - 8, The CTA
currently listens for animationend but should mirror the alert fix: change the
data-action on the tag.div from "animationend->element-removal#remove" to
"transitionend->element-removal#remove" and add a manual close control that
calls the same controller method (e.g., add a close button inside the div with
data-action="click->element-removal#remove") so the element-removal controller's
remove method is invoked either on transition end or via user click.
Separates CTA rendering from general notifications, updating the Notifiable concern and layout to handle CTAs in their own container. Refines notification partials for improved styling and interaction, including a close button for CTAs and more precise animation handling in the element_removal_controller. Ensures only relevant animations trigger element removal and improves maintainability of notification components.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@app/controllers/concerns/notifiable.rb`:
- Around line 23-30: The render_flash_cta method double-wraps the CTA because
the _cta.html.erb partial already outputs its own container; update
render_flash_cta to render the partial directly instead of wrapping it in
view_context.tag.div. Specifically, in render_flash_cta (and where it calls
resolve_cta and view_context.render), return the rendered partial HTML as-is (or
mark it html_safe if necessary) rather than calling
view_context.tag.div(view_context.render(**notification)) so Turbo Stream
replacement targets the partial's own `#cta` element correctly.
- Around line 18-19: The helper double-wraps notification partials: the
notifications.map block currently wraps view_context.render(**notification)
inside view_context.tag.div, causing nested <div> containers since
_notice.html.erb and _alert.html.erb already emit their outer div; remove the
extra wrapper by stopping the use of view_context.tag.div in the map and instead
join the rendered partials directly (keep view_context.safe_join and
notifications.map and continue calling view_context.render(**notification) so
the partials themselves supply the outer container).
♻️ Duplicate comments (1)
app/views/shared/notifications/_cta.html.erb (1)
3-4: Close button addresses the past review concern.The CTA partial now includes a manual close button (lines 20-22), which resolves the previous concern about CTAs persisting indefinitely. The lack of
animationendaction appears intentional—CTAs are meant to persist until the user explicitly dismisses them or takes action.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/controllers/concerns/notifiable.rbapp/javascript/controllers/element_removal_controller.jsapp/views/layouts/shared/_htmldoc.html.erbapp/views/shared/notifications/_alert.html.erbapp/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_notice.html.erb
🚧 Files skipped from review as they are similar to previous changes (1)
- app/javascript/controllers/element_removal_controller.js
🧰 Additional context used
📓 Path-based instructions (18)
{app/**/*.{erb,html},app/javascript/**/*.js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Prefer Hotwire-first approach: use Turbo + Stimulus for reactive UI without heavy JavaScript
Files:
app/views/shared/notifications/_notice.html.erbapp/views/layouts/shared/_htmldoc.html.erbapp/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
app/views/**/_*.html.erb
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use partials only for simple, static HTML with minimal logic in specific contexts
app/views/**/_*.html.erb: Use underscore prefix for partial files (e.g., _trend_change.html.erb, _form_errors.html.erb, _sync_indicator.html.erb)
Place context-specific partials in relevant controller view directory (e.g., accounts/_account_sidebar_tabs.html.erb)
Files:
app/views/shared/notifications/_notice.html.erbapp/views/layouts/shared/_htmldoc.html.erbapp/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
app/views/**/*.erb
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
app/views/**/*.erb: Always use theiconhelper for icons in views, never uselucide_icondirectly
Use Tailwind design tokens (e.g., text-primary, bg-container, border-secondary) instead of raw color names
Pass data to Stimulus controllers via data-*-value attributes instead of inline JavaScript
Prefer semantic HTML elements (dialog, details, summary) over JavaScript components
Use Turbo Frames for page sections instead of client-side solutions
Use server-side formatting for currencies, numbers, and dates instead of client-side
app/views/**/*.erb: Avoid heavy logic in ERB views; prefer helpers and components instead
Ensure ERB views passerb-lintchecks as configured in.erb_lint.yml
Files:
app/views/shared/notifications/_notice.html.erbapp/views/layouts/shared/_htmldoc.html.erbapp/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
**/*.{css,erb}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use Tailwind CSS v4.x with custom design system for styling
Files:
app/views/shared/notifications/_notice.html.erbapp/views/layouts/shared/_htmldoc.html.erbapp/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
**/*.{rb,erb}
📄 CodeRabbit inference engine (.cursor/rules/general-rules.mdc)
**/*.{rb,erb}: UseCurrent.userfor accessing the current user. Do NOT usecurrent_user
UseCurrent.familyfor accessing the current family. Do NOT usecurrent_family
Files:
app/views/shared/notifications/_notice.html.erbapp/controllers/concerns/notifiable.rbapp/views/layouts/shared/_htmldoc.html.erbapp/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
**/*.html.erb
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
**/*.html.erb: Prefer native HTML elements over JavaScript-based components. Use semantic HTML: for modals,for disclosures
Leverage Turbo frames to break up pages instead of JavaScript-driven client-side solutions
Use theiconhelper from application_helper.rb for icons. Never uselucide_iconhelper directly
Files:
app/views/shared/notifications/_notice.html.erbapp/views/layouts/shared/_htmldoc.html.erbapp/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
**/*.{rb,js,erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Format currencies, numbers, dates, and other values server-side, then pass to Stimulus controllers for display only
Files:
app/views/shared/notifications/_notice.html.erbapp/controllers/concerns/notifiable.rbapp/views/layouts/shared/_htmldoc.html.erbapp/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
**/*.{rb,html.erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Use ActiveRecord validations for complex validations and business logic. Simple validations may be mirrored in ActiveRecord for form error handling convenience, but prioritize client-side form validation when possible
Files:
app/views/shared/notifications/_notice.html.erbapp/controllers/concerns/notifiable.rbapp/views/layouts/shared/_htmldoc.html.erbapp/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
{app/views/**,app/helpers/**,app/javascript/controllers/**}
📄 CodeRabbit inference engine (.cursor/rules/ui-ux-design-guidelines.mdc)
{app/views/**,app/helpers/**,app/javascript/controllers/**}: Reference maybe-design-system.css for base primitives, functional tokens, and component tokens before writing styles
Prefer using functional design system tokens (e.g., text-primary, bg-container, border-primary) from maybe-design-system.css instead of raw Tailwind color values
Files:
app/views/shared/notifications/_notice.html.erbapp/views/layouts/shared/_htmldoc.html.erbapp/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
{app/views/**,app/helpers/**}
📄 CodeRabbit inference engine (.cursor/rules/ui-ux-design-guidelines.mdc)
Always generate semantic HTML
Files:
app/views/shared/notifications/_notice.html.erbapp/views/layouts/shared/_htmldoc.html.erbapp/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
**/*.{erb,html.erb}
📄 CodeRabbit inference engine (.cursor/rules/view_conventions.mdc)
**/*.{erb,html.erb}: Use ViewComponents when: element has complex logic or styling patterns, will be reused across multiple views/contexts, needs structured styling with variants/sizes, requires interactive behavior or Stimulus controllers, has configurable slots or complex APIs, or needs accessibility features or ARIA support
Use Partials when: element is primarily static HTML with minimal logic, used in only one or few specific contexts, is simple template content, doesn't need variants/sizes/complex configuration, or is more about content organization than reusable functionality
Prefer components over partials: if a component is available in app/components, use it; if not, look for a partial; if neither exists, decide between component or partial based on complexity and reusability criteria
Keep domain logic out of views: compute values like button classes, conditional logic, and data transformations in the component file, not the template file
Always use the declarative approach when integrating Stimulus controllers in views: the ERB template should declare what happens using data-* attributes, and the Stimulus controller should respond
Component controllers in app/components/ should only be used within their component templates; global controllers in app/javascript/controllers/ can be used across any view
Pass data from Rails to Stimulus using data-*-value attributes, not inline JavaScript
Files:
app/views/shared/notifications/_notice.html.erbapp/views/layouts/shared/_htmldoc.html.erbapp/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
**/*.erb
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.erb: In view templates, use native HTML elements preferred over JS components: use<dialog>for modals,<details><summary>for disclosures
Leverage Turbo frames for page sections over client-side solutions in views
Always use server-side formatting for currencies, numbers, and dates in views
Always useiconhelper inapplication_helper.rb. NEVER uselucide_icondirectly in templates.
Always generate semantic HTML in view templates
Keep domain logic OUT of view templates. Logic belongs in component files, not template files.
Files:
app/views/shared/notifications/_notice.html.erbapp/views/layouts/shared/_htmldoc.html.erbapp/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
**/*.{erb,js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{erb,js,ts,tsx}: Use query params for state management over localStorage/sessions in views
In TailwindCSS, always referenceapp/assets/tailwind/maybe-design-system.cssfor primitives and tokens
Use functional tokens defined in design system (e.g.,text-primaryinstead oftext-white,bg-containerinstead ofbg-white)
Pass data to Stimulus controllers viadata-*-valueattributes, not inline JavaScript
Files:
app/views/shared/notifications/_notice.html.erbapp/views/layouts/shared/_htmldoc.html.erbapp/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
**/*.{erb,rb}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{erb,rb}: All user-facing strings must use localization (i18n). Always uset()helper. Update locale files for each new or changed element.
Use i18n interpolation for dynamic content:t("users.greeting", name: user.name)
Use Rails pluralization in i18n:t("transactions.count", count:@transactions.count)
Files:
app/views/shared/notifications/_notice.html.erbapp/controllers/concerns/notifiable.rbapp/views/layouts/shared/_htmldoc.html.erbapp/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
**/*.{js,ts,tsx,erb}
📄 CodeRabbit inference engine (CLAUDE.md)
In Stimulus controllers, use Declarative Actions (HTML declares what happens). Example:
data-action="click->toggle#toggle"
Files:
app/views/shared/notifications/_notice.html.erbapp/views/layouts/shared/_htmldoc.html.erbapp/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
**/*.rb
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rb: Application supports two modes: 'managed' and 'self_hosted' via Rails.application.config.app_mode
Use Current.user and Current.family instead of current_user / current_family for authentication context
Optimize database queries with proper indexes to prevent N+1 queries using includes/joins
**/*.rb: Use 2-space indentation for Ruby code
Usesnake_casefor method and variable names in Ruby
UseCamelCasefor class and module names in Ruby
Runbin/rubocopbefore pushing code; use-Aflag to auto-correct safe style violations
**/*.rb: UseCurrent.userfor the current user. Do NOT usecurrent_user.
UseCurrent.familyfor the current family. Do NOT usecurrent_family.
Files:
app/controllers/concerns/notifiable.rb
app/controllers/**/*.rb
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use Rails strong parameters and CSRF protection throughout the application
Files:
app/controllers/concerns/notifiable.rb
app/**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Follow Rails conventions for folder and file names in the app directory
Files:
app/controllers/concerns/notifiable.rb
🧠 Learnings (18)
📓 Common learnings
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/stimulus_conventions.mdc:0-0
Timestamp: 2025-11-24T16:56:51.982Z
Learning: Applies to **/*_controller.{js,ts} : Use declarative actions in Stimulus controllers, not imperative event listeners - define actions in ERB templates using data-action attributes instead of binding events in the controller's connect() method
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-09-23T22:23:22.963Z
Learning: Applies to app/{views,components}/**/*.html.erb : Integrate Stimulus declaratively in ERB: templates declare data-controller/actions/targets; controllers respond to those declarations
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/stimulus_conventions.mdc:0-0
Timestamp: 2025-09-23T22:22:00.149Z
Learning: Applies to app/{views,components}/**/*.html.erb : In ERB views, wire Stimulus interactions declaratively using data-action to call controller methods
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/stimulus_conventions.mdc:0-0
Timestamp: 2025-09-23T22:22:00.149Z
Learning: Applies to **/*_controller.{js,ts} : In Stimulus controllers, do not imperatively bind DOM events (e.g., addEventListener in connect); expose methods and let views trigger them via actions
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-11-24T16:57:20.318Z
Learning: Applies to **/*.{erb,html.erb} : Always use the declarative approach when integrating Stimulus controllers in views: the ERB template should declare what happens using data-* attributes, and the Stimulus controller should respond
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T16:54:59.198Z
Learning: Applies to app/javascript/controllers/**/*_controller.js : Keep Stimulus controllers small with private methods and clear public API
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T19:11:44.566Z
Learning: Applies to **/*.{js,ts,tsx,erb} : In Stimulus controllers, use Declarative Actions (HTML declares what happens). Example: `data-action="click->toggle#toggle"`
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T16:54:59.198Z
Learning: Applies to app/components/**/*.rb : Use viewComponents and Stimulus controllers organized alongside components in app/components/
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/stimulus_conventions.mdc:0-0
Timestamp: 2025-09-23T22:22:00.149Z
Learning: Applies to **/*_controller.{js,ts} : Keep Stimulus controllers focused on UI behavior; avoid domain/business logic and prefer single, closely related responsibilities; use Stimulus callbacks, actions, targets, values, and classes
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-09-23T22:23:22.963Z
Learning: Applies to app/views/**/*.html.erb : Use ViewComponents when elements have complex logic/styling, are reused across contexts, need variants/slots/accessibility, or require Stimulus interactivity
📚 Learning: 2025-11-24T16:56:13.406Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-conventions.mdc:0-0
Timestamp: 2025-11-24T16:56:13.406Z
Learning: Applies to **/*.html.erb : Prefer native HTML elements over JavaScript-based components. Use semantic HTML: <dialog> for modals, <details><summary> for disclosures
Applied to files:
app/views/shared/notifications/_notice.html.erbapp/views/layouts/shared/_htmldoc.html.erbapp/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
📚 Learning: 2026-01-10T19:11:44.566Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T19:11:44.566Z
Learning: Applies to **/*.erb : In view templates, use native HTML elements preferred over JS components: use `<dialog>` for modals, `<details><summary>` for disclosures
Applied to files:
app/views/shared/notifications/_notice.html.erbapp/views/layouts/shared/_htmldoc.html.erbapp/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
📚 Learning: 2025-11-24T16:54:59.198Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T16:54:59.198Z
Learning: Applies to app/views/**/*.erb : Prefer semantic HTML elements (dialog, details, summary) over JavaScript components
Applied to files:
app/views/shared/notifications/_notice.html.erbapp/views/layouts/shared/_htmldoc.html.erbapp/views/shared/notifications/_cta.html.erbapp/views/shared/notifications/_alert.html.erb
📚 Learning: 2025-11-24T16:57:20.318Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-11-24T16:57:20.318Z
Learning: Applies to **/*.{erb,html.erb} : Use ViewComponents when: element has complex logic or styling patterns, will be reused across multiple views/contexts, needs structured styling with variants/sizes, requires interactive behavior or Stimulus controllers, has configurable slots or complex APIs, or needs accessibility features or ARIA support
Applied to files:
app/views/shared/notifications/_notice.html.erb
📚 Learning: 2025-09-23T22:23:22.963Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-09-23T22:23:22.963Z
Learning: Applies to app/views/**/*.html.erb : Use ViewComponents when elements have complex logic/styling, are reused across contexts, need variants/slots/accessibility, or require Stimulus interactivity
Applied to files:
app/views/shared/notifications/_notice.html.erbapp/views/layouts/shared/_htmldoc.html.erb
📚 Learning: 2025-11-24T16:54:59.198Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T16:54:59.198Z
Learning: Applies to app/views/**/*.erb : Use Tailwind design tokens (e.g., text-primary, bg-container, border-secondary) instead of raw color names
Applied to files:
app/views/shared/notifications/_notice.html.erb
📚 Learning: 2025-11-24T16:57:20.318Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-11-24T16:57:20.318Z
Learning: Applies to **/*.{erb,html.erb} : Always use the declarative approach when integrating Stimulus controllers in views: the ERB template should declare what happens using data-* attributes, and the Stimulus controller should respond
Applied to files:
app/views/shared/notifications/_notice.html.erb
📚 Learning: 2026-01-09T12:25:59.839Z
Learnt from: jjmata
Repo: we-promise/sure PR: 593
File: app/controllers/pages_controller.rb:200-203
Timestamp: 2026-01-09T12:25:59.839Z
Learning: Do not compare hard-coded strings to identify the 'Uncategorized Investments' category in Ruby/Rails code (e.g., ct.category.name != "Uncategorized Investments"). Localization/internationalization can change strings. Prefer: (1) a model-level predicate such as ct.category.uncategorized_investment? or (2) compare by identity with a canonical category, e.g., ct.category != Category.uncategorized_investments. This improves maintainability and correctness across translations. Apply this pattern to Ruby files that perform category checks (controllers, models, or elsewhere).
Applied to files:
app/controllers/concerns/notifiable.rb
📚 Learning: 2025-11-24T16:57:20.318Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-11-24T16:57:20.318Z
Learning: Applies to app/views/shared/_*.html.erb : Place shared partials in app/views/shared/ directory for reusable content across contexts
Applied to files:
app/views/layouts/shared/_htmldoc.html.erb
📚 Learning: 2025-11-24T16:57:20.318Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-11-24T16:57:20.318Z
Learning: Applies to app/views/**/_*.html.erb : Place context-specific partials in relevant controller view directory (e.g., accounts/_account_sidebar_tabs.html.erb)
Applied to files:
app/views/layouts/shared/_htmldoc.html.erb
📚 Learning: 2025-09-23T22:23:22.963Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-09-23T22:23:22.963Z
Learning: Applies to app/views/shared/**/_*.html.erb : Place shared partials in app/views/shared/
Applied to files:
app/views/layouts/shared/_htmldoc.html.erb
📚 Learning: 2025-11-24T16:56:13.406Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-conventions.mdc:0-0
Timestamp: 2025-11-24T16:56:13.406Z
Learning: Applies to **/*.html.erb : Leverage Turbo frames to break up pages instead of JavaScript-driven client-side solutions
Applied to files:
app/views/layouts/shared/_htmldoc.html.erb
📚 Learning: 2025-11-24T16:57:20.318Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-11-24T16:57:20.318Z
Learning: Applies to app/views/**/_*.html.erb : Use underscore prefix for partial files (e.g., _trend_change.html.erb, _form_errors.html.erb, _sync_indicator.html.erb)
Applied to files:
app/views/layouts/shared/_htmldoc.html.erb
📚 Learning: 2025-09-23T22:23:22.963Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-09-23T22:23:22.963Z
Learning: Applies to app/views/**/_*.html.erb : Place context-specific partials within their corresponding controller view directory (not shared)
Applied to files:
app/views/layouts/shared/_htmldoc.html.erb
📚 Learning: 2025-09-23T22:23:22.963Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-09-23T22:23:22.963Z
Learning: Applies to app/{views,components}/**/*.html.erb : Integrate Stimulus declaratively in ERB: templates declare data-controller/actions/targets; controllers respond to those declarations
Applied to files:
app/views/shared/notifications/_alert.html.erb
📚 Learning: 2026-01-10T19:11:44.566Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T19:11:44.566Z
Learning: Applies to **/*.{js,ts,tsx,erb} : In Stimulus controllers, use Declarative Actions (HTML declares what happens). Example: `data-action="click->toggle#toggle"`
Applied to files:
app/views/shared/notifications/_alert.html.erb
📚 Learning: 2025-11-24T16:56:51.982Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/stimulus_conventions.mdc:0-0
Timestamp: 2025-11-24T16:56:51.982Z
Learning: Applies to **/*_controller.{js,ts} : Use declarative actions in Stimulus controllers, not imperative event listeners - define actions in ERB templates using data-action attributes instead of binding events in the controller's connect() method
Applied to files:
app/views/shared/notifications/_alert.html.erb
⏰ 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). (1)
- GitHub Check: ci / test
🔇 Additional comments (7)
app/controllers/concerns/notifiable.rb (1)
50-55: LGTM!Handling both string and symbol keys for the
typeattribute is good defensive coding, as flash data can have varying key formats depending on how it's set.app/views/layouts/shared/_htmldoc.html.erb (1)
21-26: LGTM!The layout cleanly integrates the new
render_flash_ctahelper and maintains proper structure for the notification tray. The increased spacing (space-y-3) accommodates the animation transitions.app/views/shared/notifications/_alert.html.erb (1)
3-4: Inconsistent animation handling compared to_notice.html.erb.The alert partial lacks the
animationend->element-removal#handleAnimationEndaction that_notice.html.erbhas. This means:
- Notices auto-dismiss after the SVG stroke-fill animation completes
- Alerts require manual dismissal via the close button
If this is intentional (alerts should persist until acknowledged), consider adding a brief comment. Otherwise, add the animation handling for consistency:
<%= tag.div class: "flex gap-3 rounded-lg bg-container p-4 group w-full md:max-w-80 shadow-border-lg transition-all duration-300 ease-out opacity-0 translate-y-[-8px]", - data: { controller: "element-removal" } do %> + data: { + controller: "element-removal", + action: "animationend->element-removal#handleAnimationEnd" + } do %>Note: If auto-dismiss is desired, you'd also need to add the SVG progress indicator from
_notice.html.erb.app/views/shared/notifications/_notice.html.erb (2)
3-7: LGTM! Animation setup is correct.The
animationendevent listener is appropriate here because it targets theanimate-stroke-fillkeyframe animation on the SVG circle (line 28), not the CSS transitions. The controller'shandleAnimationEndlikely filters by animation name to trigger removal only when the progress indicator completes.
24-35: Good UX pattern.The combination of auto-dismiss via the SVG progress animation and a manual close button on hover provides flexibility. This follows the declarative Stimulus action pattern from the coding guidelines.
app/views/shared/notifications/_cta.html.erb (2)
20-22: LGTM!The close button implementation is consistent with
_notice.html.erband follows the declarative Stimulus action pattern. The hover-reveal behavior maintains a clean UI while providing a clear dismissal option.
3-4: Verify Turbo Stream targeting with the wrapper div.The
id="cta"is essential forturbo_stream.replace("cta", ...)inflash_notification_stream_items. However, ifrender_flash_ctainNotifiablewraps this partial in an additional<div>, the#ctaelement becomes nested, which could cause issues when Turbo Stream attempts to replace it.This is related to the double-wrapping concern flagged in
notifiable.rb. Ensure the wrapper is removed or the Turbo Stream targeting is adjusted accordingly.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Removes redundant wrapper divs from notification rendering in Notifiable concern and adds consistent bottom margin to notification components for improved spacing. Also removes unnecessary spacing class from notification tray container.
|
This is a nice touch, my main worry before merge is: should it be reflected in the |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on February 15
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| <%= render_flash_notifications %> | ||
|
|
||
| <div id="cta"></div> | ||
| <%= render_flash_cta %> |
There was a problem hiding this comment.
Turbo Stream CTA notifications fail without placeholder element
High Severity
The render_flash_cta helper returns nil when there's no flash[:cta], meaning no #cta element exists in the DOM on initial page load. However, flash_notification_stream_items still uses turbo_stream.replace("cta", ...) to stream CTA notifications. When Turbo attempts to replace a non-existent element, it silently fails and the CTA never appears. The previous code included a <div id="cta"></div> placeholder that guaranteed the target element always existed for replacement.
Additional Locations (1)
There was a problem hiding this comment.
This one slipped my radar, thoughts @hendriksen-mark?
There was a problem hiding this comment.
I hope to look at this this weekend, im a little busy with work and sports and stuff
|
I've been thinking about impact of these sorts of UX changes to the maintenance of the original Maybe design system, and asked Claude Code, @hendriksen-mark ... take a look at #657 and let me know what you think before we merge this (or that!) 🤔 |
Same goes for the category selector/typeahead widget you're putting together in #633 @dgilperez ... probably want to approach it from a design system POV, no? |
|
I will look at this later today or this weekend |
Introduces standardized animation utility classes in the design system (animation-utils.css) and documents their usage. Refactors the element_removal_controller.js to support configurable enter/exit animations using these utilities. Updates notification partials to use the new animation classes for consistent transitions. Adjusts layout for improved safe area handling and notification tray spacing.
This comment was marked as outdated.
This comment was marked as outdated.
3 participants
Enhanced the element_removal_controller to animate notifications with fade-in on mount and fade-out before removal. Updated the notification partial to include initial animation classes and transition properties for a smoother user experience.
before:
Schermopname.2026-01-14.om.20.01.08.mov
after:
Schermopname.2026-01-14.om.19.56.22.mov
Summary by CodeRabbit
New Features
Bug Fixes
Style
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.