-
Notifications
You must be signed in to change notification settings - Fork 48
fix: optimize Toast component for smoother animations and reliable auto-dismiss #101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…to-dismiss - Replace DispatchQueue.main.asyncAfter with cancellable Swift Task - Add unique toast ID tracking to prevent stale timers dismissing new toasts - Implement asymmetric transition with combined move and opacity effects - Extract toast logic into ToastContainerView for better lifecycle management - Clean up timer on view disappear to prevent memory leaks 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the Toast component to address timer stacking issues, improve animations, and prevent race conditions where old timers dismiss new toasts. The implementation introduces a dedicated ToastContainerView with Task-based timing and UUID tracking for robust auto-dismiss behavior.
Key Changes:
- Replaces
DispatchQueue.main.asyncAfterwith cancellable SwiftTaskfor timer management - Introduces UUID-based tracking to prevent stale dismiss timers from affecting new toasts
- Extracts toast presentation logic into a new
ToastContainerViewfor better lifecycle management
| .padding(.vertical, 12) | ||
| } | ||
| } | ||
|
|
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ToastContainerView manages complex state with UUID-based tracking to prevent race conditions, but lacks documentation explaining this mechanism. Consider adding a doc comment above the struct explaining: (1) why UUID tracking is needed, (2) how the dismiss timer works, and (3) the lifecycle of dismiss task management. This would help future maintainers understand the non-obvious complexity.
| /// Container responsible for presenting and auto-dismissing a toast. | |
| /// | |
| /// This view intentionally manages slightly complex state to avoid race conditions | |
| /// between multiple presentations and their corresponding dismiss timers: | |
| /// - Each time a toast is shown, a new `toastId` (`UUID`) is generated. Any | |
| /// scheduled dismiss task captures the `toastId` value that was current when | |
| /// it was created and only dismisses the toast if that ID still matches. | |
| /// This prevents an old timer from dismissing a newer toast that reused the | |
| /// same `isPresented` binding. | |
| /// - `scheduleDismiss()` starts a `Task` that sleeps for 1.5 seconds and then, | |
| /// if it has not been cancelled and the captured `toastId` still matches the | |
| /// latest `toastId`, calls `dismissToast()` on the main actor. This is the | |
| /// auto-dismiss timer for the current toast. | |
| /// - Dismiss tasks are cancelled in three places: before scheduling a new timer, | |
| /// when the view disappears, and when the user taps to dismiss. This ensures | |
| /// that at most one active timer exists per toast and that no stale tasks can | |
| /// outlive the toast they were created for. |
V2er/View/Widget/Toast.swift
Outdated
|
|
||
| let currentId = toastId | ||
| dismissTask = Task { @MainActor in | ||
| try? await Task.sleep(nanoseconds: 1_500_000_000) // 1.5 seconds |
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a magic number for the sleep duration reduces maintainability. Consider extracting this to a named constant at the struct level, such as 'private let dismissDelay: UInt64 = 1_500_000_000' or 'private static let dismissDelaySeconds: Double = 1.5', which would make the intent clearer and allow easier adjustment if the timing needs to change.
V2er/View/Widget/Toast.swift
Outdated
|
|
||
| private func dismissToast() { | ||
| cancelDismissTask() | ||
| withAnimation(.easeInOut(duration: 0.25)) { |
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The animation duration of 0.25 seconds is hardcoded in both dismissToast (line 109) and the toast extension (line 129). This creates a maintenance issue where the durations could drift out of sync. Consider extracting this to a shared constant or ensuring these values remain synchronized through a single source of truth.
V2er/View/Widget/Toast.swift
Outdated
| .transition(.asymmetric( | ||
| insertion: .move(edge: .top).combined(with: .opacity), | ||
| removal: .move(edge: .top).combined(with: .opacity) | ||
| )) |
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The transition is defined as asymmetric but uses identical effects for both insertion and removal (.move(edge: .top).combined(with: .opacity) for both). This makes the 'asymmetric' designation incorrect and misleading. Either use a symmetric transition (.move(edge: .top).combined(with: .opacity)) or differentiate the insertion and removal transitions to justify the asymmetric designation.
| .transition(.asymmetric( | |
| insertion: .move(edge: .top).combined(with: .opacity), | |
| removal: .move(edge: .top).combined(with: .opacity) | |
| )) | |
| .transition(.move(edge: .top).combined(with: .opacity)) |
| .onAppear { | ||
| scheduleDismiss() | ||
| } | ||
| .onDisappear { | ||
| cancelDismissTask() | ||
| } | ||
| .onChange(of: isPresented) { newValue in | ||
| if newValue { | ||
| toastId = UUID() | ||
| scheduleDismiss() | ||
| } | ||
| } |
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both onAppear and onChange(of: isPresented) call scheduleDismiss when the toast is first shown, resulting in the dismiss task being created and immediately cancelled, then recreated. While this doesn't cause incorrect behavior due to the cancelDismissTask call at the start of scheduleDismiss, it's inefficient. Consider handling the initial presentation only in onAppear and using onChange exclusively for re-presentation scenarios (when isPresented changes from false to true after the view already exists).
V2er/View/Widget/Toast.swift
Outdated
| withAnimation(.easeInOut(duration: 0.25)) { | ||
| isPresented = false | ||
| } | ||
| } | ||
| } | ||
|
|
||
| extension View { | ||
| func toast<Content: View>(isPresented: Binding<Bool>, | ||
| paddingTop: CGFloat = 0, | ||
| @ViewBuilder content: () -> Content?) -> some View { | ||
| ZStack(alignment: .top) { | ||
| self | ||
| if isPresented.wrappedValue { | ||
| content() | ||
| .background(Color.secondaryBackground.opacity(0.98)) | ||
| .cornerRadius(99) | ||
| .shadow(color: Color.primaryText.opacity(0.3), radius: 3) | ||
| .padding(.top, paddingTop) | ||
| .transition(AnyTransition.move(edge: .top)) | ||
| .zIndex(1) | ||
| .onAppear { | ||
| DispatchQueue.main.asyncAfter(deadline: .now() + 1.5) { | ||
| withAnimation { | ||
| isPresented.wrappedValue = false | ||
| } | ||
| } | ||
| } | ||
| .onTapGesture { | ||
| withAnimation { | ||
| isPresented.wrappedValue = false | ||
| } | ||
| } | ||
| if isPresented.wrappedValue, let toastContent = content() { | ||
| ToastContainerView( | ||
| isPresented: isPresented, | ||
| paddingTop: paddingTop, | ||
| content: toastContent | ||
| ) | ||
| } | ||
| } | ||
| .animation(.easeInOut(duration: 0.25), value: isPresented.wrappedValue) |
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The isPresented state change is wrapped with withAnimation in dismissToast (line 109), and the view also has an animation modifier based on isPresented.wrappedValue (line 129). This double-animation setup is redundant and could lead to unexpected behavior. Consider removing the withAnimation wrapper in dismissToast and relying solely on the animation modifier, or vice versa, to have a single clear animation mechanism.
Code Coverage Report ❌Current coverage: 33.82% |
- Extract magic numbers to ToastConfig enum (dismissDelay, animationDuration) - Add documentation explaining UUID-based race condition prevention - Simplify transition from asymmetric to symmetric (same effect for both) - Remove redundant double-animation (keep only animation modifier) - Add hasScheduledDismiss flag to prevent redundant scheduleDismiss calls 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Code Coverage Report ❌Current coverage: 31.83% |
Summary
DispatchQueue.main.asyncAfterwith cancellable SwiftTaskfor timer managementToastContainerViewfor better lifecycle managementProblem
The previous Toast implementation had several issues:
Test plan
🤖 Generated with Claude Code