Fix SWR metadata refresh, rounded images, and Jellyfin 12 compatibility - #133
Conversation
📝 WalkthroughWalkthroughThe PR pins Jellyfin compatibility to the official 12.0 OpenAPI contract, updates catalog, playback, media-segment, and session routes, adds asynchronous rounded-image handling with a software fallback, and changes stable library refreshes to update rows in place. ChangesJellyfin compatibility and playback
Rounded-image rendering
Library refresh and build configuration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
Greptile SummaryThis PR updates SWR metadata refreshes, introduces asynchronous and software-rendered rounded-image paths, gates test-only Qt dependencies, and migrates Jellyfin requests to the pinned Jellyfin 12 contract.
Confidence Score: 4/5The bulk Jellyfin revocation lifecycle should be fixed before merging because it reports completion and re-enables controls while device deletions remain in flight. Multiple device deletions share a single boolean loading state, each completion clears that state independently, and the aggregate completion signal is emitted immediately after requests are scheduled. Files Needing Attention: src/network/SessionService.cpp and tests/CMakeLists.txt
|
| Filename | Overview |
|---|---|
| src/network/SessionService.cpp | Migrates Jellyfin revocation to device deletion, but bulk operations do not aggregate asynchronous completion or loading state. |
| src/ui/ImageCacheProvider.cpp | Adds non-blocking rounded-variant lookup, coalescing, ready-map touches, and generation-aware invalidation. |
| src/ui/RoundedImage.qml | Replaces rectangular software clipping with a lazily loaded CPU-painted fallback. |
| src/ui/SoftwareRoundedImage.cpp | Implements generation-checked item capture and antialiased rounded CPU painting. |
| src/viewmodels/LibraryViewModel.cpp | Detects full canonical metadata changes and updates stable rows with targeted dataChanged notifications. |
| src/providers/jellyfin/JellyfinCatalogProvider.h | Migrates catalog and state operations to Jellyfin 12 routes and response envelopes. |
| src/network/PlaybackService.cpp | Updates playback information, segment, item lookup, trickplay, and played-state requests for Jellyfin 12. |
| tests/CMakeLists.txt | Adds rounded-image implementation sources directly to two tests, contrary to the production-target linking policy. |
Sequence Diagram
sequenceDiagram
participant UI as Active Sessions UI
participant SS as SessionService
participant JF as Jellyfin
UI->>SS: revokeAllOtherSessions()
SS->>JF: GET /Sessions
JF-->>SS: sessions grouped by device
loop Each distinct remote device
SS->>JF: "DELETE /Devices?id=deviceId"
end
SS-->>UI: allOtherSessionsRevoked(count)
Note over SS,UI: Emitted before DELETE requests settle
JF-->>SS: First DELETE completes
SS-->>UI: "isLoading=false"
JF-->>SS: Remaining DELETE completions
Comments Outside Diff (1)
-
src/network/SessionService.cpp, line 288-292 (link)Bulk revocation completes prematurely
If at least two distinct remote Jellyfin devices are active, the loop starts multiple asynchronous deletions but emits
allOtherSessionsRevokedimmediately, and the first completed request clearsisLoadingwhile the others remain in flight, causing session controls to be re-enabled before bulk revocation finishes.Knowledge Base Used: Network Services
Prompt To Fix With AI
This is a comment left during a code review. Path: src/network/SessionService.cpp Line: 288-292 Comment: **Bulk revocation completes prematurely** If at least two distinct remote Jellyfin devices are active, the loop starts multiple asynchronous deletions but emits `allOtherSessionsRevoked` immediately, and the first completed request clears `isLoading` while the others remain in flight, causing session controls to be re-enabled before bulk revocation finishes. **Knowledge Base Used:** [Network Services](https://app.greptile.com/bloom-org-2/-/custom-context/knowledge-base/crowquillx/bloom/-/docs/network-services.md) --- For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Prompt To Fix All With AI
### Issue 1
src/network/SessionService.cpp:288-292
**Bulk revocation completes prematurely**
If at least two distinct remote Jellyfin devices are active, the loop starts multiple asynchronous deletions but emits `allOtherSessionsRevoked` immediately, and the first completed request clears `isLoading` while the others remain in flight, causing session controls to be re-enabled before bulk revocation finishes.
### Issue 2
tests/CMakeLists.txt:487-490
**Tests rebuild production implementation**
`RoundedImageTest` and `VisualRegressionTest` compile `SoftwareRoundedImage.cpp` directly instead of linking the production target that owns it, allowing tests to exercise copies built with dependencies or definitions that differ from the application.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix: harden reliability and Jellyfin 12 ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/network/SessionService.cpp (1)
267-292: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftWait for device deletions before emitting bulk success.
Lines 281-288 start asynchronous requests. Line 292 emits
allOtherSessionsRevokedbefore any DELETE completes. A later failure can leave a remote device authenticated after the UI reports success. Track pending device deletions and emit the success count only after all requests succeed.🤖 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 `@src/network/SessionService.cpp` around lines 267 - 292, The sessionsLoaded handler currently emits allOtherSessionsRevoked immediately after starting asynchronous revokeSession calls. Track pending device deletions and their failures around revokeSession, then emit allOtherSessionsRevoked only after every requested deletion completes successfully; propagate an operationFailed result if any deletion fails, while preserving the current-device exclusion and revoked count.
🧹 Nitpick comments (7)
src/ui/RoundedImage.qml (1)
69-97: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueGeometry refreshes are scheduled twice.
SoftwareRoundedImagealready connectswidthChangedandheightChangedtorequestRefresh(). The loaded item fills theLoader, so a size change ofrootalso changes the item size. Lines 89-90 therefore schedule a secondrequestRefresh()for the same geometry change, which discards the first capture and starts anothergrabToImage().Consider limiting
onWidthChanged/onHeightChangedtorefreshStaticShaderSource()and letting the C++ item handle its own size changes.🤖 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 `@src/ui/RoundedImage.qml` around lines 69 - 97, Update the onWidthChanged and onHeightChanged handlers in RoundedImage so they only call refreshStaticShaderSource(), removing the refreshSoftwareFallback path for root geometry changes. Keep other refreshRenderPaths triggers unchanged and rely on SoftwareRoundedImage’s own size-change handling for fallback image refreshes.src/ui/LibraryScreen.qml (1)
1712-1721: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueQualify the root-scope property reads.
Lines 1713 read
roundedImageModeandroundedPreprocessEnabledwithout a qualifier. Both are declared onrootat Line 508-509. TheConnectionsblock at Line 1730-1738 already targetsrootfor the same properties. Useroot.roundedImageModeandroot.roundedPreprocessEnabledso the lookup is explicit andqmllintstays clean.🤖 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 `@src/ui/LibraryScreen.qml` around lines 1712 - 1721, Update getRoundedImageSource to qualify both root-declared property reads as root.roundedImageMode and root.roundedPreprocessEnabled, matching the existing root-targeted Connections usage and preserving the current condition behavior.tests/RoundedImageTest.cpp (2)
83-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that both source replacements matched.
Lines 84-90 patch the production QML by exact byte-string match. The second pattern depends on exactly 12 leading spaces before
SoftwareRoundedImage {insrc/ui/RoundedImage.qml. A reindentation of that file makes the replacement a no-op. The component then fails to resolve the type, and the test reports a generic QML error instead of the real cause.Verify the replacement count.
♻️ Proposed fix that fails with a clear message
QByteArray qmlSource = qmlFile.readAll(); + QVERIFY2(qmlSource.contains(QByteArrayLiteral("SoftwareRoundedImage {")), + "RoundedImage.qml no longer instantiates SoftwareRoundedImage"); + const qsizetype typeOffset = + qmlSource.indexOf(QByteArrayLiteral("SoftwareRoundedImage {")); qmlSource.replace( QByteArrayLiteral("import QtQuick\n"), QByteArrayLiteral( "import QtQuick\nimport BloomInternal 1.0 as BloomInternal\n")); - qmlSource.replace( - QByteArrayLiteral(" SoftwareRoundedImage {"), - QByteArrayLiteral(" BloomInternal.SoftwareRoundedImage {")); + Q_UNUSED(typeOffset); + const QByteArray before = qmlSource; + qmlSource.replace(QByteArrayLiteral("SoftwareRoundedImage {"), + QByteArrayLiteral("BloomInternal.SoftwareRoundedImage {")); + QVERIFY2(qmlSource != before, "SoftwareRoundedImage type name was not qualified");🤖 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 `@tests/RoundedImageTest.cpp` around lines 83 - 91, Update the replacement logic in the RoundedImageTest setup to verify that each exact byte-string replacement matched once. Assert the replacement count for both import and SoftwareRoundedImage substitutions before calling component.setData, so formatting changes fail with a clear test message.
163-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for a refresh after the snapshot is ready.
The software branch validates the first snapshot only. It does not exercise
requestRefresh()aftersoftwareFallbackReadybecomes true. That path is the one wherebaseImage.visibleis already false when the nextgrabToImage()runs, which is the risk raised onsrc/ui/RoundedImage.qmlLine 120-122.Resize the item after the first assertion and re-assert the corner and center pixels.
🤖 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 `@tests/RoundedImageTest.cpp` around lines 163 - 177, Extend the software branch after the existing corner and center assertions to resize the item, trigger the refresh path via the established resize behavior, wait for the updated snapshot, and capture the window again. Re-assert the corner matches background and the center matches blue, covering requestRefresh() after softwareFallbackReady is true.src/ui/ImageCacheProvider.cpp (1)
1005-1054: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the repeated
--m_activeRoundedTaskswith a scope guard.The worker lambda decrements
m_activeRoundedTasksat five separate return points. Any future early return that misses the decrement leaks the counter and corrupts theactiveRoundedTasksstatistic permanently. A single guard at the top of the lambda removes that class of defect.♻️ Proposed refactor
++m_activeRoundedTasks; auto future = QtConcurrent::run( &m_threadPool, [this, url, key, sourcePath, radiusPx, targetSize, generation]() { + const QScopeGuard taskGuard([this]() { --m_activeRoundedTasks; }); RoundedTaskResult result; if (m_cacheGeneration.load() != generation || !m_store) { - --m_activeRoundedTasks; return result; }Remove the remaining
--m_activeRoundedTasks;statements and add#include <QScopeGuard>.🤖 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 `@src/ui/ImageCacheProvider.cpp` around lines 1005 - 1054, Update the worker lambda passed to QtConcurrent::run so it creates a QScopeGuard at the start that decrements m_activeRoundedTasks on scope exit, then remove all explicit --m_activeRoundedTasks statements from its return paths. Add the QScopeGuard include required by this guard.src/cmake/BloomLibraries.cmake (1)
117-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a dedicated option instead of
BUILD_TESTINGfor the test hook.
BUILD_TESTINGdefaults toONas soon asinclude(CTest)runs. Any configuration that enables CTest then compilesblockCacheWorkerForTestandblockWorkerForTestinto the shippedBloombinary, because the definition isPUBLICand the executable linksBloom::ImageCache. The hook blocks the cache worker thread indefinitely until a semaphore release. A separate option, for exampleBLOOM_ENABLE_TEST_HOOKS, keeps the hook out of release artifacts whileBUILD_TESTINGremains free to control test registration.
PUBLICis correct here and must stay, otherwise the header declaration and the library definition diverge between the library and its consumers.♻️ Proposed gating change
-if(BUILD_TESTING) +option(BLOOM_ENABLE_TEST_HOOKS "Compile test-only synchronization hooks" ${BUILD_TESTING}) +if(BLOOM_ENABLE_TEST_HOOKS) target_compile_definitions(BloomImageCache PUBLIC BLOOM_TESTING=1) endif()🤖 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 `@src/cmake/BloomLibraries.cmake` around lines 117 - 119, Replace the BUILD_TESTING condition around target_compile_definitions with a dedicated BLOOM_ENABLE_TEST_HOOKS option, defaulting it OFF so test hooks are excluded from normal and release builds while CTest can still control test registration. Keep the BLOOM_TESTING definition PUBLIC for Bloom::ImageCache consumers, and ensure the new option is declared in the project’s existing CMake option configuration.tests/ArtworkRefreshTest.cpp (1)
522-532: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe 150 ms wall-clock threshold is flaky on loaded CI runners.
The release happens after 300 ms, so the assertion has only a 150 ms margin. On a contended runner, thread scheduling and the
QMetaObject::invokeMethoddispatch alone can consume that margin without any regression in the non-blocking behavior. The test then fails for reasons unrelated to the property under test.Assert ordering instead of duration: check that
requestRoundedImagereturns whilereleaseWorkeris still unreleased. That is the actual invariant and it does not depend on machine speed.♻️ Proposed refactor: assert ordering, keep a generous time bound
- QElapsedTimer timer; - timer.start(); + std::atomic<bool> released{false}; + auto delayedRelease = QtConcurrent::run([&releaseWorker, &released]() { + QThread::msleep(300); + released.store(true); + releaseWorker.release(); + }); + + QElapsedTimer timer; + timer.start(); QCOMPARE(cache.requestRoundedImage( QStringLiteral("https://images.example.test/busy-worker.png"), 16, 32, 32), QString()); + QVERIFY2(!released.load(), + "rounded request did not return before the cache worker resumed"); const qint64 elapsedMs = timer.elapsed(); - QVERIFY2(elapsedMs < 150, + QVERIFY2(elapsedMs < 300, qPrintable(QStringLiteral( "rounded request blocked for %1 ms on cache worker") .arg(elapsedMs)));Move the existing
delayedReleasedeclaration to the position shown above.🤖 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 `@tests/ArtworkRefreshTest.cpp` around lines 522 - 532, Replace the elapsed-time assertion around requestRoundedImage with an ordering assertion that verifies the worker remains unreleased when the request returns. Move delayedRelease’s declaration before the request as indicated, retain a generous timeout only if needed to prevent hangs, and remove the strict 150 ms wall-clock threshold.
🤖 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.
Inline comments:
In `@src/network/SessionService.cpp`:
- Around line 219-244: Update the session-revocation flow around
deviceIdForSession and onRevokeSessionFinished to detect self-revocation by
comparing the target deviceId with the current device identity, not only by
comparing sessionId with m_currentSessionId. When they match, emit
selfSessionRevoked even if the selected session differs from m_currentSessionId,
while preserving the existing behavior for other devices.
In `@src/ui/ImageCacheProvider.cpp`:
- Around line 1089-1095: Update the rounded-render completion handling around
renderRoundedPng and processPendingRounded so terminal render or save failures
remove the corresponding m_pendingRounded entry and emit the existing
failure/fallback signal, preventing callers from waiting indefinitely. Also
bound or remove stale m_knownBaseImages records after processing, while
preserving the retry behavior for valid known-base entries.
- Around line 1194-1221: In ImageCacheProvider::touchRoundedVariantAsync, limit
m_cacheMutationMutex to checking the current generation and m_store, then
release it before calling m_store->touch(key). Preserve the guarded touch
behavior by only invoking touch when both conditions were true, avoiding any
mutex hold during the blocking store call.
- Around line 1153-1191: Update the rounded-variant and known-base-image caching
flow around m_readyRounded, m_knownBaseImages, and lookupEntry() to track
ImageCacheStore::LookupResult::revision per cache key instead of using the
global m_cacheContentRevision for validity. Preserve entries whose individual
revisions remain valid after unrelated writes, and remove or invalidate the
corresponding in-memory entries whenever eviction or deletion removes a cache
key.
In `@src/ui/ImageCacheStore.cpp`:
- Around line 882-896: Update ImageCacheStore::blockWorkerForTest so the queued
lambda uses a bounded QSemaphore wait instead of indefinitely calling
release->acquire(), allowing the worker to resume when the timeout expires.
Document that callers must keep both stack-owned semaphores alive until the
worker resumes, while preserving the existing entered notification and test-only
scope.
In `@src/ui/LibraryScreen.qml`:
- Around line 1789-1794: Update the Image error handling near
delegateItem.getImageSource() so it records the failure in a separate property
instead of assigning directly to source. Preserve the source binding for live
delegates, and use the error state only to represent the failed load without
requiring GridView.onReused to restore the binding.
In `@src/ui/SoftwareRoundedImage.cpp`:
- Around line 61-112: Update SoftwareRoundedImage::requestRefresh() to schedule
the initial retry without replacing m_retryTimer’s configured 32 ms interval,
and add a retry-attempt counter checked by attemptGrab() to stop retries after a
bounded maximum. Reset that counter in requestRefresh(), increment it only for
retryable failures (missing window, failed grab, or null image), and preserve
the existing generation and readiness behavior.
- Around line 87-92: Update the capture target construction in the image-grab
flow to multiply both dimensions by
m_sourceItem->window()->effectiveDevicePixelRatio(), while preserving the
minimum-size and rounding behavior. Also handle ItemDevicePixelRatioHasChanged
in the relevant item-change logic so the capture refreshes when the screen DPR
changes, not only when windowChanged fires.
In `@tests/CMakeLists.txt`:
- Around line 487-497: Introduce or reuse the narrowest production Bloom::* UI
library owning SoftwareRoundedImage, then update tests/CMakeLists.txt lines
487-497 for RoundedImageTest and lines 1017-1018 for VisualRegressionTest to
remove SoftwareRoundedImage.h and SoftwareRoundedImage.cpp from each target’s
sources and link that same library instead, preserving an acyclic production
dependency structure.
In `@tests/contracts/provider-contracts.json`:
- Around line 511-513: Update the contract entry’s method value for the
composite PlaybackInfo and AdditionalParts flow to represent both operations,
using the established POST/GET convention or splitting it into separate
contracts while preserving the existing paths and request semantics.
In `@tests/contracts/validate_contracts.py`:
- Around line 163-171: The Jellyfin OpenAPI pin must be backed by a real
checksum-verified artifact rather than repeated metadata and handwritten
assertions. In tests/contracts/validate_contracts.py lines 163-171, replace the
blob URL-only validation with validation of the stored raw or generated manifest
artifact and derive the OpenAPI assertions from it; update
tests/contracts/provider_contracts_test.py lines 156-201 and
tests/ProviderCatalogTest.cpp lines 290-330 to consume the same derived data
instead of literals or handwritten openApiItemFields. Document artifact
retrieval and checksum verification in docs/provider-compatibility.md lines
33-44.
---
Outside diff comments:
In `@src/network/SessionService.cpp`:
- Around line 267-292: The sessionsLoaded handler currently emits
allOtherSessionsRevoked immediately after starting asynchronous revokeSession
calls. Track pending device deletions and their failures around revokeSession,
then emit allOtherSessionsRevoked only after every requested deletion completes
successfully; propagate an operationFailed result if any deletion fails, while
preserving the current-device exclusion and revoked count.
---
Nitpick comments:
In `@src/cmake/BloomLibraries.cmake`:
- Around line 117-119: Replace the BUILD_TESTING condition around
target_compile_definitions with a dedicated BLOOM_ENABLE_TEST_HOOKS option,
defaulting it OFF so test hooks are excluded from normal and release builds
while CTest can still control test registration. Keep the BLOOM_TESTING
definition PUBLIC for Bloom::ImageCache consumers, and ensure the new option is
declared in the project’s existing CMake option configuration.
In `@src/ui/ImageCacheProvider.cpp`:
- Around line 1005-1054: Update the worker lambda passed to QtConcurrent::run so
it creates a QScopeGuard at the start that decrements m_activeRoundedTasks on
scope exit, then remove all explicit --m_activeRoundedTasks statements from its
return paths. Add the QScopeGuard include required by this guard.
In `@src/ui/LibraryScreen.qml`:
- Around line 1712-1721: Update getRoundedImageSource to qualify both
root-declared property reads as root.roundedImageMode and
root.roundedPreprocessEnabled, matching the existing root-targeted Connections
usage and preserving the current condition behavior.
In `@src/ui/RoundedImage.qml`:
- Around line 69-97: Update the onWidthChanged and onHeightChanged handlers in
RoundedImage so they only call refreshStaticShaderSource(), removing the
refreshSoftwareFallback path for root geometry changes. Keep other
refreshRenderPaths triggers unchanged and rely on SoftwareRoundedImage’s own
size-change handling for fallback image refreshes.
In `@tests/ArtworkRefreshTest.cpp`:
- Around line 522-532: Replace the elapsed-time assertion around
requestRoundedImage with an ordering assertion that verifies the worker remains
unreleased when the request returns. Move delayedRelease’s declaration before
the request as indicated, retain a generous timeout only if needed to prevent
hangs, and remove the strict 150 ms wall-clock threshold.
In `@tests/RoundedImageTest.cpp`:
- Around line 83-91: Update the replacement logic in the RoundedImageTest setup
to verify that each exact byte-string replacement matched once. Assert the
replacement count for both import and SoftwareRoundedImage substitutions before
calling component.setData, so formatting changes fail with a clear test message.
- Around line 163-177: Extend the software branch after the existing corner and
center assertions to resize the item, trigger the refresh path via the
established resize behavior, wait for the updated snapshot, and capture the
window again. Re-assert the corner matches background and the center matches
blue, covering requestRefresh() after softwareFallbackReady is true.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 154ba315-8496-43a0-ab42-5af32b1ece1c
📒 Files selected for processing (43)
AGENTS.mdCMakeLists.txtdocs/build.mddocs/canonical-models.mddocs/images.mddocs/media-segments.mddocs/playback.mddocs/provider-compatibility.mddocs/viewmodels.mdsrc/CMakeLists.txtsrc/cmake/BloomLibraries.cmakesrc/network/LibraryService.cppsrc/network/PlaybackService.cppsrc/network/SessionService.cppsrc/network/SessionService.hsrc/providers/jellyfin/JellyfinCatalogProvider.hsrc/providers/jellyfin/JellyfinModelMapper.cppsrc/providers/jellyfin/JellyfinModelMapper.hsrc/providers/jellyfin/JellyfinPlaybackProvider.cppsrc/providers/jellyfin/JellyfinPlaybackProvider.hsrc/providers/jellyfin/JellyfinProviderAdapter.hsrc/providers/jellyfin/JellyfinRequestFactory.cppsrc/ui/ImageCacheProvider.cppsrc/ui/ImageCacheProvider.hsrc/ui/ImageCacheStore.cppsrc/ui/ImageCacheStore.hsrc/ui/LibraryScreen.qmlsrc/ui/RoundedImage.qmlsrc/ui/SoftwareRoundedImage.cppsrc/ui/SoftwareRoundedImage.hsrc/viewmodels/LibraryViewModel.cpptests/ArtworkRefreshTest.cpptests/CMakeLists.txttests/CanonicalModelsTest.cpptests/LibraryViewModelCanonicalTest.cpptests/PlayerControllerAutoplayContextTest.cpptests/ProviderCatalogTest.cpptests/ProviderTransportTest.cpptests/RoundedImageTest.cpptests/contracts/provider-contracts.jsontests/contracts/provider_contracts_test.pytests/contracts/run_live_contracts.pytests/contracts/validate_contracts.py
💤 Files with no reviewable changes (1)
- src/network/LibraryService.cpp
|
Automated-review follow-up: bulk device revocation now waits for every distinct DELETE callback, keeps the operation active until the last completion, reports the successful count, recognizes the current session by stable device ID, and uses an explicit fetch-scoped pending flag that is cleared on every refresh failure path ( |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/network/SessionService.cpp (1)
420-451: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winClassify 3xx responses as failures
HttpTransportroutes only status codes>= 400tofailureHandler. A3xxresponse withQNetworkReply::NoErrorreachesresponseHandler, soonRevokeSessionFinishedcan report a failedDELETE /Devicesas successful.HttpRequestOptions::retrySafetyalready defaults toRetrySafety::Never.🤖 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 `@src/network/SessionService.cpp` around lines 420 - 451, Update the HttpTransport response classification so all HTTP 3xx status codes are routed to failureHandler, not responseHandler, even when QNetworkReply reports NoError. Preserve the existing retrySafety::Never default and ensure onRevokeSessionFinished only handles successful DELETE responses.Source: Coding guidelines
♻️ Duplicate comments (1)
src/ui/SoftwareRoundedImage.cpp (1)
67-75: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the configured retry interval.
Line 75 changes
m_retryTimerto a zero-millisecond interval. LaterscheduleRetry()calls then run all retries immediately. If the source is not renderable yet, the component can exhaustMaximumGrabRetriesand remain unready.Use
QTimer::singleShot(0, this, &SoftwareRoundedImage::attemptGrab)for the initial attempt.Proposed fix
- m_retryTimer.start(0); + QTimer::singleShot(0, this, &SoftwareRoundedImage::attemptGrab);Qt 6 QTimer documentation: Does QTimer::start(int msec) update the interval used by a later QTimer::start() call?🤖 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 `@src/ui/SoftwareRoundedImage.cpp` around lines 67 - 75, In SoftwareRoundedImage::requestRefresh(), avoid changing m_retryTimer’s configured retry interval by replacing the zero-duration timer start with a one-shot queued invocation of attemptGrab(). Keep the existing timer stop, state reset, update(), and scheduleRetry() behavior unchanged.
🧹 Nitpick comments (3)
src/network/SessionService.cpp (1)
409-412: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRun the deferred bulk revocation after the load signals.
Line 411 calls
revokeLoadedOtherDevicesbeforeemit sessionsChanged()andemit sessionsLoaded()at Lines 414-415.revokeLoadedOtherDevicescan emitallOtherSessionsRevoked(0), and a synchronously completed delete can emitsessionRevokedandsessionsChangedfrom inside this block. Consumers then observe revocation results before the refreshed session list is published, andsessionsLoadedarrives last.Move the trigger below the two emits, or dispatch it with a queued invocation so the refresh completes first.
♻️ Proposed fix
- if (m_bulkRevokeRefreshPending) { - m_bulkRevokeRefreshPending = false; - revokeLoadedOtherDevices(); - } - emit sessionsChanged(); emit sessionsLoaded(); + + if (m_bulkRevokeRefreshPending) { + m_bulkRevokeRefreshPending = false; + revokeLoadedOtherDevices(); + }🤖 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 `@src/network/SessionService.cpp` around lines 409 - 412, In the session-loading completion flow, move the m_bulkRevokeRefreshPending check and revokeLoadedOtherDevices() call to after emit sessionsChanged() and emit sessionsLoaded(). Preserve clearing the pending flag before triggering the revocation so all refreshed-session signals are delivered first.tests/contracts/generate_jellyfin_openapi_manifest.py (2)
55-65: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDeduplicate parameters by
(name, in)and let operation-level entries replace path-level entries. The current generator emits both entries. The validator keeps the later operation-level entry for identical parameters, but the manifest still contains duplicates and its name-only map conflates parameters with the same name in different locations.🤖 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 `@tests/contracts/generate_jellyfin_openapi_manifest.py` around lines 55 - 65, Update the parameter collection in the manifest generator to deduplicate entries by the `(name, in)` pair, processing path-level parameters before operation-level parameters so later operation entries replace matching path entries. Ensure the final manifest contains only one entry per pair and does not use name alone as the identity.
120-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a Nix regeneration check for the Jellyfin manifest.
nix/tests.nixvalidates only the checked-in manifest and never runsgenerate_jellyfin_openapi_manifest.py. Fetch the pinned OpenAPI artifact with its fixed hash, generate to a temporary path, and compare it byte-for-byte withjellyfin-12-openapi-manifest.json.🤖 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 `@tests/contracts/generate_jellyfin_openapi_manifest.py` around lines 120 - 159, Add a Nix test in nix/tests.nix that fetches the pinned Jellyfin OpenAPI artifact with its fixed hash, runs generate_jellyfin_openapi_manifest.py using the pinned contracts, writes the result to a temporary path, and compares it byte-for-byte with jellyfin-12-openapi-manifest.json. Keep the existing manifest validation and ensure the check exercises main’s SHA/version validation.Source: Linters/SAST tools
🤖 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.
Inline comments:
In `@src/network/SessionService.cpp`:
- Around line 285-318: Update revokeLoadedOtherDevices so every queued
revocation that revokeSession cannot dispatch is completed through
finishDeviceRevocation(deviceId, false), or abort the batch while clearing
m_pendingBulkRevokeDeviceIds. Resolve and retain each target device ID before
calling revokeSession, and ensure early authentication, empty-device, or
null-transport returns cannot leave m_isLoading set or prevent
allOtherSessionsRevoked from firing.
- Around line 275-283: Update revokeAllOtherSessions so the m_isLoading
early-return branch emits operationFailed before returning, ensuring callers
receive a terminal signal when the destructive action cannot start; leave the
existing pending-flag and fetchActiveSessions flow unchanged.
In `@src/ui/ImageCacheProvider.h`:
- Around line 183-186: Update the rounded-lookup test hook declarations and
implementation of blockNextRoundedLookupForTest in src/ui/ImageCacheProvider.h
(lines 183-186 and 327-330) to retain both semaphores via
QSharedPointer<QSemaphore> across the worker boundary. In
tests/ArtworkRefreshTest.cpp (lines 560-574), create lookupEntered and
releaseLookup with QSharedPointer<QSemaphore>::create() and pass the shared
pointers to the hook, preserving semaphore usage and timeout behavior.
In `@tests/contracts/validate_contracts.py`:
- Around line 174-181: Update the stream operation handling near the existing
operations validation to retrieve ("GET", "/Videos/{itemId}/stream") safely and
pass its presence through _require, raising ContractValidationError instead of
allowing KeyError. Build stream_parameters only from dictionary parameter
entries, matching the filtering behavior already used around the earlier
parameter-validation loop, then preserve the existing deviceProfileId
deprecation check.
---
Outside diff comments:
In `@src/network/SessionService.cpp`:
- Around line 420-451: Update the HttpTransport response classification so all
HTTP 3xx status codes are routed to failureHandler, not responseHandler, even
when QNetworkReply reports NoError. Preserve the existing retrySafety::Never
default and ensure onRevokeSessionFinished only handles successful DELETE
responses.
---
Duplicate comments:
In `@src/ui/SoftwareRoundedImage.cpp`:
- Around line 67-75: In SoftwareRoundedImage::requestRefresh(), avoid changing
m_retryTimer’s configured retry interval by replacing the zero-duration timer
start with a one-shot queued invocation of attemptGrab(). Keep the existing
timer stop, state reset, update(), and scheduleRetry() behavior unchanged.
---
Nitpick comments:
In `@src/network/SessionService.cpp`:
- Around line 409-412: In the session-loading completion flow, move the
m_bulkRevokeRefreshPending check and revokeLoadedOtherDevices() call to after
emit sessionsChanged() and emit sessionsLoaded(). Preserve clearing the pending
flag before triggering the revocation so all refreshed-session signals are
delivered first.
In `@tests/contracts/generate_jellyfin_openapi_manifest.py`:
- Around line 55-65: Update the parameter collection in the manifest generator
to deduplicate entries by the `(name, in)` pair, processing path-level
parameters before operation-level parameters so later operation entries replace
matching path entries. Ensure the final manifest contains only one entry per
pair and does not use name alone as the identity.
- Around line 120-159: Add a Nix test in nix/tests.nix that fetches the pinned
Jellyfin OpenAPI artifact with its fixed hash, runs
generate_jellyfin_openapi_manifest.py using the pinned contracts, writes the
result to a temporary path, and compares it byte-for-byte with
jellyfin-12-openapi-manifest.json. Keep the existing manifest validation and
ensure the check exercises main’s SHA/version validation.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8583fe25-07dc-4eee-b361-c4b423d74b9f
📒 Files selected for processing (24)
docs/provider-compatibility.mdsrc/CMakeLists.txtsrc/cmake/BloomLibraries.cmakesrc/network/SessionService.cppsrc/network/SessionService.hsrc/providers/jellyfin/JellyfinModelMapper.cppsrc/ui/ImageCacheProvider.cppsrc/ui/ImageCacheProvider.hsrc/ui/ImageCacheStore.cppsrc/ui/ImageCacheStore.hsrc/ui/LibraryScreen.qmlsrc/ui/SoftwareRoundedImage.cppsrc/ui/SoftwareRoundedImage.hsrc/ui/SoftwareRoundedImageRegistration.htests/ArtworkRefreshTest.cpptests/CMakeLists.txttests/ProviderCatalogTest.cpptests/RoundedImageTest.cpptests/VisualRegressionTest.cpptests/contracts/generate_jellyfin_openapi_manifest.pytests/contracts/jellyfin-12-openapi-manifest.jsontests/contracts/provider-contracts.jsontests/contracts/provider_contracts_test.pytests/contracts/validate_contracts.py
🚧 Files skipped from review as they are similar to previous changes (9)
- src/ui/ImageCacheStore.h
- tests/RoundedImageTest.cpp
- src/ui/LibraryScreen.qml
- tests/ProviderCatalogTest.cpp
- docs/provider-compatibility.md
- tests/contracts/provider_contracts_test.py
- src/ui/ImageCacheProvider.cpp
- src/providers/jellyfin/JellyfinModelMapper.cpp
- tests/contracts/provider-contracts.json
1 participant
Summary
dataChangednotifications, preserving delegate identity, scroll, and focusBUILD_TESTINGand removeNO_CACHEGENafter verifying cache generation in a production buildRoot causes and impact
Library refresh equality only considered total count and item identity/order, so canonical metadata changes could be discarded. RoundedImage relied on
Rectangle.clip, whose clip remains rectangular in Qt Quick's software backend. Rounded-image delegates also synchronously crossed into the image-cache worker for lookup/touch operations, allowing a busy SQLite worker to stall the UI thread.The changes preserve structural model resets for insertions, removals, and reordering while applying stable-row metadata updates in place. Cached rounded variants are now discovered and touched asynchronously, duplicate requests remain coalesced, and stale generation results are rejected after cache invalidation.
The Jellyfin audit pins an exact Jellyfin 12 OpenAPI revision and updates library, item detail, playback, segment, session, and authentication fallback behavior to supported routes and request shapes.
Validation
./scripts/dev-build.sh --tests --jobs 4QT_QPA_PLATFORM=offscreen ctest --test-dir build-dev --output-on-failure -j4— 36/36 passedBUILD_TESTING=OFFbuild — 272/272 targets, including QML cache generationnix flake check --print-build-logsnix build --print-build-logs./scripts/run-clang-tidy.shgit diff --checkAn authenticated Jellyfin 12 server with representative media was not available, so end-to-end authenticated library/playback behavior remains a live-provider validation item.
Documentation
Updated build, image-cache, canonical model/viewmodel, playback, media-segment, and provider-compatibility documentation.
Note
Fix SWR metadata refresh, rounded images, and update Jellyfin API routes to v12
LibraryViewModelnow compares full canonical item objects instead of only IDs, emitting fine-graineddataChangedsignals per row for metadata-only updates without a full model reset.RoundedImagegains a CPU-painted software fallback (SoftwareRoundedImage) that snapshots the already-loaded base image viaQPainterwith a rounded clip path when shaders are unavailable, avoiding duplicate network loads. TheImageCacheProviderrounded-image path is reworked with revision-aware lookups, async LRU touches, and retry semantics./Itemsreplaces user-scoped paths,POST /Items/{itemId}/PlaybackInfosendsUserIdin the JSON body,DELETE /DevicesreplacesPOST /Sessions/{id}/Logoutfor revocation,ApiKeyreplacesapi_keyin URLs, andEventNameis dropped from progress reports./MediaSegments/{id}endpoint viaJellyfinModelMapper::mediaSegments.Macroscope summarized 109e34d.
Summary by CodeRabbit