justinzeus/wolfskin

Fork 0

Remove the competing Discover title preview #72

Merged

justinzeus merged 1 commit from fm/tv-details-flash-2 into fm/tv-round2

2026-11-08 01:01:38 +01:00

justinzeus commented

2026-11-08 00:47:52 +01:00

Owner

Outcome

Remove the remaining Discover Details content → skeleton → content flash. This is a shared production path, not a fixture-only loader issue.

Cause and change

The candidate recording at https://pages.justinvisser.org/wolfskin/tv-round2-walk#cand2 shows the old Discover preview before the skeleton: its distinctive controls are Request / For someone else / Not interested, with no shared-title kicker. DiscoverState.openTitle synchronously selected a title; DiscoverScreen immediately painted DiscoverTitleDestination before navigation opened the shared TitleDestination and its first Loading state. That branch had no fixture gate, so real catalogue titles, including titles later resolved to held Jellyfin identities, could take it too.

Delete that obsolete renderer and its branch. The outgoing entry stays Discover until navigation replaces it; the shared title destination alone owns loading and content. Existing same-title Ready refresh guards and authorization-loss handling remain unchanged. Update title/navigation documentation.

Verification

Native regression failed before removal when the outgoing hero lost its Details button to the preview. It now passes. The existing shared-title async cast/related/Back case also passes.
Catalogue, held and fixture refresh cases assert Ready on every rendered refresh composition and sampled frame, stationary/opaque actions, and identical before/after full-frame bitmaps. Catalogue refresh also covers loss of the read cache before the fresh reply.
Exact Back scroll offset and selected-card bounds pass through real NavDisplay and its entry decorators on SDK 34 and 36, both animated and reduced motion, using the production Home/title loaders with fixture data. Mixed poster/tile/wide Home viewport coverage also passes.

Final focused app run: 9 cases, zero failures/errors:

./docker/gradle.sh test :app:testDefaultDebugUnitTest -x :wolfskin-design:testDebugUnitTest --tests '*DiscoverTitleTimingTest' --tests '*DiscoverRefreshMotionTest' --tests '*TitleEntryBackTest.*ExactPreOpenViewport*'

Design viewport run:

./docker/gradle.sh test :wolfskin-design:testDebugUnitTest --tests '*HomeViewportTest'

Native proof and XML: /root/.treehouse/firstmate-7bab20/2/firstmate/data/tv-details-flash-2/verification.md and sibling captures. Refresh artwork is static/injected for deterministic frame comparison. No APK build, TV/emulator operation, or live authenticated Conductarr/Jellyfin walk. Initial app invocations pulled the existing transitive design-test dependency; the final app command explicitly excludes it.

Remaining watch: 72 px after Back

The supplied emulator screenshots move Continue watching from y=168 to y=240; that observation is real. It does not reproduce in the native actual-navigation cases above, which retain the exact scroll and card bounds. No speculative viewport correction is included. The screenshots lack scroller offsets, viewport constraints and activation-time measurements, so their cause remains unestablished. An instrumented capture by the emulator owner is needed to explain this remaining watch; this PR does not claim to close it.

AI assistance was used for investigation, implementation and tests.

