fix(bot-discord): modal setValue crash on null profile fields and structured logging#300
Merged
Merged
Conversation
…ured logging, and dependency updates
EditProfileCommand crashed with Discord BadRequestException when profile
nickname/about were null — setValue('') violated setMinLength constraints.
Both commands now log errors to a dedicated bot-discord daily channel with
structured context (discord_user_id, guild_id, fields, exception) and
report to the exception handler. Dependencies updated via composer/npm.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds a dedicated bot-discord logging channel and changes IntroductionCommand and EditProfileCommand to emit structured error logs to that channel (including discord/guild/user/tenant ids and submitted modal fields). EditProfileCommand now derives modal defaults from the current user/profile and sets TextInput values to null when inputs don't meet minimum multibyte-length requirements; submitted modal values are extracted before persistence to include in error context. The PR also adds feature tests for EditProfile and Introduction flows, unit tests for modal validation and logging config, and updates composer and package.json dependency constraints. 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: 3
🧹 Nitpick comments (4)
app-modules/bot-discord/tests/Feature/SlashCommands/EditProfileCommandTest.php (1)
36-114: 🏗️ Heavy liftThese tests don't exercise
EditProfileCommand.Despite the suite name, none of these cases invoke
EditProfileCommand::handle()orpersistData(). They replicate the persistence steps by callingUpsertProfile/User::updateand Eloquent queries directly, so the actual command logic — including the null→placeholder modal fix and the new structured error-logging/report()path — remains uncovered. The case on Lines 65-73 in particular just asserts thatfirstOrFail()throwsModelNotFoundException, which tests Eloquent rather than the command.Consider driving the command (or at least
persistData) with mocked interaction/components so the behavior this PR changes is actually validated.🤖 Prompt for 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. In `@app-modules/bot-discord/tests/Feature/SlashCommands/EditProfileCommandTest.php` around lines 36 - 114, The tests currently exercise UpsertProfile and direct Eloquent updates but never call EditProfileCommand::handle() or its persistData() method, so update the test cases to invoke EditProfileCommand (or at minimum call persistData on an EditProfileCommand instance) with appropriate mocked interaction/context (including modal inputs and the null→placeholder modal behavior) and assert on the structured error/report path and logging behavior; ensure one test replaces the ModelNotFoundException-only case by constructing the command with a missing profile and asserting the command/reporting behavior rather than letting firstOrFail() throw, and keep existing expectations about profile fields while using UpsertProfile only indirectly via the command invocation.app-modules/bot-discord/tests/Unit/SlashCommands/EditProfileModalValidationTest.php (1)
6-19: ⚡ Quick winThis test validates an inlined copy of the logic, not the production code.
The expression on Line 7 is a duplicate of the
setValue(...)ternaries inEditProfileCommand, so this test passes regardless of whether the real command changes. Note the productionnamefield actually usesmb_strlen((string) $name) >= 2(a cast) rather than the$value !== null && ...guard asserted here, so the two are already not identical. Consider asserting against the command's modal output (or extracting the guard into a shared helper that both the command and the test call) so the test exercises real behavior.🤖 Prompt for 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. In `@app-modules/bot-discord/tests/Unit/SlashCommands/EditProfileModalValidationTest.php` around lines 6 - 19, The test in EditProfileModalValidationTest.php duplicates the inlined ternary instead of exercising the real guard used by EditProfileCommand::setValue(...) for the name field; replace the duplicated logic by either invoking the production validation (call the command's modal handling or setValue(...) on an EditProfileCommand instance and assert its modal output) or extract the guard into a shared helper (e.g., validateMinLength(string|null $value, int $min): ?string) and use that helper from both EditProfileCommand and this test so the test verifies real behavior rather than a copy.app-modules/bot-discord/tests/Feature/SlashCommands/IntroductionCommandTest.php (1)
17-31: 💤 Low valueConsider extracting hard-coded external_account_id to a constant.
The external_account_id
'540204204242206721'is used in the helper (line 23) and later referenced in tests (lines 39, 89). Extracting it to a constant would improve maintainability and make the relationship explicit.♻️ Proposed refactor
+const TENANT_GUILD_ID = '540204204242206721'; + function createIntroductionScenario(): array { $tenant = Tenant::factory()->create(); $tenant->providers()->create([ 'provider' => IdentityProvider::Discord->value, - 'external_account_id' => '540204204242206721', + 'external_account_id' => TENANT_GUILD_ID, 'tenant_id' => $tenant->id,🤖 Prompt for 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. In `@app-modules/bot-discord/tests/Feature/SlashCommands/IntroductionCommandTest.php` around lines 17 - 31, Extract the hard-coded external_account_id string into a single constant so tests and helpers share one source of truth: add a constant (e.g., EXTERNAL_ACCOUNT_ID or DISCORD_EXTERNAL_ACCOUNT_ID) and replace the literal '540204204242206721' in createIntroductionScenario() and any test assertions that reference that value; update references in the test class to use the new constant (referencing createIntroductionScenario and any test methods that assert or reuse the external_account_id).app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php (1)
103-105: 💤 Low valueConsider consistency with field extraction in persistData().
The logging code uses null-safe operators (
?->value) when extracting modal fields, but thepersistData()method (lines 139-141) extracts the same fields without null-safety. If modal components can be missing,persistData()would also crash. If they're guaranteed to be present (due to Discord validation), the null-safe operators here may be unnecessarily defensive.For consistency, either:
- Add null-safe operators to
persistData()field extraction- Remove them from logging if components are guaranteed present
🤖 Prompt for 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. In `@app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php` around lines 103 - 105, Logging extracts modal fields using null-safe operators (?->value) but persistData() reads the same components without null-safety; update persistData() to mirror the logging extraction by using the null-safe operator when accessing components in persistData() (e.g., the same component keys 'custom_id', 'name'/'nickname'/'about') and ensure you handle possible nulls (returning null or a sensible default) before persisting to avoid crashes if a component is missing.
🤖 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/EditProfileCommand.php`:
- Around line 160-167: The log in EditProfileCommand currently writes raw user
content ($name, $nickname, $about) and identifiers via
Log::channel('bot-discord')->error; change it to avoid persisting sensitive
free-text by removing the raw values and instead log only identifiers
(interaction->user->id, interaction->guild_id, memberProvider->user->id,
memberProvider->tenant_id) plus non-sensitive metadata such as presence booleans
or lengths (e.g. 'name_present' => boolval($name), 'name_length' =>
strlen($name) ?: 0, 'about_present' => boolval($about), 'about_length' =>
strlen($about) ?: 0) or a redacted placeholder for 'about'; ensure the
'exception' is still included for debugging and update the Log::channel call to
reflect these changed fields.
- Around line 80-82: The code reads $this->memberProvider?->user?->id earlier
but later directly dereferences $this->memberProvider->user->name, risking a
null dereference; update the assignments for $name, $nickname, and $about to use
null-safe access (e.g. $this->memberProvider?->user?->name and
$profile?->nickname / $profile?->about as appropriate) and/or provide safe
fallbacks so EditProfileCommand does not access properties on a null
memberProvider or user.
In `@package.json`:
- Around line 19-20: Verify whether the explicit pins for `@emnapi/core` and
`@emnapi/runtime` (both "1.10.0") are required: if they are not needed, remove
them from devDependencies so upstream tooling/hoisting provides them
transitively; if they are required for Vite/Tailwind WASI/wasm binding
compatibility (used by `@rolldown/binding-wasm32-wasi` and
`@tailwindcss/oxide-wasm32-wasi`), keep the entries but document the reason — add
a short rationale to the PR description and add a top-level JSON field in
package.json (e.g., "x-pinnedDependencies": { "`@emnapi/core`": "Pinned for WASI
wasm bindings required by `@rolldown/`@tailwindcss", "`@emnapi/runtime`": "Pinned
for WASI wasm bindings" }) so the intent is discoverable; update the commit
message to reference these packages by name.
---
Nitpick comments:
In `@app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php`:
- Around line 103-105: Logging extracts modal fields using null-safe operators
(?->value) but persistData() reads the same components without null-safety;
update persistData() to mirror the logging extraction by using the null-safe
operator when accessing components in persistData() (e.g., the same component
keys 'custom_id', 'name'/'nickname'/'about') and ensure you handle possible
nulls (returning null or a sensible default) before persisting to avoid crashes
if a component is missing.
In
`@app-modules/bot-discord/tests/Feature/SlashCommands/EditProfileCommandTest.php`:
- Around line 36-114: The tests currently exercise UpsertProfile and direct
Eloquent updates but never call EditProfileCommand::handle() or its
persistData() method, so update the test cases to invoke EditProfileCommand (or
at minimum call persistData on an EditProfileCommand instance) with appropriate
mocked interaction/context (including modal inputs and the null→placeholder
modal behavior) and assert on the structured error/report path and logging
behavior; ensure one test replaces the ModelNotFoundException-only case by
constructing the command with a missing profile and asserting the
command/reporting behavior rather than letting firstOrFail() throw, and keep
existing expectations about profile fields while using UpsertProfile only
indirectly via the command invocation.
In
`@app-modules/bot-discord/tests/Feature/SlashCommands/IntroductionCommandTest.php`:
- Around line 17-31: Extract the hard-coded external_account_id string into a
single constant so tests and helpers share one source of truth: add a constant
(e.g., EXTERNAL_ACCOUNT_ID or DISCORD_EXTERNAL_ACCOUNT_ID) and replace the
literal '540204204242206721' in createIntroductionScenario() and any test
assertions that reference that value; update references in the test class to use
the new constant (referencing createIntroductionScenario and any test methods
that assert or reuse the external_account_id).
In
`@app-modules/bot-discord/tests/Unit/SlashCommands/EditProfileModalValidationTest.php`:
- Around line 6-19: The test in EditProfileModalValidationTest.php duplicates
the inlined ternary instead of exercising the real guard used by
EditProfileCommand::setValue(...) for the name field; replace the duplicated
logic by either invoking the production validation (call the command's modal
handling or setValue(...) on an EditProfileCommand instance and assert its modal
output) or extract the guard into a shared helper (e.g.,
validateMinLength(string|null $value, int $min): ?string) and use that helper
from both EditProfileCommand and this test so the test verifies real behavior
rather than a copy.
🪄 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: a82b7017-70d5-4fa4-8679-c85c0a100b8d
⛔ Files ignored due to path filters (2)
composer.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
app-modules/bot-discord/src/SlashCommands/EditProfileCommand.phpapp-modules/bot-discord/src/SlashCommands/IntroductionCommand.phpapp-modules/bot-discord/tests/Feature/SlashCommands/EditProfileCommandTest.phpapp-modules/bot-discord/tests/Feature/SlashCommands/IntroductionCommandTest.phpapp-modules/bot-discord/tests/Unit/SlashCommands/EditProfileModalValidationTest.phpcomposer.jsonconfig/logging.phppackage.json
6 tasks
gvieira18
added a commit
that referenced
this pull request
Jun 3, 2026
## Summary - **Guild guard**: Added `maybeHandle()` override in `AbstractSlashCommand` that rejects interactions without `guild_id` (DM context), responding with an ephemeral message - **Unified hierarchy**: All 6 slash commands that extended Laracord's `SlashCommand` directly now extend `AbstractSlashCommand`, so every command gets the guild guard - **Profile::ensureExists()**: Extracted `firstOrCreate` logic into a static method on `Profile`, replacing both the `TenantUserObserver` inline call and `IntroductionCommand`'s `firstOrFail()` — fixes `ModelNotFoundException` for users whose TenantUser pivot predates the observer ## Root cause Slash commands were registered globally (no `$guild` property), making them available in DMs where `$interaction->guild_id` and `$interaction->member` are null. Additionally, `syncWithoutDetaching()` only fires the `created` pivot event for **new** records — users who already had a TenantUser pivot but no Profile (created before the observer existed) hit `firstOrFail()` crash. ## Test plan - [x] `php artisan test --compact --filter=IntroductionCommand` — 3 tests pass - [x] `php artisan test --compact --filter=EditProfileCommand` — 4 tests pass - [x] `vendor/bin/pint --dirty` — clean - [x] `vendor/bin/phpstan analyse app-modules/bot-discord/src/SlashCommands/` — 0 errors - [ ] Verify `/apresentar` works for users with existing TenantUser pivot but no Profile (e.g. andredss, garreiz) - [ ] Verify slash commands are rejected gracefully if somehow invoked from DM <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## 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 - #302 - #300 ## Dependencies & Requirements - No new dependencies or environment/configuration changes required. ## Contributor Summary | Contributor | Lines Added | Lines Removed | Files Changed | |---|---:|---:|---:| | gvieira18 | 35 | 22 | 9 | ## Changes Summary | File Path | Change Description | |---|---| | app-modules/bot-discord/src/SlashCommands/AbstractSlashCommand.php | Added public override maybeHandle() to reject guild-less (DM) interactions with an ephemeral Portuguese message. | | app-modules/bot-discord/src/SlashCommands/CargoDelasCommand.php | Switched base class to AbstractSlashCommand (inheritance change). | | app-modules/bot-discord/src/SlashCommands/CodeCommand.php | Switched base class to AbstractSlashCommand (inheritance change). | | app-modules/bot-discord/src/SlashCommands/DontAskCommand.php | Switched base class to AbstractSlashCommand (inheritance change). | | app-modules/bot-discord/src/SlashCommands/DynamicVoiceCommand.php | Switched base class to AbstractSlashCommand (inheritance change). | | app-modules/bot-discord/src/SlashCommands/EditVoiceChannelLimitCommand.php | Switched base class to AbstractSlashCommand (inheritance change). | | app-modules/bot-discord/src/SlashCommands/IntroductionCommand.php | Switched base class to AbstractSlashCommand and replaced firstOrFail() profile lookup with Profile::ensureExists(). | | app-modules/identity/src/Tenant/Observers/TenantUserObserver.php | Replaced inline firstOrCreate with Profile::ensureExists() in created() observer. | | app-modules/profile/src/Models/Profile.php | Added public static ensureExists(string|Stringable $userId, string|Stringable $tenantId): self to create-or-return Profile safely. | <!-- end of auto-generated comment: release notes by coderabbit.ai -->
5 tasks
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
3 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
EditProfileCommandcrashed with DiscordBadRequestException(code 50035) when profilenicknameoraboutwerenullin the database —setValue('')violated Discord'ssetMinLength()constraints. Now passesnullto show placeholder instead.EditProfileCommandandIntroductionCommandnow log errors to a dedicatedbot-discorddaily channel (storage/logs/bot-discord.log, 30-day retention) with full context:discord_user_id,guild_id,user_id,tenant_id, field values, and exception. Also callsreport()for external exception tracking.composer updateandnpm update.symfony/polyfill-intl-idnfrom vulnerable version to v1.38.1, fixing Dependabot alert #39 (low severity — xn-- labels with ASCII-only Punycode payloads treated as equivalent to decoded form).Test plan
setValuevalidation (9 dataset cases including null, empty, short, valid, multibyte)bot-discordlog channel configurationEditProfileCommandpersistence flow (happy path, missing profile, partial update, null→new values)IntroductionCommandpersistence flow (full introduction, missing guild, missing tenant pivot)Description
Fixes a crash in the Discord Edit Profile modal when profile fields (
nickname,about) are null by passing null to modal setValue (so placeholders show) instead of an empty string; adds structured error logging (bot-discord daily channel) and calls report() for external tracking. Also includes dependency updates and unit/feature tests to cover the changes.Dependencies & Requirements
@emnapi/*,@tailwindcss/vite, concurrently, lint-staged, npm-check-updates, prettier, tailwindcss, tw-animate-css, vite. optionalDependencies removed.Contributor Summary
Changes Summary
bot-discorddaily log channel (storage/logs/bot-discord.log, 30 days, replace_placeholders).