feat: add shared client theme support#11454
feat: add shared client theme support#11454kshitijanurag wants to merge 1 commit intodiscordjs:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
📝 WalkthroughWalkthroughThis PR introduces a SharedClientTheme feature for Discord messages, adding builder and structure classes for managing message theming with colors, gradient angles, base mix percentages, and base theme types across the builders and discord.js packages. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use OpenGrep to find security vulnerabilities and bugs across 17+ programming languages.OpenGrep is compatible with Semgrep configurations. Add an |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/builders/__tests__/messages/message.test.ts`:
- Around line 148-154: The test for clearSharedClientTheme is asserting
unrelated content: remove the assertion that content is undefined and instead
assert only the theme-related shape; locate the test using MessageBuilder,
clearSharedClientTheme and toJSON and change the expected value so it does not
include content (e.g. assert that shared_client_theme is undefined or absent
while keeping the rest of base intact).
In `@packages/discord.js/src/structures/MessagePayload.js`:
- Around line 226-236: The outgoing serialization in MessagePayload.js currently
maps this.options.sharedClientTheme.baseTheme using "?? undefined", which loses
intentional null values; update the shared_client_theme construction (the branch
that sets base_theme) to use "?? null" so that when
this.options.sharedClientTheme.baseTheme is explicitly null it is preserved in
the payload; locate the shared_client_theme block in MessagePayload.js and
change the base_theme assignment to use null as the fallback, ensuring
consistency with Message.js and the BaseThemeType|null typing.
In `@packages/discord.js/typings/index.d.ts`:
- Around line 6776-6778: The type for sharedClientTheme on
MessageOptionsSharedClientTheme is too narrow: it only allows
JSONEncodable<SharedClientTheme> but SharedClientThemeBuilder implements
JSONEncodable<APIMessageSharedClientTheme> (API uses snake_case), so update the
union for sharedClientTheme to accept both the client and API encodable shapes
and their plain types (e.g., allow JSONEncodable<SharedClientTheme> |
JSONEncodable<APIMessageSharedClientTheme> | SharedClientTheme |
APIMessageSharedClientTheme) so SharedClientThemeBuilder can be passed into
MessageCreateOptions without type errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9cda2df4-88c7-4f6e-881a-7cbbebcc3b75
📒 Files selected for processing (12)
packages/builders/__tests__/messages/message.test.tspackages/builders/src/index.tspackages/builders/src/messages/Assertions.tspackages/builders/src/messages/Message.tspackages/builders/src/messages/SharedClientTheme.tspackages/discord.js/src/structures/Message.jspackages/discord.js/src/structures/MessagePayload.jspackages/discord.js/typings/index.d.tspackages/structures/__tests__/message.test.tspackages/structures/src/messages/Message.tspackages/structures/src/messages/MessageSharedClientTheme.tspackages/structures/src/messages/index.ts
📜 Review details
🔇 Additional comments (14)
packages/structures/src/messages/index.ts (1)
13-13: LGTM!The export follows the existing alphabetical ordering convention and properly exposes the new
MessageSharedClientThememodule.packages/builders/src/messages/Assertions.ts (3)
111-113: LGTM!The validation refinement correctly adds
shared_client_themeas a valid alternative for message content, maintaining consistency with the existing pattern for content, embeds, poll, attachments, components, and stickers.
146-146: LGTM!Using
z.null().optional()correctly restrictsshared_client_themeto be eithernullorundefinedwhen Components V2 flag is set, which aligns with the pattern used for other restricted fields likepoll.
82-87: No minimum constraint is required for thecolorsarray. The Discord API does not document an explicit minimum for SharedClientTheme colors, and this feature is undocumented client-side functionality. The current validation allowing an empty array aligns with the flexible, undocumented API behavior.packages/builders/src/index.ts (1)
91-91: LGTM!The export is correctly placed alongside other message-related exports, making
SharedClientThemeBuilderavailable in the public API.packages/structures/__tests__/message.test.ts (1)
482-509: LGTM!Comprehensive test coverage for the
SharedClientThemestructure, properly validating:
- All getter mappings from snake_case API data to camelCase properties
toJSON()serialization- Handling of undefined and null
base_themevaluespackages/structures/src/messages/Message.ts (1)
163-174: LGTM!The getter correctly exposes the raw API data, allowing consuming classes to instantiate
SharedClientThemeas needed. The naming conventionsharedClientThemeDataappropriately distinguishes raw data from an instantiated structure.Minor: The parentheses around
this[kData]on line 173 are unnecessary but don't affect functionality.packages/discord.js/src/structures/Message.js (2)
512-526: LGTM!The implementation correctly transforms snake_case API data to camelCase properties, matching the TypeScript interface definition. The
?? nullfallback forbaseThemealigns with the JSDoc typedef declaring it as?number(nullable).
503-511: LGTM!The typedef accurately documents the
MessageSharedClientThemestructure with appropriate property types and descriptions that match the API documentation.packages/structures/src/messages/MessageSharedClientTheme.ts (1)
11-51: LGTM!The
SharedClientThemeclass correctly:
- Extends the
Structurebase class with appropriate generics- Exposes camelCase getters (
colors,gradientAngle,baseMix,baseTheme) that map to snake_case API data- Returns
readonly string[]for colors to prevent mutation- Handles nullable/optional
baseThemewith the union typeBaseThemeType | undefined | nullpackages/builders/src/messages/SharedClientTheme.ts (1)
22-24: Clean builder implementation with proper cloning + validation boundary.Good use of cloning for input/output isolation and centralized validation in
toJSON().Also applies to: 33-36, 45-48, 57-60, 67-70, 75-78, 87-91
packages/discord.js/typings/index.d.ts (1)
2131-2136: Public typing surface for received message theme data looks good.
SharedClientThemeplusMessage.sharedClientTheme: SharedClientTheme | nullis a clear and ergonomic runtime-facing model.Also applies to: 2233-2233
packages/builders/src/messages/Message.ts (1)
96-107: Shared client theme integration is clean and consistent.Constructor hydration, mutator methods, and
toJSONserialization forshared_client_themealign with existingMessageBuilderpatterns and look correct.Also applies to: 643-674, 684-697
packages/builders/__tests__/messages/message.test.ts (1)
111-147: SharedClientTheme test coverage is strong.The new cases cover happy path serialization plus key validation boundaries (
colors,gradient_angle,base_mix) andclearBaseTheme()behavior effectively.Also applies to: 156-190
Add support for shared client themes.
Unsetbase theme type discord-api-types#1577