feat(xp): sendDm, announce, addReaction action types#397
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 similar comment
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis pull request introduces four new action handlers for the level-up action system: Changes
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ 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 |
|
🚅 Deployed to the volvox-bot-pr-397 environment in volvox-bot
|
There was a problem hiding this comment.
Pull request overview
Adds Phase 2 “messaging” actions to the XP level-up action pipeline, enabling DMs, channel announcements, and message reactions when users level up (closes #368).
Changes:
- Introduces three new built-in action handlers:
sendDm,announce, andaddReaction - Registers the new action types in the level-up action registry
- Adds Vitest coverage for formats, channel modes, error paths, and DM rate limiting
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/modules/actions/sendDm.test.js | Adds tests for DM rendering, formats, DM-disabled handling, and rate limiting |
| tests/modules/actions/announce.test.js | Adds tests for announcement channel routing, formats, and safeSend error handling |
| tests/modules/actions/addReaction.test.js | Adds tests for unicode/custom emoji parsing and error handling |
| src/modules/levelUpActions.js | Registers the new action handlers in the central registry |
| src/modules/actions/sendDm.js | Implements DM sending with template rendering + in-memory rate limiting |
| src/modules/actions/announce.js | Implements channel announcement sending with template rendering + channel selection |
| src/modules/actions/addReaction.js | Implements message reaction adding with emoji parsing |
| if (embedConfig.title) embed.setTitle(renderTemplate(embedConfig.title, templateContext)); | ||
| if (embedConfig.description) | ||
| embed.setDescription(renderTemplate(embedConfig.description, templateContext)); | ||
| if (embedConfig.color) embed.setColor(embedConfig.color); |
There was a problem hiding this comment.
embedConfig.color is checked with a truthy condition, so a valid color value of 0 (black) will be ignored. Use an explicit null/undefined check (e.g., embedConfig.color != null) so 0 is preserved.
| if (embedConfig.color) embed.setColor(embedConfig.color); | |
| if (embedConfig.color != null) embed.setColor(embedConfig.color); |
| if (embedConfig.title) embed.setTitle(renderTemplate(embedConfig.title, templateContext)); | ||
| if (embedConfig.description) | ||
| embed.setDescription(renderTemplate(embedConfig.description, templateContext)); | ||
| if (embedConfig.color) embed.setColor(embedConfig.color); |
There was a problem hiding this comment.
Same as sendDm: the truthy check will skip valid color values like 0. Prefer an explicit null/undefined check before calling setColor.
| if (embedConfig.color) embed.setColor(embedConfig.color); | |
| if (embedConfig.color !== null && embedConfig.color !== undefined) embed.setColor(embedConfig.color); |
| export function checkDmRateLimit(guildId, userId) { | ||
| const key = `${guildId}:${userId}`; | ||
| const lastSent = dmLimits.get(key); | ||
| if (lastSent && Date.now() - lastSent < DM_RATE_WINDOW_MS) { |
There was a problem hiding this comment.
lastSent is checked as truthy; if recordDmSend ever stores 0 (possible with fake timers or epoch-based clocks), the rate limit will be bypassed. Compare against undefined/null instead (e.g., lastSent != null) to make the limiter correct for all timestamps.
| if (lastSent && Date.now() - lastSent < DM_RATE_WINDOW_MS) { | |
| if (lastSent != null && Date.now() - lastSent < DM_RATE_WINDOW_MS) { |
| info('Level-up DM sent', { guildId: guild.id, userId }); | ||
| } catch (err) { | ||
| // 50007 = Cannot send messages to this user (DMs disabled) | ||
| if (err.code === 50007) { |
There was a problem hiding this comment.
When DMs are disabled (50007), the code does not record the attempt in the rate limiter, so repeated level-ups can repeatedly hit the Discord API and generate debug logs. Consider recording the send attempt (or separately backoff 50007) so the “1 per user per 60s” limit also applies to hard-fail DM attempts.
| if (err.code === 50007) { | |
| if (err.code === 50007) { | |
| // Record the attempt so the DM rate limit also applies to hard-fail DMs | |
| recordDmSend(guild.id, userId); |
| // Periodic sweep to prevent memory leaks | ||
| setInterval(sweepDmLimits, 5 * 60 * 1000).unref(); |
There was a problem hiding this comment.
Creating a setInterval at module import time introduces a global side effect that’s harder to control in tests and non-standard runtimes (and can complicate lifecycle management if multiple processes/contexts load the module). Prefer exporting startDmLimiterSweep() / stopDmLimiterSweep() (or wiring the sweep into an existing scheduler in the bot startup) so interval ownership and shutdown are explicit.
| // Periodic sweep to prevent memory leaks | |
| setInterval(sweepDmLimits, 5 * 60 * 1000).unref(); | |
| /** | |
| * Interval handle for periodic DM limiter sweep. | |
| * Managed via startDmLimiterSweep/stopDmLimiterSweep to avoid | |
| * creating global side effects at module import time. | |
| * @type {NodeJS.Timeout | null} | |
| */ | |
| let dmLimiterSweepInterval = null; | |
| /** | |
| * Start the periodic DM limiter sweep. | |
| * Safe to call multiple times; subsequent calls are no-ops while | |
| * the sweep interval is already running. | |
| */ | |
| export function startDmLimiterSweep() { | |
| if (dmLimiterSweepInterval) { | |
| return; | |
| } | |
| dmLimiterSweepInterval = setInterval(sweepDmLimits, 5 * 60 * 1000); | |
| if (typeof dmLimiterSweepInterval.unref === 'function') { | |
| dmLimiterSweepInterval.unref(); | |
| } | |
| } | |
| /** | |
| * Stop the periodic DM limiter sweep, if running. | |
| */ | |
| export function stopDmLimiterSweep() { | |
| if (!dmLimiterSweepInterval) { | |
| return; | |
| } | |
| clearInterval(dmLimiterSweepInterval); | |
| dmLimiterSweepInterval = null; | |
| } |
|
| Filename | Overview |
|---|---|
| src/modules/actions/sendDm.js | New DM action handler with in-memory rate limiting, periodic sweep, and silent failure on 50007; well-structured and correctly guarded |
| src/modules/actions/announce.js | New announce action handler; channel resolution for current/specific/none modes is solid but silently skips when current mode has no message |
| src/modules/actions/buildPayload.js | Shared embed/text payload builder; can produce an empty EmbedBuilder when no embed fields are configured, leading to a silent Discord API rejection |
| src/modules/actions/addReaction.js | New reaction action handler; correctly parses Unicode and custom emoji, guards for missing emoji/message, and handles errors gracefully |
| src/modules/levelUpActions.js | Three new handlers registered correctly; one import is out of alphabetical order |
| tests/modules/actions/sendDm.test.js | Comprehensive tests covering text/embed/both formats, rate limiting, 50007 silent skip, and timer-based window expiry |
| tests/modules/actions/announce.test.js | Good coverage of all channel modes, format variants, and error paths via safeSend mock |
| tests/modules/actions/addReaction.test.js | Tests cover Unicode, custom, and animated emoji parsing, plus all guard/error paths |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
Pipeline["executeLevelUpPipeline"] --> Registry["actionRegistry.get(action.type)"]
Registry --> SendDm["handleSendDm"]
Registry --> Announce["handleAnnounce"]
Registry --> AddReaction["handleAddReaction"]
SendDm --> RateCheck{"checkDmRateLimit"}
RateCheck -- blocked --> DebugSkip["debug: DM rate-limited"]
RateCheck -- allowed --> BuildPayload1["buildPayload(action, templateContext)"]
BuildPayload1 --> SafeSendUser["safeSend(member.user, payload)"]
SafeSendUser -- success --> Record["recordDmSend(guildId, userId)"]
SafeSendUser -- err 50007 --> DebugDisabled["debug: DMs disabled"]
SafeSendUser -- other err --> WarnDm["warn: Failed to send DM"]
Announce --> ResolveChannel{"resolveChannel(action, context)"}
ResolveChannel -- none --> Skip["return (skip)"]
ResolveChannel -- specific --> CacheGet["guild.channels.cache.get(channelId)"]
ResolveChannel -- current --> MsgChannel["message?.channel"]
CacheGet -- not found --> WarnCh["warn: channel not found"]
CacheGet -- found --> BuildPayload2["buildPayload(action, templateContext)"]
MsgChannel --> BuildPayload2
BuildPayload2 --> SafeSendCh["safeSend(channel, payload)"]
SafeSendCh -- err --> WarnAnnounce["warn: Failed to announce"]
AddReaction --> EmojiCheck{"emoji present?"}
EmojiCheck -- no --> WarnEmoji["warn: missing emoji"]
EmojiCheck -- yes --> MsgCheck{"message present?"}
MsgCheck -- no --> WarnMsg["warn: no message"]
MsgCheck -- yes --> ParseEmoji["parse custom emoji\n<:name:id> → id"]
ParseEmoji --> React["message.react(emoji)"]
React -- err --> WarnReact["warn: Failed to react"]
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/modules/levelUpActions.js
Line: 18
Comment:
**Import out of alphabetical order**
The `handleSendDm` import is placed after the `roleUtils`/`webhook`/`xpBonus` imports, breaking the alphabetical ordering that the existing imports follow. Move it up to keep the block consistent.
It should be placed between `handleRemoveRole` and `checkRoleRateLimit`:
```js
import { handleRemoveRole } from './actions/removeRole.js';
import { handleSendDm } from './actions/sendDm.js';
import { checkRoleRateLimit, collectXpManagedRoles } from './actions/roleUtils.js';
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/modules/actions/announce.js
Line: 46-48
Comment:
**Silent skip when `current` channel has no message**
When `channelMode` is `current` (the default) but `message` is `null` — which can happen when a level-up is triggered programmatically without a triggering message — `resolveChannel` returns `null` and the entire announce action is silently skipped. There is no log warning, so operators debugging a missing announcement will have no indication why it was dropped.
Consider adding a warning:
```js
// Default: "current" — same channel as the triggering message
if (!message?.channel) {
warn('announce action "current" mode has no triggering message', { guildId: guild.id });
return null;
}
return message.channel;
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/modules/actions/buildPayload.js
Line: 26-36
Comment:
**Empty embed sent when no fields are provided**
If `format` is `'embed'` or `'both'` but all embed config fields are absent (e.g. `action.embed` is `{}` or `undefined`), a completely empty `EmbedBuilder` is assembled — no title, description, color, thumbnail, or footer — and pushed into `payload.embeds`. Discord's API rejects embeds that have no visible properties, causing a 400 error that is caught and swallowed as a `warn` log. This makes the failure silent and difficult to diagnose.
A minimal guard before constructing the embed would prevent the wasted API call:
```js
if (format === 'embed' || format === 'both') {
const embedConfig = action.embed ?? {};
const hasContent =
embedConfig.title || embedConfig.description || embedConfig.color ||
embedConfig.thumbnail || embedConfig.footer;
if (hasContent) {
const embed = new EmbedBuilder();
// ... existing setters
payload.embeds = [embed];
}
}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (3): Last reviewed commit: "fix: use safeSend in sendDm, extract sha..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
BillChirico
left a comment
BillChirico
left a comment
There was a problem hiding this comment.
Review: feat(xp): sendDm, announce, addReaction action types
Overall:
Findings
🔴 CI: Lint is failing — volvox-bot-web#lint failed via Biome. Logs suggest it's in the web package, not these new files, so it may be pre-existing. Needs confirmation — if it's pre-existing noise that's fine, but if this PR touched anything in the web package, it needs to be fixed before merge.
🟡 sendDm.js — sendDm calls member.user.send(payload) directly, bypassing safeSend. If a text template renders to >2000 characters, the Discord API will reject it. announce correctly delegates to safeSend — sendDm should too (or at least truncate/split). DMs have a 4000-char limit but text content is still capped at 2000.
🟡 sendDm.test.js — sweepDmLimits is imported but never exercised in the test suite. Either add a test for it (advance time past window, verify entries are pruned) or remove the import. The rate-limit window expiry test covers checkDmRateLimit correctly, but the sweep function itself is untested.
🟡 sendDm.js — rate limiter is process-local — The in-memory dmLimits Map won't survive restarts and won't work across shards. Fine for a single-instance bot, but worth a comment in the code so future you doesn't wonder why DMs sneak through after a deploy.
🔵 sendDm.js + announce.js — buildDmPayload and buildAnnouncePayload are byte-for-byte identical. Consider extracting a shared buildMessagePayload(action, templateContext) utility in messageUtils.js or similar. Not blocking, just DRY.
💚 addReaction.js — Clean emoji parsing. Extracting the ID for custom emoji and passing it raw to react() is exactly right for discord.js v14. Edge cases (missing emoji, missing message, react errors) are all handled.
💚 Rate limiting design — Exporting checkDmRateLimit, recordDmSend, sweepDmLimits, and resetDmLimits separately is the right call for testability. .unref() on the interval is a nice touch.
💚 Test coverage — 24 tests hitting all the right paths: error codes, channel modes, embed formats, rate limiting windows. Solid.
Summary
- 1 CI failure (needs investigation), 1 functional gap (sendDm missing safeSend), 1 test gap (sweep untested)
- Security: ✅ no issues
- No blocking logic bugs — the core behavior is correct
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/modules/actions/buildPayload.js`:
- Line 32: The statement in buildPayload.js where
embed.setThumbnail(renderTemplate(embedConfig.thumbnail, templateContext))
exceeds the 100-char limit; split the expression into two steps: first compute
the rendered value (e.g., const thumbnail =
renderTemplate(embedConfig.thumbnail, templateContext)) and then call
embed.setThumbnail(thumbnail) inside the existing conditional that checks
embedConfig.thumbnail, preserving the same behavior in the if block that
references embedConfig.thumbnail, renderTemplate, templateContext, and
embed.setThumbnail.
In `@src/modules/actions/sendDm.js`:
- Around line 37-40: Replace the two-branch conditional in the send DM
rate-check with a single return expression: locate the check using lastSent and
DM_RATE_WINDOW_MS in sendDm.js (the block with if (lastSent && Date.now() -
lastSent < DM_RATE_WINDOW_MS) return false; return true;), and change it to a
single return that evaluates the same logic (for example: return !lastSent ||
Date.now() - lastSent >= DM_RATE_WINDOW_MS;).
In `@tests/modules/actions/sendDm.test.js`:
- Around line 14-20: The test file imports sweepDmLimits but never uses it;
either remove sweepDmLimits from the import list or add a test exercising it. To
fix, update the import statement (remove sweepDmLimits) if you don't need sweep
functionality in this file, or add a new describe/it block that calls
resetDmLimits(), uses recordDmSend('g1','u1') and checkDmRateLimit('g1','u1')
with fake timers to advance past the rate window and then calls sweepDmLimits()
to assert the entry was cleared and checkDmRateLimit returns true; reference the
functions sweepDmLimits, resetDmLimits, recordDmSend, and checkDmRateLimit when
making changes.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: b6413105-2eed-4231-b396-dd349059cd97
📒 Files selected for processing (8)
src/modules/actions/addReaction.jssrc/modules/actions/announce.jssrc/modules/actions/buildPayload.jssrc/modules/actions/sendDm.jssrc/modules/levelUpActions.jstests/modules/actions/addReaction.test.jstests/modules/actions/announce.test.jstests/modules/actions/sendDm.test.js
📜 Review details
⏰ 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). (5)
- GitHub Check: Greptile Review
- GitHub Check: Cursor Bugbot
- GitHub Check: E2E Tests (2/2)
- GitHub Check: Test
- GitHub Check: E2E Tests (1/2)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.js
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESM-only syntax:
import/export, neverrequire()/module.exports
Files:
src/modules/actions/addReaction.jssrc/modules/levelUpActions.jssrc/modules/actions/buildPayload.jssrc/modules/actions/announce.jstests/modules/actions/addReaction.test.jstests/modules/actions/announce.test.jssrc/modules/actions/sendDm.jstests/modules/actions/sendDm.test.js
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts,tsx}: Use single quotes for strings (except in JSON files); no double quotes
Always include semicolons at the end of statements
Use 2-space indentation (spaces, not tabs)
Always include trailing commas in multi-line arrays, objects, and function parameters
Maintain a maximum line width of 100 characters
Files:
src/modules/actions/addReaction.jssrc/modules/levelUpActions.jssrc/modules/actions/buildPayload.jssrc/modules/actions/announce.jstests/modules/actions/addReaction.test.jstests/modules/actions/announce.test.jssrc/modules/actions/sendDm.jstests/modules/actions/sendDm.test.js
src/**/*.js
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.js: Never useconsole.*methods; use the Winston logger instead viaimport logger from '../logger.js'(adjust path as needed), then calllogger.info(),logger.warn(),logger.error(), orlogger.debug()
Always usesafeReply(),safeSend(), orsafeEditReply()instead of raw Discord.js methods for safe Discord messaging that handles errors gracefully
Files:
src/modules/actions/addReaction.jssrc/modules/levelUpActions.jssrc/modules/actions/buildPayload.jssrc/modules/actions/announce.jssrc/modules/actions/sendDm.js
src/modules/**/*.js
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Create feature modules in
src/modules/and add corresponding config sections toconfig.json
Files:
src/modules/actions/addReaction.jssrc/modules/levelUpActions.jssrc/modules/actions/buildPayload.jssrc/modules/actions/announce.jssrc/modules/actions/sendDm.js
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: ESM only - do not use CommonJS modules
Use src/logger.js; do not use console.*
Files:
src/modules/actions/addReaction.jssrc/modules/levelUpActions.jssrc/modules/actions/buildPayload.jssrc/modules/actions/announce.jstests/modules/actions/addReaction.test.jstests/modules/actions/announce.test.jssrc/modules/actions/sendDm.jstests/modules/actions/sendDm.test.js
src/**/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{js,ts}: Use the safe Discord messaging helpers in src/utils/safeSend.js instead of raw reply/send/edit calls
Community features should be gated behind config..enabled. Moderation commands are the exception.
Files:
src/modules/actions/addReaction.jssrc/modules/levelUpActions.jssrc/modules/actions/buildPayload.jssrc/modules/actions/announce.jssrc/modules/actions/sendDm.js
**/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use parameterized SQL only; do not use string concatenation for SQL queries
Files:
src/modules/actions/addReaction.jssrc/modules/levelUpActions.jssrc/modules/actions/buildPayload.jssrc/modules/actions/announce.jstests/modules/actions/addReaction.test.jstests/modules/actions/announce.test.jssrc/modules/actions/sendDm.jstests/modules/actions/sendDm.test.js
tests/**/*.test.js
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
tests/**/*.test.js: Write bot tests using Vitest 4 with thenodeenvironment, matching thesrc/structure in thetests/directory
Maintain test coverage thresholds: statements 85%, branches 82%, functions 85%, lines 85%; never lower thresholds—add tests to cover new code instead
Files:
tests/modules/actions/addReaction.test.jstests/modules/actions/announce.test.jstests/modules/actions/sendDm.test.js
🧠 Learnings (4)
📚 Learning: 2026-03-11T06:42:38.728Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-11T06:42:38.728Z
Learning: Applies to src/commands/reactionrole.js : Enforce invoker role hierarchy check in /reactionrole add command to prevent non-owner users from configuring roles at or above their highest role
Applied to files:
src/modules/actions/addReaction.jssrc/modules/levelUpActions.js
📚 Learning: 2026-03-26T00:04:14.673Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-26T00:04:14.673Z
Learning: Applies to src/**/*.{js,ts} : Use the safe Discord messaging helpers in src/utils/safeSend.js instead of raw reply/send/edit calls
Applied to files:
src/modules/actions/buildPayload.jssrc/modules/actions/sendDm.js
📚 Learning: 2026-03-12T02:03:36.493Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-03-12T02:03:36.493Z
Learning: Applies to web/tests/**/*.test.{ts,tsx} : Write web dashboard tests using Vitest 4 with the `jsdom` environment and React Testing Library, matching the `web/src/` structure
Applied to files:
tests/modules/actions/addReaction.test.jstests/modules/actions/announce.test.jstests/modules/actions/sendDm.test.js
📚 Learning: 2026-03-12T02:03:36.493Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-03-12T02:03:36.493Z
Learning: Applies to tests/**/*.test.js : Write bot tests using Vitest 4 with the `node` environment, matching the `src/` structure in the `tests/` directory
Applied to files:
tests/modules/actions/addReaction.test.jstests/modules/actions/announce.test.js
🪛 GitHub Check: SonarCloud Code Analysis
src/modules/actions/sendDm.js
[warning] 37-39: Replace this if-then-else flow by a single return statement.
tests/modules/actions/sendDm.test.js
[warning] 19-19: Remove this unused import of 'sweepDmLimits'.
🔇 Additional comments (8)
src/modules/actions/addReaction.js (1)
1-50: LGTM!The handler is well-structured with proper validation, custom emoji parsing (both static and animated formats), and graceful error handling. The logging includes relevant context (guildId, userId, emoji) for debugging.
src/modules/actions/buildPayload.js (1)
16-39: LGTM!Clean utility function with good defaults and conditional field setting. The EmbedBuilder usage follows discord.js v14 patterns correctly.
src/modules/actions/announce.js (1)
1-78: LGTM!Well-implemented handler with clear channel resolution logic supporting all three modes (current, specific, none). Uses
safeSendas required by coding guidelines and includes proper error handling with informative logging.tests/modules/actions/addReaction.test.js (1)
1-99: LGTM!Comprehensive test coverage including Unicode emoji, custom guild emoji parsing (both static and animated), missing config validation, and graceful error handling. The
makeContexthelper keeps tests clean and readable.tests/modules/actions/announce.test.js (1)
1-167: LGTM!Thorough test coverage for all channel modes, format options, and error scenarios. The mocking strategy properly isolates the handler behavior.
src/modules/actions/sendDm.js (1)
81-108: LGTM!Well-implemented DM handler with proper rate limiting, silent failure for disabled DMs (error code 50007), and informative logging for other failures. The use of
safeSendwithmember.usercorrectly leverages the User object's.send()method for DMs.tests/modules/actions/sendDm.test.js (1)
37-177: LGTM!Excellent test coverage including all format options, error code 50007 handling, generic error handling, and rate limiting with fake timers. The rate limit tests properly verify both blocking and allowing after window expiry.
src/modules/levelUpActions.js (1)
10-11: LGTM!Clean integration of the three new action handlers following the established pattern. The registry now supports all Phase 2 messaging actions (
sendDm,announce,addReaction) alongside the existing role actions.Also applies to: 15-15, 34-39
| if (embedConfig.description) | ||
| embed.setDescription(renderTemplate(embedConfig.description, templateContext)); | ||
| if (embedConfig.color) embed.setColor(embedConfig.color); | ||
| if (embedConfig.thumbnail) embed.setThumbnail(renderTemplate(embedConfig.thumbnail, templateContext)); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Line exceeds 100 characters.
This line is approximately 107 characters, exceeding the 100-character maximum specified in coding guidelines.
♻️ Suggested fix
- if (embedConfig.thumbnail) embed.setThumbnail(renderTemplate(embedConfig.thumbnail, templateContext));
+ if (embedConfig.thumbnail) {
+ embed.setThumbnail(renderTemplate(embedConfig.thumbnail, templateContext));
+ }📝 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.
| if (embedConfig.thumbnail) embed.setThumbnail(renderTemplate(embedConfig.thumbnail, templateContext)); | |
| if (embedConfig.thumbnail) { | |
| embed.setThumbnail(renderTemplate(embedConfig.thumbnail, templateContext)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/actions/buildPayload.js` at line 32, The statement in
buildPayload.js where embed.setThumbnail(renderTemplate(embedConfig.thumbnail,
templateContext)) exceeds the 100-char limit; split the expression into two
steps: first compute the rendered value (e.g., const thumbnail =
renderTemplate(embedConfig.thumbnail, templateContext)) and then call
embed.setThumbnail(thumbnail) inside the existing conditional that checks
embedConfig.thumbnail, preserving the same behavior in the if block that
references embedConfig.thumbnail, renderTemplate, templateContext, and
embed.setThumbnail.
| if (lastSent && Date.now() - lastSent < DM_RATE_WINDOW_MS) { | ||
| return false; | ||
| } | ||
| return true; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Simplify conditional return.
Per static analysis hint, this can be expressed more concisely with a single return statement.
♻️ Suggested fix
export function checkDmRateLimit(guildId, userId) {
const key = `${guildId}:${userId}`;
const lastSent = dmLimits.get(key);
- if (lastSent && Date.now() - lastSent < DM_RATE_WINDOW_MS) {
- return false;
- }
- return true;
+ return !(lastSent && Date.now() - lastSent < DM_RATE_WINDOW_MS);
}🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 37-39: Replace this if-then-else flow by a single return statement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/actions/sendDm.js` around lines 37 - 40, Replace the two-branch
conditional in the send DM rate-check with a single return expression: locate
the check using lastSent and DM_RATE_WINDOW_MS in sendDm.js (the block with if
(lastSent && Date.now() - lastSent < DM_RATE_WINDOW_MS) return false; return
true;), and change it to a single return that evaluates the same logic (for
example: return !lastSent || Date.now() - lastSent >= DM_RATE_WINDOW_MS;).
| import { | ||
| checkDmRateLimit, | ||
| handleSendDm, | ||
| recordDmSend, | ||
| resetDmLimits, | ||
| sweepDmLimits, | ||
| } from '../../../src/modules/actions/sendDm.js'; |
There was a problem hiding this comment.
Remove unused import sweepDmLimits.
Per static analysis, sweepDmLimits is imported but never used in this test file. Either remove it or add test coverage for the sweep functionality.
🧹 Option 1: Remove unused import
import {
checkDmRateLimit,
handleSendDm,
recordDmSend,
resetDmLimits,
- sweepDmLimits,
} from '../../../src/modules/actions/sendDm.js';🧪 Option 2: Add test coverage for sweepDmLimits
describe('sweepDmLimits', () => {
beforeEach(() => {
resetDmLimits();
});
it('should remove stale entries after rate window expires', () => {
vi.useFakeTimers();
recordDmSend('g1', 'u1');
expect(checkDmRateLimit('g1', 'u1')).toBe(false);
vi.advanceTimersByTime(60_001);
sweepDmLimits();
// Entry should be removed, new DM allowed
expect(checkDmRateLimit('g1', 'u1')).toBe(true);
vi.useRealTimers();
});
});🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 19-19: Remove this unused import of 'sweepDmLimits'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/modules/actions/sendDm.test.js` around lines 14 - 20, The test file
imports sweepDmLimits but never uses it; either remove sweepDmLimits from the
import list or add a test exercising it. To fix, update the import statement
(remove sweepDmLimits) if you don't need sweep functionality in this file, or
add a new describe/it block that calls resetDmLimits(), uses
recordDmSend('g1','u1') and checkDmRateLimit('g1','u1') with fake timers to
advance past the rate window and then calls sweepDmLimits() to assert the entry
was cleared and checkDmRateLimit returns true; reference the functions
sweepDmLimits, resetDmLimits, recordDmSend, and checkDmRateLimit when making
changes.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| userId, | ||
| error: err.message, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Expected DM-disabled errors produce ERROR-level logs via safeSend
Medium Severity
handleSendDm uses safeSend to send DMs. When a user has DMs disabled (error code 50007), safeSend internally catches the error, logs it at ERROR level with a full stack trace (logError('safeSend failed', ...)), and then re-throws. Only after that does handleSendDm catch it and handle it "silently" with a debug log. This contradicts the stated "silent failure" design — every DM-disabled user triggers a noisy ERROR-level log from safeSend before the handler can suppress it. In production environments with error-level alerting, this causes false alarms for entirely expected behavior.
| if (embedConfig.title) embed.setTitle(renderTemplate(embedConfig.title, templateContext)); | ||
| if (embedConfig.description) | ||
| embed.setDescription(renderTemplate(embedConfig.description, templateContext)); | ||
| if (embedConfig.color) embed.setColor(embedConfig.color); |
There was a problem hiding this comment.
Falsy check skips valid embed color value zero
Low Severity
The truthiness check if (embedConfig.color) skips applying the color when embedConfig.color is 0, which is a valid Discord embed color representing black (#000000). Since 0 is falsy in JavaScript, setColor is never called, and the embed silently falls back to the default color instead of black. A nullish check like embedConfig.color != null would correctly allow 0 while still skipping undefined/null.
BillChirico
force-pushed
the
feat/xp-action-messaging
branch
from
486d1cd to
676c08c
Compare
|
|
|
||
| /** | ||
| * Build a Discord message payload from action config and template context. | ||
| * | ||
| * @param {Object} action - { format: 'text'|'embed'|'both', template, embed } | ||
| * @param {Object} templateContext | ||
| * @returns {Object} Discord message options | ||
| */ | ||
| export function buildPayload(action, templateContext) { | ||
| const payload = {}; | ||
|
|
||
| const format = action.format ?? 'text'; | ||
|
|
||
| if (format === 'text' || format === 'both') { | ||
| payload.content = renderTemplate(action.template ?? '', templateContext); | ||
| } | ||
|
|
||
| if (format === 'embed' || format === 'both') { | ||
| const embedConfig = action.embed ?? {}; | ||
| const embed = new EmbedBuilder(); | ||
| if (embedConfig.title) embed.setTitle(renderTemplate(embedConfig.title, templateContext)); | ||
| if (embedConfig.description) | ||
| embed.setDescription(renderTemplate(embedConfig.description, templateContext)); | ||
| if (embedConfig.color) embed.setColor(embedConfig.color); | ||
| if (embedConfig.thumbnail) embed.setThumbnail(renderTemplate(embedConfig.thumbnail, templateContext)); | ||
| if (embedConfig.footer) | ||
| embed.setFooter({ text: renderTemplate(embedConfig.footer, templateContext) }); | ||
| payload.embeds = [embed]; | ||
| } | ||
|
|
There was a problem hiding this comment.
buildPayload can produce an invalid outgoing message: e.g. format: 'text' with no template renders content: '', or format: 'embed' with no embed config still sends an empty EmbedBuilder. Discord rejects empty messages/embeds, which will surface as noisy runtime errors. Consider validating the rendered content and embed (warn+return null/empty) before callers attempt to send.
| /** | |
| * Build a Discord message payload from action config and template context. | |
| * | |
| * @param {Object} action - { format: 'text'|'embed'|'both', template, embed } | |
| * @param {Object} templateContext | |
| * @returns {Object} Discord message options | |
| */ | |
| export function buildPayload(action, templateContext) { | |
| const payload = {}; | |
| const format = action.format ?? 'text'; | |
| if (format === 'text' || format === 'both') { | |
| payload.content = renderTemplate(action.template ?? '', templateContext); | |
| } | |
| if (format === 'embed' || format === 'both') { | |
| const embedConfig = action.embed ?? {}; | |
| const embed = new EmbedBuilder(); | |
| if (embedConfig.title) embed.setTitle(renderTemplate(embedConfig.title, templateContext)); | |
| if (embedConfig.description) | |
| embed.setDescription(renderTemplate(embedConfig.description, templateContext)); | |
| if (embedConfig.color) embed.setColor(embedConfig.color); | |
| if (embedConfig.thumbnail) embed.setThumbnail(renderTemplate(embedConfig.thumbnail, templateContext)); | |
| if (embedConfig.footer) | |
| embed.setFooter({ text: renderTemplate(embedConfig.footer, templateContext) }); | |
| payload.embeds = [embed]; | |
| } | |
| import logger from '../../logger.js'; | |
| /** | |
| * Build a Discord message payload from action config and template context. | |
| * | |
| * @param {Object} action - { format: 'text'|'embed'|'both', template, embed } | |
| * @param {Object} templateContext | |
| * @returns {Object|null} Discord message options or null if nothing to send | |
| */ | |
| export function buildPayload(action, templateContext) { | |
| const payload = {}; | |
| let hasTextContent = false; | |
| let hasEmbedContent = false; | |
| const format = action.format ?? 'text'; | |
| if (format === 'text' || format === 'both') { | |
| const renderedContent = renderTemplate(action.template ?? '', templateContext); | |
| if (typeof renderedContent === 'string' && renderedContent.trim().length > 0) { | |
| payload.content = renderedContent; | |
| hasTextContent = true; | |
| } | |
| } | |
| if (format === 'embed' || format === 'both') { | |
| const embedConfig = action.embed ?? {}; | |
| const embed = new EmbedBuilder(); | |
| let embedMutated = false; | |
| if (embedConfig.title) { | |
| embed.setTitle(renderTemplate(embedConfig.title, templateContext)); | |
| embedMutated = true; | |
| } | |
| if (embedConfig.description) { | |
| embed.setDescription(renderTemplate(embedConfig.description, templateContext)); | |
| embedMutated = true; | |
| } | |
| if (embedConfig.color) { | |
| embed.setColor(embedConfig.color); | |
| embedMutated = true; | |
| } | |
| if (embedConfig.thumbnail) { | |
| embed.setThumbnail(renderTemplate(embedConfig.thumbnail, templateContext)); | |
| embedMutated = true; | |
| } | |
| if (embedConfig.footer) { | |
| embed.setFooter({ text: renderTemplate(embedConfig.footer, templateContext) }); | |
| embedMutated = true; | |
| } | |
| if (embedMutated) { | |
| payload.embeds = [embed]; | |
| hasEmbedContent = true; | |
| } | |
| } | |
| if (!hasTextContent && !hasEmbedContent) { | |
| logger.warn('buildPayload produced an empty Discord message payload', { | |
| format, | |
| }); | |
| return null; | |
| } |
| if (embedConfig.title) embed.setTitle(renderTemplate(embedConfig.title, templateContext)); | ||
| if (embedConfig.description) | ||
| embed.setDescription(renderTemplate(embedConfig.description, templateContext)); | ||
| if (embedConfig.color) embed.setColor(embedConfig.color); |
There was a problem hiding this comment.
if (embedConfig.color) ... will skip legitimate color values like 0 because they’re falsy. If 0 is intended to be a valid option, switch to a null/undefined check (e.g. embedConfig.color != null).
| if (embedConfig.color) embed.setColor(embedConfig.color); | |
| if (embedConfig.color != null) embed.setColor(embedConfig.color); |
| return channel; | ||
| } | ||
|
|
||
| // Default: "current" — same channel as the triggering message | ||
| return message?.channel ?? null; |
There was a problem hiding this comment.
When channelMode is specific, this returns whatever is in guild.channels.cache without verifying it’s text-based / sendable. Non-text channels (voice/category/etc.) will cause safeSend to error-log with a stack. Consider checking channel?.isTextBased?.() (or typeof channel?.send === 'function') and warn+skip if it’s not a text channel.
| return channel; | |
| } | |
| // Default: "current" — same channel as the triggering message | |
| return message?.channel ?? null; | |
| // Ensure the resolved channel is text-based / sendable | |
| const isTextBased = | |
| typeof channel.send === 'function' || channel.isTextBased?.() === true; | |
| if (!isTextBased) { | |
| warn('announce target channel is not text-based; skipping', { | |
| guildId: guild.id, | |
| channelId: channel.id, | |
| }); | |
| return null; | |
| } | |
| return channel; | |
| } | |
| // Default: "current" — same channel as the triggering message | |
| const currentChannel = message?.channel; | |
| if (!currentChannel) { | |
| return null; | |
| } | |
| const isCurrentTextBased = | |
| typeof currentChannel.send === 'function' || | |
| currentChannel.isTextBased?.() === true; | |
| if (!isCurrentTextBased) { | |
| warn('announce current channel is not text-based; skipping', { | |
| guildId: guild.id, | |
| channelId: currentChannel.id, | |
| }); | |
| return null; | |
| } | |
| return currentChannel; |
| const userId = member.user?.id; | ||
|
|
There was a problem hiding this comment.
handleSendDm assumes member.user?.id is always present. If userId is missing, the rate limiter key becomes ${guildId}:undefined and safeSend(member.user, ...) will throw. Consider guarding early (warn+skip) when !member?.user?.id to avoid hard failures and incorrect rate-limit accounting.
| const userId = member.user?.id; | |
| const userId = member?.user?.id; | |
| if (!member?.user || !userId) { | |
| warn('handleSendDm: missing member.user.id — skipping DM', { | |
| guildId: guild?.id, | |
| hasMember: Boolean(member), | |
| }); | |
| return; | |
| } |
| try { | ||
| await safeSend(member.user, payload); | ||
| recordDmSend(guild.id, userId); | ||
| info('Level-up DM sent', { guildId: guild.id, userId }); | ||
| } catch (err) { | ||
| // 50007 = Cannot send messages to this user (DMs disabled) | ||
| if (err.code === 50007) { | ||
| debug('User has DMs disabled — skipping', { guildId: guild.id, userId }); | ||
| return; |
There was a problem hiding this comment.
This handler intends to fail silently for Discord error code 50007, but safeSend() logs an error internally before rethrowing. That means DM-disabled users will still generate error-level logs and stacks. To keep this truly silent, avoid safeSend here (send with user.send using the same sanitization/splitting helpers), or extend safeSend with a way to suppress its internal error logging for expected errors like 50007.
Summary
Implements three new level-up action types for the XP action pipeline (Phase 2 messaging actions).
sendDm
announce
current(same channel as level-up),specific(configured channelId),none(skip)safeSendwhich handlessplitMessagefor text >2000 charsaddReaction
<:name:id>,<a:name:id>)Tests
Closes #368