feat(medium): Fix: Spotify 'Connect' button persists after successful connection

[arii]

Description

The Spotify control section on the /client/control page incorrectly showed the 'Connect' CTA instead of playback controls when a device was active but no track was playing. This occurred because the UI state relied on track metadata rather than device availability. This PR updates the UI logic to correctly display playback controls when a Spotify device is active, even if no track is currently playing, and improves device refreshing.

Fixes #9305

Change Type: 🐛 Bug fix (non-breaking change fixing an issue)

Changes Made

1. Robust UI State Logic: Updated SpotifyControls.tsx to prioritize the spotifyServiceInitialized flag and the presence of any active devices. Controls are now shown if the service is ready AND (track data exists OR an active device exists).
2. Force Device Refresh: Added a window focus listener that sends a GET_DEVICES command when the tab regains focus, resolving stale device lists.
3. UX/Accessibility Improvement: Renamed the button to "Connect to HRM Web Player" and implemented a "Searching for Web Player..." state with a loading spinner when the service is not yet initialized.

Testing

Verified with unit tests (including a new reproduction test) and visual verification via Playwright.

Related Issues

Closes #9305

Original PR Body

The Spotify control section on the /client/control page incorrectly showed the 'Connect' CTA instead of playback controls when a device was active but no track was playing. This occurred because the UI state relied on track metadata rather than device availability.

This PR implements the following fixes:

1. Robust UI State Logic: Updated SpotifyControls.tsx to prioritize the spotifyServiceInitialized flag and the presence of any active devices. Controls are now shown if the service is ready AND (track data exists OR an active device exists).
2. Force Device Refresh: Added a window focus listener that sends a GET_DEVICES command when the tab regains focus, resolving stale device lists.
3. UX/Accessibility Improvement: Renamed the button to "Connect to HRM Web Player" and implemented a "Searching for Web Player..." state with a loading spinner when the service is not yet initialized.

Verified with unit tests (including a new reproduction test) and visual verification via Playwright.

Fixes #9305

---

PR created automatically by Jules for task 5529974195545826471 started by @arii

[google-labs-jules]

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.

---

For security, I will only act on instructions from the user who triggered this task.

[github-actions]

👋 Welcome to HRM!

Thanks for your contribution. This repository uses Gemini AI for automated triage, code review, and generation.

🤖 Gemini Manual Trigger Quick Reference

Command	Action
@gemini-bot	Run AI Code Review (PR only)
@gemini-triage	Run Issue Triage
@gemini-coder <task>	Generate Code
@create-review-issues	Create issues from review (PR only)
@gemini-help	Show this help message
@pr-squash	Squash PR commits (PR only)
@conflict-resolve	Resolve merge conflicts (PR only)

For more details and GitHub CLI examples, see the Manual Trigger Guide.

[github-actions]

ℹ️ Gemini Review Disabled

Automatic review is currently disabled via GEMINI_ENABLE_PR_REVIEW. To run review manually:

Via Comment:

@gemini-bot

Via GitHub Actions:

1. Go to Actions → Reusable Gemini Review
2. Click "Run workflow"
3. Enter PR number: 9549

Via GitHub CLI:

gh workflow run reusable-gemini-review.yml -f pr_number=9549

🤖 Manual Trigger Quick Reference

Command	Action
@gemini-bot	Run AI Code Review (PR only)
@gemini-triage	Run Issue Triage
@gemini-coder <task>	Generate Code
@create-review-issues	Create issues from review (PR only)
@gemini-help	Show this help message

Full reference: Manual Trigger Guide

To re-enable: Set repository variable GEMINI_ENABLE_PR_REVIEW=true

[github-actions]

🤖 Gemini Review Skipped

Review was skipped for the following reason: Gemini review is globally disabled

🤖 Manual Trigger Quick Reference

Command	Action
@gemini-bot	Run AI Code Review (PR only)
@gemini-triage	Run Issue Triage
@gemini-coder <task>	Generate Code
@create-review-issues	Create issues from review (PR only)
@gemini-help	Show this help message

Manual Trigger Guide

[github-actions]

🤖 Gemini Review Skipped

Review was skipped for the following reason: Gemini review is globally disabled

🤖 Manual Trigger Quick Reference

Command	Action
@gemini-bot	Run AI Code Review (PR only)
@gemini-triage	Run Issue Triage
@gemini-coder <task>	Generate Code
@create-review-issues	Create issues from review (PR only)
@gemini-help	Show this help message

Manual Trigger Guide

[github-actions]

🤖 Gemini Review Skipped

Review was skipped for the following reason: Gemini review is globally disabled

🤖 Manual Trigger Quick Reference

Command	Action
@gemini-bot	Run AI Code Review (PR only)
@gemini-triage	Run Issue Triage
@gemini-coder <task>	Generate Code
@create-review-issues	Create issues from review (PR only)
@gemini-help	Show this help message

Manual Trigger Guide

[arii]

@Gemini-bot

[arii]

@Gemini-bot

[arii]

Reviewed commit: c7da47942f95337b2417c9714405c93d5005462e

This PR effectively addresses the reported bug where the Spotify 'Connect' button persisted after a successful connection, and also introduces valuable UX improvements.

Key Improvements:

1. Bug Fix (app/client/control/components/SpotifyControls.tsx): The introduction of the shouldShowControls variable correctly determines when to display Spotify playback controls. By checking spotifyServiceInitialized and whether hasSpotifyData or devices.some((d) => d.is_active) is true, the component now accurately reflects the Spotify connection status, even if a track isn't actively playing. This directly resolves the issue of the 'Connect' button persisting.
2. Enhanced 'Connect' Button UX: The 'Connect to HRM Web Player' button now provides clear visual feedback. It displays a CircularProgress indicator and changes text/background color when spotifyServiceInitialized is false, indicating that the service is searching for the web player. This significantly improves the user experience during the connection phase.
3. Device Refresh on Tab Focus (app/client/control/components/SpotifyControls.tsx): The new useEffect hook to force a device refresh when the tab regains focus is a thoughtful enhancement. This ensures that the device list is always up-to-date, preventing stale device information, especially after the browser might have been in the background.
4. Comprehensive Testing:
   • A new unit test (tests/unit/app/client/control/components/SpotifyControls.repro.test.tsx) has been added to specifically reproduce and verify the bug fix, which is excellent. It clearly demonstrates the expected behavior.
   • The Playwright VRTs were updated to use data-testid for the 'Select Music' button, improving test stability and robustness.

The changes are well-implemented, follow project guidelines, and are thoroughly tested. Great work!

Reviewed at commit: c7da47942f95337b2417c9714405c93d5005462e

---

🤖 Gemini Manual Trigger Quick Reference

Command	Action
@gemini-bot	Run AI Code Review (PR only)
@gemini-triage	Run Issue Triage
@gemini-coder <task>	Generate Code
@create-review-issues	Create issues from review (PR only)
@gemini-help	Show this help message

Manual Trigger Guide

[google-labs-jules]

Reviewed commit: c7da47942f95337b2417c9714405c93d5005462e

This PR effectively addresses the reported bug where the Spotify 'Connect' button persisted after a successful connection, and also introduces valuable UX improvements.

Key Improvements:

1. Bug Fix (app/client/control/components/SpotifyControls.tsx): The introduction of the shouldShowControls variable correctly determines when to display Spotify playback controls. By checking spotifyServiceInitialized and whether hasSpotifyData or devices.some((d) => d.is_active) is true, the component now accurately reflects the Spotify connection status, even if a track isn't actively playing. This directly resolves the issue of the 'Connect' button persisting.
2. Enhanced 'Connect' Button UX: The 'Connect to HRM Web Player' button now provides clear visual feedback. It displays a CircularProgress indicator and changes text/background color when spotifyServiceInitialized is false, indicating that the service is searching for the web player. This significantly improves the user experience during the connection phase.
3. Device Refresh on Tab Focus (app/client/control/components/SpotifyControls.tsx): The new useEffect hook to force a device refresh when the tab regains focus is a thoughtful enhancement. This ensures that the device list is always up-to-date, preventing stale device information, especially after the browser might have been in the background.
4. Comprehensive Testing:
   • A new unit test (tests/unit/app/client/control/components/SpotifyControls.repro.test.tsx) has been added to specifically reproduce and verify the bug fix, which is excellent. It clearly demonstrates the expected behavior.
   • The Playwright VRTs were updated to use data-testid for the 'Select Music' button, improving test stability and robustness.

The changes are well-implemented, follow project guidelines, and are thoroughly tested. Great work!

Reviewed at commit: c7da47942f95337b2417c9714405c93d5005462e

---

🤖 Gemini Manual Trigger Quick Reference

Command	Action
@gemini-bot	Run AI Code Review (PR only)
@gemini-triage	Run Issue Triage
@gemini-coder <task>	Generate Code
@create-review-issues	Create issues from review (PR only)
@gemini-help	Show this help message

Manual Trigger Guide

Thank you for the review and the detailed summary of the improvements. Acknowledged.

[github-actions]

🤖 Gemini Review Skipped

Review was skipped for the following reason: Gemini review is globally disabled

🤖 Manual Trigger Quick Reference

Command	Action
@gemini-bot	Run AI Code Review (PR only)
@gemini-triage	Run Issue Triage
@gemini-coder <task>	Generate Code
@create-review-issues	Create issues from review (PR only)
@gemini-help	Show this help message

Manual Trigger Guide

[arii]

🤖 AI Technical Audit

Code Review: Spotify 'Connect' button persistence fix

Overall, the core logic changes address the UX issue perfectly. Transitioning the display logic from hasSpotifyData to shouldShowControls properly accounts for the disconnected state when a device is active but playback is halted. However, there are a few areas concerning rate-limiting, accessibility, and test hygiene that need to be addressed.

🚨 ANTI-AI-SLOP DIRECTIVES

1. OVERLY VERBOSE COMMENTS: Flagged in tests/unit/app/client/control/components/SpotifyControls.repro.test.tsx (Lines 79-91). The test contains repetitive, LLM-generated narrative comments explaining 'Buggy' vs 'Fixed' behavior multiple times. This adds no value to the final codebase.
2. OVER-ENGINEERING: The window.focus event listener in SpotifyControls.tsx fires immediately without debouncing. While simple, this is a dangerous pattern for WebSocket/API triggers. Rapidly switching tabs can cause an event storm against the Spotify API.
3. DUPLICATE HOOKS/TYPES: None detected. Standard React hooks are utilized appropriately.
4. CODE RATIO: The PR adds ~120 lines (mostly tests). I have identified >10 lines of narrative comments in the test file that MUST be deleted to maintain code density.
5. STALE FEATURES: The old button state logic is successfully replaced. No orphaned features detected.

---

📂 File-by-File Analysis

app/client/control/components/SpotifyControls.tsx

• Problem 1 (Performance / Over-engineering): The window.focus listener blindly dispatches GET_DEVICES every time the window regains focus. If a user rapidly Alt+Tabs, this will spam the WebSocket and potentially hit Spotify API rate limits.
  • Implementation Sample:

    // Wrap the execution in a simple debounce or throttle
    useEffect(() => {
      let timeoutId: NodeJS.Timeout;
      const handleFocus = () => {
        clearTimeout(timeoutId);
        timeoutId = setTimeout(() => {
          if (connectionStatus === 'Connected' && spotifyServiceInitialized) {
            sendData({ type: 'SPOTIFY_COMMAND', command: 'GET_DEVICES' })
          }
        }, 1000); // 1-second debounce
      }
      window.addEventListener('focus', handleFocus)
      return () => {
        window.removeEventListener('focus', handleFocus)
        clearTimeout(timeoutId)
      }
    }, [connectionStatus, spotifyServiceInitialized, sendData])

• Problem 2 (Accessibility / HTML Validation): You are using disabled={!spotifyServiceInitialized} alongside aria-disabled={!spotifyServiceInitialized}. According to W3C ARIA specs, setting disabled inherently conveys the disabled state to screen readers. Adding aria-disabled on a native <button> element that is already disabled is redundant and can cause double-reading bugs in certain screen readers. Remove aria-disabled.

tests/unit/app/client/control/components/SpotifyControls.repro.test.tsx

• Problem (Code Ratio / Verbose Comments): Remove the redundant story-time comments. Tests should be self-documenting.
  • Implementation Sample (Lines to Delete):

    // DELETE THE FOLLOWING LINES:
    // CURRENT BEHAVIOR (Buggy):
    // The "Select Music" button is shown because hasSpotifyData is false
    // EXPECTED BEHAVIOR (Fixed):
    // Playback controls should be shown because an active device exists
    
    // EXPECTED BEHAVIOR (Fixed):
    // Playback controls should be shown because an active device exists

---

🏗️ Architectural Impact

Moving the source of truth from strictly Track Data to a hybrid of Initialization + Device Activity creates a much more resilient UI state. The addition of the component-level window focus listener is acceptable here since it's restricted to this control view, but keep an eye out: if we start adding window.addEventListener('focus', ...) across multiple components, we should abstract this to a global useWindowFocusTracker hook to centralize the event handling.

🛡️ GitHub Checks Correlation

• All Quality Gates (Lint, Knip, Scripts) passed, indicating no unused variables or type errors were introduced by the new shouldShowControls logic.
• The Visual Tests passed. The snapshot diffs (select-music-button-hover-chromium-linux.png and control-panel-chromium-linux.png) directly correlate to your changes adding the loading state and renaming the CTA. The visual regression outputs are expected and verified.

Review automatically published via RepoAuditor.

[google-labs-jules]

🤖 AI Technical Audit

Code Review: Spotify 'Connect' button persistence fix

Overall, the core logic changes address the UX issue perfectly. Transitioning the display logic from hasSpotifyData to shouldShowControls properly accounts for the disconnected state when a device is active but playback is halted. However, there are a few areas concerning rate-limiting, accessibility, and test hygiene that need to be addressed.

🚨 ANTI-AI-SLOP DIRECTIVES

1. OVERLY VERBOSE COMMENTS: Flagged in tests/unit/app/client/control/components/SpotifyControls.repro.test.tsx (Lines 79-91). The test contains repetitive, LLM-generated narrative comments explaining 'Buggy' vs 'Fixed' behavior multiple times. This adds no value to the final codebase.
2. OVER-ENGINEERING: The window.focus event listener in SpotifyControls.tsx fires immediately without debouncing. While simple, this is a dangerous pattern for WebSocket/API triggers. Rapidly switching tabs can cause an event storm against the Spotify API.
3. DUPLICATE HOOKS/TYPES: None detected. Standard React hooks are utilized appropriately.
4. CODE RATIO: The PR adds ~120 lines (mostly tests). I have identified >10 lines of narrative comments in the test file that MUST be deleted to maintain code density.
5. STALE FEATURES: The old button state logic is successfully replaced. No orphaned features detected.

---

📂 File-by-File Analysis

app/client/control/components/SpotifyControls.tsx

• Problem 1 (Performance / Over-engineering): The window.focus listener blindly dispatches GET_DEVICES every time the window regains focus. If a user rapidly Alt+Tabs, this will spam the WebSocket and potentially hit Spotify API rate limits.
  • Implementation Sample:

    // Wrap the execution in a simple debounce or throttle
    useEffect(() => {
      let timeoutId: NodeJS.Timeout;
      const handleFocus = () => {
        clearTimeout(timeoutId);
        timeoutId = setTimeout(() => {
          if (connectionStatus === 'Connected' && spotifyServiceInitialized) {
            sendData({ type: 'SPOTIFY_COMMAND', command: 'GET_DEVICES' })
          }
        }, 1000); // 1-second debounce
      }
      window.addEventListener('focus', handleFocus)
      return () => {
        window.removeEventListener('focus', handleFocus)
        clearTimeout(timeoutId)
      }
    }, [connectionStatus, spotifyServiceInitialized, sendData])

• Problem 2 (Accessibility / HTML Validation): You are using disabled={!spotifyServiceInitialized} alongside aria-disabled={!spotifyServiceInitialized}. According to W3C ARIA specs, setting disabled inherently conveys the disabled state to screen readers. Adding aria-disabled on a native <button> element that is already disabled is redundant and can cause double-reading bugs in certain screen readers. Remove aria-disabled.

tests/unit/app/client/control/components/SpotifyControls.repro.test.tsx

• Problem (Code Ratio / Verbose Comments): Remove the redundant story-time comments. Tests should be self-documenting.
  • Implementation Sample (Lines to Delete):

    // DELETE THE FOLLOWING LINES:
    // CURRENT BEHAVIOR (Buggy):
    // The "Select Music" button is shown because hasSpotifyData is false
    // EXPECTED BEHAVIOR (Fixed):
    // Playback controls should be shown because an active device exists
    
    // EXPECTED BEHAVIOR (Fixed):
    // Playback controls should be shown because an active device exists

---

🏗️ Architectural Impact

Moving the source of truth from strictly Track Data to a hybrid of Initialization + Device Activity creates a much more resilient UI state. The addition of the component-level window focus listener is acceptable here since it's restricted to this control view, but keep an eye out: if we start adding window.addEventListener('focus', ...) across multiple components, we should abstract this to a global useWindowFocusTracker hook to centralize the event handling.

🛡️ GitHub Checks Correlation

• All Quality Gates (Lint, Knip, Scripts) passed, indicating no unused variables or type errors were introduced by the new shouldShowControls logic.
• The Visual Tests passed. The snapshot diffs (select-music-button-hover-chromium-linux.png and control-panel-chromium-linux.png) directly correlate to your changes adding the loading state and renaming the CTA. The visual regression outputs are expected and verified.

Review automatically published via RepoAuditor.

Implemented the debounced focus listener to prevent API spam, removed redundant aria-disabled attributes, and cleaned up verbose comments in the test file as requested. Thanks for the thorough audit!

[github-actions]

🤖 Gemini Review Skipped

Review was skipped for the following reason: Gemini review is globally disabled

🤖 Manual Trigger Quick Reference

Command	Action
@gemini-bot	Run AI Code Review (PR only)
@gemini-triage	Run Issue Triage
@gemini-coder <task>	Generate Code
@create-review-issues	Create issues from review (PR only)
@gemini-help	Show this help message

Manual Trigger Guide

[github-actions]

🤖 Gemini Review Skipped

Review was skipped for the following reason: Gemini review is globally disabled

🤖 Gemini Manual Trigger Guide

[arii]

🤖 AI Technical Audit

PR Review: Spotify Control Logic Refinement (#9549)

Architectural Impact

This PR shifts the source of truth for the Spotify UI from a purely metadata-driven approach to a device-availability approach. This is more robust as it accounts for the 'idle' state of the Spotify Connect ecosystem. However, introducing a global window focus listener adds side effects that could lead to redundant network traffic across multiple tabs if not managed carefully.

Anti-AI-Slop Directives

1. OVERLY VERBOSE COMMENTS: The comment // Force device refresh when tab regains focus (debounced to prevent API spam) is redundant as the code clearly implements a setTimeout and clearTimeout pattern.
2. OVER-ENGINEERING: The inline styling for the Button component in SpotifyControls.tsx (lines 421-436) creates a large block of logic inside the JSX. This should be moved to a styled component or a constant to keep the render method clean.
3. DUPLICATE HOOKS/TYPES: The NodeJS.Timeout type inside the useEffect is technically a Node.js type being used in a browser context. While common, using ReturnType<typeof setTimeout> is more idiomatic for browser-agnostic code.
4. CODE RATIO (Optimization):
   • Line 70-73: hasSpotifyData is defined but only used within shouldShowControls. These can be merged to save 4 lines.
   • Line 421-436: 15 lines of inline styles can be condensed using a sx object defined outside the component.
   • Line 88-101: The focus listener logic can be abstracted or simplified.
5. STALE FEATURES: The PR successfully updates the Playwright tests to use getByTestId instead of hardcoded strings, which prevents future regressions if the labels change again.

File-by-File Analysis

app/client/control/components/SpotifyControls.tsx

Problem: Potential Memory Leak / State Race. The focus listener useEffect declares timeoutId inside the effect, but if the component unmounts before the timeout fires, clearTimeout works, but the dependency array includes sendData. If sendData (from context) changes, the listener is re-attached frequently.

Implementation Sample:

// Optimizing the boolean logic
const shouldShowControls = spotifyServiceInitialized && (
  (spotifyData.playback.track.name !== '' && spotifyData.playback.track.name !== 'No Track Playing') || 
  devices.some(d => d.is_active)
);

tests/unit/.../SpotifyControls.repro.test.tsx

Problem: The test effectively captures the bug, but uses queryByLabelText('Play') which might be brittle if the icon or label changes in the future.

GitHub Checks Correlation

• Quality Report (Failure): The failure in the Quality Report likely stems from the increased cyclomatic complexity in the JSX and the inline style objects. Moving the sx props to a separate style constant will likely resolve this.

Review automatically published via RepoAuditor.

[github-actions]

🤖 Gemini Review Skipped

Review was skipped for the following reason: Gemini review is globally disabled

🤖 Gemini Manual Trigger Guide

[arii]

🤖 AI Technical Audit

PR Review: Fix Spotify 'Connect' Button Persistence

Summary

This PR addresses an issue where the 'Connect' CTA incorrectly persisted when a Spotify device was active but no track was playing. It also introduces a focus-based device refresh mechanism and enhances the button's loading state.

Architectural Impact

• State Management: The shift from track-based logic to device-based availability logic is more robust but increases the dependency on the devices array being accurate.
• Side Effects: Introducing a global window focus listener adds a persistent side effect that relies on correct cleanup to prevent memory leaks or stale closures.

Anti-AI-Slop Directives

1. OVERLY VERBOSE COMMENTS: The comment // Mock the router and // Mock the WebSocket context in the test file are redundant as the code itself is standard Jest boilerplate.
2. OVER-ENGINEERING: The use of focusStateRef to capture connectionStatus and sendData for the focus listener is slightly over-engineered. Since sendData is usually stable (memoized) and the effect could simply depend on the values directly with a debounce, the ref approach adds mental overhead.
3. DUPLICATE HOOKS/TYPES: The INVALID_TRACK_NAMES Set is defined inside the component body on every render. This should be a constant defined outside the component.
4. CODE RATIO: This PR adds logic for a focus listener (~35 lines) that could be simplified. At least 12 lines can be saved by removing the focusStateRef and using a standard useEffect with dependencies and a simpler debounce.
5. STALE FEATURES: The PR successfully updates the VRT snapshots, indicating that visual regressions in the old state were accounted for.

File-by-File Analysis

SpotifyControls.tsx

• Problem: INVALID_TRACK_NAMES is re-allocated on every render cycle. While small, it is a bad practice for static configuration.
• Implementation Sample:

  const INVALID_TRACK_NAMES = new Set(['Awaiting Login...', '', 'No Track Playing']);
  const SpotifyControls = () => { ... }

• Problem: The focus listener logic is unnecessarily complex due to the focusStateRef. If sendData is stable, we can use it directly. Also, a 1-second delay for a 'focus' refresh is quite long for a UI that should feel responsive.

SpotifyControls.repro.test.tsx

• Problem: Good inclusion of a reproduction test. However, it mocks useRouter but doesn't utilize it in the test case itself, adding unnecessary noise.

GitHub Checks Correlation

• Visual Tests (Failure): The failures in vrt-spotify-controls.spec.ts are expected due to the button text change and the addition of the spinner. These need to be manually approved or the snapshots updated (which seems to be partially done in the diff).

Review automatically published via RepoAuditor.

[github-actions]

🤖 Gemini Review Skipped

Review was skipped for the following reason: Gemini review is globally disabled

🤖 Gemini Manual Trigger Guide