Skip to content

Codec default, settings picker fix, catalog perf, and concurrency cleanup - #89

Draft
MonsieurNours wants to merge 6 commits into
owenselles:mainfrom
MonsieurNours:improvements/settings-catalog-signing
Draft

Codec default, settings picker fix, catalog perf, and concurrency cleanup#89
MonsieurNours wants to merge 6 commits into
owenselles:mainfrom
MonsieurNours:improvements/settings-catalog-signing

Conversation

@MonsieurNours

Copy link
Copy Markdown

Summary

Groups several fixes and improvements made while getting the app building and browsing smoothly on Apple TV 4K.

Changes

Settings — modal selection sheets (fixes blank/trapped picker screens)

On tvOS, the stream-quality and controller Pickers pushed a blank detail screen and trapped navigation (no way back — the app had to be force-quit). Replaced every picker with a reusable SettingSelectionRow that opens a padded modal list, the same pattern as the already-working zone picker: compact rows plus a readable sheet with a checkmark on the selected value.

Video codec default → H265

Changed the default codec from H264 to H265 (HEVC): it's hardware-decoded on Apple TV 4K and is the only codec that carries 10-bit/HDR10 through the app's custom decoder. H264 is 8-bit only; AV1 falls back to a software SDR path. Still user-switchable.

Catalog performance

  • GameGrid now prefetches the hero image on card focus (the Store only prefetched box art before), removing the cold-decode lag when opening a game.
  • GameCarouselView opens straight to the tapped game's expanded card so focus lands on Play, and the metadata reveal delay is cut from 360 ms to 180 ms.

Concurrency / warning cleanup

  • GFNStreamController: guard let self + explicit [self] capture in the main-actor Tasks bridged from the video/signaling callbacks — clears the Swift 6 "captured var self in concurrently-executing code" and "weak ownership differs" warnings.
  • InputSender: RemoteInputMode.remoteActsAsMouse marked nonisolated.
  • CloudNowApp: token-refresh background task registered via the .backgroundTask scene modifier instead of reading the @State AuthManager in App.init (fixes the runtime "accessing State without being installed on a View" warning).

Build — local code signing

Wired Local.xcconfig as the base configuration for the project's Debug/Release configs and removed the hardcoded DEVELOPMENT_TEAM, so contributors set their own Team ID via the gitignored Local.xcconfig (per Local.xcconfig.example).

Note: this changes signing to require a local Local.xcconfig. Drop the build: commit if you'd rather keep a default team in the project file.

Testing

  • SwiftFormat and SwiftLint pass on all changed files.
  • Settings and catalog changes were exercised on an Apple TV 4K during development (screenshots). A full build/run pass is recommended before merge.

@aarikmudgal aarikmudgal self-assigned this Jul 18, 2026
@aarikmudgal
aarikmudgal self-requested a review July 18, 2026 13:10
@aarikmudgal

Copy link
Copy Markdown
Collaborator

Thanks @MonsieurNours for putting these improvements together. I reviewed the PR against the latest main, including the controller and catalog optimizations added in #91. I like the direction of the settings, background-task, codec, and carousel interaction changes.

Before merging, could you please address the points below? The main goal is to keep the useful changes from this PR without losing the newer performance and controller-navigation improvements from main.

1. Fix the Release build errors

The current PR passes SwiftFormat and SwiftLint, but the unsigned Release tvOS build fails in two places inside GFNStreamController.swift:

  • Around line 361, inside bindVideoView(_:)
  • Around line 632, inside setupSignaling(session:)

Both Task closures capture self, but then call an instance method without explicitly using self. Under the Release compiler configuration, this produces:

Implicit use of self in closure; use self. to make capture semantics explicit.

Please update them like this:

Task { @MainActor [self] in
    self.applyDecodedVideoFormat(format)
}
Task { @MainActor [self] in
    self.handleSignalingEvent(event)
}

This keeps the intended strong capture during the main-actor task and allows the Release build to compile.

2. Remove the Xcode project file changes from this PR

Could you please remove all changes to CloudNow.xcodeproj/project.pbxproj from this PR and restore that file exactly as it is in the latest main?

The PR currently:

  • Adds Local.xcconfig as a project-level base configuration.
  • Adds different DEVELOPMENT_TEAM values to the project and target configurations.
  • Still leaves the target-level team setting in control.

The target-level DEVELOPMENT_TEAM overrides the project-level value, so Local.xcconfig does not currently control signing as intended. xcodebuild -showBuildSettings still resolves the target team to P6DB682S4K, while another team ID is also present in the project configuration.

This makes the signing setup contributor-specific and difficult to reproduce in CI or on another developer’s machine. Signing configuration can be handled separately in another PR with a complete local and CI setup.

For this PR, please revert the complete project.pbxproj diff. This means removing the project file changes from the PR, not deleting the actual Xcode project file.

3. Remove the duplicate hero-art prefetch

The existing GameCardLabel implementation in LibraryView.swift already has:

.prefetchHeroArtOnFocus(game.heroImageUrl ?? game.heroBannerUrl)

The PR adds another modifier to the same card path around line 251:

.prefetchHeroArtOnFocus(game.heroBannerUrl ?? game.boxArtUrl)

Because these modifiers use different URL fallback orders, focusing one game can start two separate image requests and decoding operations.

HeroArtPrefetcher intentionally has a bounded queue and decoded-image cache. Starting two prefetches for every focused card can:

  • Occupy both concurrent prefetch slots.
  • Push useful requests out of the pending queue.
  • Download and decode artwork that may never be shown.
  • Increase memory usage and cache eviction.
  • Make fast controller scrolling less efficient.

Please remove the additional modifier added around the grid card and keep hero prefetching centralized in GameCardLabel. If the URL fallback needs changing, it should be changed once in that centralized location.

4. Rebase onto the latest main and preserve the #91 optimizations

The PR currently conflicts with the latest main in:

  • GameCarouselView.swift
  • LibraryView.swift
  • SettingsView.swift

Could you please rebase and resolve these files manually? While resolving them, it is important to keep the following changes from main.

Carousel rendering

Latest main uses GameCarouselView.positionedGames to construct only the previous, current, and next cards.

The older implementation modified by this PR still iterates over the complete request.games collection and repeatedly uses firstIndex to calculate the distance of each game. With a large catalog, that creates unnecessary SwiftUI view evaluation and repeated array searches.

Please keep the positionedGames implementation from main. The PR’s useful behavior—opening the selected game in its expanded state—can then be added to that optimized version.

Controller navigation

Latest main introduces UIControllerNavigationCoordinator as the single owner of physical-controller handlers. It switches between these modes:

  • Tabs
  • Carousel
  • Modal
  • Streaming

Please keep this coordinator and the related modifiers such as:

  • handlesCarouselControllerNavigation
  • blocksGlobalControllerNavigation

This prevents tab shoulder-button handlers from running while the user is navigating a carousel, modal sheet, or active stream.

Game filtering

Latest main uses:

  • GameFilterEngine.matches
  • GameFilterEngine.count
  • libraryFilterBaseCount
  • storeFilterBaseCount

These changes reduce filtering to one predicate pass, avoid creating and sorting full arrays just to calculate preview counts, and cache the search-aware base totals.

Please preserve these implementations during the rebase. They are especially useful when filtering the complete catalog of several thousand games.

Focus and accessibility

Please also preserve these changes from main:

  • .defaultFocus instead of delayed focus workarounds.
  • Exact focus restoration when returning from details or filters.
  • accessibilityReduceMotion handling.
  • Carousel accessibility labels and selected-state traits.
  • Modal controller-navigation blocking.

5. Keep the new settings sheets, with a few adjustments

Replacing the tvOS Picker navigation with SettingSelectionRow and SettingSelectionSheet is a good improvement. The native picker flow could open an empty screen or leave controller focus trapped, while the modal list is more predictable.

I would like to keep this approach, but could you please add the following details?

Focus the current selection

When a selection sheet opens, it should focus or scroll to the currently selected value. This is especially important for the game-language list, where the current option may be far down the list.

A FocusState, .defaultFocus, or scroll-to-selection implementation would make the sheet faster to use with a controller.

Preserve the AV1 warning style

The previous AV1 software-decoding warning used an orange foreground style. The generic SettingSelectionRow currently renders every description using .secondary, so the warning no longer looks like a warning.

Please allow the row to accept an optional description style or warning state and keep the AV1 warning orange.

Preserve useful resolution information

The previous resolution picker separated standard TV resolutions from other options and included useful labels. The new generic list is simpler, but it should preserve any grouping or descriptions that help the user understand the available output modes.

Block global controller navigation

Please apply the modal navigation-blocking behavior from latest main to the new selection sheets. Shoulder buttons should not switch application tabs while one of these sheets is open.

