Keep loaded title refreshes stable and guard request identities #66

Merged

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

2026-11-07 22:22:58 +01:00

justinzeus commented

2026-11-07 22:17:42 +01:00

Owner

Outcome

Keep already-shown title pages out of the first-load skeleton and make missing request identity a recoverable Wolfskin error, not an exception.

Head: 5f759b8fef3a44391b7f2905319adb62fd109f84
Branch: fm/tv-discover-details-flash → fm/tv-round2

Real versus fixture findings

Failure	Fixture / real reachability	Fix
Content → skeleton → content after Details	Fixture repeat loads were excluded from the Ready fast path. `TitleDestination` forces reads on identity/watch/foreground changes, so a repeated fixture read rebuilt both Loading and `TitleState`. Ordinary real catalogue/held refreshes already retain Ready; neither `TitleReadFacade` nor `ConductarrTitlePages` emits a main-page Loading during those refreshes. Real authorization loss also bypassed the fast path and constructed Loading.	Refresh fixture data and saved choices into the existing state. Do not skip the refresh. Keep the real playback revalidation path. When authorization is lost, clear the old signed-in facts directly to Unavailable rather than replaying a first-load skeleton.
`Request needs a catalogue identity`	The fixture projection discarded the positive TMDB ID already present in its JSON; its slug cannot be parsed as a catalogue route. Real catalogue pages and Conductarr-enriched held pages supply the identity. Held Jellyfin pages without TMDB metadata remain SignedOut, so Request/Add is not normally offered. A stale/incomplete requestable page or unsupported root must still be safe.	Preserve fixture TMDB IDs. Project requests only from positive movie/TV root identities, accepting a matching canonical route as fallback. Reject missing/zero/negative/mismatched/orphan-episode/person identities with the existing Wolfskin recovery-toast surface. Never substitute an episode ID for its series.

The exact effect key behind the m3 emulator flash was not recorded. The vulnerable forced-load path is identified from the code; the new native regression drives that same repeat-load path directly. The original observation remains documented in the m3 emulator walk.

The nullable request projection also serves catalogue hide/restore. Its rejected optimistic action is rolled back; the two-line shared TitleState adjustment prevents a false hide receipt after that local rejection. Request projections used for snapshot/hidden synchronization are remembered rather than repeatedly allocated.

Verification

Final focused Docker/Gradle run: 10 passed, no failures/errors/skips. No full suite, APK build, emulator/device access, or infrastructure lifecycle actions.

Touched test class	Passed	Evidence
`DiscoverRefreshMotionTest`	3	Fixture, real catalogue and held header remain Ready at every 16 ms step. Fixture/catalogue action bounds and pixel brightness stay fixed; the held headline stays fixed while playback is revalidated. Fixture saved-watch changes actually arrive; held playback becomes available again after completion.
`ConductarrTitleSurfaceTest`	1	Existing real request-for-Mindel HTTP payload and receipt still work. Revoked access stays Unavailable through twelve native frames, without a skeleton; ratings and Request/Add are removed.
`TitleRequestSafetyTest`	1	Native production `TitleDestination`: Request from deliberately incomplete/stale metadata displays the recovery toast while retaining title focus/content. Rejected hide remains unhidden and produces no false receipt.
`ConductarrTitlePagesTest`	5	Existing catalogue/held/offline/rating contracts plus identity boundaries and series-root-versus-episode request identity.

Command (using the already-provided serial/shared-lock overlay):

bash docker/gradle.sh debug --no-parallel \
  --init-script /root/.treehouse/firstmate-7bab20/2/firstmate/data/tv-picker-test-sync/serial.init.gradle \
  :app:testDefaultDebugUnitTest \
  --tests '*DiscoverRefreshMotionTest' \
  --tests '*ConductarrTitlePagesTest' \
  --tests '*ConductarrTitleSurfaceTest' \
  --tests '*TitleRequestSafetyTest' \
  -x :wolfskin-design:testDebugUnitTest

