Conversation
With Emby Server 4.9, the plabyback session id must be passed so that it can be used during the update progress. This is a UUID for the playback session is generated when the session playback is started. Multiple items could be playing at the same time for a user, so a unique id is required.
WalkthroughAdds play-session lifecycle support: API models and service signatures now accept/return playSessionId; Emby client returns a playSessionId (or UUID fallback) when starting playback; repositories, presenter, jobs, tests, and app init were updated to create, propagate, and use playSessionId for progress updates. Changes
Sequence DiagramsequenceDiagram
actor User
participant ExoPresenter as ExoplayerPresenter
participant Repo as PlaybackRepository
participant Client as SerenityClient
participant EmbyClient as EmbyAPIClient
participant Emby as Emby Server
User->>ExoPresenter: playVideo(videoId)
ExoPresenter->>ExoPresenter: if playSessionId == null
alt no session
ExoPresenter->>Repo: startPlaying(videoId)
Repo->>Client: startPlaying(videoId)
Client->>EmbyClient: startPlaying(itemId, PlaybackStartRequest)
EmbyClient->>Emby: POST /Playback/Start
Emby-->>EmbyClient: PlaybackStartInfo { playSessionId }
EmbyClient-->>Client: playSessionId or generated UUID
Client-->>Repo: playSessionId
Repo-->>ExoPresenter: playSessionId
ExoPresenter->>ExoPresenter: store playSessionId
end
ExoPresenter->>ExoPresenter: initialize player
loop periodic updates
ExoPresenter->>Repo: updatePlaybackPosition(video, playSessionId)
Repo->>Client: progress(key, offset, playSessionId)
Client->>EmbyClient: progress(key, offset, playSessionId)
EmbyClient->>Emby: POST progress (includes PlaySessionId)
Emby-->>EmbyClient: success
EmbyClient-->>Client: Boolean
end
User->>ExoPresenter: stopPlaying()
ExoPresenter->>ExoPresenter: playSessionId = null
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (3)**/*.{java,kt}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.kt📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*Test.kt📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (7)📚 Learning: 2025-12-16T18:16:58.793ZApplied to files:
📚 Learning: 2025-12-16T18:16:58.793ZApplied to files:
📚 Learning: 2025-12-16T18:16:58.793ZApplied to files:
📚 Learning: 2025-12-16T18:16:58.793ZApplied to files:
📚 Learning: 2025-12-16T18:16:58.793ZApplied to files:
📚 Learning: 2025-12-16T18:16:58.793ZApplied to files:
📚 Learning: 2025-12-16T18:16:58.793ZApplied to files:
🧬 Code graph analysis (1)serenity-app/src/test/kotlin/us/nineworlds/serenity/core/services/UpdateProgressRequestJobTest.kt (5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (6)
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: 7
🧹 Nitpick comments (8)
emby-lib/src/main/kotlin/us/nineworlds/serenity/emby/server/model/itemsmodel.kt (1)
116-118: LGTM! Sensible default for playback requests.The
PlaybackStartRequestdata class with a defaultcanSeek = trueis a clean design that simplifies API calls while allowing override when needed.Optional: If you want to ensure the default value aligns with the Emby API specification, you can verify the expected behavior:
Emby API PlaybackStartRequest canSeek default valueserenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerPresenterTest.kt (1)
79-82: AddToothpick.reset()to tearDown for proper test cleanup.Per coding guidelines, test cleanup should include both
clearAllMocks()andToothpick.reset(). TheclearAllMocks()is currently insetUp, buttearDownis missingToothpick.reset().🔎 Proposed fix
@After fun tearDown() { Dispatchers.resetMain() + clearAllMocks() + Toothpick.reset() }And remove
clearAllMocks()fromsetUpor keep it in both places for safety.Based on coding guidelines: "Define setup and cleanup routines explicitly in unit tests, ensuring all mocks are cleared in
@Afterblocks usingclearAllMocks()andToothpick.reset()".serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerPresenter.kt (1)
97-102: Consider passingplaySessionIdtostopPlayingbefore clearing it.The
playSessionIdis cleared after thestopPlayingcall, but the repository'sstopPlayingmethod (and the underlying API) accepts aplaySessionIdparameter. Consider passing the session ID to ensure proper server-side session cleanup.🔎 Proposed fix
override fun stopPlaying(currentPosition: Long) { + val sessionId = playSessionId presenterScope.launch { - playbackRepository.stopPlaying(video.id().orEmpty(), currentPosition) + playbackRepository.stopPlaying(video.id().orEmpty(), currentPosition, sessionId) } playSessionId = null }This would require updating
PlaybackRepository.stopPlayingto accept and forward the session ID.serenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/PlaybackRepositoryTest.kt (2)
25-27: Consider usingrelaxed = truefor mock initialization.Per coding guidelines, mocks should be initialized with
mockk(relaxed = true). This reduces the need for expliciteverystubs for methods that aren't central to the test.🔎 Proposed fix
private companion object { - private val mockSerenityClient = mockk<SerenityClient>() + private val mockSerenityClient = mockk<SerenityClient>(relaxed = true) }Based on learnings: "Define mocks as properties, initialized with
mockk(relaxed = true)".
38-41: AddclearAllMocks()to tearDown.Per coding guidelines, mocks should be cleared in
@Afterblocks.🔎 Proposed fix
@After fun tearDown() { Dispatchers.resetMain() + clearAllMocks() }Based on learnings: "Define setup and cleanup routines explicitly in unit tests, ensuring all mocks are cleared in
@Afterblocks usingclearAllMocks()".serenity-app/src/main/kotlin/us/nineworlds/serenity/core/services/UpdateProgressRequestJob.kt (2)
58-85: Significant code duplication with existingupdateProgressmethod.The new overload duplicates nearly all logic from the original
updateProgress. Consider having the original method delegate to this new one with a defaultnullplaySessionId.🔎 Proposed fix
fun updateProgress(position: Long, video: VideoContentInfo) { + updateProgress(position, video, null) + } + + fun updateProgress(position: Long, video: VideoContentInfo, playSessionId: String?) { if (scope == null) { job = SupervisorJob() scope = CoroutineScope(Dispatchers.IO + job!!) } scope?.launch { val id = video.id() if (id == null) { Timber.w("Video ID is null. Cannot update progress for video: ${video.id()}") return@launch } try { if (video.isWatched) { serenityClient.watched(id) - serenityClient.progress(id, "0") + serenityClient.progress(id, "0", playSessionId) } else { - serenityClient.progress(id, position.toString()) + serenityClient.progress(id, position.toString(), playSessionId) } } catch (e: Exception) { if (e is CancellationException) { return@launch } Timber.e(e, "Error updating progress") } } } - - fun updateProgress(position: Long, video: VideoContentInfo, playSessionId: String?) { - // ... remove duplicate implementation - }
29-33: Minor: Potential race condition on scope initialization.The
scope == nullcheck and assignment isn't thread-safe. If multiple threads callupdateProgressconcurrently before scope is initialized, multipleCoroutineScopeinstances could be created. Consider usingsynchronizedor lazy initialization.🔎 Proposed fix using synchronized
+ @Synchronized fun updateProgress(position: Long, video: VideoContentInfo) { if (scope == null) { job = SupervisorJob() scope = CoroutineScope(Dispatchers.IO + job!!) } + }.also { /* scope now initialized */ } - scope?.launch { + scope!!.launch {Or use a lazy delegate for initialization.
emby-lib/src/main/kotlin/us/nineworlds/serenity/emby/server/api/EmbyAPIClient.kt (1)
498-506: Consider addingplaySessionIdparameter tostopPlaying.The
UsersService.stopPlayingAPI accepts an optionalplaySessionId, but this implementation doesn't forward it. For consistent session management, consider updating to pass the session ID.🔎 Proposed fix
-override fun stopPlaying(itemId: String, offset: Long) { +override fun stopPlaying(itemId: String, offset: Long, playSessionId: String? = null) { if (userId == null) { userId = fetchUserId() } val positionTicks = offset.times(10000) - val call = usersService.stopPlaying(headerMap(), userId!!, itemId, null, positionTicks.toString()) + val call = usersService.stopPlaying(headerMap(), userId!!, itemId, null, positionTicks.toString(), playSessionId) call.execute() }This would also require updating the
SerenityClientinterface.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
emby-lib/src/main/kotlin/us/nineworlds/serenity/emby/server/api/EmbyAPIClient.ktemby-lib/src/main/kotlin/us/nineworlds/serenity/emby/server/api/UsersService.ktemby-lib/src/main/kotlin/us/nineworlds/serenity/emby/server/model/itemsmodel.ktgradle/libs.versions.tomlserenity-app/build.gradle.ktsserenity-app/src/main/java/us/nineworlds/serenity/SerenityApplication.ktserenity-app/src/main/kotlin/us/nineworlds/serenity/core/services/UpdateProgressRequestJob.ktserenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerPresenter.ktserenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/PlaybackRepository.ktserenity-app/src/test/kotlin/us/nineworlds/serenity/core/services/UpdateProgressRequestJobTest.ktserenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerPresenterTest.ktserenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/PlaybackRepositoryTest.ktserenity-common/src/main/java/us/nineworlds/serenity/common/rest/SerenityClient.kt
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{java,kt}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{java,kt}: Use Kotlin as the primary language and convert Java code to Kotlin where feasible
Enforce proper formatting using./gradlew spotlessApply
Use Toothpick for dependency injection
Files:
serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/PlaybackRepository.ktserenity-app/src/main/java/us/nineworlds/serenity/SerenityApplication.ktemby-lib/src/main/kotlin/us/nineworlds/serenity/emby/server/api/EmbyAPIClient.ktserenity-common/src/main/java/us/nineworlds/serenity/common/rest/SerenityClient.ktserenity-app/src/test/kotlin/us/nineworlds/serenity/core/services/UpdateProgressRequestJobTest.ktserenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerPresenterTest.ktserenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/PlaybackRepositoryTest.ktserenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerPresenter.ktemby-lib/src/main/kotlin/us/nineworlds/serenity/emby/server/api/UsersService.ktserenity-app/src/main/kotlin/us/nineworlds/serenity/core/services/UpdateProgressRequestJob.ktemby-lib/src/main/kotlin/us/nineworlds/serenity/emby/server/model/itemsmodel.kt
**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
Follow Kotlin naming conventions and style guidelines
Files:
serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/PlaybackRepository.ktserenity-app/src/main/java/us/nineworlds/serenity/SerenityApplication.ktemby-lib/src/main/kotlin/us/nineworlds/serenity/emby/server/api/EmbyAPIClient.ktserenity-common/src/main/java/us/nineworlds/serenity/common/rest/SerenityClient.ktserenity-app/src/test/kotlin/us/nineworlds/serenity/core/services/UpdateProgressRequestJobTest.ktserenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerPresenterTest.ktserenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/PlaybackRepositoryTest.ktserenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerPresenter.ktemby-lib/src/main/kotlin/us/nineworlds/serenity/emby/server/api/UsersService.ktserenity-app/src/main/kotlin/us/nineworlds/serenity/core/services/UpdateProgressRequestJob.ktemby-lib/src/main/kotlin/us/nineworlds/serenity/emby/server/model/itemsmodel.kt
**/*Test.kt
📄 CodeRabbit inference engine (AGENTS.md)
**/*Test.kt: All mocking operations must use MockK and no other mocking libraries are permitted
DO NOT USE MockK annotations; prefer field level creation of mocks directly
Write unit tests for all new features, bug fixes, or critical code changes
Define mocks as properties, initialized withmockk(relaxed = true), and useeveryfor expectations andverifyfor assertions
Define setup and cleanup routines explicitly in unit tests, ensuring all mocks are cleared in@Afterblocks usingclearAllMocks()andToothpick.reset()
Use@BeforeClassand@AfterClassfor static mocks in unit tests
Extendus.nineworlds.serenity.test.InjectingTestfor Toothpick-based tests and overridesetUpwith a call tosuper.setUp()as the first action
Bind mocks to instances ininstallTestModulesusing the Module pattern:bind(MainPresenter::class.java).toInstance(mockPresenter)
Files:
serenity-app/src/test/kotlin/us/nineworlds/serenity/core/services/UpdateProgressRequestJobTest.ktserenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerPresenterTest.ktserenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/PlaybackRepositoryTest.kt
🧠 Learnings (8)
📚 Learning: 2025-12-16T18:16:58.783Z
Learnt from: CR
Repo: NineWorlds/serenity-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-16T18:16:58.783Z
Learning: Applies to **/*Activity.kt : Inject Providers using `Inject lateinit var presenterProvider: Provider<MainPresenter>` and use moxyPresenter delegate: `internal val presenter by moxyPresenter { presenterProvider.get() }`
Applied to files:
serenity-app/build.gradle.kts
📚 Learning: 2025-12-16T18:16:58.783Z
Learnt from: CR
Repo: NineWorlds/serenity-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-16T18:16:58.783Z
Learning: Applies to **/*Activity.kt : Use the moxyPresenter delegate pattern for presenter injection in Activities
Applied to files:
serenity-app/build.gradle.kts
📚 Learning: 2025-12-16T18:16:58.783Z
Learnt from: CR
Repo: NineWorlds/serenity-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-16T18:16:58.783Z
Learning: Applies to **/*Test.kt : Define setup and cleanup routines explicitly in unit tests, ensuring all mocks are cleared in `After` blocks using `clearAllMocks()` and `Toothpick.reset()`
Applied to files:
serenity-app/src/test/kotlin/us/nineworlds/serenity/core/services/UpdateProgressRequestJobTest.ktserenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/PlaybackRepositoryTest.kt
📚 Learning: 2025-12-16T18:16:58.783Z
Learnt from: CR
Repo: NineWorlds/serenity-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-16T18:16:58.783Z
Learning: Applies to **/*Test.kt : Write unit tests for all new features, bug fixes, or critical code changes
Applied to files:
serenity-app/src/test/kotlin/us/nineworlds/serenity/core/services/UpdateProgressRequestJobTest.kt
📚 Learning: 2025-12-16T18:16:58.783Z
Learnt from: CR
Repo: NineWorlds/serenity-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-16T18:16:58.783Z
Learning: Applies to **/*Test.kt : DO NOT USE MockK annotations; prefer field level creation of mocks directly
Applied to files:
serenity-app/src/test/kotlin/us/nineworlds/serenity/core/services/UpdateProgressRequestJobTest.ktserenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/PlaybackRepositoryTest.kt
📚 Learning: 2025-12-16T18:16:58.783Z
Learnt from: CR
Repo: NineWorlds/serenity-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-16T18:16:58.783Z
Learning: Applies to **/*Test.kt : Use `BeforeClass` and `AfterClass` for static mocks in unit tests
Applied to files:
serenity-app/src/test/kotlin/us/nineworlds/serenity/core/services/UpdateProgressRequestJobTest.kt
📚 Learning: 2025-12-16T18:16:58.783Z
Learnt from: CR
Repo: NineWorlds/serenity-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-16T18:16:58.783Z
Learning: Applies to **/*Test.kt : Extend `us.nineworlds.serenity.test.InjectingTest` for Toothpick-based tests and override `setUp` with a call to `super.setUp()` as the first action
Applied to files:
serenity-app/src/test/kotlin/us/nineworlds/serenity/core/services/UpdateProgressRequestJobTest.ktserenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/PlaybackRepositoryTest.kt
📚 Learning: 2025-12-16T18:16:58.783Z
Learnt from: CR
Repo: NineWorlds/serenity-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-16T18:16:58.783Z
Learning: Applies to **/*Test.kt : Define mocks as properties, initialized with `mockk(relaxed = true)`, and use `every` for expectations and `verify` for assertions
Applied to files:
serenity-app/src/test/kotlin/us/nineworlds/serenity/core/services/UpdateProgressRequestJobTest.ktserenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerPresenterTest.ktserenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/PlaybackRepositoryTest.kt
🧬 Code graph analysis (2)
serenity-app/src/test/kotlin/us/nineworlds/serenity/core/services/UpdateProgressRequestJobTest.kt (5)
serenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerPresenterTest.kt (1)
bind(185-193)serenity-app/src/test/kotlin/us/nineworlds/serenity/core/services/WatchedVideoJobTest.kt (1)
bind(46-50)serenity-app/src/test/kotlin/us/nineworlds/serenity/core/services/UnWatchVideoJobTest.kt (1)
bind(46-50)serenity-app/src/test/kotlin/us/nineworlds/serenity/core/model/impl/AbstractSeriesContentInfoTest.kt (1)
bind(223-227)serenity-app/src/test/kotlin/us/nineworlds/serenity/core/model/impl/AbstractVideoContentInfoTest.kt (1)
bind(168-174)
serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerPresenter.kt (5)
emby-lib/src/main/kotlin/us/nineworlds/serenity/emby/server/api/EmbyAPIClient.kt (1)
startPlaying(484-496)emby-lib/src/main/kotlin/us/nineworlds/serenity/emby/server/api/UsersService.kt (1)
startPlaying(121-128)serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/PlaybackRepository.kt (1)
startPlaying(13-17)serenity-common/src/main/java/us/nineworlds/serenity/common/rest/SerenityClient.kt (1)
startPlaying(85-85)serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerContract.kt (1)
startPlaying(54-54)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (18)
emby-lib/src/main/kotlin/us/nineworlds/serenity/emby/server/model/itemsmodel.kt (2)
106-109: LGTM! Clean API model definition.The
PlaybackStartInfodata class is well-structured with appropriate nullable fields for capturing playback session information from Emby API responses. The Moshi annotations are correctly applied.
111-114: LGTM! Appropriate nested model.The
QueueItemdata class correctly models the queue item structure with sensible type choices foridandplaylistItemId.serenity-app/src/main/java/us/nineworlds/serenity/SerenityApplication.kt (1)
32-32: Imports are correctly added and used.All three imports (
ProcessLifecycleOwner,lifecycleScope,Dispatchers) are directly referenced in thediscoverServers()method and introduce no unnecessary dependencies.Also applies to: 33-33, 39-39
gradle/libs.versions.toml (1)
26-26: minSdkVersion change to 25 is safe.Verification found no API level 27+ features, NotificationChannel usage (API 26+), or @RequiresApi(27+) annotations in the codebase. The lowered minSdkVersion to 25 (Android 7.1 Nougat) is compatible with the current code.
emby-lib/src/main/kotlin/us/nineworlds/serenity/emby/server/api/UsersService.kt (1)
111-128: LGTM! API changes properly support session tracking.The addition of optional
playSessionIdparameters toprogress()andstopPlaying(), along with the updatedstartPlaying()signature that accepts aPlaybackStartRequestbody and returnsPlaybackStartInfo, correctly enables the playback session tracking feature.serenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerPresenterTest.kt (1)
140-150: LGTM! Test verification updated correctly.The
coVerifynow expects two arguments matching the updatedupdatePlaybackPosition(video, playSessionId)signature.serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerPresenter.kt (2)
104-111: Guard against duplicate sessions is correct, but consider async timing.The guard
if (playSessionId == null)correctly prevents creating multiple sessions. However, sincestartPlaying()launches a coroutine, theplaySessionIdmay not be set by the timeupdateServerPlaybackPositionis called, causing early progress updates to be sent without a session ID.If the server requires a valid
playSessionIdfor progress updates, consider awaiting the session creation before proceeding with playback, or ensure the API gracefully handles null session IDs during the brief initialization window.
158-162: LGTM! Playback initialization order is correct.Calling
startPlaying()beforeinitializePlayer()ensures the session creation is initiated prior to playback. The session ID will be available for subsequent progress updates once the async call completes.serenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/PlaybackRepositoryTest.kt (1)
63-89: LGTM! Test verifications correctly updated for new progress signature.The tests properly mock and verify the three-argument
progress(key, offset, playSessionId)calls, usingany()matcher for the session ID parameter.serenity-common/src/main/java/us/nineworlds/serenity/common/rest/SerenityClient.kt (1)
64-68: LGTM! Progress overload with optionalplaySessionIdmaintains backward compatibility.The new overload with a default
nullparameter allows existing callers to continue working while enabling session-aware progress updates.serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/PlaybackRepository.kt (2)
13-17: LGTM! Session ID captured and logged correctly.The
startPlayingmethod properly captures the session ID from the client and logs it for debugging purposes.
23-30: LGTM! Progress updates now include session ID.The
updatePlaybackPositioncorrectly forwards theplaySessionIdto both progress call paths (watched and unwatched).emby-lib/src/main/kotlin/us/nineworlds/serenity/emby/server/api/EmbyAPIClient.kt (2)
416-432: LGTM! Progress overload delegation is clean.The original
progressmethod correctly delegates to the new overload withnullsession ID, avoiding code duplication.
484-496: LGTM!startPlayingcorrectly returns session ID with UUID fallback.The implementation properly returns the server-provided
playSessionIdwhen available, falling back to a generated UUID when the response body is null or the call fails. This ensures session continuity even during API issues.serenity-app/src/test/kotlin/us/nineworlds/serenity/core/services/UpdateProgressRequestJobTest.kt (4)
29-30: LGTM!Mock definitions follow the coding guidelines correctly: field-level creation using
mockk(relaxed = true)without annotations.
32-37: LGTM!Setup method follows coding guidelines correctly: calls
super.setUp()first, sets up test dispatcher, and performs Toothpick injection.
44-52: LGTM!Test module setup follows coding guidelines and uses the proper Module pattern to bind mocks to instances.
54-117: Excellent test coverage!The test suite comprehensively covers:
- Both original and new API signatures (with/without
playSessionId)- Watched and unwatched video scenarios
- Exception handling
- Null ID edge cases
The tests follow proper MockK patterns with
coEveryandcoVerify, and use clear naming conventions.
| import java.io.File | ||
| import java.io.IOException | ||
| import java.util.UUID | ||
| import kotlin.uuid.Uuid |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused import kotlin.uuid.Uuid.
This import is not used in the file. The code uses java.util.UUID throughout.
🔎 Proposed fix
-import kotlin.uuid.Uuid📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import kotlin.uuid.Uuid |
🤖 Prompt for AI Agents
In
emby-lib/src/main/kotlin/us/nineworlds/serenity/emby/server/api/EmbyAPIClient.kt
around line 34, remove the unused import "import kotlin.uuid.Uuid" because the
file consistently uses java.util.UUID; simply delete that import line to
eliminate the unused import warning and rely on the existing java.util.UUID
usages.
| val okClient = RetrofitUrlManager.getInstance().with(OkHttpClient.Builder()) | ||
| logger.level = HttpLoggingInterceptor.Level.BASIC | ||
| logger.level = HttpLoggingInterceptor.Level.BODY | ||
| okClient.addInterceptor(logger) |
There was a problem hiding this comment.
Logging level BODY may expose sensitive data.
Changing from BASIC to BODY logging will log full request and response bodies, which could include sensitive information (tokens, user data). Consider keeping this at BASIC or HEADERS for production builds, or gating behind a debug flag.
🔎 Proposed fix
-logger.level = HttpLoggingInterceptor.Level.BODY
+logger.level = if (BuildConfig.DEBUG) {
+ HttpLoggingInterceptor.Level.BODY
+} else {
+ HttpLoggingInterceptor.Level.BASIC
+}🤖 Prompt for AI Agents
In
emby-lib/src/main/kotlin/us/nineworlds/serenity/emby/server/api/EmbyAPIClient.kt
around lines 57-59, the logging level is set to BODY which can expose sensitive
request/response payloads; change this to a safer default
(HttpLoggingInterceptor.Level.BASIC or HEADERS) and/or gate BODY behind a debug
flag (e.g., BuildConfig.DEBUG) so full bodies are logged only in debug builds;
update the code to set logger.level conditionally and keep the production
default to BASIC/HEADERS.
| kotlin = "2.2.21" | ||
| coreKtxVersion = "1.17.0" | ||
| espressoCoreVersion = "3.5.1" | ||
| lifecycleProcessVersion = "2.10.0" |
There was a problem hiding this comment.
lifecycle-process version 2.10.0 does not exist—use an available version instead.
The latest stable release of lifecycle-process is 2.8.6, with 2.9.0-alpha03 as the latest pre-release. Version 2.10.0 is unavailable for this artifact and will cause a dependency resolution failure during the build. Change lifecycleProcessVersion to 2.8.6 (stable) or 2.9.0-alpha03 if pre-release versions are acceptable.
Also applies to: 144-144
🤖 Prompt for AI Agents
In gradle/libs.versions.toml at line 64 (and also at line 144), the
lifecycleProcessVersion value "2.10.0" is invalid and will break dependency
resolution; change the value to a valid release such as "2.8.6" (stable) or
"2.9.0-alpha03" (pre-release) and ensure any other occurrences (line 144) are
updated consistently.
| implementation(libs.androidx.lifecycle.process) | ||
| ksp(libs.glide.compiler) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Fix incorrect indentation.
Lines 148-149 have 4 extra spaces of indentation compared to the rest of the dependencies block, making them inconsistent with the file's formatting.
🔎 Proposed fix
- implementation(libs.androidx.lifecycle.process)
- ksp(libs.glide.compiler)
+ implementation(libs.androidx.lifecycle.process)
+ ksp(libs.glide.compiler)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| implementation(libs.androidx.lifecycle.process) | |
| ksp(libs.glide.compiler) | |
| implementation(libs.androidx.lifecycle.process) | |
| ksp(libs.glide.compiler) |
🤖 Prompt for AI Agents
In serenity-app/build.gradle.kts around lines 148-149, the two dependency lines
("implementation(libs.androidx.lifecycle.process)" and
"ksp(libs.glide.compiler)") have 4 extra leading spaces making them misaligned
with the rest of the dependencies block; fix by removing the 4 extra spaces so
both lines use the same indentation level as the surrounding dependency entries.
| protected open fun discoverServers() { | ||
| MainScope().launch { | ||
| ProcessLifecycleOwner.get().lifecycleScope.launch(Dispatchers.IO) { | ||
| EmbyServerDiscover().findServers() | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for build.gradle files containing lifecycle-process dependency
find . -type f \( -name "build.gradle" -o -name "build.gradle.kts" \) | head -20Repository: NineWorlds/serenity-android
Length of output: 348
🏁 Script executed:
#!/bin/bash
# Search for lifecycle-process in all build.gradle files
echo "=== Searching for lifecycle-process ==="
grep -r "lifecycle-process" . --include="*.gradle" --include="*.gradle.kts" 2>/dev/null || echo "Not found in build files"
echo -e "\n=== Searching for androidx.lifecycle in serenity-app/build.gradle.kts ==="
grep -E "androidx\.lifecycle|lifecycle" serenity-app/build.gradle.kts 2>/dev/null || echo "Not found"
echo -e "\n=== Full serenity-app/build.gradle.kts dependencies section ==="
sed -n '/^dependencies/,/^}/p' serenity-app/build.gradle.kts 2>/dev/null | head -100Repository: NineWorlds/serenity-android
Length of output: 3515
🏁 Script executed:
#!/bin/bash
# Read the SerenityApplication.kt file, especially around lines 145-149
echo "=== SerenityApplication.kt (lines 140-160) ==="
sed -n '140,160p' serenity-app/src/main/java/us/nineworlds/serenity/SerenityApplication.kt
echo -e "\n=== Import statements in SerenityApplication.kt ==="
head -30 serenity-app/src/main/java/us/nineworlds/serenity/SerenityApplication.kt
echo -e "\n=== Check for Toothpick usage in SerenityApplication.kt ==="
grep -E "Toothpick|inject|Inject" serenity-app/src/main/java/us/nineworlds/serenity/SerenityApplication.kt || echo "No Toothpick usage found"Repository: NineWorlds/serenity-android
Length of output: 2791
Lifecycle-aware coroutine management is solid, but dependency injection pattern needs adjustment.
The lifecycle-process dependency is properly declared, and using ProcessLifecycleOwner.get().lifecycleScope.launch(Dispatchers.IO) correctly ties the server discovery coroutine to the app process lifecycle while offloading IO work. However, EmbyServerDiscover() should be injected via Toothpick rather than instantiated directly, following the project's DI guidelines used elsewhere in SerenityApplication.kt.
🤖 Prompt for AI Agents
In serenity-app/src/main/java/us/nineworlds/serenity/SerenityApplication.kt
around lines 145-149, replace the direct instantiation EmbyServerDiscover() with
a Toothpick-injected instance: obtain the EmbyServerDiscover from the
application (or appropriate) Toothpick scope and call its findServers() inside
the same lifecycleScope coroutine; ensure the EmbyServerDiscover binding is
registered in the Toothpick module and that Toothpick is initialized/imported in
this file.
...ity-app/src/test/kotlin/us/nineworlds/serenity/core/services/UpdateProgressRequestJobTest.kt
Show resolved
Hide resolved
|
|
||
| fun startPlaying(key: String): String? | ||
| fun stopPlaying(key: String, offset: Long) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find SerenityClient interface
find . -type f -name "*.kt" -o -name "*.java" | xargs grep -l "interface SerenityClient" | head -20Repository: NineWorlds/serenity-android
Length of output: 155
🏁 Script executed:
#!/bin/bash
# Search for SerenityClient interface and all classes implementing it
echo "=== Finding SerenityClient interface ==="
find . -type f \( -name "*.kt" -o -name "*.java" \) -exec grep -l "SerenityClient" {} \; | head -30
echo -e "\n=== Searching for startPlaying method ==="
find . -type f \( -name "*.kt" -o -name "*.java" \) -exec grep -n "fun startPlaying\|fun stopPlaying" {} + | head -40Repository: NineWorlds/serenity-android
Length of output: 5725
🏁 Script executed:
#!/bin/bash
# More targeted search for interface definition and implementations
echo "=== Interface Definition ==="
rg -n "interface SerenityClient|class.*:.*SerenityClient|class.*implements.*SerenityClient" --type=kt --type=java 2>/dev/null | head -50
echo -e "\n=== startPlaying method signatures ==="
rg -n "fun startPlaying\|def startPlaying" --type=kt --type=java -A1 2>/dev/null | head -80Repository: NineWorlds/serenity-android
Length of output: 139
🏁 Script executed:
#!/bin/bash
# Get full signature of JellyfinAPIClient.startPlaying
echo "=== JellyfinAPIClient.startPlaying ==="
sed -n '475,495p' ./jellyfin-lib/src/main/kotlin/us/nineworlds/serenity/jellyfin/server/api/JellyfinAPIClient.kt
echo -e "\n=== EmbyAPIClient.startPlaying ==="
sed -n '480,500p' ./emby-lib/src/main/kotlin/us/nineworlds/serenity/emby/server/api/EmbyAPIClient.ktRepository: NineWorlds/serenity-android
Length of output: 1558
🏁 Script executed:
#!/bin/bash
# Check if there are any other SerenityClient implementations
echo "=== All classes implementing SerenityClient ==="
rg "class.*:.*SerenityClient|implements SerenityClient" --type=kt --type=java -n
echo -e "\n=== Check interfaces/abstract classes ==="
rg "interface.*SerenityClient" --type=kt --type=java -nRepository: NineWorlds/serenity-android
Length of output: 232
🏁 Script executed:
#!/bin/bash
# Get the SerenityClient interface definition
echo "=== SerenityClient Interface Definition ==="
cat ./serenity-common/src/main/java/us/nineworlds/serenity/common/rest/SerenityClient.ktRepository: NineWorlds/serenity-android
Length of output: 3501
🏁 Script executed:
#!/bin/bash
# Search for all classes that extend or implement SerenityClient
echo "=== Searching for SerenityClient implementations with better patterns ==="
rg ":\s*SerenityClient|implements\s+SerenityClient" -n --type kotlin --type javaRepository: NineWorlds/serenity-android
Length of output: 3951
Not all SerenityClient implementations have been updated.
EmbyAPIClient correctly implements startPlaying(itemId: String): String?, but JellyfinAPIClient at line 478 is missing the String? return type in its startPlaying method signature.
🤖 Prompt for AI Agents
In
serenity-common/src/main/java/us/nineworlds/serenity/common/rest/SerenityClient.kt
around lines 84-86, the interface declares fun startPlaying(key: String):
String? but not all implementations match; update JellyfinAPIClient at/around
line 478 to change its startPlaying method signature to return String? (nullable
String) to match the interface, and ensure the method returns an appropriate
String? value (or null) where it currently returns nothing/Unit.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #501 +/- ##
=============================================
- Coverage 50.20% 50.05% -0.16%
- Complexity 466 467 +1
=============================================
Files 63 63
Lines 1974 1996 +22
Branches 268 273 +5
=============================================
+ Hits 991 999 +8
- Misses 867 878 +11
- Partials 116 119 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ces/UpdateProgressRequestJobTest.kt Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
1 participant
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.