## Outcome Remove the remaining Discover Details content → skeleton → content flash. This is a shared production path, not a fixture-only loader issue. ## Cause and change The candidate recording at https://pages.justinvisser.org/wolfskin/tv-round2-walk#cand2 shows the old Discover preview before the skeleton: its distinctive controls are Request / For someone else / Not interested, with no shared-title kicker. `DiscoverState.openTitle` synchronously selected a title; `DiscoverScreen` immediately painted `DiscoverTitleDestination` before navigation opened the shared `TitleDestination` and its first Loading state. That branch had no fixture gate, so real catalogue titles, including titles later resolved to held Jellyfin identities, could take it too. Delete that obsolete renderer and its branch. The outgoing entry stays Discover until navigation replaces it; the shared title destination alone owns loading and content. Existing same-title Ready refresh guards and authorization-loss handling remain unchanged. Update title/navigation documentation. ## Verification - Native regression failed before removal when the outgoing hero lost its Details button to the preview. It now passes. The existing shared-title async cast/related/Back case also passes. - Catalogue, held and fixture refresh cases assert Ready on every rendered refresh composition and sampled frame, stationary/opaque actions, and identical before/after full-frame bitmaps. Catalogue refresh also covers loss of the read cache before the fresh reply. - Exact Back scroll offset and selected-card bounds pass through real NavDisplay and its entry decorators on SDK 34 and 36, both animated and reduced motion, using the production Home/title loaders with fixture data. Mixed poster/tile/wide Home viewport coverage also passes. Final focused app run: 9 cases, zero failures/errors: ``` ./docker/gradle.sh test :app:testDefaultDebugUnitTest -x :wolfskin-design:testDebugUnitTest --tests '*DiscoverTitleTimingTest' --tests '*DiscoverRefreshMotionTest' --tests '*TitleEntryBackTest.*ExactPreOpenViewport*' ``` Design viewport run: ``` ./docker/gradle.sh test :wolfskin-design:testDebugUnitTest --tests '*HomeViewportTest' ``` Native proof and XML: `/root/.treehouse/firstmate-7bab20/2/firstmate/data/tv-details-flash-2/verification.md` and sibling captures. Refresh artwork is static/injected for deterministic frame comparison. No APK build, TV/emulator operation, or live authenticated Conductarr/Jellyfin walk. Initial app invocations pulled the existing transitive design-test dependency; the final app command explicitly excludes it. ## Remaining watch: 72 px after Back The supplied emulator screenshots move Continue watching from y=168 to y=240; that observation is real. It does not reproduce in the native actual-navigation cases above, which retain the exact scroll and card bounds. No speculative viewport correction is included. The screenshots lack scroller offsets, viewport constraints and activation-time measurements, so their cause remains unestablished. An instrumented capture by the emulator owner is needed to explain this remaining watch; this PR does not claim to close it. AI assistance was used for investigation, implementation and tests.

justinzeus added 1 commit 2026-11-08 00:47:53 +01:00

Remove the competing Discover title preview

review/code-claude Code APPROVE: preview removed, shared title owns loading

Details

review/design-claude Design PASS: outgoing hero pixel-identical, no approved pixels change

Details

c80afb98a2

justinzeus commented

2026-11-08 00:56:29 +01:00

Author

Owner

Verification note: the additional loader-only re-run (*TitleEntryBackTest.*ExactPreOpenViewport*) exhausted its 20-minute wall deadline after waiting for the shared Gradle lock and reaching unit-test resource packaging; it did not produce another test result. The preceding focused run remains the verification result: nine passing native cases, including all four SDK 34/36 normal/reduced-motion viewport cases, with its XML and frames preserved in the proof directory. No source changes followed that passing run; only the documentation/commit metadata changed. The remaining 72 px emulator watch is still explicitly unclaimed.

Verification note: the additional loader-only re-run (`*TitleEntryBackTest.*ExactPreOpenViewport*`) exhausted its 20-minute wall deadline after waiting for the shared Gradle lock and reaching unit-test resource packaging; it did not produce another test result. The preceding focused run remains the verification result: nine passing native cases, including all four SDK 34/36 normal/reduced-motion viewport cases, with its XML and frames preserved in the proof directory. No source changes followed that passing run; only the documentation/commit metadata changed. The remaining 72 px emulator watch is still explicitly unclaimed.

justinzeus commented

2026-11-08 01:00:45 +01:00

Author

Owner

Review of the Details-flash follow-up at head c80afb98a20141fab6f58e869da2e318ec9e8eb2 (base fm/tv-round2).

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

The fix is at the shown cause.

The production change deletes DiscoverTitleDestination (the Request / For someone else / Not interested stub) and the one DiscoverScreen line that painted it as soon as openTitle set title.
That matches the m3 recording: stub controls at 1.276 s, shared skeleton at 1.325 s, ready title at 1.563 s. It was not fixture-gated, so real titles flashed too.
Nothing else referenced the file or its test tag, so the build cannot break.

No dependence on the stub.

openTitle still sets title and pushes title/... synchronously.
The Discover entry goes inactive on push, so it no longer receives keys. title is cleared when the route returns to discover.
In production, requests and hides happen on the shared title page.

Earlier work intact.

Back from PR 67 and the PR 66 held-title fast path are untouched.
The refresh tests are extended: catalogue read-cache loss before the fresh reply, and Ready on every rendered composition.

No approved pixels change. The outgoing hero frame before and after openTitle is pixel-identical (0 pixels differ by more than 8, checked by me). Details stays and the old stub controls never appear.

Tests.

The new DiscoverTitleTimingTest case failed before the fix and passes after.
The refresh and Home Back viewport tests are regression guards that pass both before and after.
The Home Back viewport test runs through real NavDisplay and decorators on sdk 34 and 36, animated and reduced motion. It asserts the exact scroll offset and card bounds. It does not cover Discover's own Back.

