Repository navigation
refactor(bot): move session cluster out of utils into services - #1985
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.
Second bounded slice of #1968: collaborativePlaylist.ts holds a stateful class (CollaborativePlaylistService, module-level Map state) mislabeled as a util, with zero internal coupling of its own. Moves it to services/musicRecommendation/, updates 4 real import sites plus 2 jest.mock() string-literal mocks a prior grep-only pass missed. Stacks on refactor/bot-watchdog-to-services (#1981, not yet merged) -- both touch idleDisconnect.ts and leave.ts.
Third bounded slice of #1968. Moves sessionSnapshots.ts to services/musicRecommendation/, and sessionStartupRestore.ts + namedSessions.ts to services/musicManagement/ -- both of the latter depend on sessionSnapshots, now a cross-service import mirroring the same-directory coupling that already existed. Updates 11 import sites plus watchdog.ts/spec (from #1981) which also depends on sessionSnapshots. Found and filed separately while scoping this slice, not fixed here: #1983 (restoreSessionsOnStartup is never called -- dead code) and #1984 (utils/music/index.ts barrel has zero external consumers). Stacks on refactor/bot-collaborative-playlist-to-services (#1982).
📝 WalkthroughWalkthroughMusic session snapshot and named session imports now point to their service-layer locations. Production references and Jest mocks use the updated paths. One test fixture formatting change does not alter behavior. ChangesMusic service path migration
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 20 files
Auto-approved: Pure code move: all diffs are import path updates (plus one test formatting nit) across 20 files; no behavior, contract, or operational change.
Re-trigger cubic
This was referenced Aug 10, 2026
Base automatically changed from
refactor/bot-collaborative-playlist-to-services
to
main
August 10, 2026 19:32
LucasSantana-Dev
dismissed
cubic-dev-ai[bot]’s stale review
August 10, 2026 19:32
The base branch was changed.
…cluster-to-services # Conflicts: # packages/bot/src/functions/music/commands/music.ts # packages/bot/src/functions/music/commands/stop.ts # packages/bot/src/handlers/player/lifecycleHandlers.ts # packages/bot/src/handlers/player/queueExhaustion.ts # packages/bot/src/handlers/player/trackHandlers.ts # packages/bot/src/services/musicManagement/watchdog.spec.ts # packages/bot/src/services/musicManagement/watchdog.ts
|
LucasSantana-Dev
added a commit
that referenced
this pull request
Aug 10, 2026
…1987) Stacked on #1985 (not yet merged). ## Summary - Fourth bounded slice of #1968. Moves `replenishSuppressionStore.ts`, `voteSkipStore.ts`, and `idleDisconnect.ts` (all module-level mutable state, zero internal coupling among themselves) to `services/musicManagement/`. - Updates 8 import sites plus a dynamic `import()` in `tests/setup.ts`. - Deliberately skips `service.ts` (`TrackManagementService`) from the original slice plan — found zero external consumers while scoping this move, filed separately as #1984 rather than relocating dead code. ## Side findings (filed separately, not fixed here) - #1984 — `utils/music/index.ts` barrel + `TrackManagementService` have zero external consumers. - #1986 — `tests/setup.ts` calls `clearReplenishSuppressionCache()`, which doesn't exist on the file it's moving here. Pre-existing (the move only changed the import path string, not the destructured name), silently swallowed by a try/catch every test run. ## 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 2 expected self-referencing spec imports remain - [x] Adversarial critic pass — surfaced #1986 (verified real, pre-existing, filed separately), confirmed the service.ts skip was correct <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Moved music state stores from utils to `services/musicManagement` to centralize mutable state and simplify ownership. No behavior changes; updated imports and resolved merge conflicts after syncing with `main`. - **Refactors** - Relocated `replenishSuppressionStore.ts`, `voteSkipStore.ts`, and `idleDisconnect.ts` to `services/musicManagement/`. - Added `services/musicManagement/idleDisconnect.spec.ts` to cover scheduling, cancellation, and error paths. - Updated imports and `jest.mock` paths across commands, handlers, specs, and the dynamic import in `tests/setup.ts`; fixed relative paths in `idleDisconnect.ts` to `./watchdog` and `../musicRecommendation/collaborativePlaylist`; resolved conflicts in `handlers/player/queueExhaustion.ts` and `services/musicManagement/idleDisconnect.ts`. - Skipped `TrackManagementService` (no external consumers); filed separately. <sup>Written for commit 468a5a0. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/LucasSantana-Dev/Lucky/pull/1987?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. -->
5 tasks done
3 tasks done
LucasSantana-Dev
added a commit
that referenced
this pull request
Sep 6, 2026
## Summary Closes #1968. Deleted `packages/bot/src/utils/music/service.ts` (`TrackManagementService`) and its spec. Confirmed via repo-wide grep: zero production importers, direct or via barrel - the `utils/music/index.ts` barrel that once re-exported it was already deleted separately. Only its own 445-line spec file referenced it. This was #1984's finding (closed, but the actual deletion never landed - #1989's PR body says it was "deliberately left in place" pending a maintainer call). #1968 explicitly named this file as in its scope, so making that call here. **#1968's broader state**, re-verified while investigating this: the directory it originally flagged (136 files, 10.5k LOC of misplaced stateful services mixed into a `utils/` folder) is now 34 files / 3535 LOC. Prior bounded-slice PRs (#1985-#1989 and others, all explicitly scoped as "Nth bounded slice of #1968") already moved every DB/API-touching, timer-driven service out to `services/musicRecommendation/` and `services/musicManagement/` (`watchdog.ts`, `queueResolver.ts`, `queueStateManager.ts`, `sessionStartupRestore.ts`, the `queue*.ts` mutation files, the `autoplay/` cluster including `replenisher.ts`/`candidateScorer.ts`, `candidateFallback.ts`, `sessionSnapshots.ts`, `collaborativePlaylist.ts` - all exactly matching the issue's original "suggested approach" destinations). Checked every remaining class-based file in `utils/music/` for the same misplacement pattern (DB access, external API calls, timers/recurring background state): - `search/engineManager.ts`'s only `setTimeout` is an inline retry-backoff delay inside a function call, not a persistent scheduler - `titleComparison/service.ts`'s `Map` is a plain in-memory memoization cache, no DB/API/timers - Everything else in the remaining 34 files is pure functions/type helpers (`trackValidator.ts`, `trackNormalization.ts`, `searchQueryCleaner.ts`, `buttonComponents.ts`, `skipReasonMap.ts`, etc.) - matching the issue's own "Keep in utils/" list None of the remaining files match the pattern that made the original 136-file audit finding real. #1968 is functionally complete with this PR. ## Test plan - [x] Repo-wide grep confirms zero references to `TrackManagementService`/`trackManagementService` outside the deleted files - [x] `tsc --noEmit` clean across bot/backend/frontend/shared - [x] Full bot suite: 250 suites / 3285 tests pass (down from 251/3286 by exactly the deleted spec's own tests, no other regressions) <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Removes the unused `TrackManagementService` from `packages/bot/src/utils/music/service.ts` and its spec file. It has no production importers (the barrel re-export was already removed), so only its own tests referenced it. **Context** - It was orphaned after queue management logic migrated to `services/musicManagement/` as part of #1968. - Reviewed the remaining 34 files in `utils/music/`; none match the misplaced-stateful-service pattern (DB access, API calls, timers), so #1968 is complete with this deletion. - `tsc` is clean and the bot suite passes with only the deleted spec's tests gone (250 suites / 3285 tests). <sup>Written for commit 900210d. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/LucasSantana-Dev/Lucky/pull/2276?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
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.
Stacked on #1982 (not yet merged).
Summary
sessionSnapshots.tstoservices/musicRecommendation/, andsessionStartupRestore.ts+namedSessions.tstoservices/musicManagement/.sessionStartupRestore.tsandnamedSessions.tsdepend onsessionSnapshots.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.watchdog.ts/.spec.ts(from refactor(bot): move music watchdog service out of utils into services #1981), which also depends onsessionSnapshots.Side findings (filed separately, not fixed here)
restoreSessionsOnStartup()(in the file this PR moves) is never called anywhere in the repo except its own test. Docstring claims it runs onclientReady; it doesn't. Real regression, out of scope for a move-only refactor.utils/music/index.tsbarrel has zero external consumers repo-wide.Verification
npm run type:check --workspace=@lucky/bot— cleannpm run test:bot— 242/243 suites (same pre-existing skip, unchanged count)npm run lint --workspace=@lucky/bot— 0 errors, 73 pre-existing warnings (unchanged)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.
utils/music/sessionSnapshots.tstoservices/musicRecommendation/sessionSnapshots.ts(now imports../../utils/music/trackFields).utils/music/sessionStartupRestore.tsandutils/music/namedSessions.tstoservices/musicManagement/(now import../musicRecommendation/sessionSnapshots).mainand resolved conflicts in those files.services/musicManagement/*->services/musicRecommendation/sessionSnapshots.Written for commit c93cdac. Summary will update on new commits.
Summary by CodeRabbit
Refactor
Tests