Open
Conversation
added 11 commits
April 23, 2026 15:28
📝 WalkthroughWalkthroughAdds an Changes
Sequence Diagram(s)sequenceDiagram
participant Home as HomePage
participant Asset as AssetComponent
participant Progress as ProgressBar
participant Watchdog as WatchdogTimer
Home->>Asset: render asset (pass onAssetError)
Asset->>Progress: start progress (interval -> animationDuration)
Asset->>Home: onVideoWaiting / onAssetError
Home->>Progress: pause progress
Home->>Watchdog: start auto-skip timeout
Note over Watchdog,Home: if auto-skip fires -> Home triggers skip transition
Watchdog-->>Home: timeout fires
Home->>Home: acquire epoch lock / queue pendingTransition
Watchdog->>Home: watchdog fires (if transition hangs)
Home->>Home: force-reset lock and replay pendingTransition
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
Comment on lines
+37
to
+40
| if (progress.current >= 1 && !completed) { | ||
| completed = true; | ||
| untrack(() => onDone()); | ||
| } else if (progress.current < 1) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP -C2 'onDone:\s*\(\)\s*=>\s*void|untrack\(\(\)\s*=>\s*onDone\(\)\)' immichFrame.Web/src/lib/components/elements/progress-bar.svelte
rg -nP -C3 'const\s+handleDone\s*=\s*async|onDone=\{handleDone\}|<ProgressBar' immichFrame.Web/src/lib/components/home-page/home-page.svelteRepository: immichFrame/ImmichFrame
Length of output: 1045
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the handleDone implementation for potential rejection points
echo "=== handleDone implementation ==="
rg -n "const handleDone = async" -A 50 immichFrame.Web/src/lib/components/home-page/home-page.svelte | head -80
echo ""
echo "=== Search for other onDone handlers ==="
rg -n "onDone=" immichFrame.Web/src/lib/components/
echo ""
echo "=== Check if there are any try-catch or .catch() patterns in handleDone ==="
rg -n "\.catch|try {" immichFrame.Web/src/lib/components/home-page/home-page.svelte | grep -A2 -B2 "16[0-9]:"Repository: immichFrame/ImmichFrame
Length of output: 2362
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check what getNextAssets and getPreviousAssets do (can they reject?)
echo "=== getNextAssets/getPreviousAssets signatures ==="
rg -n "const getNextAssets|const getPreviousAssets|let getNextAssets|let getPreviousAssets" -A 5 immichFrame.Web/src/lib/components/home-page/home-page.svelte | head -40
echo ""
echo "=== Check for throw/reject patterns in asset fetching ==="
rg -n "throw |reject\(" immichFrame.Web/src/lib/components/home-page/home-page.svelte | head -20Repository: immichFrame/ImmichFrame
Length of output: 118
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get more context around getNextAssets/getPreviousAssets calls
echo "=== Context around getNextAssets call ==="
rg -n "getNextAssets\(" immichFrame.Web/src/lib/components/home-page/home-page.svelte -B 10 -A 3
echo ""
echo "=== Check for error handling patterns ==="
rg -n "catch|error" immichFrame.Web/src/lib/components/home-page/home-page.svelte | head -20
echo ""
echo "=== Check imports at top of home-page.svelte ==="
head -50 immichFrame.Web/src/lib/components/home-page/home-page.svelteRepository: immichFrame/ImmichFrame
Length of output: 4210
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the full getNextAssets implementation
echo "=== getNextAssets full implementation ==="
rg -n "async function getNextAssets" -A 30 immichFrame.Web/src/lib/components/home-page/home-page.svelte
echo ""
echo "=== getPreviousAssets full implementation ==="
rg -n "async function getPreviousAssets" -A 20 immichFrame.Web/src/lib/components/home-page/home-page.svelte
echo ""
echo "=== loadAssets function ==="
rg -n "async function loadAssets|const loadAssets" -A 20 immichFrame.Web/src/lib/components/home-page/home-page.svelte | head -50Repository: immichFrame/ImmichFrame
Length of output: 2052
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check pickAssets and updateAssetPromises implementations
echo "=== pickAssets function ==="
rg -n "async function pickAssets|const pickAssets" -A 25 immichFrame.Web/src/lib/components/home-page/home-page.svelte | head -50
echo ""
echo "=== updateAssetPromises function ==="
rg -n "async function updateAssetPromises|const updateAssetPromises" -A 20 immichFrame.Web/src/lib/components/home-page/home-page.svelte
echo ""
echo "=== Check for any unhandled promise rejections ==="
rg -n "throw " immichFrame.Web/src/lib/components/home-page/home-page.svelteRepository: immichFrame/ImmichFrame
Length of output: 1708
Handle async onDone() rejections to prevent unhandled promise failures.
The call to onDone() at line 39 of progress-bar.svelte is fire-and-forget with no error handling. In home-page.svelte, handleDone is async and can reject: loadAsset() throws errors (line 394), and assetComponent?.play?.() can also fail. Currently this results in an unhandled promise rejection. Note that handleDone's own internal recursive calls (lines 185, 213) include .catch() handlers, but the external call from progress-bar.svelte does not.
Proposed patch
interface Props {
autoplay?: boolean;
status?: ProgressBarStatus;
location?: ProgressBarLocation;
hidden?: boolean;
duration?: number;
- onDone: () => void;
+ onDone: () => void | Promise<void>;
onPlaying?: () => void;
onPaused?: () => void;
}
@@
if (progress.current >= 1 && !completed) {
completed = true;
- untrack(() => onDone());
+ const result = untrack(() => onDone());
+ void Promise.resolve(result).catch((err) => {
+ console.error('ProgressBar onDone failed:', err);
+ });
} else if (progress.current < 1) {
completed = false;
}🤖 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
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.
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