fix(bot-discord): guild guard and Profile ensure#303
Conversation
… and ensure Profile exists Slash commands were registered globally, making them available in DMs where guild_id and member are null. Added a guild guard in AbstractSlashCommand::maybeHandle() and unified all slash commands under AbstractSlashCommand to prevent null-access crashes. Also extracted Profile::ensureExists() to handle users whose TenantUser pivot predates the observer, fixing ModelNotFoundException in IntroductionCommand::persistData().
gvieira18
changed the title
|
Warning Review limit reached
More reviews will be available in 13 minutes and 4 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR centralizes Profile provisioning with a new Profile::ensureExists(userId, tenantId) helper and adds guild-id validation to AbstractSlashCommand via a maybeHandle override that rejects interactions missing guild_id. Several slash command classes are migrated to extend AbstractSlashCommand, IntroductionCommand adopts the Profile helper, and TenantUserObserver now uses the same helper for provisioning. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app-modules/bot-discord/src/SlashCommands/AbstractSlashCommand.php`:
- Around line 23-35: The current maybeHandle(Interaction $interaction) only
blocks DMs but still calls parent::maybeHandle(), which leads into
beforePipeline() where tenant lookup can return null and dereference
$this->tenantProvider->tenant_id; update maybeHandle (or beforePipeline) to
reject guilds that have no mapped Tenant: perform the tenant lookup for
$interaction->guild_id (same logic used in beforePipeline) and if no Tenant is
found respond with the "only usable in a server" / appropriate error and return
instead of calling parent::maybeHandle(); alternatively, add a null-check in
beforePipeline to handle a missing first() result and throw/return early to
prevent dereferencing $this->tenantProvider->tenant_id.
In `@app-modules/profile/src/Models/Profile.php`:
- Around line 59-70: ensureExists uses firstOrCreate which is not atomic and can
still trigger unique-constraint violations under concurrency; update
Profile::ensureExists to use an atomic approach (e.g., perform an upsert via
query()->updateOrCreate()/upsert() or run a transaction with an
insert-ignore/upsert) or explicitly catch the unique constraint integrity
exception around the insert and retry a select, ensuring you reference the
Profile::ensureExists method and the model's query() call when implementing the
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 419472e0-4cb1-41f3-8341-e249f95dd5d7
📒 Files selected for processing (9)
app-modules/bot-discord/src/SlashCommands/AbstractSlashCommand.phpapp-modules/bot-discord/src/SlashCommands/CargoDelasCommand.phpapp-modules/bot-discord/src/SlashCommands/CodeCommand.phpapp-modules/bot-discord/src/SlashCommands/DontAskCommand.phpapp-modules/bot-discord/src/SlashCommands/DynamicVoiceCommand.phpapp-modules/bot-discord/src/SlashCommands/EditVoiceChannelLimitCommand.phpapp-modules/bot-discord/src/SlashCommands/IntroductionCommand.phpapp-modules/identity/src/Tenant/Observers/TenantUserObserver.phpapp-modules/profile/src/Models/Profile.php
in ensureExists Pivot models pass LazyUuidFromString instead of plain strings for user_id and tenant_id.
gvieira18
force-pushed
the
fix/bot-discord-slash-command-null-guards
branch
from
b53af72 to
fb0148a
Compare
3 participants
Summary
maybeHandle()override inAbstractSlashCommandthat rejects interactions withoutguild_id(DM context), responding with an ephemeral messageSlashCommanddirectly now extendAbstractSlashCommand, so every command gets the guild guardfirstOrCreatelogic into a static method onProfile, replacing both theTenantUserObserverinline call andIntroductionCommand'sfirstOrFail()— fixesModelNotFoundExceptionfor users whose TenantUser pivot predates the observerRoot cause
Slash commands were registered globally (no
$guildproperty), making them available in DMs where$interaction->guild_idand$interaction->memberare null. Additionally,syncWithoutDetaching()only fires thecreatedpivot event for new records — users who already had a TenantUser pivot but no Profile (created before the observer existed) hitfirstOrFail()crash.Test plan
php artisan test --compact --filter=IntroductionCommand— 3 tests passphp artisan test --compact --filter=EditProfileCommand— 4 tests passvendor/bin/pint --dirty— cleanvendor/bin/phpstan analyse app-modules/bot-discord/src/SlashCommands/— 0 errors/apresentarworks for users with existing TenantUser pivot but no Profile (e.g. andredss, garreiz)Description
Guards Discord slash commands from DM (guild-less) invocations by adding a guild check in AbstractSlashCommand::maybeHandle(), updates six slash commands to extend the new base, and adds Profile::ensureExists() to avoid ModelNotFoundException for users whose TenantUser pivot predates the Profile observer.
References
Dependencies & Requirements
Contributor Summary
Changes Summary