Make repeated ids safe as lazy list keys (title page cast crash) #62

Closed

justinzeus wants to merge 0 commits from fm/tv-dup-lazy-key into fm/tv-round2

justinzeus commented

2026-11-07 20:31:43 +01:00

Owner

Crash

Justin's crash report (ACRA upload from the installed round 2 Opt build 1.1.0-184-gbf2efcfd, Xiaomi MiTV-AFMU0, Android 14, armeabi-v7a), destination title/079c13d9-7e77-d32c-07f3-eaea68a76001:
java.lang.IllegalArgumentException: Key "cebe8282-34dd-72da-218f-5d15ad4ebab4" was already used. If you are using LazyColumn/Row please make sure you provide a unique key for each item. thrown in a Compose lazy-list measure pass on the main thread, no app frames (R8).

Root cause

The key is a UUID, so it is a Jellyfin item/person id (not a catalogue key like movie:123). On a held title page the only Wolfskin list keyed by a Jellyfin UUID that can repeat is the Cast row: TitleShelf.kt TitleCast used key = person.id, and TitleReadFacade.build (TitleReadFacade.kt:200) builds the cast from item.people filtered to type Actor with person.id as the id and no de-duplication. Jellyfin lists a person once per credit, so the same actor appearing twice (two roles, or a duplicated People entry) yields two cast items with the same id. The related shelf and the episode shelf (same file) used the same item.id pattern and had the same hazard in principle.
I could not query the real item (no credentials), so I cannot confirm which of the three it was; the Cast row is the strongest match (a UUID that is a person id, a held title). All three are fixed, so the crash is closed whichever it was.

Fix

New LazyKeys.kt: List<String>.uniqueKeys(). The first occurrence of an id keeps its plain key; each repeat gets a stable id#n. So focus/scroll restoration by key is unchanged for every list without a repeat, and repeats are kept visible (not dropped) because the focus/column index maps 1:1 onto the list and a person with two roles is legitimately shown for both. Applied by precomputing keys with remember and keying by index.

Audit of every keyed lazy list in the Wolfskin UI

Fixed (key could repeat):

Title page: Cast row (TitleShelf.kt), More-like-this row and Episodes row (TitleMediaShelf), Season chips (TitlePage.kt)
Person filmography grid (PersonPage.kt, row key was row.first().id; credits are distinct-by-id after paging, but a first-page repeat or the same title in two rows is cheap to cover)
Home shelves incl. the Continue watching lane (HomeShelf.kt): the merged resume + next-up lane can list the same episode from both sources; this is the card key
Library grid rows (LibraryScreen.kt)
Requests sheet rows (ComingInSheet.kt)

Checked and left alone:

Title page outer LazyColumn: fixed row names (header/seasons/episodes/cast/related/end-padding)
Skeleton lists on every screen: index keys
Home outer rows (HomeScreen.kt): fixed ids plus continuation lane ids; Discover lane ids are unique by construction (services and genre names are distinct-by-key before they become query ids)
Discover outer lanes (DiscoverScreen.kt): ids as above (pk-movie, pk-tv, pk-w-<seed>, svc, hidden, catalogue ids)
Discover lane posters (DiscoverShelf.kt): item.key is type:tmdb; every source is already de-duplicated (projectDiscoverLanes adds each key to a seen-set, catalogue lanes use distinctBy { it.key }, hidden uses distinctBy)
Discover service entries: distinctBy { it.id } in the provider
Discover hero: not a lazy list (a slide index over heroes, crossfade keyed by title)
Library letter rail keys (CardGrid.kt upstream pattern is not Wolfskin UI) and Library header/toolbar/tail: fixed strings
Settings list: static group constants
Upstream Wholphin screens (app/src/main/java/com/github/damontecres/wholphin, e.g. TabRow, ViewOptionsDialog): not part of the Wolfskin UI
Who's watching picker: forEachIndexed Row, not a lazy list

Tests

DuplicateLazyKeysTest (wolfskin-design, Robolectric compose): composes each fixed list with duplicate ids and asserts it does not throw, plus a unit test of uniqueKeys. Without the fix 7 of the 8 original tests fail with IllegalArgumentException (cast, related, episodes, season chips, filmography, home shelf, library grid; requests test also fails without its fix); with the fix all pass. Also passed with the change: RequestsKeysTest, LibraryKeysTest, HomeKeysTest. Not run, by instruction: full make test, make apk-debug, device.
No visible UI change for any list without repeats; no frame proofs.

## Crash Justin's crash report (ACRA upload from the installed round 2 Opt build 1.1.0-184-gbf2efcfd, Xiaomi MiTV-AFMU0, Android 14, armeabi-v7a), destination `title/079c13d9-7e77-d32c-07f3-eaea68a76001`: `java.lang.IllegalArgumentException: Key "cebe8282-34dd-72da-218f-5d15ad4ebab4" was already used. If you are using LazyColumn/Row please make sure you provide a unique key for each item.` thrown in a Compose lazy-list measure pass on the main thread, no app frames (R8). ## Root cause The key is a UUID, so it is a Jellyfin item/person id (not a catalogue key like `movie:123`). On a held title page the only Wolfskin list keyed by a Jellyfin UUID that can repeat is the Cast row: `TitleShelf.kt` `TitleCast` used `key = person.id`, and `TitleReadFacade.build` (`TitleReadFacade.kt:200`) builds the cast from `item.people` filtered to type Actor with `person.id` as the id and no de-duplication. Jellyfin lists a person once per credit, so the same actor appearing twice (two roles, or a duplicated People entry) yields two cast items with the same id. The related shelf and the episode shelf (same file) used the same `item.id` pattern and had the same hazard in principle. I could not query the real item (no credentials), so I cannot confirm which of the three it was; the Cast row is the strongest match (a UUID that is a person id, a held title). All three are fixed, so the crash is closed whichever it was. ## Fix New `LazyKeys.kt`: `List<String>.uniqueKeys()`. The first occurrence of an id keeps its plain key; each repeat gets a stable `id#n`. So focus/scroll restoration by key is unchanged for every list without a repeat, and repeats are kept visible (not dropped) because the focus/column index maps 1:1 onto the list and a person with two roles is legitimately shown for both. Applied by precomputing keys with `remember` and keying by index. ## Audit of every keyed lazy list in the Wolfskin UI Fixed (key could repeat): - Title page: Cast row (`TitleShelf.kt`), More-like-this row and Episodes row (`TitleMediaShelf`), Season chips (`TitlePage.kt`) - Person filmography grid (`PersonPage.kt`, row key was `row.first().id`; credits are distinct-by-id after paging, but a first-page repeat or the same title in two rows is cheap to cover) - Home shelves incl. the Continue watching lane (`HomeShelf.kt`): the merged resume + next-up lane can list the same episode from both sources; this is the card key - Library grid rows (`LibraryScreen.kt`) - Requests sheet rows (`ComingInSheet.kt`) Checked and left alone: - Title page outer LazyColumn: fixed row names (header/seasons/episodes/cast/related/end-padding) - Skeleton lists on every screen: index keys - Home outer rows (`HomeScreen.kt`): fixed ids plus continuation lane ids; Discover lane ids are unique by construction (services and genre names are distinct-by-key before they become query ids) - Discover outer lanes (`DiscoverScreen.kt`): ids as above (`pk-movie`, `pk-tv`, `pk-w-<seed>`, `svc`, `hidden`, catalogue ids) - Discover lane posters (`DiscoverShelf.kt`): `item.key` is `type:tmdb`; every source is already de-duplicated (`projectDiscoverLanes` adds each key to a seen-set, catalogue lanes use `distinctBy { it.key }`, hidden uses `distinctBy`) - Discover service entries: `distinctBy { it.id }` in the provider - Discover hero: not a lazy list (a slide index over `heroes`, crossfade keyed by title) - Library letter rail keys (`CardGrid.kt` upstream pattern is not Wolfskin UI) and Library header/toolbar/tail: fixed strings - Settings list: static group constants - Upstream Wholphin screens (`app/src/main/java/com/github/damontecres/wholphin`, e.g. TabRow, ViewOptionsDialog): not part of the Wolfskin UI - Who's watching picker: `forEachIndexed` Row, not a lazy list ## Tests `DuplicateLazyKeysTest` (wolfskin-design, Robolectric compose): composes each fixed list with duplicate ids and asserts it does not throw, plus a unit test of `uniqueKeys`. Without the fix 7 of the 8 original tests fail with IllegalArgumentException (cast, related, episodes, season chips, filmography, home shelf, library grid; requests test also fails without its fix); with the fix all pass. Also passed with the change: RequestsKeysTest, LibraryKeysTest, HomeKeysTest. Not run, by instruction: full `make test`, `make apk-debug`, device. No visible UI change for any list without repeats; no frame proofs.

justinzeus added 1 commit 2026-11-07 20:31:44 +01:00

Make repeated ids safe as lazy list keys across the Wolfskin screens 6f85b9c611

justinzeus added 1 commit 2026-11-07 20:54:55 +01:00

Compute lazy list keys from the list the lazy block renders

review/code-claude Code APPROVE: no blocking finding

Details

review/design-claude Design PASS: no visible change, no blocker

Details

64432b0032

justinzeus commented

2026-11-07 20:56:48 +01:00

Author

Owner

Delta review of the duplicate-lazy-key PR at head 64432b0032049fde109baa628514190cdf691b42.

Code verdict: APPROVE (review/code-claude = success).
Design verdict: PASS (review/design-claude = success). No blocking finding.

What I checked:

uniqueKeys() keeps the first occurrence's plain key and gives each repeat a stable id#n. The output is always as long as the input, and a literal a#1 cannot collide (unit test: [a, b, a#1, a#2, a#1#1]).
In every touched list the keys are computed inside the lazy block from the same captured list that block renders, with no remember: cast, related and episodes (TitleShelf), season chips (TitlePage), filmography (PersonPage), the merged Continue watching lane (HomeShelf), Library grid rows (LibraryScreen, rowCount and rowKeys derived together) and the requests sheet (ComingInSheet). A key list can no longer be shorter than the item count, which was the bug in the earlier head 6f85b9c6.
Lists without repeats keep exactly the old keys (same ids, grid-, season-N), so key-based focus and scroll restoration is unchanged. Repeats stay visible, so index-based focus maps still line up. There is no UI, token or motion change, and generate_tokens.py --check passes.
I spot-checked the "left alone" claims. Discover lanes, services and catalogue are de-duplicated upstream (DiscoverLaneProjection, ConductarrCatalogue, ConductarrProvider).
The nine tests in DuplicateLazyKeysTest compose each fixed list with duplicate ids and assert the repeats render. Before the fix, 7 of the original 8 failed with IllegalArgumentException, per the PR.
git merge-tree against the Home PR (origin/fm/tv-r2-home), the Discover PR (origin/fm/tv-rd2-discover) and the trailer PR (origin/fm/tv-trailer-intent) is clean.

Non-blocking notes:

The real crash source is unconfirmed (no credentials to read the item). The Cast row is the strongest match and all plausible lists are fixed.
A repeat's id#n depends on its order among repeats, so reordering duplicates swaps which card keeps which state. This only affects lists that already had duplicates.
The added else { ... } wrappers in TitleShelf leave the inner block at its old indent (cosmetic).
The full suite and the two guard tests that caught the earlier bug (LibrarySpacingRegressionTest, MotionSurfaceTest) are left to the gate.

Full report (delta reviews 3): /root/.treehouse/firstmate-7bab20/2/firstmate/data/round2-review/report.md. Not run: Gradle/make, nothing on the box.

Delta review of the duplicate-lazy-key PR at head `64432b0032049fde109baa628514190cdf691b42`. **Code verdict: APPROVE (`review/code-claude` = success).** **Design verdict: PASS (`review/design-claude` = success).** No blocking finding. **What I checked:** - `uniqueKeys()` keeps the first occurrence's plain key and gives each repeat a stable `id#n`. The output is always as long as the input, and a literal `a#1` cannot collide (unit test: `[a, b, a#1, a#2, a#1#1]`). - In every touched list the keys are computed inside the lazy block from the same captured list that block renders, with no `remember`: cast, related and episodes (`TitleShelf`), season chips (`TitlePage`), filmography (`PersonPage`), the merged Continue watching lane (`HomeShelf`), Library grid rows (`LibraryScreen`, `rowCount` and `rowKeys` derived together) and the requests sheet (`ComingInSheet`). A key list can no longer be shorter than the item count, which was the bug in the earlier head `6f85b9c6`. - Lists without repeats keep exactly the old keys (same ids, `grid-`, `season-N`), so key-based focus and scroll restoration is unchanged. Repeats stay visible, so index-based focus maps still line up. There is no UI, token or motion change, and `generate_tokens.py --check` passes. - I spot-checked the "left alone" claims. Discover lanes, services and catalogue are de-duplicated upstream (`DiscoverLaneProjection`, `ConductarrCatalogue`, `ConductarrProvider`). - The nine tests in `DuplicateLazyKeysTest` compose each fixed list with duplicate ids and assert the repeats render. Before the fix, 7 of the original 8 failed with `IllegalArgumentException`, per the PR. - `git merge-tree` against the Home PR (`origin/fm/tv-r2-home`), the Discover PR (`origin/fm/tv-rd2-discover`) and the trailer PR (`origin/fm/tv-trailer-intent`) is clean. **Non-blocking notes:** 1. The real crash source is unconfirmed (no credentials to read the item). The Cast row is the strongest match and all plausible lists are fixed. 2. A repeat's `id#n` depends on its order among repeats, so reordering duplicates swaps which card keeps which state. This only affects lists that already had duplicates. 3. The added `else { ... }` wrappers in `TitleShelf` leave the inner block at its old indent (cosmetic). 4. The full suite and the two guard tests that caught the earlier bug (`LibrarySpacingRegressionTest`, `MotionSurfaceTest`) are left to the gate. Full report (delta reviews 3): `/root/.treehouse/firstmate-7bab20/2/firstmate/data/round2-review/report.md`. Not run: Gradle/make, nothing on the box.