Skip to content

Conversation

@arii
Copy link
Owner

@arii arii commented Feb 6, 2026

Description

This PR refactors the SpotifyPolling service to improve separation of concerns by moving the getCurrentlyPlaying implementation details into SpotifyPlayerManager. It also introduces a shared utility for error checking and ensures all changes are covered by unit tests. This addresses the feedback from PR #6093.

Fixes #

Change Type: 🏗️ Refactoring (code change that neither fixes bug nor adds feature)

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

This PR refactors the SpotifyPolling service to improve separation of concerns by moving the getCurrentlyPlaying implementation details into SpotifyPlayerManager. It also introduces a shared utility for error checking and ensures all changes are covered by unit tests. This addresses the feedback from PR #6093.


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

- Extract `getCurrentlyPlaying` logic from `SpotifyPolling.ts` into `SpotifyPlayerManager.ts` as `fetchPlaybackState`.
- Create `services/spotifyUtils.ts` for shared utility `isEmptyResponseError`.
- Remove `Track` and `Episode` imports from `SpotifyPolling.ts` and clean up logic.
- Update `SpotifyPolling` to use `fetchPlaybackState` and handle state updates/broadcasting.
- Add unit tests for `fetchPlaybackState` in `tests/unit/services/spotifyPlayerManager.test.ts`.
- Addresses architectural directives to decompose the polling service.

Co-authored-by: arii <342438+arii@users.noreply.github.com>
@google-labs-jules
Copy link
Contributor

👋 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 github-actions bot changed the title Refactor Spotify Polling service for better architectural separation test(medium): Refactor Spotify Polling service for better architectural separation Feb 6, 2026
@arii
Copy link
Owner Author

arii commented Feb 6, 2026

📋 Quality Gate Results

Check Status
Knip ✅ success
Lint ❌ failure
Build ✅ success
Infra Tests ❌ skipped
Unit Tests ❌ skipped
Component Tests ❌ skipped
Perf Tests ❌ skipped
Visual Tests ❌ skipped

❌ Lint Failure Details


> hrm@0.30.0 lint /home/runner/work/hrm/hrm
> eslint app/ components/ constants/ context/ hooks/ lib/ services/ tests/ types/ utils/ server.ts proxy.ts --cache


/home/runner/work/hrm/hrm/services/spotifyPolling.ts
  102:80  error  Insert `⏎·······`  prettier/prettier

/home/runner/work/hrm/hrm/tests/unit/services/spotifyPlayerManager.test.ts
  175:12  error  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any
  194:12  error  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any
  222:12  error  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any

✖ 4 problems (4 errors, 0 warnings)
  1 error and 0 warnings potentially fixable with the `--fix` option.

 ELIFECYCLE  Command failed with exit code 1.

❌ Infrastructure Test Failure Details

Log file not found.

❌ Unit Test Failure Details

Log file not found.

❌ Component Test Failure Details

Log file not found.

❌ Visual Test Failure Details

Log file not found.

❌ Performance Test Failure Details

Log file not found.

⚠️ Some checks failed. Full logs available in workflow artifacts.


Report generated for commit: 22eb1d912e5bc0ad88a4e98059ffc7c8e933e02a

@google-labs-jules
Copy link
Contributor

📋 Quality Gate Results

Check Status
Knip ✅ success
Lint ❌ failure
Build ✅ success
Infra Tests ❌ skipped
Unit Tests ❌ skipped
Component Tests ❌ skipped
Perf Tests ❌ skipped
Visual Tests ❌ skipped

❌ Lint Failure Details


> hrm@0.30.0 lint /home/runner/work/hrm/hrm
> eslint app/ components/ constants/ context/ hooks/ lib/ services/ tests/ types/ utils/ server.ts proxy.ts --cache


/home/runner/work/hrm/hrm/services/spotifyPolling.ts
  102:80  error  Insert `⏎·······`  prettier/prettier

/home/runner/work/hrm/hrm/tests/unit/services/spotifyPlayerManager.test.ts
  175:12  error  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any
  194:12  error  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any
  222:12  error  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any

✖ 4 problems (4 errors, 0 warnings)
  1 error and 0 warnings potentially fixable with the `--fix` option.

 ELIFECYCLE  Command failed with exit code 1.

❌ Infrastructure Test Failure Details

Log file not found.

❌ Unit Test Failure Details

Log file not found.

❌ Component Test Failure Details

Log file not found.

❌ Visual Test Failure Details

Log file not found.

❌ Performance Test Failure Details

Log file not found.

⚠️ Some checks failed. Full logs available in workflow artifacts.


Report generated for commit: 22eb1d912e5bc0ad88a4e98059ffc7c8e933e02a

Acknowledged. I have fixed the linting issues (Prettier formatting and explicit any usage in tests).

- Extract `fetchPlaybackState` logic into `SpotifyPlayerManager` with strict `ParsedPlaybackState` return type.
- Create `services/spotifyUtils.ts` and move `isEmptyResponseError` there for shared use.
- Update `SpotifyPolling` to delegate state fetching and remove redundant parsing logic.
- Add comprehensive unit tests for `fetchPlaybackState` and fix lint/type issues in tests.

Co-authored-by: arii <342438+arii@users.noreply.github.com>
@arii
Copy link
Owner Author

arii commented Feb 6, 2026

Reviewed commit: b90aac854df71719e5135974200203844ec497a7

This PR successfully refactors the Spotify Polling service, significantly improving architectural separation and code organization. The core logic for fetching and parsing the currently playing Spotify track/episode has been extracted from SpotifyPolling into a new fetchPlaybackState method within SpotifyPlayerManager. This clarifies responsibilities and enhances maintainability.

Strengths

  • Improved Architectural Separation: Moving the playback state fetching and parsing into SpotifyPlayerManager makes SpotifyPolling much cleaner and focused solely on the polling mechanism and state broadcasting. This aligns well with the Single Source of Truth Principle and Quality & Security First core principle outlined in docs/DEVELOPMENT.md.
  • Modularity: Extracting isEmptyResponseError into a dedicated spotifyUtils.ts file promotes reusability and better organization of utility functions.
  • Type Safety: The introduction of the ParsedPlaybackState interface in spotifyPlayerManager.ts provides clear, type-safe structures for the data, improving readability and reducing potential runtime errors.
  • Comprehensive Unit Tests: The new unit tests for fetchPlaybackState in spotifyPlayerManager.test.ts are thorough, covering various scenarios including null states, track parsing, and episode parsing. This ensures the new logic is robust.

Verdict

✅ Verified the refactoring for better architectural separation and new unit tests. No regressions found. Ready for approval.

Reviewed at commit: b90aac854df71719e5135974200203844ec497a7

@arii arii marked this pull request as ready for review February 6, 2026 21:16
@arii arii merged commit 7eee0aa into feat/refactor-spotify-polling-180427182370128719 Feb 6, 2026
32 checks passed
@arii arii deleted the feat-refactor-spotify-polling-180427182370128719-18352674082257778548 branch February 6, 2026 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant