Skip to content

Conversation

@Antoine489
Copy link
Contributor

@Antoine489 Antoine489 commented Aug 11, 2025

Versioning information

  • This includes major changes (breaking changes)
  • This includes minor changes (minimal usage changes, minor new features)
  • This includes patches (bug fixes)
  • This does not change functionality at all (code refactoring, comments)

Is this related to an issue?

No

Changes made

I have added an handling of role input for /add commands

Confirmations

  • I have updated related documentation (if necessary)
  • My changes use consistent code style
  • My changes have been tested and confirmed to work

Summary by CodeRabbit

  • New Features

    • Add command now accepts an optional member and a new optional role; either or both can be provided to grant ticket access.
    • Performs per-entity updates and sends separate confirmation messages for added members and added roles; success message shows combined targets.
  • Bug Fixes

    • Returns a clear error when neither member nor role is provided.
  • Documentation

    • Updated English texts for the role option, missing-arguments error, success placeholder, and separate log labels.

@coderabbitai
Copy link

coderabbitai bot commented Aug 11, 2025

Walkthrough

The add slash command now accepts an optional member and a new optional role option (at least one required). It validates targets, updates channel permissions and emits per-entity embeds/logs (addedMember, addedRole) for each provided target, and returns a combined success message using {args}. English locale keys for role and missing-arguments were added.

Changes

Cohort / File(s) Summary
Add command logic
src/commands/slash/add.js
Made member optional and added optional role (Role type). Enforce "at least one of member or role" with early return (no_args). Process member and role separately: update channel permissions per entity, send per-entity embeds and separate log entries (addedMember, addedRole), and build combined success {args}. Adjusted JSDoc import quotes and minor type annotations.
Localization updates
src/i18n/en-GB.yml
Added commands.slash.add.options.role and commands.slash.add.no_args (title/description). Updated success description to use {args}. Removed log.ticket.added and added log.ticket.addedMember and log.ticket.addedRole.

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
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Inspect permission-overwrite logic for member vs role and ordering/merge of overwrites.
  • Verify logging calls use new i18n keys (log.ticket.addedMember, log.ticket.addedRole) and embeds format {args} correctly.
  • Confirm early-return and no_args error embed integrate with existing command flow and tests.

Poem

I twitch my whiskers, grant a role or friend,
A hop, a note — new access I send.
If none arrive, I thump a polite knock,
Then cheer as member and role unlock.
Hooray — the ticket garden grows! 🥕

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding role handling capability to the add command, which is the primary feature introduced in this PR.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between fc502bf and 8e6111a.

📒 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 locales

We’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/*.yml for 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 member and role set as optional (required: false), allowing flexible usage of the command.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 be false.

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.

📥 Commits

Reviewing files that changed from the base of the PR and between f759527 and 58aae34.

📒 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 getMember is 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.

ticketChannel is annotated as TextChannel, and you’re using permissionOverwrites.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

@Antoine489
Copy link
Contributor Author

Antoine489 commented Oct 27, 2025

Hey @eartharoid can you review pls

Copy link
Contributor Author

@Antoine489 Antoine489 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay for me !

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.ListFormat instead 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

📥 Commits

Reviewing files that changed from the base of the PR and between 58aae34 and dfa86c7.

📒 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 Antoine489 force-pushed the add-role-handling-for-add-commands branch from dfa86c7 to 1a0d9ba Compare October 28, 2025 12:52
@Antoine489 Antoine489 requested a review from eartharoid October 28, 2025 12:53
Copy link

@coderabbitai coderabbitai bot left a 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

♻️ Duplicate comments (1)
src/commands/slash/add.js (1)

98-113: getMember must be optional; current true breaks 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: Prefer Intl.ListFormat for 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

📥 Commits

Reviewing files that changed from the base of the PR and between dfa86c7 and 1c612de.

📒 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 to import('discord.js').Client or your actual client type.


96-97: Channel type assumption.

JSDoc asserts TextChannel, but guild.channels.fetch can return various channel types. If tickets can be threads or forum posts, permissionOverwrites.edit may 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.

@Antoine489
Copy link
Contributor Author

My bad, now its good can you check again pls @eartharoid

Copy link
Member

@eartharoid eartharoid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what happened with the force push and merge commits, but the files I'm seeing look the same as before. Did you overwrite your changes?

@Antoine489
Copy link
Contributor Author

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.

@Antoine489
Copy link
Contributor Author

Hey @eartharoid everything is fine now ?

Copy link

@coderabbitai coderabbitai bot left a 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

♻️ Duplicate comments (1)
src/commands/slash/add.js (1)

53-53: Critical: Still using invalid deferReply syntax for discord.js v14.

Despite being marked as addressed in previous reviews, this line still uses flags: MessageFlags.Ephemeral which is not valid in discord.js v14. This will cause a runtime error.

Apply this diff:

-		await interaction.deferReply({ flags: MessageFlags.Ephemeral });
+		await interaction.deferReply({ ephemeral: true });

And remove the now-unused import at lines 3-4:

 const {
 	ApplicationCommandOptionType,
-	MessageFlags,
 } = require('discord.js');
🧹 Nitpick comments (2)
src/commands/slash/add.js (2)

115-169: Reduce code duplication between member and role handling.

As noted in previous review comments, lines 115-141 and 143-169 are nearly identical. Consider extracting the common logic into a helper function.

Example refactor:

async function addTargetToTicket(ticketChannel, target, targetName, actorTag, getMessage, primaryColour) {
	await ticketChannel.permissionOverwrites.edit(
		target,
		{
			AttachFiles: true,
			EmbedLinks: true,
			ReadMessageHistory: true,
			SendMessages: true,
			ViewChannel: true,
		},
		`${actorTag} added ${targetName} to the ticket`,
	);

	await ticketChannel.send({
		embeds: [
			new ExtendedEmbedBuilder()
				.setColor(primaryColour)
				.setDescription(getMessage('commands.slash.add.added', {
					added: target.toString(),
					by: interaction.member.toString(),
				})),
		],
	});
}

// Then use it:
if (member) {
	await addTargetToTicket(ticketChannel, member, member.user.tag, interaction.user.tag, getMessage, ticket.guild.primaryColour);
}

if (role) {
	await addTargetToTicket(ticketChannel, role, role.name, interaction.user.tag, getMessage, ticket.guild.primaryColour);
}

181-181: Consider using Intl.ListFormat for better i18n.

As mentioned in previous review comments, while the current array/filter/join approach works, using Intl.ListFormat would provide proper conjunction handling across different languages instead of the hardcoded ' & '.

Example implementation:

-					args: [member?.toString(), role?.toString()].filter(Boolean).join(' & '),
+					args: new Intl.ListFormat(ticket.guild.locale, { type: 'conjunction' }).format(
+						[member?.toString(), role?.toString()].filter(Boolean)
+					),

This would render "member and role" in English, "membre et rôle" in French, etc., according to each locale's conventions.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1c612de and 15e06cf.

📒 Files selected for processing (1)
  • src/commands/slash/add.js (5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/commands/slash/add.js (1)
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 (3)
src/commands/slash/add.js (3)

20-43: LGTM! Options are properly configured.

The new role option and the updated member option (now optional) are correctly defined with proper types and i18n setup. The validation logic at lines 101-113 ensures at least one is provided.


98-99: LGTM! Options are correctly retrieved.

Both getMember and getRole correctly use false for the required parameter, matching the optional option definitions.


187-215: LGTM! Logging properly tracks member and role separately.

The separate logging blocks correctly emit distinct events for member and role additions, using appropriate i18n keys (addedMember and addedRole). This provides clear audit trails for each entity.

Comment on lines +101 to +113
if (!member && !role) {
return await interaction.editReply({
embeds: [
new ExtendedEmbedBuilder({
iconURL: interaction.guild.iconURL(),
text: ticket.guild.footer,
})
.setColor(ticket.guild.errorColour)
.setTitle(getMessage('commands.slash.add.no_args.title'))
.setDescription(getMessage('commands.slash.add.no_args.description')),
],
});
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add validation for invalid targets.

While the guard correctly ensures at least one of member or role is provided, there's no validation for invalid targets. The remove command (lines 55-58 in remove.js) checks that the member is not the bot or the ticket creator.

Consider adding similar validation:

if (!member && !role) {
	return await interaction.editReply({
		embeds: [
			new ExtendedEmbedBuilder({
				iconURL: interaction.guild.iconURL(),
				text: ticket.guild.footer,
			})
				.setColor(ticket.guild.errorColour)
				.setTitle(getMessage('commands.slash.add.no_args.title'))
				.setDescription(getMessage('commands.slash.add.no_args.description')),
		],
	});
}

if (member && (member.id === client.user.id || member.id === ticket.createdById)) {
	return await interaction.editReply({
		embeds: [
			new ExtendedEmbedBuilder({
				iconURL: interaction.guild.iconURL(),
				text: ticket.guild.footer,
			})
				.setColor(ticket.guild.errorColour)
				.setTitle(getMessage('commands.slash.add.invalid_target.title'))
				.setDescription(getMessage('commands.slash.add.invalid_target.description')),
		],
	});
}
🤖 Prompt for AI Agents
In src/commands/slash/add.js around lines 101 to 113, the guard checks that
either member or role is supplied but does not validate that the provided member
is a valid target (e.g., not the bot or the ticket creator). Add a validation
after the existing guard: if member is present and its id equals client.user.id
or ticket.createdById, send an error reply using the same ExtendedEmbedBuilder
pattern but with a new title/description keys
(commands.slash.add.invalid_target.title/description) and the
ticket.guild.errorColour/footer; return after sending the reply so execution
stops.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants