fix: Fix slides from getting stuck on live photos caused by polling loop#586
fix: Fix slides from getting stuck on live photos caused by polling loop#586mikemulhearn wants to merge 2 commits into
Conversation
WalkthroughModified HTMX request handling to restart polling specifically when asset requests complete, whilst removing automatic polling restart from the Home template. Updated request locking to apply only to asset paths. Added debug logging for polling initiation timestamps. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant HTMX
participant Kiosk as kiosk.ts
participant Server
rect rgb(240, 248, 255)
Note over User,Server: Previous Flow (Removed)
User->>HTMX: Trigger request
HTMX->>Server: Send request
Server-->>HTMX: Response
HTMX->>Kiosk: hx-on::after-request<br/>(startPolling)
Kiosk->>Kiosk: Resume polling
end
rect rgb(245, 255, 250)
Note over User,Server: New Flow (Asset Request)
User->>HTMX: Trigger asset request
HTMX->>Server: GET /asset/...
Server-->>HTMX: Asset response
HTMX->>Kiosk: afterRequest handler<br/>(path check)
alt Asset path detected
Kiosk->>Kiosk: Resume polling
else Non-asset path
Kiosk->>Kiosk: Skip polling restart
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🧰 Additional context used🧬 Code graph analysis (1)frontend/src/ts/kiosk.ts (1)
🔇 Additional comments (3)
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: 0
🧹 Nitpick comments (2)
internal/routes/routes_asset_helpers.go (1)
373-389: Well-designed helper function centralising blur-skip logic.The
shouldSkipBlurfunction clearly encapsulates the conditions for skipping blur processing:
- When background blur is explicitly disabled
- When using "cover" fit without Live Photos enabled
The function is well-documented and the logic is straightforward.
internal/routes/routes_test.go (1)
159-225: Comprehensive test coverage for blur-skipping logic.The table-driven test covers the key scenarios for
shouldSkipBlur, including combinations ofBackgroundBlur,ImageFit, andLivePhotos. The test structure is clear and well-organised.However, several test cases include fields like
ImageEffectandLayoutthat aren't actually checked byshouldSkipBlur. For example, the test at line 181 ("image effect zoom") passes becauseLivePhotosdefaults tofalse, not because ofImageEffect. Consider either:
- Removing irrelevant fields from test configs to avoid confusion
- Adding comments explaining which fields are relevant to the logic being tested
This is a minor clarity issue and doesn't affect correctness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/src/ts/kiosk.ts(2 hunks)frontend/src/ts/polling.ts(1 hunks)internal/routes/routes_asset_helpers.go(1 hunks)internal/routes/routes_test.go(1 hunks)internal/templates/views/views_home.templ(0 hunks)internal/video/video.go(0 hunks)
💤 Files with no reviewable changes (2)
- internal/video/video.go
- internal/templates/views/views_home.templ
🧰 Additional context used
🧬 Code graph analysis (3)
internal/routes/routes_test.go (1)
internal/config/config.go (1)
Config(208-436)
frontend/src/ts/kiosk.ts (1)
frontend/src/ts/polling.ts (1)
startPolling(379-379)
internal/routes/routes_asset_helpers.go (2)
internal/config/config.go (1)
Config(208-436)internal/immich/immich.go (2)
AssetType(22-22)ImageType(33-33)
🔇 Additional comments (4)
frontend/src/ts/polling.ts (1)
129-132: LGTM! Helpful debug logging added.The debug log provides useful visibility into when polling starts and the configured interval, which will aid in diagnosing any future polling-related issues.
frontend/src/ts/kiosk.ts (2)
291-301: Path-aware polling restart correctly implemented.This handler ensures
startPolling()is only called for asset endpoint responses (paths starting with/asset/), preventing non-asset requests like/live/{id}and/refresh/checkfrom interfering with the slideshow timing. The implementation aligns well with the PR objectives.
430-435: Request locking correctly limited to asset requests.The early return for non-asset paths prevents
/live/,/refresh/check, and/weatherrequests from pausing polling or setting the request lock, which resolves the root cause of the slideshow freeze. The path check is clear and follows the same pattern as the after-request handler.internal/routes/routes_asset_helpers.go (1)
395-395: I'll help you verify the refactor of the blur-skipping logic. Let me search for the relevant implementations to confirm the behavior equivalence.
<function_calls>
#!/bin/bashSearch for processBlurredImage and related blur logic in the Go codebase
rg -nC5 'processBlurredImage' --type=go
</function_calls>
#!/bin/bashFind shouldSkipBlur function definition
rg -nC10 'func shouldSkipBlur' --type=go
</function_calls>
#!/bin/bashLook at the context around line 395 in routes_asset_helpers.go
sed -n '385,405p' internal/routes/routes_asset_helpers.go
</function_calls>
<function_calls>
#!/bin/bashSearch for git history to find previous implementation
git log -p --follow -S 'shouldSkipBlur' -- internal/routes/routes_asset_helpers.go | head -150
</function_calls>
#!/bin/bashGet file line count to understand structure
wc -l internal/routes/routes_asset_helpers.go
</function_calls>
#!/bin/bashRead the shouldSkipBlur function and surrounding context
rg -nB2 -A20 'func shouldSkipBlur' --type=go
</function_calls>
#!/bin/bashSearch for any inline blur condition logic that might have been replaced
rg -nC8 'skipBlur\s*:=' --type=go internal/routes/routes_asset_helpers.go
</function_calls>Let me wait for these results to analyze the blur logic refactoring.
mikemulhearn
force-pushed
the
feat/live_photo_endless_loop_fix
branch
from
3e4b142 to
aae9cbb
Compare
|
Accidentally included the background blur commits originally, rebased and removed those commits just now |
|
Did this fix the issue you were having?
|
So far, so good. I would like to continue to do some more A/B testing between existing version and this PR to ensure no bugs or other issues are introduced. I have a variety of low and high-power devices I am testing on to ensure solid performance and resilience: PC Web browsers, low and mid-range Android tablets, iPad, iPhone, Fire TV sticks, Echo Show 8 & 15, and Frameo Frame (Android 6). During my initial debugging phase, I found that During debugging, I changed the Snippet of actual debug log at that time: I'd be happy to take another look, and move the |
|
I think the thing that might have tripped you up here is that using <!-- kiosk.startPolling() only triggers from this element e.g. /assets/* -->
<main
id="kiosk"
hx-post="/asset/new"
hx-on::after-request="kiosk.startPolling()"
>where as using this catches/is triggered on all htmx request htmx.on("htmx:afterRequest", (e: HTMXEvent) => {
// gotta catch em all
});That's why you would have seen Does that make sense? |
|
Are you still seeing this issue @mikemulhearn ? |
|
Confirming this fixes the same bug for me — also reported in #673. Setup: v0.38.0, splitview, Here is what Claude thinks is also worth mentioning: @damongolding I think there's a small misunderstanding worth surfacing. The relevant descendant is the live-photo poller in <div
style="display: none"
hx-get={ "/live/" + imageData.ImmichAsset.LivePhotoVideoID }
hx-swap="outerHTML"
hx-trigger="load, every 1s"
></div>Every second this fires |
3 participants
This PR fixes an issue where the slideshow would freeze indefinitely on some slides containing Live Photos.
The introduction of Live Photos, which is such an awesome feature, resulted in my slideshow getting stuck on certain live photos, usually the same ones each time. The progress bar was doing the initial polling from 0 to a small value, but then it kept appearing to restart the poll process, and the live photos were not playing nor were the slides advancing after the configured 15 seconds.
The fix ensures that the slideshow timer (polling) is only paused and reset for real slide-change requests (
/asset/newand/asset/offline) and not for background requests such as/live/{id}(Live Photo video loading) or/refresh/check(server health checks).Potentially relevant config options
Logs prior to the fix:
Root Cause
Prior to this PR:
1. Every HTMX request triggered setRequestLock(), including:
/asset/new/asset/offline/live/<id>/weathersetRequestLock()called:pausePolling(false)This canceled the active animation frame, pausing the slideshow progress.
2. Polling was not being restarted for non-asset endpoints
The original reset loop bug was fixed by ensuring only
/asset/*calls restart polling.But because
/refresh/checkand/live/<id>still invokedsetRequestLock()(pause) and did not restart polling afterward, the slideshow remained indefinitely paused.The Fix
1. Only pause polling for real slide-changing (asset) requests
In
setRequestLock():This prevent
pausePolling()from running for:/live/id(live photos)/refresh/check/weather2. Polling restart is handled exclusively by TypeScript
Template
hx-on::after-request="kiosk.startPolling()"was removed.A new TS-only handler restarts polling exclusively for asset routes:
Result
/asset/newloads -- slideshow starts normally/live/<id>fails repeatedly -- slideshow keeps running anyway/refresh/checkcontinues -- slideshow unaffectedTesting
/asset/triggersstartPolling()/liveand/refresh/checkdo not pause or reset pollingImpact
This PR:
Conclusion
This PR resolves an issue where Live Photos could freeze the slideshow. By ensuring polling is controlled only by actual asset transitions, and never by background requests, the slideshow becomes fully stable and predictable, even when Live Photo videos fail.
Summary by CodeRabbit
Release Notes
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.