Repository navigation
refactor(bot): move track utils out of utils into services - #2353
Merged
Merged
Conversation
utils/music/trackUtils/ was never a utils module. It is a stateful service tree: TrackUtils holds a TrackProcessor, which owns a TrackCacheManager wrapping an LRUCache plus a cleanup timer, and the barrel exports a module-level singleton, `export const trackUtils = new TrackUtils()`. Same pattern as every finding in #1968. Moved wholesale to services/musicManagement/trackUtils/, the destination that series established. Only getTrackInfo escapes the directory, so the external surface is four files: queueDisplay.ts and queueStats.ts plus the two specs, whose jest.mock() string literals name the path and do not move with an import rewrite. Those literals were the most common miss in the #1968 slices, and the same trap applies inside the moved specs, which mock '../titleComparison'. Both directories sit three levels under src/, so relative depth is unchanged and only the one sibling import needed rewriting: '../titleComparison' becomes '../../../utils/music/titleComparison'. That is services depending on utils, the direction that was already correct; the inversion this issue is about is utils depending on services. titleComparison is itself on the #1991 list and moves later. Part of #1991. Four subdirectories remain: search/engineManager, search/providerHealth, titleComparison/service and youtubeErrorHandler/analyzer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pkt4D9yyHTrVhsvDYvw5an
LucasSantana-Dev
enabled auto-merge (squash)
September 10, 2026 17:54
|
Failed to generate code suggestions for PR |
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
Moves the trackUtils module (cache manager, track processor, and their types) from utils/music to services/musicManagement, updating the internal titleComparison imports and the getTrackInfo import paths in queueDisplay and queueStats accordingly. Pure relocation with no behaviour change.
No blocking issues surfaced. 1 lower-confidence candidate did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 366 functions depend on the 78 functions this change touches.
Health — this change adds coupling hotspots:
- new:
requireGuildModuleAccess()— 30 callers, 7 callees - new:
replenishQueue()— 19 callers, 9 callees - new:
calculateRecommendationScore()— 14 callers, 8 callees - new:
setupManagementRoutes()— 4 callers, 24 callees - new:
handleEvents()— 5 callers, 15 callees - new:
collectLastFmCandidates()— 5 callers, 13 callees - new:
getCommandsFromDirectory()— 11 callers, 5 callees - new:
collectRecommendationCandidates()— 7 callers, 7 callees - …and 69 more — each is listed as a finding
Verification — 366 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 269 function(s) in the blast radius were not formally verified this run
· 77 more finding(s) on lines outside this diff (see the check run).
|
LucasSantana-Dev
added a commit
that referenced
this pull request
Sep 10, 2026
…2354) Second slice of #1991, stacked behind #2353. Does not close it: `titleComparison/service` remains. ## Why these two move together Both are stateful service trees living under `utils/`: - `search/engineManager.ts` — `SearchEngineManager`, holds a `setTimeout` - `search/providerHealth.ts` — `ProviderHealthService`, holds a mutable `Map<MusicProvider, ProviderStatus>` - `youtubeErrorHandler/analyzer.ts` — `YouTubeErrorAnalyzer`, behind a `YouTubeErrorHandler` facade `search/engineManager.ts` and `search/searchContentOnYoutube.ts` both import `youtubeErrorHandler`. Moving either directory alone would leave `utils/` importing `services/`, which is the inversion #1991 exists to remove, not relocate. So they move in one slice. ## Surface is smaller than it first appears My initial grep for `utils/music/search` reported 31 references. That was wrong: it also matches `utils/music/searchQueryCleaner`, an unrelated module that does not move. The real surface: | specifier | count | |---|---| | deep imports of `search/providerHealth` | 10 | | references to `youtubeErrorHandler` | 7 | Nothing outside imports the `search` barrel or `engineManager` at all. Every external reference to `search/` is a deep import of `providerHealth`. ## Path arithmetic Both trees sit three levels under `src/` at source and destination, so relative depth is unchanged: - the one outward relative import, `'../../../types'`, still resolves to `src/types` - the sibling `'../youtubeErrorHandler'` still resolves, because both directories moved ## One reference lived outside `src/` `tests/handlers/player/playerFactory.test.ts:45` holds a `jest.mock()` literal naming the old path. A `src/`-only scan missed it; the suite caught it as `Test suite failed to run`. Fixed. Worth noting for the remaining slice: `packages/bot/tests/` is a second tree that must be scanned. ## Deliberately not changed `decisions/2026-05-16-dependabot-batch-handling-policy.md:21` still names the old `youtubeErrorHandler` path. Left alone: that ADR records where the code was in May, and editing it to match a later move falsifies the record. ## Verification - `packages/bot` full suite: 273 suites passed, 3588 tests passed, 1 skipped, exactly the pre-move baseline (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 across ts/tsx/js/json/yml/md, both `src/` and `tests/`: no remaining references outside the ADR noted above 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Moves the stateful music search and YouTube error-handling trees from `utils/music` to `services/musicManagement` as the second slice of #1991, and adds missing test coverage for the moved barrels that SonarCloud was blocking on. - Adds coverage for the search barrel, YouTube error barrel, and `searchContentOnYoutube`, raising line coverage on the moved set from 61.47% to 98.05%. - Leaves `titleComparison/service` for a later slice of #1991. - Only import paths, Jest mock literals, and formatting changed; runtime behavior is unchanged. <sup>Written for commit cbfefa3. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/LucasSantana-Dev/Lucky/pull/2354?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>
![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}
LucasSantana-Dev
added a commit
that referenced
this pull request
Sep 10, 2026
Third and final slice of #1991. Closes it, once #2354 also lands. `TitleComparisonService` holds `private readonly cache: Map<string, ArtistTitle>`, so it is a stateful service rather than a util. Last of the five classes #1991 enumerated: | class | slice | |---|---| | `TrackCacheManager` | #2353 | | `SearchEngineManager`, `ProviderHealthService`, `YouTubeErrorAnalyzer` | #2354 | | `TitleComparisonService` | this PR | ## References rewritten Eleven, across three shapes: | shape | count | from | |---|---|---| | `'../../../../utils/music/titleComparison'` | 2 | `queueDisplay.ts` + spec | | `'../../../utils/music/titleComparison'` | 8 | `trackUtils` (3 files) | | `'../../../src/utils/music/titleComparison/service'` | 1 | `tests/utils/music/titleComparison.test.ts` | That last one is a `jest.mock()` literal in the **`tests/`** tree. #2354 hit exactly this and the suite caught it rather than my scan, so this slice scanned both trees up front. ## The eight from trackUtils collapse back to `'../titleComparison'` They were relative siblings originally. #2353 moved `trackUtils` into services, which turned them into `'../../../utils/music/titleComparison'`. Now that both live under `services/musicManagement/`, they are siblings again and the specifier returns to its original form. Worth noting so a reviewer does not read it as an unrelated change. ## One outward dependency needed the reverse rewrite The moved code sits one level deeper relative to `utils/`, so: ``` '../../misc/stringUtils' -> '../../../utils/misc/stringUtils' ``` in `service.ts` and in the `jest.mock()` literal in `service.spec.ts`. ## Independence from #2354 This slice touches neither `search/` nor `youtubeErrorHandler/`, and its only prerequisite (#2353, for the `trackUtils` location) is already on `main`. So it is branched off `main` rather than stacked on #2354, and the two can merge in either order. ## Verification - `packages/bot` full suite: 273 suites passed, 3588 tests passed, 1 skipped, exactly the pre-move baseline (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 across ts/tsx/js/json/yml/md, both `src/` and `tests/`: no remaining references 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Moves `TitleComparisonService` out of `utils` into `services` because it maintains a private cache and behaves as a stateful service. This is the last slice of the #1991 refactor and closes it; no behavior changes. **Refactors** - Updates 11 import references across `queueDisplay`, `trackUtils`, and a `jest.mock()` in the test tree. - The `trackUtils` imports revert to the sibling path `../titleComparison` now that both live under `services/musicManagement`. - The outward dependency `../../misc/stringUtils` becomes `../../../utils/misc/stringUtils` in `service.ts` and its spec. **Tests** - Adds a barrel spec raising moved-file coverage from 72.46% to 100% so SonarCloud's 80% new-code gate passes. - The spec runs against the real service and covers the facade, per-instance cache, and custom threshold. - `extractArtistTitle` doesn't split "artist - title" because pattern lists ship empty; the gap is filed as #2356. <sup>Written for commit 6bd85bd. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/LucasSantana-Dev/Lucky/pull/2355?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.
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 aTrackProcessorTrackProcessorowns aTrackCacheManagerand a cleanup timer (startCacheCleanup)TrackCacheManagerwraps anLRUCacheexport 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
getTrackInfoescapes the directory, so the external surface is four files:functions/music/commands/queue/queueDisplay.tsimport { getTrackInfo }functions/music/commands/queue/queueStats.tsimport { getTrackInfo }functions/music/commands/queue/queueDisplay.spec.tsjest.mock('...')string literalfunctions/music/commands/queue/queueStats.spec.tsjest.mock('...')string literalThe
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'andrequire()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: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.tsalone.titleComparisonis itself on the #1991 list and moves in a later slice.Verification
packages/botfull 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: cleannpx madge --circular --extensions ts packages/bot/src:No circular dependency found!npm run build --workspace=packages/bot: succeedsutils/music/trackUtilsacross ts/tsx/js/json/yml/md: no remaining referencesRemaining in #1991
search/engineManager.ts,search/providerHealth.ts(8+ external consumers, the largest slice),titleComparison/service.ts,youtubeErrorHandler/analyzer.ts.🤖 Generated with Claude Code
Summary by cubic
Relocates the stateful
trackUtilsservice fromutils/music/toservices/musicManagement/, matching the destination established in#1968and continuing the#1991refactor. Since only the queue commands consumegetTrackInfo, the external change is limited to updating their imports and thejest.mock()path literals in their specs.Notes
jest.mock('../titleComparison')andrequire()calls to the new relative path.titleComparisonremains inutilsand will move in a later slice;search/engineManager,search/providerHealth, andyoutubeErrorHandler/analyzerare still pending.tscpass with no circular dependencies.Written for commit 5a734c9. Summary will update on new commits.