Stop the player backdrop wiping off screen - #89
Conversation
With the full player open the ambient backdrop wiped away and exposed the black system background beneath it. Rapid play/pause and rapid minimise/expand both triggered it, and once started it continued on its own while paused. The breathing effect was driven by a repeatForever animation. An endless SwiftUI animation is a transaction, and a transaction applies to every animatable change in its subtree — not only the state it was started for. The subtree included the safe-area expansion that sizes this backdrop: .ignoresSafeArea() does not pin a view to the screen, it expands it by the current insets. So the breath latched onto the backdrop's top edge and oscillated it between the real inset and zero for as long as the view lived, sliding the artwork off screen and uncovering the layer beneath. Device probes caught it directly, the top inset ramping 0 to 67 and back on every tap while every size probe stayed silent. Nothing stopped it either: pausing only reset the flag the breath read, while the runaway animation lived on the geometry. Scoping it with .animation(_:value:) is not enough — that modifier still animates everything in its subtree. Drive the breath from a clock with TimelineView instead, matching EqualizerBars. Same motion, no transaction to leak, and `paused:` stops it without leaving anything running. Three supporting changes fall out of the same investigation: - Clamp the artwork layer by putting it in an overlay. .aspectRatio(.fill) always resolves larger than the proposal and .frame(maxWidth:maxHeight:) adopts an oversized child rather than clamping it, so the image could inflate its own container to the artwork's aspect ratio instead of the screen's. Overlay content never feeds size back to its parent. - Overscan the layers past every edge, so the backdrop's extent no longer depends on safe-area insets at all. Defence in depth against this class. - Refuse inherited transactions at the view's boundary, so a repeatForever started anywhere else in the app cannot animate this backdrop's geometry. Drop the drawingGroup while here. The source is the 32px server-blurred variant, so there is no costly rasterization worth caching, and it forced a full-screen offscreen pass on every re-render of a view that re-renders constantly during playback.
Reviewer's GuideThis PR fixes the full‑screen player backdrop wiping off screen by replacing the Sequence diagram for clock-driven ambient breath animation in player backdropsequenceDiagram
participant AudioManager
participant SwiftUI
participant PlayerAmbientBackground
participant TimelineView
participant BackdropImage
AudioManager->>SwiftUI: publish isPlaying
SwiftUI->>PlayerAmbientBackground: body recompute
PlayerAmbientBackground->>PlayerAmbientBackground: shouldAnimateAmbient
PlayerAmbientBackground->>TimelineView: TimelineView.animation(minimumInterval:paused:)
alt shouldAnimateAmbient
TimelineView->>TimelineView: tick with context.date
TimelineView->>PlayerAmbientBackground: breathingPhase(at: context.date)
PlayerAmbientBackground->>BackdropImage: scaleEffect(1.22 + 0.06 * phase)
PlayerAmbientBackground->>BackdropImage: offset(x: -8 + 16 * phase, y: 6 - 12 * phase)
else !shouldAnimateAmbient
TimelineView->>BackdropImage: scaleEffect(1.22)
TimelineView->>BackdropImage: offset(x: -8, y: 6)
end
PlayerAmbientBackground->>PlayerAmbientBackground: transaction { animation = nil }
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesAmbient background animation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The root-level
.transaction { $0.animation = nil }prevents any implicit animations from leaking in, but it also disables any unscoped implicit animations inside this view tree; consider whether that might unintentionally suppress future layout or state transitions added underPlayerAmbientBackgroundthat rely on default animations. - The hard-coded
safeAreaOverscanof 160 works as a defensive buffer, but it might be clearer and more maintainable to derive it from screen dimensions (e.g. as a fraction of the longest edge) or tie it to a named design constant so its rationale is easier to adjust across devices.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The root-level `.transaction { $0.animation = nil }` prevents any implicit animations from leaking in, but it also disables any unscoped implicit animations inside this view tree; consider whether that might unintentionally suppress future layout or state transitions added under `PlayerAmbientBackground` that rely on default animations.
- The hard-coded `safeAreaOverscan` of 160 works as a defensive buffer, but it might be clearer and more maintainable to derive it from screen dimensions (e.g. as a fraction of the longest edge) or tie it to a named design constant so its rationale is easier to adjust across devices.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Twinskaraoke/Components/Player/PlayerAmbientBackground.swift (1)
94-123: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider smoothing the transition when
shouldAnimateAmbienttoggles.
phaseis forced to0whenevershouldAnimateAmbientisfalse(Line 100-102), butbreathingPhase(at:)is driven by absolute wall-clock time. On a rapid play/pause or minimize/expand cycle, the artwork'sscaleEffect/offsetcan jump discontinuously: once when the value snaps to the resting phase on pause, and again on resume when the phase recomputes from "now" instead of continuing from the frozen value. Neither of the two.animation(_:value:)modifiers on the outerZStack(Line 38-39) covers this transition.Add a scoped, non-repeating
.animation(_:value:)keyed onshouldAnimateAmbientdirectly on the artwork transform. This does not reintroduce the original bug, since it is a single discrete transition (not arepeatForever) and does not touch safe-area geometry.♻️ Proposed fix to smooth the play/pause transition
.scaleEffect(1.22 + 0.06 * phase) .offset( x: -8 + 16 * phase, y: 6 - 12 * phase ) + .animation( + reduceMotion ? nil : .easeInOut(duration: 0.6), + value: shouldAnimateAmbient + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Twinskaraoke/Components/Player/PlayerAmbientBackground.swift` around lines 94 - 123, In the TimelineView artwork transform, add a scoped non-repeating animation keyed by shouldAnimateAmbient after the scaleEffect/offset transforms so transitions between the resting phase and the current breathing phase interpolate smoothly. Use an appropriate easing and duration, and keep the animation local to the artwork without modifying the outer ZStack animations or safe-area geometry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Twinskaraoke/Components/Player/PlayerAmbientBackground.swift`:
- Around line 94-123: In the TimelineView artwork transform, add a scoped
non-repeating animation keyed by shouldAnimateAmbient after the
scaleEffect/offset transforms so transitions between the resting phase and the
current breathing phase interpolate smoothly. Use an appropriate easing and
duration, and keep the animation local to the artwork without modifying the
outer ZStack animations or safe-area geometry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 84e01345-d4ba-4caa-a2ab-3b19fcbb2a80
📒 Files selected for processing (1)
Twinskaraoke/Components/Player/PlayerAmbientBackground.swift
Review feedback. The breath phase was forced to zero while stopped and recomputed from wall-clock time on resume, so the artwork transform jumped at both ends of a pause. Track the time already breathed instead and carry it across, so the motion picks up where it left off. Doing it in state rather than with a scoped .animation(_:value:) keyed on the play/pause flag is deliberate. That modifier animates everything in its subtree, including the safe-area expansion that sizes this backdrop — the reason scoping the original repeatForever that way did not fix the bug — and the flag changes on play/pause, which is exactly when the popup animates its insets. It would re-arm the same leak on the same trigger, one-shot instead of endless. Also bring the overscan down to a size argued from the insets it has to clear, ~67pt at the extreme on device, rather than a round number. Deriving it from the display instead would overscan an iPad far past anything the insets can reach and pay for it in blurred area every frame.
|
Pushed a follow-up commit for the review feedback. Notes on each point: Smoothing the transform transition on play/pause — real problem, but I've taken a different route than suggested. The proposed The underlying observation was correct though: the phase was forced to zero while stopped and recomputed from wall-clock time on resume, so the transform jumped at both ends of a pause. Fixed by banking the elapsed breath time in state and carrying it across, so the motion resumes where it stopped. No animation modifier involved.
I've left it as a constant rather than deriving it from the display. A fraction of the longest edge would overscan an iPad far past anything the insets can reach, and that is blurred area paid for on every frame. The root This is a decorative full-bleed backdrop where inherited motion is the failure mode rather than a feature. The two Retested on device: rapid play/pause, rapid minimise/expand, and left paused. Backdrop holds. |
1 participant
Problem
With the full-screen player open, the ambient backdrop wiped away and exposed the black system background beneath it. Rapid play/pause and rapid minimise/expand both triggered it, and once started it kept going on its own while paused.
Cause
The backdrop's breathing effect was started with
withAnimation(.easeInOut(duration: 6.0).repeatForever(autoreverses: true)).An endless SwiftUI animation is a transaction, and a transaction applies to every animatable change in its subtree — not only the state it was started for. That subtree included the safe-area expansion that sizes the backdrop:
.ignoresSafeArea()does not pin a view to the screen, it expands it by the current insets.So the breath latched onto the backdrop's top edge and oscillated it between the real inset and zero for as long as the view lived, sliding the artwork off screen and uncovering the layer beneath.
Nothing stopped it: pausing only reset the flag the breath read, while the runaway animation lived on the geometry. Scoping it with
.animation(_:value:)is not enough either — that modifier still animates everything in its subtree.On-device probes confirmed it directly. The top safe-area inset ramped
0 → 67 → 0on every play/pause tap, autoreversing, while every size probe stayed silent — the geometry was never resized, only the inset-derived expansion moved.Fix
Drive the breath from a clock with
TimelineView, matching the existingEqualizerBarspattern. Same motion, no transaction to leak, andpaused:stops it without leaving anything running.Three supporting changes from the same investigation:
.aspectRatio(.fill)always resolves larger than the proposal, and.frame(maxWidth:maxHeight:)adopts an oversized child rather than clamping it — so the image could inflate its own container to the artwork's aspect ratio instead of the screen's. Overlay content never feeds size back to its parent.repeatForeverstarted anywhere else in the app cannot animate this backdrop's geometry.Also drops the
drawingGroup(). The source is the 32px server-blurred variant, so there is no costly rasterization worth caching, and it forced a full-screen offscreen pass on every re-render of a view that re-renders constantly during playback.Testing
Verified on device across several builds:
TwinskaraokeTestspasses; the iOS app target builds clean.Notes
Bound preference LNPopupItemPreferenceKey tried to update multiple times per framestill appears in the console. It is unrelated to this bug and predates the change, but it is a real update loop worth looking at separately.Summary by Sourcery
Stabilize the full-screen player’s ambient backdrop so it no longer slides off screen or exposes the system background during playback or layout changes.
Bug Fixes:
Enhancements:
Summary by CodeRabbit
New Features
Bug Fixes