Add FF1 artwork render status reporting

[ppupha]

Problem

Expose FF1 artwork render status through the player status response, keep it correct across artwork transitions and same-ID source refreshes, and define the artwork-source validation contract used by display, schedule, and refresh commands.

Why It Matters

Controllers need reliable pending/loading/ready/failed state without persisting stale lifecycle values. Device-local and remote DP1 playlists may legitimately use relative sources, while known non-web sources should be rejected before they replace playback state.

Acceptance Checks

• Status replies expose the current render lifecycle and clear stale values on recovery/source changes.
• Artwork source validation accepts http(s), data, relative, and protocol-relative sources; blank, malformed, and non-web schemes return { ok: false } before committing playback or a schedule.
• npm run post-implement-check and npm run verify pass locally.
• Focused render-status, visual-settings, HLS, Now Display, and item-commit tests pass.
• Manual player-route smoke: slow load, render failure, same-ID source refresh, and showRenderLoadingOverlay: false (pending device/browser route execution).
• Follow-up: ffos-user must add the typed PlayerStatus.renderStatus bridge and consumer round-trip coverage before this becomes end-to-end visible in the app.

Human Owner

PuPha / FF1 playback owner.

How The Agent Will Be Used

The agent reconciled main with the render-status branch, added source-contract documentation, ran lint/type-check/unit/build verification, and performed a separate full-diff review. Manual device-route smoke and the downstream ffos-user bridge remain human-owned coordination work.

PR or Deploy Link

#249

[feralfile-bot]

Reviewers: 3 (approve: 0, comment: 0, request-changes: 3).

Critical issues

• src/services/CanvasService.ts:223 / src/app/playlist/playlist-client.tsx:480: setRenderStatus() broadcasts a new full castInfo, so every render lifecycle update re-enters the playlist command switch. For displayPlaylist, this rebuilds playlist state and reapplies currentIndex, which can clear and reschedule timers. Render status observation should not behave like a fresh playback command, or consumers need to ignore status-only cast-info updates.

• src/services/CanvasService.ts:532, src/services/CanvasService.ts:291, src/services/CanvasService.ts:741: index-changing paths keep the previous artwork’s renderStatus. A remote can move from a ready artwork to a new item and immediately get index: <new> with renderStatus: ready until React catches up. Reset to pending when the active artwork changes, including move/update/deferred refresh/shuffle promotion paths.

• src/components/artwork-player/ArtworkPlayer.tsx:387: the same-URL slot handoff guard was removed. Adjacent playlist items can share previewURL but differ by itemIdentity; a late load from the outgoing slot can now rebind incomingSlotRef and steal readiness from the actual incoming slot. Please restore the guard or key readiness by slot identity, not URL alone.

• src/components/artwork-player/ArtworkPlayer.tsx:400 / src/components/artwork-player/ArtworkPlayer.tsx:798: render failures are accepted from any slot without checking that the slot still represents the current previewURL/identity. During a transition, an outgoing slot error can mark the new artwork failed and show the modal; later success sets status ready but does not clear the modal. The failure path needs the same stale-slot guard as successful load handling.

Medium issues

• src/services/CanvasService.ts:63: source protocol rejection is a new cast/DP1 command-contract change, and this concern recurred in two reports. The PR now rejects whole Now Display/refresh commands for anything outside http:, https:, and data: instead of accepting the playlist and reporting render failure. If this belongs here, document the accepted schemes and add compatibility coverage for production DP1/cast source shapes; otherwise split it out.

Missing coverage / handoff

• Add coverage for render status across control paths: moveToArtwork, updateIndex, queued shuffle/refresh promotion, and deferred refresh consumption.

• Add consumer-path coverage showing status-only updates do not cause PlaylistClient to reprocess playback commands or reschedule timers.

• Add same-URL/identity transition tests and stale outgoing media-error tests, including video or HLS.

• The PR validation lists Go commands for components/feral-controld, but this PR changes TypeScript/React player code. Please provide the repo-required npm run post-implement-check and npm run verify results, or explain why they could not be run.

Skipped as not worth raising actively: the unused hook dependencies cleanup is minor. There were no PR discussion comments indicating any of the active issues were already deferred or tracked elsewhere.

[feralfile-bot]

Dependency impact check for PR #249.

Summary: ff-player now rejects playlist items whose source is not http:, https:, or data:, and ff-app still serializes about:blank into its FF1 cast JSON path.

Required downstream updates:

• feral-file/ff-app: update lib/infra/database/converters.dart::playlistItemToDP1PlaylistItem / playlistItemsToDP1PlaylistItems so FF1 cast payloads do not carry about:blank or other unsupported sources, and keep the CanvasClientServiceV2.castPlaylist(..., usingUrl: false) paths in lib/ui/screens/playlist_detail_screen.dart and lib/ui/screens/work_detail_screen.dart aligned with that sanitization.

Evidence:

• ff-app defines dp1FallbackSourceUri = 'about:blank' in lib/domain/models/dp1/dp1_playlist_item.dart:12-14.
• ff-app copies item.source directly into the DP1 wire object in lib/infra/database/converters.dart:471-506.
• The FF1 cast entrypoints explicitly send the JSON playlist path with usingUrl: false in lib/ui/screens/playlist_detail_screen.dart:158-167 and lib/ui/screens/work_detail_screen.dart:302-310.
• ff-player now hard-fails displayPlaylist and refreshPlaylist when any item source protocol is not http:, https:, or data: in src/services/CanvasService.ts:63-83, src/services/CanvasService.ts:674-687, and src/services/CanvasService.ts:814-823.

[feralfile-bot]

Automated human review request for PR #249.

Risk zone: yellow
Responsibility area: FF1 runtime/device status contract
Why this needs human review: Additive but cross-surface contract change: the player now emits a new render-status lifecycle through the device status path and gates visible loading behavior from it. That is still within existing patterns, but it is a narrow runtime boundary that benefits from steward review.

This PR needs targeted human review on the scoped areas below, not a full second pass of the whole PR.

• @lpopo0856: please review renderStatus propagation in CanvasService.getStatus() / setRenderStatus() and ArtworkPlayer loading-overlay state transitions. Context: FF1 runtime/device stewardship (Brandon Yeh).

[ppupha]

The player should reject invalid item source URLs at the playlist/item validation layer. It only needs to validate the playlist and its items; if an item's source URL is invalid, the playlist load should fail.

[feralfile-bot]

Reviewers: 3 (approve: 0, comment: 0, request-changes: 3).

Critical Issues

1. renderStatus can be stale for the newly selected artwork during index-only transitions. moveToArtwork, playlist updateIndex, and deferred playlist promotion update index through setCastInfo() without resetting renderStatus, while setCastInfo() preserves the previous status via castInfo.renderStatus ?? this.renderStatus (src/services/CanvasService.ts:207-224, src/services/CanvasService.ts:299-311, src/services/CanvasService.ts:540-550, src/app/playlist/playlist-client.tsx:241-245). A status poll between that index change and ArtworkPlayer’s later markArtworkPending() can report the new item with the prior item’s ready/failed state. This same stale-status concern recurred across multiple reports and is central to the new reporting contract.

2. renderStatus is persisted with cast info and can be reported after recovery as if it describes the current page render. setRenderStatus() writes the status into castInfo and notifies persistence (src/services/CanvasService.ts:231-241), useCastInfo stores that object unchanged (src/services/custom-hooks/useCastInfo.ts:50-59), and recovery restores it into CanvasService before the artwork has mounted and reported a live lifecycle (src/context/AppContext.tsx:176-178, src/services/CanvasService.ts:454-462). Please strip renderStatus from persisted cast info or reset recovered render status to pending/undefined until the current artwork reports.

3. The new source preflight rejects URL forms the existing player path can resolve, such as relative and protocol-relative sources. isSupportedArtworkSource() uses new URL(normalizedSource) without a base (src/services/CanvasService.ts:70-83), but media loading resolves with new URL(url, window.location.href) and then assigns the original source to the element (src/utils/mediaLoader.ts:53-74, src/utils/mediaLoader.ts:184-195). This newly narrows the DP1/cast contract beyond the render-status change and can return { ok: false } while leaving the previous artwork on screen. The discussion clarified that invalid item URLs should fail, and I am not re-raising the about:blank downstream compatibility item as an active blocker; this finding is specifically about browser-supported relative/protocol-relative sources.

Missing Tests / Validation

