Closed
Conversation
Contributor
WalkthroughThe moderation commands ( Changes
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/utils/users.ts (1)
19-19: Use command-agnostic logging in shared resolver.Line 19 hardcodes
"unmute"inside a shared helper used by ban/mute/unban/unmute, which makes logs misleading during debugging.Suggested patch
- logger.debug(`unmute: no userId for username ${idOrUsername}`) + logger.debug({ idOrUsername }, "users: cannot resolve userId from username/id")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/users.ts` at line 19, The log message in the shared helper currently hardcodes "unmute" which is misleading; update the logger.debug call to be command-agnostic by removing the hardcoded verb or interpolating a passed-in action name (e.g., use logger.debug(`${action}: no userId for username ${idOrUsername}`) or simply logger.debug(`no userId for username ${idOrUsername}`)); modify the helper signature to accept an action string if needed and update all callers (ban/mute/unban/unmute) to pass the appropriate action, keeping the unique identifiers idOrUsername and the logger.debug call to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/moderation/ban.ts`:
- Around line 20-25: Update the help text for the ban command arguments to
replace incorrect "mute" wording with "ban": edit the first argument description
(the one that mentions reply handling) and the argument object with key "reason"
in the ban command definition so they state "ban" instead of "mute" (ensure the
phrasing matches the ban semantics and optional note about when reason applies).
- Line 30: The command definition currently forces reply-only usage by setting
reply: "required" which prevents the non-reply target branch in the /ban handler
from ever executing; update the command option/property named reply in
src/commands/moderation/ban.ts to be optional (e.g., "optional" or remove the
required flag) so the non-reply targeting logic in the branch around the handler
that checks for an explicit target (the code handling the non-reply path at the
branch around lines 47-63) can be reached; ensure the command validation still
handles both reply and explicit-target cases in the ban handler function.
- Line 46: The assignment for reason currently does reason = [args.reasonOrUser,
args.reason].filter(Boolean).join(" ") ?? undefined but join never yields
nullish so an empty string can slip through; update the logic that computes
reason (the variable named reason using args.reasonOrUser and args.reason) to
normalize empty or whitespace-only results to undefined (e.g., build the joined
string, trim it, and set reason = trimmed || undefined) so empty reasons become
undefined.
In `@src/commands/moderation/mute.ts`:
- Line 82: The current assignment to reason can end up as an empty string (""),
so update the expression that computes reason in mute.ts (the variable named
reason) to normalize empty results to undefined; for example, build the joined
string from [args.reasonOrUser, args.reason], trim it and then use a fallback
like || undefined (or explicitly set to undefined when === "") so that
empty-string reasons are not sent.
---
Nitpick comments:
In `@src/utils/users.ts`:
- Line 19: The log message in the shared helper currently hardcodes "unmute"
which is misleading; update the logger.debug call to be command-agnostic by
removing the hardcoded verb or interpolating a passed-in action name (e.g., use
logger.debug(`${action}: no userId for username ${idOrUsername}`) or simply
logger.debug(`no userId for username ${idOrUsername}`)); modify the helper
signature to accept an action string if needed and update all callers
(ban/mute/unban/unmute) to pass the appropriate action, keeping the unique
identifiers idOrUsername and the logger.debug call to locate the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1afee200-9269-4b32-ab8f-b19a4eb80709
📒 Files selected for processing (3)
src/commands/moderation/ban.tssrc/commands/moderation/mute.tssrc/utils/users.ts
Comment on lines
+20
to
+25
| "If the message is a reply, this argument is the reason. Otherwise, it's the username or user id of the user to mute", | ||
| }, | ||
| { | ||
| key: "reason", | ||
| optional: true, | ||
| description: "Reason to mute the user (only if the first argument is the username or user id)", |
Contributor
There was a problem hiding this comment.
Fix copy text in ban argument descriptions.
Lines 20 and 25 say "mute" in the ban command help text, which is confusing for users.
Suggested patch
- "If the message is a reply, this argument is the reason. Otherwise, it's the username or user id of the user to mute",
+ "If the message is a reply, this argument is the reason. Otherwise, it's the username or user id of the user to ban",
...
- description: "Reason to mute the user (only if the first argument is the username or user id)",
+ description: "Reason to ban the user (only if the first argument is the username or user id)",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "If the message is a reply, this argument is the reason. Otherwise, it's the username or user id of the user to mute", | |
| }, | |
| { | |
| key: "reason", | |
| optional: true, | |
| description: "Reason to mute the user (only if the first argument is the username or user id)", | |
| "If the message is a reply, this argument is the reason. Otherwise, it's the username or user id of the user to ban", | |
| }, | |
| { | |
| key: "reason", | |
| optional: true, | |
| description: "Reason to ban the user (only if the first argument is the username or user id)", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/moderation/ban.ts` around lines 20 - 25, Update the help text
for the ban command arguments to replace incorrect "mute" wording with "ban":
edit the first argument description (the one that mentions reply handling) and
the argument object with key "reason" in the ban command definition so they
state "ban" instead of "mute" (ensure the phrasing matches the ban semantics and
optional note about when reason applies).
| ], | ||
| description: "Permanently ban a user from a group", | ||
| scope: "group", | ||
| reply: "required", |
Contributor
There was a problem hiding this comment.
Make non-reply /ban path reachable.
Line 30 still has reply: "required", so the non-reply targeting branch at Lines 47-63 is effectively blocked.
Suggested patch
- reply: "required",
+ reply: "optional",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| reply: "required", | |
| reply: "optional", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/moderation/ban.ts` at line 30, The command definition currently
forces reply-only usage by setting reply: "required" which prevents the
non-reply target branch in the /ban handler from ever executing; update the
command option/property named reply in src/commands/moderation/ban.ts to be
optional (e.g., "optional" or remove the required flag) so the non-reply
targeting logic in the branch around the handler that checks for an explicit
target (the code handling the non-reply path at the branch around lines 47-63)
can be reached; ensure the command validation still handles both reply and
explicit-target cases in the ban handler function.
| return | ||
| } | ||
| user = repliedTo.from | ||
| reason = [args.reasonOrUser, args.reason].filter(Boolean).join(" ") ?? undefined |
Contributor
There was a problem hiding this comment.
Normalize empty reason to undefined.
Line 46 uses .join(" ") ?? undefined; join never returns nullish, so this can pass "" instead of undefined.
Suggested patch
- reason = [args.reasonOrUser, args.reason].filter(Boolean).join(" ") ?? undefined
+ const mergedReason = [args.reasonOrUser, args.reason].filter(Boolean).join(" ").trim()
+ reason = mergedReason || undefined📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| reason = [args.reasonOrUser, args.reason].filter(Boolean).join(" ") ?? undefined | |
| const mergedReason = [args.reasonOrUser, args.reason].filter(Boolean).join(" ").trim() | |
| reason = mergedReason || undefined |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/moderation/ban.ts` at line 46, The assignment for reason
currently does reason = [args.reasonOrUser, args.reason].filter(Boolean).join("
") ?? undefined but join never yields nullish so an empty string can slip
through; update the logic that computes reason (the variable named reason using
args.reasonOrUser and args.reason) to normalize empty or whitespace-only results
to undefined (e.g., build the joined string, trim it, and set reason = trimmed
|| undefined) so empty reasons become undefined.
| return | ||
| } | ||
| user = repliedTo.from | ||
| reason = [args.reasonOrUser, args.reason].filter(Boolean).join(" ") ?? undefined |
Contributor
There was a problem hiding this comment.
Avoid sending empty-string reasons.
Line 82 can produce "" (not undefined) because .join(" ") ?? undefined never yields nullish.
Suggested patch
- reason = [args.reasonOrUser, args.reason].filter(Boolean).join(" ") ?? undefined
+ const mergedReason = [args.reasonOrUser, args.reason].filter(Boolean).join(" ").trim()
+ reason = mergedReason || undefined🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/moderation/mute.ts` at line 82, The current assignment to reason
can end up as an empty string (""), so update the expression that computes
reason in mute.ts (the variable named reason) to normalize empty results to
undefined; for example, build the joined string from [args.reasonOrUser,
args.reason], trim it and then use a fallback like || undefined (or explicitly
set to undefined when === "") so that empty-string reasons are not sent.
Contributor
Author
|
superseded by #88 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.