-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
refactor: move full validation to ChatInputCommandBuilder #11304
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11304 +/- ##
==========================================
+ Coverage 32.45% 32.48% +0.03%
==========================================
Files 369 369
Lines 13611 13618 +7
Branches 1069 1069
==========================================
+ Hits 4417 4424 +7
Misses 9059 9059
Partials 135 135
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2f710d4 to
18c8ae9
Compare
18c8ae9 to
4772957
Compare
| const choiceStringPredicate = z.object({ | ||
| ...choiceBasePredicate.shape, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the reason for not using extend() here and in the following predicates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From https://zod.dev/api?id=extend
This approach has a few advantages:
- It uses language-level features (spread syntax) instead of library-specific APIs
- The same syntax works in Zod and Zod Mini
- It's more tsc-efficient — the .extend() method can be expensive on large schemas, and due to a TypeScript limitation it gets quadratically more expensive when calls are chained
- If you wish, you can change the strictness level of the resulting schema by using z.strictObject() or z.looseObject()
I'm not strictly committed to this approach. Down to revert the change if it's unwanted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it better honestly, probably makes sense to use it in some other places too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors validation logic for chat input commands by moving full structural validation (including options) to the ChatInputCommandBuilder level rather than validating each option independently.
Key Changes
- Modified
chatInputCommandPredicateto validate the complete command structure including nested options, subcommands, and subcommand groups - Updated all basic option classes (user, role, mentionable, boolean, attachment) to define type-specific predicates
- Changed option serialization to skip individual validation (
toJSON(false)), delegating full validation to the command builder level
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
packages/builders/src/interactions/commands/chatInput/options/user.ts |
Added userOptionPredicate to ChatInputCommandUserOption class |
packages/builders/src/interactions/commands/chatInput/options/role.ts |
Added roleOptionPredicate to ChatInputCommandRoleOption class |
packages/builders/src/interactions/commands/chatInput/options/mentionable.ts |
Added mentionableOptionPredicate to ChatInputCommandMentionableOption class |
packages/builders/src/interactions/commands/chatInput/options/boolean.ts |
Added booleanOptionPredicate to ChatInputCommandBooleanOption class |
packages/builders/src/interactions/commands/chatInput/options/attachment.ts |
Added attachmentOptionPredicate to ChatInputCommandAttachmentOption class |
packages/builders/src/interactions/commands/chatInput/options/ApplicationCommandOptionBase.ts |
Removed default predicate assignment, requiring subclasses to provide type-specific predicates |
packages/builders/src/interactions/commands/chatInput/ChatInputCommand.ts |
Changed option validation to always be disabled during serialization, deferring to command-level validation |
packages/builders/src/interactions/commands/chatInput/Assertions.ts |
Refactored validation predicates: created specific predicates for each option type, enhanced chatInputCommandPredicate to validate full structure including options and subcommands |
packages/builders/__tests__/interactions/ChatInputCommands/ChatInputCommands.test.ts |
Added test case validating commands with both subcommands and subcommand groups |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
4772957 to
3f449bc
Compare
3f449bc to
273547b
Compare
📝 WalkthroughWalkthroughThe changes refactor chat input command validation by introducing specific option-type predicates in place of a generic base predicate, updating assertion schemas to validate subcommands and subcommand groups, and altering option serialization to always disable validation overrides during JSON conversion. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
packages/builders/__tests__/interactions/ChatInputCommands/ChatInputCommands.test.ts(1 hunks)packages/builders/src/interactions/commands/chatInput/Assertions.ts(3 hunks)packages/builders/src/interactions/commands/chatInput/ChatInputCommand.ts(1 hunks)packages/builders/src/interactions/commands/chatInput/options/ApplicationCommandOptionBase.ts(1 hunks)packages/builders/src/interactions/commands/chatInput/options/attachment.ts(1 hunks)packages/builders/src/interactions/commands/chatInput/options/boolean.ts(1 hunks)packages/builders/src/interactions/commands/chatInput/options/mentionable.ts(1 hunks)packages/builders/src/interactions/commands/chatInput/options/role.ts(1 hunks)packages/builders/src/interactions/commands/chatInput/options/user.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
packages/builders/src/interactions/commands/chatInput/options/user.ts (1)
packages/builders/src/interactions/commands/chatInput/Assertions.ts (1)
userOptionPredicate(97-100)
packages/builders/src/interactions/commands/chatInput/options/attachment.ts (1)
packages/builders/src/interactions/commands/chatInput/Assertions.ts (1)
attachmentOptionPredicate(77-80)
packages/builders/src/interactions/commands/chatInput/ChatInputCommand.ts (3)
packages/builders/src/components/selectMenu/StringSelectMenu.ts (1)
options(24-26)packages/builders/src/interactions/commands/chatInput/ChatInputCommandSubcommands.ts (1)
options(39-41)packages/builders/src/interactions/commands/chatInput/mixins/SharedChatInputCommandOptions.ts (1)
options(30-32)
packages/builders/src/interactions/commands/chatInput/options/mentionable.ts (1)
packages/builders/src/interactions/commands/chatInput/Assertions.ts (1)
mentionableOptionPredicate(87-90)
packages/builders/src/interactions/commands/chatInput/options/boolean.ts (1)
packages/builders/src/interactions/commands/chatInput/Assertions.ts (1)
booleanOptionPredicate(82-85)
packages/builders/src/interactions/commands/chatInput/options/role.ts (1)
packages/builders/src/interactions/commands/chatInput/Assertions.ts (1)
roleOptionPredicate(92-95)
packages/builders/src/interactions/commands/chatInput/Assertions.ts (1)
packages/builders/src/Assertions.ts (1)
memberPermissionsPredicate(8-8)
⏰ 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: Tests
- GitHub Check: Tests
🔇 Additional comments (11)
packages/builders/src/interactions/commands/chatInput/options/boolean.ts (1)
2-2: LGTM! Predicate wiring follows the established pattern.The addition of the class-level predicate binding using
booleanOptionPredicatecorrectly implements the new validation architecture where each option type declares its own predicate.Also applies to: 9-12
packages/builders/src/interactions/commands/chatInput/ChatInputCommand.ts (1)
33-33: Validation architecture correctly centralized.The change from
option.toJSON(validationOverride)tooption.toJSON(false)is the core of this refactoring. Individual options now skip self-validation, and comprehensive validation occurs at line 36 viachatInputCommandPredicate. This ensures the entire command structure, including all nested options, is validated as a single unit.packages/builders/src/interactions/commands/chatInput/options/role.ts (1)
2-2: LGTM! Consistent predicate wiring.The role option correctly adopts the same predicate binding pattern as other option types.
Also applies to: 9-12
packages/builders/__tests__/interactions/ChatInputCommands/ChatInputCommands.test.ts (1)
382-384: Test refactoring improves maintainability.The change to use helper functions (
getNamedBuilder(),getSubcommand(),getSubcommandGroup()) makes the test more consistent with the modular builder pattern used throughout the test suite while preserving the same test coverage.packages/builders/src/interactions/commands/chatInput/options/user.ts (1)
2-2: LGTM! Predicate wiring implemented correctly.The user option follows the established predicate binding pattern consistently.
Also applies to: 9-12
packages/builders/src/interactions/commands/chatInput/options/mentionable.ts (1)
2-2: LGTM! Consistent predicate implementation.The mentionable option correctly implements the predicate binding pattern.
Also applies to: 9-12
packages/builders/src/interactions/commands/chatInput/options/attachment.ts (1)
2-2: LGTM! Predicate binding follows the pattern.The attachment option correctly adopts the predicate binding approach.
Also applies to: 9-12
packages/builders/src/interactions/commands/chatInput/options/ApplicationCommandOptionBase.ts (1)
26-26: Base class correctly delegates predicate to subclasses.Changing the
predicatefield from a concrete value to a type-only declaration (protected static readonly predicate: z.ZodType) is the foundational change that enables the per-option-type predicate wiring pattern. Subclasses now must provide their own predicates via override, and the runtime access at line 62 ((this.constructor as typeof ApplicationCommandOptionBase).predicate) correctly uses the subclass-provided value.packages/builders/src/interactions/commands/chatInput/Assertions.ts (3)
72-100: Well-structured option predicate hierarchy.The introduction of
baseBasicOptionPredicateand individual type-specific predicates is a clean approach. Each simple option type correctly extends the base and adds its specific type literal.
155-165: Subcommand structure is correct.The subcommand predicate correctly allows optional basic options (max 25), and the subcommand group correctly requires at least one subcommand with
.min(1). This aligns with Discord's API requirements for command nesting.
167-180: Full command predicate correctly enforces option structure and validates with Zod 4.The
optionsunion correctly implements Discord's constraint that commands can have either basic options OR subcommands/subcommand groups, but not a mix. Zod 4'sz.enum()successfully validates the numeric enums (InteractionContextType,ApplicationIntegrationType) fromdiscord-api-types/v10. While Zod discourages numeric enums as error-prone, they function correctly here and the predicate is actively used in ChatInputCommand validation without issues.
Supersedes #11303
Closes #11308
Made the
chatInputCommandPredicatevalidate the full structure, including options, so users that may be using this predicate can actually validate Chat Input Commands