fix: voice state#149
Merged
Merged
Conversation
WalkthroughRefactors voice-channel handling into action classes, makes Changes
Sequence DiagramsequenceDiagram
participant Event as DynamicVoiceEvent
participant HSC as HandleStateChannelAction
participant Cache as Cache (voice_channels / voice_tracking)
participant JCA as JoiningChannelAction
participant LCA as LeftChannelAction
Event->>HSC: execute(userId, channelId)
activate HSC
HSC->>Cache: get('active_voice_channels_keys')
Cache-->>HSC: activeChannels
HSC->>Cache: get('user_last_channel:{userId}')
Cache-->>HSC: oldChannelId
alt Joining
HSC->>JCA: execute(channelId, activeChannels, user)
activate JCA
JCA->>Cache: save updated activeChannels (add user, usersCount++, lastJoinedAt=now)
JCA-->>HSC: done
deactivate JCA
HSC->>Cache: set('user_last_channel:{userId}', channelId, TTL=24h)
else Moving
HSC->>LCA: execute(activeChannels, user)
activate LCA
LCA->>Cache: save activeChannels (remove user, usersCount--)
LCA-->>HSC: done
deactivate LCA
HSC->>JCA: execute(newChannelId, activeChannels, user)
activate JCA
JCA->>Cache: save activeChannels (add user, usersCount++, lastJoinedAt=now)
JCA-->>HSC: done
deactivate JCA
HSC->>Cache: set('user_last_channel:{userId}', newChannelId, TTL=24h)
else Leaving
HSC->>LCA: execute(activeChannels, user)
activate LCA
LCA->>Cache: save activeChannels (remove user, usersCount--)
LCA-->>HSC: done
deactivate LCA
HSC->>Cache: forget('user_last_channel:{userId}')
else UpdatingSame
HSC->>HSC: no cache structural change (maybe update lastJoinedAt)
end
deactivate HSC
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
danielhe4rt
previously approved these changes
Dec 12, 2025
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
app-modules/bot-discord/src/Events/DynamicVoiceEvent.php (1)
115-118: Tighten types forisUpdatingInSameChannel()to match the other predicates.
Keeps the API consistent and reduces accidental misuse.-private function isUpdatingInSameChannel($newChannelId, ?string $oldChannelId): bool +private function isUpdatingInSameChannel(?string $newChannelId, ?string $oldChannelId): bool { return ! is_null($newChannelId) && $oldChannelId === $newChannelId; }
📜 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 (8)
app-modules/bot-discord/src/Actions/JoiningChannelAction.php(1 hunks)app-modules/bot-discord/src/Actions/LeftChannelAction.php(1 hunks)app-modules/bot-discord/src/DTO/VoiceChannelDTO.php(2 hunks)app-modules/bot-discord/src/Events/DynamicVoiceEvent.php(2 hunks)app-modules/bot-discord/src/SlashCommands/DynamicVoiceCommand.php(0 hunks)app-modules/bot-discord/tests/Unit/Actions/JoiningChannelActionTest.php(1 hunks)app-modules/bot-discord/tests/Unit/Actions/LeftChannelActionTest.php(1 hunks)composer.json(1 hunks)
💤 Files with no reviewable changes (1)
- app-modules/bot-discord/src/SlashCommands/DynamicVoiceCommand.php
🧰 Additional context used
🧬 Code graph analysis (4)
app-modules/bot-discord/tests/Unit/Actions/JoiningChannelActionTest.php (2)
app-modules/bot-discord/src/Actions/JoiningChannelAction.php (1)
JoiningChannelAction(9-30)app-modules/bot-discord/src/DTO/VoiceChannelDTO.php (2)
VoiceChannelDTO(9-41)make(20-30)
app-modules/bot-discord/src/Actions/JoiningChannelAction.php (2)
app-modules/bot-discord/src/DTO/VoiceChannelDTO.php (1)
VoiceChannelDTO(9-41)app-modules/bot-discord/src/Actions/LeftChannelAction.php (2)
execute(11-23)saveActiveChannels(25-28)
app-modules/bot-discord/src/Actions/LeftChannelAction.php (2)
app-modules/bot-discord/src/DTO/VoiceChannelDTO.php (1)
VoiceChannelDTO(9-41)app-modules/bot-discord/src/Actions/JoiningChannelAction.php (2)
execute(11-24)saveActiveChannels(26-29)
app-modules/bot-discord/src/Events/DynamicVoiceEvent.php (3)
app-modules/bot-discord/src/Actions/JoiningChannelAction.php (2)
JoiningChannelAction(9-30)execute(11-24)app-modules/bot-discord/src/Actions/LeftChannelAction.php (2)
LeftChannelAction(9-29)execute(11-23)app-modules/bot-discord/src/Actions/UserCharacterResolver.php (1)
resolve(16-69)
🪛 GitHub Check: Perform Pint format / Run
app-modules/bot-discord/tests/Unit/Actions/JoiningChannelActionTest.php
[warning] 1-1:
Found violation(s) of type: ordered_imports
app-modules/bot-discord/tests/Unit/Actions/LeftChannelActionTest.php
[warning] 1-1:
Found violation(s) of type: ordered_imports
⏰ 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 (1)
composer.json (1)
56-56: Thedriftingly/rector-laravel ^2.1.7dependency is compatible with your current setup (rector ^2.2.14, Laravel 12.42.0, PHP 8.4). Compatibility is already verified through your CI pipeline that runs rector checks on every push and pull request.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
app-modules/bot-discord/tests/Unit/Actions/LeftChannelActionTest.php (4)
5-7: Import ordering issue (already flagged).This has already been identified in a previous review. The imports should be alphabetically ordered as per the project's conventions.
31-41: Cache access before assertion (already flagged).This issue was already identified in a previous review. The test accesses
$cachedChannels[0]on line 34 before asserting the cache is non-empty on line 35, which can cause a fatal error if the cache write fails.
66-69: Cache access before assertion (already flagged).This issue was already identified in a previous review. The test accesses
$cachedChannels[0]on line 69 before asserting the cache is non-empty on line 70.
70-76: Assert array size before accessing index.Line 76 accesses
$activeChannelFromCache->users[0]without first asserting that the users array contains exactly one element. WhileusersCountis checked to be 1, adding an explicit array size assertion would catch potential inconsistencies betweenusersCountand the actual array length.Apply this diff:
expect($cachedChannels)->not->toBeEmpty() ->and($activeChannelFromCache)->toBeInstanceOf(VoiceChannelDTO::class) ->and($activeChannelFromCache->guildId)->toBe($activeChannel->guildId) ->and($activeChannelFromCache->channelId)->toBe($activeChannel->channelId) ->and($activeChannelFromCache->usersCount)->toBe(1) ->and($activeChannelFromCache->lastJoinedAt)->not->toBeNull() + ->and($activeChannelFromCache->users)->toHaveCount(1) ->and($activeChannelFromCache->users[0])->toBe('fuedase-123');
🧹 Nitpick comments (1)
app-modules/bot-discord/tests/Unit/Actions/LeftChannelActionTest.php (1)
23-23: Use camelCase convention for variable naming.The variable
$activechannelsshould follow camelCase convention and be renamed to$activeChannelsfor consistency with PHP and PSR-12 standards.Apply this diff:
- $activechannels[] = $activeChannel; + $activeChannels[] = $activeChannel; $action = new LeftChannelAction(); $action->execute( - activeChannels: $activechannels, + activeChannels: $activeChannels, user: $userId );Apply the same change in the second test (lines 58, 62).
Also applies to: 27-27, 58-58, 62-62
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app-modules/bot-discord/tests/Unit/Actions/JoiningChannelActionTest.php(1 hunks)app-modules/bot-discord/tests/Unit/Actions/LeftChannelActionTest.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app-modules/bot-discord/tests/Unit/Actions/JoiningChannelActionTest.php
🧰 Additional context used
🧬 Code graph analysis (1)
app-modules/bot-discord/tests/Unit/Actions/LeftChannelActionTest.php (2)
app-modules/bot-discord/src/Actions/LeftChannelAction.php (1)
LeftChannelAction(9-29)app-modules/bot-discord/src/DTO/VoiceChannelDTO.php (2)
VoiceChannelDTO(9-41)make(20-30)
⏰ 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
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app-modules/bot-discord/src/Actions/VoiceChannel/JoiningChannelAction.php (1)
11-23: Guard cached entry type + prevent duplicate users /usersCountdrift.
This is the same issue previously flagged: a non-VoiceChannelDTOentry (or duplicate join events) can break invariants.foreach ($activeChannels as $index => $channel) { - /** @var VoiceChannelDTO $channel */ - if (isset($channel->channelId) && $channel->channelId === $channelId) { - $activeChannels[$index]->users[] = $user; - $activeChannels[$index]->usersCount++; + if (! $channel instanceof VoiceChannelDTO) { + continue; + } + + if ($channel->channelId === $channelId) { + if (! in_array($user, $activeChannels[$index]->users, true)) { + $activeChannels[$index]->users[] = $user; + } + $activeChannels[$index]->usersCount = count($activeChannels[$index]->users); $activeChannels[$index]->lastJoinedAt = now(); $this->saveActiveChannels($activeChannels); break; } }app-modules/bot-discord/src/Actions/VoiceChannel/LeftChannelAction.php (1)
11-22: Guard cached entry type + keepusersCountconsistent with actual removals.
Same previously flagged issue: duplicates can causeusersCountto desync (filter removes N, decrement removes 1), and malformed entries can error.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--; + if (! $channel instanceof VoiceChannelDTO) { + continue; + } + + if (! in_array($user, $channel->users, true)) { + continue; + } + + $filtered = array_values(array_filter( + $channel->users, + static fn ($userId): bool => $userId !== $user + )); + + $activeChannels[$index]->users = $filtered; + $activeChannels[$index]->usersCount = count($filtered); $this->saveActiveChannels($activeChannels); break; } }
🧹 Nitpick comments (4)
app-modules/bot-discord/tests/Unit/Actions/VoiceChannel/JoiningChannelActionTest.php (1)
9-40: Make the test isolated + resilient to cache driver differences (tag support / serialization).
Right now the test assumes a cleanvoice_channelstag namespace and may behave differently depending on whether the cache store serializes values or supports tags.it('should insert user into channel cache', function (): void { + cache()->tags(['voice_channels'])->flush(); + $channelId = 'channel-123-id'; $userId = Uuid::uuid4()->toString(); @@ - $activechannels[] = $activeChannel; + $activeChannels = [$activeChannel]; $action = new JoiningChannelAction(); @@ channelId: $channelId, - activeChannels: $activechannels, + activeChannels: $activeChannels, user: $userId );Also please verify
ramsey/uuidis available in the test environment (dev dependency) and that the configured cache store supportstags().app-modules/bot-discord/tests/Unit/Actions/VoiceChannel/LeftChannelActionTest.php (1)
9-42: Flush tagged cache between tests; fix minor naming/text.
Without a flush, previous tests can leakactive_voice_channels_keysinto later runs. Also consider renaming$activechannelsfor readability and fixing the test description string.-it('should remove user from channel when he lefts the channel', function (): void { +it('should remove user from channel when he leaves the channel', function (): void { + cache()->tags(['voice_channels'])->flush(); @@ - $activechannels[] = $activeChannel; + $activeChannels = [$activeChannel]; @@ $action->execute( - activeChannels: $activechannels, + activeChannels: $activeChannels, user: $userId ); }); + it('should be able to remove user from channel when have multiple users', function (): void { + cache()->tags(['voice_channels'])->flush(); @@ - $activechannels[] = $activeChannel; + $activeChannels = [$activeChannel]; @@ $action->execute( - activeChannels: $activechannels, + activeChannels: $activeChannels, user: $userId );Please also verify the cache store used in tests supports
tags().Also applies to: 43-77
app-modules/bot-discord/src/Events/DynamicVoiceEvent.php (1)
25-36: Delegation looks correct; consider minor log formatting.
Maybe add a space afterChannel Members:for readability.-$this->logger()->info('Channel Members:'.($state->channel?->members?->count() ?? 0)); +$this->logger()->info('Channel Members: '.($state->channel?->members?->count() ?? 0));app-modules/bot-discord/tests/Feature/Actions/VoiceChannel/HandleStateChannelActionTest.php (1)
9-33: Clearvoice_trackingin setup; re-fetch cache after state transitions.
This avoids cross-test pollution and makes assertions independent of whether the cache store returns shared object instances vs serialized copies.beforeEach(function (): void { + cache()->tags(['voice_channels'])->flush(); + cache()->tags(['voice_tracking'])->flush(); + $this->userId = Uuid::uuid4()->toString(); @@ cache()->tags(['voice_channels'])->put('active_voice_channels_keys', $activeChannels); }); @@ // user moves from first channel to second $action->execute($this->userId, $this->secondChannelId); + + $cachedChannels = cache()->tags(['voice_channels'])->get('active_voice_channels_keys'); + $firstChannelFromCache = $cachedChannels[0]; + $secondChannelFromCache = $cachedChannels[1];Please verify the feature test environment uses a cache driver that supports
tags().Also applies to: 54-105
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app-modules/bot-discord/src/Actions/VoiceChannel/HandleStateChannelAction.php(1 hunks)app-modules/bot-discord/src/Actions/VoiceChannel/JoiningChannelAction.php(1 hunks)app-modules/bot-discord/src/Actions/VoiceChannel/LeftChannelAction.php(1 hunks)app-modules/bot-discord/src/Events/DynamicVoiceEvent.php(2 hunks)app-modules/bot-discord/tests/Feature/Actions/VoiceChannel/HandleStateChannelActionTest.php(1 hunks)app-modules/bot-discord/tests/Unit/Actions/VoiceChannel/JoiningChannelActionTest.php(1 hunks)app-modules/bot-discord/tests/Unit/Actions/VoiceChannel/LeftChannelActionTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
app-modules/bot-discord/src/Actions/VoiceChannel/LeftChannelAction.php (3)
app-modules/bot-discord/src/DTO/VoiceChannelDTO.php (1)
VoiceChannelDTO(9-41)app-modules/bot-discord/src/Actions/VoiceChannel/HandleStateChannelAction.php (1)
execute(9-58)app-modules/bot-discord/src/Actions/VoiceChannel/JoiningChannelAction.php (2)
execute(11-24)saveActiveChannels(26-29)
app-modules/bot-discord/src/Events/DynamicVoiceEvent.php (3)
app-modules/bot-discord/src/Actions/VoiceChannel/HandleStateChannelAction.php (2)
HandleStateChannelAction(7-97)execute(9-58)app-modules/bot-discord/src/Actions/VoiceChannel/JoiningChannelAction.php (1)
execute(11-24)app-modules/bot-discord/src/Actions/VoiceChannel/LeftChannelAction.php (1)
execute(11-23)
app-modules/bot-discord/tests/Unit/Actions/VoiceChannel/JoiningChannelActionTest.php (4)
app-modules/bot-discord/src/Actions/VoiceChannel/JoiningChannelAction.php (2)
JoiningChannelAction(9-30)execute(11-24)app-modules/bot-discord/src/DTO/VoiceChannelDTO.php (1)
VoiceChannelDTO(9-41)app-modules/bot-discord/src/Actions/VoiceChannel/HandleStateChannelAction.php (1)
execute(9-58)app-modules/bot-discord/src/Actions/VoiceChannel/LeftChannelAction.php (1)
execute(11-23)
app-modules/bot-discord/tests/Unit/Actions/VoiceChannel/LeftChannelActionTest.php (4)
app-modules/bot-discord/src/Actions/VoiceChannel/LeftChannelAction.php (2)
LeftChannelAction(9-29)execute(11-23)app-modules/bot-discord/src/DTO/VoiceChannelDTO.php (1)
VoiceChannelDTO(9-41)app-modules/bot-discord/src/Actions/VoiceChannel/HandleStateChannelAction.php (1)
execute(9-58)app-modules/bot-discord/src/Actions/VoiceChannel/JoiningChannelAction.php (1)
execute(11-24)
app-modules/bot-discord/src/Actions/VoiceChannel/HandleStateChannelAction.php (2)
app-modules/bot-discord/src/Actions/VoiceChannel/JoiningChannelAction.php (2)
execute(11-24)JoiningChannelAction(9-30)app-modules/bot-discord/src/Actions/VoiceChannel/LeftChannelAction.php (2)
execute(11-23)LeftChannelAction(9-29)
app-modules/bot-discord/src/Actions/VoiceChannel/JoiningChannelAction.php (3)
app-modules/bot-discord/src/DTO/VoiceChannelDTO.php (1)
VoiceChannelDTO(9-41)app-modules/bot-discord/src/Actions/VoiceChannel/HandleStateChannelAction.php (1)
execute(9-58)app-modules/bot-discord/src/Actions/VoiceChannel/LeftChannelAction.php (2)
execute(11-23)saveActiveChannels(25-28)
⏰ 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
gvieira18
previously approved these changes
Dec 12, 2025
There was a problem hiding this comment.
Actionable comments posted: 3
📜 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 (2)
app-modules/bot-discord/src/Actions/VoiceChannel/DeleteVoiceChannelAction.php(1 hunks)app-modules/bot-discord/src/Tasks/DynamicVoiceTask.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app-modules/bot-discord/src/Actions/VoiceChannel/DeleteVoiceChannelAction.php (1)
app-modules/bot-discord/src/DTO/VoiceChannelDTO.php (2)
VoiceChannelDTO(9-41)isLongTermEmpty(37-40)
app-modules/bot-discord/src/Tasks/DynamicVoiceTask.php (1)
app-modules/bot-discord/src/Actions/VoiceChannel/DeleteVoiceChannelAction.php (2)
DeleteVoiceChannelAction(10-43)execute(12-21)
⏰ 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 (1)
app-modules/bot-discord/src/Tasks/DynamicVoiceTask.php (1)
25-28: LGTM! Clean delegation to action class.The refactor successfully extracts voice channel cleanup logic into a dedicated action class, improving separation of concerns and testability.
1pride
approved these changes
Dec 16, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Refactor
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.