Merged
Conversation
Co-authored-by: Tataihono Nikora <tataihono.nikora@gmail.com>
Co-authored-by: Tataihono Nikora <tataihono.nikora@gmail.com>
Co-authored-by: Tataihono Nikora <tataihono.nikora@gmail.com>
Co-authored-by: Tataihono Nikora <tataihono.nikora@gmail.com>
Co-authored-by: Tataihono Nikora <tataihono.nikora@gmail.com>
|
Cursor Agent can help with this pull request. Just |
5 tasks
|
Caution Review failedThe pull request is closed. WalkthroughThese changes add essential repository configuration best practices: EditorConfig for consistent code formatting across editors, Dependabot for automated dependency management across npm and infrastructure-as-code ecosystems, Node version pinning via .nvmrc, and CI workflow updates to enforce lockfile integrity. CONTRIBUTING.md is added to document the contribution workflow. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
✨ 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 |
2 tasks
9 tasks
Ur-imazing
added a commit
that referenced
this pull request
Apr 23, 2026
…+ crimson palette (#830) * docs(tv): brainstorm + plan for video-player auto-hide controls Captures the requirements (5s auto-hide via D-pad, play/pause initial focus, crimson palette alignment) and the 7-unit implementation plan for apps/tv/src/components/VideoPlayer.tsx. Brainstorm: docs/brainstorms/2026-04-21-tv-video-player-controls-auto-hide-requirements.md Plan: docs/plans/2026-04-22-001-feat-tv-video-player-auto-hide-controls-plan.md * feat(tv): scaffold video-player auto-hide state + retire warm-salmon palette Unit 1 of docs/plans/2026-04-22-001-feat-tv-video-player-auto-hide-controls-plan.md. Introduces the auto-hide state machine scaffolding in VideoPlayer.tsx and retires the file-local warm-salmon design tokens. This is foundation-only — subscriptions + refs are in place but the behavioral surface (fade, catcher, buffering/error handling) lands in Units 2-7. - Add controlsVisible, controlsFocusable, status, hasError, isScreenReaderEnabled, isReduceMotionEnabled state. - Add revealFocusPending + errorFocusPending one-shot focus flags (I6), mirroring Fix #5's clear-after-render pattern. - Add stable handler refs (scheduleHideRef, revealControlsRef) so subscriptions in Units 2-3 don't churn the native event emitter. - Mirror controlsVisible and isScreenReaderEnabled into refs so the useTVEventHandler callback in Unit 3 can read without re-binding. - Subscribe to AccessibilityInfo screenReaderChanged and reduceMotionChanged (HomeHero subscription shape). - Subscribe to AppState: on 'active', snap visible + rearm timer. - Claim hardware Menu on tvOS via TVEventControl.enableTVMenuKey() with menuKeyEnabledRef bookkeeping so cleanup is idempotent. - Clear inactivityTimerRef on unmount. - Move hasTVPreferredFocus from back pill to play/pause ({shouldRequestFocus || revealFocusPending}); back pill now receives {errorFocusPending} for Unit 5's error-state focus. - Retire the file-local warm-salmon tokens (ACCENT, ACCENT_ON, TEXT_PRIMARY, TEXT_SECONDARY) and swap every call-site to the shared Crimson Gallery COLORS.* tokens. Play button + progress fill are now COLORS.primary; icons + title use COLORS.text; subtitle + time readouts use COLORS.muted. All eight existing numbered fixes (#4, #5, #6, #8, #9, #15, #24, #25) are preserved. Typecheck + lint clean. * feat(tv): wire video-player auto-hide fade + 5s inactivity timer Unit 2 of docs/plans/2026-04-22-001-feat-tv-video-player-auto-hide-controls-plan.md. Implements the core hide/reveal cycle that Unit 1 scaffolded. - Wrap topBar and controlsContainer in Animated.View, both bound to the shared opacityAnim; collapsable={false} preserves z-order above the Android TV VideoView surface. - Add hideControls: setControlsFocusable(false) before the animation so UIFocusEngine releases the controls before they're invisible (I7 ordering), then 150 ms ease-out fade (or opacityAnim.setValue(0) when reduce-motion is active), then flip controlsVisible to false. - Add scheduleHide: idempotent timer arm — clears any in-flight timer, then only re-arms a new 5 s if the state supports auto-hide (not paused, not loading/error, no hasError, no screen reader). - Add revealControls: early-return when already visible to neutralize the catcher-onPress vs useTVEventHandler-select double-dispatch race that Unit 3 introduces. Does NOT reset opacityAnim before animating to 1 — in-flight hide animations reverse from their current mid-fade value, avoiding a black flash. - Assign scheduleHide / revealControls into their stable refs so the AppState handler (Unit 1) and event handlers (Unit 3) see the latest closure — mirrors Fix #15's onDismissRef. - Modify playingChange listener: clear timer on pause, call scheduleHideRef on resume. First isPlaying=true arms the D1 initial 5 s countdown. - Add a 2 s mount fallback that calls scheduleHideRef unconditionally — covers the race where autoplay retry succeeds but playingChange hasn't fired yet. scheduleHide is idempotent so the normal path wins. - Every control Pressable (back, rewind, play/pause, forward) now reads focusable={controlsFocusable} and calls scheduleHide on onFocus; the three active controls also call scheduleHide on onPress after their original handler (satisfies D14). Typecheck + lint clean. * feat(tv): add D-pad catcher + event routing for hidden video-player chrome Unit 3 of docs/plans/2026-04-22-001-feat-tv-video-player-auto-hide-controls-plan.md. Closes the loop from Unit 2: hidden controls now have an owner for D-pad input, so any key reveals the chrome without firing the focused button's action. - Add useTVEventHandler with a ref-stable useCallback (reads controlsVisibleRef / isScreenReaderEnabledRef / revealControlsRef / scheduleHideRef) so the native TV emitter doesn't re-register on every render. Hidden state: any recognized event triggers reveal. Visible state: Siri-remote swipes reset the timer for D14 (arrow / Select events already reset via Pressable onFocus / onPress). Defensive whitelist-or-fallback on the event-type string — names vary across react-native-tvos versions and Siri-remote generations. - Add BackHandler.addEventListener('hardwareBackPress'). Returns true to consume. Hidden → revealControls. Visible → onDismissRef. react-native-tvos bridges tvOS hardware Menu into this event, so a single subscription covers both platforms. Paired with TVEventControl.enableTVMenuKey from Unit 1 to keep Expo Router's Stack from popping before our handler runs. - Render an invisible Pressable catcher as the first child of TVFocusGuideView when !controlsVisible && !isScreenReaderEnabled. StyleSheet.absoluteFillObject lifts it above the sibling flex layout; hasTVPreferredFocus claims focus on mount (fresh mount per hide cycle); onPress={revealControls} is the primary Select path. collapsable={false} preserves Android TV z-order above the VideoView surface. accessibilityLabel + accessibilityRole for App Store review. - revealControls' existing early-return on controlsVisibleRef.current already neutralizes the catcher-onPress vs useTVEventHandler-select double-dispatch race (see Unit 2). Typecheck + lint clean. * docs(tv): document U4 focus-restore mitigation choice on play/pause Pressable Unit 4 of docs/plans/2026-04-22-001-feat-tv-video-player-auto-hide-controls-plan.md. The focus-restore wiring itself was landed across Units 1 and 2: - U1 added revealFocusPending state + clearing useEffect + wired hasTVPreferredFocus={shouldRequestFocus || revealFocusPending} on play/pause. - U2 added setRevealFocusPending(true) inside revealControls(). The plan offered three mitigations for tvOS's continuously-mounted Pressable focus behaviour. We rely on mitigation (b) — per-cycle focusable toggle — because Unit 2 already flips controlsFocusable on every hide/reveal, producing a "new focus target" event for UIFocusEngine. This comment records the choice so device QA knows which path is in play and what the fallback is (mitigation (a): a per-cycle `key` on the Pressable). * feat(tv): handle video-player buffering + terminal playback error Unit 5 of docs/plans/2026-04-22-001-feat-tv-video-player-auto-hide-controls-plan.md. - Subscribe to player.addListener('statusChange', ...). Branches on the expo-video VideoPlayerStatus literal union — confirmed from node_modules as 'idle' | 'loading' | 'readyToPlay' | 'error' (there is no 'buffering' or 'playing' value). Uses controlsVisibleRef and seekTargetRef to avoid stale-closure reads inside the long-lived callback. - 'loading' + seekTargetRef.current !== null → no-op. Fix #4's seek guard already handles the UI; without this branch the timer would suspend (and force-reveal) on every 10 s skip press. - 'loading' + no seek → clear timer, force reveal if hidden. A hidden stall is indistinguishable from 'video ended' for a low-confidence user; showing chrome during buffering gives them an affordance. revealControls' early-return makes this a no-op if already visible. - 'readyToPlay' → scheduleHideRef to restart the 5 s countdown. - 'idle' → no-op (mount-time default, not a runtime transition). - 'error' → setHasError(true), clear the timer permanently, snap chrome visible, set errorFocusPending=true. hasError gates every subsequent scheduleHide call (added to U2's guard list). Back pill claims focus via hasTVPreferredFocus={errorFocusPending} (Unit 1). - Replace titleRow contents with 'Playback failed — press Back to exit.' when hasError. Inline, not a separate layer — preserves the single-file / single-overlay constraint. Subtitle stays for context. - Ghost rewind / play/pause / forward Pressables when hasError: focusable={controlsFocusable && !hasError} plus a new styles.controlDisabled (opacity 0.3) applied to each. The controls remain mounted so the spatial layout doesn't collapse — only the back pill is meaningful in the error state. Typecheck + lint clean. * feat(tv): flash chrome for one paint before video-player playToEnd dismiss Unit 6 of docs/plans/2026-04-22-001-feat-tv-video-player-auto-hide-controls-plan.md. - Modify the playToEnd listener so that when chrome is hidden at end-of-video, we synchronously snap it visible (setControlsVisible / setControlsFocusable / opacityAnim.setValue(1)) and dispatch onDismissRef via setTimeout(0). Intent is imperceptible technical continuity — the chrome appears for one paint so the dismiss transition doesn't start from a black-screen-with-no-UI. setTimeout(0) queues the dismiss after the current render commit; rAF's mapping to the native paint thread is less specified under react-native-tvos, so we avoid it here. Fallback to rAF if device QA shows a black frame on a hidden-to-dismissed transition. - Refactor to a doDismiss closure so both visible- and hidden-state paths share the Fix #24 try/catch wrapping. - Preserves I4a: playToEnd still always calls onDismissRef.current() — the new logic only delays the call by one tick when chrome was hidden, it never suppresses it. - Expand the comment on Unit 1's AppState handler to document the paused-state branch: scheduleHide's internal isPaused guard (U2) makes an explicit AppState-level guard unnecessary, which avoids re-subscribing AppState on every pause toggle. Also clarifies that playback resume behaviour is out of scope per the plan's deferred items. Typecheck + lint clean. * feat(tv): polish video-player accessibility for screen readers Unit 7 of docs/plans/2026-04-22-001-feat-tv-video-player-auto-hide-controls-plan.md. - Add a useEffect keyed on isScreenReaderEnabled that handles mid-session SR toggles. SR→on: reveal chrome if hidden (the user just gained access to controls they couldn't navigate) plus AccessibilityInfo.announceForAccessibility('Player controls visible') for audible confirmation. SR→off: rearm via scheduleHideRef so a passive viewer who briefly toggled VO on-then- off doesn't get stuck with permanent chrome. A srSeededRef guard skips the first invocation so the mount-time seed from AccessibilityInfo doesn't fire spurious side-effects. - Add accessibilityLabel + accessibilityRole='button' to every control Pressable: back: "Back to {subtitle}" when a subtitle is present, else "Back" rewind: "Rewind 10 seconds" play/pause: dynamic — "Play" when paused, "Pause" when playing forward: "Forward 10 seconds" The invisible catcher (Unit 3) already carries "Show player controls" + role=button. - Unit 3's catcher conditional render ({!controlsVisible && !isScreenReaderEnabled && ...}) means the catcher is NEVER rendered when a screen reader is active, so VoiceOver/TalkBack never encounters a blank interactive element. Plus U2's scheduleHide already bails on isScreenReaderEnabled, so auto-hide never fires while SR is on. Typecheck + lint clean. * fix(tv): stop video-player auto-hide loop + shorten inactivity to 3s Two fixes surfaced by manual QA on the Apple TV simulator: 1. Reveal-on-any-event was too aggressive. The hidden-state branch of useTVEventHandler triggered revealControls on ANY TV event, including the synthetic focus/blur/pan events that react-native-tvos fires when the engine reassigns focus. When the catcher mounted post-hide with hasTVPreferredFocus, the focus acquisition fired a synthetic event, the handler saw controlsVisibleRef.current === false, and called revealControls — producing an instant hide→reveal loop. Fix: strict-whitelist the eventType on the hidden-state branch. Only real directional (up/down/left/right), select, longSelect, and swipe* events count as user intent. Focus-change events are explicitly ignored. 2. Shorten the auto-hide inactivity period from 5000 ms → 3000 ms per user feedback (feels snappier during manual QA). Updated the three inline comments that cited the old value. * fix(tv): resolve 4 code-review P1 bugs in video-player auto-hide Discovered via /ce-code-review on the branch. All four break user- visible behaviour on playback transitions that manual QA on the first play cycle happened to miss. P1.1 — Stale-closure in scheduleHide broke resume-from-pause (adversarial 80, correctness+julik cross-corroborated). scheduleHide read isPaused/status/hasError from the render closure. After paused→ play, the onPress wrapper called scheduleHide synchronously before setIsPaused(false) committed, and the playingChange handler then called scheduleHideRef.current() also before the commit — both saw stale isPaused=true and bailed. Auto-hide never rearmed after any resume. Fix: mirror isPaused / status / hasError into refs (mirroring the existing controlsVisibleRef / isScreenReaderEnabledRef pattern). scheduleHide reads from refs. playingChange and statusChange handlers sync the relevant ref BEFORE calling scheduleHideRef so the guard reads the post-transition value. P1.2 — Animated.timing hide completion callback ran after state transitions (reliability rel-1 75, adversarial #4 60, julik-9 75). The callback unconditionally called setControlsVisible(false), which clobbered force-reveals triggered by error / AppState / unmount. Fix: capture the Animated.CompositeAnimation handle in hideAnimRef. Guard the completion callback with `if (finished)` — stopped animations carry finished=false. Call .stop() from revealControls, the error branch, the AppState handler, and the unmount cleanup so no stale completion can clobber a just-revealed chrome. P1.3 — 150 ms fade dead-zone dropped D-pad input (correctness 75, julik-1 75). During the fade, controls were non-focusable (set synchronously) but the catcher wasn't mounted yet (gated on !controlsVisible, which only flipped in the anim-completion). No focusable target → UIFocusEngine dropped input. Fix: gate the catcher on !controlsFocusable instead of !controlsVisible. The catcher mounts synchronously with the focusable flip, giving tvOS a valid focus target throughout the fade. P1.4 — AppState 'active' bypassed revealControls → orphaned focus (julik-5 75). The handler directly flipped state + opacity but never set revealFocusPending, so on foreground resume the catcher unmounted but play/pause had no hasTVPreferredFocus claim. Matches react- native-tvos #852. Fix: AppState handler now reads hasErrorRef and routes focus to the correct Pressable — errorFocusPending for error state (back pill), revealFocusPending for normal playback (play/pause). Also stops any in-flight hide animation so its completion doesn't clobber the force-reveal. Removed hasError from the effect's deps (reads via ref now — avoids AppState re-subscription on every hasError flip). Also includes P2.1 opportunistically: error branch of statusChange now explicitly setRevealFocusPending(false) so a same-tick reveal doesn't double-claim hasTVPreferredFocus against the back pill's errorFocusPending (adversarial #3 65 + julik-4 50). Typecheck + lint clean. * fix(tv): harden video-player listeners for late-emission + media keys Two more fixes from /ce-code-review P2 tier. P2.2 — useTVEventHandler whitelist excluded hardware media keys (julik-3 50, correctness 25). The strict whitelist that fixed the hide→reveal loop in 2fd092c (excluding focus/blur/pan synthetic events) also excluded playPause / fastForward / rewind events that some Android TV remotes and Siri remote gen-1 emit. Users pressing the physical play/pause button while chrome was hidden saw nothing happen. Fix: denylist the four synthetic focus events (focus, blur, pan, panBegin, panEnd) instead of whitelisting a closed set of known user inputs. Anything not synthetic is treated as intent. Also route media-key events in the visible branch to scheduleHideRef so they reset the timer (same D14 behaviour as Siri swipes). P2.3 — Late-emission listeners could setState on an unmounted component (reliability rel-2 75, rel-3 75). Fix #24's try/catch only catches a throwing onDismiss — it doesn't cover the "callback fires after React unmount" case. Risks surface during rapid mount/unmount (e.g. quickly backing out of a video and opening a new one) when expo-video's native emitter delivers a queued statusChange / playing Change / playToEnd AFTER subscription.remove() has run. Fix: new isMountedRef, flipped to false in the existing unmount useEffect cleanup. Early-returns added to the statusChange + playingChange listener bodies and to the playToEnd hidden-chrome setTimeout(0) callback, so late native emissions are silently ignored instead of calling setState / onDismiss on a dead tree. Typecheck + lint clean. * docs(solutions): capture TV video-overlay async-event patterns + refresh 3 related docs New learning (docs/solutions/design-patterns/): - rntvos-video-overlay-async-native-event-patterns-2026-04-23.md Four primary + two extension patterns extracted from the PR #830 code- review fix batch in apps/tv/src/components/VideoPlayer.tsx: ref-mirror + eager-sync for state-gated guards called from native callbacks; Animated.CompositeAnimation handle capture + if(finished) + .stop() on every transition; gate focus-trap catchers on synchronous focusability, not async visibility; set a one-shot hasTVPreferredFocus flag yourself on AppState foreground (react-native-tvos #852 workaround); denylist synthetic focus events in useTVEventHandler; isMountedRef for late- emission native callbacks. Cross-references added (ce-compound-refresh on 3 related best-practices docs — additions only, no rewrites): - react-native-tvos-porting-pitfalls-20260414.md: Related entry noting the new doc extends the pitfall catalog with async-event-vs-React-state issues that aren't covered by the six existing type/layout pitfalls. - playlist-video-player-sdui-mobile-20260409.md: inline "On TV, additionally" note in Section 5 (wasPlayingRef AppState guard) pointing to Pattern 4's one-shot hasTVPreferredFocus flag; Related entry for the TV companion. - tv-focus-driven-hero-patterns-20260420.md: Related entry linking Pattern 5's useTVEventHandler denylist as a complement to Section 6's onFocus-on-leaf rule. * style(docs): prettier --write for brainstorm + plan docs to satisfy CI CI's `format` job runs `prettier --check .` on the whole repo. The brainstorm + plan were written by the ce-brainstorm / ce-plan skills, which don't run prettier on *.md at write time. lint-staged only covers *.{ts,tsx} + package.json on commit, so the drift slipped past locally. Pure formatting; no content changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
chore(repo): implement configuration best practices
This PR introduces several best practices for repository configuration and development workflow:
frozen-lockfilein CI workflows to ensure consistent dependency installations..nvmrcand configures CI to use it..editorconfigfor consistent code formatting (2-space indent, LF, trim whitespace).CONTRIBUTING.mdto guide contributors with an issue-first workflow.Resolves #5
Contracts Changed
Regeneration Required
Validation
Summary by CodeRabbit
Documentation
Chores