Repository navigation
refactor(bot): move search and youtube error handler into services - #2354
Conversation
Both directories under utils/music/ are stateful service trees, not utils. search/ holds SearchEngineManager (a timer) and ProviderHealthService (a mutable Map keyed by provider); youtubeErrorHandler/ holds YouTubeErrorAnalyzer behind a YouTubeErrorHandler facade. They move together on purpose. search/engineManager and search/searchContentOnYoutube both import youtubeErrorHandler, so moving either one alone would leave utils importing services, which is the inversion #1991 exists to remove rather than relocate. The external surface is smaller than it first looks. Nothing imports the search barrel or engineManager from outside; only providerHealth is reached, by deep path, from ten call sites. An early grep for 'utils/music/search' overcounted badly because it also matches utils/music/searchQueryCleaner, an unrelated module that does not move. Depth is unchanged, both trees sit three levels under src/, so the one outward relative import ('../../../types') and the sibling '../youtubeErrorHandler' both still resolve untouched. One reference lived outside src/, a jest.mock() literal in tests/handlers/player/playerFactory.test.ts, and a src-only scan missed it until the suite caught it. decisions/2026-05-16-dependabot-batch-handling-policy.md still names the old path. Left alone deliberately: it records where the code was in May, and editing an ADR to match a later move falsifies the record. Part of #1991. Remaining: titleComparison/service. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pkt4D9yyHTrVhsvDYvw5an
|
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
Relocates the music search and YouTube error-handling modules from utils/music to services/musicManagement, updating every importer (initializer, music command, errorEventHandlers, resilientStreamBridge, streamRecovery) and their spec mocks to the new paths. No behavioural change beyond the module layout; the moved SearchEngineManager, providerHealth, and enhancedSearch code is only reformatted.
No blocking issues surfaced. 5 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 261 functions depend on the 205 functions this change touches.
Health — this change adds coupling hotspots:
- new:
replenishQueue()— 19 callers, 9 callees - new:
collectLastFmCandidates()— 5 callers, 13 callees - new:
collectRecommendationCandidates()— 7 callers, 7 callees - new:
collectSeedSimilarCandidates()— 5 callers, 8 callees - new:
collectBroadFallbackCandidates()— 5 callers, 7 callees - new:
setupLifecycleHandlers()— 3 callers, 10 callees - new:
setupTrackHandlers()— 4 callers, 6 callees - new:
handlePlayerSkip()— 2 callers, 11 callees - …and 19 more — each is listed as a finding
Verification — 261 functions in the blast radius were not formally verified this run (proofs are advisory here).
Health delta baseline: last indexed commit 89c847c, 1 commit(s) behind this PR's base.
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: 244 function(s) in the blast radius were not formally verified this run
· 27 more finding(s) on lines outside this diff (see the check run).
SonarCloud failed this PR on new_coverage 63.4 percent against an 80 percent threshold. A pure move produces no new logic, but Sonar counts moved files as new code, so coverage that was always missing became gate-blocking here. Three files sat at 0 percent and had done so before the move: search/index.ts, search/searchContentOnYoutube.ts and youtubeErrorHandler/index.ts. The other three moved files were already above 94 percent. The new tests target the behaviour that actually branches rather than just executing the lines. For the search barrel that is engine selection, since pinning the engine is the only thing separating its four wrappers, including the playlist-vs-track choice. For the error barrel it is one case per message branch in createYouTubeErrorMessage, plus the error, warn and info routing in logYouTubeError, and the import-time singleton, which holds a different analyzer instance than a locally constructed handler and so needs stubbing separately. For searchContentOnYoutube it is the fallback chain, covering both an unsuccessful search and a success carrying no result, since both must fall through, plus the recoverable and unrecoverable error routes. Coverage on the moved set goes from 61.47 to 98.05 percent of lines. No threshold, exclusion or gate configuration was changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pkt4D9yyHTrVhsvDYvw5an
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.
Graphify review — findings
Moves the music search stack — providerHealthService, SearchEngineManager, analyzeYouTubeError, and the enhanced-search barrel — from utils/music to services/musicManagement, updating all importers (initializer, music command, player error/recovery/bridge handlers) and their test mocks accordingly. Adds index.spec.ts covering the EnhancedSearchService wrapper and the enhancedSearch/enhancedAutoSearch/enhancedYouTubeSearch/enhancedSpotifySearch helpers, asserting each delegates to the engine manager with the right preferred engine.
Worth a look
- search types import path removed —
packages/bot/src/utils/music/search/types.ts· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 277 functions depend on the 221 functions this change touches.
Health — this change adds coupling hotspots:
- new:
replenishQueue()— 19 callers, 9 callees - new:
collectLastFmCandidates()— 5 callers, 13 callees - new:
collectRecommendationCandidates()— 7 callers, 7 callees - new:
collectSeedSimilarCandidates()— 5 callers, 8 callees - new:
collectBroadFallbackCandidates()— 5 callers, 7 callees - new:
setupLifecycleHandlers()— 3 callers, 10 callees - new:
setupTrackHandlers()— 4 callers, 6 callees - new:
handlePlayerSkip()— 2 callers, 11 callees - …and 19 more — each is listed as a finding
Verification — 277 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: 260 function(s) in the blast radius were not formally verified this run
· 27 more finding(s) on lines outside this diff (see the check run).
|
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>
![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}
1 participant
Second slice of #1991, stacked behind #2353. Does not close it:
titleComparison/serviceremains.Why these two move together
Both are stateful service trees living under
utils/:search/engineManager.ts—SearchEngineManager, holds asetTimeoutsearch/providerHealth.ts—ProviderHealthService, holds a mutableMap<MusicProvider, ProviderStatus>youtubeErrorHandler/analyzer.ts—YouTubeErrorAnalyzer, behind aYouTubeErrorHandlerfacadesearch/engineManager.tsandsearch/searchContentOnYoutube.tsboth importyoutubeErrorHandler. Moving either directory alone would leaveutils/importingservices/, 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/searchreported 31 references. That was wrong: it also matchesutils/music/searchQueryCleaner, an unrelated module that does not move. The real surface:search/providerHealthyoutubeErrorHandlerNothing outside imports the
searchbarrel orengineManagerat all. Every external reference tosearch/is a deep import ofproviderHealth.Path arithmetic
Both trees sit three levels under
src/at source and destination, so relative depth is unchanged:'../../../types', still resolves tosrc/types'../youtubeErrorHandler'still resolves, because both directories movedOne reference lived outside
src/tests/handlers/player/playerFactory.test.ts:45holds ajest.mock()literal naming the old path. Asrc/-only scan missed it; the suite caught it asTest 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:21still names the oldyoutubeErrorHandlerpath. Left alone: that ADR records where the code was in May, and editing it to match a later move falsifies the record.Verification
packages/botfull suite: 273 suites passed, 3588 tests passed, 1 skipped, exactly the pre-move baseline (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: succeedssrc/andtests/: no remaining references outside the ADR noted above🤖 Generated with Claude Code
Summary by cubic
Moves the stateful music search and YouTube error-handling trees from
utils/musictoservices/musicManagementas the second slice of #1991, and adds missing test coverage for the moved barrels that SonarCloud was blocking on.searchContentOnYoutube, raising line coverage on the moved set from 61.47% to 98.05%.titleComparison/servicefor a later slice of refactor(bot): 4 more utils/music/ subdirectories contain stateful services, out of scope of #1968 #1991.Written for commit cbfefa3. Summary will update on new commits.