Repository navigation
refactor(bot): move state-store cluster out of utils into services - #1987
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).
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. Found and filed separately, not fixed here: #1986 (tests/setup.ts calls a cache-clear function that doesn't exist on replenishSuppressionStore.ts -- pre-existing, silently swallowed by a try/catch, unrelated to this move). Stacks on refactor/bot-session-cluster-to-services (#1985).
|
Warning Review limit reached
Next review available in: 35 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 (16)
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 15 files
Auto-approved: Pure import-path refactor moving three state stores to services/musicManagement; only import paths and formatting changed, no behavior changes. Tests pass.
Re-trigger cubic
This was referenced Aug 10, 2026
Base automatically changed from
refactor/bot-session-cluster-to-services
to
main
August 10, 2026 19:43
LucasSantana-Dev
dismissed
cubic-dev-ai[bot]’s stale review
August 10, 2026 19:43
The base branch was changed.
…ores-to-services # Conflicts: # packages/bot/src/handlers/player/queueExhaustion.ts # packages/bot/src/services/musicManagement/idleDisconnect.ts
idleDisconnect.ts had no spec file on main; sonarcloud's new-code coverage gate failed on this pr (55.3%, needs 80%) because the move surfaced 24 previously-untested lines as new code.
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Auto-approved: Pure refactor relocating three state stores; diffs show only import path updates, formatting, and a new idleDisconnect spec. No behavior or contract changes; tests and type-check pass per description.
Re-trigger cubic
|
LucasSantana-Dev
added a commit
that referenced
this pull request
Aug 10, 2026
Stacked on #1987 (not yet merged). ## Summary - Fifth bounded slice of #1968, the largest so far. Moves the entire 27-file `autoplay/` directory plus `candidateFallback.ts` (tightly coupled to `autoplay/`, bundled so their sibling imports stay intact) to `services/musicRecommendation/`. - Keeps `autoplayManager.ts` as a thin facade at its original `utils/music/` location — only its one internal import needed fixing, so its 6 external consumers needed no changes at all. - Also fixes `queueEditOps.ts`, `queueManipulation.ts`, and `queueRescue.ts` (plus ~7 spec files), which reach into `autoplay/` from `utils/music/`, and removes a now-redundant `utils/music/autoplay` entry from `queueResolver.guard.spec.ts`'s architecture-guard target list (already covered by the existing `services/musicRecommendation` entry via recursion). ## Honesty about how this one went This slice took five rounds of fix-and-recheck via typecheck/test runs, not just grep — worth being upfront about given the size: 1. A bulk sed substring-collided `autoplayManager` with `autoplay` (fixed by reverting 3 files back to their original path) 2. Two off-by-one relative-path depth errors (`../` instead of `../../`) 3. Three files (`queueEditOps.ts`, `queueManipulation.ts`, `queueRescue.ts`) missed entirely on the first pass despite being flagged by earlier research 4. Several `.spec.ts`-only `jest.mock()` paths missed (the `.ts` files were fixed, their specs weren't) 5. One bare `require()` call inside a test body (`replenisher.spec.ts`) that only surfaced by actually running the suite — grep for import/mock patterns doesn't catch it Each was caught by actually running `type:check`/`test:bot`, not assumed fixed after the sed. Two separate adversarial critic passes ran on this diff (one before the final green run turned something up, one after) — both eventually landed ACCEPT. ## 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 for old paths — zero stale references (2 apparent hits were false positives: an unrelated same-named `functions/music/commands/autoplay/` directory, and files now correctly colocated with the moved `autoplay/`) - [x] Two adversarial critic passes — ACCEPT <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Moved the autoplay engine from utils to services to align with our services architecture without changing behavior. `autoplayManager.ts` remains a thin facade so existing callers don’t need changes. - **Refactors** - Moved all `utils/music/autoplay/*` files and `candidateFallback.ts` to `services/musicRecommendation/autoplay/` (including `audioFeatures.ts` and `vcWeights.ts`); removed old copies. - Updated imports in commands (`previous`, `skip`, `stop`), player handlers, Last.fm exports, queue helpers/tests, and `jest.mock` paths. - Routed telemetry through `services/musicRecommendation/autoplay/recommendationSourceMapping` and `recommendationBasis`. - Kept `utils/music/autoplayManager.ts` re-exporting from `services/musicRecommendation/autoplay/index`. - Removed the redundant `utils/music/autoplay` entry from the architecture guard. - Updated Sonar CPD exclusion paths to the new locations to prevent duplicate-code violations. <sup>Written for commit 1cf31fa. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/LucasSantana-Dev/Lucky/pull/1988?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 #1985 (not yet merged).
Summary
replenishSuppressionStore.ts,voteSkipStore.ts, andidleDisconnect.ts(all module-level mutable state, zero internal coupling among themselves) toservices/musicManagement/.import()intests/setup.ts.service.ts(TrackManagementService) from the original slice plan — found zero external consumers while scoping this move, filed separately as chore(bot): utils/music/index.ts barrel has zero external consumers #1984 rather than relocating dead code.Side findings (filed separately, not fixed here)
utils/music/index.tsbarrel +TrackManagementServicehave zero external consumers.tests/setup.tscallsclearReplenishSuppressionCache(), 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
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
Moved music state stores from utils to
services/musicManagementto centralize mutable state and simplify ownership. No behavior changes; updated imports and resolved merge conflicts after syncing withmain.replenishSuppressionStore.ts,voteSkipStore.ts, andidleDisconnect.tstoservices/musicManagement/.services/musicManagement/idleDisconnect.spec.tsto cover scheduling, cancellation, and error paths.jest.mockpaths across commands, handlers, specs, and the dynamic import intests/setup.ts; fixed relative paths inidleDisconnect.tsto./watchdogand../musicRecommendation/collaborativePlaylist; resolved conflicts inhandlers/player/queueExhaustion.tsandservices/musicManagement/idleDisconnect.ts.TrackManagementService(no external consumers); filed separately.Written for commit 468a5a0. Summary will update on new commits.