Skip to content

[Agent] feat(tvOS): retrowave design language consistency wave 2 - #3611

Merged
github-actions[bot] merged 4 commits into
developfrom
agent/issue-3598-wave2
Aug 13, 2026
Merged

[Agent] feat(tvOS): retrowave design language consistency wave 2#3611
github-actions[bot] merged 4 commits into
developfrom
agent/issue-3598-wave2

Conversation

@github-actions

Copy link
Copy Markdown
Contributor

Summary

Continues the retrowave design language consistency work from #3599, applying the shared RetroSettingsComponents to the next batch of settings views.

Fixed: RetroAchievementsView

  • Added tvOSSettingsHorizontalPadding() (80pt) so content clears the side-menu bar on tvOS
  • Replaced .buttonStyle(.card) + .retroThemedFocus with .retroFocusButtonStyle on the login and disconnect buttons

Fixed: ControllerSettingsView

  • Replaced hardcoded gradient List background with RetroSettingsBackground (black + RetroGrid)
  • scrollContentBackground(.hidden) so the grid shows through the list on tvOS
  • .padding(.horizontal, 80) for sidebar inset on tvOS
  • All tvOS section headers upgraded to RetroSettingsSectionHeader (Button Remapping, Controller Assignments, Preferred Player Slots, Preferences, Help & Guides)
  • .buttonStyle(.card) + .retroThemedFocus replaced with retroFocusButtonStyle on player-assignment and preference buttons

Fixed: BackupRestoreView

  • Replaced inline Color.black + RetroGrid() ZStack with RetroSettingsBackground
  • tvOSSettingsHorizontalPadding() for sidebar inset on tvOS
  • Backup/Restore action buttons migrated to RetroSettingsActionButton — previously had no tvOS focus styling at all; now get consistent retrowave border + built-in retroFocusButtonStyle

Fixed: BatchArtworkMatchingView

  • tvOSSettingsHorizontalPadding() on filter controls and action buttons
  • All .buttonStyle(.card) + .retroThemedFocus usages replaced with retroFocusButtonStyle (Find Artwork, Retry, Refresh, Clear/Apply Selected)

Remaining work (follow-up PRs)

Systems, Cores, Theme, Core Language, Core Options, RetroArch Settings, Cloud Sync, Controller Profiles, Import Saves, Rumble/Deadzone sliders — see #3598.

Part of #3598

Test plan

  • RetroAchievements on tvOS: content clears side-menu bar; login/disconnect use gradient focus ring
  • Controller Settings on tvOS: retrowave grid background visible; section headers are pink+uppercase; player rows and reset button use gradient focus ring
  • Backup & Restore on tvOS: retrowave grid visible; Backup/Restore buttons use gradient focus ring and themed border
  • Batch Artwork on tvOS: content has 80pt inset; action buttons use gradient focus ring

🤖 Generated with Claude Code

@github-actions
github-actions Bot requested a review from JoeMatt as a code owner March 29, 2026 23:47
@github-actions
github-actions Bot enabled auto-merge (squash) March 29, 2026 23:47
@github-actions github-actions Bot added agent-work PR or issue being worked on by the AI agent ai-reviewing AI code review in progress labels Mar 29, 2026
@github-actions

Copy link
Copy Markdown
Contributor Author

Robot PR created. AI review starting automatically.

@JoeMatt

JoeMatt commented Mar 29, 2026

Copy link
Copy Markdown
Member

@claude please do a self-review pass on this PR:

Verify all changed files compile (check for unresolved symbols, missing imports, type mismatches)
Check for any dead code, unused variables, or missing test coverage
Ensure multi-platform compatibility (iOS + tvOS + macOS Catalyst)
Fix all issues
Be throgough

@github-actions

github-actions Bot commented Mar 29, 2026

Copy link
Copy Markdown
Contributor Author

Review Complete

Self-review finished. Found and fixed 3 issues (dead code x2, alert binding anti-pattern). PR is ready for human review.

See the full review summary in the PR comments above.

@github-actions

Copy link
Copy Markdown
Contributor Author

Self-Review Results — Fixes Applied

Issues Found & Fixed

Critical — Compile Error

  • RetroAchievementsView.swift: Removed duplicate ShareSheet struct that redeclared the type already defined in ShareSheet.swift. This caused an "Invalid redeclaration of 'ShareSheet'" compile error.

Dead Code Removed

  • RetroAchievementsView.swift: Removed duplicate RetroToggleStyle struct (already exists as RetroTheme.RetroToggleStyle). Updated all call sites to use RetroTheme.RetroToggleStyle().
  • RetroAchievementsView.swift: Removed unused currentUser variable (fetched but never read).
  • BatchArtworkMatchingView.swift: Removed unused @State private var scanlineOffset: CGFloat = 0.

Visual Bug Fixed

  • BatchArtworkMatchingView.swift: Refresh button label was missing .foregroundColor(.white) — text/icon rendered with system default color against a dark gradient background, making it invisible/illegible.

Deprecated API Warnings Fixed (iOS 17)

  • RetroAchievementsView.swift: Two onChange(of:) { newValue in } calls updated to two-param form { _, newValue in }.
  • BackupRestoreView.swift: Two onChange(of:) { state in } calls updated to { _, state in }.

Unnecessary Annotation Removed

  • RetroAchievementsView.swift: Dropped @available(iOS 15.0, tvOS 15.0, macOS 12.0, *) — minimum deployment target is already iOS 17/tvOS 17.

Files Not Modified

  • ControllerSettingsView.swift: No issues found — compiles correctly on all platforms, platform guards are correct, no dead code.

Multi-Platform Check

All changes verified safe for iOS, tvOS, and macOS Catalyst. Platform guards (#if os(tvOS), #if !os(tvOS)) are intact and correct.

@github-actions github-actions Bot removed the ai-reviewing AI code review in progress label Mar 30, 2026
@github-actions
github-actions Bot force-pushed the agent/issue-3598-wave2 branch 4 times, most recently from 4e77171 to ebe085d Compare April 1, 2026 01:07
@JoeMatt

JoeMatt commented Apr 1, 2026

Copy link
Copy Markdown
Member

@claude please do a self-review pass on this PR:

Verify all changed files compile (check for unresolved symbols, missing imports, type mismatches)
Check for any dead code, unused variables, or missing test coverage
Ensure multi-platform compatibility (iOS + tvOS + macOS Catalyst)
Fix all issues
Be throgough

@github-actions

github-actions Bot commented Apr 1, 2026

Copy link
Copy Markdown
Contributor Author

Self-Review Results

Reviewed all 4 changed files for compilation correctness, dead code, platform compatibility, and type safety. Found and fixed 3 issues:

Fixed Issues

1. Dead code — currentSession state variable (RetroAchievementsView.swift)
@State private var currentSession: RAUserSession? was declared but never read or written after the refactor to userProfile. Removed.

2. Dead code — moveTVOSBackup(from:) function (BackupRestoreView.swift)
This #if os(tvOS) private method was left over from a previous implementation before BackupCoordinator took over responsibility for moving the backup file. It was never called. Removed.

3. Alert binding anti-pattern (BatchArtworkMatchingView.swift)
.constant(errorMessage != nil) was used as the isPresented binding for uiKitAlert. This creates a read-only constant snapshot binding — UIKit cannot write false back through it to dismiss the alert, so tapping outside or using system dismiss would cause the alert to reappear. Fixed by adding a proper @State private var showErrorAlert = false and updating all error-setting paths to set showErrorAlert = true.

Also Simplified

initializeClient in RetroAchievementsView.swift — the function unnecessarily wrapped code in await MainActor.run { Task { ... } } when the function itself is already @MainActor async. Simplified to a flat synchronous flow (since isAuthenticated is a plain Bool property, not async).

No Issues Found

  • All type references resolve correctly (RAUserSession, RAUserProfile, UserProfile, RetroAchievementsClient APIs)
  • Platform guards are correct throughout (#if os(tvOS), #if !os(tvOS), #if os(iOS) || targetEnvironment(macCatalyst))
  • No missing imports
  • Multi-platform compatibility verified for iOS, tvOS, and macOS Catalyst
  • ControllerSettingsView.swift — no issues found

@github-actions github-actions Bot added the ready-for-review AI approved; awaiting human review label Apr 1, 2026
@github-actions
github-actions Bot force-pushed the agent/issue-3598-wave2 branch 12 times, most recently from 9f355e9 to 99ea2dd Compare April 3, 2026 03:01
@github-actions
github-actions Bot force-pushed the agent/issue-3598-wave2 branch 9 times, most recently from 3bca7c8 to d00fd92 Compare April 20, 2026 19:08
@github-actions
github-actions Bot force-pushed the agent/issue-3598-wave2 branch 2 times, most recently from 3170a30 to d56836a Compare April 23, 2026 21:37
@github-actions

github-actions Bot commented May 1, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Merge conflict detected — Claude has been dispatched to resolve the conflicts automatically.

If conflicts cannot be resolved cleanly, the workflow will post a comment explaining what needs manual attention.

@github-actions

github-actions Bot commented May 1, 2026

Copy link
Copy Markdown
Contributor Author

✅ Fixes Pushed — Awaiting Re-review

Progress

  • 🚀 Agent started
  • 📖 Read Copilot review comments (3 inline comments)
  • 🔨 Applied 3 fixes (tvOS inset consistency)
  • 📤 Pushed changes (85cd691)
  • 🤖 Awaiting AI re-review

Updated: 2026-08-05 01:05 UTC

@github-actions
github-actions Bot force-pushed the agent/issue-3598-wave2 branch from d56836a to f74b624 Compare May 1, 2026 08:06
@github-actions

github-actions Bot commented May 1, 2026

Copy link
Copy Markdown
Contributor Author

Rebase Conflict Resolved ✅

Resolved 1 conflict in PVUI/Sources/PVSwiftUI/Settings/RetroAchievementsView.swift and force-pushed the rebased branch.

What happened

The develop branch gained a RetroSettingsToggleRow struct (using the old RetroToggleStyle()) plus a standalone RetroToggleStyle definition. The PR's self-review commit (11a96ed49) was written before these landed in develop, so it removed the RetroToggleStyle duplicate without knowing RetroSettingsToggleRow would be present.

Resolution

  • Kept RetroSettingsToggleRow (it's referenced in retroArchConfigSection)
  • Updated its toggle style to RetroTheme.RetroToggleStyle() (the canonical definition)
  • Removed redundant @available(iOS 15.0, tvOS 15.0, macOS 12.0, *) annotation (min target is iOS 17)
  • Fixed onChange(of:) { newValue in }{ _, newValue in } (iOS 17 two-parameter form)
  • Dropped the standalone RetroToggleStyle struct (duplicate of RetroTheme.RetroToggleStyle)

@github-actions
github-actions Bot force-pushed the agent/issue-3598-wave2 branch 5 times, most recently from 9871654 to 82ab659 Compare May 4, 2026 02:50
@JoeMatt

JoeMatt commented May 19, 2026

Copy link
Copy Markdown
Member

👋 Triage pass — this [Agent] PR has been open ~50-60 days and develop has moved significantly since (controller-mapping refactor, thin-wrapper turbo/lightgun, Metal viewport fix, Sentry filter, MAME button suppression). It will almost certainly need a rebase before it can merge.

Please react on this comment so I know what to do:

  • 👍 — still wanted, please rebase against develop and resolve conflicts (I can run @claude rebase if you tag me)
  • 👎 — close as superseded / no longer needed
  • 🚀 — high priority, I'll pick up the rebase myself this week
  • 👀 — leave open, you'll come back to it

No reaction within 7 days → I'll close as stale. Re-open any time if you change your mind.

@github-actions

Copy link
Copy Markdown
Contributor Author

👍 Rebased — Ready

The branch had no common ancestor with develop (independent history root), so a standard rebase wasn't possible. Instead I cherry-picked the 3 feature commits onto a fresh develop base — all three applied with zero conflicts.

What changed since the original branch

The only develop work that had any overlap potential was the controller-mapping refactor (refactor(controls): move ButtonIdentifier/Mapping to PVCoreBridge). It didn't touch any of the 4 files in this PR (RetroAchievementsView, ControllerSettingsView, BackupRestoreView, BatchArtworkMatchingView), so the cherry-pick was clean.

PR branch now

  • Base: origin/develop (tip: 9dd68d223)
  • 3 commits ahead, 0 behind
  • No merge conflicts

CI will re-run automatically against the new base. The PR is still ready-for-review.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR continues the tvOS retrowave design consistency work by applying the shared RetroSettingsComponents patterns (background, section headers, focus button styling, and tvOS sidebar insets) to additional Settings views in the PVUI SwiftUI layer.

Changes:

  • Updates multiple settings views to use retroFocusButtonStyle on tvOS (replacing .buttonStyle(.card) + .retroThemedFocus) for consistent focus appearance.
  • Standardizes tvOS section headers and backgrounds using RetroSettingsSectionHeader and RetroSettingsBackground.
  • Introduces/expands use of tvOSSettingsHorizontalPadding() to keep content clear of the tvOS side-menu bar and improves error alert presentation in Batch Artwork matching.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
PVUI/Sources/PVSwiftUI/Settings/Views/ControllerSettingsView.swift Switches tvOS section headers/focus styling and updates list background to RetroSettingsBackground with tvOS inset.
PVUI/Sources/PVSwiftUI/Settings/Views/BatchArtworkMatchingView.swift Applies retrowave focus styling to buttons, adds tvOS inset usage, and fixes alert presentation state binding.
PVUI/Sources/PVSwiftUI/Settings/Views/BackupRestoreView.swift Migrates background and action buttons to shared retrowave components and modernizes onChange closures.
PVUI/Sources/PVSwiftUI/Settings/RetroAchievementsView.swift Adds tvOS sidebar inset and migrates focus/toggle styling toward shared retrowave patterns; simplifies client initialization logic.

Comment on lines +100 to +101
.padding()
.tvOSSettingsHorizontalPadding()
Comment on lines +85 to +97
// Filter controls
filterControls
.tvOSSettingsHorizontalPadding()
#if !os(tvOS)
.padding(.horizontal)
#endif

// Action buttons
actionButtons
.tvOSSettingsHorizontalPadding()
#if !os(tvOS)
.padding(.horizontal)
#endif
Comment thread PVUI/Sources/PVSwiftUI/Settings/Views/ControllerSettingsView.swift Outdated
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

✅ Fixes pushed — addressed all 3 Copilot review comments:

  • RetroAchievementsView.swift.padding() is now vertical-only on tvOS so it no longer stacks with tvOSSettingsHorizontalPadding()'s 80pt inset (was ~96pt, now the standard 80pt).
  • BatchArtworkMatchingView.swift — moved tvOSSettingsHorizontalPadding() to the container VStack so the title, games list, and bottom selection action bar all get the tvOS side-menu inset, not just the filter/action controls.
  • ControllerSettingsView.swift — replaced the hardcoded .padding(.horizontal, 80) with the shared tvOSSettingsHorizontalPadding() modifier to avoid duplicating the magic number.

Commit: 85cd691

github-actions Bot and others added 4 commits August 12, 2026 21:54
…ler, Backup, BatchArtwork

- RetroAchievementsView: tvOS 80pt inset + retroFocusButtonStyle on login/disconnect
- ControllerSettingsView: RetroSettingsBackground, RetroSettingsSectionHeader on all
  sections, retroFocusButtonStyle replaces .card/.retroThemedFocus throughout
- BackupRestoreView: RetroSettingsBackground + tvOSSettingsHorizontalPadding; migrate
  Backup/Restore buttons to RetroSettingsActionButton
- BatchArtworkMatchingView: tvOSSettingsHorizontalPadding + retroFocusButtonStyle on
  all action buttons

Part of #3598

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Remove duplicate ShareSheet definition (already in ShareSheet.swift),
  which caused an "Invalid redeclaration" compile error
- Remove duplicate RetroToggleStyle struct; use RetroTheme.RetroToggleStyle()
- Drop unused currentUser variable and unused scanlineOffset @State var
- Fix deprecated onChange(of:) single-param closures to two-param form (iOS 17)
- Remove redundant @available(iOS 15.0,...) annotation (min target is iOS 17)
- Add missing .foregroundColor(.white) on Refresh button in BatchArtworkMatchingView

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Remove unused @State currentSession in RetroAchievementsView
- Simplify initializeClient: remove unnecessary nested Task inside MainActor.run
- Remove dead moveTVOSBackup(from:) in BackupRestoreView (never called)
- Fix .constant(errorMessage != nil) anti-pattern in BatchArtworkMatchingView
  with proper @State showErrorAlert so the alert can dismiss correctly

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- RetroAchievementsView: avoid double horizontal inset by making
  .padding() vertical-only on tvOS since tvOSSettingsHorizontalPadding()
  already supplies the 80pt side inset.
- BatchArtworkMatchingView: move tvOSSettingsHorizontalPadding() to the
  container VStack so title, list content, and the bottom selection
  bar all get the inset, not just filterControls/actionButtons.
- ControllerSettingsView: replace hardcoded .padding(.horizontal, 80)
  with the shared tvOSSettingsHorizontalPadding() modifier.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-work PR or issue being worked on by the AI agent

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants