feat: dynamic voice#145
Conversation
WalkthroughReplaced several uses of the container helper Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Discord as "Discord API / Guild"
participant Bot as "Bot (Slash Command/Event Handler)"
participant Cache as "App Cache (voice_channels)"
participant Task as "DynamicVoiceTask"
User->>Bot: /sala (tipo, quantidade)
Bot->>Discord: create voice channel (ChannelBuilder) (await)
Discord-->>Bot: Channel created (channelId)
Bot->>Cache: persist VoiceChannelDTO (guildId, channelId, ownerId, usersCount=0, lastJoinedAt=now)
Bot->>User: respond with embed + channel link
Note over Discord,Bot: Later, voice state updates occur
Discord->>Bot: VOICE_STATE_UPDATE (user joined/left)
Bot->>Cache: update VoiceChannelDTO (add/remove user, inc/dec usersCount, update lastJoinedAt)
Bot-->>Discord: (no direct change on join/leave)
par periodic cleanup
Task->>Cache: read active voice channels
alt channel.isEmpty() and isLongTermEmpty()
Task->>Discord: delete channel by channelId
Discord-->>Task: deletion ack
Task->>Cache: remove channel entry and reindex
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
app-modules/message/src/Actions/NewMessage.php (1)
23-23: Consider injecting UserCharacterResolver instead of resolving in the methodThe
resolve(UserCharacterResolver::class)->resolve(...)call works, but sincePersistMessageis already constructor-injected, you could optionally injectUserCharacterResolveras well to avoid the service-locator pattern and make this action easier to test.app-modules/bot-discord/src/SlashCommands/EditProfileCommand.php (1)
142-142: Optional: inject UpdateProfile instead of resolving it inside persistDataCalling
resolve(UpdateProfile::class)->handle($payload)is fine, but since this command is already using a DTO and has other collaborators, you might consider constructor-injectingUpdateProfileto avoid the service-locator pattern and make the command easier to unit test.app-modules/bot-discord/src/Tasks/DynamicVoiceTask.php (1)
44-49: Redundant cache removal logic.The
unset($channels[$arrayIndex])followed byarray_filterchecking for the samechannelIdis redundant. The filter alone is sufficient to remove the channel by ID, andunsetby index can be problematic if the cache was modified between reads.if (isset($channels[$arrayIndex])) { - - unset($channels[$arrayIndex]); - $channels = array_filter($channels, fn (array $channel) => $channel['channelId'] !== $channelId); $channels = array_values($channels); - - dump($channels); cache()->tags(['voice_channels'])->put('active_voice_channels_keys', $channels); }app-modules/bot-discord/src/SlashCommands/DynamicVoiceCommand.php (2)
125-125: Simplify redundant string conversion.
$item['name']is already a string, sostr($item['name'])->toString()is unnecessary.- return array_map(fn (array $item) => ['name' => $item['name'], 'value' => str($item['name'])->toString()], $items); + return array_map(fn (array $item) => ['name' => $item['name'], 'value' => $item['name']], $items);
101-106: Consider adding input validation for the user limit.The
quantidadeoption accepts any integer without bounds. Discord voice channels have limits (max 99 users, min 0 for unlimited). Invalid values could cause API errors.[ 'name' => 'quantidade', 'description' => 'Manage how many people can use the voice channel.', 'type' => Option::INTEGER, 'required' => true, + 'min_value' => 1, + 'max_value' => 99, ],app-modules/bot-discord/src/Events/DynamicVoiceEvent.php (3)
61-72: Grammar: renameleavedChanneltoleftChannel.The past tense of "leave" is "left", not "leaved".
- private function leavedChannel(array $activeChannels, $user): void + private function leftChannel(string $channelId, array $activeChannels, string $user): void { foreach ($activeChannels as $index => $channel) { - - if (in_array($user, $channel['users'])) { + if ($channel['channelId'] === $channelId && in_array($user, $channel['users'], true)) { $activeChannels[$index]['users'] = array_values(array_filter($channel['users'], fn ($userId) => $userId !== $user)); $activeChannels[$index]['usersCount']--; cache()->tags(['voice_channels'])->put('active_voice_channels_keys', $activeChannels); break; } } }
47-47: Add type hint for$userparameter.The
$userparameter lacks a type hint. Since$state->user_idis a string, add the type declaration for consistency and type safety.- private function joinedChannel(string $channelId, array $activeChannels, $user): void + private function joinedChannel(string $channelId, array $activeChannels, string $user): void
65-65: Use strict comparison inin_array.Without the third parameter set to
true,in_arrayuses loose comparison which can lead to unexpected matches.- if (in_array($user, $channel['users'])) { + if (in_array($user, $channel['users'], true)) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
app-modules/authentication/src/Enums/OAuthProviderEnum.php(1 hunks)app-modules/authentication/src/Http/Controllers/TenantLogoutController.php(1 hunks)app-modules/authentication/tests/Feature/Actions/AuthenticateActionTest.php(2 hunks)app-modules/bot-discord/config/he4rt-bot-discord.php(1 hunks)app-modules/bot-discord/src/Events/DynamicVoiceEvent.php(1 hunks)app-modules/bot-discord/src/Events/MessageReceivedEvent.php(1 hunks)app-modules/bot-discord/src/Events/WelcomeMember.php(1 hunks)app-modules/bot-discord/src/Providers/BotDiscordServiceProvider.php(1 hunks)app-modules/bot-discord/src/SlashCommands/DynamicVoiceCommand.php(2 hunks)app-modules/bot-discord/src/SlashCommands/EditProfileCommand.php(1 hunks)app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php(1 hunks)app-modules/bot-discord/src/Tasks/DynamicVoiceTask.php(2 hunks)app-modules/bot-discord/tests/Feature/Actions/UserCharacterResolverTest.php(2 hunks)app-modules/events/src/Filament/App/EventModels/Pages/ListEventModels.php(2 hunks)app-modules/events/src/Filament/Shared/EventLogin.php(2 hunks)app-modules/events/src/Models/EventAgenda.php(1 hunks)app-modules/integrations/src/Discord/OAuth/DiscordOAuthAccessDTO.php(1 hunks)app-modules/integrations/src/Twitch/OAuth/DTO/TwitchOAuthDTO.php(1 hunks)app-modules/message/src/Actions/NewMessage.php(1 hunks)app-modules/provider/src/Models/Token.php(1 hunks)app-modules/user/src/Filament/User/Pages/UserProfile.php(1 hunks)composer.json(3 hunks)config/discord.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
app-modules/bot-discord/src/SlashCommands/EditProfileCommand.php (1)
app-modules/user/src/Actions/UpdateProfile.php (2)
UpdateProfile(12-32)handle(14-31)
app-modules/events/src/Filament/App/EventModels/Pages/ListEventModels.php (2)
app-modules/events/src/Actions/AttendEventAction.php (2)
AttendEventAction(10-20)execute(15-19)app-modules/events/src/Actions/LeaveEventAction.php (2)
execute(11-14)LeaveEventAction(9-15)
app-modules/bot-discord/src/Events/WelcomeMember.php (1)
app-modules/bot-discord/src/Actions/UserCharacterResolver.php (2)
resolve(16-69)UserCharacterResolver(14-70)
app-modules/integrations/src/Discord/OAuth/DiscordOAuthAccessDTO.php (3)
app-modules/integrations/src/Twitch/OAuth/DTO/TwitchOAuthDTO.php (1)
make(13-26)app-modules/authentication/src/DTO/OAuthUserDTO.php (1)
make(21-21)app-modules/integrations/src/Discord/OAuth/DiscordOAuthUser.php (1)
make(13-24)
app-modules/bot-discord/tests/Feature/Actions/UserCharacterResolverTest.php (1)
app-modules/bot-discord/src/Actions/UserCharacterResolver.php (2)
resolve(16-69)UserCharacterResolver(14-70)
app-modules/authentication/tests/Feature/Actions/AuthenticateActionTest.php (1)
app-modules/authentication/src/Actions/AuthenticateAction.php (1)
AuthenticateAction(19-96)
app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php (1)
app-modules/user/src/Actions/UpdateProfile.php (2)
UpdateProfile(12-32)handle(14-31)
app-modules/bot-discord/src/Events/MessageReceivedEvent.php (3)
app-modules/bot-discord/src/Actions/UserCharacterResolver.php (1)
resolve(16-69)app-modules/message/src/Actions/NewMessage.php (2)
NewMessage(13-54)persist(20-53)app-modules/message/src/DTO/NewMessageDTO.php (1)
NewMessageDTO(10-36)
app-modules/authentication/src/Enums/OAuthProviderEnum.php (2)
app-modules/bot-discord/src/Actions/UserCharacterResolver.php (1)
resolve(16-69)app-modules/integrations/src/Discord/OAuth/DiscordOAuthClient.php (1)
DiscordOAuthClient(13-46)
app-modules/message/src/Actions/NewMessage.php (1)
app-modules/bot-discord/src/Actions/UserCharacterResolver.php (2)
resolve(16-69)UserCharacterResolver(14-70)
app-modules/authentication/src/Http/Controllers/TenantLogoutController.php (1)
app-modules/authentication/src/Http/Responses/TenantLogoutResponse.php (1)
TenantLogoutResponse(11-26)
app-modules/bot-discord/src/Events/DynamicVoiceEvent.php (3)
app-modules/bot-discord/src/SlashCommands/DynamicVoiceCommand.php (1)
handle(64-89)app-modules/bot-discord/src/Tasks/DynamicVoiceTask.php (1)
handle(24-32)app-modules/docs/src/DocsController.php (1)
index(35-43)
🪛 PHPMD (2.15.0)
app-modules/bot-discord/src/Events/DynamicVoiceEvent.php
24-24: Avoid unused parameters such as '$discord'. (undefined)
(UnusedFormalParameter)
24-24: Avoid unused parameters such as '$oldState'. (undefined)
(UnusedFormalParameter)
⏰ 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: Perform Pest Tests / Run
🔇 Additional comments (18)
app-modules/provider/src/Models/Token.php (1)
22-23: LGTM!The code is well-structured with proper trait usage, appropriate access control, and a clean factory pattern implementation. The spacing adjustment improves readability.
app-modules/events/src/Models/EventAgenda.php (1)
16-16: Database migration already includes softDeletes() support.The migration file
app-modules/events/database/migrations/2025_11_27_145728_create_events_agenda_table.phpalready contains$table->softDeletes()(line 24), so thedeleted_atcolumn is properly defined. The Filament admin resource is also fully configured to handle soft-deleted records with restore and force-delete actions, and the resource query explicitly includes soft-deleted records viawithoutGlobalScopes([SoftDeletingScope::class]).No action is required for database migration support—it's complete and properly integrated.
Likely an incorrect or invalid review comment.
app-modules/integrations/src/Discord/OAuth/DiscordOAuthAccessDTO.php (1)
11-17: Return type narrowed toselffor a more precise factorySwitching
make()to returnselfmatches the concreteDiscordOAuthAccessDTOinstance the method already constructs, improving static typing and aligning with the pattern used in other integrations DTOs, without changing runtime behavior.app-modules/integrations/src/Twitch/OAuth/DTO/TwitchOAuthDTO.php (1)
13-25: Alignsmake()return type with the abstract base and concrete DTOHaving
TwitchOAuthDTO::make()returnselfmatches the abstractOAuthUserDTO::make(): selfcontract and communicates to callers that they get a concreteTwitchOAuthDTO, with no change to the underlying logic.config/discord.php (1)
5-9: Discord options config looks consistent and self‑documentingThe new
optionsblock is clear, typed correctly, and the inline comment explains the transport compression setting; no issues from a config/structure standpoint.app-modules/bot-discord/tests/Feature/Actions/UserCharacterResolverTest.php (1)
13-13: Container resolution helper swap in tests is fineUsing
resolve(UserCharacterResolver::class)in both tests keeps the same behavior and is consistent with the rest of this PR’s helper changes.Also applies to: 63-63
app-modules/authentication/src/Http/Controllers/TenantLogoutController.php (1)
23-23: Logout response resolution remains correctSwitching to
resolve(TenantLogoutResponse::class)keeps the container-based instantiation and still satisfies theLogoutResponsereturn type.app-modules/authentication/tests/Feature/Actions/AuthenticateActionTest.php (1)
58-58: AuthenticateAction resolution helper change is safeBoth tests now resolve
AuthenticateActionviaresolve(...), which is equivalent here and consistent with the rest of the codebase changes.Also applies to: 101-101
app-modules/events/src/Filament/Shared/EventLogin.php (1)
146-146: Login and failure event resolution changes maintain behaviorResolving
LoginResponse::classandFailed::classviaresolve(...)keeps the same container-driven behavior as before; the authentication flow and failed-login event dispatch remain intact.Also applies to: 303-303
app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php (1)
132-151: Standardize container resolution forUpdateProfileSwitching to
resolve(UpdateProfile::class)->handle($payload)keeps the flow intact while aligning with the project‑wide pattern for resolving actions. No further changes needed here.app-modules/bot-discord/src/Events/MessageReceivedEvent.php (1)
33-48: Message persistence handler resolution aligned with new conventionUsing
resolve(NewMessage::class)->persist(...)here is consistent with the rest of the PR and doesn’t alter the DTO payload or error handling in this event.app-modules/bot-discord/src/Events/WelcomeMember.php (1)
24-35: UserCharacterResolver resolution updated without behavior changeThe switch to
resolve(UserCharacterResolver::class)->resolve(...)keeps the welcome logic identical while matching the DI style used elsewhere in the PR.app-modules/user/src/Filament/User/Pages/UserProfile.php (1)
485-503: Email‑change notifications now resolved via the container helperResolving
VerifyEmailChangeandNoticeOfEmailChangeRequestthrough the helper keeps the existing flow (URL generation, cache key, mail routes) intact while unifying how these notifications are instantiated. No further adjustments needed in this method.app-modules/bot-discord/src/Providers/BotDiscordServiceProvider.php (1)
12-18: Register method correctly merges Bot Discord configOverriding
register()to call the parent and thenmergeConfigFrom(__DIR__.'/../../config/he4rt-bot-discord.php', 'he4rt-bot-discord')cleanly wires the new module config without affecting the existingbot()setup.app-modules/bot-discord/config/he4rt-bot-discord.php (1)
1-7: New Bot Discord config for dynamic voice categoryThis minimal config file is correctly structured and matches the
he4rt-bot-discordconfig key merged in the service provider. Since thecategory_idis guild‑specific, keeping it in config is appropriate and allows environment‑level overrides if needed.composer.json (1)
13-71: Framework & tooling version bumps – verify ecosystem compatibilitySeveral core dependencies (Laravel 12.41.1, Filament 4.3.0, Flux 2.9.2, Rector, Sail, Pest) have been updated. Verify the following before merging:
- All automated test suites pass after
composer update:composer checkruns static analysis (Rector, Pint, PHPStan);composer testruns the Pest suite.- Filament/Laravel integration works in a local environment (panels, auth, profile page).
- No new deprecations introduced by these versions, especially the Filament + Laravel combination.
Both scripts are configured in composer.json and ready to use.
app-modules/authentication/src/Enums/OAuthProviderEnum.php (1)
22-27: Container bindings properly configured for OAuth clientsBoth
TwitchOAuthService::classandDiscordOAuthClient::classare correctly bound in the container and implementOAuthClientContract.TwitchOAuthClient(bound toTwitchOAuthService) implements theTwitchOAuthServiceinterface which extendsOAuthClientContract, andDiscordOAuthClientdirectly implementsOAuthClientContract. The return type is satisfied.app-modules/bot-discord/src/Tasks/DynamicVoiceTask.php (1)
26-31: Potential issue with Carbon deserialization from cache.
$channel['lastJoinedAt']->diffInSeconds(now())assumeslastJoinedAtis a Carbon instance. Depending on your cache driver (e.g., Redis, database), Carbon objects may not survive serialization/deserialization properly and could become strings or arrays.Verify that your cache driver properly handles Carbon objects, or consider storing timestamps as integers/strings and parsing them:
- if ($channel['usersCount'] === 0 && abs($channel['lastJoinedAt']->diffInSeconds(now())) >= 20) { + $lastJoinedAt = $channel['lastJoinedAt'] instanceof \Carbon\Carbon + ? $channel['lastJoinedAt'] + : \Carbon\Carbon::parse($channel['lastJoinedAt']); + if ($channel['usersCount'] === 0 && abs($lastJoinedAt->diffInSeconds(now())) >= 20) {
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app-modules/bot-discord/src/Events/DynamicVoiceEvent.php (1)
25-75: Fix voice state handling to correctly process channel switches and leaves.Right now the handler only:
- Adds the user to the new channel when
$state->channel_idis non‑null, or- Removes the user from some channel when
$state->channel_idis null.It never removes the user from the previous channel when they switch, so
users/usersCountdrift over time. This is the same issue previously flagged in review and still unresolved. You already have$oldState, so you can handle leave and switch explicitly and makeleavedChanneltarget a specific channel.A concrete approach:
- public function handle(VoiceStateUpdate $state, Discord $discord, ?VoiceStateUpdate $oldState): void - { - $channelId = $state->channel_id; - $user = $state->user_id; - $activeChannels = cache()->tags(['voice_channels'])->get('active_voice_channels_keys', []); - $this->logger()->info('Channel Members:'.($state->channel?->members?->count() ?? 0)); - - if (! is_null($channelId)) { - $this->joinedChannel( - channelId: $channelId, - activeChannels: $activeChannels, - user: $user - ); - - return; - } - - $this->leavedChannel( - activeChannels: $activeChannels, - user: $user - ); - } + public function handle(VoiceStateUpdate $state, Discord $discord, ?VoiceStateUpdate $oldState): void + { + $channelId = $state->channel_id; + $oldChannelId = $oldState?->channel_id; + $user = $state->user_id; + $activeChannels = cache()->tags(['voice_channels'])->get('active_voice_channels_keys', []); + + $this->logger()->info('Channel Members:'.($state->channel?->members?->count() ?? 0)); + + // Handle leave/switch from previous channel (if any). + if ($oldChannelId !== null && $oldChannelId !== $channelId) { + $this->leavedChannel( + channelId: $oldChannelId, + activeChannels: $activeChannels, + user: $user, + ); + + // Refresh cache after removal so join uses the updated state. + $activeChannels = cache()->tags(['voice_channels'])->get('active_voice_channels_keys', []); + } + + // Handle join into the new channel (if any). + if ($channelId !== null) { + $this->joinedChannel( + channelId: $channelId, + activeChannels: $activeChannels, + user: $user, + ); + } + } @@ - private function joinedChannel(string $channelId, array $activeChannels, $user): void + private function joinedChannel(string $channelId, array $activeChannels, string $user): void @@ - private function leavedChannel(array $activeChannels, $user): void + private function leavedChannel(string $channelId, array $activeChannels, string $user): void { foreach ($activeChannels as $index => $channel) { /** @var VoiceChannelDTO $channel */ - if (in_array($user, $channel->users)) { - $activeChannels[$index]->users = array_values(array_filter($channel->users, fn ($userId) => $userId !== $user)); - $activeChannels[$index]->usersCount--; - - cache()->tags(['voice_channels'])->put('active_voice_channels_keys', $activeChannels); - break; - } + if ($channel->channelId !== $channelId) { + continue; + } + + if (in_array($user, $channel->users, true)) { + $activeChannels[$index]->users = array_values(array_filter( + $channel->users, + fn (string $userId): bool => $userId !== $user, + )); + + $activeChannels[$index]->usersCount--; + + cache()->tags(['voice_channels'])->put('active_voice_channels_keys', $activeChannels); + } + + break; } }This:
- Correctly removes users from their previous channel on both pure leave and channel switch.
- Keeps membership and
usersCountin sync per channel.- Starts using
$oldState(addressing that PHPMD warning); only$discordremains unused for now, which you can keep for signature compatibility.app-modules/bot-discord/src/SlashCommands/DynamicVoiceCommand.php (1)
67-88: UseGuild::createChannel()(or repository create/save) instead ofchannels->build()so the voice channel actually exists.
$interaction->guild->channels->build(...)only constructs a channel payload/part; it does not persist the channel on Discord. This meansawait(...)here likely gives you a local object whose ID isn’t backed by a real guild channel. A previous review already called this out and the code still usesbuild().DiscordPHP exposes
Guild::createChannel(ChannelBuilder|Channel|string)specifically for this use case. You can keep usingChannelBuilderbut let the guild create the channel:- $channel = await($interaction->guild->channels->build( - $interaction->guild, - ChannelBuilder::new($this->value('tipo')) - ->setType(2) - ->setUserLimit($this->value('quantidade')) - ->setParentId(config('he4rt-bot-discord.category_id', '1447692330235859104')) // TODO: change to "use/sala" category id - )); + $channelBuilder = ChannelBuilder::new($this->value('tipo')) + ->setType(2) + ->setUserLimit($this->value('quantidade')) + ->setParentId(config('he4rt-bot-discord.category_id', '1447692330235859104')); // TODO: change to "use/sala" category id + + $channel = await($interaction->guild->createChannel($channelBuilder));This ensures the command actually creates the voice channel before you cache its ID or send it back to the user.
Please double‑check against the DiscordPHP docs for your exact version to confirm
Guild::createChannelwithChannelBuilderis the recommended pattern.
🧹 Nitpick comments (1)
app-modules/bot-discord/src/DTO/VoiceChannelDTO.php (1)
32-40: SimplifyisLongTermEmpty()by relying on Carbon’s absolute diff.
diffInSeconds()is already absolute by default, so the extraabs()is redundant, and using$this->lastJoinedAtas the base reads clearer:- public function isLongTermEmpty(): bool - { - return abs(now()->diffInSeconds($this->lastJoinedAt)) >= 20; - } + public function isLongTermEmpty(): bool + { + return $this->lastJoinedAt->diffInSeconds(now()) >= 20; + }This keeps behavior identical while tightening up the implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app-modules/bot-discord/src/DTO/VoiceChannelDTO.php(1 hunks)app-modules/bot-discord/src/Events/DynamicVoiceEvent.php(1 hunks)app-modules/bot-discord/src/SlashCommands/DynamicVoiceCommand.php(2 hunks)app-modules/bot-discord/src/Tasks/DynamicVoiceTask.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app-modules/bot-discord/src/Tasks/DynamicVoiceTask.php
🧰 Additional context used
🧬 Code graph analysis (1)
app-modules/bot-discord/src/SlashCommands/DynamicVoiceCommand.php (1)
app-modules/bot-discord/src/DTO/VoiceChannelDTO.php (2)
VoiceChannelDTO(9-41)make(20-30)
🪛 PHPMD (2.15.0)
app-modules/bot-discord/src/Events/DynamicVoiceEvent.php
25-25: Avoid unused parameters such as '$discord'. (undefined)
(UnusedFormalParameter)
25-25: Avoid unused parameters such as '$oldState'. (undefined)
(UnusedFormalParameter)
3 participants
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.