Native evidence and XML are outside the checkout at:

/root/.treehouse/firstmate-7bab20/2/firstmate/data/tv-rd2-discover/details-flash/

discover-fixture-refresh-strip.png, discover-refresh-strip.png, discover-held-refresh-strip.png: loaded, +16 ms, +96 ms, +192 ms, in reading order.
native/: 43 full-resolution PNGs, including missing-identity recovery and revoked-access frames.
test-counts-final.json and the four JUnit XML files.

The native renderer is SDK 34 at 1920×1080 with deterministic decoded image bitmaps. This proves the rendered state/focus/layout paths, not physical-TV frame timing or live artwork fetching.

Scope

Updated the existing TV specification. No Home rows, title/episode layout, Back handling, updater UI, motion curves or durations changed. The shared design changes are only the recovery message and the locally-rejected-hide receipt condition.

## Outcome Keep already-shown title pages out of the first-load skeleton and make missing request identity a recoverable Wolfskin error, not an exception. Head: `5f759b8fef3a44391b7f2905319adb62fd109f84` Branch: `fm/tv-discover-details-flash` → `fm/tv-round2` ## Real versus fixture findings | Failure | Fixture / real reachability | Fix | |---|---|---| | Content → skeleton → content after Details | Fixture repeat loads were excluded from the Ready fast path. `TitleDestination` forces reads on identity/watch/foreground changes, so a repeated fixture read rebuilt both Loading and `TitleState`. Ordinary real catalogue/held refreshes already retain Ready; neither `TitleReadFacade` nor `ConductarrTitlePages` emits a main-page Loading during those refreshes. Real authorization loss also bypassed the fast path and constructed Loading. | Refresh fixture data and saved choices into the existing state. Do not skip the refresh. Keep the real playback revalidation path. When authorization is lost, clear the old signed-in facts directly to Unavailable rather than replaying a first-load skeleton. | | `Request needs a catalogue identity` | The fixture projection discarded the positive TMDB ID already present in its JSON; its slug cannot be parsed as a catalogue route. Real catalogue pages and Conductarr-enriched held pages supply the identity. Held Jellyfin pages without TMDB metadata remain SignedOut, so Request/Add is not normally offered. A stale/incomplete requestable page or unsupported root must still be safe. | Preserve fixture TMDB IDs. Project requests only from positive movie/TV root identities, accepting a matching canonical route as fallback. Reject missing/zero/negative/mismatched/orphan-episode/person identities with the existing Wolfskin recovery-toast surface. Never substitute an episode ID for its series. | The exact effect key behind the m3 emulator flash was not recorded. The vulnerable forced-load path is identified from the code; the new native regression drives that same repeat-load path directly. The original observation remains documented in [the m3 emulator walk](https://pages.justinvisser.org/wolfskin/tv-round2-walk). The nullable request projection also serves catalogue hide/restore. Its rejected optimistic action is rolled back; the two-line shared `TitleState` adjustment prevents a false hide receipt after that local rejection. Request projections used for snapshot/hidden synchronization are remembered rather than repeatedly allocated. ## Verification Final focused Docker/Gradle run: **10 passed, no failures/errors/skips**. No full suite, APK build, emulator/device access, or infrastructure lifecycle actions. | Touched test class | Passed | Evidence | |---|---:|---| | `DiscoverRefreshMotionTest` | 3 | Fixture, real catalogue and held header remain Ready at every 16 ms step. Fixture/catalogue action bounds and pixel brightness stay fixed; the held headline stays fixed while playback is revalidated. Fixture saved-watch changes actually arrive; held playback becomes available again after completion. | | `ConductarrTitleSurfaceTest` | 1 | Existing real request-for-Mindel HTTP payload and receipt still work. Revoked access stays Unavailable through twelve native frames, without a skeleton; ratings and Request/Add are removed. | | `TitleRequestSafetyTest` | 1 | Native production `TitleDestination`: Request from deliberately incomplete/stale metadata displays the recovery toast while retaining title focus/content. Rejected hide remains unhidden and produces no false receipt. | | `ConductarrTitlePagesTest` | 5 | Existing catalogue/held/offline/rating contracts plus identity boundaries and series-root-versus-episode request identity. | Command (using the already-provided serial/shared-lock overlay): ```sh bash docker/gradle.sh debug --no-parallel \ --init-script /root/.treehouse/firstmate-7bab20/2/firstmate/data/tv-picker-test-sync/serial.init.gradle \ :app:testDefaultDebugUnitTest \ --tests '*DiscoverRefreshMotionTest' \ --tests '*ConductarrTitlePagesTest' \ --tests '*ConductarrTitleSurfaceTest' \ --tests '*TitleRequestSafetyTest' \ -x :wolfskin-design:testDebugUnitTest ``` Native evidence and XML are outside the checkout at: `/root/.treehouse/firstmate-7bab20/2/firstmate/data/tv-rd2-discover/details-flash/` - `discover-fixture-refresh-strip.png`, `discover-refresh-strip.png`, `discover-held-refresh-strip.png`: loaded, +16 ms, +96 ms, +192 ms, in reading order. - `native/`: 43 full-resolution PNGs, including missing-identity recovery and revoked-access frames. - `test-counts-final.json` and the four JUnit XML files. The native renderer is SDK 34 at 1920×1080 with deterministic decoded image bitmaps. This proves the rendered state/focus/layout paths, not physical-TV frame timing or live artwork fetching. ## Scope Updated the existing TV specification. No Home rows, title/episode layout, Back handling, updater UI, motion curves or durations changed. The shared design changes are only the recovery message and the locally-rejected-hide receipt condition.

justinzeus added 1 commit 2026-11-07 22:17:42 +01:00

Keep loaded title refreshes stable and guard request identities

review/code-claude Code APPROVE: no stale data, request guard recovers via toast

Details

review/design-claude Design PASS: no look change beyond flash removal

Details

5f759b8fef

justinzeus commented

2026-11-07 22:21:59 +01:00

Author

Owner

Review of the Discover Details flash and Request guard fix at head 5f759b8fef3a44391b7f2905319adb62fd109f84 (base fm/tv-round2, b51dced5).

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

1. Loaded title stays loaded.

The real signed-in Ready path keeps the existing in-place refresh into the same TitleState.
The debug fixture path now refreshes the same TitleState (guarded by title === fixtureRefresh) instead of rebuilding Loading.
A different route, viewer or person route still builds a fresh empty TitleState, so no stale data for another title or person survives.
Real auth loss starts the new state as Unavailable instead of Loading. See note 1.
Tests assert Ready with identical bounds and brightness at every 16 ms step for the fixture, held and catalogue cases. The revoked case stays Unavailable for twelve frames.

2. Request with incomplete identity.

requestTitle returns null unless the root is a movie or tv with a positive TMDB id (from tmdbId or a matching kind:id route). Episodes, persons, zero, negative, mismatched and malformed ids are rejected, and an episode never substitutes for its series.
A null item sets the Wolfskin recovery toast. No sheet opens and nothing is sent.
A rejected hide rolls back and shows no false receipt. A rejected unhide does not claim "Back in your picks."
Fixture TMDB ids are preserved. The toast reuses TitleNotice and RecoveryToast with the token position, shadow and timing. The native frame shows Can’t update this title right now. top-right with the page and focus unchanged.

3. Looks. There is no layout, token or motion change. The 16, 96 and 192 ms strip is identical to the loaded frame. Tests assert behaviour, not wiring.

4. Merge. A merge-tree check against fm/tv-round2 (b51dced5) is clean. fm/tv-back-title is not on origin yet, so I could not test it. It touches the BackHandler hunk (around line 340) while this PR touches load() (lines 90-130) in the same file. A textual conflict is unlikely [INFERENCE]; re-check once it is pushed.

Non-blocking notes:

