feat: /review code review requests with claim workflow (#49)#114
feat: /review code review requests with claim workflow (#49)#114BillChirico merged 3 commits intomainfrom
Conversation
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughAdds a code review feature: DB schema, slash command (/review with request/list/complete), claim button interactions (thread creation), stale-expiry scheduler, review handler module, config/dashboard entries, tests, and event/scheduler integration. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary — 6 Issues Found
🔴 Critical (2)
-
TOCTOU race condition in claim flow (
src/modules/reviewHandler.js:158-212)
The SELECT and UPDATE are separate queries with no locking. Two users clicking "Claim" simultaneously can both pass the status check, resulting in the second overwriting the first. Fix: use atomicUPDATE ... WHERE status = 'open' RETURNING *. -
staleAfterDaysconfig is ignored (src/modules/reviewHandler.js:272)
The config exposesstaleAfterDays(editable from dashboard), but the query hardcodesINTERVAL '7 days'. Users changing the setting see no effect.
🟡 Warning (3)
-
No URL validation (
src/commands/review.js:121)
Theurlfield is stored without validation. Validate withnew URL(url)at minimum. -
Misleading error for unclaimed reviews (
src/commands/review.js:250-268)
/review completeon an open/stale review shows "Only the assigned reviewer can complete" — confusing when there's no reviewer. Add explicitopen/stalestatus checks with appropriate messages. -
Missing
ReviewConfigtype (web/src/types/config.ts:175)
BotConfiginterface doesn't include areviewproperty, so all dashboarddraftConfig.review.*accesses are untyped.
🔵 Nitpick (1)
- Missing
guild_idscope in complete UPDATE (src/commands/review.js:271-277)
The UPDATE usesWHERE id = $2withoutAND guild_id. Defense-in-depth: add guild scoping.
Documentation Gap
Per AGENTS.md rules, new commands/modules/tables must be documented:
AGENTS.mdKey Files table: missing entries forsrc/commands/review.js,src/modules/reviewHandler.jsAGENTS.mdDatabase Tables: missingreviewstable entryREADME.md: no mention of/reviewcommand
See inline comments for detailed fix suggestions.
|
| Filename | Overview |
|---|---|
| src/commands/review.js | Added /review command with request/list/complete subcommands; URL validation needs protocol restriction, missing error handling for message send failures |
| src/modules/reviewHandler.js | Implemented review embed builders, atomic claim logic, and per-guild stale expiry; properly handles race conditions and per-guild config |
| migrations/010_reviews.cjs | Created reviews table with proper schema, indexes, and status constraints |
| tests/commands/review.test.js | Comprehensive test coverage (39 tests) covering all command paths, edge cases, and error scenarios |
Sequence Diagram
sequenceDiagram
participant User
participant Command as /review command
participant DB as PostgreSQL
participant Handler as reviewHandler
participant Scheduler
participant Channel
Note over User,Channel: Review Request Flow
User->>Command: /review request [url] [description]
Command->>DB: INSERT review (status='open')
DB-->>Command: review row with id
Command->>Handler: buildReviewEmbed(review)
Handler-->>Command: embed + claim button
Command->>Channel: send embed with button
Channel-->>Command: message with id
Command->>DB: UPDATE review SET message_id
Command-->>User: ✅ Request #N posted
Note over User,Channel: Claim Flow (Button Click)
User->>Handler: click "Claim" button
Handler->>DB: SELECT review (self-claim check)
Handler->>DB: UPDATE SET status='claimed'<br/>WHERE status='open' (atomic)
alt Claim successful (rowCount=1)
DB-->>Handler: claimed review
Handler->>Channel: startThread (optional)
Handler->>Handler: updateReviewMessage (disable button)
Handler-->>User: ✅ Claimed #N
else Already claimed/completed (rowCount=0)
DB-->>Handler: no rows updated
Handler-->>User: ❌ No longer available
end
Note over User,Channel: Complete Flow
User->>Command: /review complete [id]
Command->>DB: SELECT review
alt Status and permission checks pass
Command->>DB: UPDATE SET status='completed'
Command->>DB: INSERT/UPDATE reputation (XP reward)
Command->>Handler: updateReviewMessage
Command-->>User: ✅ Completed +50 XP
else Invalid state or permissions
Command-->>User: ❌ Error message
end
Note over Scheduler,Channel: Stale Review Expiry (60s poll)
Scheduler->>DB: SELECT DISTINCT guild_id<br/>WHERE status='open'
loop For each guild
Scheduler->>DB: UPDATE SET status='stale'<br/>WHERE created_at > staleDays
DB-->>Scheduler: stale reviews
Scheduler->>Channel: send nudge message
Scheduler->>Handler: updateReviewMessage
end
Last reviewed commit: edb92b7
There was a problem hiding this comment.
Pull request overview
Adds a /review workflow to the Discord bot for requesting peer code reviews, claiming them via a button, completing them (optionally awarding XP), and expiring stale requests; includes dashboard config support and test coverage.
Changes:
- Introduces a
reviewsDB table and new/reviewcommand with request/list/complete flows. - Adds claim-button interaction handling + stale-expiry logic wired into the event system and scheduler.
- Extends config (backend allowlist + dashboard UI) and adds comprehensive Vitest coverage.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| migrations/010_reviews.cjs | Adds a migration intended to create the reviews table and indexes. |
| src/commands/review.js | Implements /review request, /review list, and /review complete (incl. optional XP award). |
| src/modules/reviewHandler.js | Implements embed/button builders, claim handler, message updater, and stale expiry logic. |
| src/modules/events.js | Registers an interactionCreate handler for review_claim_* buttons. |
| src/modules/scheduler.js | Calls expireStaleReviews from the 60s scheduler loop. |
| config.json | Adds default review config and permissions entry. |
| src/api/utils/configAllowlist.js | Adds review to SAFE_CONFIG_KEYS for API config editing. |
| web/src/components/dashboard/config-editor.tsx | Adds dashboard controls for Code Reviews (enabled/channel/stale days/xp reward). |
| tests/commands/review.test.js | Adds tests for /review command behavior + claim handler + stale expiry + embed/button builders. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@migrations/010_reviews.cjs`:
- Around line 23-30: The schema currently allows status and created_at to be
NULL despite the CHECK and DEFAULT; update the migration so the status column is
defined as NOT NULL (e.g., status TEXT DEFAULT 'open' NOT NULL CHECK (status IN
('open','claimed','completed','stale'))) and make created_at non-nullable as
well (created_at TIMESTAMPTZ DEFAULT NOW() NOT NULL); keep
claimed_at/completed_at nullable if they are set later by lifecycle actions.
Ensure you update the definitions for the status and created_at columns in the
migration (referencing the status column and created_at column names) so the DB
enforces non-null values.
- Around line 33-35: Add a global index to support the stale-expiry query by
creating an index on reviews(status, created_at) (e.g., name it
idx_reviews_status_created_at) in the up migration so queries that filter by
status and created_at can use an index rather than the current guild-prefixed
indexes (idx_reviews_guild, idx_reviews_status); also add the corresponding DROP
INDEX IF EXISTS idx_reviews_status_created_at in the down migration to cleanly
roll back the change.
In `@src/commands/review.js`:
- Around line 87-103: The guild-only check must occur before reading
guild-specific config: move the interaction.guildId validation to run before
calling getConfig(interaction.guildId) and using guildConfig.review; update the
block around getConfig/guildConfig so that if (!interaction.guildId) you call
safeEditReply(interaction, { content: '❌ This command can only be used in a
server.' }) and return immediately, then retrieve guildConfig and proceed to
check guildConfig.review?.enabled, and keep the existing pool check and
safeEditReply usage as-is (refer to getConfig, guildConfig, interaction.guildId,
and safeEditReply to locate the code).
- Around line 125-160: The review row is being inserted before sending the
Discord message which can leave orphaned DB records if the outbound send fails,
and the code uses targetChannel.send instead of the required safeSend
sanitization; modify the flow in review.js so you first build the embed and
components (buildReviewEmbed, buildClaimButton), then send via
safeSend(targetChannel, { embeds: [embed], components: [row] }) and only after a
successful send insert (or update) the database with message_id and channel_id;
alternatively perform the INSERT and the send inside a DB transaction or ensure
rollback/delete of the inserted review row if safeSend throws—replace direct
targetChannel.send calls with safeSend and ensure the UPDATE/INSERT logic
(pool.query for INSERT INTO reviews ... RETURNING * and the subsequent UPDATE
reviews SET ...) reflects the reordered/transactional flow.
- Around line 251-295: The current flow reads review.status then later updates
it and separately awards XP, which allows a TOCTOU race; change to perform the
status update and XP award atomically: wrap the UPDATE of reviews (currently the
pool.query that sets status = 'completed', completed_at = NOW(), feedback = $1
RETURNING *) and the reputation INSERT/UPDATE in a single transaction (BEGIN ...
COMMIT/ROLLBACK), modify the UPDATE to include a status predicate (WHERE id = $2
AND status != 'completed') and check the RETURNING rows (the existing
updated/completedReview variable) so you only call updateReviewMessage and
perform the XP INSERT/UPDATE when the UPDATE actually returned a row, and
rollback on error; reference the existing pool.query that updates reviews, the
completedReview variable, and the XP award block using
guildConfig.review?.xpReward and guildConfig.reputation?.enabled.
In `@src/modules/events.js`:
- Around line 362-368: Before calling handleReviewClaim in the
Events.InteractionCreate handler, gate the logic by the guild's review-module
config so disabled guilds are skipped: when receiving a button interaction (the
block using interaction.isButton() and
interaction.customId.startsWith('review_claim_')), look up the guild
configuration (e.g., via the existing getGuildConfig / config store using
interaction.guildId) and return early if the review module is not enabled (check
config.review.enabled or equivalent) instead of proceeding to await
handleReviewClaim(interaction).
In `@src/modules/reviewHandler.js`:
- Around line 267-273: The expiry logic is hardcoded to 7 days; update the SQL
and message text to use the guild config value (staleAfterDays) instead. In the
block that calls pool.query to mark stale reviews (the UPDATE query currently
using NOW() - INTERVAL '7 days'), replace the literal interval with a
parameterized interval using make_interval(days => $1) and pass the guild's
staleAfterDays value; likewise change any user-facing text (the messages around
lines where you reference "7 days") to interpolate staleAfterDays instead of the
hardcoded 7 so the same config is honored. Ensure you reference the existing
staleAfterDays variable from the guild/review config and keep pool.query
parameterization to avoid injection.
- Around line 289-291: The code currently skips processing when reviewChannelId
is falsy (const reviewChannelId = guildConfig.review?.channelId; if
(!reviewChannelId) continue;), which leaves embeds/buttons stale for guilds
using the default channel; instead remove the continue and compute a fallback
channel id (e.g., const reviewChannelId = guildConfig.review?.channelId ||
guild.systemChannelId) so the update logic still runs against the guild's
system/default channel; apply the same change to the second occurrence noted
around the review update block (the similar if (!reviewChannelId) continue at
304-307) so both paths update embeds/buttons using the fallback channel.
- Around line 206-212: The current unconditional UPDATE that sets
status='claimed' (the pool.query call updating reviews with reviewer_id =
interaction.user.id and reviewId) can be raced by concurrent claimers; change
the UPDATE to include the precondition in its WHERE clause (e.g., WHERE id = $2
AND status = 'available') so the transition is atomic, then check the returned
rows (`updated` from pool.query) to detect zero rows and handle the "already
claimed" case (notify the user or abort) instead of assuming success.
In `@web/src/components/dashboard/config-editor.tsx`:
- Around line 1434-1450: The change handlers for the number inputs (the onChange
callbacks that use parseNumberInput and call setDraftConfig to update
draftConfig.review.staleAfterDays and draftConfig.review.xpReward) only enforce
minimums via parseNumberInput but do not clamp the upper bounds; modify those
handlers to clamp the parsed value to the declared max (90 for staleAfterDays,
1000 for xpReward) before calling setDraftConfig so the state cannot exceed the
input's max even if the DOM constraint is bypassed.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
config.jsonmigrations/010_reviews.cjssrc/api/utils/configAllowlist.jssrc/commands/review.jssrc/modules/events.jssrc/modules/reviewHandler.jssrc/modules/scheduler.jstests/commands/review.test.jsweb/src/components/dashboard/config-editor.tsx
📜 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). (3)
- GitHub Check: Agent
- GitHub Check: Greptile Review
- GitHub Check: claude-review
🧰 Additional context used
📓 Path-based instructions (7)
**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
**/*.js: Use ESM modules only — import/export, never require()
Always use node: protocol for Node.js builtin imports (e.g., import { readFileSync } from 'node:fs')
Files:
src/api/utils/configAllowlist.jssrc/modules/reviewHandler.jssrc/modules/scheduler.jssrc/modules/events.jstests/commands/review.test.jssrc/commands/review.js
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,tsx}: Always use semicolons
Use single quotes — enforced by Biome
Use 2-space indentation — enforced by Biome
Files:
src/api/utils/configAllowlist.jssrc/modules/reviewHandler.jssrc/modules/scheduler.jssrc/modules/events.jstests/commands/review.test.jssrc/commands/review.jsweb/src/components/dashboard/config-editor.tsx
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: NEVER use console.log, console.warn, console.error, or any console.* method in src/ files — always use Winston logger instead: import { info, warn, error } from '../logger.js'
Pass structured metadata to Winston logger: info('Message processed', { userId, channelId })
Use custom error classes from src/utils/errors.js and always log errors with context before re-throwing
Use getConfig(guildId?) from src/modules/config.js to read config; use setConfigValue(path, value, guildId?) to update at runtime
Use safeSend() utility for outgoing Discord messages to sanitize mentions and enforce allowedMentions
Use splitMessage() utility for messages exceeding Discord's 2000-character limit
Add tests for all new code with mandatory 80% coverage threshold on statements, branches, functions, and lines; run pnpm test before every commit
Files:
src/api/utils/configAllowlist.jssrc/modules/reviewHandler.jssrc/modules/scheduler.jssrc/modules/events.jssrc/commands/review.js
src/modules/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/modules/*.js: Register event handlers in src/modules/events.js by importing handler functions and calling client.on() with config parameter
Check config.yourModule.enabled before processing in module event handlers
Prefer per-request getConfig() pattern in new modules over reactive onConfigChange() wiring; only add onConfigChange() listeners for stateful resources that cannot re-read config on each use
Files:
src/modules/reviewHandler.jssrc/modules/scheduler.jssrc/modules/events.js
src/commands/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/commands/*.js: Use parseDuration() from src/utils/duration.js for duration-based commands (timeout, tempban, slowmode); enforce Discord duration caps (timeouts max 28 days, slowmode max 6 hours)
Create slash commands by exporting data (SlashCommandBuilder) and execute() function from src/commands/*.js; export adminOnly = true for mod-only commands; commands are auto-discovered on startup
Files:
src/commands/review.js
web/**/*.{tsx,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use next/image Image component with appropriate layout and sizing props in Next.js components
Files:
web/src/components/dashboard/config-editor.tsx
web/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use Zustand store (zustand) for state management in React components; implement fetch-on-demand pattern in stores
Files:
web/src/components/dashboard/config-editor.tsx
🧠 Learnings (7)
📚 Learning: 2026-02-27T16:24:15.054Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-27T16:24:15.054Z
Learning: Applies to src/commands/*mod*.js : Moderation commands must follow the shared pattern: deferReply({ ephemeral: true }), validate inputs, sendDmNotification(), execute Discord action, createCase(), sendModLogEmbed(), checkEscalation()
Applied to files:
src/modules/reviewHandler.jssrc/commands/review.js
📚 Learning: 2026-02-27T16:24:15.054Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-27T16:24:15.054Z
Learning: Applies to src/index.js : The tempban scheduler runs on a 60s interval and catches up on missed unbans after restart; started in index.js startup and stopped in graceful shutdown
Applied to files:
src/modules/scheduler.js
📚 Learning: 2026-02-27T16:24:15.054Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-27T16:24:15.054Z
Learning: Applies to src/modules/*.js : Register event handlers in src/modules/events.js by importing handler functions and calling client.on() with config parameter
Applied to files:
src/modules/events.js
📚 Learning: 2026-02-27T16:24:15.054Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-27T16:24:15.054Z
Learning: Applies to src/**/*.js : Add tests for all new code with mandatory 80% coverage threshold on statements, branches, functions, and lines; run pnpm test before every commit
Applied to files:
tests/commands/review.test.js
📚 Learning: 2026-02-27T16:24:15.054Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-27T16:24:15.054Z
Learning: Applies to src/commands/*.js : Create slash commands by exporting data (SlashCommandBuilder) and execute() function from src/commands/*.js; export adminOnly = true for mod-only commands; commands are auto-discovered on startup
Applied to files:
src/commands/review.js
📚 Learning: 2026-02-27T16:24:15.054Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-27T16:24:15.054Z
Learning: Applies to src/commands/*.js : Use parseDuration() from src/utils/duration.js for duration-based commands (timeout, tempban, slowmode); enforce Discord duration caps (timeouts max 28 days, slowmode max 6 hours)
Applied to files:
src/commands/review.js
📚 Learning: 2026-02-27T16:24:15.054Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-27T16:24:15.054Z
Learning: Applies to src/**/*.js : Use getConfig(guildId?) from src/modules/config.js to read config; use setConfigValue(path, value, guildId?) to update at runtime
Applied to files:
web/src/components/dashboard/config-editor.tsx
🧬 Code graph analysis (4)
migrations/010_reviews.cjs (2)
src/commands/review.js (4)
pool(95-95)pool(125-130)pool(191-191)pool(237-240)src/modules/reviewHandler.js (3)
pool(151-151)pool(158-161)pool(263-263)
src/modules/scheduler.js (1)
src/modules/reviewHandler.js (1)
expireStaleReviews(262-312)
src/modules/events.js (2)
src/modules/reviewHandler.js (1)
handleReviewClaim(147-255)src/utils/safeSend.js (1)
safeReply(138-145)
web/src/components/dashboard/config-editor.tsx (2)
web/src/components/ui/card.tsx (3)
Card(76-76)CardContent(81-81)CardTitle(79-79)src/modules/config.js (1)
num(874-874)
🔇 Additional comments (4)
src/api/utils/configAllowlist.js (1)
25-25: Allowlist update is correct.Adding
'review'toSAFE_CONFIG_KEYScleanly enables API access for the new review config section.src/modules/scheduler.js (1)
185-186: Good scheduler integration for stale-review housekeeping.Hooking
expireStaleReviews(client)into the existing 60s scheduler cycle is the right place operationally.config.json (1)
153-155: Config additions are coherent with the new review workflow.
allowedCommands.reviewand the newreviewdefaults (enabled/channelId/staleAfterDays/xpReward) are consistent with the command + dashboard integration.Also applies to: 205-210
tests/commands/review.test.js (1)
242-792: Strong workflow coverage for the new review feature.The suite exercises primary command paths, claim edge cases, stale expiry behavior, and rendering helpers in a way that meaningfully reduces regression risk.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary — 10 Issues Found
Good progress on fixing the atomic claim, stale config, URL validation, and error messages from the first round. However, several issues remain.
🔴 Critical (1)
- Migration 010 uses wrong API — will crash at runtime (
migrations/010_reviews.cjs)
Usespool.query()but node-pg-migrate passes aMigrationBuilder(not a Pool). All other migrations useexports.up = (pgm) => { pgm.sql(...) }. The migration will throwTypeError: pgm.query is not a function. Also,statusandcreated_atcolumns should beNOT NULL.
🟡 Warning (7)
-
targetChannel.send()bypassessafeSend()(src/commands/review.js:176)
Per AGENTS.md, all outgoing Discord messages must usesafeSend(). Also creates orphaned DB rows if send fails. -
Missing
review.enabledconfig gate in claim handler (src/modules/events.js:362-367)
Per AGENTS.md: "Checkconfig.yourModule.enabledbefore processing." Disabling reviews won't stop claim buttons from working. -
/review completeUPDATE lacksguild_idscope andstatuspredicate (src/commands/review.js:308-315)
MissingAND guild_id = $3 AND status = 'claimed'— concurrent completion can award XP twice. -
Guild validation comes after config check (
src/commands/review.js:87-104)
In DMs,getConfig(null)runs before the guild check, showing "reviews not enabled" instead of "server only." -
Stale embed updates skipped when no
channelId(src/modules/reviewHandler.js:303)
continuealso skipsupdateReviewMessagecalls, leaving stale embeds with active "Claim" buttons. -
Invalid
customIdsilently swallowed (src/modules/reviewHandler.js:148-149)
Returns without acknowledging the interaction — user sees Discord's "This interaction failed." -
Missing
ReviewConfigtype inBotConfig(web/src/types/config.ts:165-175)
BotConfiginterface doesn't includereview— all dashboarddraftConfig.review.*accesses are untyped.
🔵 Nitpick (2)
-
parseNumberInputmissing max constraint (web/src/components/dashboard/config-editor.tsx:1437,1449)
Only passesmintoparseNumberInputbut notmax(90 for staleAfterDays, 1000 for xpReward). -
Documentation gaps (AGENTS.md, README.md)
Per AGENTS.md: "Keep docs up to date — this is non-negotiable." Missing entries forreview.js,reviewHandler.jsin Key Files table,reviewstable in Database Tables, and/reviewin README commands.
AI prompt to fix all issues
Fix the following issues in the volvox-bot repo on branch feat/review-command:
1. migrations/010_reviews.cjs — Rewrite to use node-pg-migrate MigrationBuilder API:
- Change `async function up(pool)` to `exports.up = (pgm) => { pgm.sql(...) }`
- Change `async function down(pool)` to `exports.down = (pgm) => { pgm.sql(...) }`
- Remove `module.exports = { up, down }`
- Add NOT NULL to `status` column (line 23): `status TEXT NOT NULL DEFAULT 'open' CHECK (...)`
- Add NOT NULL to `created_at` column (line 28): `created_at TIMESTAMPTZ NOT NULL DEFAULT NOW()`
2. src/commands/review.js lines 87-104 — Move `if (!interaction.guildId)` check BEFORE `getConfig(interaction.guildId)` call.
3. src/commands/review.js line 176 — Replace `targetChannel.send(...)` with `safeSend(targetChannel, ...)`.
- Add `safeSend` to the import at line 18: `import { safeEditReply, safeSend } from '../utils/safeSend.js'`
- Wrap in try/catch: on failure, delete the orphaned review row with `pool.query('DELETE FROM reviews WHERE id = $1', [review.id])` and reply with error.
4. src/commands/review.js lines 308-315 — Change the UPDATE query to:
`UPDATE reviews SET status = 'completed', completed_at = NOW(), feedback = $1 WHERE id = $2 AND guild_id = $3 AND status = 'claimed' RETURNING *`
Pass `[feedback ?? null, reviewId, interaction.guildId]` as params.
After the query, if `updated.length === 0`, reply with error and return early (before XP award).
Remove the now-redundant `if (review.status === 'completed')` check above since the atomic UPDATE handles it.
5. src/modules/events.js lines 361-367 — In registerReviewClaimHandler, after the customId check, add:
`if (!interaction.guildId) return;`
`const guildConfig = getConfig(interaction.guildId);`
`if (!guildConfig.review?.enabled) { await safeReply(interaction, { content: '❌ Code reviews are currently disabled.', ephemeral: true }).catch(() => {}); return; }`
6. src/modules/reviewHandler.js lines 148-149 — When `Number.isNaN(reviewId)`, instead of bare `return`, call `await safeReply(interaction, { content: '❌ Invalid review ID.', ephemeral: true })` before returning.
7. src/modules/reviewHandler.js line 303 — Replace `if (!reviewChannelId) continue;` with conditional logic that skips the nudge but still runs the `updateReviewMessage` loop (lines 317-320).
8. web/src/types/config.ts — Add a `ReviewConfig` interface with `enabled: boolean`, `channelId: string | null`, `staleAfterDays: number`, `xpReward: number` and add `review?: ReviewConfig` to the `BotConfig` interface.
9. web/src/components/dashboard/config-editor.tsx line 1437 — Change `parseNumberInput(e.target.value, 1)` to `parseNumberInput(e.target.value, 1, 90)`. Line 1449 — Change `parseNumberInput(e.target.value, 0)` to `parseNumberInput(e.target.value, 0, 1000)`.
10. AGENTS.md — Add to Key Files table:
- `src/commands/review.js` | Slash command — `/review request`, `/review list`, `/review complete`
- `src/modules/reviewHandler.js` | Review embed builders, claim handler, stale review expiry
Add to Database Tables:
- `reviews` | Code review requests — one row per review per guild, tracks status lifecycle (open→claimed→completed/stale)
Run tests after all changes: pnpm test
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * @param {import('pg').Pool} pool | ||
| */ | ||
| async function up(pool) { | ||
| await pool.query(` | ||
| CREATE TABLE IF NOT EXISTS reviews ( | ||
| id SERIAL PRIMARY KEY, | ||
| guild_id TEXT NOT NULL, | ||
| requester_id TEXT NOT NULL, | ||
| reviewer_id TEXT, | ||
| url TEXT NOT NULL, | ||
| description TEXT NOT NULL, | ||
| language TEXT, | ||
| status TEXT DEFAULT 'open' CHECK (status IN ('open', 'claimed', 'completed', 'stale')), | ||
| message_id TEXT, | ||
| channel_id TEXT, | ||
| thread_id TEXT, | ||
| feedback TEXT, | ||
| created_at TIMESTAMPTZ DEFAULT NOW(), | ||
| claimed_at TIMESTAMPTZ, | ||
| completed_at TIMESTAMPTZ | ||
| ); | ||
|
|
||
| CREATE INDEX IF NOT EXISTS idx_reviews_guild ON reviews(guild_id); | ||
| CREATE INDEX IF NOT EXISTS idx_reviews_status ON reviews(guild_id, status); | ||
| `); |
There was a problem hiding this comment.
This migration doesn’t follow the node-pg-migrate signature used by the rest of the repo (exports.up/exports.down with a MigrationBuilder pgm). With the current async function up(pool) + module.exports = { up, down }, pnpm migrate (node-pg-migrate) will not execute this migration.
| SET status = 'stale' | ||
| WHERE status = 'open' | ||
| AND guild_id = $1 | ||
| AND created_at < NOW() - ($2 || ' days')::INTERVAL |
There was a problem hiding this comment.
PostgreSQL interval construction here is invalid for an integer parameter: ($2 || ' days')::INTERVAL will error because || is text concatenation and $2 is numeric. Use make_interval(days => $2) or cast first (e.g. ($2::text || ' days')::interval) to avoid the scheduler throwing every poll loop.
| AND created_at < NOW() - ($2 || ' days')::INTERVAL | |
| AND created_at < NOW() - make_interval(days => $2) |
| * Basic URL validity check. | ||
| * | ||
| * @param {string} str | ||
| * @returns {boolean} | ||
| */ | ||
| function isValidUrl(str) { | ||
| try { | ||
| new URL(str); | ||
| return true; |
There was a problem hiding this comment.
URL validation currently accepts any protocol that new URL() parses (e.g. javascript:). Other commands in this repo restrict to http:/https:; doing the same here prevents unsafe/non-clickable schemes from being stored and embedded.
| * Basic URL validity check. | |
| * | |
| * @param {string} str | |
| * @returns {boolean} | |
| */ | |
| function isValidUrl(str) { | |
| try { | |
| new URL(str); | |
| return true; | |
| * Basic HTTP/HTTPS URL validity check. | |
| * | |
| * Attempts to construct a URL object and ensures that the resulting URL | |
| * uses either the HTTP or HTTPS protocol. This prevents unsafe or | |
| * non-clickable schemes (e.g., `javascript:`, `data:`, `file:`) from | |
| * being treated as valid review URLs. | |
| * | |
| * @param {string} str - The URL string to validate. | |
| * @returns {boolean} `true` if the URL is syntactically valid and uses | |
| * an allowed protocol (`http:` or `https:`); otherwise, `false`. | |
| */ | |
| function isValidUrl(str) { | |
| try { | |
| const parsedUrl = new URL(str); | |
| // Only allow HTTP and HTTPS protocols for review URLs. | |
| return parsedUrl.protocol === 'http:' || parsedUrl.protocol === 'https:'; |
| const subcommand = interaction.options.getSubcommand(); | ||
|
|
||
| if (subcommand === 'request') { | ||
| await handleRequest(interaction, pool, guildConfig); | ||
| } else if (subcommand === 'list') { | ||
| await handleList(interaction, pool); | ||
| } else if (subcommand === 'complete') { | ||
| await handleComplete(interaction, pool, guildConfig); | ||
| } |
There was a problem hiding this comment.
execute() dispatches to subcommand handlers without a try/catch, so any DB/Discord error will bubble and can leave the interaction without a final response. Other commands (e.g. /showcase, /snippet) wrap handler execution and send a generic failure message; mirroring that pattern here will improve reliability.
| // Gate on review feature being enabled for this guild | ||
| const guildConfig = getConfig(interaction.guildId); | ||
| if (!guildConfig.review?.enabled) return; |
There was a problem hiding this comment.
When the review feature is disabled, this handler returns without acknowledging the button interaction. That will cause Discord to show “This interaction failed” to the user if a stale claim button is clicked after disabling. Consider replying ephemerally with a short ‘reviews are disabled’ message (or deferUpdate) instead of returning silently.
| // Gate on review feature being enabled for this guild | |
| const guildConfig = getConfig(interaction.guildId); | |
| if (!guildConfig.review?.enabled) return; | |
| // Gate on review feature being enabled for this guild. If disabled, | |
| // acknowledge the interaction so the user does not see a generic | |
| // "This interaction failed" message on Discord. | |
| const guildConfig = getConfig(interaction.guildId); | |
| if (!guildConfig.review?.enabled) { | |
| if (!interaction.replied && !interaction.deferred) { | |
| try { | |
| await safeReply(interaction, { | |
| content: '⚠️ Reviews are currently disabled in this server.', | |
| ephemeral: true, | |
| }); | |
| } catch { | |
| // Best-effort only; ignore failures when sending the notice. | |
| } | |
| } | |
| return; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/commands/review.js (3)
87-104:⚠️ Potential issue | 🟡 MinorValidate guild context before config gating.
In DMs (
interaction.guildId === null),getConfig(null)returns global config. If globalreview.enabledisfalse, users see "reviews not enabled" instead of the more accurate "server-only" message. Move the guildId check first for clearer error messaging.🔧 Suggested reorder
export async function execute(interaction) { await interaction.deferReply({ ephemeral: true }); + if (!interaction.guildId) { + await safeEditReply(interaction, { content: '❌ This command can only be used in a server.' }); + return; + } + const guildConfig = getConfig(interaction.guildId); if (!guildConfig.review?.enabled) { await safeEditReply(interaction, { content: '❌ Code reviews are not enabled on this server.', }); return; } const pool = getPool(); if (!pool) { await safeEditReply(interaction, { content: '❌ Database is not available.' }); return; } - if (!interaction.guildId) { - await safeEditReply(interaction, { content: '❌ This command can only be used in a server.' }); - return; - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/review.js` around lines 87 - 104, Check interaction.guildId first and return the "server-only" message before calling getConfig or inspecting guildConfig.review?.enabled; specifically, move the existing null/undefined check of interaction.guildId to run prior to calling getConfig(interaction.guildId) and before the guildConfig.review?.enabled guard so the command responds with the correct "This command can only be used in a server." via safeEditReply when invoked in DMs.
309-342:⚠️ Potential issue | 🔴 CriticalMake completion atomic to prevent double XP award.
The
UPDATEat line 309 lacks astatus = 'claimed'predicate. Two concurrent/review completecalls can both pass the earlier status check and both succeed, awarding XP twice.🐛 Proposed atomic update
- // Update status to completed - const { rows: updated } = await pool.query( - `UPDATE reviews - SET status = 'completed', completed_at = NOW(), feedback = $1 - WHERE id = $2 - RETURNING *`, - [feedback ?? null, reviewId], - ); - - const completedReview = updated[0]; + // Atomic update: only succeeds if still 'claimed' and owned by this reviewer + const { rows: updated } = await pool.query( + `UPDATE reviews + SET status = 'completed', completed_at = NOW(), feedback = $1 + WHERE id = $2 + AND guild_id = $3 + AND reviewer_id = $4 + AND status = 'claimed' + RETURNING *`, + [feedback ?? null, reviewId, interaction.guildId, interaction.user.id], + ); + + if (updated.length === 0) { + await safeEditReply(interaction, { + content: `❌ Review **#${reviewId}** could not be completed. It may have already been completed or is no longer claimable.`, + }); + return; + } + + const completedReview = updated[0];With the atomic update, you can also remove the earlier status and reviewer_id checks (lines 274-306) as they become redundant, simplifying the flow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/review.js` around lines 309 - 342, The UPDATE that marks a review completed must be made atomic by adding the prior-state predicate so only a row currently in the expected state is transitioned and returned; modify the SQL in the pool.query that sets status = 'completed' to include WHERE id = $2 AND status = 'claimed' (and optionally reviewer_id = $3 if you require the claimer match interaction.user.id), then use the returned row (completedReview) to decide whether to call updateReviewMessage and to award XP; if no row is returned, treat it as already-claimed/completed and skip XP awarding/notification to avoid double credit.
176-183:⚠️ Potential issue | 🟠 MajorUse
safeSend()and handle send failures to prevent orphaned DB records.Line 176 uses
targetChannel.send()directly, bypassing the requiredsafeSend()utility. Additionally, if the send fails after theINSERT, an orphaned review record remains in the database.As per coding guidelines: "Use safeSend() utility for outgoing Discord messages to sanitize mentions and enforce allowedMentions."
🐛 Proposed fix
+import { safeEditReply, safeSend } from '../utils/safeSend.js'; -import { safeEditReply } from '../utils/safeSend.js'; ... const embed = buildReviewEmbed(review, interaction.user.username); const row = buildClaimButton(review.id); - const message = await targetChannel.send({ embeds: [embed], components: [row] }); + let message; + try { + message = await safeSend(targetChannel, { embeds: [embed], components: [row] }); + } catch (err) { + await pool.query('DELETE FROM reviews WHERE id = $1', [review.id]); + warn('Failed to post review request message', { + reviewId: review.id, + guildId: interaction.guildId, + error: err.message, + }); + await safeEditReply(interaction, { + content: '❌ Failed to post review request. Please try again.', + }); + return; + } // Store message + channel reference for later updates🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/review.js` around lines 176 - 183, Replace the direct call to targetChannel.send with the safeSend utility (e.g., safeSend(targetChannel, { embeds, components })) and wrap the send in a try/catch; if safeSend throws, roll back the DB changes that would create an orphaned record—either avoid updating message_id/channel_id or delete the review row created earlier via pool.query('DELETE FROM reviews WHERE id = $1', [review.id])—and log the error. Specifically update the block using targetChannel.send to call safeSend, catch failures before running the UPDATE that sets message_id and channel_id (or undo the INSERT if the UPDATE already ran), and ensure message.id and targetChannel.id are only written to the reviews row when the send succeeds.src/modules/reviewHandler.js (1)
299-321:⚠️ Potential issue | 🟠 MajorEmbed updates are skipped when
review.channelIdis unset.The
continueon line 303 exits the entire loop iteration, which meansupdateReviewMessageis never called for guilds without a configured review channel. This leaves stale review embeds with the old status and an enabled claim button, even though the DB status changed to'stale'.The nudge message is optional (requires a channel), but the embed update should always run.
🐛 Proposed fix
for (const [guildId, reviews] of byGuild) { const guildConfig = getConfig(guildId); const reviewChannelId = guildConfig.review?.channelId; const staleDays = guildConfig?.review?.staleAfterDays ?? 7; - if (!reviewChannelId) continue; - try { - const channel = await client.channels.fetch(reviewChannelId).catch(() => null); - if (!channel) continue; - - const ids = reviews.map((r) => `#${r.id}`).join(', '); - await safeSend(channel, { - content: `⏰ The following review request${reviews.length > 1 ? 's have' : ' has'} gone stale (no reviewer after ${staleDays} days): **${ids}**\n> Re-request if you still need a review!`, - }); - } catch (nudgeErr) { - warn('Failed to send stale review nudge', { guildId, error: nudgeErr.message }); + if (reviewChannelId) { + try { + const channel = await client.channels.fetch(reviewChannelId).catch(() => null); + if (channel) { + const ids = reviews.map((r) => `#${r.id}`).join(', '); + await safeSend(channel, { + content: `⏰ The following review request${reviews.length > 1 ? 's have' : ' has'} gone stale (no reviewer after ${staleDays} days): **${ids}**\n> Re-request if you still need a review!`, + }); + } + } catch (nudgeErr) { + warn('Failed to send stale review nudge', { guildId, error: nudgeErr.message }); + } } // Update embeds for stale reviews for (const review of reviews) { await updateReviewMessage(review, client); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/reviewHandler.js` around lines 299 - 321, The loop currently does "if (!reviewChannelId) continue", which skips calling updateReviewMessage for that guild; instead remove the early continue and only skip the nudge send when reviewChannelId is falsy: wrap the channel fetch/send logic in a conditional (using reviewChannelId) and keep the post-send loop that calls updateReviewMessage(review, client) running for all reviews so embeds are always updated; refer to byGuild, reviewChannelId, client.channels.fetch, safeSend and updateReviewMessage to locate the affected code and ensure error handling (warn) remains around the send operation only.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/commands/review.test.js`:
- Around line 296-336: Add a unit test that asserts the /review request path
rejects invalid URLs by creating an interaction via makeInteraction('request', {
url: 'not-a-valid-url', description: '...' }), calling execute(interaction), and
verifying pool.query was not called and interaction.editReply was called with a
response containing a 'not valid' message; reference makeInteraction, execute,
pool.query, and interaction.editReply so the new test mirrors the existing
patterns in this file and checks URL validation failure handling.
---
Duplicate comments:
In `@src/commands/review.js`:
- Around line 87-104: Check interaction.guildId first and return the
"server-only" message before calling getConfig or inspecting
guildConfig.review?.enabled; specifically, move the existing null/undefined
check of interaction.guildId to run prior to calling
getConfig(interaction.guildId) and before the guildConfig.review?.enabled guard
so the command responds with the correct "This command can only be used in a
server." via safeEditReply when invoked in DMs.
- Around line 309-342: The UPDATE that marks a review completed must be made
atomic by adding the prior-state predicate so only a row currently in the
expected state is transitioned and returned; modify the SQL in the pool.query
that sets status = 'completed' to include WHERE id = $2 AND status = 'claimed'
(and optionally reviewer_id = $3 if you require the claimer match
interaction.user.id), then use the returned row (completedReview) to decide
whether to call updateReviewMessage and to award XP; if no row is returned,
treat it as already-claimed/completed and skip XP awarding/notification to avoid
double credit.
- Around line 176-183: Replace the direct call to targetChannel.send with the
safeSend utility (e.g., safeSend(targetChannel, { embeds, components })) and
wrap the send in a try/catch; if safeSend throws, roll back the DB changes that
would create an orphaned record—either avoid updating message_id/channel_id or
delete the review row created earlier via pool.query('DELETE FROM reviews WHERE
id = $1', [review.id])—and log the error. Specifically update the block using
targetChannel.send to call safeSend, catch failures before running the UPDATE
that sets message_id and channel_id (or undo the INSERT if the UPDATE already
ran), and ensure message.id and targetChannel.id are only written to the reviews
row when the send succeeds.
In `@src/modules/reviewHandler.js`:
- Around line 299-321: The loop currently does "if (!reviewChannelId) continue",
which skips calling updateReviewMessage for that guild; instead remove the early
continue and only skip the nudge send when reviewChannelId is falsy: wrap the
channel fetch/send logic in a conditional (using reviewChannelId) and keep the
post-send loop that calls updateReviewMessage(review, client) running for all
reviews so embeds are always updated; refer to byGuild, reviewChannelId,
client.channels.fetch, safeSend and updateReviewMessage to locate the affected
code and ensure error handling (warn) remains around the send operation only.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
config.jsonsrc/api/utils/configAllowlist.jssrc/commands/review.jssrc/modules/events.jssrc/modules/reviewHandler.jstests/commands/review.test.jsweb/src/components/dashboard/config-editor.tsx
📜 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). (2)
- GitHub Check: Greptile Review
- GitHub Check: Agent
🧰 Additional context used
📓 Path-based instructions (7)
**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
**/*.js: Use ESM modules only — import/export, never require()
Always use node: protocol for Node.js builtin imports (e.g., import { readFileSync } from 'node:fs')
Files:
src/modules/reviewHandler.jstests/commands/review.test.jssrc/api/utils/configAllowlist.jssrc/commands/review.jssrc/modules/events.js
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,tsx}: Always use semicolons
Use single quotes — enforced by Biome
Use 2-space indentation — enforced by Biome
Files:
src/modules/reviewHandler.jstests/commands/review.test.jssrc/api/utils/configAllowlist.jssrc/commands/review.jssrc/modules/events.jsweb/src/components/dashboard/config-editor.tsx
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: NEVER use console.log, console.warn, console.error, or any console.* method in src/ files — always use Winston logger instead: import { info, warn, error } from '../logger.js'
Pass structured metadata to Winston logger: info('Message processed', { userId, channelId })
Use custom error classes from src/utils/errors.js and always log errors with context before re-throwing
Use getConfig(guildId?) from src/modules/config.js to read config; use setConfigValue(path, value, guildId?) to update at runtime
Use safeSend() utility for outgoing Discord messages to sanitize mentions and enforce allowedMentions
Use splitMessage() utility for messages exceeding Discord's 2000-character limit
Add tests for all new code with mandatory 80% coverage threshold on statements, branches, functions, and lines; run pnpm test before every commit
Files:
src/modules/reviewHandler.jssrc/api/utils/configAllowlist.jssrc/commands/review.jssrc/modules/events.js
src/modules/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/modules/*.js: Register event handlers in src/modules/events.js by importing handler functions and calling client.on() with config parameter
Check config.yourModule.enabled before processing in module event handlers
Prefer per-request getConfig() pattern in new modules over reactive onConfigChange() wiring; only add onConfigChange() listeners for stateful resources that cannot re-read config on each use
Files:
src/modules/reviewHandler.jssrc/modules/events.js
src/commands/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/commands/*.js: Use parseDuration() from src/utils/duration.js for duration-based commands (timeout, tempban, slowmode); enforce Discord duration caps (timeouts max 28 days, slowmode max 6 hours)
Create slash commands by exporting data (SlashCommandBuilder) and execute() function from src/commands/*.js; export adminOnly = true for mod-only commands; commands are auto-discovered on startup
Files:
src/commands/review.js
web/**/*.{tsx,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use next/image Image component with appropriate layout and sizing props in Next.js components
Files:
web/src/components/dashboard/config-editor.tsx
web/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use Zustand store (zustand) for state management in React components; implement fetch-on-demand pattern in stores
Files:
web/src/components/dashboard/config-editor.tsx
🧠 Learnings (9)
📚 Learning: 2026-02-27T16:24:15.054Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-27T16:24:15.054Z
Learning: Applies to src/commands/*mod*.js : Moderation commands must follow the shared pattern: deferReply({ ephemeral: true }), validate inputs, sendDmNotification(), execute Discord action, createCase(), sendModLogEmbed(), checkEscalation()
Applied to files:
src/modules/reviewHandler.jssrc/commands/review.js
📚 Learning: 2026-02-27T16:24:15.054Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-27T16:24:15.054Z
Learning: Applies to src/modules/moderation.js : Case numbering must be per-guild sequential, assigned atomically using COALESCE(MAX(case_number), 0) + 1 in a single INSERT statement
Applied to files:
src/modules/reviewHandler.jssrc/commands/review.js
📚 Learning: 2026-02-27T16:24:15.054Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-27T16:24:15.054Z
Learning: Applies to src/**/*.js : Add tests for all new code with mandatory 80% coverage threshold on statements, branches, functions, and lines; run pnpm test before every commit
Applied to files:
tests/commands/review.test.js
📚 Learning: 2026-02-27T16:24:15.054Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-27T16:24:15.054Z
Learning: Applies to src/commands/*.js : Create slash commands by exporting data (SlashCommandBuilder) and execute() function from src/commands/*.js; export adminOnly = true for mod-only commands; commands are auto-discovered on startup
Applied to files:
src/commands/review.js
📚 Learning: 2026-02-27T16:24:15.054Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-27T16:24:15.054Z
Learning: Applies to src/commands/*.js : Use parseDuration() from src/utils/duration.js for duration-based commands (timeout, tempban, slowmode); enforce Discord duration caps (timeouts max 28 days, slowmode max 6 hours)
Applied to files:
src/commands/review.js
📚 Learning: 2026-02-27T16:24:15.054Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-27T16:24:15.054Z
Learning: Applies to src/**/*.js : Use safeSend() utility for outgoing Discord messages to sanitize mentions and enforce allowedMentions
Applied to files:
src/commands/review.js
📚 Learning: 2026-02-27T16:24:15.054Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-27T16:24:15.054Z
Learning: Applies to src/**/*.js : Use getConfig(guildId?) from src/modules/config.js to read config; use setConfigValue(path, value, guildId?) to update at runtime
Applied to files:
src/commands/review.js
📚 Learning: 2026-02-27T16:24:15.054Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-27T16:24:15.054Z
Learning: Applies to src/modules/*.js : Register event handlers in src/modules/events.js by importing handler functions and calling client.on() with config parameter
Applied to files:
src/modules/events.js
📚 Learning: 2026-02-27T16:24:15.054Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-27T16:24:15.054Z
Learning: Applies to src/modules/*.js : Check config.yourModule.enabled before processing in module event handlers
Applied to files:
src/modules/events.js
🧬 Code graph analysis (3)
src/modules/reviewHandler.js (4)
src/modules/config.js (2)
err(94-94)getConfig(282-313)src/logger.js (2)
warn(237-239)info(230-232)src/db.js (1)
getPool(142-147)src/utils/safeSend.js (2)
safeReply(138-145)safeSend(116-123)
src/commands/review.js (4)
src/modules/reviewHandler.js (16)
pool(151-151)pool(158-161)pool(189-194)pool(258-258)pool(275-283)review(171-171)embed(43-60)embed(133-133)buildReviewEmbed(40-97)row(134-134)buildClaimButton(106-114)message(129-129)STATUS_LABELS(25-30)STATUS_LABELS(25-30)reviewId(148-148)updateReviewMessage(122-140)src/modules/config.js (2)
getConfig(282-313)err(94-94)src/utils/safeSend.js (1)
safeEditReply(178-185)src/db.js (1)
getPool(142-147)
src/modules/events.js (4)
src/modules/config.js (8)
client(161-161)client(522-522)client(526-529)client(711-711)guildConfig(499-499)guildConfig(636-636)getConfig(282-313)err(94-94)src/commands/review.js (1)
guildConfig(87-87)src/modules/reviewHandler.js (2)
guildConfig(300-300)handleReviewClaim(147-250)src/utils/safeSend.js (1)
safeReply(138-145)
🔇 Additional comments (8)
src/api/utils/configAllowlist.js (1)
26-26: LGTM!Adding
'review'toSAFE_CONFIG_KEYScorrectly enables API access to the new review configuration section. This aligns with the newreviewobject inconfig.jsonand contains no sensitive fields requiring masking.config.json (1)
154-154: LGTM!The new review configuration is well-structured:
- Permission set to
"everyone"appropriately enables community peer reviews.enabled: falseensures explicit opt-in per guild.staleAfterDays: 7matches the issue requirement for auto-expiry.xpReward: 50provides a sensible default for reputation integration.Also applies to: 209-214
src/modules/reviewHandler.js (2)
1-18: LGTM on module structure and imports.The module correctly:
- Separates embed/button builders from the command for scheduler compatibility.
- Uses Winston logger instead of
console.*.- Uses
safeSend/safeReplyutilities for outgoing Discord messages.- Uses
getConfig()for per-guild configuration.
187-204: Good: Atomic claim prevents TOCTOU race.The
WHERE status = 'open'predicate in theUPDATEensures only one concurrent claimer succeeds, and therowCount === 0check handles the race gracefully. This addresses the previous review feedback.src/modules/events.js (1)
356-392: LGTM!The review claim handler is correctly implemented:
- Filters button interactions by
review_claim_prefix.- Gates processing on
guildConfig.review?.enabled(addresses the coding guideline to checkconfig.yourModule.enabledbefore processing).- Error handling follows the established pattern (log + ephemeral fallback).
- Registration in
registerEventHandlersensures it's wired at startup.web/src/components/dashboard/config-editor.tsx (1)
51-51: LGTM!The review feature is correctly integrated into the config editor:
'review'added toknownSectionsfor type guard validation.- Code Reviews toggle follows the established Community Features pattern.
Note: Only the enable/disable toggle is exposed. If detailed settings (channelId, staleAfterDays, xpReward) should be configurable via dashboard, additional UI would be needed — but that can be a follow-up enhancement.
Also applies to: 1204-1204
tests/commands/review.test.js (2)
1-160: LGTM on test structure and mocks.The test file is well-organized with:
- Comprehensive mocks for discord.js, db, logger, config, and safeSend.
- Clean helper factories (
makePool,makeInteraction,makeReview).- Good separation of test describe blocks by functionality.
541-678: Good coverage of claim handler edge cases.Tests correctly verify:
- Successful claim with atomic UPDATE.
- Self-claim prevention.
- Double-claim, completed, and stale status blocking.
- Error cases (not found, null pool, invalid customId).
| describe('/review request', () => { | ||
| it('creates a review and posts embed to current channel', async () => { | ||
| const review = makeReview(); | ||
| pool.query | ||
| .mockResolvedValueOnce({ rows: [review] }) // INSERT | ||
| .mockResolvedValueOnce({ rows: [] }); // UPDATE message_id | ||
|
|
||
| const interaction = makeInteraction('request', { | ||
| url: 'https://github.com/test/pr/1', | ||
| description: 'Review my changes', | ||
| language: 'JavaScript', | ||
| }); | ||
|
|
||
| await execute(interaction); | ||
|
|
||
| expect(pool.query).toHaveBeenCalledWith( | ||
| expect.stringContaining('INSERT INTO reviews'), | ||
| expect.arrayContaining(['guild-123', 'user-789']), | ||
| ); | ||
| expect(interaction.channel.send).toHaveBeenCalledWith( | ||
| expect.objectContaining({ embeds: expect.any(Array), components: expect.any(Array) }), | ||
| ); | ||
| expect(interaction.editReply).toHaveBeenCalledWith( | ||
| expect.objectContaining({ content: expect.stringContaining('✅') }), | ||
| ); | ||
| }); | ||
|
|
||
| it('creates a review without optional language', async () => { | ||
| const review = makeReview({ language: null }); | ||
| pool.query.mockResolvedValueOnce({ rows: [review] }).mockResolvedValueOnce({ rows: [] }); | ||
|
|
||
| const interaction = makeInteraction('request', { | ||
| url: 'https://github.com/test/pr/1', | ||
| description: 'Review my changes', | ||
| }); | ||
|
|
||
| await execute(interaction); | ||
| expect(interaction.editReply).toHaveBeenCalledWith( | ||
| expect.objectContaining({ content: expect.stringContaining('#1') }), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good test coverage for /review request.
The tests cover the happy path well. Consider adding a test for URL validation rejection:
💡 Suggested additional test
it('rejects request with invalid URL', async () => {
const interaction = makeInteraction('request', {
url: 'not-a-valid-url',
description: 'Review my changes',
});
await execute(interaction);
expect(pool.query).not.toHaveBeenCalled();
expect(interaction.editReply).toHaveBeenCalledWith(
expect.objectContaining({ content: expect.stringContaining('not valid') }),
);
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/commands/review.test.js` around lines 296 - 336, Add a unit test that
asserts the /review request path rejects invalid URLs by creating an interaction
via makeInteraction('request', { url: 'not-a-valid-url', description: '...' }),
calling execute(interaction), and verifying pool.query was not called and
interaction.editReply was called with a response containing a 'not valid'
message; reference makeInteraction, execute, pool.query, and
interaction.editReply so the new test mirrors the existing patterns in this file
and checks URL validation failure handling.
| const message = await targetChannel.send({ embeds: [embed], components: [row] }); | ||
|
|
||
| // Store message + channel reference for later updates | ||
| await pool.query('UPDATE reviews SET message_id = $1, channel_id = $2 WHERE id = $3', [ | ||
| message.id, | ||
| targetChannel.id, | ||
| review.id, | ||
| ]); |
There was a problem hiding this comment.
review record orphaned if message send fails. If targetChannel.send() throws (permissions, rate limit, channel deleted), the review exists in DB but message_id stays null. Users can't claim it via button, and it's not cleaned up.
wrap in try-catch and rollback:
| const message = await targetChannel.send({ embeds: [embed], components: [row] }); | |
| // Store message + channel reference for later updates | |
| await pool.query('UPDATE reviews SET message_id = $1, channel_id = $2 WHERE id = $3', [ | |
| message.id, | |
| targetChannel.id, | |
| review.id, | |
| ]); | |
| try { | |
| const message = await targetChannel.send({ embeds: [embed], components: [row] }); | |
| // Store message + channel reference for later updates | |
| await pool.query('UPDATE reviews SET message_id = $1, channel_id = $2 WHERE id = $3', [ | |
| message.id, | |
| targetChannel.id, | |
| review.id, | |
| ]); | |
| } catch (sendErr) { | |
| // Rollback - delete the orphaned review | |
| await pool.query('DELETE FROM reviews WHERE id = $1', [review.id]); | |
| throw sendErr; // Re-throw so outer handler can report to user | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/commands/review.js
Line: 176-183
Comment:
review record orphaned if message send fails. If `targetChannel.send()` throws (permissions, rate limit, channel deleted), the review exists in DB but `message_id` stays null. Users can't claim it via button, and it's not cleaned up.
wrap in try-catch and rollback:
```suggestion
try {
const message = await targetChannel.send({ embeds: [embed], components: [row] });
// Store message + channel reference for later updates
await pool.query('UPDATE reviews SET message_id = $1, channel_id = $2 WHERE id = $3', [
message.id,
targetChannel.id,
review.id,
]);
} catch (sendErr) {
// Rollback - delete the orphaned review
await pool.query('DELETE FROM reviews WHERE id = $1', [review.id]);
throw sendErr; // Re-throw so outer handler can report to user
}
```
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| for (const { guild_id: guildId } of openGuilds) { | ||
| const config = getConfig(guildId); | ||
| const staleDays = config?.review?.staleAfterDays ?? 7; |
There was a problem hiding this comment.
validate staleAfterDays is positive. Config could have 0 or negative value. Add guard:
| const staleDays = config?.review?.staleAfterDays ?? 7; | |
| const staleDays = Math.max(1, config?.review?.staleAfterDays ?? 7); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/modules/reviewHandler.js
Line: 273
Comment:
validate `staleAfterDays` is positive. Config could have 0 or negative value. Add guard:
```suggestion
const staleDays = Math.max(1, config?.review?.staleAfterDays ?? 7);
```
How can I resolve this? If you propose a fix, please make it concise.
Summary
Adds a peer code review request system to the bot via the
/reviewcommand.What's included
migrations/010_reviews.cjs—reviewstable with full schema and indexessrc/commands/review.js—/review request,/review list,/review completesubcommandssrc/modules/reviewHandler.js— embed builders, claim button handler (handleReviewClaim), stale expiry (expireStaleReviews)src/modules/events.js—registerReviewClaimHandlerwired intoregisterEventHandlerssrc/modules/scheduler.js—expireStaleReviewspiggybacks on the 60s poll loopconfig.json—reviewsection added with defaults,review: everyonein permissionssrc/api/utils/configAllowlist.js—reviewadded toSAFE_CONFIG_KEYSweb/src/components/dashboard/config-editor.tsx— Code Reviews toggle + channel ID + stale days + XP reward settings cardtests/commands/review.test.js— 39 tests covering all paths (request, list, complete, claim, self-claim, double-claim, stale expiry, XP reward, config disabled)Key design decisions
reviewHandler.js(notreview.js) so the scheduler can import the module without pulling inSlashCommandBuilder— prevents breakingindex.test.js's discord.js mockclaimedANDcompletedstatusstaleAfterDays)Closes #49