Add role handling for add commands #652
Conversation
WalkthroughThe add slash command makes both 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 target
alt No target
Bot-->>User: Error embed (no_args)
else One or both targets
opt Member provided
Bot->>DiscordAPI: Update channel perms for Member
DiscordAPI-->>Bot: Ack
Bot-->>User: Emit embed & log (addedMember)
end
opt Role provided
Bot->>DiscordAPI: Update channel perms for Role
DiscordAPI-->>Bot: Ack
Bot-->>User: Emit embed & log (addedRole)
end
Bot-->>User: Final success embed (uses {args})
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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
🔭 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
|
Hey @eartharoid can you review pls |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/commands/slash/add.js (1)
178-182: Use localized list formatting for{args}.Build a natural-language list using
Intl.ListFormatinstead of joining with " & ".- args: [member?.toString(), role?.toString()].filter(Boolean).join(' & '), + args: new Intl.ListFormat(ticket.guild.locale ?? 'en-GB', { + style: 'long', + type: 'conjunction', + }).format([member?.toString(), role?.toString()].filter(Boolean)),
🧹 Nitpick comments (2)
src/commands/slash/add.js (2)
114-168: DRY the duplicate add/announce blocks with a small helper.Both branches repeat the same permissions and messaging. Extract a helper and a shared PERMS object for clarity and easier future changes.
@@ - 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.send({ - embeds: [ - new ExtendedEmbedBuilder() - .setColor(ticket.guild.primaryColour) - .setDescription(getMessage('commands.slash.add.added', { - added: member.toString(), - by: interaction.member.toString(), - })), - ], - }); - - } + const PERMS = { + AttachFiles: true, + EmbedLinks: true, + ReadMessageHistory: true, + SendMessages: true, + ViewChannel: true, + }; + + const grant = async (target, reason, addedLabel) => { + await ticketChannel.permissionOverwrites.edit(target, PERMS, reason); + await ticketChannel.send({ + embeds: [ + new ExtendedEmbedBuilder() + .setColor(ticket.guild.primaryColour) + .setDescription(getMessage('commands.slash.add.added', { + added: addedLabel, + by: interaction.member.toString(), + })), + ], + }); + }; + + if (member) { + await grant( + member, + `${interaction.user.tag} added ${member.user.tag} to the ticket`, + member.toString(), + ); + } @@ - 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.send({ - embeds: [ - new ExtendedEmbedBuilder() - .setColor(ticket.guild.primaryColour) - .setDescription(getMessage('commands.slash.add.added', { - added: role.toString(), - by: interaction.member.toString(), - })), - ], - }); - - } + if (role) { + await grant( + role, + `${interaction.user.tag} added ${role.name} to the ticket`, + role.toString(), + ); + }
186-214: Combine logs when both member and role are added.Reduce noise by emitting a single log entry with both updates when applicable.
- if (member) { - logTicketEvent(this.client, { - action: 'update', - diff: { - original: {}, - updated: { [getMessage('log.ticket.addedMember')]: member.user.tag }, - }, - target: { - id: ticket.id, - name: `<#${ticket.id}>`, - }, - userId: interaction.user.id, - }); - } - - if (role) { - logTicketEvent(this.client, { - action: 'update', - diff: { - original: {}, - updated: { [getMessage('log.ticket.addedRole')]: role.name }, - }, - target: { - id: ticket.id, - name: `<#${ticket.id}>`, - }, - userId: interaction.user.id, - }); - } + const updated = {}; + if (member) updated[getMessage('log.ticket.addedMember')] = member.user.tag; + if (role) updated[getMessage('log.ticket.addedRole')] = role.name; + if (Object.keys(updated).length) { + logTicketEvent(this.client, { + action: 'update', + diff: { original: {}, updated }, + target: { id: ticket.id, name: `<#${ticket.id}>` }, + userId: interaction.user.id, + }); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/commands/slash/add.js(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/commands/slash/add.js (2)
src/lib/tickets/manager.js (23)
interaction(1056-1066)ticket(87-87)ticket(616-616)ticket(697-697)ticket(819-826)ticket(922-929)ticket(1053-1053)ticket(1131-1131)ticket(1201-1201)ticket(1226-1226)ticket(1229-1232)member(191-191)member(1277-1277)ExtendedEmbedBuilder(18-18)getMessage(174-174)getMessage(192-192)getMessage(400-400)getMessage(827-827)getMessage(930-930)getMessage(1015-1015)getMessage(1067-1067)getMessage(1081-1081)getMessage(1132-1132)src/commands/slash/remove.js (8)
ticketChannel(91-91)ticket(49-52)member(92-92)ExtendedEmbedBuilder(5-5)getMessage(56-56)getMessage(70-70)interaction(43-144)client(10-38)
⏰ 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 (2)
src/commands/slash/add.js (2)
21-29: Options correctly marked optional.Member and role are optional as intended; nice catch aligning
getMember(..., false)/getRole(..., false)with the option schema.
100-112: Good early return when no targets provided.Clear guard prevents side effects and gives a localized error. LGTM.
Antoine489
force-pushed
the
add-role-handling-for-add-commands
branch
from
dfa86c7 to
1a0d9ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/commands/slash/add.js (1)
98-113:getMembermust be optional; currenttruebreaks the “at least one” contract.Calling
getMember('member', true)will throw when only a role is provided, making the no‑args guard (Lines 101‑113) unreachable.- const member = interaction.options.getMember('member', true); + const member = interaction.options.getMember('member', false);
🧹 Nitpick comments (2)
src/commands/slash/add.js (2)
115-141: DRY the nearly identical member/role blocks.Permission overwrite + announce logic is duplicated. Extract a small helper to reduce surface for mistakes and keep reasons/messages consistent.
Example (inline helper inside
run):const grantAndAnnounce = async (target, label) => { await ticketChannel.permissionOverwrites.edit( target, { AttachFiles: true, EmbedLinks: true, ReadMessageHistory: true, SendMessages: true, ViewChannel: true }, `${interaction.user.tag} added ${label} to the ticket`, ); await ticketChannel.send({ embeds: [ new ExtendedEmbedBuilder() .setColor(ticket.guild.primaryColour) .setDescription(getMessage('commands.slash.add.added', { added: target.toString(), by: interaction.member.toString(), })), ], }); }; // usage if (member) await grantAndAnnounce(member, member.user.tag); if (role) await grantAndAnnounce(role, role.name);Also applies to: 143-169
181-183: PreferIntl.ListFormatfor locale‑correct joining.Improves i18n and avoids hard‑coded separators.
- args: [member?.toString(), role?.toString()].filter(Boolean).join(' & '), + args: new Intl.ListFormat(ticket.guild.locale || 'en-GB', { style: 'long', type: 'conjunction' }) + .format([member?.toString(), role?.toString()].filter(Boolean)),
📜 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(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/i18n/en-GB.yml
🧰 Additional context used
🧬 Code graph analysis (1)
src/commands/slash/add.js (2)
src/lib/tickets/manager.js (22)
interaction(1056-1066)ticket(87-87)ticket(616-616)ticket(697-697)ticket(819-826)ticket(922-929)ticket(1053-1053)ticket(1131-1131)ticket(1201-1201)ticket(1226-1226)ticket(1229-1232)member(191-191)member(1277-1277)getMessage(174-174)getMessage(192-192)getMessage(400-400)getMessage(827-827)getMessage(930-930)getMessage(1015-1015)getMessage(1067-1067)getMessage(1081-1081)getMessage(1132-1132)src/commands/slash/remove.js (5)
ticketChannel(91-91)ticket(49-52)member(92-92)getMessage(56-56)getMessage(70-70)
⏰ 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: Analyze (javascript)
🔇 Additional comments (3)
src/commands/slash/add.js (3)
50-50: Verify JSDoc client type.
import('client')assumes a module/type alias named “client”. If not defined in your typings, switch toimport('discord.js').Clientor your actual client type.
96-97: Channel type assumption.JSDoc asserts
TextChannel, butguild.channels.fetchcan return various channel types. If tickets can be threads or forum posts,permissionOverwrites.editmay differ. Confirm tickets are always text channels; otherwise guard or narrow.
26-30: Role option addition looks good.
Clean, optional role param with proper type; matches the new behavior.
|
My bad, now its good can you check again pls @eartharoid |
|
I’m on my phone right now, and I see your changes have been made. If I missed something or have done a msitake, I’ll double-check it on my computer as soon as I can. |
2 participants
Versioning information
Is this related to an issue?
No
Changes made
I have added an handling of role input for /add commands
Confirmations
Summary by CodeRabbit
New Features
Bug Fixes
Documentation