Add multi-bot voice architecture with audio channels and progressive updates #36
Conversation
…ents, and event-based slash command registration
…onsistently, add connectionWatchdog cleanup
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds multi-bot Discord support, a broadcast-style multi-channel audio pipeline with sinks and connection/management, progressive in-place message updates, voice-status/listening actions, data-URL attachment utilities, many dynamic providers, expanded types/exports, and accompanying tests and helpers. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer/User
participant Registry as DiscordClientRegistry
participant Service as DiscordService
participant ConnMgr as VoiceConnectionManager
participant VM as VoiceManager
participant Sink as DiscordAudioSink
Dev->>Registry: registerBot(config)
Registry->>Service: create client + attach VoiceManager
Service->>ConnMgr: registerConnection(botId,guildId,channelId,channel,voiceManager)
Dev->>Service: request playAudio(stream, channel)
Service->>VM: playAudio(stream, channel)
VM->>Sink: feed(stream)
Sink-->>VM: emit statusChange / error
ConnMgr->>Service: expose VoiceTarget status
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)src/clientRegistry.ts (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (12)
✏️ Tip: You can disable this entire section by setting 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 |
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive multi-bot voice token support to the Discord plugin, enabling multiple Discord bots to manage voice connections across different channels. Key enhancements include progressive message updates for real-time user feedback, a priority-based multi-channel audio system, and voice connection health monitoring.
Key Changes:
- Multi-bot infrastructure with token validation and client registry
- Progressive message updates that edit Discord messages in real-time
- Priority-based audio channel system (TTS, Music, SFX, Ambient)
- Voice connection health monitoring and auto-reconnection
- Audio sink abstraction for plugin interoperability
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/voiceConnectionManager.ts | New voice connection tracker for multiple bots across guilds |
| src/voice.ts | Major refactor with multi-channel audio, health monitoring, ducking |
| src/clientRegistry.ts | Multi-bot client management with token validation |
| src/service.ts | Updated initialization for multi-bot support and auto-join |
| src/messages.ts | Progressive message tracking with TTL and correlation IDs |
| src/progressiveMessage.ts | Helper for throttled, debounced message updates |
| src/audioChannels.ts | Priority-based audio channel configuration |
| src/contracts.ts | IAudioSink interface for audio playback abstraction |
| src/sinks/discordAudioSink.ts | Discord implementation of IAudioSink |
| src/types.ts | New interfaces for multi-bot, VoiceTarget, audio settings |
| src/utils.ts | Added editMessageContent for message editing |
| src/environment.ts | New voice ducking and threshold settings |
| src/actions/* | New actions for voice channel status and listening activity |
| src/providers/audioState.ts | Provider for bot audio state (mute/deafen) |
| tests/* | Test coverage for new features |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
300-322: Remove duplicate "Attachment Handler" section.There are two "Attachment Handler" sections:
- Lines 306-309 (under new components)
- Lines 318-322 (duplicate)
Remove the duplicate at lines 318-322.
🔎 Proposed fix
### Attachment Handler - Downloads and processes Discord attachments - Supports various media types - Integrates with media transcription ### VoiceManager - Manages voice channel interactions - Handles joining and leaving voice channels - Processes voice events and audio streams - Integrates with transcription services -### Attachment Handler - -- Downloads and processes Discord attachments -- Supports various media types -- Integrates with media transcription - ## Developer Guide
🧹 Nitpick comments (28)
src/attachments.ts (1)
104-108: Leftover TODO comment should be resolved or removed.The comment
// 'audio/wav' default?appears to be an open question or debugging note. The current code correctly handles allaudio/*content types generically (preserving the original buffer and mime type), which is appropriate behavior.If there's a specific concern about WAV handling, please clarify or create an issue. Otherwise, consider removing this comment before merging.
PROGRESSIVE_UPDATES.md (1)
10-14: Consider adding language hints to fenced code blocks.Static analysis flagged these blocks as missing language specifiers. While intentionally plain text for user/bot interaction examples, adding a language hint (e.g.,
```text) would satisfy linters and improve syntax highlighting consistency.Also applies to: 21-30, 125-130, 143-146
src/actions/setListeningActivity.ts (2)
160-165: Use runtime.logger instead of console.error for consistency.The codebase uses
runtime.loggerfor structured logging. Replaceconsole.errorcalls withruntime.logger.errorfor consistency and to enable proper log aggregation.🔎 Proposed fix
} catch (error) { - console.error('Error setting listening activity:', error); + runtime.logger.error({ + src: 'plugin:discord', + action: 'SET_LISTENING_ACTIVITY', + error: error instanceof Error ? error.message : String(error), + }, 'Error setting listening activity'); await callback({ text: 'I encountered an error while trying to update my listening activity.', source: 'discord', }); }
102-108: Same logging consistency issue here.if (!activityInfo) { - console.error("Couldn't parse listening activity information from message"); + runtime.logger.error("Couldn't parse listening activity information from message");src/providers/channelState.ts (1)
62-63: Remove the uncertain comment or resolve the question.The comment
// is ServiceType.DISCORD better?suggests unresolved design uncertainty. Since this PR is standardizing onDISCORD_SERVICE_NAMEfor service lookups (as seen consistently in other files likesrc/tests.ts), this comment should either be removed or the question should be resolved before merging.Proposed fix
- // is ServiceType.DISCORD better? const discordService = runtime.getService(DISCORD_SERVICE_NAME) as DiscordService;__tests__/voiceManager.test.ts (2)
117-148: Address potential test flakiness and unhandled promise.Two concerns with this test:
Unhandled promise:
playPromiseis never awaited or handled, which could lead to unhandled rejection warnings if the promise rejects.Arbitrary delay: The 50ms
setTimeout(line 140) is a race condition waiting to happen in CI environments. Consider using a deterministic approach.Proposed improvement
// Start playback but don't await fully - we just want to check setup const playPromise = voiceManager.playAudio(originalStream, { guildId }); - // Wait a bit for async operations - await new Promise(resolve => setTimeout(resolve, 50)); + // Wait for demuxProbe to be called + await vi.waitFor(() => { + expect(demuxProbeMock).toHaveBeenCalled(); + }, { timeout: 100 }); expect(demuxProbeMock).toHaveBeenCalledWith(originalStream); expect(createAudioResourceMock).toHaveBeenCalledWith(demuxedStream, { inputType: 'opus', inlineVolume: true }); // Clean up originalStream.destroy(); demuxedStream.destroy(); + + // Ensure promise is handled to avoid unhandled rejection + playPromise.catch(() => { /* expected during cleanup */ });
150-203: Consider simplifying the assertion for userStream event.The test correctly validates that no empty-stream warning is logged and that the
userStreamevent is emitted. However, the JSON.stringify check on line 194 for detecting warnings could be brittle if the log format changes.Alternative assertion approach
// Check that no warning about empty stream was logged const warningCalls = warnSpy.mock.calls; const hasEmptyStreamWarning = warningCalls.some((call) => { const msg = call[0]; - return typeof msg === 'object' && JSON.stringify(msg).includes('No receiveStream'); + if (typeof msg === 'object' && msg !== null) { + return Object.values(msg).some(v => + typeof v === 'string' && v.includes('No receiveStream') + ); + } + return typeof msg === 'string' && msg.includes('No receiveStream'); });__tests__/token-validation.test.ts (3)
1-1: Inconsistent test framework usage.This file uses
bun:testwhile other test files in this PR (e.g.,voiceManager.test.ts,multibot-simulcast.test.ts,progressiveMessage.test.ts) usevitest. Consider standardizing on one test framework to maintain consistency and avoid potential CI configuration issues.Convert to vitest for consistency
-import { describe, it, expect } from 'bun:test'; +import { describe, it, expect } from 'vitest';
85-91: Token format validation may be too lenient.The mock token
'AAAAAAAAAAAAAAAAAAAAAA.AAAAAA.AAAAAAAAAAAAAAAAAAAAAAAAA_a'has 55 characters, just above the 50-character threshold. Real Discord tokens are typically 70+ characters. Consider using a more realistic mock token length to ensure the validation properly handles real-world token sizes.
3-41: ExportvalidateDiscordTokenfor direct testing.The test function duplicates the actual validation logic from
src/clientRegistry.tsrather than testing the real implementation. If the actual implementation changes, these tests won't catch regressions. ExportvalidateDiscordTokenso tests can directly invoke the source function instead of maintaining parallel logic.__tests__/multibot-simulcast.test.ts (1)
63-78: Timing-based assertions may be flaky in CI.The assertions on lines 70 and 77 rely on timing thresholds (100ms and 50ms) which can fail in resource-constrained CI environments or under load. Consider:
- Increasing the thresholds with a buffer
- Making the assertions relative rather than absolute
- Skipping timing assertions in CI if necessary
Proposed adjustment for reliability
- // Assert: Route should complete quickly (under 100ms for simulcast) - expect(routeTime).toBeLessThan(100); + // Assert: Route should complete quickly (under 500ms for simulcast, allowing CI variance) + expect(routeTime).toBeLessThan(500); // Verify all targets started within acceptable window const playbackTimes = mockTargets.map(t => (t as any).playbackStartTime); const maxDelta = Math.max(...playbackTimes) - Math.min(...playbackTimes); - // All targets should start within 50ms of each other - expect(maxDelta).toBeLessThan(50); + // All targets should start within 200ms of each other (allowing CI variance) + expect(maxDelta).toBeLessThan(200);__tests__/progressiveMessage.test.ts (1)
99-111: Correlation ID test provides limited value.The test comment acknowledges that internal correlation IDs can't be tested directly, and the test ends with
expect(true).toBe(true)which is essentially a no-op. Consider either:
- Removing this test if correlation IDs truly can't be validated
- Testing observable behavior that depends on correlation IDs (e.g., that updates don't interfere between instances)
Potential improvement
describe('Correlation ID', () => { - it('should generate unique correlation IDs for different instances', () => { + it('should allow concurrent instances without interference', async () => { const progress1 = new ProgressiveMessage(mockCallback, 'discord'); const progress2 = new ProgressiveMessage(mockCallback, 'discord'); - // Both should have different internal correlation IDs - // (We can't test this directly, but we test the behavior) - progress1.update('Test 1'); - progress2.update('Test 2'); + // Both instances should complete independently + await Promise.all([ + progress1.complete('Done 1'), + progress2.complete('Done 2'), + ]); - expect(true).toBe(true); // Simple sanity check + // Both should have sent their respective messages + const texts = calledWith.map(c => c.text); + expect(texts).toContain('Done 1'); + expect(texts).toContain('Done 2'); }); });VOICE_STATUS_FEATURES.md (1)
82-93: Document potential API availability concerns.The REST API call to
/channels/{channel.id}/voice-statusmay not be available in all Discord API versions or may require specific bot permissions. Consider adding a note about API availability and fallback behavior when the endpoint is not accessible.__tests__/multibot-independent.test.ts (3)
45-72: Consider using a more deterministic wait mechanism instead of fixed timeouts.The 100ms timeout on line 57 for waiting for stream data to be processed could lead to flaky tests on slower CI environments.
🔎 Suggestion: Use event-based or polling wait
// Wait for stream data to be processed - await new Promise(resolve => setTimeout(resolve, 100)); + // Wait for all targets to receive their stream type + await Promise.all(mockTargets.map(target => + new Promise<void>(resolve => { + const checkInterval = setInterval(() => { + if ((target as any).streamType) { + clearInterval(checkInterval); + resolve(); + } + }, 10); + // Fallback timeout + setTimeout(() => { clearInterval(checkInterval); resolve(); }, 500); + }) + ));
135-153: Incomplete test with commented-out assertions.This test has no actual assertions - the volume control API is hypothetical. Consider either implementing the test when the API is ready, or removing/skipping the test entirely with a clear TODO issue reference.
🔎 Suggestion: Mark as skipped with explanation
- it('should allow independent volume control per zone', async () => { + it.skip('should allow independent volume control per zone', async () => { // Note: This would require VoiceTarget to support volume control - // This test demonstrates the API design + // TODO: Implement when VoiceTarget gains setVolume() method + // Issue: <link-to-tracking-issue>
274-279: 50ms timeout fallback could mask test issues.This timeout ensures the play promise resolves even without data, but it could mask actual bugs where streams don't emit data when they should.
Consider logging when the timeout fallback is used to aid debugging:
// Timeout fallback - resolve after 50ms if no data setTimeout(() => { if (!resolved) { resolved = true; + console.warn(`[Test] Mock target ${target.id} play resolved via timeout fallback`); resolve(); } }, 50);src/sinks/discordAudioSink.ts (2)
75-78: Hardcoded channel value may limit flexibility.The
channel: 1is hardcoded for "Music channel". If the multi-channel audio system supports different channel priorities, this should be configurable.🔎 Consider making channel configurable
+import { CHANNEL_MUSIC } from '../audioChannels'; + export class DiscordAudioSink extends EventEmitter implements IAudioSink { readonly id: string; private guildId: string; private voiceManager: VoiceManager; private _status: AudioSinkStatus = 'disconnected'; + private channel: number; - constructor(id: string, guildId: string, voiceManager: VoiceManager) { + constructor(id: string, guildId: string, voiceManager: VoiceManager, channel: number = CHANNEL_MUSIC) { super(); this.id = id; this.guildId = guildId; this.voiceManager = voiceManager; + this.channel = channel; // ... } async feed(stream: Readable): Promise<void> { // ... await this.voiceManager.playAudio(stream, { guildId: this.guildId, - channel: 1, // Music channel + channel: this.channel, }); }
147-148: Field declarations should be at the top of the class.The
connectionPollIntervalandconnectionAttachedfields are declared mid-file after the method that uses them. Moving them to the top with other fields improves readability.🔎 Move field declarations
export class DiscordAudioSink extends EventEmitter implements IAudioSink { readonly id: string; private guildId: string; private voiceManager: VoiceManager; private _status: AudioSinkStatus = 'disconnected'; + private connectionPollInterval: NodeJS.Timeout | null = null; + private connectionAttached = false; constructor(id: string, guildId: string, voiceManager: VoiceManager) { // ... } // ... methods ... - private connectionPollInterval: NodeJS.Timeout | null = null; - private connectionAttached = false;src/types.ts (1)
566-575: Consider adding validation constraints to DiscordBotConfig.The
tokenfield should ideally have a minimum length or format hint in the documentation, as Discord tokens have a specific format. Also,autoJoincould benefit from documenting that these should be voice channel IDs.src/actions/setVoiceChannelStatus.ts (1)
89-91: Empty catch block silently swallows errors.When channel fetch by ID fails, the error is silently ignored. Consider logging at debug level for troubleshooting.
🔎 Proposed fix
} catch (e) { - // ID not found, continue to name search + // ID not found or access denied, continue to name search + // Could log at debug level if a logger were available }Alternatively, pass the runtime logger to this function for better observability.
src/voiceConnectionManager.ts (1)
42-42: Consider usinglogger.infoorlogger.debuginstead oflogger.log.The
logger.logmethod is less common in structured logging. For connection lifecycle events,logger.infoorlogger.debugwould be more appropriate for filtering and observability.Also applies to: 51-51
src/service.ts (1)
2196-2379:handleAutoJoinVoiceChannelis well-documented but very long.The method is ~180 lines with excellent inline documentation explaining the multi-bot auto-join logic. While the comments are valuable, consider extracting helper functions for testability and readability.
The core logic could be split into:
parseAutoJoinChannelIds()- Parse and validate channel IDsfindAvailableBotForChannel()- Find a bot that can join a specific channelreportAutoJoinResults()- Log the summaryThis would make the main method easier to follow while preserving the detailed documentation.
src/voice.ts (6)
252-263: Consider stronger typing forresourceandvolumeTransformer.Using
anyfor these fields reduces type safety. Consider importing and using proper types from@discordjs/voice:Suggested type improvements
+import type { AudioResource, VolumeTransformer } from '@discordjs/voice'; + interface ChannelPlayerState { player: AudioPlayer; channel: number; guildId: string; - resource: any; + resource: AudioResource; finished: () => void; cancelled: () => void; abortController?: AbortController; originalVolume?: number; duckedVolume?: number; - volumeTransformer?: any; + volumeTransformer?: VolumeTransformer; }
749-754: Minor typo in comment.Line 749 has a typo: "autopausesdue" should be "autopauses due".
- // This handles network hiccups where the player autopausesdue to missing connection + // This handles network hiccups where the player autopauses due to missing connection
1435-1512: Consider extractingisValidTranscriptionto a module-level function.This function is quite comprehensive (80+ lines) and could benefit from being extracted outside of
processTranscriptionfor:
- Better testability
- Potential reuse by other components
- Improved readability of the parent method
Suggested refactor
// Move to module level (before VoiceManager class) function isValidTranscription(text: string): boolean { if (!text || text.trim().length < 2) return false; // ... rest of implementation }
1670-1686: Stream type handling can be simplified.Lines 1670-1679 manually handle stream type conversion, but the comment at line 1676 notes that
playAudio()handles Web ReadableStream conversion internally (lines 2087-2111). Consider passing the stream directly:Simplified approach
if (responseStream) { - let audioStream: Readable; - if (Buffer.isBuffer(responseStream)) { - audioStream = Readable.from(responseStream, { objectMode: false }); - } else if (responseStream instanceof Readable) { - audioStream = responseStream; - } else { - // playAudio() handles Web ReadableStream conversion internally - // For other types, try to wrap with Readable.from() - audioStream = responseStream as any; - } + // playAudio() handles Buffer, Readable, and Web ReadableStream internally + const audioStream = Buffer.isBuffer(responseStream) + ? Readable.from(responseStream, { objectMode: false }) + : responseStream; // Use mix: true so TTS ducks music instead of stopping it await this.playAudio(audioStream, {
2817-2839: Consider adding error handling for connection destruction during bridge.If the source connection is destroyed while the bridge is active, the speaking listener may continue firing and cause errors. Consider adding connection state validation:
Suggested improvement
// Listen for speaking events sourceConn.receiver.speaking.on('start', speakingStartHandler); + + // Handle source connection destruction + sourceConn.on('stateChange', (_, newState) => { + if (newState.status === VoiceConnectionStatus.Destroyed) { + bridgeCleanup(); + } + });
1114-1120: UsingclearTimeoutonsetIntervalhandle.
rampTimeris created withsetInterval(line 2563) but cleared withclearTimeouthere and at line 1943. While Node.js allows this, it's technically incorrect and could be confusing.Suggested fix
- if (duckState.rampTimer) { - clearTimeout(duckState.rampTimer as any); - } + if (duckState.rampTimer) { + clearInterval(duckState.rampTimer); + }Apply similarly at line 1943 in
stopChannelPlayer.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (31)
COORDINATION_PATTERNS.mdMUSIC_ARCHITECTURE.mdPROGRESSIVE_UPDATES.mdREADME.mdVOICE_STATUS_FEATURES.md__tests__/messageManager.test.ts__tests__/multibot-independent.test.ts__tests__/multibot-simulcast.test.ts__tests__/progressiveMessage.test.ts__tests__/token-validation.test.ts__tests__/voiceManager.test.tssrc/actions/setListeningActivity.tssrc/actions/setVoiceChannelStatus.tssrc/attachments.tssrc/audioChannels.tssrc/clientRegistry.tssrc/contracts.tssrc/environment.tssrc/index.tssrc/messages.tssrc/progressiveMessage.tssrc/providers/audioState.tssrc/providers/channelState.tssrc/service.tssrc/sinks/discordAudioSink.tssrc/sinks/index.tssrc/tests.tssrc/types.tssrc/utils.tssrc/voice.tssrc/voiceConnectionManager.ts
🧰 Additional context used
🧬 Code graph analysis (8)
src/providers/audioState.ts (1)
src/voice.ts (1)
getVoiceConnection(825-839)
src/messages.ts (1)
src/utils.ts (3)
editMessageContent(768-793)getAttachmentFileName(187-233)sendMessageInChunks(309-486)
src/contracts.ts (1)
src/sinks/discordAudioSink.ts (1)
status(60-62)
src/progressiveMessage.ts (1)
src/service.ts (1)
delay(2831-2833)
__tests__/messageManager.test.ts (2)
src/types.ts (1)
IDiscordService(478-492)src/messages.ts (1)
MessageManager(43-820)
src/index.ts (3)
src/service.ts (2)
setVoiceChannelStatus(3799-3848)setListeningActivity(3858-3884)src/providers/audioState.ts (1)
audioStateProvider(16-164)src/permissions.ts (1)
getPermissionValues(238-247)
src/clientRegistry.ts (2)
src/voice.ts (1)
VoiceManager(318-2910)src/index.ts (1)
DiscordEventTypes(175-175)
src/voice.ts (4)
src/index.ts (4)
AudioChannelConfig(30-30)PlaybackHandle(30-30)DEFAULT_CHANNEL_CONFIGS(36-36)DiscordEventTypes(175-175)src/compat.ts (1)
ICompatRuntime(53-58)src/environment.ts (1)
getDiscordSettings(93-194)src/sinks/discordAudioSink.ts (1)
status(60-62)
🪛 LanguageTool
COORDINATION_PATTERNS.md
[style] ~95-~95: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ... Service Discovery ### Problem Plugins need to connect to each other at runtime. ### ...
(REP_NEED_TO_VB)
PROGRESSIVE_UPDATES.md
[grammar] ~102-~102: Ensure spelling is correct
Context: ...s don't expire. ### 3. Why Debouncing (300ms minDelay)? Problem: If an operatio...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~106-~106: Ensure spelling is correct
Context: ...Solution*: The first update() waits 300ms before sending. If complete() is call...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~114-~114: Ensure spelling is correct
Context: ...s like waiting. ### 4. Why Throttling (500ms between updates)? Problem: Discord...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~118-~118: Ensure spelling is correct
Context: ...ger rate limits. Solution: Enforce 500ms minimum between updates. If update() ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~120-~120: Ensure spelling is correct
Context: ...ebounce and send the last value. Why 500ms?: Allows ~2 edits/second (10 per 5 sec...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~164-~164: Ensure spelling is correct
Context: ...er API (no flag to forget). ### 7. Why isInterim Flag Instead of isFinal? Reason: D...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
PROGRESSIVE_UPDATES.md
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
21-21: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
125-125: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
143-143: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Upload results
- GitHub Check: Cursor Bugbot
🔇 Additional comments (52)
src/utils.ts (2)
435-435: Good type narrowing improvement.The explicit type guard
(c): c is ButtonBuilder | StringSelectMenuBuilder => c !== nullproperly narrows the array type, eliminating the need for downstream type assertions.
750-793: Well-designed utility with appropriate error handling.The
editMessageContentfunction follows good practices:
- Returns
nullon failure for graceful degradation- Validates empty content upfront
- Truncates to Discord's limit with ellipsis indicator
- Comprehensive JSDoc explaining design decisions
Minor note: The truncation math is correct (1997 + 3 = 2000 chars max).
PROGRESSIVE_UPDATES.md (1)
1-50: Excellent documentation with clear rationale.The progressive updates documentation thoroughly explains the architecture, design decisions (correlation IDs, TTL, debouncing, throttling), and provides actionable usage guidance. The "why" explanations are particularly valuable for future maintainers.
README.md (1)
567-672: Comprehensive audio system documentation.The Multi-Channel Audio System and Audio Sink Integration sections provide clear explanations of:
- Predefined channels and priority-based behavior
- Custom channel registration
- IAudioSink interface usage
- Auto-reconnection and plugin integration
This will help developers integrate with the audio system effectively.
src/audioChannels.ts (2)
1-94: Excellent documentation with clear design rationale.The inline documentation thoroughly explains the "why" behind each channel's priority, pausability, and interruptibility settings. This will be valuable for maintainers and developers extending the audio system.
165-178: Correct priority-based interrupt logic.The
canInterruptfunction correctly implements the priority system:
- Returns
falseif configs are missing (safe default)- Requires both higher priority AND existing channel to be interruptible
- CHANNEL_TTS (interruptible: false) cannot be interrupted as intended
src/clientRegistry.ts (4)
24-59: Good token validation with helpful error messages.The
validateDiscordTokenfunction provides thorough format validation with actionable error messages. The checks for empty strings, literal "undefined"/"null", length, and dot-separated format are practical heuristics that catch common misconfiguration issues.
340-350: Verify iteration safety in destroyAll.The code uses
Array.from(this.clients.keys())to create a snapshot before iterating, which correctly avoids modifying the map during iteration. This is safe.One consideration:
removeBotmay delete both the botId and alias keys (lines 318-321), so if multiple keys point to the same client, subsequent deletions may warn about "not found." This is harmless but could be cleaner.
202-263: Login timeout and error handling look solid.The 30-second timeout is appropriate for Discord login. The error handler on the client properly logs errors, and the Promise correctly rejects on login failure with cleanup.
424-433: Smart optimization for large guilds.Skipping user pre-fetch for guilds with >1000 members and discovering users organically is a good performance optimization. The background sync for smaller guilds is appropriately non-blocking.
src/sinks/index.ts (1)
1-2: Clean barrel export.Standard re-export pattern that exposes
DiscordAudioSinkfrom the sinks module.src/providers/channelState.ts (1)
5-5: LGTM!The import change from
ServiceTypetoDISCORD_SERVICE_NAMEaligns with the PR's goal of standardizing service discovery using public constants.src/tests.ts (2)
13-13: LGTM!Consistent import of
DISCORD_SERVICE_NAMEconstant for standardized service discovery.
76-76: LGTM!Service lookup using
DISCORD_SERVICE_NAMEis consistent with the PR's standardization approach and matches the pattern established across other files.COORDINATION_PATTERNS.md (1)
1-9: Good documentation structure.The document provides a clear overview of the broadcast architecture with well-organized sections. The status and last-updated metadata help track currency.
__tests__/voiceManager.test.ts (1)
16-32: LGTM!Comprehensive mocking of
@discordjs/voicemodule with all necessary exports for testing the audio pipeline.__tests__/multibot-simulcast.test.ts (1)
204-232: Mock target's play() resolves immediately but has async side effects.The
play()method resolves immediately (line 230) while stream handlers continue to run asynchronously. This design choice is intentional (as per the comment on lines 228-229) but could lead to test flakiness if assertions run before stream handlers execute.The tests mitigate this with
setTimeoutdelays, but consider documenting this behavior more explicitly or using a more deterministic synchronization mechanism.__tests__/progressiveMessage.test.ts (2)
1-23: LGTM!Clean test setup with a well-structured mock callback that captures all invocations for assertion.
113-129: LGTM!Good test coverage for source field validation, ensuring both Discord and non-Discord sources are handled correctly.
VOICE_STATUS_FEATURES.md (1)
1-9: LGTM!Clear introduction documenting the new capabilities for voice channel status and listening activity.
__tests__/multibot-independent.test.ts (2)
1-12: LGTM! Clean test setup with appropriate imports.The imports are well-organized, bringing in test utilities from Vitest, stream utilities from Node, and the necessary types/classes for testing multi-bot routing.
295-308: Well-designed test stream helper with proper synchronization.Using
process.nextTickto ensure listeners are set up before emitting data is the correct pattern for test streams.src/providers/audioState.ts (1)
139-163: Well-structured provider response with comprehensive audio state.The response structure is consistent across all return paths, with proper data/values/text separation. The issue detection logic is clear and the text summary is user-friendly.
src/contracts.ts (1)
1-69: Well-designed audio sink contract with excellent documentation.The interface design properly separates concerns between Discord internals and the music-player consumer. The typed event methods and clear lifecycle documentation make this easy to implement correctly. The "WHY" comments explaining design decisions are particularly valuable for future maintainers.
__tests__/messageManager.test.ts (2)
59-80: Good refactoring to dependency injection pattern.The
mockDiscordServiceproperly implements theIDiscordServiceinterface withgetChannelTypeandbuildMemoryFromMessagemethods. This aligns with the constructor signature change and improves testability.
338-359: Test correctly validates attachment processing throughprocessMessage.The test now directly calls
processMessageand verifies the attachment is included in the result, which is a more focused unit test approach.src/messages.ts (4)
50-53: Good design for progressive message tracking with proper TypeScript typing.The Map structure with message reference and timeout handle is appropriate for managing progressive update state.
78-139: Excellent progressive message lifecycle management with thorough documentation.The three helper methods (
trackProgressiveMessage,resetProgressiveTTL,cleanupProgressiveMessage) properly handle the lifecycle with 60-second TTL for leak prevention. The detailed "WHY" comments explaining the design decisions are invaluable for maintainability.
195-196: Good improvement: prefer server-specific displayName (nickname).Using
message.member?.displayNameas the primary source with fallback tomessage.author.displayNamecorrectly prioritizes server nicknames.
337-410: Well-implemented progressive update flow with proper fallback handling.The logic correctly handles:
- Editing existing tracked messages
- TTL refresh on successful edits
- Skipping memory creation for interim updates
- Graceful fallback when edits fail
The detailed inline comments explaining each decision point are excellent.
src/sinks/discordAudioSink.ts (1)
217-233: Good cleanup in destroy() with proper resource handling.The destroy method properly clears the polling interval, stops playback, and removes listeners. Adding the connection listener cleanup (as noted above) would complete this.
MUSIC_ARCHITECTURE.md (3)
63-78: Documentation shows methods not present in the actual interface.The documented
IAudioSinkinterface includesconnect(channelId)anddisconnect()methods, but the implementation insrc/contracts.tsdoesn't include these. This creates confusion about the actual API contract.🔎 Either update docs or add methods to interface
If connect/disconnect are not part of the current implementation:
interface IAudioSink extends EventEmitter { readonly id: string; readonly status: AudioSinkStatus; // 'connected' | 'disconnected' | 'connecting' | 'error' feed(stream: Readable): Promise<void>; stop(): Promise<void>; - - connect(channelId: string): Promise<void>; - disconnect(): Promise<void>; + + getDescription(): string; }
1-39: Excellent architecture documentation with clear diagrams.The ASCII diagram effectively illustrates the broadcast-centric model and the separation between plugin-music-player and plugin-discord. The explanatory comments make the design decisions accessible.
169-204: Good documentation of multi-channel audio system and priority behavior.The channel priority constants and DJ integration example provide clear guidance for implementers. The related documentation links help with discoverability.
src/progressiveMessage.ts (4)
1-65: Exceptional documentation explaining the "why" behind design decisions.The extensive JSDoc comments explaining debouncing, throttling, and platform-specific behavior make this code highly maintainable. The usage example is particularly helpful.
152-161: Fire-and-forget pattern for important updates on non-progressive platforms.The callback is called without awaiting (line 153-158), which means errors are only logged, not propagated. This is intentional for interim updates, but for "important" updates, you might want to track failures differently.
Is fire-and-forget the intended behavior for important updates? If an important update fails silently, users on non-Discord platforms might not get feedback about long operations.
140-193: Well-implemented debounce and throttle logic.The update method correctly handles:
- First update waits for
minDelaybefore sending- Subsequent updates are throttled to prevent rate limiting
- Pending updates are properly managed with timer cleanup
This ensures responsive UX while respecting Discord's rate limits.
237-253: Smart completion logic handles fast operations gracefully.The check on line 247 correctly bypasses progressive mode if the operation completed before any updates were sent and was faster than
minDelay. This prevents unnecessary message edits for instant operations.src/types.ts (1)
534-561: Well-designed VoiceTarget interface for multi-bot routing.The
VoiceTargetinterface provides a clean abstraction for voice connection targets with appropriate methods for playback control. The composite ID formatbot-uuid:guild-id:channel-idenables unique identification across multi-bot scenarios.src/voiceConnectionManager.ts (1)
65-97: Overall clean implementation of multi-bot voice target management.The
getVoiceTargets()method elegantly creates VoiceTarget proxies that delegate to the underlying VoiceManager. The composite key pattern (botId:guildId:channelId) ensures unique identification across the multi-bot topology.src/index.ts (1)
29-51: Clean export organization for the new multi-bot infrastructure.The exports are well-organized with clear comments grouping related types and utilities. This makes it easy for external consumers to understand and use the new audio channel, progressive message, and multi-bot voice capabilities.
src/service.ts (2)
1084-1149: Well-implemented voice state update handler for agent tracking.The
voiceStateUpdatehandler properly tracks agent connect/disconnect/move events and updates audio state. The conditional logic clearly handles all state transition cases (join, leave, move, mute/deafen changes).
3790-3848: Clean implementation ofsetVoiceChannelStatuswith proper validation.The method includes all necessary checks (client ready, channel exists, voice channel type), truncates status to Discord's 500-char limit with a warning, and uses the correct REST endpoint. Good error handling throughout.
src/actions/setVoiceChannelStatus.ts (1)
99-106: Loose channel name matching could match unintended channels.The regex pattern
[^a-z0-9 ]strips all special characters when matching channel names. This causes "voice-1" and "voice_1" to both normalize to "voice1", creating ambiguous matches. When the exact match fails and the fuzzy match is applied,.find()returns the first matching channel, potentially returning an unintended one if multiple channels normalize to the same pattern. Consider prioritizing exact matches or implementing stricter matching logic with disambiguation.Also applies to: 118-125
src/voice.ts (8)
1-54: Imports look appropriate for the new functionality.The new imports (
demuxProbe,getDiscordSettings,DEFAULT_CHANNEL_CONFIGS,DiscordEventTypes) align with the channel-based audio architecture being introduced.
446-453: Timer lifecycle management looks correct.The
voiceStatFlushTimerandconnectionWatchdogintervals are properly cleared in thecleanup()method (lines 553-562). Good practice for resource management.
489-542: Connection health monitoring implementation is solid.The watchdog properly:
- Skips connections with pending reconnects
- Uses appropriate timeout (15s) for stuck connections
- Handles both recovery and cleanup paths
- Cleans up orphaned health records
854-864: Good duplicate monitoring prevention.The
monitoredUsersSet effectively prevents duplicate audio monitors for the same user, which was likely causing resource leaks or duplicate processing in the previous implementation.
2224-2244: Promise resolver pattern is correctly implemented.The external resolver pattern used here is safe because the Promise constructor callback executes synchronously, guaranteeing
finishedResolverandcancelledResolverare assigned before any async code that uses them.
2562-2586: Volume ramp implementation is well-designed.The gradual volume restoration with configurable ramp duration provides a smooth user experience. The use of
setIntervalwith step calculations is appropriate.Minor: Line 2585 casts to
anydue to type mismatch. Consider usingReturnType<typeof setInterval>for consistency with other timer declarations.
1962-1971: Defensive check for malformed keys is reasonable.The check at lines 1965-1967 for keys containing ':' provides safety against accidental misuse of the
connectionsMap, even though the current implementation should only use guildId keys.
1823-1843: Legacy method uses wrong key type for connection lookup.
playAudioStreamlooks up connections usingentityId, but theconnectionsMap is keyed byguildId. This will cause the method to never find a connection.Consider:
- Deprecate and remove: If this method is unused, remove it entirely.
- Fix the lookup: If still needed, change the parameter from
entityIdtoguildId:- async playAudioStream(entityId: UUID, audioStream: Readable) { - const connection = this.connections.get(entityId); + async playAudioStream(guildId: string, audioStream: Readable) { + const connection = this.connections.get(guildId);
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/actions/reactToMessage.ts (1)
224-308: Excellent fix for the forbidden emoji fallback issue! 🎉The previous concern about forbidden emojis being used when all alternatives are filtered has been comprehensively addressed. The new fallback cascade is robust:
- Lines 273-280: Tries neutral emojis filtered for forbidden items
- Lines 282-292: Scans ALL emoji categories to find any non-forbidden option
- Lines 294-299: Only falls back to neutral[0] as absolute last resort with a clear warning
This ensures the character's forbidden list is respected in almost all cases, with clear logging when it cannot be.
Optional: Extract common neutral filtering logic
The neutral emoji filtering logic appears in multiple places (lines 273, 441). Consider extracting to a helper:
function getNeutralAllowedEmojis(prefs: { forbidden: string[] }): string[] { return sentimentEmojis.neutral.filter((e) => !prefs.forbidden.includes(e)); }This would reduce duplication and make the intent clearer.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
__tests__/voiceManager.test.tssrc/actions/reactToMessage.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/voiceManager.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (5)
src/actions/reactToMessage.ts (5)
127-140: LGTM! Well-structured sentiment emoji mapping.The sentiment categories are well-chosen and provide good coverage for common Discord interactions. The neutral fallback options are appropriate.
146-162: Simple but effective sentiment detection.The keyword-based heuristic provides a good balance between simplicity and effectiveness for emoji selection. The neutral default ensures graceful handling of unmatched patterns.
168-218: LGTM! Robust preference handling.The dual-format support for emoji preferences (simple array vs. categorized object) provides good flexibility for character configuration. The defensive type checking ensures graceful handling of missing or malformed settings.
427-453: Previous LLM path forbidden emoji issue is now fixed! ✅The concern about the LLM path falling back to a forbidden emoji when no alternatives exist has been properly addressed. The current implementation:
- Validates the LLM-selected emoji against the forbidden list (line 431)
- Attempts to find allowed alternatives from sentiment-matched emojis (lines 435-438)
- Falls back to neutral alternatives (lines 441-443)
- Crucially, sets
emoji = nullwhen no safe alternative exists (line 450), which prevents the reaction entirely rather than using a forbidden emojiThis is the correct behavior and resolves the previous review concern.
392-409: LGTM! Character-aware emoji selection well integrated.The character path integration properly falls back to character-based emoji selection when the fast path fails. The additional check on line 401 for
characterForbidsEmojisis technically redundant (already checked inselectCharacterEmojiat line 229) but provides clearer logging context, which is acceptable.
- Add filterAttachmentsForMemory() to strip data URLs from stored memories - Add createAttachmentFromMedia() to handle base64→Buffer conversion for Discord - Add scrubBase64ImagesFromMemories() to clean existing memories on startup - Update all memory creation points to filter attachments This prevents the RECENT_MESSAGES provider from loading huge base64 images into LLM context, which was causing 200k token limit errors.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/service.ts (2)
401-519: Align send-path memories with new base64/data-URL filtering
handleSendMessagecorrectly usescreateAttachmentFromMediafor Discord uploads, but when persisting memories you still copycontent.attachmentsverbatim. That means base64/data-URL images sent by the agent will continue to land in themessagestable, bypassingfilterAttachmentsForMemoryand relying solely on the startup scrubber.Consider applying
filterAttachmentsForMemory(content.attachments)here too so new sent-message memories never store raw data URLs in the first place.Suggested change (memory creation block)
- ...(hasAttachments && content.attachments - ? { attachments: content.attachments } - : {}), + ...(hasAttachments && content.attachments + ? { attachments: filterAttachmentsForMemory(content.attachments) } + : {}),
4693-4757: UseActivityType.Listeningenum instead of magic numberIn
setListeningActivity, the code uses the hard-codedtype: 2:await this.client.user.setActivity(activity, { type: 2, // ActivityType.Listening url, });Import and use
ActivityType.Listeningfrom discord.js (v14.18.0) instead. This is the recommended approach and avoids silent breakage if the underlying enum changes.-import { … } from 'discord.js'; +import { …, ActivityType } from 'discord.js'; - await this.client.user.setActivity(activity, { - type: 2, // ActivityType.Listening - url: url, - }); + await this.client.user.setActivity(activity, { + type: ActivityType.Listening, + url, + });src/utils.ts (1)
190-241: Data‑URL parsing only supports simple;base64,form
parseDataUrlcurrently matches onlydata:<mime>;base64,<data>:const match = dataUrl.match(/^data:([^;]+);base64,(.+)$/);This will return
nullfor valid data URLs that include additional parameters (e.g.data:image/png;charset=utf-8;base64,...).If you expect those forms from upstream tools, consider relaxing the regex to allow extra
;...segments before;base64,or delegating to a small data-URL parsing helper.src/messages.ts (1)
390-557: Avoid duplicating attachments on each chunk in progressive final send pathIn the progressive branch where there is no existing tracked message (or edit failed), you send new messages (possibly chunked) and then create memories:
for (const m of messages) { const memory: Memory = { … content: { ...content, attachments: filterAttachmentsForMemory(content.attachments), … }, … }; }Unlike the non-progressive path, this does not check whether each
mactually carries attachments; all chunks get the sameattachmentsarray, so a multi-chunk progressive response will duplicate attachments across all associated memories.To keep behavior consistent with the normal path (which uses
hasAttachments), consider only attaching filtered attachments to the message(s) wherem.attachments.size > 0.Suggested change
- const memories: Memory[] = []; - for (const m of messages) { - const memory: Memory = { + const memories: Memory[] = []; + for (const m of messages) { + const hasAttachments = m.attachments?.size > 0; + const memory: Memory = { … content: { ...content, - // Filter out base64 attachments to prevent context bloat - attachments: filterAttachmentsForMemory(content.attachments), + // Only include attachments for the chunk that actually has them + attachments: + hasAttachments && content.attachments + ? filterAttachmentsForMemory(content.attachments) + : undefined, … }, … };src/index.ts (1)
54-132: Reuse sharedisDataUrl/filtering helpers to avoid divergence
src/index.tsdefines its ownisDataUrland manual filtering logic inscrubBase64ImagesFromMemories, even thoughsrc/utils.tsnow exportsisDataUrlandfilterAttachmentsForMemory.To keep behavior consistent and avoid future drift between the startup scrubber and per-message filtering, consider:
- Importing
isDataUrl(and possiblyfilterAttachmentsForMemory) from./utilsinstead of re-implementing them here.- Or, at minimum, delegating the attachment filtering in the scrubber to
filterAttachmentsForMemoryso both paths use the same placeholder and criteria.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
src/index.tssrc/messages.tssrc/service.tssrc/utils.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/index.ts (2)
src/utils.ts (1)
isDataUrl(186-188)src/permissions.ts (1)
getPermissionValues(238-247)
src/messages.ts (1)
src/utils.ts (4)
editMessageContent(899-924)filterAttachmentsForMemory(342-366)createAttachmentFromMedia(313-332)sendMessageInChunks(442-617)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (7)
src/utils.ts (2)
306-365: Attachment creation + memory filtering behavior looks goodThe combination of:
createAttachmentFromMediadecoding data URLs intoAttachmentBuilderbuffers, andfilterAttachmentsForMemorydropping those data URLs (with a placeholder summary),gives a nice separation between what’s sent to Discord and what’s persisted into memories.
No functional issues spotted; just noting that this aligns well with the new startup scrubber.
882-923: 2000 character limit is correct for Discord message editsThe 2000 character limit is confirmed as the correct hard limit for regular Discord accounts. The implementation sensibly truncates rather than splits, since message edits can only update one message.
Note: The codebase uses
MAX_MESSAGE_LENGTH = 1900elsewhere for new messages (a conservative buffer), whileeditMessageContenthardcodes2000to match Discord's actual API limit. Both approaches are valid for their use cases. If targeting Nitro accounts with their 4000 character limit, the function would need enhancement.src/messages.ts (2)
50-143: Progressive message tracking and TTL behaviorThe
progressiveMessagesmap withtrackProgressiveMessage,resetProgressiveTTL, andcleanupProgressiveMessagegives a clear, bounded way to support correlation‑ID based progressive updates without leaking entries indefinitely. The 60s TTL and reset-on-update logic make sense for long-running actions.No functional issue here; just calling out that this is a solid structure, and cleanup is correctly invoked on final updates and edit failures.
575-645: Non‑progressive attachment handling and memory filtering look consistentIn the normal (non-progressive) path you:
- Convert
content.attachmentsviacreateAttachmentFromMediabefore sending (DM or channel), and- Only attach
filterAttachmentsForMemory(content.attachments)to memories wherem.attachments.size > 0.That both preserves outgoing behavior and keeps memory rows free of base64 data URLs. This looks correct and consistent with the new utils and scrubber behavior.
src/index.ts (1)
161-252: Token validation and startup scrubber behaviorThe updated
initnow:
- Treats only
DISCORD_API_TOKENandDISCORD_BOT_TOKENSas valid bot-token sources, and- Logs a clear banner when neither is set, explicitly clarifying that
DISCORD_APPLICATION_IDis not a token.- Runs
scrubBase64ImagesFromMemoriesonce on startup to clean the last ~1000 memories.This addresses earlier confusion around
DISCORD_APPLICATION_IDand keeps base64 cleanup non-fatal. No issues spotted; just a note that the additional startup I/O is bounded and should be fine.src/service.ts (2)
2513-2650: Thetimeoption is supported in discord.js v14 and is a documented parameter forGuildMemberManager.fetch()—it sets the max wait time in milliseconds for gateway responses (default ~60000). Your concern about it being unsupported can be disregarded.That said, in the large-guild path (lines ~2540–2548), you do have redundant timeout layers: the
time: 5000option infetch()combined with thePromise.racewrapper using a 6000ms timeout. The discord.js timeout will trigger first. Consider removing thetimeoption there and relying solely on thePromise.racewrapper for clarity. In the small-guild path (line ~2625), thetime: 10000option is useful and has no competing timeout.Likely an incorrect or invalid review comment.
4634-4691: The REST endpoint and payload are correct; the GuildVoice restriction is appropriateThe
/channels/{id}/voice-statusendpoint with payload{ status: string | null }is correct. The discord.js REST client defaults to API version v10, so the call is properly formatted. However, note that this endpoint is undocumented in the official Discord API documentation and remains subject to change—consider adding a comment noting this limitation.Regarding Stage channels: the voice-status endpoint is not supported for
GuildStageVoicechannels, onlyGuildVoice, so the current type restriction is correct and appropriate.
Prevents runtime throws for text-only clients or failed voiceManager construction by checking voiceManager exists before calling getVoiceConnection() or joinChannel(). Logs debug message when skipping clients without voiceManager.
…stant - Skip logging voice connection state changes when status hasn't actually changed - Skip logging audio player state changes when status hasn't actually changed - Add DISCORD_SERVICE_NAME constant for service identification - Minor import cleanup in types.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/service.ts (1)
2557-2627: Optional: Simplify redundant timeout logic.The online member fetching has two timeout mechanisms:
guild.members.fetch({ time: 5000 })andPromise.racewith a 6-second timeout. The inner timeout (5s) will always fire first, making the outer timeout (6s) redundant.Consider simplifying:
- const fetchPromise = guild.members.fetch({ limit: 100, time: 5000 }); - const timeoutPromise = new Promise((_, reject) => - setTimeout(() => reject(new Error('Member fetch timeout')), 6000) - ); - - const onlineMembers = (await Promise.race([fetchPromise, timeoutPromise])) as any; + const onlineMembers = await guild.members.fetch({ limit: 100, time: 5000 });The fetch's built-in timeout is sufficient and clearer.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
src/service.tssrc/types.tssrc/voice.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/types.ts (1)
src/index.ts (2)
VoiceTarget(46-46)DiscordBotConfig(46-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (17)
src/types.ts (5)
11-11: LGTM: Import additions support new voice/stream functionality.The new imports enable:
Serviceextension for IDiscordService (line 502)Readabletype for VoiceTarget.play() signature (line 580)VOICE_TRANSCRIPTIONevent type for voice transcription events (line 64)Also applies to: 24-24, 64-64
502-516: LGTM: Service extension enables proper integration with ElizaOS core.Extending
Serviceand addinggetChannelTypeandbuildMemoryFromMessagemethods provides a cleaner API surface for Discord service implementations.
527-554: LGTM: Comprehensive voice settings with sensible defaults.The
DiscordSettingsinterface provides fine-grained control over voice behavior:
- Voice ducking configuration for dynamic volume adjustment
- Listen-only mode for transcription without TTS
- Channel whitelisting and message filtering
All settings are optional with defaults documented, enabling zero-config usage.
560-601: LGTM: Multi-bot voice architecture types are well-designed.
VoiceTargetprovides a clean abstraction for audio playback:
- Composite ID format enables per-guild targeting across multiple bots
play(stream: Readable)signature aligns with Node.js streamsgetStatus()return type uses string literals for type safety
DiscordBotConfigsupports multi-bot orchestration with optional auto-join.
670-706: LGTM: Discord component types enable rich interactions.The component types (
DiscordSelectOption,DiscordComponentOptions,DiscordActionRow) mirror Discord's API structure for buttons, select menus, and other interactive components.Numeric type codes match Discord's API conventions.
src/voice.ts (8)
260-346: LGTM: Audio channel architecture is well-documented.The new interfaces (
AudioChannelConfig,PlaybackHandle,ChannelPlayerState) support priority-based audio with ducking and interruption control.The extensive class documentation clearly explains:
- Audio playback pipeline
- Critical stream handling requirements (no listeners before demuxProbe)
- Channel-based architecture
430-486: LGTM: Constructor properly initializes audio system.Key initialization:
- Default audio channels registered immediately (ensures availability)
- Ducking config loaded from settings with proper boolean parsing
- Stats flushing (30s) and connection watchdog (5s) timers started
Timers are properly cleaned up in
cleanup()method (lines 580-616).
580-616: LGTM: Cleanup method comprehensively handles all resources.All previously flagged gaps from past review have been addressed:
- Reconnect timeouts cleared (lines 596-600)
- Ducking timers cleared (lines 602-607)
- Active bridges cleaned up (lines 609-613)
The method now properly cleans up stats flush timer, connection watchdog, reconnect timeouts, ducking state, and audio bridges.
✅ Past review issue resolved.
1009-1069: LGTM: Race condition properly fixed with immediate tracking.The race condition flagged in past review has been resolved:
- User added to
monitoredUsersimmediately after check (line 1017)- Early exit paths clean up tracking (lines 1022-1024, 1039-1040, 1064-1066)
- Pipeline callback ensures cleanup on completion (lines 1145-1159)
This prevents duplicate monitors when
monitorMemberis called concurrently for the same user.✅ Past review issue resolved.
1816-1822: LGTM: Listen-only mode correctly uses parsed settings.The truthy string bug flagged in past review has been resolved:
- Uses
getDiscordSettings()for proper boolean parsing (line 1819)- Checks
settings.voiceListenOnlyinstead of raw setting string- Transcription event always emitted (lines 1796-1814)
- Response generation skipped only when truly in listen-only mode
✅ Past review issue resolved.
2428-2462: LGTM: Web ReadableStream conversion uses streaming approach.The memory buffering issue flagged in past review has been resolved:
- Primary path uses
Readable.fromWeb()for streaming conversion (lines 2438-2440)- No buffering into memory for modern runtimes (Node.js 18+, Bun)
- Fallback to buffering only for older runtimes with warning (lines 2442-2460)
- Proper cleanup with
reader.releaseLock()in finally blockThis avoids OOM for large audio files while maintaining compatibility.
✅ Past review issue resolved.
2364-2653: LGTM: Audio playback pipeline properly implements format detection.The
playAudio()method correctly:
- Uses
demuxProbefor format detection (lines 2483-2508)- Handles Web ReadableStream conversion (lines 2428-2462)
- Validates stream before and after probe
- Creates
AudioResourcewith detected format (lines 2521-2524)- Handles priority-based interruption and ducking (line 2404)
- Monitors resource stream for debugging (lines 2532-2549)
- Properly cleans up on completion/error (lines 2614-2638)
The extensive documentation clearly explains stream handling requirements.
3105-3286: LGTM: Audio bridging provides clean telephone booth POC.The bridging implementation correctly:
- Captures audio from source guild's receiver (line 3162-3165)
- Creates
AudioResourcefrom Opus stream (lines 3169-3171)- Plays to target guild via dedicated
AudioPlayer(line 3174)- Tracks per-user streams to prevent duplicates (lines 3138-3150)
- Provides proper cleanup for user streams and bridges (lines 3198-3216)
- Supports bidirectional bridging (lines 3236-3265)
- Cleanup functions are called in
cleanup()method (lines 609-613)src/service.ts (4)
199-201: Token validation correctly fixed.The token fallback chain now only includes valid bot token settings (
DISCORD_API_TOKENandDISCORD_BOT_TOKENS), withDISCORD_APPLICATION_IDproperly removed as flagged in previous reviews.
2793-2797: LGTM! Null guards added as requested in previous review.The auto-join logic now properly guards against missing
voiceManagerin both the pre-population loop and the join loop, preventing runtime errors for text-only clients or failed voiceManager construction.Also applies to: 2838-2842
3385-3395: LGTM! Reaction removal properly delegates to generic handler.The implementation now correctly uses
handleReaction(reaction, user, 'remove')instead of duplicating logic, addressing the previous review concern about code duplication and deprecated property usage.
4746-4769: LGTM! Activity cleared using proper Discord API.The implementation correctly uses
setPresence({ activities: [] })instead of the problematicsetActivity(null as any)that was flagged in previous reviews. This prevents invalid Gateway packets.
…nager for multi-bot - Track auto-join setTimeout for proper cleanup in stop() - Use voiceConnectionManager to find correct voice manager per guild - Fixes audio sink creation in multi-bot deployments where primary bot may not be in target guild
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/service.ts (1)
2772-2970: Optional: Remove auto-join timeout from tracking array after execution for consistency.The auto-join timeout (line 2772) is added to
this.timeouts(line 2970) but never removed after execution. This differs from the button interaction timeouts (lines 2291-2294, 2340-2343) which clean up after themselves. While this doesn't cause functional issues (the timeout ID is harmless after firing and gets cleared on service stop), removing it after execution would be more consistent with the rest of the codebase and prevent accumulating stale timeout IDs.🔎 Proposed fix
const autoJoinTimeout = setTimeout(async () => { try { // ... auto-join logic ... } catch (error) { this.runtime.logger.error(`Error in auto-join process: ${error instanceof Error ? error.message : String(error)}`); } + // Remove from tracking array after execution + const index = this.timeouts.indexOf(autoJoinTimeout); + if (index > -1) { + this.timeouts.splice(index, 1); + } }, 5000); // Track timeout for cleanup on service stop this.timeouts.push(autoJoinTimeout);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
src/service.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (6)
src/service.ts (6)
167-282: LGTM! Multi-bot initialization is well-structured.The multi-bot infrastructure initialization is comprehensive and robust:
- Proper promise coordination via
clientReadyPromiseensures all consumers wait for initialization- Error handling provides helpful context for token validation failures with actionable guidance
- Cleanup of partial state on errors prevents resource leaks
- Token validation correctly removes
DISCORD_APPLICATION_IDfrom fallback chain (addressing past review comment)- Backward compatibility maintained by setting
this.clientandthis.voiceManagerto primary clientThe error handling flow ensures
readyResolver()is called in all paths (success, error, no token), preventing promise deadlocks.
297-327: LGTM! Audio sink selection correctly handles multi-bot deployments.The enhanced
getAudioSinklogic now properly addresses the multi-bot scenario flagged in past reviews:
- First checks
voiceConnectionManagerfor guilds where a bot is already connected- Uses the voice manager from the active bot in that guild
- Falls back to primary voice manager if no connections exist
- Returns
nullwith clear logging if no voice manager is availableThis ensures audio sinks work correctly when the primary bot is not in the target guild.
2752-2971: LGTM! Auto-join implementation comprehensively handles multi-bot scenarios.The rewritten auto-join logic addresses all concerns from past reviews and adds robust multi-bot support:
✅ Past issues resolved:
- Timeout is now tracked in
this.timeoutsarray for proper cleanup- Pre-existing connections are checked before attempting joins
- Guards against missing
voiceManagerprevent runtime errors✅ Multi-bot logic:
- Parses comma-separated channel IDs for flexibility
- Pre-populates
activeConnectionsto prevent duplicate joins- Iterates channels first (outer loop) for balanced distribution
- Comprehensive result tracking and summary logging
✅ Error handling:
- Try-catch around each join attempt
- Non-fatal errors logged with context
- Summary shows success/failure breakdown per channel
The 5-second delay provides adequate time for guild cache population across multiple bots.
1344-1405: LGTM! Voice state tracking properly handles agent state changes.The new
voiceStateUpdatehandler provides comprehensive tracking:
- Delegates non-agent voice state updates to
voiceManager- Safely checks both
oldState.member?.idandnewState.member?.idto handle partial states- Covers all transition scenarios: connect, disconnect, move, mute/deaf changes
- Updates audio state and delegates appropriate handler calls to
voiceManager- Clear logging for each transition type
4668-4783: LGTM! Voice status methods correctly implemented.All three new public methods are well-implemented:
✅
setVoiceChannelStatus(lines 4668-4717):
- Validates channel type before setting status
- Truncates to Discord's 500-character limit with warning
- Uses REST API directly (correct approach - discord.js has no helper for this endpoint)
✅
setListeningActivity(lines 4727-4753):
- Uses correct activity type (2 = Listening)
- Handles optional URL parameter
✅
clearActivity(lines 4760-4783):
- Now uses
setPresence({ activities: [] })instead ofnull as any(addresses past review comment)- Correctly clears presence without invalid Gateway packets
All methods have proper error handling and logging.
2568-2641: LGTM! Member fetching timeout prevents hangs on large guilds.The new conditional fetch with timeout protection (lines 2568-2641) is a smart optimization:
- Only triggers when cache has fewer than 100 members for large guilds
- Uses
Promise.racewith 6-second timeout as safety net beyond discord.js's 5-second request timeout- Error handling treats timeout as non-fatal with debug-level logging
- Falls back to cached members on failure
The dual-timeout approach (5s in fetch + 6s in Promise.race) ensures the operation won't hang indefinitely.
- Add agentIdentifier getter to DiscordService, MessageManager, VoiceManager, AttachmentManager - Returns character name with agentId fallback for human-readable logs - Data objects (Entity, Memory, World, Room) still use UUID agentId as required by types
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/messages.ts (1)
89-139: Consider extracting the TTL constant.The 60-second TTL value (60000ms) is duplicated in both
trackProgressiveMessage(line 117) andresetProgressiveTTL(line 136). Extracting this to a named constant improves maintainability and makes the timeout duration self-documenting.🔎 Suggested refactor
+ private static readonly PROGRESSIVE_MESSAGE_TTL_MS = 60000; + private trackProgressiveMessage(key: string, message: DiscordMessage): void { // Clear any existing timeout for this key const existing = this.progressiveMessages.get(key); if (existing?.timeout) { clearTimeout(existing.timeout); } - // Set 60-second TTL + // Set TTL for auto-cleanup const timeout = setTimeout(() => { this.progressiveMessages.delete(key); this.runtime.logger.debug(`Progressive message ${key} TTL expired`); - }, 60000); + }, MessageManager.PROGRESSIVE_MESSAGE_TTL_MS);Apply the same change in
resetProgressiveTTL.src/service.ts (1)
256-262: Log client destruction errors for better observability.The empty catch block on line 258 silently swallows any errors from
client.destroy(). While these errors during cleanup might be expected, logging them would improve debugging and observability.🔎 Suggested improvement
// Cleanup any partially initialized clients if (this.client) { - this.client.destroy().catch(() => { }); + this.client.destroy().catch((err) => { + this.runtime.logger.debug(`Error destroying client during cleanup: ${err instanceof Error ? err.message : String(err)}`); + }); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
src/attachments.tssrc/messages.tssrc/service.tssrc/voice.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/attachments.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/messages.ts (1)
src/utils.ts (4)
editMessageContent(899-924)filterAttachmentsForMemory(342-366)createAttachmentFromMedia(313-332)sendMessageInChunks(442-617)
src/service.ts (2)
src/types.ts (1)
VoiceTarget(560-587)src/utils.ts (1)
createAttachmentFromMedia(313-332)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (6)
src/messages.ts (6)
50-53: LGTM!The
progressiveMessagesmap with{ message, timeout }structure is a clean design for tracking progressive updates with TTL cleanup.
221-222: LGTM!Good improvement to prefer server-specific display name (nickname) over global display name for better context in multi-server scenarios.
500-505: DM attachment handling is now correct.The DM send now properly includes both text content and file attachments, addressing the previous review concern about dropped attachments.
519-521: LGTM!Proper guard ensures we only track messages that were successfully sent and have valid IDs.
649-653: LGTM!Good use of
filterAttachmentsForMemoryto prevent base64 data URLs from bloating the memory/context. The conditional attachment inclusion based onhasAttachmentsensures data consistency.
724-734: LGTM!Good defensive programming with the 30-second failsafe timeout for the typing indicator. This prevents the typing indicator from showing indefinitely if something goes wrong in the message processing pipeline.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
… timeouts - Memory.agentId must be UUID, not character name (messages.ts) - Remove invalid 'time' option from guild.members.fetch() calls - Wrap member fetch in Promise.race for proper timeout handling
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/messages.ts (1)
491-491: Consider stronger typing for messages array.The
messagesvariable is typed asany[](lines 491, 568), and the tracking validation at line 519-521 only checksmessages[0].idwithout confirming it's a proper Discord message. While the current code works (both paths return Discord messages), explicit typing would prevent future regressions and improve IDE support.💡 Suggested type improvement
- let messages: any[] = []; + let messages: DiscordMessage[] = [];Apply the same change at line 568 in the non-progressive flow.
Also applies to: 519-521, 568-568
src/service.ts (1)
153-157: Inconsistent optional chaining inagentIdentifiergetter.The getter uses optional chaining for
this.runtime?.character?.namebut then accessesthis.runtime.agentIdwithout optional chaining. Ifthis.runtimeis undefined, the second part of the expression will throw a TypeError.🔎 Suggested fix for consistent optional chaining
private get agentIdentifier(): string { - return this.runtime?.character?.name || this.runtime.agentId; + return this.runtime?.character?.name || this.runtime?.agentId || 'unknown'; }Alternatively, if
runtimeis guaranteed to be defined after construction (which it should be), remove the optional chaining entirely:private get agentIdentifier(): string { - return this.runtime?.character?.name || this.runtime.agentId; + return this.runtime.character?.name || this.runtime.agentId; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
src/messages.tssrc/service.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/messages.ts (1)
src/utils.ts (4)
editMessageContent(899-924)filterAttachmentsForMemory(342-366)createAttachmentFromMedia(313-332)sendMessageInChunks(442-617)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (10)
src/messages.ts (5)
55-60: Previous critical issue resolved: Memory.agentId now correctly uses UUID.The past review flagged that
agentIdentifier(returning a character name) was incorrectly assigned toMemory.agentId, which requires a UUID. The current code correctly usesthis.runtime.agentIdat all three memory creation sites (lines 447-448, 545-546, 639-640), and reservesagentIdentifierexclusively for logging as documented in the getter comment.Also applies to: 447-448, 545-546, 639-640
50-53: Well-designed progressive message tracking with proper resource management.The Map-based tracking with 60-second TTL prevents memory leaks from stalled actions, and the timeout reset logic ensures long-running actions with frequent updates don't expire prematurely. The cleanup is thorough (clearing timeout before deleting entry), and the detailed inline comments clearly explain the design rationale.
Also applies to: 89-150
492-513: Previous issue resolved: DM path now includes file attachments.The past review flagged that the DM send (line 435 in old code) only sent text, dropping any prepared attachments. The current implementation correctly includes the
filesparameter (lines 500-503) and captures the returned message for tracking (line 500-505), resolving both the attachment loss and message tracking issues.
481-489: Attachment handling is consistent and correct across all message flows.All paths (progressive DM, progressive channel, non-progressive DM, non-progressive channel) correctly:
- Convert
MediatoAttachmentBuilderusingcreateAttachmentFromMedia- Filter out null results from failed conversions
- Include attachments in both message sends and memory creation
- Apply
filterAttachmentsForMemoryto prevent base64 data URLs from bloating contextAlso applies to: 584-592, 610-619
445-462: Memory creation is consistent and correctly structured across all paths.All memory creation sites (progressive edit, progressive fallback, non-progressive) correctly:
- Use
this.runtime.agentIdfor bothentityIdandagentId(UUID requirement)- Apply
filterAttachmentsForMemoryto prevent base64 data URLs from bloating conversation context- Populate all required Memory fields with appropriate values
- Associate memories with the correct room and message references
Also applies to: 541-564, 631-659
src/service.ts (5)
206-217: Excellent fix: Token validation now correctly excludes DISCORD_APPLICATION_ID.The token fallback chain has been properly corrected to only use valid bot token environment variables (
DISCORD_API_TOKENandDISCORD_BOT_TOKENS). The helpful warning messages clearly distinguish between a missing token and the application ID, which should prevent user confusion.This addresses the previous critical issue where
DISCORD_APPLICATION_IDwas incorrectly included in the token fallback chain.
2575-2732: Well done: Member fetch calls now properly handle timeouts.Both
guild.members.fetch()calls have been corrected:
- Line 2588: Removed the invalid
timeoption while keeping the existingPromise.racetimeout protection.- Lines 2668-2672: Added proper
Promise.racetimeout wrapper (10 seconds) to prevent indefinite hangs on slow networks.The implementation correctly uses Discord.js's valid options (
limit) and provides robust timeout handling for reliability in production environments.
2763-2982: Outstanding implementation: Auto-join feature comprehensively addresses all previous concerns.This implementation has successfully resolved all flagged issues from the previous review:
- Timeout tracking (line 2981): The
autoJoinTimeoutis properly tracked inthis.timeoutsarray for cleanup on service stop.- Pre-existing connections (lines 2808-2831): The
activeConnectionsSet is pre-populated by checking all clients' existing voice connections, preventing duplicate join attempts.- VoiceManager guards (lines 2817-2820, 2862-2865): Null checks added in both the pre-population loop and the join loop to safely skip text-only clients.
Additional highlights:
- Excellent documentation: The inline comments thoroughly explain the "why" behind design decisions (comma-separated IDs, Discord's one-connection-per-guild limitation, algorithm choice, etc.).
- Multi-bot coordination: Properly handles complex scenarios like multiple bots across multiple guilds.
- Comprehensive logging: Summary results clearly show which bots joined which channels, making debugging straightforward.
- Robust error handling: Try-catch blocks and defensive checks throughout.
This is production-ready code with exceptional documentation quality.
3413-3421: Good refactoring: Eliminated code duplication inhandleReactionRemove.The method now properly delegates to the generic
handleReactionmethod instead of duplicating ~80 lines of logic. This:
- Reduces maintenance burden (single source of truth for reaction handling)
- Eliminates the potential for divergence between add/remove handlers
- Correctly uses
messageServerIdinstead of the deprecatedserverIdproperty (via the generic handler)
4771-4794: Perfect fix: Activity clearing now uses proper Discord.js API.The
clearActivitymethod has been corrected to usesetPresence({ activities: [] })instead of the problematicsetActivity(null as any). This prevents invalid Gateway packets that could invalidate the bot's session.The implementation correctly handles edge cases:
- Checks if client is ready
- Checks if client.user is available
- Proper error handling with logging
…dates - Add flushInProgress flag to prevent concurrent flush operations - Schedule retry timer when flush is already in progress - Clear flag on callback completion using .finally() - Prevents duplicate Discord messages when API response is slow
- Memory.agentId must be UUID, not character name - Fixes build error: Type 'string' is not assignable to UUID type
…race condition fix - Embed reply chain (up to 5 levels) directly in message text for agent context - Add metadata: inReplyToAuthor, discordInReplyToMessageId, discordInReplyToUserId - Add agentRoleProvider for agent's Discord role/permissions info - Fix progressive message race condition with waitForFlushComplete() - Add backwards compatibility for messageServerId (Discord snowflake) fallback - Actions now use channelId with snowflake fallback for guild lookup
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/actions/leaveChannel.ts (1)
412-446: Bug:currentServerIdis undefined.The
findChannelcalls at lines 418, 426, 436, and 444 passcurrentServerIdas an argument, but this variable is never defined in the handler scope. This will always beundefined, breaking the guild lookup infindChannel.Should this be using the resolved guild or passing
messageServerIdinstead?🔎 Proposed fix - use messageServerId
// Find the channel (try voice first if it's a voice request) let targetChannel = isVoiceRequest ? await findChannel( discordService, channelInfo.channelIdentifier, currentChannelId, - currentServerId, + messageServerId, true, ) : await findChannel( discordService, channelInfo.channelIdentifier, currentChannelId, - currentServerId, + messageServerId, false, ); // If not found, try the opposite type if (!targetChannel) { targetChannel = isVoiceRequest ? await findChannel( discordService, channelInfo.channelIdentifier, currentChannelId, - currentServerId, + messageServerId, false, ) : await findChannel( discordService, channelInfo.channelIdentifier, currentChannelId, - currentServerId, + messageServerId, true, ); }
♻️ Duplicate comments (8)
src/actions/readChannel.ts (1)
17-24: DuplicateisDiscordSnowflakehelper.Same function duplicated here. See comment on
searchMessages.tsfor consolidation suggestion.src/actions/getUserInfo.ts (1)
17-24: DuplicateisDiscordSnowflakehelper.Same function duplicated here. See comment on
searchMessages.tsfor consolidation suggestion.src/actions/serverInfo.ts (1)
14-21: DuplicateisDiscordSnowflakehelper.Same function duplicated here. See comment on
searchMessages.tsfor consolidation suggestion.src/actions/leaveChannel.ts (1)
21-28: DuplicateisDiscordSnowflakehelper.Same function duplicated here. See comment on
searchMessages.tsfor consolidation suggestion.src/actions/sendDM.ts (2)
17-24: DuplicateisDiscordSnowflakehelper.Same function duplicated here. See comment on
searchMessages.tsfor consolidation suggestion.
26-62: DuplicategetGuildFromRoomhelper.Same helper duplicated here. See comment on
leaveChannel.tsfor consolidation suggestion.src/actions/joinChannel.ts (2)
20-27: DuplicateisDiscordSnowflakehelper.Same function duplicated here. See comment on
searchMessages.tsfor consolidation suggestion.
94-130: DuplicategetGuildFromRoomhelper.Same helper duplicated here. See comment on
leaveChannel.tsfor consolidation suggestion.
🧹 Nitpick comments (7)
src/actions/searchMessages.ts (2)
17-24: Consider extractingisDiscordSnowflaketo a shared utility.This helper is duplicated across
searchMessages.ts,readChannel.ts,getUserInfo.ts,serverInfo.ts,leaveChannel.ts,sendDM.ts, andjoinChannel.ts. Extract it to a shared location (e.g.,src/utils.ts) to reduce duplication and ensure consistent behavior.
243-277: Guild resolution logic is consistent with other actions.The multi-path approach (channelId → messageServerId fallback) aligns with the pattern used in other action files. However, consider extracting this into a shared
getGuildFromRoomhelper like inleaveChannel.ts,sendDM.ts, andjoinChannel.tsto reduce duplication.The empty catch blocks at lines 254-256 and 266-268 silently swallow errors. While this is acceptable for fallback logic, consider logging at debug level for troubleshooting.
🔎 Optional: Add debug logging for failed fetches
try { currentChannel = await discordService.client.channels.fetch(channelId) as GuildChannel | undefined; } catch { - // Channel fetch failed + // Channel fetch failed - continue to fallback }src/actions/readChannel.ts (1)
186-220: Guild resolution logic is correct but inconsistent with other files.This file uses inline guild resolution while
leaveChannel.ts,sendDM.ts, andjoinChannel.tsuse agetGuildFromRoomhelper. Consider using the same helper pattern for consistency.src/actions/leaveChannel.ts (1)
30-66: DuplicategetGuildFromRoomhelper.This helper is also defined in
sendDM.tsandjoinChannel.ts. Extract to a shared utility to reduce duplication.src/providers/agentRole.ts (1)
77-122: Consider adding debug logging in catch blocks.The empty catch blocks at lines 82 and 107 silently swallow errors, which can make debugging difficult. While the graceful fallbacks are appropriate, adding debug-level logging would help diagnose issues in production.
🔎 Suggested improvement
if (!channel) { try { channel = await discordService.client.channels.fetch(channelId) as GuildChannel | undefined; - } catch { + } catch (error) { + runtime.logger?.debug?.({ src: 'agentRoleProvider', channelId, error: error instanceof Error ? error.message : String(error) }, 'Failed to fetch channel'); return { data: {}, values: {}, text: '', }; } }try { botMember = guild.members.cache.get(discordService.client.user?.id ?? ''); if (!botMember && discordService.client.user?.id) { botMember = await guild.members.fetch(discordService.client.user.id); } - } catch { - // Bot might not be in guild cache yet + } catch (error) { + // Bot might not be in guild cache yet - log for debugging + runtime.logger?.debug?.({ src: 'agentRoleProvider', guildId: guild.id, error: error instanceof Error ? error.message : String(error) }, 'Failed to fetch bot member'); }src/index.ts (2)
250-253: Awaiting memory scrub blocks plugin initialization.The
await scrubBase64ImagesFromMemories(runtime)call blocks the init function from completing until all memories are processed. Combined with the earlier performance concern, consider making this non-blocking:🔎 Non-blocking alternative
// Clean up base64 images from existing memories to prevent context bloat // This runs once on startup to fix any memories that were saved before this fix - await scrubBase64ImagesFromMemories(runtime); + // Run in background to not block plugin initialization + scrubBase64ImagesFromMemories(runtime).catch((error) => { + runtime.logger.warn( + { src: 'plugin:discord', error: error instanceof Error ? error.message : String(error) }, + 'Background memory scrubbing failed (non-fatal)' + ); + });
68-133: Startup memory scrubbing blocks plugin initialization and may impact boot time on large deployments.The function fetches up to 1000 memories and updates them sequentially with individual
awaitcalls at line 252, blocking initialization. While error handling prevents plugin failure, the synchronous wait still impacts startup time. The code comment indicates "once on startup," but there's no guard to prevent re-running on subsequent restarts, so it executes on every initialization.Consider deferring this to a background task after initialization, or adding a timestamp check to skip unnecessary re-runs if already completed recently.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (12)
src/actions/getUserInfo.tssrc/actions/joinChannel.tssrc/actions/leaveChannel.tssrc/actions/readChannel.tssrc/actions/searchMessages.tssrc/actions/sendDM.tssrc/actions/serverInfo.tssrc/index.tssrc/progressiveMessage.tssrc/providers/agentRole.tssrc/service.tssrc/voice.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/progressiveMessage.ts (1)
src/index.ts (1)
ProgressiveMessage(44-44)
src/index.ts (2)
src/utils.ts (1)
isDataUrl(186-188)src/permissions.ts (1)
getPermissionValues(238-247)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (16)
src/actions/searchMessages.ts (1)
96-107: Good defensive normalization for LLM output.The
normalizeNullishfunction handles common LLM hallucinations like"null","undefined", and"none"strings. This is a solid approach to make the JSON parsing more robust.src/progressiveMessage.ts (2)
324-338: Polling implementation is acceptable but consider event-driven alternative.The 50ms polling with 2s timeout is a pragmatic solution. The warning at line 336 provides visibility when timeouts occur.
For a future improvement, you could refactor to use a Promise that resolves when the flush completes, avoiding the polling loop entirely:
🔎 Optional future improvement
private flushPromise: Promise<void> | null = null; private flushUpdate(): void { // ... existing checks ... this.flushPromise = this.callback(content) .catch(error => { logger.warn(`Progressive update flush failed: ${error}`); }) .finally(() => { this.flushInProgress = false; this.flushPromise = null; }); // ... } private async waitForFlushComplete(): Promise<void> { if (this.flushPromise) { await Promise.race([ this.flushPromise, new Promise(resolve => setTimeout(resolve, 2000)) ]); } }
260-285: Complete method handles the race condition correctly.The
waitForFlushComplete()call at line 274 ensures the final message waits for any in-progress flush. The fast-path optimization at lines 279-281 for operations underminDelayis a good UX touch.src/actions/getUserInfo.ts (1)
168-202: Guild resolution implementation is correct.The multi-path resolution (channelId → messageServerId) is implemented consistently. The backward compatibility fallback ensures existing room data structures continue to work.
src/actions/serverInfo.ts (1)
133-167: LGTM - Guild resolution is correct.The implementation is consistent with other action files.
src/actions/sendDM.ts (1)
259-268: LGTM - Correct usage of channel/server ID resolution.The handler correctly extracts
currentChannelIdandmessageServerIdfrom room data and passes them tofindUser.src/actions/joinChannel.ts (1)
289-349: LGTM - Guild resolution and channel finding are correctly wired.The handler properly extracts IDs from room data and uses the multi-path resolution for both explicit channel finding and the fallback to the user's current voice channel.
src/providers/agentRole.ts (4)
1-6: LGTM!Imports are well-organized, properly use the
typekeyword for type-only imports, and follow TypeScript best practices.
25-75: LGTM!The guard clauses provide good defensive programming, handling non-Discord messages, missing rooms, DMs, and service availability checks with graceful fallbacks.
124-244: LGTM!The role extraction, permission mapping, and text generation logic are well-implemented. The comprehensive permission list covers the most relevant Discord capabilities, and the behavioral guidelines provide useful context for the agent.
249-257: LGTM!The
formatPermissionNamehelper correctly converts camelCase to Title Case using a clean regex approach.src/index.ts (5)
18-29: LGTM!New imports for actions and providers are well-organized and align with the expanded plugin functionality.
31-53: LGTM!The expanded exports are well-documented with clear grouping comments, making it easy for external consumers to understand what's available.
156-159: LGTM!New actions and providers are properly wired into the plugin definition.
229-248: LGTM!Token validation correctly checks only
DISCORD_API_TOKENandDISCORD_BOT_TOKENS. The warning message is clear, actionable, and properly clarifies thatDISCORD_APPLICATION_IDis the OAuth2 client ID, not a bot token.
256-292: LGTM!The additional exports are well-organized with clear grouping comments, providing a comprehensive public API for external consumers.
…, firstUpdateSent timing - setVoiceChannelStatus: use channelId/messageServerId pattern like other actions - index.ts: remove duplicate isDataUrl, import from utils - progressiveMessage: set firstUpdateSent before async callback to prevent minDelay bypass
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/progressiveMessage.ts (1)
326-340: Consider storing the flush promise directly.The polling approach works but could be simplified by storing the callback promise as a class field and awaiting it directly in
waitForFlushComplete(), eliminating the need for polling loops and timeouts.🔎 Optional refactor using promise-based approach
Add a field to store the flush promise:
+ private flushPromise: Promise<void> | null = null;Update
flushUpdate()to store the promise:private flushUpdate(): void { // ... existing guard logic ... this.flushInProgress = true; this.firstUpdateSent = true; // ... existing content setup ... - this.callback(content) + this.flushPromise = this.callback(content) .catch(error => { logger.warn(`Progressive update flush failed: ${error}`); }) .finally(() => { this.flushInProgress = false; + this.flushPromise = null; }); }Simplify
waitForFlushComplete():private async waitForFlushComplete(): Promise<void> { - if (!this.flushInProgress) return; - - const maxWait = 2000; - const pollInterval = 50; - const startTime = Date.now(); - - while (this.flushInProgress && (Date.now() - startTime) < maxWait) { - await new Promise(resolve => setTimeout(resolve, pollInterval)); - } - - if (this.flushInProgress) { - logger.warn('Progressive message flush timed out - proceeding with final message'); - } + if (this.flushPromise) { + await this.flushPromise; + } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
src/actions/setVoiceChannelStatus.tssrc/index.tssrc/progressiveMessage.ts
✅ Files skipped from review due to trivial changes (1)
- src/index.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/progressiveMessage.ts (2)
src/index.ts (1)
ProgressiveMessage(45-45)src/service.ts (1)
delay(3501-3503)
src/actions/setVoiceChannelStatus.ts (2)
src/index.ts (3)
DiscordService(255-255)DiscordService(256-256)DISCORD_SERVICE_NAME(254-254)src/service.ts (2)
DiscordService(117-4854)setVoiceChannelStatus(4738-4787)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (11)
src/actions/setVoiceChannelStatus.ts (4)
18-63: Well-structured utility functions with proper backwards compatibility.The
isDiscordSnowflakehelper correctly distinguishes Discord IDs from UUIDs, andgetGuildFromRoomimplements the established pattern (channelId primary, messageServerId fallback with snowflake validation) mentioned in past review feedback. The type assertions are safely guarded by subsequent null and property checks.
90-144: Excellent defensive validation that addresses past review feedback.The function properly validates both
channelIdentifierandstatusMessageas strings with explicit type guards (lines 116-123, 127-134), preventing undefined values from reachingdiscordService.setVoiceChannelStatus. The retry logic with debug logging and final warning after 3 attempts is robust. Allowing empty strings forstatusMessageis correct for clearing voice channel status.
149-229: Null-safe channel resolution that addresses past review feedback.The function correctly addresses the previous concern about unsafe optional chaining by:
- Pre-computing normalized strings once (lines 174-177) to avoid repeated
toLowerCase()calls- Explicitly checking
chandch.nameexistence before accessing properties (lines 186, 207)- Using
runtime.logger.errorinstead ofconsole.error(line 226)The search strategy (current guild first via
getGuildFromRoom, then all guilds) is well-designed and handles both ID and name-based lookups with proper error recovery.
231-416: Comprehensive action implementation with proper error handling.The handler follows best practices:
- Validates Discord source and service availability upfront (lines 243-265)
- Uses the established room property pattern with
channelIdandmessageServerId(lines 278-280)- Provides user-friendly error messages at each failure point
- Creates structured memory with metadata on success (lines 310-328)
- Includes diverse examples covering set, update, clear, and emoji scenarios (lines 351-412)
The integration with
findVoiceChannelanddiscordService.setVoiceChannelStatusis clean, and the comprehensive try-catch with detailed logging ensures operational visibility.src/progressiveMessage.ts (7)
4-65: Excellent documentation that explains design rationale.The comprehensive comments explain not just what the code does, but why each design decision was made (debouncing, throttling, important flag, etc.). This makes the code highly maintainable.
93-106: LGTM! Constructor with sensible defaults.The default values (300ms minDelay to suppress fast operations, 500ms throttle to stay under Discord rate limits) align well with the documented rationale.
141-194: LGTM! Correct debouncing and throttling implementation.The update method properly handles:
- Platform-specific behavior (important flag for non-Discord sources)
- First-update debouncing to suppress fast operations
- Throttled subsequent updates with proper timer management
227-230: Timing fix correctly applied!The previous review concern about
firstUpdateSenttiming has been properly addressed. SettingfirstUpdateSent = trueimmediately (line 230) before the async callback ensures subsequentupdate()calls respect throttle timing and don't bypassminDelaywhile the flush is in-flight.
212-255: Race condition properly mitigated.The
flushInProgressflag and retry scheduling (lines 217-225) correctly prevent concurrent flushes from creating duplicate messages. The cleanup in the.finally()block ensures the flag is always reset.
262-287: LGTM! Proper synchronization before final message.The
complete()method correctly:
- Clears pending timers
- Waits for in-progress flushes to prevent race conditions
- Implements fast-path optimization for quick operations
345-366: LGTM! Clean final message handling.The
sendFinal()method correctly:
- Conditionally includes progressive metadata
- Sets
isInterim: falseto create a memory for the final message- Handles errors gracefully by returning an empty array
Discord.js v14+ requires Partials.Channel to receive DM messages. Without it, DM channels aren't cached and messageCreate events don't fire for direct messages. - Add Partials.Channel, Partials.Message, Partials.User to Client config - Add debug logging for messageCreate events and event listener setup
- reactToMessage: Return null when all emojis are forbidden instead of using neutral[0], matching docstring and LLM path behavior - clientRegistry: Call client.destroy() on failed bot registration to release Discord.js resources (REST client, caches, WebSocket state)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/clientRegistry.ts (2)
208-269: Consider exposingloginPromisesor removing unused tracking.The
loginPromisesmap stores pending login promises but is never read from. This could be useful for:
- Awaiting all pending logins before operations
- Cancellation support in
destroyAllIf not needed, consider removing the tracking to reduce complexity.
324-327: Dead code: alias is never a map key after successful login.After login succeeds, clients are re-keyed from
tempId(alias) tobotIdat lines 242-244. The alias is stored inconfig.aliasbut is no longer a key in theclientsmap, making this delete a no-op.Consider removing the redundant delete or adding a comment explaining when this branch would be relevant.
Suggested simplification
// Cleanup clientInfo.client.destroy(); this.clients.delete(botId); - if (clientInfo.config.alias) { - this.clients.delete(clientInfo.config.alias); - }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
src/actions/reactToMessage.tssrc/clientRegistry.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (13)
src/actions/reactToMessage.ts (5)
127-162: LGTM!The sentiment detection implementation is clean. The keyword-based approach with a neutral fallback is appropriate for this use case, and the emoji mappings provide good coverage for common sentiments.
164-218: LGTM!The character preference utilities handle both configuration formats cleanly. The defensive programming with optional chaining and array/object type guards ensures robustness against malformed settings.
271-300: Previous forbidden emoji fallback issue is now properly addressed.The multi-tier fallback chain (neutral → all categories → null) ensures a forbidden emoji is never returned. This correctly addresses the previous review comments about
sentimentEmojis.neutral[0]potentially being forbidden.
389-409: LGTM!The character path correctly distinguishes between style-forbidden (early return) and preference-forbidden (fall through to LLM) cases. The flow allows the LLM path a chance to find alternatives like custom Discord emojis not in the predefined sentiment lists.
427-461: Previous LLM path forbidden emoji issue is now properly addressed.The refactored logic correctly handles forbidden emojis:
- Tries sentiment-matched alternatives first
- Falls back to filtered neutral emojis
- Sets
emoji = nullwhen no safe option exists (line 450)- Only creates
reactionInfowhenemojiis valid (line 455)This correctly addresses the previous comments about the
|| emojifallback returning forbidden emojis.src/clientRegistry.ts (8)
1-6: LGTM!Imports are well-organized with clear separation between discord.js, core framework, and local module dependencies.
8-17: LGTM!The interface cleanly separates construction-time properties from post-login properties with appropriate optionality.
19-59: LGTM!Comprehensive token validation with clear, actionable error messages for each failure mode. The structured return type allows callers to provide specific guidance.
64-73: LGTM!Clean class structure with appropriate private state management using Maps for efficient lookups.
78-104: LGTM!The token handling is now correct. The code properly uses only
DISCORD_API_TOKENfor authentication and the comments clearly document thatDISCORD_APPLICATION_IDis not a valid token.
179-200: LGTM!The error handling now correctly calls
client.destroy()to release Discord.js resources before cleanup. The troubleshooting messages are helpful for debugging token issues.
274-287: LGTM!The two-phase lookup (direct key then alias scan) handles both ID and alias queries efficiently for typical bot counts.
376-447: LGTM!Well-structured event emission with appropriate handling for large guilds. The fire-and-forget background user sync pattern at lines 432-436 avoids blocking startup while still attempting to pre-populate user data for small guilds.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Fixes race condition where in-flight login callbacks could add clients back to the map after destroyAll cleared it. Now uses Promise.allSettled to wait for pending logins to complete or fail before destroying clients.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/clientRegistry.ts`:
- Around line 216-268: The timeout path can reject while the
client.once('ready') handler remains attached, allowing that handler to run
later and re-key this.clients; to fix, store the ready callback in a named
function (the handler passed to client.once('ready')) and on timeout (the
timeout variable's callback) remove that handler (e.g., client.removeListener or
client.off for 'ready') and also remove the 'error' listener you attached, then
reject; likewise ensure you clear the timeout and remove those listeners in the
client.login.catch path and any early rejects so no lingering listeners remain.
🧹 Nitpick comments (1)
src/clientRegistry.ts (1)
24-59: Token validation doesn't return the trimmed token.The function validates
trimmedTokenbut returns only{ valid: true }without the trimmed value. Callers use the original (possibly whitespace-padded) token. If a token has leading/trailing whitespace, it will pass validation but may fail at Discord login.Consider returning the sanitized token:
♻️ Suggested improvement
-function validateDiscordToken(token: string): { valid: boolean; error?: string } { +function validateDiscordToken(token: string): { valid: boolean; error?: string; sanitizedToken?: string } { if (!token) { return { valid: false, error: 'Token is empty or undefined' }; } const trimmedToken = token.trim(); // ... existing validation ... - return { valid: true }; + return { valid: true, sanitizedToken: trimmedToken }; }Then use
validation.sanitizedTokeninstead of the original token when registering bots.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
src/clientRegistry.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/clientRegistry.ts (3)
src/voice.ts (1)
VoiceManager(347-3294)src/service.ts (1)
DiscordService(117-4872)src/index.ts (1)
DiscordEventTypes(259-259)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (7)
src/clientRegistry.ts (7)
78-104: LGTM - Token fallback clarification addressed.The comments on lines 84-85, 94, and 101 now clearly document that
DISCORD_APPLICATION_IDis the OAuth2 client/application ID used for invite URLs, not for bot authentication. This addresses the previous review concern about confusing error messages.
179-200: LGTM - Client cleanup on failed registration now implemented.The error handler at lines 195-196 now properly calls
client.destroy()before removing from maps, addressing the previous review concern about unreleased Discord.js Client resources.
274-287: LGTM!The lookup logic correctly handles both direct ID lookup and alias-based search.
292-306: LGTM!Both utility methods are implemented correctly with clear semantics.
311-328: LGTM!The removal logic properly cleans up the client and handles both ID and alias keys defensively.
346-366: LGTM - Race condition fix properly implemented.The
destroyAllmethod now awaits pending logins viaPromise.allSettledbefore destroying clients, addressing the previous review concern about in-flight logins surviving cleanup. Theclients.clear()on line 364 is redundant sinceremoveBotalready deletes entries, but it's harmless as a defensive measure.
386-457: Well-structured world event emission with appropriate optimization.The method properly emits both Discord-specific and platform-agnostic
WORLD_CONNECTEDevents. The optimization to skip user pre-fetch for large guilds (>1000 members) and use background sync for smaller guilds is a good design choice that prevents blocking startup. BothbuildStandardizedRoomsandbuildStandardizedUsersmethods exist as public methods on the service and are correctly used here.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
The timeout, ready, and login error paths could leave lingering event listeners attached to the client. If timeout fired first, the ready handler could still execute later and re-key the clients map. - Add settled flag to prevent multiple path execution - Create cleanup function to remove all listeners and clear timeout - Call cleanup in all exit paths (ready, timeout, login error) - Use client.off() to remove named handlers properly
1 participant
Note
Introduces scalable voice and messaging architecture with clear contracts, multi-bot support, and user-visible status improvements.
DiscordClientRegistryandVoiceConnectionManagerwith per-bot clients andVoiceManagerintegration; exportsCHANNEL_TTS/MUSIC/SFX/AMBIENTand defaults inaudioChannels.tswith priority/duckingIAudioSink(src/contracts.ts) andDiscordAudioSinkexport for broadcast-based music integration; docs detail auto-wiring and reconnectionProgressiveMessage, MessageManager adjustments, and README/guide (PROGRESSIVE_UPDATES.md) for in-place edits with throttling/TTLSET_VOICE_CHANNEL_STATUSandSET_LISTENING_ACTIVITYwith robust parsing/validation; existing actions updated to resolve guild viachannelIdwith snowflake-safe fallbacksattachmentsDISCORD_API_TOKEN/DISCORD_BOT_TOKENS), early validation with actionable errors; additional voice env defaultsaudioState,agentRole, and plugin info providers; exports sink/contracts, channels, multi-bot typesMUSIC_ARCHITECTURE.md,COORDINATION_PATTERNS.md), README expansions; tests for token validation, progressive updates, message manager, voice pipeline, and env changesWritten by Cursor Bugbot for commit efb42f9. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Documentation
Providers
Utilities
Tests
✏️ Tip: You can customize this high-level summary in your review settings.