• Please add coverage for render status reset semantics across moveToArtwork, timer/source-end updateIndex, deferred playlist promotion, and persisted recovery containing stored ready/failed.
• The PR validation currently lists only Go commands for components/feral-controld; this PR changes TypeScript/React playback, cast status, and remote config behavior, so the handoff should include the repo’s JS verification path such as npm run verify and any relevant manual smoke evidence for the playback-facing status flow.

[feralfile-bot]

Reviewers: 3 (approve: 0, comment: 2, request-changes: 1).

Critical Issues

• src/services/CanvasService.ts:103 still preserves stale renderStatus when a refresh replaces the current item's source but keeps the same item id. Two reviewers raised this, and re-review confirms the path: refreshPlaylist spreads prior into the next cast info, resolveRenderStatusForCastInfo compares only selected item ids, and then returns the previous ready/failed status. The playlist route will reload/render the changed source, so an immediate status poll can report ready for media that has not rendered yet. The reset check should include render-affecting identity, at minimum the selected item source, and there should be CanvasService coverage for same id/index with a changed source.

Medium Issues

• src/services/CanvasService.ts:103 also compares raw indices while the playlist route normalizes accepted positive indices. moveToArtwork({ index: 2 }) is accepted for a 2-item playlist, and the route displays index 0, but the resolver reads nextItems.at(2) as undefined and can preserve the previous ready/failed status instead of resetting to pending. Normalize the compared indices consistently with PlaylistClient, or reject positive out-of-range indices before committing cast state. Add CanvasService coverage for this accepted normalized-index case.

Missing Tests / Validation

• The PR validation only records Go commands under components/feral-controld, but this PR changes TypeScript/React player status and artwork-rendering behavior. Please record relevant FF Player verification, at minimum npm run verify, plus the targeted render-status tests that cover the changed status contract.

Considered But Not Re-Raised

• The discussion already called out the new rejection of unsupported artwork source protocols and the author clarified that invalid item sources should fail playlist validation. I’m not raising that as an active finding here, assuming the downstream ff-app alignment noted in the thread is handled outside this PR.

[feralfile-bot]

Dependency impact check for PR #249.

Summary: The player now rejects non-http(s)/data: artwork sources before accepting a playlist, so ff-app must stop sending raw non-web DP1 item sources into player-bound casts.

Required downstream updates:

• feral-file/ff-app: normalize or filter player-bound DP1 item sources in lib/infra/database/converters.dart::playlistItemToDP1PlaylistItem and lib/infra/services/dp1_playlist_resolver_service.dart::_normalizeResolverItem before they reach DatabaseConverters.playlistAndItemsToDP1Playlist(...).

Evidence:

• PR Add FF1 artwork render status reporting #249 adds isSupportedArtworkSource() / findInvalidArtworkSource() and makes CanvasService.nowDisplayPlaylist() and CanvasService.refreshPlaylist() return { ok: false } for unsupported sources instead of accepting them.
• In ff-app, DatabaseConverters.playlistItemToDP1PlaylistItem() copies item.source directly into the DP1 wire item, and Dp1PlaylistResolverService._normalizeResolverItem() also forwards item.source unchanged.
• ff-app integration test test/integration/infra/services/url_dp1_playlist_resolver_flow_integration_test.dart persists a resolved playlist item with source: 'tezos:KT1U6EHmNxJTkvaWJ4ThczG4FSDaHC21ssvi:824876', which would now be rejected by the player unless normalized upstream.

[feralfile-bot]

Reviewers: 3 (approve: 0, comment: 1, request-changes: 2).

Critical issues

• src/services/CanvasService.ts:103-130 resets renderStatus based only on selected item id. All reviewers raised this in some form. A refreshPlaylist can keep the same active item id while changing its source, and refreshPlaylist commits the new playlist at src/services/CanvasService.ts:979-984. During that window, getStatus() can report the new source with the old ready/failed lifecycle until ArtworkPlayer later marks it pending. Please treat render-relevant identity as at least id + source, and add service coverage for same-id/different-source refresh.

• src/components/artwork-player/ArtworkPlayer.tsx:1163-1216 can leave status stuck at failed after WebGL recovery. handleWebGLLost() publishes failed, recovery reloads the active iframe, and the normal ready path reaches markArtworkReady() via loadedSource() / slot-ready handling, but markArtworkReady() now returns early for any current failed status at src/components/artwork-player/ArtworkPlayer.tsx:239-247. That guard seems intended for model-viewer failure ordering, but it also suppresses successful WebGL recovery from publishing ready. Please scope the suppression or explicitly publish pending/ready during recovery, with a regression test.