Changes from this PR that should remain

I would like to keep the following changes after the rebase:

  • SettingSelectionRow and SettingSelectionSheet, with the focus and warning-style improvements above.
  • The SwiftUI .backgroundTask implementation in CloudNowApp, because it follows the SwiftUI lifecycle and avoids accessing @State before the view is installed.
  • The valid nonisolated change for the pure remoteActsAsMouse property.
  • H265 as the default codec and H264 remains available as a user-selectable compatibility option.
  • Opening the selected carousel game directly in its expanded state, because it removes one extra controller action.
  • The shorter 180 ms metadata delay if testing on a real Apple TV confirms that it does not cause animation or frame-time hitches.

Validation result

I checked the current PR head with the repository-pinned tools:

  • SwiftFormat 0.62.1: passed, 0/75 files require formatting.
  • SwiftLint 0.65.0: passed with zero violations.
  • Unsigned Release tvOS build: currently fails because of the two explicit-self errors described above.

Once the Release errors, project-file changes, duplicate prefetch, rebase conflicts and mentioned feedbacks are addressed, this PR should combine well with the newer performance and controller-navigation work already available in main.

@MonsieurNours
MonsieurNours force-pushed the improvements/settings-catalog-signing branch from 97440cf to 23c1ca3 Compare July 18, 2026 16:42
H265 (HEVC) is hardware-decoded on Apple TV 4K and is the only codec that
carries 10-bit/HDR10 through the app's custom decoder. The previous default
H264 is 8-bit only; AV1 falls back to a software SDR path. Users can still
switch manually.
- GFNStreamController: bind self to a named local (`guard let controller = self`)
  and let the main-actor Tasks bridged from the VideoSurfaceView and
  SignalingClient callbacks use it. This fixes the Release-configuration
  "Implicit use of 'self' in closure" errors while keeping the intended strong
  capture during the task, and stays SwiftFormat-stable (no redundant `self.`).
- InputSender: mark RemoteInputMode.remoteActsAsMouse nonisolated so it can be
  read from nonisolated contexts.
- CloudNowApp: register the token-refresh background task via the
  .backgroundTask scene modifier instead of reading the @State AuthManager in
  App.init, fixing the runtime "accessing State without being installed on a
  View" warning.
…heets

On tvOS the Picker pushed-detail presentation rendered a blank screen for the
stream-quality and controller settings and trapped controller focus with no way
back. Replace those pickers with a reusable SettingSelectionRow that opens a
padded modal list (SettingSelectionSheet), leaving the owenselles#92 server-location UI,
toggles and steppers untouched.

The sheet:
- focuses the currently selected value on open via .defaultFocus (important for
  the long game-language list),
- keeps the resolution TV-standards / Other grouping and SF Symbol labels,
- renders the color-mode description in orange when AV1 is selected (software
  decode warning),
- blocks global tab shoulder-button navigation while open via
  .blocksGlobalControllerNavigation().
- Seed expandedGame in GameCarouselView.init so a tapped game opens directly in
  its expanded card; focus lands on Play via the expanded detail's existing
  .defaultFocus, removing one controller action. Built on main's positionedGames
  layout and its handlesCarouselControllerNavigation(isEnabled:) gating.
- Reduce the metadata reveal delay from 360 ms to 180 ms (reduceMotion bypass
  preserved) for a snappier open.
@MonsieurNours
MonsieurNours force-pushed the improvements/settings-catalog-signing branch from 23c1ca3 to 49d497b Compare July 18, 2026 17:18
@MonsieurNours

Copy link
Copy Markdown
Author

