Repository navigation
refactor(bot): extract MessagePipeline handler chain - #981
Merged
Merged
Conversation
Split 322-LOC messageHandler into typed MessageHandler implementations: - MessagePipeline runner with stop-signal short-circuit - AutoModHandler (violation enforcement) - SpamHandler (spam detection, extracted from autoMod) - CustomCommandHandler (command matching) - XpHandler (XP/leveling) Each handler tested in isolation. Adding handlers requires no changes to existing files.
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 28 minutes and 30 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, 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 trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR refactors Discord message handling from monolithic in-file implementations into a modular pipeline architecture. A new ChangesMessage Handler Pipeline & Modular Architecture
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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. ✨ 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 |
|
Failed to generate code suggestions for PR |
Cover each handler's canHandle/handle branches in isolation: - AutoModHandler: violation types, exemptions, stop signal - SpamHandler: spam detection, stop signal - CustomCommandHandler: trigger matching, usage tracking - XpHandler: cooldown, level-up, role rewards
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![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
🧹 Nitpick comments (4)
packages/bot/src/handlers/message/__tests__/pipeline.test.ts (2)
11-29: ⚡ Quick winTest name says “order”, but it only checks chaining.
At Line 11, this test does not assert execution order; it only asserts
register()fluency. Please execute the pipeline and asserthandler1runs beforehandler2.Proposed test fix
-it('should register handlers in order', () => { +it('should register handlers in order', async () => { const pipeline = new MessagePipeline() @@ - pipeline.register(handler1) - pipeline.register(handler2) - - const result = pipeline.register(handler1) + const result = pipeline.register(handler1).register(handler2) expect(result).toBe(pipeline) + + const message = { guild: { id: 'guild1' }, member: { id: 'member1' } } as unknown as Message + const context = { + guild: message.guild, + member: message.member, + featureToggles: {}, + } as unknown as MessageContext + + await pipeline.execute(message, context) + expect((handler1.handle as jest.Mock).mock.invocationCallOrder[0]) + .toBeLessThan((handler2.handle as jest.Mock).mock.invocationCallOrder[0]) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/handlers/message/__tests__/pipeline.test.ts` around lines 11 - 29, The test currently only checks that register() is chainable; update it to actually run the pipeline and assert handler execution order: after registering handler1 and handler2 on the MessagePipeline instance, invoke the pipeline (e.g., await pipeline.execute(message) or await pipeline.handle(message) — whichever the MessagePipeline API exposes) with a dummy message so both handlers' canHandle/handle mocks run, then assert handler1.handle was called before handler2.handle by comparing their mock invocation order (use handler1.handle.mock.invocationCallOrder[0] < handler2.handle.mock.invocationCallOrder[0]) and keep the existing chaining assertion (expect(result).toBe(pipeline)).
125-185: ⚡ Quick winAssert
errorLogcalls in the error-path tests.At Line 125 and Line 156, error isolation is validated, but the expected logging side-effect is not. Add assertions so observability regressions are caught.
Proposed assertions
+import { errorLog } from '`@lucky/shared/utils`' @@ +const errorLogMock = jest.mocked(errorLog) + it('should isolate errors - one handler error does not crash pipeline', async () => { @@ await expect(pipeline.execute(message, context)).resolves.not.toThrow() expect(handler2.handle).toHaveBeenCalled() + expect(errorLogMock).toHaveBeenCalledWith( + expect.objectContaining({ + message: expect.stringContaining('Handler1'), + }), + ) }) @@ it('should catch canHandle errors and continue', async () => { @@ await expect(pipeline.execute(message, context)).resolves.not.toThrow() expect(handler2.handle).toHaveBeenCalled() + expect(errorLogMock).toHaveBeenCalledWith( + expect.objectContaining({ + message: expect.stringContaining('Handler1'), + }), + ) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/handlers/message/__tests__/pipeline.test.ts` around lines 125 - 185, The tests for MessagePipeline error isolation need to also assert that the pipeline logs errors; update both tests that create handler1 (the one that throws in canHandle or handle) to spy/mock the pipeline's logger (errorLog or the module logger used by MessagePipeline), run pipeline.execute(message, context) as before, then add assertions that the logger.error / errorLog was called with the handler name (handler1.name) and the thrown Error (or its message) to validate observability; ensure the spy is cleared/restored after each test.packages/bot/src/handlers/message/__tests__/xpHandler.test.ts (1)
223-273: ⚡ Quick winAdd a reward test for level-up without
announceChannel.Current coverage validates rewards only when
announceChannelexists. Add a case withleveledUp: trueand noannounceChannelthat still expectscontext.member.roles.add(...)to run.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/handlers/message/__tests__/xpHandler.test.ts` around lines 223 - 273, The test suite is missing a case that asserts role rewards are applied when a user levels up but no announceChannel is configured; add a new spec that mocks levelService.getConfig to return enabled:true and no announceChannel (omit or set undefined), mock levelService.getMemberXP to allow XP, mock levelService.addXP to resolve { leveledUp: true, newLevel: 5 }, mock levelService.getRewards to include { level:5, roleId:'role5' }, call xpHandler.handle(message, context) and assert context.member.roles.add was called with 'role5' (use the same helpers/mocks as the existing test for xpHandler.handle, levelService.getConfig, getMemberXP, addXP, getRewards and context.member.roles.add).packages/bot/src/handlers/message/__tests__/autoModHandler.test.ts (1)
153-318: ⚡ Quick winAdd a violation-path assertion for moderation side effects.
Current tests verify rule-check invocations but not the enforcement outcome (
stop: true,message.delete(), and moderation action/case side effects). Adding one such test will catch regressions in the actual moderation flow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/handlers/message/__tests__/autoModHandler.test.ts` around lines 153 - 318, Add a test that asserts enforcement side effects when a violation is detected: stub autoModService.getSettings to enable one rule (e.g., capsEnabled:true), stub the corresponding check (e.g., autoModService.checkCaps) to resolve true, spy/mock message.delete and the moderation side-effect API used by the handler (e.g., moderationService.createCase or moderationService.applyAction), then call autoModHandler.handle(message, context) and assert result.stop is true, expect(message.delete).toHaveBeenCalled(), and expect the moderation service mock (createCase/applyAction) was called with the guild id/member and relevant reason; reference autoModHandler.handle, autoModService.checkCaps (or checkLinks/checkInvites/checkWords), message.delete, and moderationService.createCase/applyAction to locate the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bot/src/handlers/message/autoModHandler.ts`:
- Around line 58-62: The auto-mod stores every detected violation with action:
'delete' (see violations.push calls in autoModHandler) but the downstream switch
only handles 'warn'/'mute'/'kick'/'ban', so enforcement branches are never
reached; fix by either (A) setting the correct action value when pushing
violations (use 'warn'/'mute'/'kick'/'ban' as appropriate) or (B) adding a
'delete' branch to the enforcement switch (and call the existing
delete/moderation helper and createCase logic for deletes), and remove any dead
branches; update all violations.push occurrences (caps, spam, mention, link
detectors) and the enforcement switch in autoModHandler accordingly so actions
and handlers match.
In `@packages/bot/src/handlers/message/xpHandler.ts`:
- Around line 49-67: The role reward logic is incorrectly nested under the
announceChannel check so rewards only run when announcements are enabled; change
the flow in the leveled-up branch (where result.leveledUp is checked) to first,
if config.announceChannel is set then fetch the channel and send the message
(using message.client.channels.fetch and TextChannel.send as currently written),
and separately (not nested under announceChannel) call
levelService.getRewards(guildId), find the matching reward for result.newLevel,
and call context.member.roles.add(reward.roleId) as before; ensure you still
guard optional operations with .catch(() => {}) and null-checks on
context.member so reward granting runs regardless of announceChannel.
In `@packages/bot/src/handlers/messageHandler.ts`:
- Around line 22-31: The two feature toggle lookups
(featureToggleService.isEnabled for 'AUTOMOD' and 'CUSTOM_COMMANDS' that
populate featureToggles) currently run with awaits that can reject and
short-circuit pipeline.execute; change to resolve each lookup independently and
default failures to false (e.g., use Promise.allSettled or individual try/catch
around each featureToggleService.isEnabled call) so that any rejection only sets
that toggle to false and does not prevent calling pipeline.execute or other
handlers like AutoMod/spam/XP.
---
Nitpick comments:
In `@packages/bot/src/handlers/message/__tests__/autoModHandler.test.ts`:
- Around line 153-318: Add a test that asserts enforcement side effects when a
violation is detected: stub autoModService.getSettings to enable one rule (e.g.,
capsEnabled:true), stub the corresponding check (e.g., autoModService.checkCaps)
to resolve true, spy/mock message.delete and the moderation side-effect API used
by the handler (e.g., moderationService.createCase or
moderationService.applyAction), then call autoModHandler.handle(message,
context) and assert result.stop is true,
expect(message.delete).toHaveBeenCalled(), and expect the moderation service
mock (createCase/applyAction) was called with the guild id/member and relevant
reason; reference autoModHandler.handle, autoModService.checkCaps (or
checkLinks/checkInvites/checkWords), message.delete, and
moderationService.createCase/applyAction to locate the code.
In `@packages/bot/src/handlers/message/__tests__/pipeline.test.ts`:
- Around line 11-29: The test currently only checks that register() is
chainable; update it to actually run the pipeline and assert handler execution
order: after registering handler1 and handler2 on the MessagePipeline instance,
invoke the pipeline (e.g., await pipeline.execute(message) or await
pipeline.handle(message) — whichever the MessagePipeline API exposes) with a
dummy message so both handlers' canHandle/handle mocks run, then assert
handler1.handle was called before handler2.handle by comparing their mock
invocation order (use handler1.handle.mock.invocationCallOrder[0] <
handler2.handle.mock.invocationCallOrder[0]) and keep the existing chaining
assertion (expect(result).toBe(pipeline)).
- Around line 125-185: The tests for MessagePipeline error isolation need to
also assert that the pipeline logs errors; update both tests that create
handler1 (the one that throws in canHandle or handle) to spy/mock the pipeline's
logger (errorLog or the module logger used by MessagePipeline), run
pipeline.execute(message, context) as before, then add assertions that the
logger.error / errorLog was called with the handler name (handler1.name) and the
thrown Error (or its message) to validate observability; ensure the spy is
cleared/restored after each test.
In `@packages/bot/src/handlers/message/__tests__/xpHandler.test.ts`:
- Around line 223-273: The test suite is missing a case that asserts role
rewards are applied when a user levels up but no announceChannel is configured;
add a new spec that mocks levelService.getConfig to return enabled:true and no
announceChannel (omit or set undefined), mock levelService.getMemberXP to allow
XP, mock levelService.addXP to resolve { leveledUp: true, newLevel: 5 }, mock
levelService.getRewards to include { level:5, roleId:'role5' }, call
xpHandler.handle(message, context) and assert context.member.roles.add was
called with 'role5' (use the same helpers/mocks as the existing test for
xpHandler.handle, levelService.getConfig, getMemberXP, addXP, getRewards and
context.member.roles.add).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 10644abb-66b1-44dd-8c89-6d4468225047
📒 Files selected for processing (13)
packages/bot/src/handlers/message/__tests__/autoModHandler.test.tspackages/bot/src/handlers/message/__tests__/customCommandHandler.test.tspackages/bot/src/handlers/message/__tests__/pipeline.test.tspackages/bot/src/handlers/message/__tests__/spamHandler.test.tspackages/bot/src/handlers/message/__tests__/xpHandler.test.tspackages/bot/src/handlers/message/autoModHandler.tspackages/bot/src/handlers/message/customCommandHandler.tspackages/bot/src/handlers/message/index.tspackages/bot/src/handlers/message/pipeline.tspackages/bot/src/handlers/message/spamHandler.tspackages/bot/src/handlers/message/types.tspackages/bot/src/handlers/message/xpHandler.tspackages/bot/src/handlers/messageHandler.ts
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
LucasSantana-Dev
added a commit
that referenced
this pull request
May 24, 2026
## Summary Replaces the 15+ positional arguments passed through the autoplay collector pipeline with a single `AutoplayContext` value object, improving call-site clarity and making the pipeline easier to extend. ### Changes - Introduce `AutoplayContext` interface in `autoplayContext.ts` - Update `candidateCollector`, `lastFmSeeder`, `spotifyRecommender`, `replenisher`, and `candidateFallback` to accept `AutoplayContext` instead of positional args - Fix missing `beforeEach` mock setup in `candidateCollector` tests - Remove leftover `.bak` file from refactor ### Why Positional argument lists of 15+ items are hard to read, easy to misorder, and brittle to extend. A named value object surfaces intent at every call site and isolates future additions to one interface definition. Part of the architecture refactor series (T1–T5): - T1 #979 ✅ — extract `MessagePipeline` - T2 #980 ✅ — introduce `IGuildAutomationRepository` - T3 #981 ✅ — `AutomationPlan` result type - T4 #982 ✅ — `GuildAutomationRepository` / `Orchestrator` split - T5 (this PR) — `AutoplayContext` value object
LucasSantana-Dev
added a commit
that referenced
this pull request
May 24, 2026
ADRs for the 5 refactors merged in PRs #979–#983 and the Phase 4 test reduction strategy. ## Decision records added - `2026-05-23-artist-suggestion-service.md` — ArtistSuggestionService extraction (#980) - `2026-05-23-autoplay-context-value-object.md` — AutoplayContext value object (#983) - `2026-05-23-bot-test-reduction-phase4-replacement-strategy.md` — Phase 4 test replacement strategy - `2026-05-23-guild-automation-orchestrator-repository-split.md` — GuildAutomationService split (#982) - `2026-05-23-message-pipeline-handler-chain.md` — MessagePipeline handler chain (#981) - `2026-05-23-recommendation-engine-single-entrypoint.md` — recommendTracks() single entrypoint (#979) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added architecture decision records documenting planned service refactoring, test optimization strategies, and API consolidation initiatives to improve system maintainability. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/LucasSantana-Dev/Lucky/pull/1040?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
LucasSantana-Dev
added a commit
that referenced
this pull request
May 25, 2026
## Summary - Bumps all package.json files from `2.14.1` → `2.15.0` - Populates `CHANGELOG.md` with everything since v2.14.1 - Adds ADR `docs/decisions/2026-05-24-ci-runtime-baseline-accepted.md` (CI runtime baseline: 3–4 min accepted, Jest sharding deferred) ### Changes included in this release **Added** - ServerLogs + ServerSettings UI pages (#965) - AutoMessages executor wiring into execution service (#950) **Changed** - 4 UI redesigns: Admin, Config, Login, ServersPage, CustomCommands, GuildAutomation, Spotify, LastFm (#967–#970) - 5 refactors: AutoplayContext VO (#983), GuildAutomationOrchestrator/Repository split (#982), MessagePipeline chain (#981), ArtistSuggestionService (#980), recommendTracks entrypoint (#979) **Fixed** — deploy CI gap sweep (A–F) - Gap A: hard-fail on OAuth 429 (#1045) - Gap B: surface async deploy via commit statuses (#1046) - Gap C: bot healthcheck polls Discord gateway, not Redis TCP (#1047) - Gap D: post error status on lock contention (#1052) - Gap E: add bot to required containers, remove dead unhealthy grep (#1054) - Gap F: wait for homelab-deploy completion on docker_rebuilt=true path (#1056) - lockfile-hash BuildKit cache key (#1016) - squash-merged release branch archive (#946) **Internal** - Phase 4 test cleanup — 93 tests removed (#956–#1035) - Pre-commit hooks: husky + lint-staged + tsc (#1007) - madge gate promoted to blocking - Dependabot routine bumps (#971–#977, #1042) > **Note:** #1054 (Gap E) may still be in CI — merge this PR after #1054 lands.
](https://app.coderabbit.ai/change-stack/LucasSantana-Dev/Lucky/pull/1040?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
This branch was successfully deployed
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.
Summary
Implements T3 of the architecture refactor: Split the 322-LOC messageHandler into a handler chain pattern with stop-signal support.
Changes
Design Decisions
Verification
Summary by CodeRabbit
Release Notes
New Features
Tests