Missing tests / validation

• Add tests for the two cases above: selected item same id but changed source, and WebGL loss followed by successful iframe recovery returning status to ready.

• The PR validation currently lists only Go commands under components/feral-controld. This PR changes React playback/status behavior in ff-player, so the repo-required npm run post-implement-check and npm run verify evidence is still missing.

Considered but not re-raised

• Reviewers flagged the new http(s)/data: source restriction as a cast contract change. The PR discussion explicitly confirms invalid item sources should be rejected at playlist/item validation, and the dependency impact comments already call out required ff-app normalization work, so I’m not treating that as an active blocker here beyond ensuring the downstream follow-up remains visible.

[feralfile-bot]

Reviewers: 3 (approve: 0, comment: 2, request-changes: 1).

Critical issues

• src/components/artwork-player/ArtworkPlayer.tsx:239 prevents every failed -> ready transition. That protects the model-viewer error path, but it also breaks WebGL recovery: handleWebGLLost() publishes failed, startWebGLRecovery() later calls reloadIframe(), and the iframe onLoad path can recover visually while markArtworkReady() returns early. This leaves getStatus().renderStatus stuck at failed after a successful recovery, which defeats the new status contract. This should be scoped to the model-viewer failure ordering case instead of blocking all failed-to-ready recovery paths.

Medium issues

• src/services/CanvasService.ts:103-130 only compares raw previous/next item ids when deciding whether to reset renderStatus to pending. Two reviewers independently flagged this, and re-review confirms it is valid. A refreshPlaylist can replace the current item’s source while reusing the same DP1 id; CanvasService then preserves the previous ready/failed status even though src/app/playlist/playlist-client.tsx:446-450 will render the new source. The same raw .at(index) comparison also misses accepted positive out-of-range indices from moveToArtwork, while the playlist client normalizes those before display. Please compare the effective playback identity, including source where needed, using the same normalization rules as the player.

Missing tests

• Add coverage for WebGL recovery after a failed status: WebGL loss publishes failed, recovery reloads the current iframe, and the recovered artwork can publish pending/loading and finally ready.

• Add CanvasService or PlaylistClient coverage for a refresh where the current item keeps the same id but changes source, asserting status resets to pending before the new artwork reports ready. Also cover positive out-of-range moveToArtwork if wrapped indices remain accepted behavior.

Notes considered

• The repeated dependency-impact comments about rejecting non-http(s)/data: sources appear intentional for this PR, and the author explicitly confirmed invalid item sources should fail playlist load. I am not raising that as an active ff-player finding here; the downstream ff-app normalization work should be handled separately.

• The PR validation currently lists Go checks under components/feral-controld, but this PR changes the TS/React player. Before merge, the handoff should include the repo-required npm run post-implement-check and npm run verify evidence.

[ppupha]

Review follow-up completed in this branch:

Fixed here:

• Reconciled origin/main with render-status handling, preserving transition and boot-recovery behavior.
• Failed incoming artwork (iframe/object/HLS included) now resolves through the transition pipeline before reporting failed, so the previous artwork and visual settings do not remain latched.
• Restored AppContext boot-recovery coverage and documented the cast/schedule source contract: accept http(s), data, relative, and protocol-relative sources; reject blank, malformed, and non-web schemes before committing state.
• Updated the PR description to the repository template.

Out of scope:

• The ffos-user typed PlayerStatus.renderStatus bridge and consumer round-trip test. This PR owns the player-side producer and status response; the remaining work is in a separately shipped downstream repository and needs its owner/coordinated rollout. It is recorded as a follow-up in the PR body.

Validation: npm run verify passed locally (298 tests, lint, typecheck, build). Manual device/player-route smoke coverage remains pending and is recorded in the PR body.

[feralfile-bot]

Reviewers: 3 (approve: 0, comment: 0, request-changes: 3).

Critical issues

• [P1] resolveRenderStatusForCastInfo() compares raw indices with .at(). moveToArtwork accepts out-of-range positive indices, while PlaylistClient normalizes them modulo playlist length. Moving from 0 to 3 in a two-item playlist renders item 1, but this comparison sees no next item and preserves the old ready/failed status. Normalize both indices before comparing identities, and add coverage for this case.

• [P1] Relative and protocol-relative sources are explicitly accepted, but MIME detection still calls new URL(previewURL) without a base. This throws before extension/MIME inference, causing valid relative image/video sources without an explicit MIME type to fall back to an iframe instead of their native renderer. This relative-source concern has already recurred in discussion; resolving the iframe display URL fixed only the later iframe path, not this MIME-detection path. Resolve before MIME detection and use the resolved URL consistently; add native image/video coverage for both relative forms.

Medium issues

• [P2] showRenderLoadingOverlay: false does not suppress the model loading overlay. ArtworkPlayer hides its own spinner for models, but ModelViewerScreen always renders “Loading 3D model.” Thread the setting through or narrow the documented contract; add GLB/glTF coverage.

• [P2] A successful refreshArtwork reply can be followed immediately by a stale ready or failed status poll. The service returns { ok: true } after scheduling React state, while pending is only published later by the reload effect. Publish RenderStatus.pending synchronously once the refresh handler accepts the command, with a command-level immediate-poll test.

• [P2] docs/DEVICE_LOCAL_PLAYER.md removes the still-applicable setup-overlay background artwork contract. Restore that operational documentation alongside the new render-status/source-validation sections.

Missing validation

• The required player-route manual smoke remains incomplete for slow loads, failures, same-ID refreshes, and disabled loading overlays. This PR changes runtime playback/status behavior, so the pending smoke evidence should be completed before merge.

I did not re-raise the separately owned ffos-user typed status bridge or the deferred ff-app source-normalization work; both are explicitly documented as downstream follow-ups.

[feralfile-bot]

Reviewers: 3 (approve: 0, comment: 1, request-changes: 2).

Critical issues

• CanvasService.setRenderStatus() republishes castInfo with the previous castCommand. For a displayPlaylist cast, each lifecycle update (pending, loading, ready, or failed) is therefore reprocessed as a new display command by PlaylistClient, replacing the playlist state and re-arming its timer. Slow or failed renders can extend/reset finite artwork durations. Use a status-only notification path, or explicitly ignore render-status-only updates in the route. Add route-level coverage proving lifecycle changes do not replay display setup or reset timers.

Follow-ups considered

• The ffos-user typed renderStatus bridge is explicitly documented as an out-of-scope downstream follow-up, so it is not raised again here.
• Manual player-route smoke coverage remains pending as recorded in the PR body.
• The mixed-case DATA: report is not valid: new URL() normalizes the scheme to data: before the protocol allowlist check.
• The HLS non-fatal default-error behavior predates this PR; this change reports that existing terminal path rather than introducing the teardown policy.

[ppupha]

Review follow-up for review 4804214898:

Fix now — completed

• setRenderStatus() now updates status state without republishing the prior cast command. This prevents lifecycle changes from re-running displayPlaylist and restarting finite artwork timers.
• Added service-level and playlist-route timer regression coverage.

Out of scope

• The ffos-user typed PlayerStatus.renderStatus bridge is a separately owned downstream-repository change, already documented as a follow-up in this PR.
• Manual device/player-route smoke coverage remains human-owned operational verification; it is recorded as pending in the PR rather than represented as a code defect in this review follow-up.

Invalid / no change needed

• Mixed-case DATA: is normalized by new URL() to the allowed data: scheme.
• The cited nonfatal HLS behavior predates this PR; this branch does not change its teardown policy.

Validation: npm run post-implement-check and env -u SENTRY_AUTH_TOKEN npm run verify passed (lint, typecheck, 300 tests, production build). Full-diff reviewer rerun found no remaining code issues; its only outstanding note is the already-recorded manual smoke follow-up.

[feralfile-bot]

Reviewers: 3 (approve: 0, comment: 2, request-changes: 1).

• Medium — isSupportedArtworkSource() accepts every data:-prefixed value without validating its payload, so malformed values such as data:;base64,%%%% are acknowledged by now-display, schedule-play, and refresh and can commit unusable playback state. This conflicts with the documented contract that malformed sources return { ok: false } before state changes. Validate data: URL structure/payload and add command-level coverage for malformed data URLs. (src/services/CanvasService.ts:92)

