Repository navigation
refactor(bot): move music watchdog service out of utils into services - #1981
Merged
Merged
Conversation
First bounded slice of #1968: watchdog.ts holds a stateful class (timers, private state Maps, recurring scan loop) mislabeled as a util. Moves it to services/musicManagement/, updates the 13 import sites, and deletes utils/misc/pathUtils.ts (confirmed zero import sites, flagged safe-to-delete in the same audit). The full split #1968 proposes (~20+ files into services/musicRecommendation/ and services/musicManagement/) is out of scope for this PR -- too large for one mechanical move, per the issue's own recommendation to scope it. This is the first unit; rest follows as separate PRs.
|
Warning Review limit reached
Next review available in: 28 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (18)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Failed to generate code suggestions for PR |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 18 files
Auto-approved: Pure refactor: relocate the watchdog service with import-path updates and formatting that preserves behavior; remove pathUtils and tests as dead code with no references — no product, security, or operational impact.
Re-trigger cubic
|
LucasSantana-Dev
added a commit
that referenced
this pull request
Aug 10, 2026
Stacked on #1981 (not yet merged) — both touch `idleDisconnect.ts` and `leave.ts`. ## Summary - Second bounded slice of #1968: `collaborativePlaylist.ts` holds `CollaborativePlaylistService`, a stateful class with module-level `Map` state, mislabeled as a util. Zero internal imports of its own. - Moved to `services/musicRecommendation/`, updated 4 real import sites (`idleDisconnect.ts`, `leave.ts`, `playlist.ts`, `playHandler.ts`) plus 2 `jest.mock()` string-literal mocks (`playlist.spec.ts`, `play/index.spec.ts`) that a prior grep-only research pass for the watchdog slice missed — caught this time with a repo-wide grep before committing. ## Verification - [x] `npm run type:check --workspace=@lucky/bot` — clean - [x] `npm run test:bot` — 242/243 suites (1 pre-existing skip, unchanged count) - [x] `npm run lint --workspace=@lucky/bot` — 0 errors, 73 pre-existing warnings (unchanged) - [x] Repo-wide grep for the old path after the move — zero remaining references <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Moved the stateful collaborative playlist logic into a service under services/musicRecommendation to clarify module boundaries. No behavior changes. - **Refactors** - Relocated `collaborativePlaylist.ts` and its spec to `services/musicRecommendation/collaborativePlaylist`. - Updated imports in `leave.ts`, `playlist.ts`, `play/handlers/playHandler.ts`, and `utils/music/idleDisconnect.ts`. - Updated `jest.mock` paths in `functions/music/commands/playlist.spec.ts` and `functions/music/commands/play/index.spec.ts`. - Verified clean typecheck, tests, and lint in `@lucky/bot`. - Note: overlaps with the watchdog-to-services work; both touch `idleDisconnect.ts` and `leave.ts`. <sup>Written for commit 3711449. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/LucasSantana-Dev/Lucky/pull/1982?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. -->
LucasSantana-Dev
added a commit
that referenced
this pull request
Aug 10, 2026
Stacked on #1982 (not yet merged). ## Summary - Third bounded slice of #1968. Moves `sessionSnapshots.ts` to `services/musicRecommendation/`, and `sessionStartupRestore.ts` + `namedSessions.ts` to `services/musicManagement/`. - Both `sessionStartupRestore.ts` and `namedSessions.ts` depend on `sessionSnapshots.ts` — now a cross-service import (`services/musicManagement/* -> services/musicRecommendation/sessionSnapshots`), which just relocates the same-directory coupling that already existed; not new architectural coupling. - Updates 11 external import sites plus `watchdog.ts`/`.spec.ts` (from #1981), which also depends on `sessionSnapshots`. ## Side findings (filed separately, not fixed here) - #1983 — `restoreSessionsOnStartup()` (in the file this PR moves) is never called anywhere in the repo except its own test. Docstring claims it runs on `clientReady`; it doesn't. Real regression, out of scope for a move-only refactor. - #1984 — `utils/music/index.ts` barrel has zero external consumers repo-wide. ## Verification - [x] `npm run type:check --workspace=@lucky/bot` — clean - [x] `npm run test:bot` — 242/243 suites (same pre-existing skip, unchanged count) - [x] `npm run lint --workspace=@lucky/bot` — 0 errors, 73 pre-existing warnings (unchanged) - [x] Repo-wide grep after the move — only the 3 expected self-referencing spec imports remain - [x] Adversarial critic pass — ACCEPT, no issues <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Move the music session code from utils to services to align with domain boundaries and clarify dependencies. No runtime behavior changes; only import path and test mock updates. - **Refactors** - Moved `utils/music/sessionSnapshots.ts` to `services/musicRecommendation/sessionSnapshots.ts` (now imports `../../utils/music/trackFields`). - Moved `utils/music/sessionStartupRestore.ts` and `utils/music/namedSessions.ts` to `services/musicManagement/` (now import `../musicRecommendation/sessionSnapshots`). - Updated imports and jest.mocks across commands, player handlers, watchdog, and tests; rebased on `main` and resolved conflicts in those files. - Kept a deliberate cross-service import: `services/musicManagement/*` -> `services/musicRecommendation/sessionSnapshots`. <sup>Written for commit c93cdac. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/LucasSantana-Dev/Lucky/pull/1985?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Reorganized internal music session and recommendation services without changing music playback, session management, restoration, or queue behavior. - Existing music commands and event handling continue to work as before. - **Tests** - Updated automated test coverage to follow the reorganized service structure. - Test behavior and assertions remain unchanged. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
LucasSantana-Dev
added a commit
that referenced
this pull request
Sep 10, 2026
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img](https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a){kind=link}
First slice of #1991. Does not close it: four subdirectories remain. ## Why this is not a utils module `utils/music/trackUtils/` is a stateful service tree: - `TrackUtils` (barrel) holds a `TrackProcessor` - `TrackProcessor` owns a `TrackCacheManager` and a cleanup timer (`startCacheCleanup`) - `TrackCacheManager` wraps an `LRUCache` - the barrel exports a module-level singleton: `export const trackUtils = new TrackUtils()` Same pattern as every finding in #1968, which established `services/musicManagement/` as the destination across its 7 slices (#1981, #1982, #1985, #1987, #1988, #1989, #1990). ## Surface Only `getTrackInfo` escapes the directory, so the external surface is four files: | file | reference | |---|---| | `functions/music/commands/queue/queueDisplay.ts` | `import { getTrackInfo }` | | `functions/music/commands/queue/queueStats.ts` | `import { getTrackInfo }` | | `functions/music/commands/queue/queueDisplay.spec.ts` | `jest.mock('...')` string literal | | `functions/music/commands/queue/queueStats.spec.ts` | `jest.mock('...')` string literal | The `jest.mock()` string literals do not move with an import rewrite. #1991 flags these as the most common miss in the #1968 slices, and the same trap applied *inside* the moved specs, which mock `'../titleComparison'` and `require()` it in five more places. ## Path arithmetic Both directories sit three levels under `src/`, so relative depth is unchanged. Only the one sibling import needed rewriting: ``` '../titleComparison' -> '../../../utils/music/titleComparison' ``` That is services depending on utils, which is the correct direction. The inversion #1991 is about is utils depending on services, which this avoids by moving the whole tree rather than `cacheManager.ts` alone. `titleComparison` is itself on the #1991 list and moves in a later slice. ## Verification - `packages/bot` full suite: 273 suites passed, 3586 tests passed, 1 skipped, matching the pre-move baseline exactly (pure move, no test added or removed) - `npx tsc --noEmit`: clean - `npx madge --circular --extensions ts packages/bot/src`: `No circular dependency found!` - `npm run build --workspace=packages/bot`: succeeds - repo-wide grep for `utils/music/trackUtils` across ts/tsx/js/json/yml/md: no remaining references ## Remaining in #1991 `search/engineManager.ts`, `search/providerHealth.ts` (8+ external consumers, the largest slice), `titleComparison/service.ts`, `youtubeErrorHandler/analyzer.ts`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Relocates the stateful `trackUtils` service from `utils/music/` to `services/musicManagement/`, matching the destination established in `#1968` and continuing the `#1991` refactor. Since only the queue commands consume `getTrackInfo`, the external change is limited to updating their imports and the `jest.mock()` path literals in their specs. **Notes** - Moved specs also updated their `jest.mock('../titleComparison')` and `require()` calls to the new relative path. - `titleComparison` remains in `utils` and will move in a later slice; `search/engineManager`, `search/providerHealth`, and `youtubeErrorHandler/analyzer` are still pending. - Full test suite and `tsc` pass with no circular dependencies. <sup>Written for commit 5a734c9. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/LucasSantana-Dev/Lucky/pull/2353?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. --> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
packages/bot/src/utils/music/watchdog.tsexportsMusicWatchdogService, a stateful class (timers, private state Maps, recurring scan loop) mislabeled as a utilpackages/bot/src/services/musicManagement/watchdog.ts, updated all 13 import sites (functions/music/commands/{leave,music,stop}, bot/start/initializer, handlers/player/{lifecycleHandlers,queueExhaustion,trackHandlers}, utils/music/idleDisconnect, plus a top-level tests/ integration test)utils/misc/pathUtils.ts— confirmed zero import sites anywhere in the repo, flagged safe-to-delete in the same audit issueScope note
#1968 proposes a much larger split (~20+ files into
services/musicRecommendation/andservices/musicManagement/, including the 40-fileautoplay/subdirectory). That full scope is out of this PR — too large for one mechanical move, matching the issue's own recommendation to scope it via a refactor plan rather than doing it blind. This is the first unit; the rest follows as separate PRs.Verification
npm run type:check --workspace=@lucky/bot— cleannpm run test:bot— 242/243 suites passed (1 pre-existing skip, confirmed unrelated viagit stashcomparison against main)npm run lint --workspace=@lucky/bot— 0 errors, 73 pre-existing warnings (none in touched files)Summary by cubic
Moved the stateful
MusicWatchdogServicefrom utils to services to better reflect its role and align with #1968. Updated imports and removed an unused path utility. No behavior changes.Written for commit 38608c3. Summary will update on new commits.