Conversation
Major refactoring to improve code organization and testability: Command Handler Extraction: - Create lib/command-handlers.js with dependency injection pattern - Extract 19 commands: playback (9), queue (5), volume (2), search (3) - Reduce index.js by ~500 lines Queue Utilities: - Create lib/queue-utils.js for sorting and track utilities - Extract pure functions for better testability Test Infrastructure: - Add comprehensive mocks for Sonos, Slack, Discord, Spotify - New test files: auth-handler, command-handlers, discord, queue-utils, setconfig, slack - Expand voting.test.mjs with direct module tests - Total: 511 unit tests passing Integration Tests: - Expand from 21 to 70 integration tests - Add pre-flight checks for clean state validation - Add queue verification before/after modifications - Keep volume safe (max 20, reset to 5) - Fix all validators to match actual bot responses Bug Fixes: - Fix votecheck to show track metadata when track leaves queue - Store track title/artist when votes are cast
There was a problem hiding this comment.
Pull request overview
Refactors SlackONOS by extracting several command implementations out of index.js into a dependency-injected lib/command-handlers.js, adds lib/queue-utils.js helpers, expands test/mocks + test suites, and adjusts voting votecheck output to retain metadata after a track leaves the queue.
Changes:
- Extracted playback/queue/volume/search command logic into
lib/command-handlers.jsand wired it intoindex.js. - Added
lib/queue-utils.jsand corresponding unit tests for search relevance sorting and queue helpers. - Enhanced voting
votecheckperformance and display by caching queue lookups and persisting voted-track metadata.
Reviewed changes
Copilot reviewed 26 out of 27 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| voting.js | Stores voted-track metadata to improve votecheck output after queue changes; optimizes queue matching. |
| test/voting.test.mjs | Adds module-level tests for voting behaviors with mocked dependencies. |
| test/tools/integration-test-suite.mjs | Expands/reshapes integration test ordering and adds pre-flight checks (per diff). |
| test/slack.test.mjs | Adds Slack-related tests (currently largely simulated logic). |
| test/setconfig.test.mjs | Adds setconfig validation tests (currently re-implements logic locally). |
| test/queue-utils.test.mjs | Adds unit tests for new queue utility functions. |
| test/mocks/spotify-mock.js | Adds a Spotify API mock factory for tests. |
| test/mocks/sonos-mock.js | Adds a Sonos API mock factory for tests. |
| test/mocks/slack-mock.js | Adds Slack WebClient/SocketMode mocks and helpers. |
| test/mocks/discord-mock.js | Adds Discord.js mocks and helpers. |
| test/mocks/index.js | Barrel export for test mocks. |
| test/discord.test.mjs | Adds Discord-related tests (currently largely simulated logic). |
| test/command-handlers.test.mjs | Adds tests for the extracted command handlers. |
| test/auth-handler.test.mjs | Adds tests for auth handler behavior. |
| templates/help/helpTextAdmin.txt | Adds admin help entry for diagnostics command. |
| templates/help/helpText.txt | Adds user help entry for featurerequest. |
| package.json | Adds lodash and bumps several dependency versions. |
| package-lock.json | Updates lockfile to match dependency changes. |
| lib/spotify-validator.js | Trailing whitespace/newlines change. |
| lib/sonos-discovery.js | Trailing whitespace/newlines change. |
| lib/slack-validator.js | Trailing whitespace/newlines change. |
| lib/queue-utils.js | New queue/search utility module. |
| lib/command-handlers.js | New extracted command handlers with dependency injection. |
| index.js | Wires in command-handlers, adds diagnostics, adjusts command registry + source detection logic, and improves featurerequest token handling/logging. |
| docs/DISCORD_DISCOVERY_CONTENT.md | Trailing whitespace/newlines change. |
| app.manifest.json | Trailing whitespace/newlines change. |
| .github/workflows/feature-request-enhance.yml | Workflow updated but currently contains unresolved merge conflict markers. |
| describe('Track Message Cleanup Logic', function() { | ||
| // Same logic as Slack - 1 hour max age | ||
| const TRACK_MESSAGE_MAX_AGE_MS = 60 * 60 * 1000; | ||
|
|
||
| function simulateCleanup(trackMessages, logger = null) { | ||
| const now = Date.now(); | ||
| const cutoff = now - TRACK_MESSAGE_MAX_AGE_MS; | ||
| let removedCount = 0; | ||
|
|
||
| for (const [messageId, data] of trackMessages.entries()) { | ||
| if (data.timestamp < cutoff) { | ||
| trackMessages.delete(messageId); | ||
| removedCount++; | ||
| } | ||
| } | ||
|
|
||
| if (removedCount > 0 && logger) { | ||
| logger.debug(`Cleaned up ${removedCount} old track messages`); | ||
| } | ||
|
|
||
| return removedCount; | ||
| } | ||
|
|
There was a problem hiding this comment.
Similar to the Slack tests, most of these assertions validate locally defined helper functions instead of importing/testing the real discord.js integration code. This can drift from actual behavior and won’t catch regressions in the production module. Consider extracting/exporting the tested logic from the Discord integration and importing it here so these tests meaningfully increase coverage.
| try { | ||
| const tracks = await spotify.searchTrackList(term, searchLimit || 10); | ||
|
|
||
| if (!tracks || tracks.length === 0) { |
There was a problem hiding this comment.
search/searchalbum/searchplaylist call spotify.* unconditionally, but initialize() allows deps.spotify to be omitted (sets spotify = null). If the bot is run without Spotify configured, these commands will throw at runtime. Either require deps.spotify during initialization (when these commands are registered) or add a guard in each search handler to send a user-facing message when Spotify isn’t available.
| result.items.map(function (item, i) { | ||
| let trackTitle = item.title; | ||
| let prefix = ''; | ||
|
|
||
| // Match by position OR by title/artist | ||
| const positionMatch = track && (i + 1) === track.queuePosition; | ||
| const nameMatch = track && item.title === track.title && item.artist === track.artist; | ||
| const isCurrentTrack = positionMatch || (nameMatch && isFromQueue); | ||
|
|
||
| // Check if track is gong banned (immune) | ||
| const isImmune = voting && voting.isTrackGongBanned({ title: item.title, artist: item.artist, uri: item.uri }); | ||
| if (isImmune) { | ||
| prefix = ':lock: '; | ||
| trackTitle = item.title; | ||
| } else if (isCurrentTrack && isFromQueue) { | ||
| trackTitle = '*' + trackTitle + '*'; | ||
| } else { | ||
| trackTitle = '_' + trackTitle + '_'; | ||
| } | ||
|
|
||
| // Add star prefix for tracks with active votes | ||
| const hasVotes = voting && voting.hasActiveVotes(i, item.uri, item.title, item.artist); | ||
| if (hasVotes) { | ||
| prefix = ':star: ' + prefix; | ||
| } | ||
|
|
||
| if (isCurrentTrack && isFromQueue) { | ||
| tracks.push(':notes: ' + '_#' + i + '_ ' + trackTitle + ' by ' + item.artist); | ||
| } else { | ||
| tracks.push(prefix + '>_#' + i + '_ ' + trackTitle + ' by ' + item.artist); | ||
| } | ||
| }); |
There was a problem hiding this comment.
result.items.map(...) is being used only for side effects (pushing into tracks). Using .forEach() here would better reflect intent and avoid confusion about the unused returned array.
| // Config definitions from index.js | ||
| const allowedConfigs = { | ||
| gongLimit: { type: 'number', min: 1, max: 20 }, | ||
| voteLimit: { type: 'number', min: 1, max: 20 }, | ||
| voteImmuneLimit: { type: 'number', min: 1, max: 20 }, | ||
| flushVoteLimit: { type: 'number', min: 1, max: 20 }, | ||
| maxVolume: { type: 'number', min: 0, max: 100 }, | ||
| searchLimit: { type: 'number', min: 1, max: 50 }, | ||
| voteTimeLimitMinutes: { type: 'number', min: 1, max: 60 }, | ||
| themePercentage: { type: 'number', min: 0, max: 100 }, | ||
| crossfadeDurationSeconds: { type: 'number', min: 0, max: 30 }, | ||
| aiModel: { type: 'string', minLen: 1, maxLen: 50, allowed: ['gpt-4o', 'gpt-4o-mini', 'gpt-4-turbo', 'gpt-3.5-turbo'] }, | ||
| aiPrompt: { type: 'string', minLen: 1, maxLen: 500 }, | ||
| defaultTheme: { type: 'string', minLen: 0, maxLen: 100 }, | ||
| telemetryEnabled: { type: 'boolean' }, | ||
| soundcraftEnabled: { type: 'boolean' }, | ||
| soundcraftIp: { type: 'string', minLen: 0, maxLen: 50 }, | ||
| crossfadeEnabled: { type: 'boolean' }, | ||
| slackAlwaysThread: { type: 'boolean' }, | ||
| logLevel: { type: 'string', minLen: 4, maxLen: 5, allowed: ['error', 'warn', 'info', 'debug'] } | ||
| }; | ||
|
|
||
| // Validation functions from index.js | ||
| function normalizeKey(key) { | ||
| return Object.keys(allowedConfigs).find(k => k.toLowerCase() === key.toLowerCase()); | ||
| } | ||
|
|
||
| function validateNumber(value, configDef) { | ||
| const numValue = Number(value); | ||
| if (isNaN(numValue)) { | ||
| return { valid: false, error: 'not_a_number' }; | ||
| } | ||
| if (numValue < configDef.min || numValue > configDef.max) { | ||
| return { valid: false, error: 'out_of_range', min: configDef.min, max: configDef.max }; | ||
| } | ||
| return { valid: true, value: numValue }; | ||
| } | ||
|
|
||
| function validateString(value, configDef) { | ||
| if (value.length < (configDef.minLen || 0) || value.length > (configDef.maxLen || 500)) { | ||
| return { valid: false, error: 'invalid_length', minLen: configDef.minLen, maxLen: configDef.maxLen }; | ||
| } | ||
| if (configDef.allowed) { | ||
| const matchedValue = configDef.allowed.find(a => a.toLowerCase() === value.toLowerCase()); | ||
| if (!matchedValue) { | ||
| return { valid: false, error: 'not_allowed', allowed: configDef.allowed }; | ||
| } | ||
| return { valid: true, value: matchedValue }; | ||
| } | ||
| return { valid: true, value }; | ||
| } | ||
|
|
||
| function validateBoolean(value) { | ||
| const lowerValue = value.toLowerCase(); | ||
| if (lowerValue === 'true' || lowerValue === '1' || lowerValue === 'yes' || lowerValue === 'on') { | ||
| return { valid: true, value: true }; | ||
| } | ||
| if (lowerValue === 'false' || lowerValue === '0' || lowerValue === 'no' || lowerValue === 'off') { | ||
| return { valid: true, value: false }; | ||
| } | ||
| return { valid: false, error: 'invalid_boolean' }; | ||
| } |
There was a problem hiding this comment.
These tests re-implement the allowedConfigs map and validation logic inline instead of exercising the real _setconfig implementation in index.js. This can drift from production behavior and gives a false sense of coverage. Consider extracting the setconfig validation/normalization into a shared module (e.g., lib/setconfig-utils.js) and importing it both in index.js and in these tests so the tests cover the actual logic.
| describe('Track Message Cleanup Logic', function() { | ||
| // Simulate the cleanup logic from slack.js | ||
| const TRACK_MESSAGE_MAX_AGE_MS = 60 * 60 * 1000; // 1 hour | ||
|
|
||
| function simulateCleanup(trackMessages, logger = null) { | ||
| const now = Date.now(); | ||
| const cutoff = now - TRACK_MESSAGE_MAX_AGE_MS; | ||
| let removedCount = 0; | ||
|
|
||
| for (const [messageKey, data] of trackMessages.entries()) { | ||
| if (data.timestamp < cutoff) { | ||
| trackMessages.delete(messageKey); | ||
| removedCount++; | ||
| } | ||
| } | ||
|
|
||
| if (removedCount > 0 && logger) { | ||
| logger.debug(`Cleaned up ${removedCount} old track messages`); | ||
| } | ||
|
|
||
| return removedCount; | ||
| } | ||
|
|
There was a problem hiding this comment.
This file mostly tests locally defined helper functions that simulate behavior from slack.js rather than importing and testing the real module behavior. That means failures/regressions in slack.js won’t necessarily be caught by these tests. Consider exporting the relevant helpers/constants from slack.js (or extracting them into a small shared module) and importing them here so the tests cover production code paths.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| .then((result) => { | ||
| if (cb) { | ||
| return cb(result.total); | ||
| } |
There was a problem hiding this comment.
Deprecated function left as dead code
Low Severity
The _upNextDeprecated function is defined but never called. The command registry at line 2901 uses commandHandlers.upNext instead. This function appears to have been renamed during the refactoring but left behind as dead code rather than being removed.
| } | ||
|
|
||
| return bScore - aScore; | ||
| }); |
There was a problem hiding this comment.
Sorting functions duplicated between index.js and queue-utils.js
Medium Severity
The sorting functions sortAlbumsByRelevance, sortPlaylistsByRelevance, and sortTracksByRelevance in queue-utils.js duplicate nearly identical implementations of _sortAlbumsByRelevance, _sortPlaylistsByRelevance, and _sortTracksByRelevance that remain in index.js. Both sets are actively used: the index.js versions by add commands, and queue-utils versions by search commands. This creates maintenance burden and risk of inconsistent bug fixes.
Additional Locations (1)
| "@slack/web-api": "^7.12.0", | ||
| "bcrypt": "^6.0.0", | ||
| "discord.js": "^14.17.3", | ||
| "lodash": "^4.17.23", |
There was a problem hiding this comment.
Unused lodash dependency added to package.json
Low Severity
The lodash package was added as a direct dependency in package.json but is never imported or used anywhere in the codebase. No require('lodash') or lodash function usage exists in any of the changed files or elsewhere in the project.
Summary
index.jsintolib/command-handlers.jsusing dependency injection pattern (19 commands: playback, queue, volume, search)lib/queue-utils.jsfor sorting and track helper functionsChanges
lib/command-handlers.js,lib/queue-utils.jstest/mocks/(sonos, slack, discord, spotify)Test plan
npm test)npm run test:integration)Note
Scope
index.jsintolib/command-handlers.jswith dependency injection; updatesindex.jsto initialize and route commands to handlerslib/queue-utils.jsfor shared sorting/queue helpers; replaces some loops withfindIndexfor minor perf/clarityqueuePosition; addsdiagnosticsadmin command for deep checksBehavioral/UX tweaks
featurerequestnow non-admin and requiresgithubToken; improves error/loggingsetconfiggainslogLevel(and help text updated); help templates mentionfeaturerequestanddiagnosticsTests & tooling
test/mocks/; add unit tests for auth, handlers, queue utils, Slack/Discord, setconfiglodash, bumpsopenai,posthog-node,selfsigned,soundcraft-ui-connection)Notes
index.jswith moved functions and minor refactors (e.g., duplicate detection, gong cleanup)Written by Cursor Bugbot for commit 0548de4. This will update automatically on new commits. Configure here.