For a held title that loses Conductarr auth, the reset runs Ready -> Unavailable -> saved copy -> Ready. The user briefly sees "This title can’t be read ... no saved copy of the page yet", which is wrong for a held page that has one. It also cannot tell an auth blip from a sign-out. Keeping the held data and only demoting the connection facts would avoid that. [INFERENCE on the duration]
The PR says the exact m3 flash trigger was not recorded. The fixture fix is BuildConfig.DEBUG-only, so the Opt build only gets the auth-loss and request-guard changes. Re-test Discover hero -> Details on a non-debug build in the next walk.
A forced reload of an already-Unavailable page now keeps the error panel without the skeleton. Acceptable.
The hide rollback relies on the Conductarr action running synchronously inside TitleState.handle, as it does today.
Native evidence is Robolectric SDK 34 at 1920x1080, with no live artwork or TV frame timing.

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

Review of the Discover Details flash and Request guard fix at head `5f759b8fef3a44391b7f2905319adb62fd109f84` (base `fm/tv-round2`, `b51dced5`). **Code verdict: APPROVE (`review/code-claude` = success).** **Design verdict: PASS (`review/design-claude` = success).** No blocking finding. **1. Loaded title stays loaded.** - The real signed-in Ready path keeps the existing in-place refresh into the same `TitleState`. - The debug fixture path now refreshes the same `TitleState` (guarded by `title === fixtureRefresh`) instead of rebuilding Loading. - A different route, viewer or person route still builds a fresh empty `TitleState`, so no stale data for another title or person survives. - Real auth loss starts the new state as `Unavailable` instead of `Loading`. See note 1. - Tests assert `Ready` with identical bounds and brightness at every 16 ms step for the fixture, held and catalogue cases. The revoked case stays `Unavailable` for twelve frames. **2. Request with incomplete identity.** - `requestTitle` returns null unless the root is a movie or tv with a positive TMDB id (from `tmdbId` or a matching `kind:id` route). Episodes, persons, zero, negative, mismatched and malformed ids are rejected, and an episode never substitutes for its series. - A null item sets the Wolfskin recovery toast. No sheet opens and nothing is sent. - A rejected hide rolls back and shows no false receipt. A rejected unhide does not claim "Back in your picks." - Fixture TMDB ids are preserved. The toast reuses `TitleNotice` and `RecoveryToast` with the token position, shadow and timing. The native frame shows `Can’t update this title right now.` top-right with the page and focus unchanged. **3. Looks.** There is no layout, token or motion change. The 16, 96 and 192 ms strip is identical to the loaded frame. Tests assert behaviour, not wiring. **4. Merge.** A merge-tree check against `fm/tv-round2` (`b51dced5`) is clean. `fm/tv-back-title` is not on origin yet, so I could not test it. It touches the `BackHandler` hunk (around line 340) while this PR touches `load()` (lines 90-130) in the same file. A textual conflict is unlikely [INFERENCE]; re-check once it is pushed. **Non-blocking notes:** 1. For a held title that loses Conductarr auth, the reset runs Ready -> Unavailable -> saved copy -> Ready. The user briefly sees "This title can’t be read ... no saved copy of the page yet", which is wrong for a held page that has one. It also cannot tell an auth blip from a sign-out. Keeping the held data and only demoting the connection facts would avoid that. [INFERENCE on the duration] 2. The PR says the exact m3 flash trigger was not recorded. The fixture fix is `BuildConfig.DEBUG`-only, so the Opt build only gets the auth-loss and request-guard changes. Re-test Discover hero -> Details on a non-debug build in the next walk. 3. A forced reload of an already-Unavailable page now keeps the error panel without the skeleton. Acceptable. 4. The hide rollback relies on the Conductarr action running synchronously inside `TitleState.handle`, as it does today. 5. Native evidence is Robolectric SDK 34 at 1920x1080, with no live artwork or TV frame timing. Full report (review 5): `/root/.treehouse/firstmate-7bab20/2/firstmate/data/round2-review/report.md`. Not run: Gradle/make, nothing on the box.