Implements Paging Support for Issue #374#516
Conversation
…paging support Implements the paging data sources that are needed for the Leanback Paging library support. Also updates the Emby Client to work with paging. Includes a plan for ais to help generate the rest of the items.
Process the lists in multiple chunks.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds end-to-end paging: paginated client overloads and Retrofit query params, PagingSource implementations, a Leanback PagingData adapter bridge, repository and presenter plumbing producing PagingData flows, UI/adapter changes to consume PagingData, concurrent model builders, tests, and dependency additions. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as DetailsFragment
participant Presenter as DetailsMVPPresenter
participant Pager as Pager
participant PS as SimilarItemsPagingSource
participant Repo as VideoRepository
participant Client as SerenityClient
participant Adapter as SerenityPagingDataAdapter
User->>UI: open details / scroll similar
UI->>Presenter: loadSimilarItems(itemId, type)
Presenter->>Pager: create Pager(...) -> flow
Presenter->>Adapter: create SerenityPagingDataAdapter
Presenter->>Adapter: submitData(flow.cachedIn(scope))
loop pages loaded
Pager->>PS: load(position, loadSize)
PS->>Repo: fetchSimilarItemsList(itemId, type, startIndex, limit)
Repo->>Client: fetchSimilarItemById(itemId,type,startIndex,limit)
Client-->>Repo: IMediaContainer
Repo-->>PS: List<VideoContentInfo>
PS-->>Pager: LoadResult.Page(data, prevKey, nextKey)
Pager->>Adapter: notify updates
Adapter->>UI: DataObserver notifies Leanback
end
sequenceDiagram
participant User as User
participant Presenter as MainMenuPresenter
participant Pager as Pager
participant PS as VideoCategoryPagingSource
participant Repo as CategoryRepository
participant Client as SerenityClient
participant View as MainMenuFragment
participant Adapter as SerenityPagingDataAdapter
User->>Presenter: navigate to main menu
Presenter->>Presenter: load categories (initial)
loop per category
Presenter->>Pager: Pager(...) -> VideoCategoryPagingSource
Presenter->>View: updateCategories(category, pagingData)
View->>Adapter: create per-category SerenityPagingDataAdapter
View->>Adapter: submitData(pagingData)
end
loop user scrolls
Pager->>PS: load(position, loadSize)
PS->>Repo: fetchItemsByCategory(categoryId,itemId,type,startIndex,limit)
Repo->>Client: retrieveItemByIdCategory(...)
Client-->>Repo: IMediaContainer
Repo-->>PS: List<VideoContentInfo>
PS-->>Pager: LoadResult.Page(...)
Adapter->>View: DataObserver triggers UI update
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
emby-lib/src/test/kotlin/us/nineworlds/serenity/emby/server/api/EmbyAPIClientTest.kt (1)
56-66:⚠️ Potential issue | 🟡 MinorInconsistent base URL:
setUpuses192.168.68.97:8096butloginUserWithPasswordusesyourserver:yourport.Line 34 sets the URL to a specific IP, but line 58 overrides it with the placeholder
"http://yourserver:yourport". This test will always fail since neither URL is likely to be valid in most environments. If the intent is to skip this in CI, consider adding@Ignorewith a comment.emby-lib/src/main/kotlin/us/nineworlds/serenity/emby/server/api/EmbyAPIClient.kt (1)
241-263:⚠️ Potential issue | 🔴 Critical
startIndexparameter is accepted but never passed to the service call — pagination for similar items is broken.In
fetchSimilarItemById(lines 241-263), thestartIndexparameter is accepted but not forwarded to theusersService.fetchSimilarItemById()call. Additionally,UsersService.fetchSimilarItemById(defined in UsersService.kt) does not define a@Query("StartIndex")parameter at all.This breaks pagination:
SimilarItemsPagingSourcecallsrepository.fetchSimilarItemsList()with incrementing position values, but EmbyAPIClient ignores those positions when calling the API, causing every page request to return the same first page of results.Add
@Query("StartIndex") startIndex: Int = 0toUsersService.fetchSimilarItemByIdand pass it through in the EmbyAPIClient call.emby-lib/src/main/kotlin/us/nineworlds/serenity/emby/server/api/UsersService.kt (1)
40-58:⚠️ Potential issue | 🟠 MajorRemove
LimitCountquery parameter fromfetchItemQuery— not a valid Emby API parameter.Per Emby's official REST API reference, the Items endpoint (
GET /Users/{UserId}/Items) supports onlyLimitandStartIndexfor pagination.LimitCountis not a documented parameter and including it will send an invalid query parameter to the API. The newLimitparameter is correct; remove the pre-existingLimitCount(line 51) to avoid potential API conflicts or unexpected behavior.serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/activity/leanback/details/DetailsMVPPresenter.kt (1)
86-112:⚠️ Potential issue | 🟠 MajorAdd explicit
Dispatchers.Mainto presenterScope.launch call for thread safety.
presenterScopedoes not default to the Main dispatcher — the dispatcher is inherited from the coroutine context or defaults to the inherited one. At line 106,presenterScope.launch { collectLatest { viewState.updateSeasonEpisodes(...) } }will execute on whichever dispatcher is active, which may not be the Main thread. Since Moxy view state commands must run on the Main thread, explicitly specifyDispatchers.Main:presenterScope.launch(Dispatchers.Main) { pagingFlow.collectLatest { pagingData -> viewState.updateSeasonEpisodes(season, pagingData) } }(Note: the
updateSeriesfunction at line 89 correctly useswithContext(Dispatchers.Main)for the similar operation; apply the same pattern here.)serenity-app/src/main/java/us/nineworlds/serenity/fragments/MainMenuVideoContentVerticalGridFragment.kt (1)
108-122:⚠️ Potential issue | 🟠 MajorAdapter created but unreachable when
categoryDetailis null.On Line 113, if
category.categoryDetailisnull, theSerenityPagingDataAdapteris still created and added as aListRowtovideoContentAdapter(Line 120), but it is never stored inadapterMap. This meansupdateCategory(Line 101) can never find it and will silently skip submitting paging data, leaving an empty row in the UI.Either skip adding the
ListRowentirely whencategoryDetailis null, or ensurecategoryDetailis always non-null at this point.Proposed fix — skip categories with null detail
for (category in categories.categories) { + val detail = category.categoryDetail ?: continue val listContentRowAdapter = SerenityPagingDataAdapter(CategoryVideoPresenter(), videoCategoryDiffCallback) - category.categoryDetail?.let { detail -> - adapterMap[detail] = listContentRowAdapter - } + adapterMap[detail] = listContentRowAdapter - val header = HeaderItem(category.categoryDetail) + val header = HeaderItem(detail) val imageListRow = ListRow(header, listContentRowAdapter) videoContentAdapter.add(imageListRow) }jellyfin-lib/src/main/kotlin/us/nineworlds/serenity/jellyfin/server/api/JellyfinAPIClient.kt (2)
461-475:⚠️ Potential issue | 🟡 Minor
playSessionIdparameter is accepted but silently ignored.The
progressmethod at Line 463 acceptsplaySessionId: String?but never passes it tousersService.progress(Line 471). If this parameter is intentionally unsupported on Jellyfin, consider removing it from the signature or adding a comment explaining why it's ignored, to avoid confusing future maintainers.
240-262:⚠️ Potential issue | 🔴 Critical
startIndexparameter accepted but cannot be used — Jellyfin API doesn't support pagination for the Similar endpoint.The method accepts
startIndex: Int(Line 240) but never passes it tousersService.fetchSimilarItemById(Lines 252–258). This is because the Jellyfin API's/Items/{itemId}/Similarendpoint does not support aStartIndexquery parameter—it only supportslimit,userId,fields, andexcludeArtistIds.Consequently,
SimilarItemsPagingSourcewill always fetch results from the beginning of the list on every page, retrieving duplicate data rather than actual pagination.Recommended fix: Either remove
startIndexfrom this method's signature or mark it as a no-op with a comment explaining that Jellyfin API doesn't support offset-based pagination for similar items. If pagination is required, verify whether an alternative endpoint or approach exists.jellyfin-lib/src/main/kotlin/us/nineworlds/serenity/jellyfin/server/api/JellyfinUsersService.kt (2)
90-105:⚠️ Potential issue | 🔴 CriticalDuplicate
Limitparameter — hardcoded in@GETURL and also as@Query.Line 90 hardcodes
?Limit=20in the URL annotation, while Line 104 adds@Query("Limit") limit: Int? = null. When a non-nulllimitis passed (as it will be during paging), Retrofit appends&Limit=<value>to the URL, producing a request like:/Users/{userId}/Items?Limit=20&...&Limit=30The server behavior with duplicate query parameters is undefined and will likely cause incorrect page sizes.
Proposed fix — remove hardcoded Limit from the URL
- `@GET`("/Users/{userId}/Items?Limit=20") + `@GET`("/Users/{userId}/Items") fun latestItems( `@HeaderMap` headerMap: Map<String, String>, `@Path`("userId") userId: String, `@Query`("ParentId") parentId: String, `@Query`("Recursive") recursive: Boolean = true, `@Query`("SortBy") sortOptions: String = "DateCreated", `@Query`("SortOrder") sortOrder: String = "Descending", `@Query`("IsPlayed") isPlayed: Boolean = false, `@Query`("Filters") filters: String = "IsNotFolder,IsUnPlayed", `@Suppress`("ktlint:standard:max-line-length") `@Query`("Fields") fields: String = "Overview,MediaStreams,Studios,ParentId,Genres,MediaSources,UserData,OfficialRating,CommunityRating", `@Query`("IncludeItemTypes") includeItemType: String? = null, `@Query`("StartIndex") startIndex: Int = 0, - `@Query`("Limit") limit: Int? = null + `@Query`("Limit") limit: Int? = 20 ): Call<QueryResult>
147-156:⚠️ Potential issue | 🔴 CriticalMissing
StartIndexparameter onfetchSimilarItemById.This endpoint lacks a
@Query("StartIndex")parameter, so it cannot support offset-based pagination. As flagged in theJellyfinAPIClientreview, thestartIndexaccepted by the client method has no way to reach the server. If the Jellyfin Similar Items API supportsStartIndex, it should be added here.
🤖 Fix all issues with AI agents
In
`@emby-lib/src/test/kotlin/us/nineworlds/serenity/emby/server/api/EmbyAPIClientTest.kt`:
- Line 34: Replace the hardcoded LAN IP in the test by making the base URL
configurable and avoiding running against a developer-specific server: change
the call to client.updateBaseUrl(...) (in EmbyAPIClientTest) to read from an
environment variable (e.g., EMBY_BASE_URL) with a safe default like
"http://localhost:8096", and mark the integration test class or individual tests
with `@Ignore` (or a project test-category annotation) so they do not run in CI by
default.
In `@gradle/libs.versions.toml`:
- Line 23: The libs.versions.toml currently sets leanbackPagingVersion = "1.1.0"
but uses pagingVersion = "3.4.0", which is incompatible; update the
pagingVersion entries (all occurrences of the pagingVersion property in the
file) to use paging-runtime-ktx 3.3.6 (i.e., set pagingVersion = "3.3.6") so
leanback-paging 1.1.0 remains compatible, and ensure any dependency aliases that
reference pagingVersion are aligned to the new value.
In `@prompts/mvp_viewmodel_prompt.txt`:
- Around line 8-15: The instructions are inconsistent about base class (mentions
ViewModel on line 8 but requires AndroidViewModel(application) on line 14);
standardize on AndroidViewModel by replacing all references to 'ViewModel' with
'AndroidViewModel(application)' in the prompt and ensure the Presenter
instruction explicitly says "Extend AndroidViewModel(application) and implement
androidx.lifecycle.DefaultLifecycleObserver and Contract.Presenter"; also update
any example class signatures or steps that reference 'ViewModel' to use
'AndroidViewModel(application)' so all guidance is aligned.
- Around line 25-28: The current ViewModel factory uses an unsafe type-check
direction; update the factory for MyPresenter to either use the modern AndroidX
factory APIs (e.g., viewModelFactory { initializer { ... } } or
ViewModelProvider.Factory.from(...) ) to remove manual checks, or if
implementing ViewModelProvider.Factory manually, replace the unsafe
isAssignableFrom logic by checking the exact class equality (modelClass ==
MyPresenter::class.java) before returning presenterProvider.get() as T, or use
the correct direction MyPresenter::class.java.isAssignableFrom(modelClass);
update the factory object referenced by viewModels and ensure presenterProvider
and presenter are returned only when the type matches.
In
`@serenity-app/src/main/kotlin/us/nineworlds/serenity/core/model/impl/EpisodeMediaContainer.kt`:
- Around line 22-28: The code calls factory.baseURL() concurrently inside
createEpisodeContentInfo when videos are processed in parallel (chunked +
async), which can race if implementations like JellyfinAPIClient mutate
baseUrl/accessToken/userId; to fix, call factory.baseURL() once on the calling
thread and pass the resulting immutable string into createEpisodeContentInfo
(and any other functions referencing factory.baseURL()) so each coroutine uses
the captured value, or alternatively make factory.baseURL() and the backing
fields thread-safe (synchronized/volatile/immutable) if you must keep concurrent
access.
In
`@serenity-app/src/main/kotlin/us/nineworlds/serenity/core/model/impl/MovieMediaContainer.kt`:
- Around line 20-68: The code uses runBlocking inside createVideos which blocks
the calling thread; change createVideos to be a suspend fun and replace
runBlocking with coroutineScope (or simply make the mapping run in the current
coroutine), and remove unnecessary async/Dispatchers.Default overhead for
lightweight mapping — either map directly in the coroutine or wrap heavy CPU
work with withContext(Dispatchers.Default) for isolated CPU-bound work; update
all callers to call the new suspend createVideos (or document/bridge callers if
they cannot yet be suspend) and remove the runBlocking/async usage to avoid
thread blocking and coroutine dispatch overhead.
- Around line 39-40: MovieMediaContainer is assigning tick-based Longs directly
to Int fields (resumeOffset, duration); convert ticks to milliseconds first
using the existing convertTicksToMilliseconds(...) helper before casting to Int.
Update the assignments in MovieMediaContainer (where resumeOffset =
movie.viewOffset.toInt() and duration = movie.duration.toInt()) to call
convertTicksToMilliseconds(movie.viewOffset) and
convertTicksToMilliseconds(movie.duration) and then cast to Int, following the
pattern used in JellyfinMediaContainerAdaptor and MediaContainerAdaptor to avoid
truncation/overflow.
In
`@serenity-app/src/main/kotlin/us/nineworlds/serenity/core/paging/EpisodePagingSource.kt`:
- Around line 24-26: The catch block in EpisodePagingSource.kt (and the
equivalent in SimilarItemsPagingSource.kt) currently catches Exception which
swallows CancellationException; update the error handling in the load(...)
method(s) to rethrow CancellationException (e.g., if (ex is
CancellationException) throw ex) before returning LoadResult.Error, or restrict
the catch to more specific exceptions like IOException so cancellation
propagates correctly; modify the catch blocks in EpisodePagingSource.load and
SimilarItemsPagingSource.load accordingly.
In
`@serenity-app/src/main/kotlin/us/nineworlds/serenity/core/repository/VideoRepository.kt`:
- Around line 42-50: MovieMediaContainer.createVideos() currently uses
runBlocking and blocks threads when called from the suspend function
fetchSimilarItemsList; remove runBlocking and either (A) make createVideos() a
suspend function and implement its parallel chunking using coroutine primitives
(e.g., withContext(Dispatchers.IO) and coroutineScope/async/await or mapChunks
with mapConcurrent), or (B) change createVideos() to return a plain List
synchronously (no blocking) and move the chunked parallel processing into a new
suspend helper (e.g., fetchSimilarItemsList can call a suspend
processInChunks(...) that uses coroutineScope and async on Dispatchers.IO).
Update all call sites (like fetchSimilarItemsList and any uses similar to
EpisodeMediaContainer) to call the new suspend variant so no runBlocking
remains.
In
`@serenity-app/src/main/kotlin/us/nineworlds/serenity/fragments/mainmenu/MainMenuPresenter.kt`:
- Around line 57-66: The loading spinner is hidden immediately because
viewState.hideLoading() runs right after processesCategories returns while that
function launches child coroutines to fetch/paginate asynchronously; change the
flow so hideLoading() runs only after the first page is delivered — e.g., modify
processesCategories to either suspend until the first page is submitted (make it
a suspend function that awaits its child jobs or returns a Job/Deferred which
the caller can join/await), or add an onFirstPageLoaded callback/continuation
that you invoke where paging data is submitted (inside the paging
collector/submitData) and call viewState.hideLoading() there; update
presenterScope.launch to await the returned Job/Deferred or use the callback so
hideLoading() executes after first-page delivery.
In
`@serenity-app/src/main/kotlin/us/nineworlds/serenity/fragments/mainmenu/MainMenuView.kt`:
- Around line 21-22: The AddToEndStrategy is causing stale PagingData commands
to accumulate for updateCategories when populateMovieCategories creates new
collectLatest flows and galleryJob?.cancel() only stops active flows; ensure
clearCategories() is invoked before loading new categories in
populateMovieCategories (and any other movie/tv library reload paths) so queued
updateCategories entries are removed, or replace AddToEndStrategy for
updateCategories with a custom Moxy StateStrategy that removes previous
updateCategories commands when new ones are enqueued; update the code paths that
call populateMovieCategories and the view interface method updateCategories to
perform clearCategories() first or adopt the custom strategy to prevent
replaying single-use PagingData.
In
`@serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/activity/leanback/details/DetailsFragment.kt`:
- Around line 250-252: updateSeasonEpisodes currently launches a lifecycleScope
coroutine that calls adapter.submitData without cancelling prior collectors,
which leaks coroutines; change it to track and cancel a previously stored Job
(e.g., seasonEpisodesJob) or use a dedicated CoroutineScope tied to that adapter
before launching a new coroutine that calls adapter.submitData, and do the same
pattern for addSimilarItems (e.g., similarItemsJob) so any existing Job is
cancelled prior to re-subscribing to new PagingData.
- Around line 62-68: areContentsTheSame in videoContentDiffCallback currently
uses oldItem == newItem which falls back to reference equality because
AbstractVideoContentInfo and its subclasses (MoviePosterInfo, EpisodePosterInfo)
don't override equals()/hashCode(); either implement content-aware equality by
overriding equals() and hashCode() in AbstractVideoContentInfo (comparing
identity fields like id, title, artwork, etc.) so existing comparisons work, or
change the areContentsTheSame implementation to explicitly compare the relevant
fields of VideoContentInfo (e.g., id(), title(), posterPath, metadata) to
determine content equality.
In
`@serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/activity/leanback/details/DetailsMVPPresenter.kt`:
- Around line 99-103: The current call passes season.key.orEmpty() into
EpisodePagingSource which silently converts null to an empty string and can
cause failed/empty API calls; update the logic in DetailsMVPPresenter where the
PagingSource is created (the pagingSourceFactory that constructs
EpisodePagingSource) to first validate season.key and only instantiate
EpisodePagingSource when season.key is non-null and non-blank (e.g., skip or
return an empty/failed Flow for seasons with null/blank keys), otherwise
short-circuit to an appropriate empty paging/flow result so you don't pass ""
into EpisodePagingSource.
In
`@serenity-app/src/test/kotlin/us/nineworlds/serenity/core/repository/CategoryRepositoryTest.kt`:
- Around line 41-45: The tearDown() method in CategoryRepositoryTest is missing
a call to closeScope(), causing Toothpick DI scope state to leak between tests;
update the CategoryRepositoryTest.tearDown() to call closeScope() (matching
VideoRepositoryTest and SimilarItemsPagingSourceTest) after clearAllMocks() and
before/after Dispatchers.resetMain() to ensure the Toothpick scope is properly
cleaned up.
🧹 Nitpick comments (10)
serenity-app/src/main/kotlin/us/nineworlds/serenity/core/model/impl/MovieMediaContainer.kt (1)
44-45: Minor:replaceFirst("/", "")may not handle all URL patterns correctly.Line 44 uses
movie.backgroundImageKey?.let { "$baseUrl${it.replaceFirst("/", "")}" }— if the key doesn't start with/, this will strip the first/from anywhere in the string. Consider usingtrimStart('/')orremovePrefix("/")for safer prefix-only removal.Suggested fix
-setBackgroundURL(movie.backgroundImageKey?.let { "$baseUrl${it.replaceFirst("/", "")}" } ?: baseImageResource) -setImageURL(movie.thumbNailImageKey?.let { "$baseUrl${it.replaceFirst("/", "")}" } ?: "") +setBackgroundURL(movie.backgroundImageKey?.let { "$baseUrl${it.removePrefix("/")}" } ?: baseImageResource) +setImageURL(movie.thumbNailImageKey?.let { "$baseUrl${it.removePrefix("/")}" } ?: "")Also apply the same fix at line 54:
-directPlayUrl = "$baseUrl${it.key.replaceFirst("/", "")}" +directPlayUrl = "$baseUrl${it.key.removePrefix("/")}"serenity-app/build.gradle.kts (1)
133-134: Paging dependencies added correctly.The
paging-runtime-ktxandleanback-pagingdependencies are properly referenced from the version catalog. The codebase includes three PagingSource test files (EpisodePagingSourceTest, VideoCategoryPagingSourceTest, SimilarItemsPagingSourceTest) that manually construct LoadParams and assert results. Whileandroidx.paging:paging-testingwithTestPagercould simplify this boilerplate in the future, it's not required—the current tests work effectively with the existing approach.serenity-app/src/test/kotlin/us/nineworlds/serenity/core/paging/EpisodePagingSourceTest.kt (1)
100-111: Consider adding a test forAppendload (non-zero key) to cover multi-page scenarios.The current tests only exercise
Refreshwithkey = 0. A test usingPagingSource.LoadParams.Appendwith a non-zero key (e.g.,key = 30) would validate thatstartIndexis correctly forwarded to the repository for subsequent pages, and thatprevKeyis set appropriately.Also, the test name
getRefreshKey returns correct keyis slightly misleading since the expected value isnull. A name likegetRefreshKey returns nullwould be clearer.serenity-app/src/main/kotlin/us/nineworlds/serenity/fragments/mainmenu/MainMenuPresenter.kt (1)
69-69: Typo in method name:processesCategoriesshould beprocessCategories.Proposed fix
- private fun processesCategories(scope: CoroutineScope, categories: List<CategoryInfo>, itemId: String, type: String) { + private fun processCategories(scope: CoroutineScope, categories: List<CategoryInfo>, itemId: String, type: String) {And update the call site on Line 60 accordingly.
serenity-app/src/main/kotlin/us/nineworlds/serenity/core/model/impl/EpisodeMediaContainer.kt (1)
21-29:runBlockinginside a coroutine context blocks the IO thread unnecessarily.
createVideos()is called fromVideoRepository.fetchEpisodeswhich already runs insidewithContext(Dispatchers.IO). UsingrunBlockinghere blocks the IO dispatcher thread while waiting forDispatchers.Defaultwork to complete, wasting the thread. Prefer makingcreateVideos()asuspendfunction and usingcoroutineScopeinstead.Suggested refactor
- override fun createVideos(): List<VideoContentInfo> { + override suspend fun createVideos(): List<VideoContentInfo> { val videos = mc.videos ?: return emptyList() val baseUrl = factory.baseURL().orEmpty() var parentPosterURL: String? = null if (!mc.parentPosterURL.isNullOrEmpty() && !mc.parentPosterURL.contains("show")) { parentPosterURL = baseUrl + mc.parentPosterURL.substring(1) } - val createdVideos = runBlocking { + val createdVideos = coroutineScope { videos.chunked(25).map { chunk -> async(Dispatchers.Default) { chunk.map { episode -> createEpisodeContentInfo(mc, baseUrl, parentPosterURL, episode) } } }.awaitAll().flatten() }This would require the parent class's
createVideos()signature to also besuspend, which may be a broader change. If that's not feasible now, consider at minimum documenting whyrunBlockingis used here.As per coding guidelines,
**/*.kt: Use Coroutines and Retrofit for asynchronous operations.serenity-app/src/test/kotlin/us/nineworlds/serenity/core/paging/VideoCategoryPagingSourceTest.kt (1)
111-122:getRefreshKeytest only covers the trivial null path.With
pages = emptyList(),closestPageToPosition(10)returnsnull, so the entire expression short-circuits tonull. This doesn't exercise the actual key computation logic (prevKey + pageSizeornextKey - pageSize). Consider adding a test case with at least onePagein the state to validate the non-null branch.serenity-app/src/main/kotlin/us/nineworlds/serenity/core/paging/EpisodePagingSource.kt (1)
13-27: Inconsistent error-handling pattern compared toVideoCategoryPagingSource.
EpisodePagingSourceandSimilarItemsPagingSourceuse try-catch on the raw repository call, whileVideoCategoryPagingSourceuses aResultsealed class. This inconsistency means one path risks swallowingCancellationExceptionwhile the other doesn't. Consider aligning all three paging sources to use the same pattern (preferably theResultsealed class approach which avoids catching exceptions at all).serenity-app/src/test/kotlin/us/nineworlds/serenity/core/paging/SimilarItemsPagingSourceTest.kt (1)
71-87: Consider also testing a non-zerokeyto validate subsequent page loads.The two load tests both use
key = 0. Adding a test withkey = 30(simulating a second page load) would verify thatstartIndexis correctly forwarded andprevKey/nextKeyare computed properly for non-initial pages.serenity-app/src/test/kotlin/us/nineworlds/serenity/core/repository/VideoRepositoryTest.kt (1)
58-100: Tests verify delegation but not data transformation.All three tests only assert the result type (
isInstanceOf) and that the client was called with correct parameters. They don't verify that the repository correctly transforms theIMediaContainerresponse into the returned list. Consider adding assertions on the returned data (e.g., list size or content) to catch regressions in the mapping logic.serenity-app/src/main/java/us/nineworlds/serenity/fragments/MainMenuVideoContentVerticalGridFragment.kt (1)
38-38:ConcurrentHashMapis unnecessary here — all access is on the main thread.Fragment lifecycle callbacks and UI updates are main-thread-only. A plain
HashMapormutableMapOf()would be simpler and slightly more efficient.
| client = EmbyAPIClient(context = ApplicationProvider.getApplicationContext()) | ||
| // Update this for local testing of the client and make sure it works | ||
| client.updateBaseUrl("http://yourserver:yourport") | ||
| client.updateBaseUrl("http://192.168.68.97:8096") |
There was a problem hiding this comment.
Hardcoded local IP address will break CI and other developers' environments.
"http://192.168.68.97:8096" is a developer-specific LAN address. This will cause all tests in this class to fail in CI or for anyone without this specific server. Consider reverting to "http://localhost:8096" or reading from an environment variable, and marking these integration tests with @Ignore (or a custom annotation/category) so they don't run in CI by default.
🤖 Prompt for AI Agents
In
`@emby-lib/src/test/kotlin/us/nineworlds/serenity/emby/server/api/EmbyAPIClientTest.kt`
at line 34, Replace the hardcoded LAN IP in the test by making the base URL
configurable and avoiding running against a developer-specific server: change
the call to client.updateBaseUrl(...) (in EmbyAPIClientTest) to read from an
environment variable (e.g., EMBY_BASE_URL) with a safe default like
"http://localhost:8096", and mark the integration test class or individual tests
with `@Ignore` (or a project test-category annotation) so they do not run in CI by
default.
| androidPluginVersion = "8.13.1" | ||
| leanbackPreferenceVersion = "1.2.0" | ||
| leanbackVersion = "1.2.0" | ||
| leanbackPagingVersion = "1.1.0" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
androidx.leanback:leanback-paging latest version 2025
💡 Result:
The latest 2025 release of androidx.leanback:leanback-paging is 1.1.0, released April 23, 2025. [1][2]
Gradle
implementation("androidx.leanback:leanback-paging:1.1.0")🌐 Web query:
androidx.paging:paging-runtime-ktx latest version 2025 compatibility
💡 Result:
- Latest stable in 2025 (recommended for production):
3.3.6(released Feb 12, 2025) forandroidx.paging:paging-runtime-ktx. [1] - Latest 2025 prerelease on the 3.4 line:
3.4.0-beta01(released Dec 17, 2025). [2]
2025 compatibility notes
- Android minSdk
- Paging 3.3.x: default minSdk 21 (implied by the 3.4 change below). [2]
- Paging 3.4.0 (starting in 2025 alphas): default minSdk moved from 21 → 23 (notably called out in 3.4.0-alpha03, Aug 27, 2025). [2]
What to choose
- If you need API 21–22 support in 2025: use
3.3.6. [1][2] - If your app is minSdk 23+ and you want the newer 3.4 line in 2025: use
3.4.0-beta01(or a later 3.4 prerelease). [2]
Sources:
[1] Maven artifact page showing paging-runtime-ktx:3.3.6 date/version. (mvnrepository.com)
[2] Official AndroidX Paging release notes (3.4.0 timeline + minSdk 21→23 change). (developer.android.com)
Citations:
- 1: https://mvnrepository.com/artifact/androidx.paging/paging-runtime-ktx/3.3.6?utm_source=openai
- 2: https://developer.android.com/jetpack/androidx/releases/paging?utm_source=openai
🌐 Web query:
androidx leanback-paging 1.1.0 paging 3.4.0 compatibility
💡 Result:
androidx.leanback:leanback-paging:1.1.0is built to use Paging 3.1.0 (it was updated to Paging 3.1.0 in1.1.0-alpha10, and1.1.0later moved that line to stable “with no changes”). [1]- Its published Maven metadata also shows a compile dependency on
androidx.paging:paging-runtime:3.1.0. [2] - Paging 3.4.0 is a newer stable release (current stable listed as 3.4.0). [3]
What that means for “compatibility”:
- There’s no official guarantee (in the Leanback-Paging 1.1.0 notes/metadata) that it’s tested against Paging 3.4.0; the declared target is 3.1.0. [1][2]
- If you add
paging-runtime:3.4.0to your app, Gradle will typically resolve to 3.4.0 (newer wins), but you should treat that as “works if binary-compatible” and verify with builds + runtime testing (watch forNoSuchMethodError/ClassNotFoundException).
Sources: [1] AndroidX Leanback release notes, [2] MVNRepository dependency listing for leanback-paging 1.1.0, [3] AndroidX Paging release notes.
Use paging-runtime-ktx 3.3.6 instead of 3.4.0 to ensure compatibility with leanback-paging 1.1.0.
The leanbackPagingVersion = "1.1.0" is correct (latest, released April 23, 2025), but pagingVersion = "3.4.0" creates a compatibility issue. The leanback-paging 1.1.0 library was built against and tested with paging 3.1.0. Using paging 3.4.0—which raised the minSdk from 21 to 23 and introduces significant version drift—risks runtime failures without explicit verification. The recommended stable version for 2025 is paging 3.3.6 (Feb 12, 2025). If minSdk 23+ is confirmed, either use 3.3.6 or verify binary compatibility with 3.4.0 through testing.
Also applies to: 33-33, 79-79, 83-83
🤖 Prompt for AI Agents
In `@gradle/libs.versions.toml` at line 23, The libs.versions.toml currently sets
leanbackPagingVersion = "1.1.0" but uses pagingVersion = "3.4.0", which is
incompatible; update the pagingVersion entries (all occurrences of the
pagingVersion property in the file) to use paging-runtime-ktx 3.3.6 (i.e., set
pagingVersion = "3.3.6") so leanback-paging 1.1.0 remains compatible, and ensure
any dependency aliases that reference pagingVersion are aligned to the new
value.
serenity-app/src/main/kotlin/us/nineworlds/serenity/core/model/impl/EpisodeMediaContainer.kt
Show resolved
Hide resolved
| @StateStrategyType(AddToEndStrategy::class) | ||
| fun updateCategories(category: CategoryInfo, pagingData: PagingData<VideoCategory>) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how clearCategories interacts with the view state and if there's cleanup logic
echo "=== clearCategories implementations ==="
rg -n 'clearCategories' --type=kt --type=java -C5
echo ""
echo "=== AddToEndStrategy usage ==="
rg -n 'AddToEndStrategy' --type=kt --type=java -C2Repository: NineWorlds/serenity-android
Length of output: 213
🏁 Script executed:
#!/bin/bash
echo "=== clearCategories implementations ==="
rg 'clearCategories' -C5
echo ""
echo "=== AddToEndStrategy usage ==="
rg 'AddToEndStrategy' -C2
echo ""
echo "=== MainMenuView.kt content ==="
find . -name 'MainMenuView.kt' -type f -exec cat -n {} \;Repository: NineWorlds/serenity-android
Length of output: 8200
🏁 Script executed:
#!/bin/bash
echo "=== MainMenuPresenter.kt full content ==="
find . -name 'MainMenuPresenter.kt' -path '*/mainmenu/*' -type f -exec cat -n {} \;
echo ""
echo "=== MainMenuVideoContentVerticalGridFragment clearGallery and updateCategory ==="
rg 'fun clearGallery|fun updateCategory' -A 10Repository: NineWorlds/serenity-android
Length of output: 7557
🏁 Script executed:
#!/bin/bash
echo "=== Check Moxy lifecycle and view state management ==="
rg 'presenterScope|InjectViewState|viewState' serenity-app/src/main/kotlin/us/nineworlds/serenity/fragments/mainmenu/ -A2 -B2
echo ""
echo "=== Check if there's any view state clearing on detach/destroy ==="
rg 'onDestroy|onDetach' serenity-app/src/main/kotlin/us/nineworlds/serenity/fragments/mainmenu/ -A5Repository: NineWorlds/serenity-android
Length of output: 5830
🏁 Script executed:
#!/bin/bash
echo "=== MainMenuFragment implementation ==="
find . -name 'MainMenuFragment.kt' -type f -exec cat -n {} \;Repository: NineWorlds/serenity-android
Length of output: 6558
AddToEndStrategy accumulation with PagingData when switching movie/tvshow categories without explicit clearing.
When populateMovieCategories is called repeatedly for movie or tvshow types, each call creates new collectLatest flows that queue updateCategories commands via AddToEndStrategy. Switching between different movie libraries calls galleryJob?.cancel(), which stops active flows, but the queued commands remain in Moxy's view state queue indefinitely. Since clearCategories() is only invoked for non-movie types, normal movie-to-movie switches never trigger view state cleanup.
On view detach/reattach (e.g., rotation), Moxy replays accumulated commands. Replaying stale PagingData objects is problematic since PagingData is single-use and cannot be collected twice. While the fragment's updateCategory silently skips missing adapters, this masks inefficiency and potential data consistency issues.
Call clearCategories() before loading new categories regardless of type, or use a custom Moxy StateStrategy that clears previous updateCategories entries when categories reload.
🤖 Prompt for AI Agents
In
`@serenity-app/src/main/kotlin/us/nineworlds/serenity/fragments/mainmenu/MainMenuView.kt`
around lines 21 - 22, The AddToEndStrategy is causing stale PagingData commands
to accumulate for updateCategories when populateMovieCategories creates new
collectLatest flows and galleryJob?.cancel() only stops active flows; ensure
clearCategories() is invoked before loading new categories in
populateMovieCategories (and any other movie/tv library reload paths) so queued
updateCategories entries are removed, or replace AddToEndStrategy for
updateCategories with a custom Moxy StateStrategy that removes previous
updateCategories commands when new ones are enqueued; update the code paths that
call populateMovieCategories and the view interface method updateCategories to
perform clearCategories() first or adopt the custom strategy to prevent
replaying single-use PagingData.
| private val videoContentDiffCallback = object : DiffUtil.ItemCallback<VideoContentInfo>() { | ||
| override fun areItemsTheSame(oldItem: VideoContentInfo, newItem: VideoContentInfo): Boolean = | ||
| oldItem.id() == newItem.id() | ||
|
|
||
| override fun areContentsTheSame(oldItem: VideoContentInfo, newItem: VideoContentInfo): Boolean = | ||
| oldItem == newItem | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the VideoContentInfo class
fd -t f -e kt | xargs rg -l 'class VideoContentInfo' | head -5Repository: NineWorlds/serenity-android
Length of output: 174
🏁 Script executed:
# Search for VideoContentInfo class definition with context
rg -n 'class VideoContentInfo' -A 10 -B 2 --type=ktRepository: NineWorlds/serenity-android
Length of output: 97
🏁 Script executed:
# Check if it's a data class and if it overrides equals
rg -n 'data class VideoContentInfo|override fun equals' -B 5 -A 5 --type=ktRepository: NineWorlds/serenity-android
Length of output: 97
🏁 Script executed:
# Find all kotlin files that mention VideoContentInfo
fd -e kt -x rg -l 'class VideoContentInfo' {} \;Repository: NineWorlds/serenity-android
Length of output: 176
🏁 Script executed:
# Search across all kotlin files for VideoContentInfo class definition
rg 'class VideoContentInfo' -A 15 --glob='*.kt'Repository: NineWorlds/serenity-android
Length of output: 2417
🏁 Script executed:
# Also check if it's a data class
rg 'data class VideoContentInfo' -A 15 --glob='*.kt'Repository: NineWorlds/serenity-android
Length of output: 53
🏁 Script executed:
# Search for all mentions of VideoContentInfo to find where it's defined
rg 'VideoContentInfo' --glob='*.kt' | grep -E '(class|interface|data class) VideoContentInfo' | head -10Repository: NineWorlds/serenity-android
Length of output: 346
🏁 Script executed:
# Look for VideoContentInfo in all files more broadly
rg 'VideoContentInfo' --glob='*.kt' -l | head -20Repository: NineWorlds/serenity-android
Length of output: 1897
🏁 Script executed:
# Check if it's defined in the DetailsFragment or nearby files
rg 'VideoContentInfo' --glob='*.kt' -B 2 -A 5 serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/activity/leanback/details/ 2>/dev/null | head -100Repository: NineWorlds/serenity-android
Length of output: 13383
🏁 Script executed:
# Read the VideoContentInfo interface
cat -n serenity-app/src/main/java/us/nineworlds/serenity/core/model/VideoContentInfo.ktRepository: NineWorlds/serenity-android
Length of output: 1776
🏁 Script executed:
# Check ContentInfo interface it extends
rg 'interface ContentInfo' -A 20 --glob='*.kt'Repository: NineWorlds/serenity-android
Length of output: 1800
🏁 Script executed:
# Check concrete implementations of VideoContentInfo
rg 'class.*:.*VideoContentInfo|class.*implements.*VideoContentInfo' -A 5 --glob='*.kt'Repository: NineWorlds/serenity-android
Length of output: 1121
🏁 Script executed:
# Check AbstractVideoContentInfo implementation
cat -n serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/AbstractVideoContentInfo.kt | head -150Repository: NineWorlds/serenity-android
Length of output: 6098
🏁 Script executed:
# Look for concrete implementations that extend AbstractVideoContentInfo
rg 'class.*AbstractVideoContentInfo' -A 10 --glob='*.kt'Repository: NineWorlds/serenity-android
Length of output: 4306
🏁 Script executed:
# Search for equals() implementations in AbstractVideoContentInfo and concrete classes
rg 'override fun equals|class.*VideoContentInfo.*{' -B 2 -A 8 --glob='*.kt' serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/Repository: NineWorlds/serenity-android
Length of output: 226
🏁 Script executed:
# Search more carefully for equals in AbstractVideoContentInfo
grep -n 'override fun equals' serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/AbstractVideoContentInfo.ktRepository: NineWorlds/serenity-android
Length of output: 53
🏁 Script executed:
# Get the full size of AbstractVideoContentInfo to know how much to read
wc -l serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/AbstractVideoContentInfo.ktRepository: NineWorlds/serenity-android
Length of output: 168
🏁 Script executed:
# Check concrete implementations - look for MoviePosterInfo and EpisodePosterInfo
cat -n serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/MoviePosterInfo.ktRepository: NineWorlds/serenity-android
Length of output: 401
🏁 Script executed:
# Read the rest of AbstractVideoContentInfo to see if equals() is defined
tail -70 serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/AbstractVideoContentInfo.ktRepository: NineWorlds/serenity-android
Length of output: 2274
🏁 Script executed:
# Check for EpisodePosterInfo
cat -n serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/EpisodePosterInfo.ktRepository: NineWorlds/serenity-android
Length of output: 474
🏁 Script executed:
# Find all classes extending AbstractVideoContentInfo
rg 'extends AbstractVideoContentInfo|: AbstractVideoContentInfo' --glob='*.kt' -A 3Repository: NineWorlds/serenity-android
Length of output: 626
areContentsTheSame uses reference equality instead of content equality — VideoContentInfo implementations don't override equals().
The oldItem == newItem comparison relies on equals(). Since AbstractVideoContentInfo and its concrete subclasses (MoviePosterInfo, EpisodePosterInfo) don't override equals(), this falls back to reference equality. Two distinct instances with identical content will not be considered equal, causing DiffUtil to skip the content comparison optimization and force full rebinds unnecessarily. Override equals() and hashCode() in AbstractVideoContentInfo or implement content-aware comparison in areContentsTheSame.
🤖 Prompt for AI Agents
In
`@serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/activity/leanback/details/DetailsFragment.kt`
around lines 62 - 68, areContentsTheSame in videoContentDiffCallback currently
uses oldItem == newItem which falls back to reference equality because
AbstractVideoContentInfo and its subclasses (MoviePosterInfo, EpisodePosterInfo)
don't override equals()/hashCode(); either implement content-aware equality by
overriding equals() and hashCode() in AbstractVideoContentInfo (comparing
identity fields like id, title, artwork, etc.) so existing comparisons work, or
change the areContentsTheSame implementation to explicitly compare the relevant
fields of VideoContentInfo (e.g., id(), title(), posterPath, metadata) to
determine content equality.
...y-app/src/main/kotlin/us/nineworlds/serenity/ui/activity/leanback/details/DetailsFragment.kt
Show resolved
Hide resolved
...p/src/main/kotlin/us/nineworlds/serenity/ui/activity/leanback/details/DetailsMVPPresenter.kt
Show resolved
Hide resolved
| @After | ||
| fun tearDown() { | ||
| clearAllMocks() | ||
| Dispatchers.resetMain() | ||
| } |
There was a problem hiding this comment.
Missing closeScope() in tearDown — inconsistent with other test classes.
Both VideoRepositoryTest and SimilarItemsPagingSourceTest call closeScope() during teardown to clean up the Toothpick DI scope. This test omits it, which could leak scope state between tests.
Proposed fix
`@After`
fun tearDown() {
clearAllMocks()
Dispatchers.resetMain()
+ closeScope()
}📝 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.
| @After | |
| fun tearDown() { | |
| clearAllMocks() | |
| Dispatchers.resetMain() | |
| } | |
| `@After` | |
| fun tearDown() { | |
| clearAllMocks() | |
| Dispatchers.resetMain() | |
| closeScope() | |
| } |
🤖 Prompt for AI Agents
In
`@serenity-app/src/test/kotlin/us/nineworlds/serenity/core/repository/CategoryRepositoryTest.kt`
around lines 41 - 45, The tearDown() method in CategoryRepositoryTest is missing
a call to closeScope(), causing Toothpick DI scope state to leak between tests;
update the CategoryRepositoryTest.tearDown() to call closeScope() (matching
VideoRepositoryTest and SimilarItemsPagingSourceTest) after clearAllMocks() and
before/after Dispatchers.resetMain() to ensure the Toothpick scope is properly
cleaned up.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
serenity-app/src/main/java/us/nineworlds/serenity/fragments/MainMenuVideoContentVerticalGridFragment.kt (1)
106-120:⚠️ Potential issue | 🟡 MinorCategories with
nullcategoryDetailcreate orphaned rows that can never receive data.When
category.categoryDetailis null (Line 111), the adapter is not stored inadapterMap, but theListRowis still added tovideoContentAdapter(Line 118). This meansupdateCategorycan never submit paging data to that row, leaving it permanently empty.Either skip adding the row entirely when
categoryDetailis null, or ensure it's always non-null upstream.Proposed fix
for (category in categories.categories) { + val detail = category.categoryDetail ?: continue val listContentRowAdapter = SerenityPagingDataAdapter(CategoryVideoPresenter(), videoCategoryDiffCallback) - category.categoryDetail?.let { detail -> - adapterMap[detail] = listContentRowAdapter - } + adapterMap[detail] = listContentRowAdapter - val header = HeaderItem(category.categoryDetail) + val header = HeaderItem(detail) val imageListRow = ListRow(header, listContentRowAdapter) videoContentAdapter.add(imageListRow)
🤖 Fix all issues with AI agents
In
`@serenity-app/src/main/kotlin/us/nineworlds/serenity/core/paging/SimilarItemsPagingSource.kt`:
- Around line 11-25: In SimilarItemsPagingSource.load, the prevKey arithmetic
uses params.loadSize inconsistently causing incorrect prevKey after the larger
initialLoadSize; update prevKey to subtract params.loadSize (or a dedicated
pageSize property) from position so key math matches the load window — e.g., set
prevKey = if (position == 0) null else position - params.loadSize and keep
nextKey logic consistent (use params.loadSize or the fixed Pager pageSize value
for both prevKey and nextKey calculations) in the LoadResult.Page construction.
🧹 Nitpick comments (1)
serenity-app/src/main/java/us/nineworlds/serenity/fragments/MainMenuVideoContentVerticalGridFragment.kt (1)
38-38:ConcurrentHashMapis unnecessary — all access is on the main thread.Both
setupGallery,clearGallery, andupdateCategory(vialifecycleScope) run on the main thread. A plainHashMapormutableMapOf()would suffice and better communicates the intended threading model.
serenity-app/src/main/kotlin/us/nineworlds/serenity/core/paging/SimilarItemsPagingSource.kt
Show resolved
Hide resolved
Fix failing test and address a few code review comments around Paging.
This reverts commit 8f1bceb.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
serenity-app/src/main/kotlin/us/nineworlds/serenity/core/model/impl/EpisodeMediaContainer.kt (1)
57-57:⚠️ Potential issue | 🟡 MinorPossible typo:
"${baseUrl}:/resources/..."— the://scheme separator looks wrong.After switching to
baseUrl(which likely already contains the scheme and host, e.g.http://host:port), the colon before/resources/show-fanart.jpgwould produce a malformed URL likehttp://host:port:/resources/show-fanart.jpg. This should likely be"${baseUrl}/resources/show-fanart.jpg"(no colon).
🤖 Fix all issues with AI agents
In
`@serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/activity/leanback/details/DetailsFragment.kt`:
- Around line 259-273: addSimilarItems currently constructs a new ListRow and
SerenityPagingDataAdapter each call (creating duplicate "Similar" rows when
PagingData re-emits); instead, create and reuse the ListRow and items adapter
once (e.g., as lazy or class-level properties) and only call
itemsAdapter.submitData(pagingData) inside addSimilarItems, or guard the
detailsAdapter.add(similarRow) so it only adds the row if it's not already
present; update references to ListRow, SerenityPagingDataAdapter/ itemsAdapter,
similarRow, similarItemsJob, and addSimilarItems to use the single shared
instance and cancel/launch the job only to submitData.
🧹 Nitpick comments (3)
serenity-app/src/main/kotlin/us/nineworlds/serenity/core/model/impl/EpisodeMediaContainer.kt (1)
47-51: Redundant null checks —isNullOrEmpty()already coversnull.On lines 47 and 50,
episode.parentThumbNailImageKey != null && !episode.parentThumbNailImageKey.isNullOrEmpty()can be simplified to!episode.parentThumbNailImageKey.isNullOrEmpty(). Same forgrandParentThumbNailImageKey.Proposed simplification
- if (episode.parentThumbNailImageKey != null && !episode.parentThumbNailImageKey.isNullOrEmpty()) { + if (!episode.parentThumbNailImageKey.isNullOrEmpty()) { this.parentPosterURL = baseUrl + episode.parentThumbNailImageKey.substring(1) } - if (episode.grandParentThumbNailImageKey != null && !episode.grandParentThumbNailImageKey.isNullOrEmpty()) { + if (!episode.grandParentThumbNailImageKey.isNullOrEmpty()) { grandParentPosterURL = baseUrl + episode.grandParentThumbNailImageKey.substring(1) }serenity-app/src/main/kotlin/us/nineworlds/serenity/core/paging/SimilarItemsPagingSource.kt (1)
11-24: Minor formatting nit on Line 20.The closing parenthesis of
LoadResult.Page(...)is appended to the end of thenextKeyline rather than on its own line. This makes the code harder to scan.- nextKey = if (data.isEmpty() || data.size < loadSize) null else position + loadSize ) + nextKey = if (data.isEmpty() || data.size < loadSize) null else position + loadSize + )serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/activity/leanback/details/DetailsMVPPresenter.kt (1)
92-114: Season key null-guard looks good; consider extracting duplicatePagingConfig.The
seasonsKeynull check on Line 95 properly addresses the earlier concern about empty keys.The identical
PagingConfig(pageSize = 15, initialLoadSize = 30, enablePlaceholders = false)is repeated here and at Line 59. Consider extracting it to a companion constant to keep page-size decisions in one place.Suggested refactor
class DetailsMVPPresenter : MvpPresenter<DetailsView>() { + + companion object { + private val DEFAULT_PAGING_CONFIG = PagingConfig( + pageSize = 15, + initialLoadSize = 30, + enablePlaceholders = false + ) + }Then reference
DEFAULT_PAGING_CONFIGin bothloadSimilarItemsandupdateSeries.
| override fun addSimilarItems(pagingData: PagingData<VideoContentInfo>) { | ||
| classPresenterSelector.addClassPresenter(ListRow::class.java, ListRowPresenter()) | ||
| val itemsAdapter = ArrayObjectAdapter(VideoContentInfoPresenter()) | ||
| val itemsAdapter = SerenityPagingDataAdapter(VideoContentInfoPresenter(), videoContentDiffCallback) | ||
|
|
||
| val header = HeaderItem("Similar") | ||
| itemsAdapter.addAll(0, videoInfo) | ||
| val similarRow = ListRow(header, itemsAdapter) | ||
|
|
||
| val detailsAdapter = adapter as ArrayObjectAdapter | ||
| detailsAdapter.add(similarRow) | ||
|
|
||
| similarItemsJob?.cancel() | ||
| similarItemsJob = viewLifecycleOwner.lifecycleScope.launch { | ||
| itemsAdapter.submitData(pagingData) | ||
| } | ||
| } |
There was a problem hiding this comment.
addSimilarItems creates a new row and adapter on every call.
If collectLatest in the presenter re-emits PagingData (e.g., on invalidation), this method is called again, adding a duplicate "Similar" row. Consider creating the row/adapter once (e.g., lazily or in setup) and only calling submitData on subsequent invocations.
🤖 Prompt for AI Agents
In
`@serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/activity/leanback/details/DetailsFragment.kt`
around lines 259 - 273, addSimilarItems currently constructs a new ListRow and
SerenityPagingDataAdapter each call (creating duplicate "Similar" rows when
PagingData re-emits); instead, create and reuse the ListRow and items adapter
once (e.g., as lazy or class-level properties) and only call
itemsAdapter.submitData(pagingData) inside addSimilarItems, or guard the
detailsAdapter.add(similarRow) so it only adds the row if it's not already
present; update references to ListRow, SerenityPagingDataAdapter/ itemsAdapter,
similarRow, similarItemsJob, and addSimilarItems to use the single shared
instance and cancel/launch the job only to submitData.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/activity/leanback/details/DetailsFragment.kt (1)
269-271:⚠️ Potential issue | 🔴 Critical
addSimilarSeriesthrowsTODO— this will crash at runtime when viewing similar items for a TV show.
DetailsMVPPresenter.loadSimilarItemscallsviewState.addSimilarSeries(...)for theTypes.SERIESpath (Line 81 of the presenter). This unimplemented method will throwNotImplementedErrorat runtime.Either implement the method or guard the call site in the presenter to avoid this path until the implementation is ready.
🤖 Fix all issues with AI agents
In
`@serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/activity/leanback/details/DetailsMVPPresenter.kt`:
- Around line 51-84: The SERIES branch in DetailsMVPPresenter.loadSimilarItems
calls viewState.addSimilarSeries(...) which currently throws a TODO in
DetailsFragment; update DetailsFragment to implement addSimilarSeries to accept
the list produced by SeriesMediaContainer(result).createSeries(), or modify
DetailsMVPPresenter to avoid calling addSimilarSeries until implemented (e.g.,
route SERIES results to the same paging UI or a safe no-op method). Locate
addSimilarSeries in DetailsFragment and implement the UI update logic (adapter
update or view binding) to safely handle the series list returned from
repository.fetchSimilarItems/SeriesMediaContainer.createSeries().
🧹 Nitpick comments (1)
serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/activity/leanback/details/DetailsMVPPresenter.kt (1)
93-111: Each season spawns an independentPager+collectLatestcoroutine — may trigger many concurrent API calls.For a show with many seasons, this
forEachloop creates N paging flows that all start loading immediately. Each initial load (size 30) will fire concurrently, which could overwhelm the backend or the device on lower-end hardware (like the TCL TV mentioned in the PR).Consider staggering the loads (e.g., only paginate the currently visible/selected season) or limiting concurrency with a semaphore/dispatcher. This isn't blocking but worth considering for reliability on constrained devices.
| fun loadSimilarItems(itemId: String, type: String) { | ||
| presenterScope.launch { | ||
| val itemType = when (type) { | ||
| "tvshows" -> Types.SERIES | ||
| else -> Types.MOVIES | ||
| } | ||
|
|
||
| val result = repository.fetchSimilarItems(itemId, itemType) | ||
| when (itemType) { | ||
| Types.MOVIES -> { | ||
| val videos = MovieMediaContainer(result) | ||
| .createVideos() | ||
| .filterNot { item -> | ||
| item.getType() == Types.SERIES | ||
| } | ||
| val itemType = when (type) { | ||
| "tvshows" -> Types.SERIES | ||
| else -> Types.MOVIES | ||
| } | ||
|
|
||
| viewState.addSimilarItems(videos) | ||
| if (itemType == Types.MOVIES) { | ||
| val pagingFlow = Pager( | ||
| config = PagingConfig( | ||
| pageSize = 15, | ||
| initialLoadSize = 30, | ||
| enablePlaceholders = false | ||
| ), | ||
| pagingSourceFactory = { | ||
| SimilarItemsPagingSource(repository, itemId, itemType) | ||
| } | ||
| ).flow | ||
| .map { pagingData -> | ||
| pagingData.filter { item -> item.getType() != Types.SERIES } | ||
| } | ||
| .cachedIn(presenterScope) | ||
|
|
||
| else -> viewState.addSimilarSeries(SeriesMediaContainer(result).createSeries()) | ||
| presenterScope.launch { | ||
| pagingFlow.collectLatest { pagingData -> | ||
| viewState.addSimilarItems(pagingData) | ||
| } | ||
| } | ||
| } else { | ||
| presenterScope.launch { | ||
| val result = repository.fetchSimilarItems(itemId, itemType) | ||
| viewState.addSimilarSeries(SeriesMediaContainer(result).createSeries()) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Paging flow for similar MOVIES with filtering and caching is well-wired.
The Pager → flow → map { filter } → cachedIn → collectLatest pipeline follows the standard Paging 3 pattern correctly. The fallback to direct fetch for SERIES is a reasonable interim approach.
One note: addSimilarSeries in DetailsFragment throws TODO("Not yet implemented") (Line 270 of DetailsFragment.kt), so the SERIES path on Line 81 will crash at runtime.
#!/bin/bash
# Verify addSimilarSeries implementation
rg -n 'addSimilarSeries' --type=kt -C 3🤖 Prompt for AI Agents
In
`@serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/activity/leanback/details/DetailsMVPPresenter.kt`
around lines 51 - 84, The SERIES branch in DetailsMVPPresenter.loadSimilarItems
calls viewState.addSimilarSeries(...) which currently throws a TODO in
DetailsFragment; update DetailsFragment to implement addSimilarSeries to accept
the list produced by SeriesMediaContainer(result).createSeries(), or modify
DetailsMVPPresenter to avoid calling addSimilarSeries until implemented (e.g.,
route SERIES results to the same paging UI or a safe no-op method). Locate
addSimilarSeries in DetailsFragment and implement the UI update logic (adapter
update or view binding) to safely handle the series list returned from
repository.fetchSimilarItems/SeriesMediaContainer.createSeries().
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #516 +/- ##
=============================================
+ Coverage 48.63% 53.23% +4.59%
- Complexity 474 518 +44
=============================================
Files 65 69 +4
Lines 2204 2303 +99
Branches 312 329 +17
=============================================
+ Hits 1072 1226 +154
+ Misses 1001 911 -90
- Partials 131 166 +35
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:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@serenity-app/src/main/kotlin/us/nineworlds/serenity/core/paging/VideoCategoryPagingSource.kt`:
- Around line 19-23: The computed prevKey in VideoCategoryPagingSource's
LoadResult.Page can go negative (position - loadSize) after a refresh; update
the prevKey calculation to clamp to zero (e.g., use coerceAtLeast(0) or
Math.max(0, position - loadSize)) so the repository never receives a negative
startIndex, leaving the nextKey logic unchanged.
🧹 Nitpick comments (4)
serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/leanback/adapters/SerenityPagingDataAdapter.kt (1)
50-59: Redundant double-guard inget().The explicit bounds check (Line 51) already returns
nullfor invalid positions, making thetry-catchonIndexOutOfBoundsExceptionunreachable under normal conditions. Consider keeping only one guard to reduce noise. The bounds check alone is sufficient.♻️ Simplify get()
override fun get(position: Int): Any? { if (position < 0 || position >= size()) { return null } - return try { - internalAdapter.get(position) - } catch (e: IndexOutOfBoundsException) { - null - } + return internalAdapter.get(position) }serenity-app/src/test/kotlin/us/nineworlds/serenity/ui/leanback/adapters/SerenityPagingDataAdapterTest.kt (2)
26-28:@Configannotation has no parameters.This is a no-op — Robolectric will use its defaults. If that's intentional, consider removing it to reduce noise, or specify the SDK level explicitly (e.g.,
@Config(sdk = [33])) for deterministic test behavior across environments.
80-111: Observer test could be more precise about which notification method fired.Currently all four observer callbacks set the same
notifiedflag, so the test confirms something was notified but not what. For a bridge adapter this is acceptable, but if you want stronger guarantees you could track which method was called (e.g.,onItemRangeInsertedis the expected one for fresh data).♻️ More precise observer assertion
- var notified = false + var insertedCount = 0 val observer = object : ObjectAdapter.DataObserver() { - override fun onChanged() { - notified = true - } - - override fun onItemRangeChanged(positionStart: Int, itemCount: Int) { - notified = true - } - override fun onItemRangeInserted(positionStart: Int, itemCount: Int) { - notified = true - } - - override fun onItemRangeRemoved(positionStart: Int, itemCount: Int) { - notified = true + insertedCount += itemCount } } adapter.registerObserver(observer) @@ // ... submitData ... - assertThat(notified).isTrue() + assertThat(insertedCount).isEqualTo(1)serenity-app/src/test/kotlin/us/nineworlds/serenity/ui/activity/leanback/details/DetailsMVPPresenterTest.kt (1)
44-51: Consider using the sameUnconfinedTestDispatcherinstance forsetMainandrunTest.
Dispatchers.setMain(Dispatchers.Unconfined)andrunTest(UnconfinedTestDispatcher())use separate dispatchers. If the presenter launches coroutines onDispatchers.Main, they won't be governed byrunTest's test scope, soadvanceUntilIdle()won't control them. It works here because both are unconfined (eager execution), but the recommended pattern is:private val testDispatcher = UnconfinedTestDispatcher() `@Before` fun setUp() { Dispatchers.setMain(testDispatcher) // ... } // then: runTest(testDispatcher) { ... }This ensures
runTestproperly tracks all coroutines launched onMain.
serenity-app/src/main/kotlin/us/nineworlds/serenity/core/paging/VideoCategoryPagingSource.kt
Show resolved
Hide resolved
…g/VideoCategoryPagingSource.kt Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@serenity-app/src/main/kotlin/us/nineworlds/serenity/core/paging/VideoCategoryPagingSource.kt`:
- Around line 16-22: The Result.Success branch in VideoCategoryPagingSource
(when handling repository.fetchItemsByCategory(...)) currently extracts
result.data but fails to return a LoadResult.Page; update the is Result.Success
block to return LoadResult.Page(items = data, prevKey = if (position - loadSize
<= 0) null else position - loadSize, nextKey = if (data.isEmpty()) null else
position + data.size) so the when expression yields a LoadResult<Int,
VideoContentInfo> and prevKey is clamped to avoid negative offsets.
serenity-app/src/main/kotlin/us/nineworlds/serenity/core/paging/VideoCategoryPagingSource.kt
Show resolved
Hide resolved
|
1 participant
Updates the code to make use of the PagingAdapters and Paging Libraries. This is a pretty big refactor adding, paging support to the Serenity Client, as well as updating both the EmbyAPIClient and JellfyinClients for this support.
Unit tests have been added for most of the code changes.
With these changes, time to first image appearing is reduced by almost half on a TCL TV.
Implements #374
Summary by CodeRabbit
New Features
Performance
Chores
Tests
Documentation