feat: base discord bot#126
Conversation
WalkthroughAdds a new bot-discord module (commands, slash commands, events, tasks, provider resolution), refactors message and profile flows to use resolver DTOs, updates DTOs and actions, adjusts tests to skip some cases, updates root composer and env, and adds .laracord to .gitignore. Changes
Sequence Diagram(s)sequenceDiagram
participant Discord as Discord API
participant Event as MessageReceivedEvent
participant Resolver as UserCharacterResolver
participant NewMsg as NewMessage
participant Persist as PersistMessage
Discord->>Event: MESSAGE_CREATE (Message)
Event->>Event: ignore bot messages
Event->>Resolver: resolve(provider, providerId, username, tenantId)
alt existing linkage
Resolver-->>Event: ResolvedUserCharacter(existing)
else new linkage
Resolver-->>Event: ResolvedUserCharacter(new)
end
Event->>NewMsg: handle(NewMessageDTO)
NewMsg->>NewMsg: compute obtainedExperience from Character
NewMsg->>Persist: persist(NewMessageDTO with obtainedExperience, providerId)
Persist-->>NewMsg: persisted
NewMsg-->>Event: done
sequenceDiagram
participant User
participant Slash as IntroductionCommand
participant Modal as Discord Modal
participant Resolver as UserCharacterResolver
participant Update as UpdateProfile
User->>Slash: /introduction
Slash->>Modal: show modal (name,nickname,github,linkedin,about)
Modal-->>Slash: submit components
Slash->>Resolver: resolve(provider, providerId, username, tenantId)
Resolver-->>Slash: ResolvedUserCharacter
Slash->>Update: handle(UpdateProfileDTO)
Update-->>Slash: success
Slash->>Discord: reply confirmation + assign role
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (3)
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: 2
🧹 Nitpick comments (11)
app-modules/message/src/DTO/NewMessageDTO.php (1)
12-21: New requiredproviderUsernamefield may break existingmake()callersYou’ve added a required
public string $providerUsernameand now unconditionally read$payload['provider_username']inmake(). Any existing callers that build the payload without this key will now error at runtime.Consider either:
- Verifying all producers of this payload now include
provider_username, or- Providing a safe fallback / validation when the key is missing (e.g., explicit assertion or default).
Also applies to: 23-35
app/Console/Commands/GenerateDiscordTenant.php (1)
18-45: Make tenant generation idempotent and improve command descriptionTwo points here:
- Idempotency / duplicates
Tenant::factory()->afterCreating(...)->create();(Lines 32–42) will always create a new Tenant and associated Provider for the same{guildId}. If someone runs the command twice for the same guild, you’ll end up with multiple Tenants and multipleProviderrows, whileMessageReceivedEventlater does afirstOrFail()lookup bymodel_typeandprovider_idonly.Consider guarding against this by checking for an existing provider for the guild before creating a new tenant, e.g.:
use He4rt\Provider\Enums\ProviderEnum; +use He4rt\Provider\Models\Provider; use He4rt\Tenant\Models\Tenant; @@ public function handle(): void { + $guildId = (string) $this->argument('guildId'); + + $exists = Provider::query() + ->where('model_type', Tenant::class) + ->where('provider', ProviderEnum::Discord) + ->where('provider_id', $guildId) + ->exists(); + + if ($exists) { + $this->warn('A tenant/provider for this guild already exists.'); + return; + } + Tenant::factory() - ->afterCreating(function (Tenant $tenant): void { + ->afterCreating(function (Tenant $tenant) use ($guildId): void { $tenant ->providers() ->create([ 'tenant_id' => $tenant->getKey(), 'provider' => ProviderEnum::Discord, - 'provider_id' => $this->argument('guildId'), + 'provider_id' => $guildId, ]); }) ->create();
- Command description
protected $description = 'Command description';(Line 25) is still a placeholder. Updating it to something like “Generate a Tenant and Discord provider binding for a guild ID” will makephp artisan listmore useful.composer.json (1)
18-20: Globalminimum-stability: dev+ dev-next framework may affect production stabilityYou’ve introduced
laracord/framework: "dev-next"and switched the project-wide"minimum-stability"to"dev"(Line 157). Together, this allows Composer to pull dev versions of dependencies more freely.That’s fine if:
- This app is expected to run against bleeding-edge packages, and
- You’re comfortable with the risk that a transitive dependency update could introduce breaking changes.
If this is intended for production, consider whether you can instead:
- Keep
"minimum-stability": "stable"and allow dev versions only where explicitly needed (e.g., via explicitdev-constraints/aliases), and/or- Pin
laracord/framework(and other key libs) to a known-good commit/tag.Please double-check that this relaxation of stability is intentional and aligns with your deployment expectations.
Also applies to: 36-40, 47-47, 51-69, 157-170
app-modules/bot-discord/src/Events/MessageReceivedEvent.php (1)
25-57: Tighten guild handling, provider lookup, and static-analysis suppressionA few targeted improvements here:
- Skip direct messages early
For non-guild messages
guild_idis typicallynull. Right now this yields an empty-string lookup and afirstOrFail()exception on every DM, which then gets logged as an error. Consider short‑circuiting when there is no guild:public function handle(Message $message, Discord $discord): void { if ($message->author->bot) { return; } + + // Ignore direct messages (no guild context). + if ($message->guild_id === null) { + return; + }
- Constrain tenant provider lookup to Discord
To avoid future collisions if another provider type ever writes Tenant providers with the same
provider_id, you can also filter by theproviderenum:- try { - $tenantProvider = Provider::query() - ->where('model_type', Tenant::class) - ->where('provider_id', (string) $message->guild_id) - ->firstOrFail(); + try { + $tenantProvider = Provider::query() + ->where('model_type', Tenant::class) + ->where('provider', ProviderEnum::Discord) + ->where('provider_id', (string) $message->guild_id) + ->firstOrFail();
- Suppress unused
$discordparameter warningSince the method signature is likely defined by Laracord, it’s reasonable that
$discordis unused. Instead of changing the signature, you can suppress PHPMD for this method:- /** - * Handle the event. - */ - public function handle(Message $message, Discord $discord): void + /** + * Handle the event. + * + * @SuppressWarnings(PHPMD.UnusedFormalParameter) + */ + public function handle(Message $message, Discord $discord): void
- Logging verbosity
The
sprintfcurrently logs the full trace for every failure (Line 55). That’s great for debugging, but in high‑traffic or production environments it can be noisy and may expose internal details. Depending on your logging policy, you might later want to either:
- Log a shorter message to the main logger and send full traces only to a debug channel, or
- Downgrade to
debug()for traces and keeperror()for the message + basic context.app-modules/user/src/Actions/UpdateProfile.php (2)
14-21: Consider scoping provider lookup by tenant and using DTO’s tenantId
UpdateProfileDTOcarriestenantId, but the lookup only filters bymodel_type,providerandprovider_id. In a multi-tenant setup, the sameprovider_idcould exist in more than one tenant and update the wrong user.If
providershas atenant_id(as used elsewhere withTenant::class), it’s safer to include it and align with the DTO:- $provider = Provider::query() - ->where('model_type', User::class) - ->where('provider', $profileDTO->provider) - ->where('provider_id', $profileDTO->providerId) - ->first(); + $provider = Provider::query() + ->where('tenant_id', $profileDTO->tenantId) + ->where('model_type', User::class) + ->where('provider', $profileDTO->provider) + ->where('provider_id', $profileDTO->providerId) + ->first();If
tenant_idis intentionally not relevant here, consider droppingtenantIdfrom the DTO to avoid confusion.
23-29: Harden logging and relation access for missing user/informationRight now you only log a generic
"Provider not found"and then assume$provider->userand itsinformation()relation are always present. Any data inconsistency on the provider’smodel_idcould trigger a fatal error here.Two small robustness tweaks:
- Log with context to help debugging.
- Guard against a missing user before updating.
- if (! $provider) { - Log::error('Provider not found'); - - return; - } - - $provider->user->information()->update($profileDTO->toProfile()); + if (! $provider) { + Log::error('Provider not found', [ + 'provider' => $profileDTO->provider->name ?? null, + 'provider_id' => $profileDTO->providerId, + 'tenant_id' => $profileDTO->tenantId, + ]); + + return; + } + + if (! $provider->user) { + Log::error('Provider has no related user', [ + 'provider_id' => $provider->id, + ]); + + return; + } + + $provider->user->information()->update($profileDTO->toProfile());app-modules/bot-discord/src/Commands/PingCommand.php (1)
44-52: Address unused$argsparameter while keeping Laracord signature
$argsis unused and PHPMD complains, but the signature likely must stay aligned withLaracord\Commands\Command::handle(Message $message, array $args).You can silence the warning without changing behavior, e.g.:
- public function handle(Message $message, array $args): void + /** + * Handle the command. + * + * @SuppressWarnings(PHPMD.UnusedFormalParameter) + */ + public function handle(Message $message, array $args): void { - $this + // $args intentionally unused + $this ->message() ->title('Ping Command') ->content('Hello world!') ->button('👋', route: 'wave') ->reply($message); }Alternatively, you could rename it to
array $_argsto signal intentional non-use.app-modules/bot-discord/composer.json (1)
1-22: Optional: enrich package metadata and platform constraintsThe manifest is valid as-is, but a few small tweaks would improve clarity and tooling support:
- Add a meaningful
"description"so Composer/Packagist consumers know what this module is.- Consider adding
"php"(and possibly"laravel/framework"/"laracord/*") constraints in"require"to document the supported runtime.- If this package is only ever used inside the monorepo, you can still add those constraints to help static analysis and future extraction.
No functional change required now; just something to consider.
app-modules/user/src/DTO/UpdateProfileDTO.php (2)
23-35: MakefromPayloadmore defensive, especially for optional fields
fromPayloadassumes all keys exist; any missing key will trigger notices or type issues, even for nullable properties. If payloads come from external sources (Discord interactions, HTTP), it’s safer to tolerate missing optional fields:public static function fromPayload(array $payload): self { return new self( - providerId: $payload['provider_id'], - provider: $payload['provider'], - tenantId: $payload['tenant_id'], - name: $payload['name'], - nickname: $payload['nickname'], - linkedinUrl: $payload['linkedin_url'], - githubUrl: $payload['github_url'], - birthdate: $payload['birthdate'], - about: $payload['about'] + providerId: $payload['provider_id'], + provider: $payload['provider'], + tenantId: $payload['tenant_id'], + name: $payload['name'] ?? null, + nickname: $payload['nickname'] ?? null, + linkedinUrl: $payload['linkedin_url'] ?? null, + githubUrl: $payload['github_url'] ?? null, + birthdate: $payload['birthdate'] ?? null, + about: $payload['about'] ?? null, ); }If you rely on some of them being mandatory, you can assert/validate explicitly instead of relying on array indexes.
11-20: Ensure$payload['provider']is aProviderEnum(or cast it here)The constructor typehints
ProviderEnum $provider, butfromPayloadpasses$payload['provider']directly. If callers ever pass the raw scalar (e.g.'discord') instead of an enum instance, this will fail at runtime.Either document that
fromPayloadexpects aProviderEnuminstance inprovider, or normalize here:- provider: $payload['provider'], + provider: $payload['provider'] instanceof ProviderEnum + ? $payload['provider'] + : ProviderEnum::from($payload['provider']),This keeps the DTO boundary strict while being resilient to string/int payloads.
app-modules/bot-discord/src/Events/GreetingsEvent.php (1)
24-48: Silence unused$discordparameter or use it for contextThe
handle(Message $message, Discord $discord)signature matches the event contract, but$discordisn’t used, hence the PHPMD warning.Two options:
- Explicitly mark the parameter as intentionally unused:
- public function handle(Message $message, Discord $discord): void + /** + * Handle the event. + * + * @SuppressWarnings(PHPMD.UnusedFormalParameter) + */ + public function handle(Message $message, Discord $discord): void
- Or rename to signal that it’s unused:
- public function handle(Message $message, Discord $discord): void + public function handle(Message $message, Discord $_discord): voidFunctionally nothing changes; it just keeps static analysis clean.
📜 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 (16)
.gitignore(1 hunks)app-modules/bot-discord/composer.json(1 hunks)app-modules/bot-discord/src/Commands/PingCommand.php(1 hunks)app-modules/bot-discord/src/Events/GreetingsEvent.php(1 hunks)app-modules/bot-discord/src/Events/MessageReceivedEvent.php(1 hunks)app-modules/bot-discord/src/Providers/BotDiscordServiceProvider.php(1 hunks)app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php(1 hunks)app-modules/character/src/Actions/FindCharacterIdByUserId.php(1 hunks)app-modules/message/src/Actions/NewMessage.php(1 hunks)app-modules/message/src/DTO/NewMessageDTO.php(2 hunks)app-modules/provider/src/Models/Provider.php(1 hunks)app-modules/user/src/Actions/UpdateProfile.php(1 hunks)app-modules/user/src/DTO/UpdateProfileDTO.php(1 hunks)app/Console/Commands/GenerateDiscordTenant.php(1 hunks)bootstrap/providers.php(2 hunks)composer.json(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
app-modules/bot-discord/src/Providers/BotDiscordServiceProvider.php (4)
app-modules/bot-discord/src/Commands/PingCommand.php (1)
PingCommand(11-63)app-modules/bot-discord/src/Events/GreetingsEvent.php (1)
GreetingsEvent(12-49)app-modules/bot-discord/src/Events/MessageReceivedEvent.php (1)
MessageReceivedEvent(18-60)app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php (1)
IntroductionCommand(19-181)
app-modules/bot-discord/src/Events/MessageReceivedEvent.php (5)
app-modules/message/src/Actions/NewMessage.php (2)
NewMessage(15-72)persist(22-71)app-modules/message/src/DTO/NewMessageDTO.php (1)
NewMessageDTO(10-36)app-modules/provider/src/Models/Provider.php (1)
Provider(28-94)app-modules/tenant/src/Models/Tenant.php (1)
Tenant(23-103)app-modules/bot-discord/src/Providers/BotDiscordServiceProvider.php (1)
bot(16-25)
app-modules/bot-discord/src/Commands/PingCommand.php (1)
app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php (1)
handle(66-114)
app/Console/Commands/GenerateDiscordTenant.php (2)
app-modules/provider/src/Models/Provider.php (1)
Provider(28-94)app-modules/tenant/src/Models/Tenant.php (1)
Tenant(23-103)
app-modules/user/src/DTO/UpdateProfileDTO.php (1)
app-modules/provider/src/Models/Provider.php (1)
Provider(28-94)
app-modules/user/src/Actions/UpdateProfile.php (4)
app-modules/provider/src/Models/Provider.php (2)
Provider(28-94)user(57-60)app-modules/user/src/DTO/UpdateProfileDTO.php (2)
UpdateProfileDTO(9-49)toProfile(38-48)app-modules/user/src/Models/Information.php (1)
user(34-37)app-modules/user/src/Models/User.php (1)
information(82-85)
app-modules/bot-discord/src/Events/GreetingsEvent.php (2)
app-modules/bot-discord/src/Events/MessageReceivedEvent.php (1)
handle(30-59)app-modules/bot-discord/src/Providers/BotDiscordServiceProvider.php (1)
bot(16-25)
🪛 PHPMD (2.15.0)
app-modules/bot-discord/src/Events/MessageReceivedEvent.php
30-30: Avoid unused parameters such as '$discord'. (undefined)
(UnusedFormalParameter)
app-modules/bot-discord/src/Commands/PingCommand.php
44-44: Avoid unused parameters such as '$args'. (undefined)
(UnusedFormalParameter)
app-modules/bot-discord/src/Events/GreetingsEvent.php
24-24: Avoid unused parameters such as '$discord'. (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). (2)
- GitHub Check: Perform Pest Tests / Run
- GitHub Check: Perform Rector Check / Run
🔇 Additional comments (7)
app-modules/character/src/Actions/FindCharacterIdByUserId.php (2)
12-15: Based on my verification, I have found critical issues with the original review comment. Let me provide the rewritten version:
The review comment contains incorrect assumptions that do not match the codebase reality.
The two main concerns in the original comment are unfounded:
Caching removal claim is unsupported: There is no evidence of caching in the current code or in the git history. The file contains only one commit ("wip") and the HEAD version is identical to the current implementation—both lack any caching logic. The claim that "caching logic has been removed" cannot be verified.
Discord bot context concern is invalid:
FindCharacterIdByUserIdis only invoked from HTTP controllers (CharactersController,NewVoiceMessageviaMessagesController). It is never called from Discord bot event handlers. The Discord bot passestenantIdexplicitly via DTO objects (as seen inMessageReceivedEventandNewMessage), sorequest()->tenant_idis available in all actual execution paths.No verification of call frequency is needed since the concerns do not reflect actual usage patterns.
20-20: Based on my verification, I found that the original review comment's concern is not applicable to this PR. Here's what I discovered:
Discord bot does NOT call
FindCharacterIdByUserId: The Discord bot's event handlers (MessageReceivedEvent,IntroductionCommand) use different actions likeNewMessageandUpdateProfile.Discord bot correctly passes tenant_id: When
NewMessageis called from Discord context, tenant_id is explicitly passed as a parameter throughNewMessageDTO, not obtained fromrequest()->tenant_id.FindCharacterIdByUserId is only used in HTTP contexts: The three actions that depend on it (
NewVoiceMessage,ClaimDailyBonus,ClaimCharacterBadge) are not called from Discord bot code and are only used in HTTP-based message and character modules.
request()->tenant_idon line 20 is not used by Discord bot integration.This action is only invoked from HTTP contexts within the message and character modules, where an HTTP request context is always available. Discord bot handlers use different code paths that properly pass
tenant_idas parameters. No changes are required..gitignore (1)
43-43: Ignoring.laracorddirectory is appropriateThis keeps Laracord’s local artifacts out of version control and is consistent with the other framework/IDE ignores already present.
app-modules/provider/src/Models/Provider.php (1)
19-27: Docblock update fortenant_idmatches existing model stateAdding
@property int $tenant_idaligns the PHPDoc with the actual$fillablefields onProviderand improves IDE/static-analysis support without changing runtime behavior.app-modules/bot-discord/src/Providers/BotDiscordServiceProvider.php (1)
14-25: Clear, cohesive registration of Discord events and commandsThe
bot()method cleanly wires events and commands in one fluent chain, which keeps the Discord bot configuration centralized and readable. No issues spotted here.bootstrap/providers.php (1)
5-7: DiscordServiceProvider registration fits the existing bootstrap patternImporting and appending
DiscordServiceProvider::classto the providers array integrates the Discord stack cleanly with the existing Laravel bootstrap flow.Also applies to: 16-27
app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php (1)
69-109: I'll verify the concerns raised in this review comment about thesetValue()calls with sample data andfake()usage in the modal. Let me start by examining the actual code and understanding the DiscordPHP API behavior.
<function_calls>
#!/bin/bashFirst, let's locate and examine the IntroductionCommand.php file
find . -type f -name "IntroductionCommand.php" | head -5
</function_calls>
<function_calls>
#!/bin/bashRead the IntroductionCommand.php file to see the complete context
cat -n app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php | head -150
</function_calls>
<function_calls>
DiscordPHP TextInput setValue placeholder behavior modal form
</function_calls>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
app-modules/bot-discord/src/Tasks/DynamicVoiceTask.php (1)
9-27: Placeholder task needs implementation.The class name
DynamicVoiceTasksuggests voice channel management functionality, but the current implementation only logs "Hello world." Additionally, the 5-second interval is aggressive for a placeholder that may create log noise in production.Consider either:
- Implementing the actual dynamic voice logic
- Increasing the interval or disabling the task until ready
- Adding a TODO comment documenting the intended behavior
Would you like me to open an issue to track the implementation of the dynamic voice channel functionality?
app-modules/bot-discord/src/SlashCommands/ProfileCommand.php (1)
24-24: Improve the user-facing command description.The description "The Profile Command slash command." is a placeholder that will be visible to users in Discord's command picker. Consider a more descriptive message like "View your He4rt profile" or similar.
- protected $description = 'The Profile Command slash command.'; + protected $description = 'View your He4rt community profile.';app-modules/bot-discord/src/SlashCommands/DynamicVoiceCommand.php (1)
24-24: Improve the user-facing command description.Similar to other commands, the description "The Dynamic Voice Command slash command." is placeholder text visible to users. Consider describing the actual functionality.
- protected $description = 'The Dynamic Voice Command slash command.'; + protected $description = 'Create or manage dynamic voice channels.';app-modules/bot-discord/src/SlashCommands/EditProfileCommand.php (1)
24-24: Improve the user-facing command description.The description "The Edit Profile Command slash command." should be more user-friendly, e.g., "Edit your He4rt community profile."
- protected $description = 'The Edit Profile Command slash command.'; + protected $description = 'Edit your He4rt community profile.';
📜 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 (5)
app-modules/bot-discord/src/Providers/BotDiscordServiceProvider.php(1 hunks)app-modules/bot-discord/src/SlashCommands/DynamicVoiceCommand.php(1 hunks)app-modules/bot-discord/src/SlashCommands/EditProfileCommand.php(1 hunks)app-modules/bot-discord/src/SlashCommands/ProfileCommand.php(1 hunks)app-modules/bot-discord/src/Tasks/DynamicVoiceTask.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
app-modules/bot-discord/src/SlashCommands/EditProfileCommand.php (2)
app-modules/bot-discord/src/SlashCommands/DynamicVoiceCommand.php (1)
handle(57-65)app-modules/bot-discord/src/SlashCommands/ProfileCommand.php (1)
handle(57-64)
app-modules/bot-discord/src/SlashCommands/ProfileCommand.php (2)
app-modules/bot-discord/src/SlashCommands/DynamicVoiceCommand.php (1)
handle(57-65)app-modules/bot-discord/src/SlashCommands/EditProfileCommand.php (1)
handle(57-64)
app-modules/bot-discord/src/SlashCommands/DynamicVoiceCommand.php (4)
app-modules/meeting/src/Models/Meeting.php (1)
admin(71-74)app-modules/bot-discord/src/SlashCommands/EditProfileCommand.php (1)
handle(57-64)app-modules/bot-discord/src/SlashCommands/ProfileCommand.php (1)
handle(57-64)app-modules/bot-discord/src/Tasks/DynamicVoiceTask.php (1)
handle(24-27)
🔇 Additional comments (4)
app-modules/bot-discord/src/Providers/BotDiscordServiceProvider.php (1)
10-19: Clean service provider implementation.The provider correctly extends
LaracordServiceProviderand chains discovery methods for events, commands, slash commands, and tasks with properly aligned paths and namespaces.app-modules/bot-discord/src/SlashCommands/ProfileCommand.php (1)
57-64: Placeholder implementation noted.The scaffold is consistent with other slash commands in this PR. The actual profile retrieval logic will need to be implemented before release.
app-modules/bot-discord/src/SlashCommands/DynamicVoiceCommand.php (1)
67-75: Good use of interaction routing.The
interactions()method correctly demonstrates Laracord's button interaction routing pattern with the 'wave' route. This provides a good foundation for adding more complex interactions.app-modules/bot-discord/src/SlashCommands/EditProfileCommand.php (1)
57-64: Consider modal interaction for profile editing.Profile editing typically requires user input. Laracord supports modals for form-based interactions. When implementing the actual logic, consider using
->modal()to collect profile data from the user.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app-modules/bot-discord/src/SlashCommands/ProfileCommand.php (1)
80-97: Color value looks like hex, but Laracord expects decimalLaracord’s
->color()expects a decimal value (often passed as a string). The value'800080'looks like the hex color#800080(purple), but here it will be treated as the decimal 800,080, which is a completely different color.If your intent is
#800080, consider converting it to decimal:- ->color('800080') + ->color('8388736') // decimal for hex 0x800080(or use whatever brand color you actually want in decimal).
Please double‑check Laracord’s current
color()docs to confirm the expected format and adjust the value if needed.
📜 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/SlashCommands/ProfileCommand.php(1 hunks)app-modules/bot-discord/src/Tasks/DynamicVoiceTask.php(1 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/ProfileCommand.php (4)
app-modules/provider/src/Models/Provider.php (1)
Provider(28-94)app-modules/bot-discord/src/SlashCommands/EditProfileCommand.php (1)
handle(57-64)app-modules/bot-discord/src/SlashCommands/DynamicVoiceCommand.php (1)
handle(57-65)app-modules/user/src/Models/User.php (1)
information(82-85)
⏰ 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: Perform Pest Tests / Run
- GitHub Check: Perform Rector Check / Run
🔇 Additional comments (1)
app-modules/bot-discord/src/SlashCommands/ProfileCommand.php (1)
13-56: Nice, consistent SlashCommand setup and defaultsClass metadata (
$name,$description,$options,$permissions,$admin,$hidden) follows Laracord’s conventions and keeps the command self‑describing. No issues here.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app-modules/bot-discord/src/SlashCommands/DontAskCommand.php (1)
66-66: Consider externalizing the hardcoded image URL.The Discord CDN URL is hardcoded. Consider moving it to a configuration file or environment variable for easier maintenance and updates.
Example approach - add to a config file:
// config/bot-discord.php return [ 'images' => [ 'dont_ask' => env('DISCORD_DONT_ASK_IMAGE', 'https://media.discordapp.net/attachments/546151895010508827/1046092564513701909/Frame_1282_1.png'), ], ];Then reference it:
- ->imageUrl('https://media.discordapp.net/attachments/546151895010508827/1046092564513701909/Frame_1282_1.png') + ->imageUrl(config('bot-discord.images.dont_ask'))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app-modules/bot-discord/src/SlashCommands/DontAskCommand.php(1 hunks)
🔇 Additional comments (3)
app-modules/bot-discord/src/SlashCommands/DontAskCommand.php (3)
1-10: LGTM!The namespace, imports, and file structure are properly configured.
27-39: Verify the admin and hidden flag settings.The command is currently set to
$admin = falseand$hidden = false, making it accessible to all users. Given the command's moderator-oriented purpose (prompting users to provide better context), consider whether this should be restricted to admins or hidden from the command list.
41-53: LGTM!The command options are properly configured with appropriate type and requirement settings.
…o feat/discord-bot # Conflicts: # app-modules/bot-discord/src/Tasks/DynamicVoiceTask.php
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)
app-modules/user/src/Http/Controllers/UsersController.php (1)
44-52: Type mismatch:UpdateProfile::handle()now expectsUpdateProfileDTO, but controller passesstring
UpdateProfile::handle()is typed ashandle(UpdateProfileDTO $profileDTO): void(seeapp-modules/user/src/Actions/UpdateProfile.php), butputProfile()currently calls:$action->handle($value);Here
$valueis declared asstring, so this will trigger aTypeErrorat runtime and the new DTO-based flow is never used. TheUpdateProfileRequest $requestis also unused.You should construct an
UpdateProfileDTOfrom the route param + validated request data and pass that DTO intoUpdateProfile::handle(), e.g.:// Pseudocode – use the actual factory/constructor on UpdateProfileDTO $profileDTO = /* build UpdateProfileDTO from $request->validated() and $value */; $action->handle($profileDTO);This will align the controller with the refactored action and avoid runtime type errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.env.example(1 hunks)app-modules/bot-discord/src/SlashCommands/EditProfileCommand.php(1 hunks)app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php(1 hunks)app-modules/user/src/Http/Controllers/UsersController.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php
🧰 Additional context used
🧬 Code graph analysis (2)
app-modules/user/src/Http/Controllers/UsersController.php (1)
app-modules/user/src/Actions/UpdateProfile.php (1)
handle(14-30)
app-modules/bot-discord/src/SlashCommands/EditProfileCommand.php (4)
app-modules/provider/src/Models/Provider.php (1)
Provider(28-94)app-modules/user/src/Actions/UpdateProfile.php (1)
UpdateProfile(12-31)app-modules/user/src/DTO/UpdateProfileDTO.php (2)
UpdateProfileDTO(9-49)fromPayload(23-36)app-modules/user/src/Models/User.php (1)
information(82-85)
🔇 Additional comments (5)
.env.example (1)
77-77: Based on my verification, I can now provide the rewritten review comment:
DISCORD_TOKEN is not a duplicate of HE4RT_DISCORD_BOT_KEY—they serve different purposes.
DISCORD_TOKEN(line 77) is intended for the Laracord framework, whileHE4RT_DISCORD_BOT_KEY(line 63) is used by the existing bot configuration inconfig/he4rt.php. However,DISCORD_TOKENis not referenced anywhere in the codebase, which suggests either:
- The Laracord framework reads it automatically during service provider initialization, or
- The PR setup is incomplete.
Consider adding a clarifying comment in
.env.exampleaboveDISCORD_TOKEN=to document that it's intended for the Laracord Discord bot framework, distinguishing it from the legacyHE4RT_DISCORD_BOT_KEY.Alternatively, align the naming convention (e.g.,
LARACORD_DISCORD_TOKENorHE4RT_DISCORD_BOT_TOKEN) to make the distinction clearer for future maintainers.app-modules/bot-discord/src/SlashCommands/EditProfileCommand.php (4)
169-169: Potential null value: guild icon may benullif the guild has no custom icon.Consider handling this gracefully to avoid unexpected behavior.
- ->footerIcon($interaction->guild->icon) + ->footerIcon($interaction->guild->icon ?? '')
66-80: LGTM!The provider lookup and early return pattern with a helpful message directing users to
/introductionis well-implemented.
84-129: Modal implementation looks good.The modal is well-structured with appropriate validation constraints (min/max lengths, required flags) and prefills values from the existing profile.
137-179: Solid error handling and response pattern.The try-catch with logging and the dual response (public embed + ephemeral confirmation) provides good UX. Minor issues noted separately.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app-modules/bot-discord/src/SlashCommands/AbstractSlashCommand.php(1 hunks)app-modules/bot-discord/src/SlashCommands/ProfileCommand.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app-modules/bot-discord/src/SlashCommands/ProfileCommand.php
🧰 Additional context used
🧬 Code graph analysis (1)
app-modules/bot-discord/src/SlashCommands/AbstractSlashCommand.php (3)
app-modules/provider/src/Models/Provider.php (1)
Provider(28-94)app-modules/tenant/src/Models/Tenant.php (1)
Tenant(23-103)app-modules/bot-discord/src/Providers/BotDiscordServiceProvider.php (1)
bot(12-19)
🔇 Additional comments (2)
app-modules/bot-discord/src/SlashCommands/AbstractSlashCommand.php (2)
18-20: LGTM!Nullable provider properties are appropriately typed for storing resolved context.
22-38: LGTM!The pipeline setup correctly integrates with Laracord's middleware system and passes the interaction context through.
…haracter handling
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (7)
app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php (4)
94-100: Confirm “Linkedisney” label is intentionalThe label
"Linkedisney (Opcional)"looks like a typo for LinkedIn; if this isn’t a deliberate in‑community joke, consider renaming to avoid confusion (and keep it consistent withEditProfileCommand).
135-138: Provider lookup should be constrained by provider type (Discord)The tenant provider query only filters by
model_typeandprovider_id. If other providers ever share the sameprovider_id(e.g. multi‑provider setup), this can become ambiguous. AbstractSlashCommand’sbeforePipeline()already uses aproviderfilter (ProviderEnum::Discord); mirroring that here would make the lookup deterministic.use He4rt\Provider\Models\Provider; +use He4rt\Provider\Enums\ProviderEnum; @@ - $tenantProvider = Provider::query() - ->where('model_type', Tenant::class) - ->where('provider_id', (string) $interaction->guild_id) - ->firstOrFail(); + $tenantProvider = Provider::query() + ->where('model_type', Tenant::class) + ->where('provider', ProviderEnum::Discord) + ->where('provider_id', (string) $interaction->guild_id) + ->firstOrFail();This also keeps it consistent with
AbstractSlashCommand::beforePipeline().
120-123:respondWithMessageshould receive aMessageBuilder, not a stringThese calls pass a raw string into
Interaction::respondWithMessage, but DiscordPHP’s API expects aMessageBuilderas the first argument. This is likely to trigger aTypeErrorat runtime; your success path already usesmessage()->reply()instead.You can fix by wrapping the strings in
MessageBuilderinstances and importing it:use Discord\Builders\Components\TextInput; +use Discord\Builders\MessageBuilder; use Discord\Helpers\Collection; @@ - if (! $role) { - $interaction->respondWithMessage('Erro ao encontrar o role He4rt', true); + if (! $role) { + $interaction->respondWithMessage( + MessageBuilder::new()->setContent('Erro ao encontrar o role He4rt'), + true + ); @@ - if ($hasRole) { - $interaction->respondWithMessage('Você já apresentou!!', true); + if ($hasRole) { + $interaction->respondWithMessage( + MessageBuilder::new()->setContent('Você já apresentou!!'), + true + ); @@ - $interaction->respondWithMessage( - 'Ocorreu um erro ao processar sua apresentação. Por favor, tente novamente mais tarde.', - true - ); + $interaction->respondWithMessage( + MessageBuilder::new()->setContent( + 'Ocorreu um erro ao processar sua apresentação. Por favor, tente novamente mais tarde.' + ), + true + );What is the PHP method signature of `Discord\Parts\Interactions\Interaction::respondWithMessage` in DiscordPHP?Also applies to: 128-132, 181-184
140-149:birthdateis never collected in the modalThe payload maps:
'birthdate' => $components->get('custom_id', 'birthdate')?->value ?? null,but the modal defines no
TextInputwithcustom_id = 'birthdate', so this will always evaluate tonull. GivenUpdateProfileDTO::fromPayload()expects abirthdatekey, this is confusing and may eventually lead to accidental data clearing if this command is ever used against pre‑existing profiles.Consider either:
- Adding a
TextInputforbirthdateto the modal (and documenting expected format), or- Intentionally preserving any existing DB value (e.g. by resolving the user provider and reading their current
information->birthdateinstead of using components).app-modules/bot-discord/src/SlashCommands/EditProfileCommand.php (3)
91-105: Same “Linkedisney” label here – verify it’s not a typoThis modal also labels the LinkedIn field as
"Linkedisney (Opcional)". If that’s not an intentional joke, consider correcting it (and aligning with the rest of the UX):- TextInput::new('Linkedisney (Opcional)', TextInput::STYLE_SHORT) + TextInput::new('LinkedIn (Opcional)', TextInput::STYLE_SHORT)
61-69: WraprespondWithMessagestrings inMessageBuilderBoth the “missing introduction” guard and the catch block pass plain strings to
respondWithMessage, which likely violates DiscordPHP’sInteraction::respondWithMessage(MessageBuilder $builder, ?bool $ephemeral)signature.Import
MessageBuilderand wrap these calls, e.g.:use Discord\Builders\Components\TextInput; +use Discord\Builders\MessageBuilder; @@ - if (! $this->memberProvider?->user?->information) { - $interaction->respondWithMessage( - 'Parece que você ainda não completou sua apresentação. Use o comando `/introduction` para continuar.', - true); + if (! $this->memberProvider?->user?->information) { + $interaction->respondWithMessage( + MessageBuilder::new()->setContent( + 'Parece que você ainda não completou sua apresentação. Use o comando `/introduction` para continuar.' + ), + true + ); @@ - } catch (Throwable $throwable) { - $this->logger()->error($throwable->getMessage()); - - $interaction->respondWithMessage('Erro ao persistir dados', true); - } + } catch (Throwable $throwable) { + $this->logger()->error($throwable->getMessage()); + + $interaction->respondWithMessage( + MessageBuilder::new()->setContent('Erro ao persistir dados'), + true + ); + }What is the PHP method signature of `Discord\Parts\Interactions\Interaction::respondWithMessage` in current DiscordPHP?Also applies to: 162-166
126-136: Editing profile will currently wipe any existing birthdateHere:
'birthdate' => $components->get('custom_id', 'birthdate')?->value ?? null,the modal does not define a
birthdateinput, so this always evaluates tonull. Combined withUpdateProfile::handle()callinginformation()->update($profileDTO->toProfile()), any run of/edit-profilewill overwrite an existingbirthdatewithnull.A minimal fix is to preserve the current value from the user’s information instead of reading from components:
$payload = UpdateProfileDTO::fromPayload([ 'tenant_id' => $this->memberProvider->tenant_id, 'provider' => $this->memberProvider->provider, 'provider_id' => $interaction->user->id, @@ - 'github_url' => $components->get('custom_id', 'github_url')->value, - 'birthdate' => $components->get('custom_id', 'birthdate')?->value ?? null, + 'github_url' => $components->get('custom_id', 'github_url')->value, + 'birthdate' => $this->memberProvider?->user?->information?->birthdate, 'about' => $components->get('custom_id', 'about')->value, ]);If you intend to let users change birthdate via Discord too, a follow‑up would be to add a
TextInputforbirthdateto the modal (with documented format) and map from components instead.
🧹 Nitpick comments (4)
app-modules/bot-discord/src/Events/WelcomeMember.php (1)
22-22: Address the TODO: Hardcoded channel ID should be configurable.The channel ID is hardcoded with a TODO comment. Consider moving this to environment configuration or a tenant-specific setting to avoid code changes when the channel needs to change.
Do you want me to open an issue to track making this configurable?
app-modules/bot-discord/src/Events/MessageReceivedEvent.php (1)
50-54: Consider structured logging instead of string concatenation.Laravel's
Logfacade supports context arrays which are better for log aggregation and searching:} catch (Throwable $throwable) { - $this->logger()->error( - sprintf('%s | File: %s | Line: %s | Trace: %s', $throwable->getMessage(), $throwable->getFile(), $throwable->getLine(), $throwable->getTraceAsString()), - ); + $this->logger()->error($throwable->getMessage(), [ + 'file' => $throwable->getFile(), + 'line' => $throwable->getLine(), + 'trace' => $throwable->getTraceAsString(), + ]); }app-modules/message/src/Actions/NewMessage.php (1)
45-52: Silent error swallowing may cause message loss without visibility.Catching
Throwableand only logging means message persistence failures are silently dropped. For a Discord bot, this may be acceptable (messages are transient), but consider whether:
- Failed messages should be queued for retry
- Metrics/alerts should be triggered on persistent failures
If silent failure is intentional, document it:
- } catch (Throwable $throwable) { + } catch (Throwable $throwable) { + // Intentionally catching all exceptions to prevent Discord event handler failures. + // Message persistence failures are logged but not propagated. Log::error('NewMessage failed', [app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php (1)
102-108: Avoid usingfake()for modal default contentUsing
fake()->paragraph(10)as the default value makes the field large and random on every invocation, and risks bumping against the 1000‑char limit. A short, static example sentence as default (or no default, just the placeholder) is safer and clearer for users.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app-modules/bot-discord/src/Actions/UserCharacterResolver.php(1 hunks)app-modules/bot-discord/src/DTO/ResolvedUserCharacter.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/SlashCommands/EditProfileCommand.php(1 hunks)app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php(1 hunks)app-modules/bot-discord/src/SlashCommands/ProfileCommand.php(1 hunks)app-modules/message/src/Actions/NewMessage.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app-modules/bot-discord/src/SlashCommands/ProfileCommand.php
🧰 Additional context used
🧬 Code graph analysis (3)
app-modules/bot-discord/src/Actions/UserCharacterResolver.php (4)
app-modules/bot-discord/src/DTO/ResolvedUserCharacter.php (1)
ResolvedUserCharacter(11-20)app-modules/character/src/Models/Character.php (1)
Character(28-128)app-modules/provider/src/Models/Provider.php (2)
Provider(28-94)model(49-52)app-modules/user/src/Models/User.php (2)
address(57-60)information(82-85)
app-modules/bot-discord/src/DTO/ResolvedUserCharacter.php (2)
app-modules/character/src/Models/Character.php (1)
Character(28-128)app-modules/provider/src/Models/Provider.php (1)
Provider(28-94)
app-modules/bot-discord/src/SlashCommands/EditProfileCommand.php (4)
app-modules/user/src/Actions/UpdateProfile.php (1)
UpdateProfile(12-31)app-modules/user/src/DTO/UpdateProfileDTO.php (2)
UpdateProfileDTO(9-49)fromPayload(23-36)app-modules/bot-discord/src/SlashCommands/AbstractSlashCommand.php (1)
AbstractSlashCommand(16-55)app-modules/user/src/Models/User.php (1)
information(82-85)
🪛 PHPMD (2.15.0)
app-modules/bot-discord/src/Events/WelcomeMember.php
20-20: Avoid unused parameters such as '$discord'. (undefined)
(UnusedFormalParameter)
app-modules/bot-discord/src/Events/MessageReceivedEvent.php
27-27: Avoid unused parameters such as '$discord'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (2)
app-modules/bot-discord/src/DTO/ResolvedUserCharacter.php (1)
11-19: LGTM! Clean DTO implementation.The value object correctly encapsulates the resolution result with typed properties. Consider marking the class as
readonlyfor full immutability (PHP 8.2+), but this is optional.app-modules/message/src/Actions/NewMessage.php (1)
23-43: Good refactor: Cleaner separation of concerns.Delegating user/character resolution to
UserCharacterResolverimproves maintainability and reusability. The flow is now clearer:
- Resolve user/character/provider
- Calculate experience
- Update character
- Persist message
Ensure
UserCharacterResolveris wrapped in a transaction (as noted in my other comment) for atomicity.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
app-modules/bot-discord/src/Actions/UserCharacterResolver.php (2)
27-55: Wrap the multi-step new-user creation in a DB transaction to avoid partial recordsUser, Address, Information, Character, and Provider are all created in sequence with no transaction. If any step after
User::create()fails, you can end up with inconsistent or orphaned data (e.g., User without Character/Provider), especially under failure or retry scenarios.You can make this atomic by wrapping the whole branch in a transaction and returning the DTO from inside the closure, e.g.:
+use Illuminate\Support\Facades\DB; @@ - if (! $providerEntity) { - $user = User::query()->create([ + if (! $providerEntity) { + return DB::transaction(function () use ($provider, $providerId, $username, $tenantId) { + $user = User::query()->create([ 'id' => Uuid::uuid4()->toString(), 'username' => $username, 'name' => $username, 'is_donator' => false, - ]); + ]); @@ - $providerEntity = $user->providers()->create([ + $providerEntity = $user->providers()->create([ 'tenant_id' => $tenantId, 'model_type' => User::class, 'provider' => $provider, 'provider_id' => $providerId, - ]); - - return new ResolvedUserCharacter( - user: $user, - provider: $providerEntity, - character: $character, - isNewUser: true - ); + ]); + + return new ResolvedUserCharacter( + user: $user, + provider: $providerEntity, + character: $character, + isNewUser: true, + ); + }); }This keeps your
resolvecontract but ensures all-or-nothing persistence.
59-63: Handling existing users without a Character for this tenantFor existing providers you do:
$character = Character::query() ->where('tenant_id', $tenantId) ->where('user_id', $user->id) ->firstOrFail();If the user exists (via this provider) but hasn’t yet got a
Characterfor the currenttenant_id,firstOrFail()will throw and break the resolution path. Given the method name (resolve) and the new-user branch behavior, it might be more useful to create the Character when missing, similar to the new-user flow, e.g.:$character = Character::query() ->where('tenant_id', $tenantId) ->where('user_id', $user->id) ->first(); if (! $character) { $character = $user->character()->create([ 'tenant_id' => $tenantId, ]); }If you also care about distinguishing “new character for this tenant” from “existing character”, you may want to extend
ResolvedUserCharacterwith anisNewCharacterflag or derive that elsewhere.
🧹 Nitpick comments (3)
app-modules/bot-discord/src/DTO/ResolvedUserCharacter.php (1)
11-18: Consider making this DTO immutable to better express value-object semanticsGiven this is just a data carrier, you may want to prevent accidental mutation by using
readonly(PHP ≥ 8.1), e.g.readonly final class ResolvedUserCharacterorpublic readonly User $useretc. This keeps intent clear without changing current behavior.app-modules/bot-discord/src/Actions/UserCharacterResolver.php (2)
16-21: Double-checkint $tenantIdmatches your actualtenant_idcolumn type
tenant_idoncharactersandprovidersoften ends up as a UUID/string in multi-tenant setups (especially withHasUuidson related models). Here it’s typed asint, which may rely on implicit casting or be incorrect if the DB column is not numeric.If
tenant_idis a UUID or string, consider changing the signature tostring $tenantId(or a UUID value object) to avoid subtle type mismatches.
22-25: Provider lookup ignores tenant and model_type — confirm this won’t cross tenants unexpectedlyThe lookup:
$providerEntity = Provider::query() ->where('provider', $provider) ->where('provider_id', $providerId) ->first();does not filter by
tenant_idormodel_type, even though you set both when creating the Provider. If the sameprovider_idcan appear under multiple tenants, this may resolve users across tenants unintentionally.Consider whether you should also filter by
tenant_id(and possiblymodel_type), e.g.:->where('tenant_id', $tenantId) ->where('model_type', User::class)if that matches your intended data model.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app-modules/bot-discord/src/Actions/UserCharacterResolver.php(1 hunks)app-modules/bot-discord/src/DTO/ResolvedUserCharacter.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app-modules/bot-discord/src/DTO/ResolvedUserCharacter.php (2)
app-modules/character/src/Models/Character.php (1)
Character(28-128)app-modules/provider/src/Models/Provider.php (1)
Provider(28-94)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app-modules/bot-discord/src/SlashCommands/CodeCommand.php (1)
24-24: Improve the command description for clarity.The description "The Code Command command." is redundant and uninformative. Consider a more descriptive message that explains the command's purpose.
Apply this diff:
- protected $description = 'The Code Command command.'; + protected $description = 'Shows how to format code in Discord messages.';
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app-modules/provider/src/Models/Provider.php (1)
24-24: LGTM! Good documentation improvement.The PHPDoc annotation correctly documents the existing
tenant_idproperty and improves IDE support.For consistency with other relationships in this model (
user(),tokens(),messages()), consider adding atenant()relationship method:/** * @return BelongsTo<Tenant, $this> */ public function tenant(): BelongsTo { return $this->belongsTo(Tenant::class); }This would allow eager loading and relationship queries like
$provider->tenantorProvider::with('tenant').composer.json (2)
18-49: New bot & framework dependencies: double‑check BrowserKit belongs inrequireAdding
he4rt/bot-discordandlaracord/frameworkhere fits the new Discord bot module and looks consistent with the rest of the he4rt stack. One thing to verify:symfony/browser-kitis pinned inrequire, but it is commonly used only for testing. If you don’t rely on it at runtime, consider moving it torequire-devto keep the production dependency surface smaller.
157-170:minimum-stability: dev+ custom repos: confirm this is intentionally broadSetting
"minimum-stability": "dev"with"prefer-stable": true, plus adding a wildcard path repo forapp-modules/*and a VCS repo forlaracord-framework, is a valid monorepo/dev flow but slightly broad: Composer can now choose dev versions for non‑he4rt packages if constraints allow. If your only dev requirement islaracord/framework(or specific local modules), you might prefer keeping globalminimum-stabilityatstableand relying on per‑package dev constraints instead. Please confirm this behavior matches what you want fromcomposer update.
📜 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 (3)
app-modules/provider/src/Models/Provider.php(1 hunks)bootstrap/providers.php(2 hunks)composer.json(5 hunks)
🔇 Additional comments (1)
bootstrap/providers.php (1)
5-27: RegisteringDiscordServiceProviderat the end of the provider list looks fineThe import and registration are consistent with the other app providers, and placing the Discord provider after the core ones should work well as long as it only depends on services they already register. No issues from this diff.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app-modules/message/tests/Feature/NewMessageTest.php(3 hunks)app-modules/message/tests/Unit/Actions/NewMessageTest.php(1 hunks)app-modules/message/tests/Unit/Actions/PersistMessageTest.php(1 hunks)app-modules/user/tests/Feature/UpdateProfileTest.php(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- app-modules/message/tests/Feature/NewMessageTest.php
⏰ 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/message/tests/Unit/Actions/PersistMessageTest.php (1)
17-26: LGTM - Test updated correctly for new DTO signature.The test has been properly updated to include the
providerUsernameparameter and uses named arguments for clarity. The changes align with the updatedNewMessageDTOconstructor signature.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
composer.json (1)
18-34: Review loose version constraints on he4rt/ packages with dev minimum-stability.*All
he4rt/*packages use>=1constraints (lines 18, 16-34), which allow any major version. Combined withminimum-stability: dev, this could introduce unexpected major version changes without explicit opt-in.Consider tightening constraints to
^X.Y.Zor~X.Y.Z(e.g.,^1.0.0instead of>=1) to prevent unintended major version jumps, especially if these are internal packages under active development.Please review whether
>=1is intentional for all he4rt/* packages or if they should use more specific version ranges.
📜 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 (4)
.github/workflows/_phpstan.yml(1 hunks)app-modules/bot-discord/src/SlashCommands/DynamicVoiceCommand.php(1 hunks)app-modules/bot-discord/src/Tasks/DynamicVoiceTask.php(1 hunks)composer.json(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app-modules/bot-discord/src/SlashCommands/DynamicVoiceCommand.php
- app-modules/bot-discord/src/Tasks/DynamicVoiceTask.php
🔇 Additional comments (4)
.github/workflows/_phpstan.yml (1)
49-51: I'll help you verify the review comment. Let me check if the.env.testing.examplefile exists in the repository.
<function_calls>
#!/bin/bashVerify that .env.testing.example exists in the repository root
fd -t f '.env.testing.example' . --max-depth 1
</function_calls>composer.json (3)
168-170: Verify VCS repository configuration for laracord-framework fork.The custom VCS repository points to a fork at
https://github.com/danielhe4rt/laracord-framework. Ensure:
- The repository is accessible and correctly configured
- This is an intentional architectural decision (not a temporary workaround)
- There's a plan to migrate back to the upstream package if/when the needed changes are merged
Please confirm the repository URL is correct and accessible. If this is a permanent fork, document why the upstream package doesn't meet requirements.
47-47: Verify symfony/browser-kit v7.0.8 compatibility with other dependencies.The pinned version
v7.0.8is specific, which is good for reproducibility. However, verify it doesn't conflict with symfony versions required by:
laravel/framework: ^12.40.1filament/filament: ^4.2.3- Other packages that may have implicit symfony dependencies
Run
composer update --dry-runto verify no version conflicts exist before merging.
157-157: I'll verify the review comment concerns aboutminimum-stability: devin the composer.json file. Let me gather information about the PR context, dependency strategy, and whether this is properly justified.
<function_calls>
Examine the full composer.json to understand dependency structure and configuration
#!/bin/bashRead composer.json to see full dependency structure
cat composer.json | head -200
</function_calls>
<function_calls>
Check for PR description and comments that might explain the minimum-stability decision
#!/bin/bashCheck PR title, description, and comments using GitHub CLI
gh pr view --json body,title,comments
</function_calls>
<function_calls>
Look for CI/testing configuration files that might explain risk mitigation
#!/bin/bashFind common CI configuration files
fd -t f '(.github|.gitlab|.circleci|phpunit|pytest|package.json)' --max-depth 3 | head -20
</function_calls>
danielhe4rt
force-pushed
the
feat/discord-bot
branch
from
5871040 to
955ca01
Compare
5 participants
Summary by CodeRabbit
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.