resolve asset transition lockups and video stall freezes#636
Closed
resolve asset transition lockups and video stall freezes#636
Conversation
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds an optional Changes
Sequence DiagramsequenceDiagram
participant User
participant HomePage
participant Asset
participant Media
User->>HomePage: open slideshow / request advance
HomePage->>Asset: render(asset)
Asset->>Media: mount (img / video)
alt media load fails
Media->>Asset: error event
Asset->>HomePage: call onAssetError()
HomePage->>HomePage: handleDone(skip=true)
else video stalls
Media->>HomePage: onVideoWaiting()
HomePage->>HomePage: start stall timeout (VIDEO_STALL_MS)
HomePage-->>HomePage: timeout -> handleDone(skip=true)
else playback resumes
Media->>HomePage: onVideoPlaying()
HomePage->>HomePage: clear stall timeout
end
HomePage->>HomePage: acquire transition lock + start watchdog (TRANSITION_WATCHDOG_MS)
HomePage->>Asset: request next asset
Asset->>Media: present next media
Media->>User: display / playback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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. 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: 4
🧹 Nitpick comments (1)
immichFrame.Web/src/lib/components/home-page/home-page.svelte (1)
128-134: Fire-and-forget revoke is fine; just confirm rejected promises here are benign.Switching to non-awaited revoke avoids serializing cleanup behind unresolved load promises — good change. Note that any rejection from
loadAsset(network/HTTP failure, missing album info, etc.) reaches the.catchhere as a "Failed to resolve asset during cleanup" warning, even though the original failure already propagated to the caller. That's harmless but produces duplicate console noise on every transition where a load failed; logging atdebuglevel (or only when the URL was a blob:) would be cleaner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@immichFrame.Web/src/lib/components/home-page/home-page.svelte` around lines 128 - 134, The current cleanup loop (keysToRemove.forEach) logs promise rejections with console.warn which duplicates noise; update the .catch handler on assetPromisesDict lookups to either use console.debug instead of console.warn or suppress logging unless the resolved url is a blob (check the resolved [url] and only log if url?.startsWith('blob:')), referencing keysToRemove, assetPromisesDict, and revokeObjectUrl so the fire-and-forget revoke still runs but duplicate warnings are reduced.
🤖 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 497-500: The timeout calculation for videoStallTimeout uses
Math.min(VIDEO_STALL_MS, currentDuration * 1000) which causes short videos to be
skipped too eagerly; update the expression used where videoStallTimeout is set
(the line referencing videoStallTimeout, VIDEO_STALL_MS, currentDuration and
calling handleDone) to enforce a meaningful minimum stall wait (for example wrap
currentDuration*1000 in Math.max(..., 5000) before the Math.min) so the timer is
at least a reasonable floor (e.g. 5s) but still bounded by VIDEO_STALL_MS.
- Around line 159-189: The handleDone function currently clears watchdogTimer
but not videoStallTimeout, allowing a previously-scheduled stall timer to fire
and skip the new asset; modify handleDone (the function named handleDone) to
clearTimeout(videoStallTimeout) at the start (alongside
clearTimeout(watchdogTimer)) so any prior onVideoWaiting stall timer is
cancelled before starting the next transition, then proceed to set/reset
watchdogTimer and the rest of the logic; reference videoStallTimeout,
watchdogTimer, isHandlingAssetTransition, and handleDone when making the change.
- Around line 159-189: The watchdog timeout in handleDone currently multiplies
TRANSITION_WATCHDOG_MS by currentDuration, making the force-reset too slow for
long assets; change the watchdogTimer setTimeout delay to a flat interval (e.g.,
TRANSITION_WATCHDOG_MS or 2 * TRANSITION_WATCHDOG_MS) instead of
(currentDuration * 1000) + TRANSITION_WATCHDOG_MS so the lock is released
promptly if getNextAssets/getPreviousAssets/tick/assetComponent.play hang;
update the setTimeout call in handleDone and keep the existing force-reset logic
unchanged.
- Around line 494-510: The stall timer in the onVideoWaiting handler currently
always arms videoStallTimeout and can fire while userPaused is true, causing
handleDone(false, true) to skip an asset during a user pause; modify
onVideoWaiting to only set the stall timeout when !userPaused (gate the
setTimeout with the userPaused flag) and change onVideoPlaying to always
clearTimeout(videoStallTimeout) (unconditionally clear the timer) so any pending
stall is canceled when playback resumes; update references around
VIDEO_STALL_MS, currentDuration, videoStallTimeout, userPaused,
progressBar.pause/play, and handleDone to implement this behavior.
---
Nitpick comments:
In `@immichFrame.Web/src/lib/components/home-page/home-page.svelte`:
- Around line 128-134: The current cleanup loop (keysToRemove.forEach) logs
promise rejections with console.warn which duplicates noise; update the .catch
handler on assetPromisesDict lookups to either use console.debug instead of
console.warn or suppress logging unless the resolved url is a blob (check the
resolved [url] and only log if url?.startsWith('blob:')), referencing
keysToRemove, assetPromisesDict, and revokeObjectUrl so the fire-and-forget
revoke still runs but duplicate warnings are reduced.
🪄 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: f46a27c8-2b22-4b3e-9dd4-86fb980b2c5d
📒 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
♻️ Duplicate comments (1)
immichFrame.Web/src/lib/components/home-page/home-page.svelte (1)
170-175:⚠️ Potential issue | 🟠 MajorWatchdog still scales with
currentDuration— long videos leave the lock held for too long after a hang.This was raised in a prior review and remains unfixed. The work the watchdog guards (
getNextAssets/getPreviousAssets/tick/assetComponent.play) is bounded by network/IO, not by the displayed asset's duration. Scaling bycurrentDuration * 1000means a hang during a 60 s video keeps the slideshow frozen for ~70 s before the lock is released. Use a flat timeout based onTRANSITION_WATCHDOG_MS(or a small multiple) to keep the safety net responsive.Proposed fix
- watchdogTimer = window.setTimeout(() => { - if (isHandlingAssetTransition) { - console.error('Transition watchdog triggered: Force-resetting lock due to hang'); - isHandlingAssetTransition = false; - } - }, (currentDuration * 1000) + TRANSITION_WATCHDOG_MS); + watchdogTimer = window.setTimeout(() => { + if (isHandlingAssetTransition) { + console.error('Transition watchdog triggered: Force-resetting lock due to hang'); + isHandlingAssetTransition = false; + } + }, TRANSITION_WATCHDOG_MS);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@immichFrame.Web/src/lib/components/home-page/home-page.svelte` around lines 170 - 175, The watchdog timeout currently multiplies TRANSITION_WATCHDOG_MS by currentDuration (watchdogTimer set with (currentDuration * 1000) + TRANSITION_WATCHDOG_MS), which lets long videos keep isHandlingAssetTransition locked; change the watchdog to use a flat timeout (e.g., TRANSITION_WATCHDOG_MS or a small multiple) independent of currentDuration so the lock is released promptly on hang; update the watchdogTimer initialization in the block that sets isHandlingAssetTransition to use TRANSITION_WATCHDOG_MS (and keep clearing/resetting watchdogTimer where getNextAssets, getPreviousAssets, tick or assetComponent.play complete) so the safety net covers IO/network-bound operations rather than asset duration.
🧹 Nitpick comments (1)
immichFrame.Web/src/lib/components/home-page/home-page.svelte (1)
48-50: Nit: timer IDs don't need$state.
watchdogTimer,videoStallTimeout, andtimeoutIdare opaque numeric handles — nothing in the template reads them, so wrapping them in$state(...)adds reactivity overhead with no benefit. Plainletis sufficient.Proposed change
- let watchdogTimer: number | undefined = $state(); - let videoStallTimeout: number | undefined = $state(); - let timeoutId: number | undefined = $state(); + let watchdogTimer: number | undefined; + let videoStallTimeout: number | undefined; + let timeoutId: number | undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@immichFrame.Web/src/lib/components/home-page/home-page.svelte` around lines 48 - 50, The three timer variables watchdogTimer, videoStallTimeout, and timeoutId are declared with $state reactivity unnecessarily; change their declarations to plain non-reactive locals (e.g., let watchdogTimer: number | undefined, let videoStallTimeout: number | undefined, let timeoutId: number | undefined) so they remain opaque numeric handles without adding Svelte store/ $state overhead; keep the same types and any existing assignments/usages intact.
🤖 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 511-513: The onAssetError handler currently calls
handleDone(false, true) which gets ignored if isHandlingAssetTransition is true;
change the logic so the skip/retry is not silently dropped by either (a) queuing
a pending flag (e.g. pendingSkipOnAssetError) that handleDone consumes in its
finally block to invoke the retry/skip once the transition lock clears, or (b)
deferring the call by scheduling handleDone(false, true) on the next microtask
(e.g. Promise.resolve().then(...)) so it runs after the current transition
finishes; update the onAssetError handler and the handleDone implementation (and
referenced isHandlingAssetTransition logic) to check and clear the queued
pendingSkip flag to ensure the failed asset is retried/skipped.
---
Duplicate comments:
In `@immichFrame.Web/src/lib/components/home-page/home-page.svelte`:
- Around line 170-175: The watchdog timeout currently multiplies
TRANSITION_WATCHDOG_MS by currentDuration (watchdogTimer set with
(currentDuration * 1000) + TRANSITION_WATCHDOG_MS), which lets long videos keep
isHandlingAssetTransition locked; change the watchdog to use a flat timeout
(e.g., TRANSITION_WATCHDOG_MS or a small multiple) independent of
currentDuration so the lock is released promptly on hang; update the
watchdogTimer initialization in the block that sets isHandlingAssetTransition to
use TRANSITION_WATCHDOG_MS (and keep clearing/resetting watchdogTimer where
getNextAssets, getPreviousAssets, tick or assetComponent.play complete) so the
safety net covers IO/network-bound operations rather than asset duration.
---
Nitpick comments:
In `@immichFrame.Web/src/lib/components/home-page/home-page.svelte`:
- Around line 48-50: The three timer variables watchdogTimer, videoStallTimeout,
and timeoutId are declared with $state reactivity unnecessarily; change their
declarations to plain non-reactive locals (e.g., let watchdogTimer: number |
undefined, let videoStallTimeout: number | undefined, let timeoutId: number |
undefined) so they remain opaque numeric handles without adding Svelte store/
$state overhead; keep the same types and any existing assignments/usages intact.
🪄 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: 39e3f84c-d5cc-4b35-a883-e2f6920086ab
📒 Files selected for processing (1)
immichFrame.Web/src/lib/components/home-page/home-page.svelte
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 160-168: handleDone currently drops "previous" (back-skip)
requests during an ongoing transition because only the branch for instant &&
!previous sets pendingAssetError; update the logic in handleDone to queue
back-skip intent as well: when isHandlingAssetTransition is true and instant &&
previous, set a new flag (e.g., pendingPreviousSkip) or reuse a shared pending
intent structure so the pending request is honored after the transition; ensure
the handler that processes pendingAssetError (or the new pendingPreviousSkip)
executes the correct behavior for previous skips once isHandlingAssetTransition
clears, and update any references to pendingAssetError,
isHandlingAssetTransition, and the transition-complete flow to check and consume
the new pending flag.
- Around line 175-180: The watchdog callback currently force-resets
isHandlingAssetTransition but doesn't handle any queued pendingAssetError,
leaving a stranded skip; update the watchdog (the window.setTimeout using
TRANSITION_WATCHDOG_MS and watchdogTimer) to also drain/consume
pendingAssetError the same way the normal transition cleanup does (the logic in
onAssetError / the transition finally-block) — e.g., capture and clear
pendingAssetError inside the watchdog before resetting
isHandlingAssetTransition, and invoke the same skip/advance handling used by the
finally-block so the UI moves off the broken asset instead of leaving it
stranded.
🪄 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: 791f1dc7-6362-4130-9e99-d0ce1062933d
📒 Files selected for processing (1)
immichFrame.Web/src/lib/components/home-page/home-page.svelte
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
immichFrame.Web/src/lib/components/home-page/home-page.svelte (1)
176-202: Optional: deduplicate pending-transition drain and guard against unhandled rejections.The drain block is identical in the watchdog callback (176-180) and the
finally(198-202), and both invocations ofhandleDoneare fire-and-forgetasynccalls — any rejection inside the retried transition becomes an unhandled promise rejection.♻️ Proposed refactor
+ const drainPendingTransition = () => { + if (!pendingTransition) return; + const next = pendingTransition; + pendingTransition = null; + handleDone(next.previous, next.instant).catch((err) => + console.error('Queued transition failed:', err) + ); + }; @@ - watchdogTimer = window.setTimeout(() => { - if (isHandlingAssetTransition) { - console.error('Transition watchdog triggered: Force-resetting lock due to hang'); - isHandlingAssetTransition = false; - - if (pendingTransition) { - const next = pendingTransition; - pendingTransition = null; - handleDone(next.previous, next.instant); - } - } - }, (currentDuration * 1000) + TRANSITION_WATCHDOG_MS); + watchdogTimer = window.setTimeout(() => { + if (isHandlingAssetTransition) { + console.error('Transition watchdog triggered: Force-resetting lock due to hang'); + isHandlingAssetTransition = false; + drainPendingTransition(); + } + }, (currentDuration * 1000) + TRANSITION_WATCHDOG_MS); @@ - if (pendingTransition) { - const next = pendingTransition; - pendingTransition = null; - handleDone(next.previous, next.instant); - } + drainPendingTransition();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@immichFrame.Web/src/lib/components/home-page/home-page.svelte` around lines 176 - 202, The pending-transition drain is duplicated and calls the async handleDone fire-and-forget (risking unhandled rejections); extract the duplicate logic into a single helper (e.g., drainPendingTransition) that checks pendingTransition, clears it, calls await handleDone(next.previous, next.instant) inside a try/catch to surface or log errors, and then replace both the watchdog callback block and the finally block with calls to that helper (also ensure watchdogTimer/videoStallTimeout clearing and isHandlingAssetTransition toggling remain in the finally flow).
🤖 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 165-204: The watchdog race occurs because handleDone invocations
share global state (isHandlingAssetTransition, watchdogTimer, videoStallTimeout,
pendingTransition) and a hung call can later clobber timers/state for a retry;
modify handleDone to use an ownership guard: create a local transitionEpoch (or
capture the watchdogTimer id) at start of the transition (e.g., increment a
module-level transitionEpoch and store it in a localEpoch variable) and only
perform finally-block cleanup (resetting isHandlingAssetTransition, clearing
watchdogTimer/videoStallTimeout, and draining pendingTransition) if the current
localEpoch still matches the global epoch (or the captured timer id matches the
active id). Apply this change around the try/finally in handleDone so that only
the owning invocation clears timers and releases the lock; keep existing logic
for scheduling the watchdog and invoking pendingTransition but gate those
actions by the epoch/timer ownership check.
---
Nitpick comments:
In `@immichFrame.Web/src/lib/components/home-page/home-page.svelte`:
- Around line 176-202: The pending-transition drain is duplicated and calls the
async handleDone fire-and-forget (risking unhandled rejections); extract the
duplicate logic into a single helper (e.g., drainPendingTransition) that checks
pendingTransition, clears it, calls await handleDone(next.previous,
next.instant) inside a try/catch to surface or log errors, and then replace both
the watchdog callback block and the finally block with calls to that helper
(also ensure watchdogTimer/videoStallTimeout clearing and
isHandlingAssetTransition toggling remain in the finally flow).
🪄 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: 2e84cced-8575-4518-9aad-bd97cebddd70
📒 Files selected for processing (1)
immichFrame.Web/src/lib/components/home-page/home-page.svelte
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
immichFrame.Web/src/lib/components/home-page/home-page.svelte (1)
515-528:⚠️ Potential issue | 🟠 MajorStall-timer gating regressions from prior reviews are still present.
Three items previously flagged and marked "✅ Addressed" aren't actually reflected in this revision:
onVideoWaitingarmsvideoStallTimeoutunconditionally, and its callback doesn't checkuserPaused. If the user pauses while a video is buffering, the timer still fires after ~15 s andhandleDone(false, true)skips the asset out from under the paused user.clearTimeout(videoStallTimeout)inonVideoPlayingis still nested insideif (!userPaused). When the user resumes from a pause that happened during buffering, the previously-armed stall timer is never canceled on the subsequentplayingevent path gated byuserPaused.Math.min(VIDEO_STALL_MS, currentDuration * 1000)has no lower floor, so a 5 s video will skip after 5 s on any transient initial buffering rather than honoring a reasonable grace period.🛠️ Proposed fix
onVideoWaiting={async () => { await progressBar.pause(); clearTimeout(videoStallTimeout); videoStallTimeout = window.setTimeout(() => { + if (userPaused) return; console.warn('Video stalled, skipping...'); handleDone(false, true); - }, Math.min(VIDEO_STALL_MS, currentDuration * 1000)); + }, Math.min(VIDEO_STALL_MS, Math.max(currentDuration * 1000, 5000))); }} onVideoPlaying={async () => { + clearTimeout(videoStallTimeout); if (!userPaused) { await progressBar.play(); - clearTimeout(videoStallTimeout); } }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@immichFrame.Web/src/lib/components/home-page/home-page.svelte` around lines 515 - 528, The stall-timer logic in the onVideoWaiting/onVideoPlaying handlers is regressing previous fixes: change onVideoWaiting to only arm videoStallTimeout when the user is not paused and ensure the timeout callback re-checks userPaused before calling handleDone(false, true); move clearTimeout(videoStallTimeout) out of the if (!userPaused) branch in onVideoPlaying so it always cancels any pending stall timer when playback resumes, and replace Math.min(VIDEO_STALL_MS, currentDuration * 1000) with a bounded value that applies a lower floor (e.g., use Math.max(MIN_VIDEO_GRACE_MS, Math.min(VIDEO_STALL_MS, currentDuration * 1000))) so short videos get a minimum grace period; refer to onVideoWaiting, onVideoPlaying, videoStallTimeout, userPaused, handleDone, VIDEO_STALL_MS, and currentDuration when locating the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@immichFrame.Web/src/lib/components/home-page/home-page.svelte`:
- Around line 515-528: The stall-timer logic in the
onVideoWaiting/onVideoPlaying handlers is regressing previous fixes: change
onVideoWaiting to only arm videoStallTimeout when the user is not paused and
ensure the timeout callback re-checks userPaused before calling
handleDone(false, true); move clearTimeout(videoStallTimeout) out of the if
(!userPaused) branch in onVideoPlaying so it always cancels any pending stall
timer when playback resumes, and replace Math.min(VIDEO_STALL_MS,
currentDuration * 1000) with a bounded value that applies a lower floor (e.g.,
use Math.max(MIN_VIDEO_GRACE_MS, Math.min(VIDEO_STALL_MS, currentDuration *
1000))) so short videos get a minimum grace period; refer to onVideoWaiting,
onVideoPlaying, videoStallTimeout, userPaused, handleDone, VIDEO_STALL_MS, and
currentDuration when locating the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 650a03be-9f80-4f37-b916-fd78e47bc69e
📒 Files selected for processing (1)
immichFrame.Web/src/lib/components/home-page/home-page.svelte
There was a problem hiding this comment.
🧹 Nitpick comments (1)
immichFrame.Web/src/lib/components/home-page/home-page.svelte (1)
161-210: Epoch-guardedhandleDonelooks solid — two small follow-ups.The
transitionEpoch+pendingTransition+ flat watchdog pattern cleanly addresses the hung-transition race flagged in the previous round. Two minor items to consider (both non-blocking):
Stale
videoStallTimeoutcan outlive the transition. It's cleared at the top ofhandleDone(line 171) but not in thefinallyblock. If the outgoing video fireswaitingduringawait getNextAssets()/await tick(),onVideoWaitingarms a stall timer that then outlives the swap and, if no furtherhandleDoneruns first, will fire on the new asset and cause a spurious skip. You can't unconditionally clear it infinally(that would kill a legitimate stall timer armed by the new video inawait assetComponent?.play?.()), but epoch-tagging the stall timer would solve both cases.Re-entrant
handleDonecalls from watchdog andfinallyare not awaited/void'd (lines 181, 206). If the nested transition rejects, it surfaces as an unhandled rejection. A.catch(err => console.error('Chained transition failed:', err))would keep the logs clean.Optional: epoch-guarded stall timer + error-logged re-entry
onVideoWaiting={async () => { await progressBar.pause(); clearTimeout(videoStallTimeout); if (userPaused) return; + const stallEpoch = transitionEpoch; videoStallTimeout = window.setTimeout(() => { - if (!userPaused) { + if (!userPaused && stallEpoch === transitionEpoch) { console.warn('Video stalled, skipping...'); handleDone(false, true); } }, Math.max(5000, Math.min(VIDEO_STALL_MS, currentDuration * 1000))); }}- if (pendingTransition) { - const next = pendingTransition; - pendingTransition = null; - handleDone(next.previous, next.instant); - } + if (pendingTransition) { + const next = pendingTransition; + pendingTransition = null; + handleDone(next.previous, next.instant).catch((err) => + console.error('Chained transition failed:', err) + ); + }(apply the
.catchto both the watchdog re-entry at line 181 and thefinallyre-entry at line 206)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@immichFrame.Web/src/lib/components/home-page/home-page.svelte` around lines 161 - 210, The watchdog and finally blocks call handleDone(...) without handling rejections and the videoStallTimeout can cross epochs; update handleDone to (1) epoch-tag any stall timer set by onVideoWaiting by capturing transitionEpoch when you arm/reset videoStallTimeout so the timeout callback checks that its epoch matches transitionEpoch before acting (use the existing transitionEpoch and videoStallTimeout identifiers), and (2) attach a .catch(err => console.error('Chained transition failed:', err)) (or equivalent Promise error handling) to both re-entrant handleDone calls invoked from the watchdog timer and from the finally block so nested transitions cannot produce unhandled rejections (references: handleDone, onVideoWaiting, videoStallTimeout, transitionEpoch, watchdogTimer, pendingTransition).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@immichFrame.Web/src/lib/components/home-page/home-page.svelte`:
- Around line 161-210: The watchdog and finally blocks call handleDone(...)
without handling rejections and the videoStallTimeout can cross epochs; update
handleDone to (1) epoch-tag any stall timer set by onVideoWaiting by capturing
transitionEpoch when you arm/reset videoStallTimeout so the timeout callback
checks that its epoch matches transitionEpoch before acting (use the existing
transitionEpoch and videoStallTimeout identifiers), and (2) attach a .catch(err
=> console.error('Chained transition failed:', err)) (or equivalent Promise
error handling) to both re-entrant handleDone calls invoked from the watchdog
timer and from the finally block so nested transitions cannot produce unhandled
rejections (references: handleDone, onVideoWaiting, videoStallTimeout,
transitionEpoch, watchdogTimer, pendingTransition).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a4d4dcba-e874-4cea-922e-0263acfa2a84
📒 Files selected for processing (1)
immichFrame.Web/src/lib/components/home-page/home-page.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.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Closes #601