fix: asset transition/video stall freezes#637
Open
3rob3 wants to merge 15 commits into
Open
Conversation
added 11 commits
April 23, 2026 15:28
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaces the reactive ChangesProgress bar
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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
🧹 Nitpick comments (2)
immichFrame.Web/src/lib/components/elements/progress-bar.svelte (1)
57-61:pause()viaset(progress.current)— works, but worth a brief comment.Freezing by setting the target to the current animated value relies on the duration callback returning ~0 when
to ≈ from. It's subtle enough that a one-line comment would help future maintainers (and prevents someone from "simplifying" the duration function to always returnduration * 1000 * (to - from), which would break the reset path).📝 Optional clarifying comment
export const pause = async () => { status = ProgressBarStatus.Paused; onPaused(); + // Freeze in place: targeting the current value with duration(to-from≈0) ≈ 0 halts motion. await progress.set(progress.current); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@immichFrame.Web/src/lib/components/elements/progress-bar.svelte` around lines 57 - 61, Add a one-line explanatory comment inside the pause() function above the call to progress.set(progress.current) clarifying that freezing animation relies on the duration callback returning ~0 when to ≈ from, and that this intentional behavior must be preserved (i.e., avoid refactoring the duration calculation to a form like duration * 1000 * (to - from) which would break the reset/pause behavior); reference the pause() function and the progress.set(progress.current) call so future maintainers understand why the target is explicitly set to the current value.immichFrame.Web/src/lib/components/elements/asset.svelte (1)
57-60: Add a comment explaining the untrack pattern and usevoidto avoid unused expression warnings.The code reads
asset[1].idto establish it as a dependency (triggering re-evaluation when the asset changes) while usinguntrack(() => interval)to exempt interval updates from re-triggering the derived. This ensures the interval value is snapshotted only when the asset changes, preventing CSS animations from restarting mid-transition. Add a comment explaining this intent and prefixasset[1].idwithvoidto prevent ESLint from flagging the unused expression:Suggested change
+ // Re-evaluate only when the asset changes; keep the interval stable for the + // lifetime of the current asset so zoom/pan animations don't restart. const animationDuration = $derived.by(() => { - asset[1].id; + void asset[1].id; return untrack(() => interval); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@immichFrame.Web/src/lib/components/elements/asset.svelte` around lines 57 - 60, Add an inline comment above the animationDuration derived explaining the untrack pattern: that we intentionally read asset[1].id solely to establish a dependency so the derived recomputes when the asset changes while using untrack(() => interval) to snapshot the interval and prevent interval updates from restarting CSS animations mid-transition; also change the bare expression to use void asset[1].id to avoid ESLint/unused-expression warnings and keep the untrack usage as-is. Reference: animationDuration, asset, untrack, interval.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@immichFrame.Web/src/lib/components/home-page/home-page.svelte`:
- Around line 533-535: The onAssetError handler is causing unbounded rapid skips
and double-skips in split view; modify the onAssetError logic (the inline
handler that currently calls handleDone(false, true)) to debounce/sketch one
skip per render cycle and count consecutive error skips before triggering
further action: introduce a module-level flag (e.g., assetErrorPending) or a
per-render timestamp to ignore additional onAssetError calls until the current
transition completes, increment a consecutiveErrorSkips counter when a skip is
performed, reset consecutiveErrorSkips to 0 in successful playback/render
handlers such as onVideoPlaying, and when consecutiveErrorSkips exceeds a
threshold surface an error state instead of calling handleDone or back off calls
to loadAssets; ensure pendingTransition and handleDone integrations respect the
new guard so a second onerror in the same tick cannot queue a duplicate skip.
- Around line 170-184: The watchdog currently only resets
isHandlingAssetTransition and hands off to pendingTransition, leaving the
autoplay progress paused when pendingTransition is null; change the watchdog
timeout handler (watchdogTimer callback that checks currentEpoch,
transitionEpoch, isHandlingAssetTransition and uses
pendingTransition/handleDone) so that if pendingTransition is null it also
forces the slideshow to resume/advance (e.g., trigger progressBar.play() or
progressBar.restart(true) / invoke the same code path that advances the slide)
instead of returning immediately; additionally, add timeout
support/AbortController to the underlying api.getAssets and api.getAsset calls
and ensure getNextAssets accepts and propagates an AbortSignal so hung fetches
are actively rejected and the finally blocks (and isHandlingAssetTransition
cleanup) can run.
---
Nitpick comments:
In `@immichFrame.Web/src/lib/components/elements/asset.svelte`:
- Around line 57-60: Add an inline comment above the animationDuration derived
explaining the untrack pattern: that we intentionally read asset[1].id solely to
establish a dependency so the derived recomputes when the asset changes while
using untrack(() => interval) to snapshot the interval and prevent interval
updates from restarting CSS animations mid-transition; also change the bare
expression to use void asset[1].id to avoid ESLint/unused-expression warnings
and keep the untrack usage as-is. Reference: animationDuration, asset, untrack,
interval.
In `@immichFrame.Web/src/lib/components/elements/progress-bar.svelte`:
- Around line 57-61: Add a one-line explanatory comment inside the pause()
function above the call to progress.set(progress.current) clarifying that
freezing animation relies on the duration callback returning ~0 when to ≈ from,
and that this intentional behavior must be preserved (i.e., avoid refactoring
the duration calculation to a form like duration * 1000 * (to - from) which
would break the reset/pause behavior); reference the pause() function and the
progress.set(progress.current) call so future maintainers understand why the
target is explicitly set to the current value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0c820c13-a70a-4cc6-a0cc-3145d543ae9c
📒 Files selected for processing (4)
immichFrame.Web/src/lib/components/elements/asset-component.svelteimmichFrame.Web/src/lib/components/elements/asset.svelteimmichFrame.Web/src/lib/components/elements/progress-bar.svelteimmichFrame.Web/src/lib/components/home-page/home-page.svelte
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
immichFrame.Web/src/lib/components/elements/progress-bar.svelte (1)
51-62:⚠️ Potential issue | 🔴 CriticalInterrupted tween promises never resolve —
play()/pause()can leave awaiters hung, skipping critical cleanup.Per the Svelte motion docs, when a
Tween.set(...)is interrupted by anotherset(...), the interrupted call's promise never resolves. This causes a problem:
play()awaitsprogress.set(1). Whenpause()orrestart(...)is later invoked, that tween is interrupted and theplay()promise hangs permanently.pause()awaitsprogress.set(progress.current). When interrupted, the promise hangs.- In home-page.svelte, callers like
onVideoWaitinghave critical cleanup code after the await:Ifawait progressBar.pause(); clearTimeout(videoStallTimeout);pause()is interrupted (e.g., byrestart()orplay()), theclearTimeoutnever executes, leaving the stall timeout active and causing timing bugs.The suggested fix is correct: remove the await from the tween call and let completion be signaled via the existing
onDoneeffect instead.Suggested fix
export const play = async () => { status = ProgressBarStatus.Playing; onPlaying(); - await progress.set(1); + progress.set(1); }; export const pause = async () => { status = ProgressBarStatus.Paused; onPaused(); - await progress.set(progress.current); + progress.set(progress.current); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@immichFrame.Web/src/lib/components/elements/progress-bar.svelte` around lines 51 - 62, The play()/pause() functions currently await progress.set(...) which can hang if that tween is interrupted (e.g., by pause(), play(), or restart()), so remove the await from the progress.set calls in play and pause (keep status updates and onPlaying/onPaused calls), and rely on the existing onDone effect to signal completion instead of awaiting the tween; ensure play uses progress.set(1) without await and pause uses progress.set(progress.current) without await so callers do not get permanently stuck.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@immichFrame.Web/src/lib/components/home-page/home-page.svelte`:
- Line 183: The fire-and-forget calls to handleDone(...) in the watchdog
callback and the finally-drain are causing unhandled promise rejections because
handleDone can throw; update both call sites (the watchdog recovery caller and
the finally-drain caller) to either await handleDone(...) or attach a
.catch(...) to consume/log errors so rejections don’t surface as unhandled and
to ensure the transition lock and retry logic behave correctly; locate the
invocations of handleDone(next.previous, next.instant) and the finally-block
invocation (both currently invoked without awaiting) and change them to handle
the returned promise (await or .catch with proper logging using the existing
logger/context).
---
Outside diff comments:
In `@immichFrame.Web/src/lib/components/elements/progress-bar.svelte`:
- Around line 51-62: The play()/pause() functions currently await
progress.set(...) which can hang if that tween is interrupted (e.g., by pause(),
play(), or restart()), so remove the await from the progress.set calls in play
and pause (keep status updates and onPlaying/onPaused calls), and rely on the
existing onDone effect to signal completion instead of awaiting the tween;
ensure play uses progress.set(1) without await and pause uses
progress.set(progress.current) without await so callers do not get permanently
stuck.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cfc61a12-96ea-4db2-8f11-e6e8d1457d7a
📒 Files selected for processing (3)
immichFrame.Web/src/lib/components/elements/asset.svelteimmichFrame.Web/src/lib/components/elements/progress-bar.svelteimmichFrame.Web/src/lib/components/home-page/home-page.svelte
🚧 Files skipped from review as they are similar to previous changes (1)
- immichFrame.Web/src/lib/components/elements/asset.svelte
added 2 commits
April 25, 2026 20:23
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
immichFrame.Web/src/lib/components/elements/progress-bar.svelte (1)
28-33: Clamp computed tween duration to avoid negative-time transitions.At Line 31,
duration * 1000 * (to - from)can go negative whento < from(possible around pause/resume frame timing). Clamp to non-negative for stable behavior.Proposed patch
const progress = new Tween<number>(0, { duration: (from: number, to: number) => { if (to === 0) return 0; - return duration * 1000 * (to - from); + const delta = Math.max(0, to - from); + return Math.max(0, duration) * 1000 * delta; } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@immichFrame.Web/src/lib/components/elements/progress-bar.svelte` around lines 28 - 33, The computed duration for the Tween<number> assigned to progress can become negative when to < from; update the duration callback in the progress Tween (the anonymous function using parameters from and to) to clamp the computed value to a non-negative number (e.g., wrap duration * 1000 * (to - from) with Math.max(0, ...)) so the Tween never receives a negative time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@immichFrame.Web/src/lib/components/elements/progress-bar.svelte`:
- Around line 37-40: The call to onDone() inside progress-bar.svelte is invoked
fire-and-forget via untrack(() => onDone()) and can produce unhandled promise
rejections when the handler (e.g., handleDone which calls loadAsset() /
assetComponent.play()) rejects; update the invocation to capture the returned
promise and attach a rejection handler so rejections are handled (e.g., call
onDone(), check if it returns a Promise, and call .catch(...) or use void
onDone().catch(...)) while keeping it wrapped in untrack; refer to
progress.current, completed, untrack and onDone to locate the spot to change.
---
Nitpick comments:
In `@immichFrame.Web/src/lib/components/elements/progress-bar.svelte`:
- Around line 28-33: The computed duration for the Tween<number> assigned to
progress can become negative when to < from; update the duration callback in the
progress Tween (the anonymous function using parameters from and to) to clamp
the computed value to a non-negative number (e.g., wrap duration * 1000 * (to -
from) with Math.max(0, ...)) so the Tween never receives a negative time.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 22f3240e-fe7a-41ad-a91c-dc60dbfdc9c0
📒 Files selected for processing (1)
immichFrame.Web/src/lib/components/elements/progress-bar.svelte
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #601
Summary by CodeRabbit
New Features
Bug Fixes
Refactor