fix(tv): address Fire TV rc.1+4 feedback - #161
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe TV app now propagates complete display versions, improves focused-item visibility and match-code sizing, restores playback state after player exit, refines playback start-position resolution, and adds unit-test coverage. ChangesTV application updates
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/player/TvFireTvRcFeedbackOwnershipTest.kt (1)
58-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftRecommend: replace source-text pattern matching with behavioral tests for the Kotlin logic checks.
These two tests read
TvVideoPlaybackStarter.kt/TvPlayerScreen.kt/TvPlayerViewModel.kt/TvItemDetailViewModel.ktas raw text and assert on substring presence, counts, or relative ordering (contains,assertBefore'sindexOf), instead of exercising the actual functions.This has two consequences. First, it cannot catch a logic regression that leaves the same tokens in the same order but changes the value flowing through them — for example,
starter.contains("overridePosition = request.resumePositionOverride")at line 68 matches both theplayerStartPosandsourceStartPoscall sites inTvVideoPlaybackStarter.kt, since the substring is a prefix of theplayerStartPoscall'soverridePosition = request.resumePositionOverride.takeIf(::isExplicitStartOver)too. Second,asyncBody.contains("TvDetailTrackSelectionSession.remember")at line 93 also matchesTvDetailTrackSelectionSession.rememberPlaybackReturn, so a rename or behavior change to that call would not be caught as long as the prefix survives.For the Gradle/YAML checks (release display version), this is a reasonable choice since those files aren't directly unit-testable. For the Kotlin logic — start-position resolution and exit-ordering — consider testing the real functions/classes with fakes instead, so a value or behavior regression is actually caught.
Also applies to: 71-110
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/player/TvFireTvRcFeedbackOwnershipTest.kt` around lines 58 - 69, Replace the raw source-text assertions in the Kotlin logic tests, including explicit start-position resolution and exit-ordering checks, with behavioral tests that invoke the real functions/classes such as TvVideoPlaybackStarter, TvPlayerScreen, TvPlayerViewModel, and TvItemDetailViewModel using fakes. Assert the resolved values and observable call/order behavior directly, while retaining source-text checks only for Gradle/YAML release-version validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/auth/TvServerSetupScreen.kt`:
- Line 847: Update the separator width used by MatchCodeCard to the shared
MATCH_CODE_SEPARATOR_WIDTH_DP value instead of the hardcoded 12dp values. Apply
this consistently to the separator rendering at the referenced locations while
leaving matchCodeTileWidthDp unchanged, so the setup row honors the shared
maximum width.
In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/detail/TvItemDetailViewModel.kt`:
- Around line 1507-1527: Update rememberPlaybackReturn so the subtitle field
falls back to previous?.subtitle when the subtitle argument is null, preserving
the prior explicit choice for keep-current signals. Keep explicit non-null
subtitle values unchanged, and add a test in TvTrackSelectionPersistenceTest
mirroring playbackReturnPreservesPreviouslySelectedAudio for the null-subtitle
case.
In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModel.kt`:
- Around line 3914-3937: Update stopSessionForExitAsync to apply the provided
positionMs and durationMs directly to _uiState before capturing the exit
snapshot, bypassing onPositionChanged and its seek-recovery or transport-mount
gates. Convert the values to the existing positionSeconds and durationSeconds
units, preserve the current state when either value is absent, and ensure
rememberPlaybackReturn uses the updated snapshot.
In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvVideoPlaybackStarter.kt`:
- Around line 200-219: The sourceStartPos calculation in the
resolvePlaybackStartPosition call must use the already-rewound
startRequestPosition as the override instead of the raw
request.resumePositionOverride. Update this call while preserving
serverSourceStartPos and the existing playerStartPos logic, so
StartParams.startPosition matches the value sent as the server startPosition
during resume rewinds.
In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/recommendations/TvRecommendationsScreen.kt`:
- Around line 303-316: The first recommendation’s onItemFocused handler must
keep enforcing the scroll-to-top reset until focus relocation finishes, rather
than issuing a single animateScrollToItem call. Update the index-0 callback
around recommendationsListState and focusBridgeScope so it rechecks the list
position and reapplies the reset while the first row remains focused, preserving
the existing no-op behavior when already at item 0.
---
Nitpick comments:
In
`@androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/player/TvFireTvRcFeedbackOwnershipTest.kt`:
- Around line 58-69: Replace the raw source-text assertions in the Kotlin logic
tests, including explicit start-position resolution and exit-ordering checks,
with behavioral tests that invoke the real functions/classes such as
TvVideoPlaybackStarter, TvPlayerScreen, TvPlayerViewModel, and
TvItemDetailViewModel using fakes. Assert the resolved values and observable
call/order behavior directly, while retaining source-text checks only for
Gradle/YAML release-version validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c8e085d-2beb-4c88-bf63-f20b48640bc0
📒 Files selected for processing (12)
.github/workflows/release.ymlandroidTvApp/build.gradle.ktsandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/components/TvAnchoredSelectorMenu.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/auth/TvServerSetupScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/detail/TvItemDetailViewModel.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvPlayerViewModel.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/player/TvVideoPlaybackStarter.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/recommendations/TvRecommendationsScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/TvSettingsScreen.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/detail/TvTrackSelectionPersistenceTest.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/player/TvFireTvRcFeedbackOwnershipTest.kt
|
Review follow-up pushed in The Kotlin source-text checks called out in the review were removed. Real behavioral tests now cover:
Fresh verification on the merged tree:
@coderabbitai review |
|
✅ Action performedReview finished.
|
|
Pushed audit follow-up in
Fresh verification passed: |
1 participant
Summary
Addresses the Fire TV rc.1+4 QA feedback reported on 2026-08-03:
The coordinated server change that limits newly generated pairing codes to eight letters is Silo-Server/silo-server#535. This client remains compatible with older long codes.
Implementation notes
BuildConfig.DISPLAY_VERSIONfromVERSION_NAME; release builds receive the full tag throughSILO_DISPLAY_VERSION.Testing
./gradlew :shared:testDebugUnitTest :androidTvApp:testDebugUnitTest :androidTvApp:assembleDebug -PsiloVersionName=1.0.0 -PsiloDisplayVersion=1.0.0-rc.1+4 --no-daemonBUILD SUCCESSFUL— 110 actionable tasks1.0.0-rc.1+4in the generated debug build.Related work
Summary by CodeRabbit
New Features
Bug Fixes