The repeated manual player-route smoke gap was considered but is already explicitly recorded as a human-owned PR follow-up, so it is not re-raised as an active code finding. Likewise, the downstream ffos-user typed status bridge is explicitly deferred and remains out of scope for this player-side PR.

[feralfile-bot]

Reviewers: 3 (approve: 0, comment: 0, request-changes: 3).

• src/services/CanvasService.ts:93 — isValidDataUrl() returns true for every non-base64 data: payload, so malformed percent escapes such as data:image/svg+xml,%ZZ are accepted and can replace active playback or be scheduled. This contradicts the documented validation contract. Validate malformed escapes while retaining the intended raw-percent SVG compatibility, and add Now Display, Schedule Play, and refresh coverage.

The pending manual player-route smoke scenarios and the downstream ffos-user status bridge were explicitly recorded as deferred/out-of-scope follow-ups, so I am not raising them as active findings.

[feralfile-bot]

Reviewers: 3 (approve: 0, comment: 0, request-changes: 3).

Critical issues

• isValidDataUrl still accepts incomplete/invalid non-base64 percent escapes such as data:image/svg+xml,broken%, %2, %A, and %G-. The regex at CanvasService.ts:98 only evaluates % followed by two alphanumeric characters, so these malformed inputs bypass the new validation and can replace active playback or be scheduled despite the documented { ok: false } contract. Reject incomplete/invalid attempted escapes while retaining the intended raw-SVG literal-percent compatibility. This was independently identified by all three reviewers.

Missing tests

• Add regression coverage for incomplete and partially malformed percent escapes across Now Display, Schedule Play, and refresh, alongside the existing %ZZ case.

The ffos-user typed renderStatus bridge is explicitly documented as a separately owned follow-up, so I am not raising it again here. Likewise, the pending manual player-route smoke is acknowledged in the PR body as operational follow-up rather than a separate code finding.

[feralfile-bot]

Reviewers: 3 (approve: 0, comment: 0, request-changes: 3).

• Critical — iframe/PDF failures can be reported as ready. ArtworkPlayer promotes iframe and PDF content to ready solely on load. Browsers commonly fire that event for an error document after a failed navigation, while iframe error is not a dependable remote-navigation failure signal. This makes the new status contract falsely report ready for supported artwork that did not render as intended. Avoid treating iframe load as verified success, or introduce a reliable failure/verification path; add browser-level coverage for this case.

• Manual playback smoke remains a merge gate. This was raised independently by reviewers 2 and 3 and remains explicitly unchecked in the PR: slow load, render failure, same-ID source refresh, and showRenderLoadingOverlay: false. The repository workflow requires manual smoke evidence for this playback-facing lifecycle change. This recurrence should be resolved before merge.

I re-checked the reported hook-dependency issue: it is not active because ArtworkPlayer.tsx intentionally disables react-hooks/exhaustive-deps, and pauseSlotPlayback is a stable callback. I also considered the downstream ffos-user status bridge and upstream source normalization concerns; both are explicitly recorded as separately owned/out-of-scope follow-ups, so they are not raised again here.

[feralfile-bot]

Reviewers: 3 (approve: 0, comment: 1, request-changes: 2).

Critical issues

• ArtworkPlayer.tsx:816 lets a stale iframe load mutate current state before confirming its slot/layer is current. In particular, an outgoing iframe load can invoke handleWebGLLost(), publish renderStatus: failed, and start recovery for newer artwork. Guard handleIframeLoad with isCurrentArtworkSlot(slotIndex, layer) before any side effect. Add a stale-iframe-load regression test.

• Valid base64 data:image/... sources are accepted by the new cast contract but cannot reliably become ready on the normal playlist route. That route supplies no MIME type, and getContentTypeFromURL() appends cache-busting text to the data payload before its HEAD; this invalidates base64 payloads, falls back to an iframe, and iframe lifecycle intentionally remains loading. Derive the MIME type locally from a data URL and select the renderer without a HEAD request. Add playlist-route coverage asserting data:image/... renders and reports ready.

Missing tests

• The pending manual player-route smoke coverage is already explicitly recorded in the PR, so it is not repeated as an active finding.
• The separately owned ffos-user typed status bridge is documented as a downstream follow-up and is likewise not raised again here.