Merge. A merge-tree check against current fm/tv-round2 (60bc59c8) is clean.

72 px Back watch. The PR leaves it open. I agree that no speculative viewport correction should be added. The instrumented capture it requests (first visible index, offset, viewport and route at open, deactivation, reactivation and settle) is the right next step, and the emulator owner has to run it.

Non-blocking notes:

DiscoverState.title, titleButton and titleKey now have no production UI. DiscoverKeysTest and DiscoverCaptureTest still exercise them, and the request and hidden-receipt captures (for example st-058 and st-059) now sit over the Discover hero instead of the stub. A follow-up could delete the dead state or re-home those tests.
While the outgoing Discover entry is retained, DiscoverState still reads the route-scoped shell.view. For a lane card (not the hero), the outgoing page may show a hero focus ring for the length of the push animation. The hero Details case is pixel-stable, but that test uses a single shell without NavDisplay. [INFERENCE] Check it on the next emulator walk, and use screenView(shell) if it flashes.
The hero before/after test only asserts that text is present. A sameAs bitmap comparison would be stronger. I checked by hand that the frames are identical.
The lane's supplementary re-run timed out on the shared Gradle lock, so the nine-case result stands for the committed source.

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

Review of the Details-flash follow-up at head `c80afb98a20141fab6f58e869da2e318ec9e8eb2` (base `fm/tv-round2`). **Code verdict: APPROVE (`review/code-claude` = success).** **Design verdict: PASS (`review/design-claude` = success).** No blocking finding. **The fix is at the shown cause.** - The production change deletes `DiscoverTitleDestination` (the Request / For someone else / Not interested stub) and the one `DiscoverScreen` line that painted it as soon as `openTitle` set `title`. - That matches the m3 recording: stub controls at 1.276 s, shared skeleton at 1.325 s, ready title at 1.563 s. It was not fixture-gated, so real titles flashed too. - Nothing else referenced the file or its test tag, so the build cannot break. **No dependence on the stub.** - `openTitle` still sets `title` and pushes `title/...` synchronously. - The Discover entry goes inactive on push, so it no longer receives keys. `title` is cleared when the route returns to `discover`. - In production, requests and hides happen on the shared title page. **Earlier work intact.** - Back from PR 67 and the PR 66 held-title fast path are untouched. - The refresh tests are extended: catalogue read-cache loss before the fresh reply, and `Ready` on every rendered composition. **No approved pixels change.** The outgoing hero frame before and after `openTitle` is pixel-identical (0 pixels differ by more than 8, checked by me). Details stays and the old stub controls never appear. **Tests.** - The new `DiscoverTitleTimingTest` case failed before the fix and passes after. - The refresh and Home Back viewport tests are regression guards that pass both before and after. - The Home Back viewport test runs through real `NavDisplay` and decorators on sdk 34 and 36, animated and reduced motion. It asserts the exact scroll offset and card bounds. It does not cover Discover's own Back. **Merge.** A merge-tree check against current `fm/tv-round2` (`60bc59c8`) is clean. **72 px Back watch.** The PR leaves it open. I agree that no speculative viewport correction should be added. The instrumented capture it requests (first visible index, offset, viewport and route at open, deactivation, reactivation and settle) is the right next step, and the emulator owner has to run it. **Non-blocking notes:** 1. `DiscoverState.title`, `titleButton` and `titleKey` now have no production UI. `DiscoverKeysTest` and `DiscoverCaptureTest` still exercise them, and the request and hidden-receipt captures (for example st-058 and st-059) now sit over the Discover hero instead of the stub. A follow-up could delete the dead state or re-home those tests. 2. While the outgoing Discover entry is retained, `DiscoverState` still reads the route-scoped `shell.view`. For a lane card (not the hero), the outgoing page may show a hero focus ring for the length of the push animation. The hero Details case is pixel-stable, but that test uses a single shell without `NavDisplay`. [INFERENCE] Check it on the next emulator walk, and use `screenView(shell)` if it flashes. 3. The hero before/after test only asserts that text is present. A `sameAs` bitmap comparison would be stronger. I checked by hand that the frames are identical. 4. The lane's supplementary re-run timed out on the shared Gradle lock, so the nine-case result stands for the committed source. Full report (review 10): `/root/.treehouse/firstmate-7bab20/2/firstmate/data/round2-review/report.md`. Not run: Gradle/make, nothing on the box.