Thanks for the thorough review! I rebased the branch onto the latest main (on top of #91 and #92) and addressed every point. Since both sides had rewritten the same files, I re-applied the useful changes onto a fresh main base rather than resolving line-by-line conflicts, so the #91 controller-navigation and #92 server-region work is preserved intact.

1. Release build errors — Fixed. I kept the strong capture during the main-actor task, but instead of [self] + self. I bind a named local:

view.onDecodedVideoFormatChanged = { [weak self] format in
    guard let controller = self else { return }
    Task { @MainActor in controller.applyDecodedVideoFormat(format) }
}

Reason: the repo's SwiftFormat (redundantSelf) strips the self. from Task { @MainActor [self] in self.foo() }, which then re-introduces the Release "Implicit use of self" error — so the literal [self] + self. form fails the lint gate. The named-local form compiles under Release and is SwiftFormat-stable, while keeping the intended strong capture. Happy to switch to [self] + self. with a // swiftformat:disable directive if you'd prefer the exact wording.

2. Xcode project file — Reverted. CloudNow.xcodeproj/project.pbxproj is now identical to main; no signing/Local.xcconfig changes in this PR.

3. Duplicate hero-art prefetch — Removed. main already prefetches centrally in GameCardLabel, and the grid button carries none, so LibraryView.swift has no change in this PR.

4. Rebase onto main — Done. positionedGames, UIControllerNavigationCoordinator (+ handlesCarouselControllerNavigation / blocksGlobalControllerNavigation), GameFilterEngine + base counts, .defaultFocus, focus restoration, accessibilityReduceMotion, and carousel accessibility traits are all preserved. The "open selected game expanded" behavior is now built on top of main's positionedGames (I only seed expandedGame in init); Play focus is handled by the expanded detail's existing .defaultFocus, so the old delayed-focus workaround is gone.

5. Settings sheets — Kept, with the requested adjustments:

  • Focus lands on the current selection when a sheet opens (.defaultFocus), which matters for the game-language list.
  • The AV1 software-decode description renders in orange again (descriptionIsWarning).
  • Resolution keeps its TV-standards / Other grouping and SF Symbol labels (the sheet supports grouped sections).
  • Sheets apply .blocksGlobalControllerNavigation(), so shoulder buttons no longer switch tabs while a sheet is open.

Kept: SettingSelectionRow/SettingSelectionSheet, the .backgroundTask lifecycle, the nonisolated remoteActsAsMouse, H265 default (H264 still selectable), opening the carousel game expanded, and the 180 ms metadata delay.

Validation: SwiftFormat (0.62.1) and SwiftLint (0.65.0) pass with zero issues on all changed files, and the app builds cleanly for tvOS. The guard let controller change removes the implicit-self usage entirely, so the Release-configuration error can no longer occur. The 180 ms metadata delay is applied and being validated on a real Apple TV 4K for smoothness.

@aarikmudgal

Copy link
Copy Markdown
Collaborator

Hi @MonsieurNours

Thanks for the work on review comments.

Did some testing and found few things.

  1. UI inconsistencies that should be fixed.
Simulator Screenshot - Apple TV 4K (3rd generation) - 2026-07-19 at 16 50 55
  1. UI overlay flickers while rendering the row for the first time.
Simulator Screenshot - Apple TV 4K (3rd generation) - 2026-07-19 at 16 51 49 Simulator Screenshot - Apple TV 4K (3rd generation) - 2026-07-19 at 16 51 50
  1. Rumble feature stopped working.

The modal selection sheets wrapped their List in a NavigationStack with
.navigationTitle. On tvOS that floating title lets scrolled rows render
through it (visible when defaultFocus scrolls to the selection) and the
navigation bar's deferred layout pass flashes the list at the wrong
offset on first presentation. Replace it with a fixed header above the
List, matching the server-location screens.

Also replace the SF Symbol Label in option rows with an explicit
fixed-width icon column so resolution icons no longer collide with
their labels.
The previous commit dropped the NavigationStack entirely, but the sheet
sizes to its content's ideal height and a bare List collapses without
it, leaving only the floating title. Keep the NavigationStack for
sizing, suppress its floating title with .navigationTitle(""), and
render the fixed header above the List — the same pattern
ServerPickerScreen already uses.
@MonsieurNours

Copy link
Copy Markdown
Author

Thanks for testing!

Points 1 and 2 are fixed in 8f71a11: the selection sheets no longer use a floating navigation title — the list was scrolling under it (the ghosted "Resolution" text), and the navigation bar's deferred layout pass was also causing the first-render flicker. The sheets now use the same fixed-header pattern as the server-location screens. The resolution icons also got proper spacing instead of colliding with their labels.

Point 3 (rumble): I couldn't reproduce it — just retested on this branch and rumble works as expected on my setup. Nothing in this PR touches the input/rumble path (InputSender/control channel). Could you share which controller and game you tested with, and whether rumble also fails for you on main? That would help narrow down whether it's related to this branch at all.

@MonsieurNours MonsieurNours reopened this Jul 20, 2026
@aarikmudgal

Copy link
Copy Markdown
Collaborator

@MonsieurNours Thanks for the changes. No BLOCKER or MAJOR code defects left, but I recommend keeping the review pending for two minor fixes and final runtime verification.

Findings

MINOR — Restore selected-state accessibility

In CloudNow/UI/SettingsView.swift, around lines 718–736, the custom option buttons show selection only through a green checkmark. This loses the selected-state accessibility semantics previously supplied by the native Picker.

Please:

  • Add the .isSelected accessibility trait to the active option.
  • Hide the decorative resolution and checkmark symbols from accessibility.
  • Ensure VoiceOver announces the option label without duplicating the SF Symbol descriptions.

For example:

.accessibilityAddTraits(option.value == selection ? .isSelected : [])

MINOR — Propagate background-task cancellation

The .backgroundTask implementation in CloudNowApp.swift correctly moves registration into the SwiftUI lifecycle. However, AuthManager.refresh(session:) creates and awaits an independent, coalesced Task.

When the system expires and cancels the SwiftUI background handler, that cancellation does not automatically cancel the underlying refresh task. Network refresh work may therefore continue after the permitted background execution window has ended.

Please make the refresh ownership and cancellation behavior explicit. A background-owned refresh should stop on expiration, while a refresh already shared with a foreground caller should not be canceled accidentally.

MINOR — Update the branch from current main

The PR head (8f71a11) is based on 92ff05c, while current main is now 7cd6dc4 from PR #94.

PR #94 also modifies GFNStreamController.swift. The automatic merge is currently conflict-free, and I successfully built the merged result, but the branch should still be updated so the final PR head and CI represent the code that will actually be merged.

Previous feedback status

Review point Status
Release explicit-self errors ✅ Fixed; unsigned Release build passes
Xcode project/signing changes ✅ Removed; no project.pbxproj diff
Duplicate hero-art prefetch ✅ Removed; LibraryView.swift is unchanged
PR #91 controller/catalog optimizations ✅ Preserved
positionedGames carousel optimization ✅ Preserved
Exact focus restoration and .defaultFocus ✅ Preserved
Reduce Motion handling ✅ Preserved
Carousel accessibility ✅ Preserved
Modal controller-navigation blocking ✅ Preserved
Current selection focused in sheets ✅ Implemented
AV1 orange warning ✅ Preserved
Resolution grouping, badges, and icons ✅ Preserved
H.265 default with H.264 compatibility ✅ Implemented
Existing stored codec selections ✅ Preserved
Carousel opens directly in expanded state ✅ Implemented
180 ms metadata reveal ✅ Implemented
UI overlap and first-render flicker ⚠️ Structurally addressed; runtime retest still required
Reported rumble regression ⚠️ No causal code change found; Testing needs to be confirmed one final time

UI overlap and flicker

Commits 96a4086 and 8f71a11 structurally address the reported UI problems:

  • The floating navigation title was replaced with a fixed header.
  • The empty navigation title prevents rows from rendering beneath the old title.
  • NavigationStack remains in place to preserve the sheet’s correct sizing.
  • Resolution icons now use a fixed-width column and spacing.

The implementation matches the reported cause, but first-presentation flicker is runtime behavior and cannot be conclusively verified through compilation or static review. Please repeat the original simulator or Apple TV interaction before closing this point.

Rumble regression

I did not find a code change in this PR that explains a rumble-only regression:

  • Rumble enablement and intensity are unchanged.
  • ControllerHaptics and haptic packet handling are unchanged.
  • The only InputSender change is the nonisolated annotation on a pure computed property, which has no runtime behavior.
  • H.264 and H.265 continue using the same input protocol.
  • Codec filtering affects video negotiation, not the input/haptics data channel.

Please A/B test using:

  1. The same Apple TV, controller, and game on current main.
  2. The same setup on PR head 8f71a11.
  3. PR head with H.264 selected explicitly, to exclude the new H.265 default. (I will do the test once again myself)

Final verdict

There are no known BLOCKER or MAJOR code defects.

Before approval, I recommend:

  1. Rebasing the branch on current main. (Auto rebase should work)
  2. Restoring selected-state accessibility in the custom setting sheets.
  3. Making background refresh cancellation propagate correctly.
  4. Repeating the original sheet overlap/flicker scenario.
  5. Completing the rumble A/B test against current main.

Once these points are addressed or conclusively verified, the PR should be ready for approval and merge

@aarikmudgal
aarikmudgal marked this pull request as draft July 31, 2026 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants