"feat(large): Refactor: Implement Unified Spotify Service Bus""

[arii]

Description

This PR refactors the Spotify service integration to implement a Unified Spotify Service Bus. This change centralizes and streamlines Spotify service interactions, improving maintainability and scalability.

This implementation provides a new, unified approach following the revert of arii/hrm#7629, addressing complexities or issues present in the previous attempt.

Fixes # #7629 (by providing a new, stable implementation)

Change Type: ✨ New feature (non-breaking change adding functionality)

PR Scope Checklist

This checklist is mandatory for all PRs.

• PR has a clear, single purpose: The title and description of the PR clearly state the purpose of the change.
• All changes relate to the stated objective: The code changes should be directly related to the purpose of the PR.
• No unrelated cleanup or refactoring: The PR should not contain any changes that are not directly related to the stated objective.
• Title and description match the actual changes: The title and description should accurately reflect the changes in the PR.
• Tests cover the specific change scope: The tests should be focused on the changes in the PR and should not include unrelated tests.

Impact Assessment

• Changes are backward compatible (or breaking changes are documented)
• Tests are added/updated for new functionality
• Documentation is updated if needed
• ADR is created/updated for significant architectural changes

Original PR Body

Reverts #7629

[arii]

🤖 AI Technical Audit

Code Review: Unified Spotify Service Bus

This PR implements a significant architectural shift by centralizing Spotify interactions, which is a positive step for maintainability. However, there are substantial issues regarding scope creep, type safety regressions, and potential dead code that need addressing.

🚫 ANTI-AI-SLOP DIRECTIVES

1. UNRELATED REFACTORING (Scope Creep):

   • Problem: More than 50% of the diff (files in .github/) relates to "Jules" (AI Agent) operations and CI/CD routing. This violates the PR Scope Checklist item "No unrelated cleanup or refactoring".
   • Action: These changes must be extracted to a separate PR to keep the history clean and the review focused on Spotify.

2. TYPE SAFETY REGRESSION (Bad Practice):

   • Problem: You deleted services/safeSpotifyApi.ts (a robust Type Guard pattern) and replaced it with @ts-expect-error suppressions in services/spotifyPolling.ts.
   • Diff Reference: // @ts-expect-error SDK types mandate string... vs the deleted createSafeSpotifyApi wrapper.
   • Action: Restore the type-safe wrapper. Do not suppress TS errors in core service logic.

3. POTENTIAL STALE FEATURE (Dead Code):

   • Problem: app/api/spotify/control/route.ts is refactored but appears effectively unused by the internal application. The frontend now uses useSpotifyCommand (WebSocket). Unless this route is strictly for external 3rd-party use, it creates dual maintenance paths for the same logic (one in SpotifyPolling.ts via WS, one in route.ts via HTTP).

📁 File-by-File Analysis

services/spotifyPolling.ts

• Problem: Regression in type safety. The usage of @ts-expect-error to handle undefined device IDs is fragile.
• Implementation Sample:

  // Bad
  // @ts-expect-error SDK types mandate string, but runtime accepts undefined for active device
  deviceId || undefined,
  
  // Good (Restore SafeSpotifyApi or extend type)
  const safeSdk = createSafeSpotifyApi(sdk);
  await safeSdk.player.startResumePlayback(deviceId || undefined, ...);

app/api/spotify/control/route.ts

• Architectural Issue: This route instantiates a new SpotifyApi instance on every request. In the context of a "Unified Service Bus", commands should ideally be routed through the persistent service (SpotifyPolling) or the service bus handler to ensure state consistency and rate limit management.
• Performance: Repeatedly re-initializing the SDK with SpotifyApi.withAccessToken prevents connection reuse if the SDK supports it.

hooks/useSpotifyCommand.ts

• Best Practice: This is a clean abstraction. It correctly delegates command execution to the WebSocket transport (sendData), aligning with the new architecture.

.github/**/*

• Issue: Out of scope. Please remove from this PR.

Summary

Please revert the CI/CD changes and the deletion of SafeSpotifyApi. Clarify if route.ts is still required; if so, verify if it can leverage the existing SpotifyService instance rather than creating its own.

Review automatically published via RepoAuditor.

[arii]

🤖 AI Technical Audit

Code Review: Unified Spotify Service Bus

This PR implements a significant refactor of the Spotify integration, successfully moving from a manual fetch implementation to the official @spotify/web-api-ts-sdk. This greatly simplifies the codebase and improves maintainability. However, the PR is heavily polluted with unrelated CI/CD changes, violating the core "Scope Checklist".

⚠️ ANTI-AI-SLOP DIRECTIVES

1. CODE RATIO / UNRELATED CHANGES: CRITICAL. Approximately 40% of this diff (files in .github/) is unrelated to the "Spotify Service Bus".
   • Action: Remove all changes to .github/actions, .github/scripts, and .github/workflows. These belong in a separate PR titled "chore: Update Jules Ops and CI workflows".
2. OVER-ENGINEERING (Type Suppression): In services/spotifyPolling.ts, the use of @ts-expect-error is a lazy solution to type mismatches. While the SDK types might be strict, suppressing the error hides potential future breakages.
   • Recommendation: Use a type assertion as string if you are confident the runtime behavior supports undefined, or better yet, verify if the SDK exports a type that allows optionality.
3. STALE FEATURES: ✅ Confirmed deletion of hooks/useSpotifyRemoteExecution.ts and services/safeSpotifyApi.ts. This is excellent cleanup.
4. DUPLICATE TYPES: The file services/spotifyApiErrorHandling.server.ts seems to duplicate logic often found in utils/apiError.ts or similar generic error handlers, but specifically for Next.js Responses. Ensure this doesn't diverge from the app's standard error response format.

📁 File-by-File Analysis

app/api/spotify/control/route.ts

• Refactor Quality: Excellent simplification using the SDK. The switch statement is much cleaner than the previous manual URL construction.
• Security: deviceId validation (typeof deviceId !== 'string') is a good addition.
• Issue: token_type: 'Bearer' is hardcoded. While standard for Spotify, session.tokenType (if available) would be safer, though likely not a blocker.

services/spotifyPolling.ts

• Problem: The @ts-expect-error usage is fragile.
• Implementation Sample:

  // Current:
  // @ts-expect-error SDK types mandate string, but runtime accepts undefined for active device
  deviceId || undefined,
  
  // Preferred:
  (deviceId || undefined) as string, // If you are certain runtime handles it, cast it rather than suppress the check.

components/SpotifyDisplay.tsx

• Logic: The isSliding check in useEffect is a necessary evil for UI responsiveness, but ensure isSliding is reset correctly if the socket disconnects or component unmounts to prevent state locking.

.github/* (All files)

• Problem: Out of scope.
• Action: DELETE from this PR.

Architectural Impact

The move to the Unified Service Bus and the official SDK reduces technical debt significantly. The removal of the SafeSpotifyApi wrapper (which was essentially a hack) is a positive architectural shift towards standard library usage.

Review automatically published via RepoAuditor.