Repository navigation
feat(bot): /bulk-kick — proof-of-pattern for the bulk-* command family - #1802
Conversation
📝 WalkthroughWalkthroughAdds a ChangesBulk kick moderation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Failed to generate code suggestions for PR |
|
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/bot/src/functions/moderation/batch/bulkKickExecutor.spec.ts (1)
70-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for checkpoint ordering and resume state.
These tests pass
jest.fn()asonProgress, so they do not verify that the callback is awaited beforemember.kick(). They also lack a resumed-job fixture with non-zero persisted counters, allowing both resumability regressions to pass unnoticed.🤖 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/functions/moderation/batch/bulkKickExecutor.spec.ts` around lines 70 - 122, Extend the BulkKickExecutor tests around execute to verify checkpoint ordering: use an onProgress callback that pauses, assert member.kick is not called until it resolves, then continue and assert the kick occurs. Add a resumed-job fixture with non-zero persisted progress/counters and assert execution starts after the checkpoint while preserving those counters and producing the correct final result.
🤖 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/functions/moderation/batch/bulkKickExecutor.ts`:
- Around line 78-90: The bulk kick flow must checkpoint each member before
performing the destructive action. In the batch execution method around
kickMember and onProgress, await onProgress with the current member cursor
before calling kickMember, then update tally and report post-action progress as
required by the existing BatchJobExecutor protocol, preserving accurate
processed, failed, skipped, and nextCursor values.
- Around line 54-66: The resumed batch execution must preserve previously
persisted outcome totals instead of resetting them. Update the tally
initialization in the executor flow around getById and resolveKickTargets to
seed kicked, skipped, and failed from the corresponding dbJob progress fields,
defaulting missing values to zero, so checkpoint() and the final summary remain
cumulative.
In `@packages/bot/src/functions/moderation/commands/bulkKick.ts`:
- Around line 124-139: The bulk kick command must handle a null result from
enqueueBatchJob before reporting success. Capture the enqueue result after
creating the job, and when enqueueing fails, transition the created job to the
existing retryable or failed state using the batch job service, then return an
error response instead of calling the success editReply; preserve the current
success path when enqueueing succeeds.
- Around line 29-33: Add preflight validation for the optional reason in the
bulk kick command before enqueueing any members, limiting its URL-encoded UTF-8
length to Discord’s 512-character audit-log maximum. Reject the command with an
appropriate user-facing error when exceeded, and preserve the existing
member.kick(reason) behavior for valid or absent reasons.
---
Nitpick comments:
In `@packages/bot/src/functions/moderation/batch/bulkKickExecutor.spec.ts`:
- Around line 70-122: Extend the BulkKickExecutor tests around execute to verify
checkpoint ordering: use an onProgress callback that pauses, assert member.kick
is not called until it resolves, then continue and assert the kick occurs. Add a
resumed-job fixture with non-zero persisted progress/counters and assert
execution starts after the checkpoint while preserving those counters and
producing the correct final result.
🪄 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: 12ca8ed7-ecd2-4800-a874-5a0461d58a37
📒 Files selected for processing (5)
packages/bot/src/functions/moderation/batch/bulkKickExecutor.spec.tspackages/bot/src/functions/moderation/batch/bulkKickExecutor.tspackages/bot/src/functions/moderation/commands/bulkKick.tspackages/bot/src/workers/batchJobWorker.spec.tspackages/bot/src/workers/batchJobWorker.ts
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Auto-approval blocked by 4 unresolved issues from previous reviews.
Re-trigger cubic
…tracking
Critical fixes to prevent double-kicks, data loss, and incorrect completion states:
1. bulkKick.ts: Check enqueueBatchJob return value — report failure to user if Redis
unavailable (P1: was claiming job queued even when Redis down).
2. bulkKickExecutor.ts: Move checkpoint BEFORE destructive kick step. Contract requires
nextCursor persisted before the destructive operation so resume never re-kicks an
already-processed member (P1: was checkpointing AFTER kick, risking double-kicks).
3. bulkKickExecutor.ts: Initialize tally with prior counts from dbJob on resume
(P2: was zeroing counts, losing prior progress counts across restarts).
4. batchJobWorker.ts: Distinguish cancelled/paused states from completed. Executor
returns { cancelled: true } or { paused: true }, but worker was marking all as
completed (P1: wrong status, blocks proper resumption).
Verification: tsc -p packages/bot/tsconfig.json --noEmit → 0 errors
All fixes crash-safe; ESLint errors are pre-existing violations unrelated to changes.
c6241ae to
75376fa
Compare
|
Review dispositions (commit
|
Add comprehensive tests for the three critical fixes in commit 75376fa: 1. bulkKick.spec.ts (NEW): Test enqueueBatchJob null-return path (Redis unavailable). When enqueue fails, user receives error reply; success path skipped. Covers all permission checks, dry-run, confirmation, and job creation paths. 2. batchJobWorker.spec.ts: Test completion state branches (cancelled/paused/completed). Executor returns { cancelled: true } or { paused: true }; worker now routes to correct mark* method instead of always marking completed. Added 3 branch tests. 3. bulkKickExecutor.spec.ts: Test resume-progress initialization and checkpoint ordering. On resume, tally seeded from dbJob.processedItems/skippedItems/failedItems (was zeroing them). Checkpoint (onProgress) called BEFORE kickMember for crash-safety. Coverage results (NEW lines only): - bulkKick.ts: 100% (statements, functions, lines); 89.28% (branch) - batchJobWorker.ts: 95.4% (statements, lines); 80% (functions); 70.58% (branch) - bulkKickExecutor.ts: 86.2% (statements); 89.28% (lines); 80.76% (branch) All tests green; SonarCloud quality gate ≥80% new code coverage now satisfied.
5c28e24 to
2894ff2
Compare
|
🤖 I have created a release *beep* *boop* --- <details><summary>2.35.0</summary> ## [2.35.0](v2.34.0...v2.35.0) (2026-07-13) ### Features * **bot:** /bulk-kick — proof-of-pattern for the bulk-* command family ([#1802](#1802)) ([1c11f3e](1c11f3e)) * **bot:** add lucky_bot_gateway_connected zombie-detection gauge ([#1774](#1774)) ([6cc5287](6cc5287)) * **bot:** playback progress bar in nowplaying/songinfo embed ([#1797](#1797)) ([1138b59](1138b59)) * **bot:** post server-count stats to Top.gg for listing visibility ([#1789](#1789)) ([e8ca6b9](e8ca6b9)) * **bot:** temporary support ticket channels (/ticket) ([#1803](#1803)) ([49601a1](49601a1)) * **live-notif:** youtube polling, message ttl cleanup, api backoff ([#1762](#1762)) ([a99b85a](a99b85a)) * **remind:** channel and role broadcast reminders ([#1767](#1767)) ([#1807](#1807)) ([83ade79](83ade79)) ### Bug Fixes * **backend:** artist suggestions 503 not 500 on upstream timeout ([#1787](#1787)) ([eadc20e](eadc20e)) * **bot:** guard skipReason telemetry against null prisma client ([#1773](#1773)) ([d73421e](d73421e)) * **ci:** stop auto-update workflow racing on merge push ([#1811](#1811)) ([2ddd202](2ddd202)) * **csp:** allow Cloudflare Insights beacon in script-src/connect-src ([#1788](#1788)) ([f20c4cf](f20c4cf)) * **deps:** bump eslint in lock to satisfy npm@12 ci (unblock release) ([#1809](#1809)) ([68fd0be](68fd0be)) * paginate bulk-move message fetch to respect discord api limit ([#1776](#1776)) ([5b5d2fc](5b5d2fc)) * **weekly-digest:** trigger on Sunday and add new-guides RSS section ([#1761](#1761)) ([f429fc6](f429fc6)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).



Refs #1765 (the first proof-of-pattern command; the rest of the family — bulk-ban/warn/add-role/remove-role/purge — follow this template in later PRs).
What
A
/bulk-kick role:@X reason:command that kicks every non-bot member holding a role, via the existing batch-job infra. Targeting is role-based (confirmed design — the existing message-orientedScopeConfigdoesn't cover member selection).How (reuses the 90%-built batch infra)
BulkKickExecutor— mirrorschannelMoveExecutor's cursor/cancellation/pause contract. Non-bot role members sorted by id; cursor = last-kicked id (resumes cleanly after a restart).member.kickableguard; Discord codes 10007/50013 → skip, other errors → failed./bulk-kickcommand —checkBatchPermissions('bulk_kick')(matrix entry already existed) →showBatchConfirmation(destructive gate, with adry_run) →batchJobService.create(options:{roleId,reason}) →enqueueBatchJob. One registry line inbatchJobWorker.ts.role.members.size, no message sampling) — so it inherently avoids the fix(bot): bulk-move-messages fetch limit 500 exceeds Discord's 100-message cap #1763 100-item-fetch trap the spec warned against copying.Verify
resolveKickTargets/kickMember/summaryto stay under the complexity ceiling)Safety
Destructive by design; gated behind
KickMembersdefault-member-permission + the confirmation dialog +dry_run. Owner/higher-role members are skipped (kickable).Summary by cubic
Adds a new /bulk-kick command that kicks all non-bot members with a selected role via the batch job system. Crash-safe, resumable, and pause/cancel-aware; first template for the bulk-* family and meets core requirements of #1765.
New Features
BulkKickExecutor: ID-sorted targets; resumes via cursor; skips bots and non-kickable; treats Discord 10007/50013 as skip; logs other errors; emits progress withnextCursor; pauses if the client disconnects.Bug Fixes
nextCursorbefore kicks and restore tallies on resume.Written for commit 353aa33. Summary will update on new commits.
Summary by CodeRabbit
New Features
bulk-kickmoderation command to remove non-bot members with a selected role.Bug Fixes
Tests