Add role handling for add commands #652
Conversation
WalkthroughThe add slash command now accepts optional member and role options (at least one required). It grants channel permissions per provided entity and posts per-entity "added" embeds; final success uses a combined {args} field. English locale entries for the role option and a missing-arguments error were added. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Bot
participant DiscordAPI as Discord API
User->>Bot: /add [member?] [role?]
Bot->>Bot: Validate at least one of member or role
alt Neither provided
Bot-->>User: Error embed (no_args)
else Member and/or Role provided
opt Member provided
Bot->>DiscordAPI: Grant channel perms to Member
DiscordAPI-->>Bot: Ack
Bot-->>User: Embed: member added
end
opt Role provided
Bot->>DiscordAPI: Grant channel perms to Role
DiscordAPI-->>Bot: Ack
Bot-->>User: Embed: role added
end
Bot-->>User: Success message with {args}
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (1)
src/commands/slash/add.js (1)
184-195: Update logging to include both member and role.The logging only captures the member being added but ignores the role. Both should be logged when provided.
logTicketEvent(this.client, { action: 'update', diff: { original: {}, - updated: { [getMessage('log.ticket.added')]: member.user.tag }, + updated: { + [getMessage('log.ticket.added')]: [ + member?.user.tag, + role?.name + ].filter(Boolean).join(', ') + }, }, target: { id: ticket.id, name: `<#${ticket.id}>`, }, userId: interaction.user.id, });
🧹 Nitpick comments (2)
src/commands/slash/add.js (2)
44-48: Fix inconsistent import path styles.The JSDoc annotations use inconsistent quote styles for import paths. Line 44 uses single quotes while line 47 uses single quotes without the .js extension.
/** - * @param {import('discord.js').ChatInputCommandInteraction} interaction + * @param {import('discord.js').ChatInputCommandInteraction} interaction */ async run(interaction) { - /** @type {import('client')} */ + /** @type {import('../../client')} */ const client = this.client;
112-138: Extract permission configuration to reduce duplication.The permission overwrites configuration is duplicated between member and role handling. Consider extracting it to a constant or method.
+const TICKET_PERMISSIONS = { + AttachFiles: true, + EmbedLinks: true, + ReadMessageHistory: true, + SendMessages: true, + ViewChannel: true, +}; + if (member) { await ticketChannel.permissionOverwrites.edit( member, - { - AttachFiles: true, - EmbedLinks: true, - ReadMessageHistory: true, - SendMessages: true, - ViewChannel: true, - }, + TICKET_PERMISSIONS, `${interaction.user.tag} added ${member.user.tag} to the ticket`, );Apply the same change to the role section (lines 142-152):
await ticketChannel.permissionOverwrites.edit( role, - { - AttachFiles: true, - EmbedLinks: true, - ReadMessageHistory: true, - SendMessages: true, - ViewChannel: true, - }, + TICKET_PERMISSIONS, `${interaction.user.tag} added ${role.name} to the ticket`, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/commands/slash/add.js(5 hunks)src/i18n/en-GB.yml(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/commands/slash/add.js (6)
src/commands/slash/tickets.js (2)
member(43-43)ExtendedEmbedBuilder(4-4)src/commands/user/create.js (1)
ExtendedEmbedBuilder(3-3)src/commands/slash/help.js (1)
ExtendedEmbedBuilder(3-3)src/commands/slash/move.js (1)
ExtendedEmbedBuilder(3-3)src/commands/slash/tag.js (1)
ExtendedEmbedBuilder(3-3)src/commands/message/pin.js (1)
ExtendedEmbedBuilder(2-2)
⏰ 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: Codacy Static Code Analysis
🔇 Additional comments (4)
src/i18n/en-GB.yml (3)
54-56: LGTM! Clear error messaging for missing arguments.The new error message clearly communicates that at least one argument (member or role) is required when using the add command.
61-63: LGTM! Consistent role option definition.The role option follows the same structure as the existing member option, maintaining consistency in the localization file.
68-68: Verify placeholder consistency across all localesWe’ve updated the add-success message in en-GB (
src/i18n/en-GB.yml:68) to use{args}instead of{member}. Please confirm that:
- All other locale files are updated to use
{args}(not{member}) for this message.- The command’s formatting logic correctly handles cases where both member and role are provided.
Pinpoint locations for review:
src/i18n/en-GB.yml(line 68):description: "{args} has been added to {ticket}."- Search
src/i18n/*.ymlfor occurrences of{member}and update to{args}as needed.src/commands/slash/add.js (1)
19-27: LGTM! Well-structured option configuration.The options are properly configured with both
memberandroleset as optional (required: false), allowing flexible usage of the command.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/commands/slash/add.js (1)
95-99: Fix incorrect required flag for getMember; makes the new “at least one of member/role” logic unreachable.
getMember('member', true)will force the option to be present and throw before your custom no-args check. Since both options are optional, this must befalse.Apply:
-const member = interaction.options.getMember('member', true); +const member = interaction.options.getMember('member', false);
🧹 Nitpick comments (4)
src/commands/slash/add.js (4)
49-50: Verify the JSDoc type import path for client.The
import('client')path may not resolve unless you have a declared module named "client" in your type roots. If your custom client is exported from a local module (e.g., src/client.js/ts), consider pointing to it explicitly.If needed:
-/** @type {import('client')} */ +/** @type {import('../../client').Client} */Adjust the relative path and exported type name to match your codebase. If you do have a global "client" module declaration, feel free to ignore this.
114-166: Reduce duplication and centralize permission overwrite payload.The overwrite payload is duplicated for member and role. Centralizing it reduces maintenance risk and keeps the two paths in lockstep. Also, consider batching operations when both inputs are present.
Here’s a minimal refactor within this scope:
+const overwriteAllow = { + AttachFiles: true, + EmbedLinks: true, + ReadMessageHistory: true, + SendMessages: true, + ViewChannel: true, +}; + if (member) { - await ticketChannel.permissionOverwrites.edit( - member, - { - AttachFiles: true, - EmbedLinks: true, - ReadMessageHistory: true, - SendMessages: true, - ViewChannel: true, - }, - `${interaction.user.tag} added ${member.user.tag} to the ticket`, - ); + await ticketChannel.permissionOverwrites.edit( + member, + overwriteAllow, + `${interaction.user.tag} added ${member.user.tag} to the ticket`, + ); // ... } if (role) { - await ticketChannel.permissionOverwrites.edit( - role, - { - AttachFiles: true, - EmbedLinks: true, - ReadMessageHistory: true, - SendMessages: true, - ViewChannel: true, - }, - `${interaction.user.tag} added ${role.name} to the ticket`, - ); + await ticketChannel.permissionOverwrites.edit( + role, + overwriteAllow, + `${interaction.user.tag} added ${role.name} to the ticket`, + ); // ... }Optionally, when both are present, you can run the edit/send pairs sequentially (as now) or selectively parallelize with care. Err surfaces are friendlier when kept sequential, so parallelization is optional here.
180-182: Nit: prefer a word-joiner over ' & ' for args, or let i18n own the joiner.Using a symbol is language-agnostic but less natural. Given you localize everything else, consider using " and " (as previously suggested) or let i18n provide a joiner for locale-correct list formatting.
-args: [member?.toString(), role?.toString()].filter(Boolean).join(' & '), +args: [member?.toString(), role?.toString()].filter(Boolean).join(' and '),If you want full localization, expose the joiner or list formatter from i18n and build the args via that API.
186-199: Check logging schema: localized keys in diff.updated may hamper downstream analytics.You’re using
[getMessage('log.ticket.addedMember')]/[...addedRole]as dynamic object keys. If consumers expect stable machine-readable keys, using localized text as keys can fragment metrics per locale.If your log pipeline expects stable keys, consider:
- updated: { [getMessage('log.ticket.addedMember')]: member.user.tag }, + updated: { addedMember: member.user.tag },and apply localization at the presentation layer instead of the event payload.
Also applies to: 201-214
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/commands/slash/add.js(4 hunks)src/i18n/en-GB.yml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/i18n/en-GB.yml
⏰ 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: Analyze (javascript)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (4)
src/commands/slash/add.js (4)
21-29: Role option addition looks correct and consistent with i18n plumbing.
- member option is now optional, matching runtime logic.
- role option is properly added as optional Role type.
- The mapping below populates name/description via i18n, so omission of inline description fields here is fine.
46-46: JSDoc for interaction type is accurate.Using import('discord.js').ChatInputCommandInteraction is the right annotation for v14.
100-112: Good: explicit “no arguments” branch with localized feedback.Once
getMemberis optional (see above), this block correctly handles the case where neither member nor role is supplied.
95-96: Channel type assumptions: ensure tickets are always text channels.
ticketChannelis annotated asTextChannel, and you’re usingpermissionOverwrites.edit, which is not supported on all channel types (e.g., threads behave differently). If tickets can be threads or other channel types, the cast and overwrite ops may fail.Would you like me to scan the repo to confirm ticket channel creation always produces a TextChannel and not a Thread? If it’s strictly TextChannel, the current approach is fine; otherwise we should widen the type and gate overwrite logic accordingly.
Also applies to: 114-124, 142-152
Versioning information
Is this related to an issue?
No
Changes made
I have added a handling or role input for /add commands
Confirmations
Summary by CodeRabbit