Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughReplaces guild-specific admin/moderator guards with a role-based system: adds permission-to-role mapping, a requireRole(minRole) middleware, frontend dashboard guild context and role utilities, updates routes to use role checks, and converts backup I/O to async/await. Changes
Possibly related PRs
🚥 Pre-merge checks | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)✅ Unit Test PR creation complete.
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 |
There was a problem hiding this comment.
Pull request overview
This PR introduces tiered “dashboard roles” (viewer/moderator/admin/owner) derived from Discord permission bitfields and uses them to gate backend routes and (partially) the web dashboard navigation.
Changes:
- Added shared role derivation utilities on the web (
dashboard-roles.ts) and a selected-guild provider for role-aware UI. - Updated Next.js bot-api proxy authorization to support minimum-role checks (
authorizeGuildRole) and switched analytics to viewer access. - Updated Express API routes to use
requireRole(minRole)and adjusted/expanded guild route tests for the new mapping.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/types/discord.ts | Adds GuildAccessRole and MutualGuild.access for role-aware guild objects. |
| web/src/lib/discord.server.ts | Derives access for mutual guilds and filters out guilds without dashboard access. |
| web/src/lib/dashboard-roles.ts | New frontend role derivation + route access helpers for the sidebar. |
| web/src/lib/bot-api-proxy.ts | Adds authorizeGuildRole and keeps authorizeGuildAdmin as a wrapper. |
| web/src/contexts/dashboard-guild-context.tsx | New client context for loading guilds + selected guild persistence. |
| web/src/components/layout/sidebar.tsx | Filters navigation items based on the selected guild’s role. |
| web/src/components/layout/server-selector.tsx | Refactors to consume the new dashboard guild context. |
| web/src/components/layout/dashboard-shell.tsx | Wraps dashboard layout with DashboardGuildProvider. |
| web/src/app/api/guilds/[guildId]/analytics/route.ts | Lowers analytics authorization to viewer via authorizeGuildRole. |
| tests/api/routes/guilds.test.js | Updates role expectations and adds coverage for viewer access. |
| src/api/routes/tickets.js | Switches from requireGuildAdmin to requireRole('admin'). |
| src/api/routes/tempRoles.js | Switches from moderator middleware to requireRole('moderator'). |
| src/api/routes/moderation.js | Switches moderation auth to requireRole('moderator'). |
| src/api/routes/members.js | Switches member endpoints to requireRole('admin'). |
| src/api/routes/guilds.js | Introduces role mapping + requireRole and lowers several endpoints to viewer. |
| src/api/routes/conversations.js | Switches conversations endpoints to requireRole('admin'). |
| src/api/routes/auditLog.js | Switches audit log endpoints to requireRole('admin'). |
| src/api/routes/ai-feedback.js | Switches AI feedback endpoints to requireRole('admin'). |
Comments suppressed due to low confidence (2)
src/api/routes/guilds.js:40
- The JSDoc for
permissionsToDashboardRolesays@param {number} permissions, but this function is called withug.permissionswhich is a string from the Discord API. Update the JSDoc to match actual inputs (e.g.,string | number) to avoid misleading API docs and tooling.
/**
* Map Discord guild permission bitfield to dashboard role (no bot-owner check).
* Admin = ADMINISTRATOR or MANAGE_GUILD; Moderator = MANAGE_MESSAGES or KICK_MEMBERS or BAN_MEMBERS; Viewer = VIEW_CHANNEL.
* @param {number} permissions - Guild permission bitfield from Discord API
* @returns {'admin'|'moderator'|'viewer'|null} Highest role granted, or null if no dashboard access
*/
function permissionsToDashboardRole(permissions) {
web/src/lib/bot-api-proxy.ts:48
- Because
permissionsToDashboardRole()never returns'owner', thederived === 'owner'branch here is unreachable. Either remove the'owner'handling from the derived-permissions path, or changepermissionsToDashboardRoleto return a non-owner role union so this can’t silently become dead code.
// When access not set (e.g. bot unavailable), derive from Discord permissions
const derived = permissionsToDashboardRole(guild.permissions);
if (derived === 'admin' || derived === 'owner') return 'admin';
if (derived === 'moderator') return 'moderator';
if (derived === 'viewer') return 'viewer';
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
web/src/types/discord.ts
Outdated
| /** Dashboard role for the current user in this guild (from Discord permissions or backend). */ | ||
| export type GuildAccessRole = 'viewer' | 'moderator' | 'admin' | 'owner'; | ||
|
|
||
| export interface MutualGuild extends DiscordGuild { | ||
| botPresent: boolean; | ||
| /** Current user's dashboard role in this guild. Set when guild list is built. */ | ||
| access?: GuildAccessRole; |
There was a problem hiding this comment.
GuildAccessRole duplicates the DashboardRole union defined in web/src/lib/dashboard-roles.ts. Keeping two separate role unions increases the chance of drift (e.g., adding a new role in one place but not the other). Consider exporting a single shared role type and reusing it here (or re-exporting from a central types module).
web/src/lib/dashboard-roles.ts
Outdated
| /** | ||
| * Map Discord guild permission bitfield to dashboard role (no owner — owner is bot owner from backend). | ||
| */ | ||
| export function permissionsToDashboardRole(permissions: string): DashboardRole | null { | ||
| const p = Number(permissions); | ||
| if (Number.isNaN(p)) return null; |
There was a problem hiding this comment.
permissionsToDashboardRole is documented as not producing an owner role, but its return type is DashboardRole | null where DashboardRole includes 'owner'. This makes downstream code think 'owner' can be derived from the Discord permission bitfield. Narrow the return type to exclude 'owner' (or update the role model) so type checks and branches don’t become misleading/dead code.
web/src/lib/discord.server.ts
Outdated
| return userGuilds | ||
| .filter((guild) => botGuildIds.has(guild.id)) | ||
| .map((guild) => ({ | ||
| ...guild, | ||
| botPresent: true as const, | ||
| })); | ||
| .map((guild) => { | ||
| const access = permissionsToDashboardRole(guild.permissions); | ||
| return { ...guild, botPresent: true as const, access: access ?? undefined }; | ||
| }) | ||
| .filter((guild): guild is MutualGuild => guild.access != null); |
There was a problem hiding this comment.
getMutualGuilds() now filters out guilds with no derived access when the Bot API is available, but when the Bot API is unavailable the earlier branch returns guilds without setting access at all. This makes downstream role-based UI/authorization behave inconsistently depending on Bot API availability. Consider deriving (and optionally filtering) access in both code paths so callers can rely on access semantics consistently.
| const guildContext = useContext(DashboardGuildContext); | ||
| const role: DashboardRole | null = guildContext?.selectedGuild?.access ?? null; | ||
|
|
||
| const visibleNav = role === null ? navigation : navigation.filter((item) => canAccessRoute(role, item.href)); |
There was a problem hiding this comment.
When role is null (initial load / no guild selected), this falls back to showing all navigation items, including admin-only routes. That undermines the role-filtering behavior and can cause confusing UI flashes or allow users to click into routes they’ll be forbidden from. Consider hiding the nav until the role is known, or defaulting to the most restrictive set (e.g. viewer-only) while loading.
| const visibleNav = role === null ? navigation : navigation.filter((item) => canAccessRoute(role, item.href)); | |
| // When the dashboard role is not yet known (null), do not render any navigation items. | |
| // This avoids briefly exposing admin-only routes before role-based filtering is applied. | |
| const visibleNav = role === null ? [] : navigation.filter((item) => canAccessRoute(role, item.href)); |
| const authError = await authorizeGuildRole(request, guildId, 'viewer', '[api/guilds/:guildId/analytics]'); | ||
| if (authError) return authError; |
There was a problem hiding this comment.
This endpoint was updated to allow 'viewer' access via authorizeGuildRole, but other read-only guild proxy routes (e.g. /api/guilds/[guildId]/channels and /api/guilds/[guildId]/roles) still use authorizeGuildAdmin, which will continue to block viewers even though the backend now permits viewer access. If the intent is to support viewer-level dashboards, those proxy routes likely need to be aligned to use authorizeGuildRole(..., 'viewer', ...) as well.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/api/routes/tempRoles.js (1)
119-166:⚠️ Potential issue | 🔴 CriticalProtect DELETE /temp-roles/:id with role authorization (currently missing).
The revoke endpoint performs a privileged action but has no
requireRolemiddleware, so this migration is incomplete and leaves a security gap.🔒 Suggested fix
-router.delete('/:id', async (req, res) => { +router.delete('/:tempRoleId', adaptGuildIdParam, requireRole('moderator'), async (req, res) => { try { const guildId = req.query.guildId; @@ - const id = Number.parseInt(req.params.id, 10); + const id = Number.parseInt(req.params.tempRoleId, 10);As per coding guidelines,
src/api/routes/**/*.{js,ts}: "All API routes must use auth middleware if handling sensitive data and be mounted insrc/api/server.js".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/routes/tempRoles.js` around lines 119 - 166, The DELETE /temp-roles/:id route (router.delete handler) is missing authorization: wrap this route with the existing requireRole middleware (or the project's auth middleware used by other sensitive endpoints) so only authorized moderators/admins can call it; modify the route registration to call requireRole(...) before the async handler (same pattern used by other routes) and ensure the middleware checks the appropriate permission/role name expected by revokeTempRoleById operations; also confirm this protected route is mounted under the authenticated API in server setup (same mounting pattern as other protected routes in src/api/server.js).src/api/routes/moderation.js (1)
18-20:⚠️ Potential issue | 🟡 MinorUpdate stale middleware reference in the comment.
Line 18 still mentions
requireGuildModerator, but the route now usesrequireRole('moderator'). Please align the comment to avoid confusion.✏️ Suggested comment fix
- * Middleware: adapt query param guildId to path param for requireGuildModerator. + * Middleware: adapt query param guildId to path param for requireRole('moderator').🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/routes/moderation.js` around lines 18 - 20, Update the stale comment that references requireGuildModerator to reflect the current middleware usage: change the comment that explains adapting query param guildId to a path param so it names requireRole('moderator') (or generically "requireRole('moderator') middleware") instead of requireGuildModerator, ensuring the comment matches the actual middleware used in the route.src/api/routes/guilds.js (1)
420-447:⚠️ Potential issue | 🟡 MinorOpenAPI
accesscontract is stale vs runtime response.Docs still describe admin-only filtering and enum
[admin, moderator, bot-owner], but runtime now returnsaccess: 'owner'(Line 471) and role-derived values including viewer (Line 503). This can break generated clients and dashboard typing.📝 Proposed OpenAPI update
- * For OAuth users: returns guilds where the user has MANAGE_GUILD or ADMINISTRATOR. + * For OAuth users: returns guilds where the user has dashboard access (viewer/moderator/admin). * Bot owners see all guilds. For API-secret users: returns all bot guilds. @@ - * access: - * type: string - * enum: [admin, moderator, bot-owner] + * access: + * type: string + * enum: [viewer, moderator, admin, owner]Also applies to: 471-472, 493-504
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/routes/guilds.js` around lines 420 - 447, The OpenAPI schema for the guild list is out of sync with runtime: update the "access" property in the response schema in src/api/routes/guilds.js (the OpenAPI block that defines items.properties.access) to match the actual runtime values (add "owner" and "viewer" and any role-derived values your runtime emits, not just ["admin","moderator","bot-owner"]), and revise the surrounding description that currently says "admin-only filtering" to accurately describe the current access semantics (e.g., owner, admin, moderator, viewer, bot-owner) so generated clients and typings reflect the real responses.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/routes/guilds.js`:
- Around line 268-277: Convert the deprecated wrappers isOAuthGuildAdmin and
isOAuthGuildModerator to async functions using await instead of .then(): mark
both functions async, call const r = await getOAuthDashboardRole(user, guildId)
inside each, and return the same boolean expressions (r === 'owner' || r ===
'admin' for isOAuthGuildAdmin; r === 'owner' || r === 'admin' || r ===
'moderator' for isOAuthGuildModerator); keep the existing JSDoc deprecation
comments and function names to preserve backward compatibility.
- Around line 1661-1664: The API-secret check (req.authMethod !== 'api-secret')
must run before any role or guild validation to avoid unnecessary
OAuth/session/Discord permission checks; update the POST '/:id/actions' route so
the API-secret guard executes first (either by moving the check into a new
middleware placed before requireRole('admin') and validateGuild, or by
reordering middlewares) and return 403 immediately when authMethod is not
'api-secret'; reference the existing route handler, requireRole, validateGuild,
and the req.authMethod check when applying the change.
In `@web/src/components/layout/sidebar.tsx`:
- Line 54: The code currently sets visibleNav = role === null ? navigation :
navigation.filter(...) which exposes all links when role is null; change it so
unknown role does not bypass gating—e.g., compute visibleNav by filtering
navigation and only including items when role is non-null and
canAccessRoute(role, item.href) returns true (or return an empty array when role
is null). Update the assignment for visibleNav to use role and canAccessRoute
consistently (referencing the visibleNav variable, role variable, navigation
array, and canAccessRoute function) so no links are shown when role is unknown.
In `@web/src/contexts/dashboard-guild-context.tsx`:
- Around line 74-98: When handling the fetchedGuilds result in the block that
calls setGuilds, ensure you clear any stale selected guild if fetchedGuilds is
empty: if fetchedGuilds.length === 0 then call setSelectedGuildState(null) (or
the appropriate empty value), remove the SELECTED_GUILD_KEY from localStorage
inside a try/catch, and call broadcastSelectedGuild(null) (or the appropriate
sentinel) so downstream state is not left stale; update the logic around
setGuilds/fetchedGuilds to perform these clear actions before early-returning or
proceeding to the restore/first-item selection.
In `@web/src/lib/dashboard-roles.ts`:
- Around line 1-4: The dashboard role flags and mapping are duplicated; extract
the canonical role model and the permissionsToDashboardRole mapper into a single
shared module (exporting the role enum/array, permission bit constants, and the
permissionsToDashboardRole function), then update both the existing
dashboard-roles implementation and the backend guilds route to import and use
those exports (replace local role flags/order/mapping with imports and delegate
role derivation to the shared permissionsToDashboardRole). Ensure the shared
module exports the same identifiers used elsewhere (role list/name constants and
permissionsToDashboardRole) so callers need only swap local definitions for
imports.
- Around line 62-67: canAccessRoute currently returns true when
NAV_MIN_ROLE[path] is undefined, which leaves unmapped dashboard routes open;
update it to fail-closed for dashboard paths and perform nested route matching:
compute the best matching min role by finding the longest NAV_MIN_ROLE key that
is a prefix of the requested path (or exact match), then if a match is found
call hasMinimumRole(role, min); if no match and path startsWith('/dashboard')
return false; otherwise (non-dashboard unmapped paths) return true. Update the
function canAccessRoute and reference NAV_MIN_ROLE and hasMinimumRole
accordingly.
---
Outside diff comments:
In `@src/api/routes/guilds.js`:
- Around line 420-447: The OpenAPI schema for the guild list is out of sync with
runtime: update the "access" property in the response schema in
src/api/routes/guilds.js (the OpenAPI block that defines
items.properties.access) to match the actual runtime values (add "owner" and
"viewer" and any role-derived values your runtime emits, not just
["admin","moderator","bot-owner"]), and revise the surrounding description that
currently says "admin-only filtering" to accurately describe the current access
semantics (e.g., owner, admin, moderator, viewer, bot-owner) so generated
clients and typings reflect the real responses.
In `@src/api/routes/moderation.js`:
- Around line 18-20: Update the stale comment that references
requireGuildModerator to reflect the current middleware usage: change the
comment that explains adapting query param guildId to a path param so it names
requireRole('moderator') (or generically "requireRole('moderator') middleware")
instead of requireGuildModerator, ensuring the comment matches the actual
middleware used in the route.
In `@src/api/routes/tempRoles.js`:
- Around line 119-166: The DELETE /temp-roles/:id route (router.delete handler)
is missing authorization: wrap this route with the existing requireRole
middleware (or the project's auth middleware used by other sensitive endpoints)
so only authorized moderators/admins can call it; modify the route registration
to call requireRole(...) before the async handler (same pattern used by other
routes) and ensure the middleware checks the appropriate permission/role name
expected by revokeTempRoleById operations; also confirm this protected route is
mounted under the authenticated API in server setup (same mounting pattern as
other protected routes in src/api/server.js).
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (18)
src/api/routes/ai-feedback.jssrc/api/routes/auditLog.jssrc/api/routes/conversations.jssrc/api/routes/guilds.jssrc/api/routes/members.jssrc/api/routes/moderation.jssrc/api/routes/tempRoles.jssrc/api/routes/tickets.jstests/api/routes/guilds.test.jsweb/src/app/api/guilds/[guildId]/analytics/route.tsweb/src/components/layout/dashboard-shell.tsxweb/src/components/layout/server-selector.tsxweb/src/components/layout/sidebar.tsxweb/src/contexts/dashboard-guild-context.tsxweb/src/lib/bot-api-proxy.tsweb/src/lib/dashboard-roles.tsweb/src/lib/discord.server.tsweb/src/types/discord.ts
📜 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: Agent
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (6)
src/**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{js,ts,jsx,tsx}: Use ESM only — Useimport/export, no CommonJS
Use single quotes for strings — No double quotes except in JSON
Always require semicolons at end of statements
Use 2-space indent, enforced by Biome
Always use async/await for asynchronous operations and promise handling
Files:
src/api/routes/ai-feedback.jssrc/api/routes/moderation.jssrc/api/routes/auditLog.jssrc/api/routes/members.jssrc/api/routes/conversations.jssrc/api/routes/tickets.jssrc/api/routes/tempRoles.jssrc/api/routes/guilds.js
{src,web}/**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Winston logger from
src/logger.js, NEVER useconsole.*
Files:
src/api/routes/ai-feedback.jsweb/src/types/discord.tssrc/api/routes/moderation.jsweb/src/contexts/dashboard-guild-context.tsxweb/src/lib/dashboard-roles.tsweb/src/components/layout/server-selector.tsxsrc/api/routes/auditLog.jssrc/api/routes/members.jssrc/api/routes/conversations.jssrc/api/routes/tickets.jsweb/src/lib/bot-api-proxy.tssrc/api/routes/tempRoles.jsweb/src/app/api/guilds/[guildId]/analytics/route.tssrc/api/routes/guilds.jsweb/src/components/layout/dashboard-shell.tsxweb/src/lib/discord.server.tsweb/src/components/layout/sidebar.tsx
src/**/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use parameterized SQL queries — Never use string interpolation in database queries
Files:
src/api/routes/ai-feedback.jssrc/api/routes/moderation.jssrc/api/routes/auditLog.jssrc/api/routes/members.jssrc/api/routes/conversations.jssrc/api/routes/tickets.jssrc/api/routes/tempRoles.jssrc/api/routes/guilds.js
src/api/routes/**/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
src/api/routes/**/*.{js,ts}: All API routes must use auth middleware if handling sensitive data and be mounted insrc/api/server.js
API endpoints must include tests intests/api/directory
Files:
src/api/routes/ai-feedback.jssrc/api/routes/moderation.jssrc/api/routes/auditLog.jssrc/api/routes/members.jssrc/api/routes/conversations.jssrc/api/routes/tickets.jssrc/api/routes/tempRoles.jssrc/api/routes/guilds.js
web/**/*.{ts,tsx,jsx,js}
📄 CodeRabbit inference engine (AGENTS.md)
Use Next.js 16 App Router for web dashboard, with Discord OAuth2 authentication, dark/light theme support, and mobile-responsive design
Files:
web/src/types/discord.tsweb/src/contexts/dashboard-guild-context.tsxweb/src/lib/dashboard-roles.tsweb/src/components/layout/server-selector.tsxweb/src/lib/bot-api-proxy.tsweb/src/app/api/guilds/[guildId]/analytics/route.tsweb/src/components/layout/dashboard-shell.tsxweb/src/lib/discord.server.tsweb/src/components/layout/sidebar.tsx
tests/**/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Maintain 80% test coverage threshold — Never lower the coverage requirement
Files:
tests/api/routes/guilds.test.js
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-02T21:23:59.491Z
Learning: Applies to web/**/*.{ts,tsx,jsx,js} : Use Next.js 16 App Router for web dashboard, with Discord OAuth2 authentication, dark/light theme support, and mobile-responsive design
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-02T21:23:59.491Z
Learning: Applies to src/api/routes/**/*.{js,ts} : All API routes must use auth middleware if handling sensitive data and be mounted in `src/api/server.js`
📚 Learning: 2026-03-02T21:23:59.491Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-02T21:23:59.491Z
Learning: Applies to web/**/*.{ts,tsx,jsx,js} : Use Next.js 16 App Router for web dashboard, with Discord OAuth2 authentication, dark/light theme support, and mobile-responsive design
Applied to files:
src/api/routes/ai-feedback.jsweb/src/contexts/dashboard-guild-context.tsxweb/src/lib/dashboard-roles.tsweb/src/components/layout/server-selector.tsxsrc/api/routes/conversations.jsweb/src/lib/bot-api-proxy.tssrc/api/routes/guilds.jsweb/src/components/layout/dashboard-shell.tsxweb/src/components/layout/sidebar.tsx
📚 Learning: 2026-03-02T21:23:59.491Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-02T21:23:59.491Z
Learning: Applies to src/modules/**/*.{js,ts} : Use Discord cache utilities — `src/utils/discordCache.js` for channels/roles/members, `src/utils/reputationCache.js` for leaderboard/rank data
Applied to files:
web/src/types/discord.tsweb/src/lib/dashboard-roles.tssrc/api/routes/members.jsweb/src/lib/discord.server.ts
📚 Learning: 2026-03-02T21:23:59.491Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-02T21:23:59.491Z
Learning: Applies to src/api/routes/**/*.{js,ts} : All API routes must use auth middleware if handling sensitive data and be mounted in `src/api/server.js`
Applied to files:
src/api/routes/moderation.jssrc/api/routes/auditLog.jssrc/api/routes/members.jssrc/api/routes/tickets.jssrc/api/routes/tempRoles.js
🧬 Code graph analysis (15)
src/api/routes/ai-feedback.js (1)
src/api/routes/guilds.js (1)
requireRole(343-391)
src/api/routes/moderation.js (1)
src/api/routes/guilds.js (1)
requireRole(343-391)
web/src/contexts/dashboard-guild-context.tsx (2)
web/src/types/discord.ts (1)
MutualGuild(19-23)web/src/lib/guild-selection.ts (2)
SELECTED_GUILD_KEY(1-1)broadcastSelectedGuild(9-26)
web/src/lib/dashboard-roles.ts (1)
src/api/routes/guilds.js (2)
permissions(218-218)p(42-42)
web/src/components/layout/server-selector.tsx (1)
web/src/contexts/dashboard-guild-context.tsx (1)
useDashboardGuild(26-32)
src/api/routes/auditLog.js (1)
src/api/routes/guilds.js (8)
requireRole(343-391)validateGuild(400-410)req(401-401)req(461-461)req(897-897)req(1009-1009)req(1565-1565)req(1670-1670)
src/api/routes/conversations.js (1)
src/api/routes/guilds.js (8)
requireRole(343-391)validateGuild(400-410)req(401-401)req(461-461)req(897-897)req(1009-1009)req(1565-1565)req(1670-1670)
src/api/routes/tickets.js (1)
src/api/routes/guilds.js (7)
requireRole(343-391)req(401-401)req(461-461)req(897-897)req(1009-1009)req(1565-1565)req(1670-1670)
web/src/lib/bot-api-proxy.ts (1)
web/src/lib/dashboard-roles.ts (1)
permissionsToDashboardRole(26-39)
src/api/routes/tempRoles.js (1)
src/api/routes/guilds.js (7)
requireRole(343-391)req(401-401)req(461-461)req(897-897)req(1009-1009)req(1565-1565)req(1670-1670)
web/src/app/api/guilds/[guildId]/analytics/route.ts (1)
web/src/lib/bot-api-proxy.ts (1)
authorizeGuildRole(55-92)
src/api/routes/guilds.js (3)
web/src/lib/dashboard-roles.ts (1)
permissionsToDashboardRole(26-39)src/api/utils/sessionStore.js (1)
getSessionToken(138-145)src/api/utils/discordApi.js (2)
guilds(62-62)fetchUserGuilds(36-72)
web/src/components/layout/dashboard-shell.tsx (3)
web/src/contexts/dashboard-guild-context.tsx (1)
DashboardGuildProvider(34-127)web/src/components/layout/server-selector.tsx (1)
ServerSelector(22-122)web/src/components/layout/sidebar.tsx (1)
Sidebar(49-91)
web/src/lib/discord.server.ts (2)
web/src/lib/dashboard-roles.ts (1)
permissionsToDashboardRole(26-39)web/src/types/discord.ts (1)
MutualGuild(19-23)
web/src/components/layout/sidebar.tsx (3)
web/src/lib/dashboard-roles.ts (2)
DashboardRole(14-14)canAccessRoute(62-67)web/src/contexts/dashboard-guild-context.tsx (1)
DashboardGuildContext(24-24)web/src/lib/utils.ts (1)
cn(4-6)
🔇 Additional comments (15)
src/api/routes/ai-feedback.js (1)
11-11: Role-based auth migration is correctly applied here.
requireRole('admin')is consistently wired on both sensitive endpoints, andvalidateGuildremains in place.Also applies to: 90-91, 196-197
src/api/routes/moderation.js (1)
10-10: Moderator role gate migration looks correct.Global route auth now consistently uses
requireRole('moderator')with the guildId adapter.Also applies to: 34-34
web/src/components/layout/dashboard-shell.tsx (1)
1-1: Provider wiring is clean and correctly scoped.Wrapping the shell with
DashboardGuildProvideris a good integration point for role-aware sidebar/server selection.Also applies to: 18-35
src/api/routes/auditLog.js (1)
11-11: Admin-role guard migration is applied correctly on both endpoints.No regressions observed in middleware order (
rateLimit→requireRole→validateGuild).Also applies to: 148-148, 200-200
web/src/types/discord.ts (1)
16-23: Type updates are coherent with the role-based dashboard model.The new
GuildAccessRoleandMutualGuild.accessextension look good.src/api/routes/members.js (1)
15-15: Admin role enforcement is consistently applied across members routes.The migration to
requireRole('admin')is uniform and keeps guild validation in the chain.Also applies to: 87-88, 297-297, 564-565, 762-763, 893-894
src/api/routes/tempRoles.js (1)
74-74: Good role-gating on list/create temp-role endpoints.
requireRole('moderator')is correctly applied for GET and POST flows.Also applies to: 193-193
web/src/lib/discord.server.ts (1)
4-4: Access-role derivation in mutual guild mapping looks correct.Computing
accessfrom permissions and filtering null-access guilds is aligned with the new role-based dashboard flow.Also applies to: 227-231
web/src/app/api/guilds/[guildId]/analytics/route.ts (1)
21-21: Role-based authorization is correctly applied for this route.Good migration to
authorizeGuildRolewith an explicit minimum role.src/api/routes/tickets.js (1)
11-11: RBAC guard migration is consistent across ticket endpoints.All updated ticket routes keep explicit admin-level protection with
requireRole('admin').Also applies to: 76-76, 204-204, 336-336
tests/api/routes/guilds.test.js (1)
223-314: Nice test expansion for role mapping and access boundaries.These additions improve confidence around viewer/admin behavior and endpoint authorization expectations.
Also applies to: 368-443
src/api/routes/conversations.js (1)
12-12: Authorization migration is applied consistently on conversation routes.The updated routes all retain explicit admin-level protection via
requireRole('admin').Also applies to: 207-207, 432-432, 617-617, 781-781, 970-970
web/src/components/layout/server-selector.tsx (1)
23-23: Good context integration and no-op select guard.Using shared guild context here simplifies state flow and avoids unnecessary selection churn.
Also applies to: 101-101
web/src/lib/bot-api-proxy.ts (1)
26-104: Role-based authorization scaffolding is clean and backward-compatible.The new
authorizeGuildRoleflow plusauthorizeGuildAdminwrapper keeps existing behavior while enabling finer-grained route checks.web/src/contexts/dashboard-guild-context.tsx (1)
41-49: 🧹 Nitpick | 🔵 TrivialRemove duplicate localStorage persistence in
selectGuild.
broadcastSelectedGuildalready persists the selected guild key, so the localsetItemhere is redundant.Refactor
const selectGuild = useCallback((guild: MutualGuild) => { setSelectedGuildState(guild); - try { - localStorage.setItem(SELECTED_GUILD_KEY, guild.id); - } catch { - /* localStorage unavailable */ - } broadcastSelectedGuild(guild.id); }, []);Likely an incorrect or invalid review comment.
src/api/routes/guilds.js
Outdated
| /** @deprecated Use requireRole('admin') instead. Kept for backward compatibility. */ | ||
| function isOAuthGuildAdmin(user, guildId) { | ||
| return hasOAuthGuildPermission(user, guildId, ADMINISTRATOR_FLAG); | ||
| return getOAuthDashboardRole(user, guildId).then((r) => r === 'owner' || r === 'admin'); | ||
| } | ||
|
|
||
| /** | ||
| * Check if an OAuth2 user has moderator permissions on a guild. | ||
| * Moderator = ADMINISTRATOR or MANAGE_GUILD, aligning with the slash-command isModerator check. | ||
| * | ||
| * @param {Object} user - Decoded JWT user payload | ||
| * @param {string} guildId - Guild ID to check | ||
| * @returns {Promise<boolean>} True if user has moderator-level permission | ||
| */ | ||
| /** @deprecated Use requireRole('moderator') instead. Kept for backward compatibility. */ | ||
| function isOAuthGuildModerator(user, guildId) { | ||
| return hasOAuthGuildPermission(user, guildId, ADMINISTRATOR_FLAG | MANAGE_GUILD_FLAG); | ||
| return getOAuthDashboardRole(user, guildId).then( | ||
| (r) => r === 'owner' || r === 'admin' || r === 'moderator', | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
rg -n -C2 "function isOAuthGuild(Admin|Moderator)|getOAuthDashboardRole\(.*\)\.then" src/api/routes/guilds.jsRepository: VolvoxLLC/volvox-bot
Length of output: 591
Convert deprecated wrappers to async/await syntax.
The functions isOAuthGuildAdmin (line 270) and isOAuthGuildModerator (line 275) use Promise .then() chains, which violates the async/await guideline. Convert both to async functions:
♻️ Refactor
/** `@deprecated` Use requireRole('admin') instead. Kept for backward compatibility. */
-function isOAuthGuildAdmin(user, guildId) {
- return getOAuthDashboardRole(user, guildId).then((r) => r === 'owner' || r === 'admin');
+async function isOAuthGuildAdmin(user, guildId) {
+ const role = await getOAuthDashboardRole(user, guildId);
+ return role === 'owner' || role === 'admin';
}
/** `@deprecated` Use requireRole('moderator') instead. Kept for backward compatibility. */
-function isOAuthGuildModerator(user, guildId) {
- return getOAuthDashboardRole(user, guildId).then(
- (r) => r === 'owner' || r === 'admin' || r === 'moderator',
- );
+async function isOAuthGuildModerator(user, guildId) {
+ const role = await getOAuthDashboardRole(user, guildId);
+ return role === 'owner' || role === 'admin' || role === 'moderator';
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** @deprecated Use requireRole('admin') instead. Kept for backward compatibility. */ | |
| function isOAuthGuildAdmin(user, guildId) { | |
| return hasOAuthGuildPermission(user, guildId, ADMINISTRATOR_FLAG); | |
| return getOAuthDashboardRole(user, guildId).then((r) => r === 'owner' || r === 'admin'); | |
| } | |
| /** | |
| * Check if an OAuth2 user has moderator permissions on a guild. | |
| * Moderator = ADMINISTRATOR or MANAGE_GUILD, aligning with the slash-command isModerator check. | |
| * | |
| * @param {Object} user - Decoded JWT user payload | |
| * @param {string} guildId - Guild ID to check | |
| * @returns {Promise<boolean>} True if user has moderator-level permission | |
| */ | |
| /** @deprecated Use requireRole('moderator') instead. Kept for backward compatibility. */ | |
| function isOAuthGuildModerator(user, guildId) { | |
| return hasOAuthGuildPermission(user, guildId, ADMINISTRATOR_FLAG | MANAGE_GUILD_FLAG); | |
| return getOAuthDashboardRole(user, guildId).then( | |
| (r) => r === 'owner' || r === 'admin' || r === 'moderator', | |
| ); | |
| /** `@deprecated` Use requireRole('admin') instead. Kept for backward compatibility. */ | |
| async function isOAuthGuildAdmin(user, guildId) { | |
| const role = await getOAuthDashboardRole(user, guildId); | |
| return role === 'owner' || role === 'admin'; | |
| } | |
| /** `@deprecated` Use requireRole('moderator') instead. Kept for backward compatibility. */ | |
| async function isOAuthGuildModerator(user, guildId) { | |
| const role = await getOAuthDashboardRole(user, guildId); | |
| return role === 'owner' || role === 'admin' || role === 'moderator'; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/routes/guilds.js` around lines 268 - 277, Convert the deprecated
wrappers isOAuthGuildAdmin and isOAuthGuildModerator to async functions using
await instead of .then(): mark both functions async, call const r = await
getOAuthDashboardRole(user, guildId) inside each, and return the same boolean
expressions (r === 'owner' || r === 'admin' for isOAuthGuildAdmin; r === 'owner'
|| r === 'admin' || r === 'moderator' for isOAuthGuildModerator); keep the
existing JSDoc deprecation comments and function names to preserve backward
compatibility.
src/api/routes/guilds.js
Outdated
| router.post('/:id/actions', requireRole('admin'), validateGuild, async (req, res) => { | ||
| if (req.authMethod !== 'api-secret') { | ||
| return res.status(403).json({ error: 'Actions endpoint requires API secret authentication' }); | ||
| } |
There was a problem hiding this comment.
Short-circuit API-secret auth before role checks on POST /:id/actions.
Line 1661 applies requireRole('admin'), but Lines 1662-1664 then enforce API-secret-only access. This causes avoidable OAuth/session/Discord permission checks for requests that should be rejected immediately.
⚡ Proposed fix
-router.post('/:id/actions', requireRole('admin'), validateGuild, async (req, res) => {
- if (req.authMethod !== 'api-secret') {
- return res.status(403).json({ error: 'Actions endpoint requires API secret authentication' });
- }
+router.post(
+ '/:id/actions',
+ (req, res, next) => {
+ if (req.authMethod !== 'api-secret') {
+ return res.status(403).json({ error: 'Actions endpoint requires API secret authentication' });
+ }
+ return next();
+ },
+ validateGuild,
+ async (req, res) => {
if (!req.body) {
return res.status(400).json({ error: 'Missing request body' });
}
@@
-});
+ },
+);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| router.post('/:id/actions', requireRole('admin'), validateGuild, async (req, res) => { | |
| if (req.authMethod !== 'api-secret') { | |
| return res.status(403).json({ error: 'Actions endpoint requires API secret authentication' }); | |
| } | |
| router.post( | |
| '/:id/actions', | |
| (req, res, next) => { | |
| if (req.authMethod !== 'api-secret') { | |
| return res.status(403).json({ error: 'Actions endpoint requires API secret authentication' }); | |
| } | |
| return next(); | |
| }, | |
| validateGuild, | |
| async (req, res) => { | |
| if (!req.body) { | |
| return res.status(400).json({ error: 'Missing request body' }); | |
| } | |
| // ... [remainder of route handler] | |
| }, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/routes/guilds.js` around lines 1661 - 1664, The API-secret check
(req.authMethod !== 'api-secret') must run before any role or guild validation
to avoid unnecessary OAuth/session/Discord permission checks; update the POST
'/:id/actions' route so the API-secret guard executes first (either by moving
the check into a new middleware placed before requireRole('admin') and
validateGuild, or by reordering middlewares) and return 403 immediately when
authMethod is not 'api-secret'; reference the existing route handler,
requireRole, validateGuild, and the req.authMethod check when applying the
change.
| const guildContext = useContext(DashboardGuildContext); | ||
| const role: DashboardRole | null = guildContext?.selectedGuild?.access ?? null; | ||
|
|
||
| const visibleNav = role === null ? navigation : navigation.filter((item) => canAccessRoute(role, item.href)); |
There was a problem hiding this comment.
Do not bypass route gating when role is unknown.
When role is null, this renders the full navigation. That bypasses role-based visibility and exposes links users may not have access to.
Suggested fix
- const visibleNav = role === null ? navigation : navigation.filter((item) => canAccessRoute(role, item.href));
+ const visibleNav = navigation.filter((item) => canAccessRoute(role, item.href));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const visibleNav = role === null ? navigation : navigation.filter((item) => canAccessRoute(role, item.href)); | |
| const visibleNav = navigation.filter((item) => canAccessRoute(role, item.href)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/layout/sidebar.tsx` at line 54, The code currently sets
visibleNav = role === null ? navigation : navigation.filter(...) which exposes
all links when role is null; change it so unknown role does not bypass
gating—e.g., compute visibleNav by filtering navigation and only including items
when role is non-null and canAccessRoute(role, item.href) returns true (or
return an empty array when role is null). Update the assignment for visibleNav
to use role and canAccessRoute consistently (referencing the visibleNav
variable, role variable, navigation array, and canAccessRoute function) so no
links are shown when role is unknown.
| setGuilds(fetchedGuilds); | ||
|
|
||
| let restored = false; | ||
| try { | ||
| const savedId = localStorage.getItem(SELECTED_GUILD_KEY); | ||
| if (savedId) { | ||
| const saved = fetchedGuilds.find((g) => g.id === savedId); | ||
| if (saved) { | ||
| setSelectedGuildState(saved); | ||
| restored = true; | ||
| } | ||
| } | ||
| } catch { | ||
| /* localStorage unavailable */ | ||
| } | ||
|
|
||
| if (!restored && fetchedGuilds.length > 0) { | ||
| setSelectedGuildState(fetchedGuilds[0]); | ||
| try { | ||
| localStorage.setItem(SELECTED_GUILD_KEY, fetchedGuilds[0].id); | ||
| } catch { | ||
| /* ignore */ | ||
| } | ||
| broadcastSelectedGuild(fetchedGuilds[0].id); | ||
| } |
There was a problem hiding this comment.
Clear selectedGuild when the fetched guild list is empty.
If the latest fetch returns no guilds, selectedGuild can remain stale from a prior load, causing downstream state mismatch.
Suggested fix
setGuilds(fetchedGuilds);
+
+ if (fetchedGuilds.length === 0) {
+ setSelectedGuildState(null);
+ try {
+ localStorage.removeItem(SELECTED_GUILD_KEY);
+ } catch {
+ /* localStorage unavailable */
+ }
+ return;
+ }
let restored = false;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| setGuilds(fetchedGuilds); | |
| let restored = false; | |
| try { | |
| const savedId = localStorage.getItem(SELECTED_GUILD_KEY); | |
| if (savedId) { | |
| const saved = fetchedGuilds.find((g) => g.id === savedId); | |
| if (saved) { | |
| setSelectedGuildState(saved); | |
| restored = true; | |
| } | |
| } | |
| } catch { | |
| /* localStorage unavailable */ | |
| } | |
| if (!restored && fetchedGuilds.length > 0) { | |
| setSelectedGuildState(fetchedGuilds[0]); | |
| try { | |
| localStorage.setItem(SELECTED_GUILD_KEY, fetchedGuilds[0].id); | |
| } catch { | |
| /* ignore */ | |
| } | |
| broadcastSelectedGuild(fetchedGuilds[0].id); | |
| } | |
| setGuilds(fetchedGuilds); | |
| if (fetchedGuilds.length === 0) { | |
| setSelectedGuildState(null); | |
| try { | |
| localStorage.removeItem(SELECTED_GUILD_KEY); | |
| } catch { | |
| /* localStorage unavailable */ | |
| } | |
| return; | |
| } | |
| let restored = false; | |
| try { | |
| const savedId = localStorage.getItem(SELECTED_GUILD_KEY); | |
| if (savedId) { | |
| const saved = fetchedGuilds.find((g) => g.id === savedId); | |
| if (saved) { | |
| setSelectedGuildState(saved); | |
| restored = true; | |
| } | |
| } | |
| } catch { | |
| /* localStorage unavailable */ | |
| } | |
| if (!restored && fetchedGuilds.length > 0) { | |
| setSelectedGuildState(fetchedGuilds[0]); | |
| try { | |
| localStorage.setItem(SELECTED_GUILD_KEY, fetchedGuilds[0].id); | |
| } catch { | |
| /* ignore */ | |
| } | |
| broadcastSelectedGuild(fetchedGuilds[0].id); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/contexts/dashboard-guild-context.tsx` around lines 74 - 98, When
handling the fetchedGuilds result in the block that calls setGuilds, ensure you
clear any stale selected guild if fetchedGuilds is empty: if
fetchedGuilds.length === 0 then call setSelectedGuildState(null) (or the
appropriate empty value), remove the SELECTED_GUILD_KEY from localStorage inside
a try/catch, and call broadcastSelectedGuild(null) (or the appropriate sentinel)
so downstream state is not left stale; update the logic around
setGuilds/fetchedGuilds to perform these clear actions before early-returning or
proceeding to the restore/first-item selection.
web/src/lib/dashboard-roles.ts
Outdated
| /** | ||
| * Dashboard role derivation from Discord guild permission bitfield. | ||
| * Must stay in sync with backend (src/api/routes/guilds.js permissionsToDashboardRole). | ||
| */ |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Centralize role derivation in one shared module.
Line 3 explicitly notes manual sync with src/api/routes/guilds.js, but role flags/order/mapping are duplicated across two files. This creates drift risk between dashboard gating and API authorization logic. Please extract the role model and permission-to-role mapper into a single shared source consumed by both modules.
Also applies to: 16-39
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/lib/dashboard-roles.ts` around lines 1 - 4, The dashboard role flags
and mapping are duplicated; extract the canonical role model and the
permissionsToDashboardRole mapper into a single shared module (exporting the
role enum/array, permission bit constants, and the permissionsToDashboardRole
function), then update both the existing dashboard-roles implementation and the
backend guilds route to import and use those exports (replace local role
flags/order/mapping with imports and delegate role derivation to the shared
permissionsToDashboardRole). Ensure the shared module exports the same
identifiers used elsewhere (role list/name constants and
permissionsToDashboardRole) so callers need only swap local definitions for
imports.
web/src/lib/dashboard-roles.ts
Outdated
| export function canAccessRoute(role: DashboardRole | null, path: string): boolean { | ||
| if (role === null) return false; | ||
| const min = NAV_MIN_ROLE[path]; | ||
| if (!min) return true; | ||
| return hasMinimumRole(role, min); | ||
| } |
There was a problem hiding this comment.
canAccessRoute is fail-open for unmapped paths.
Line 65 returns true whenever a path is not present in NAV_MIN_ROLE. That means new or nested /dashboard/* routes become accessible until someone updates the map.
🔒 Proposed fix (fail-closed for dashboard paths + nested route matching)
export function canAccessRoute(role: DashboardRole | null, path: string): boolean {
if (role === null) return false;
- const min = NAV_MIN_ROLE[path];
- if (!min) return true;
- return hasMinimumRole(role, min);
+ const normalizedPath = path.replace(/\/+$/, '');
+ const min = Object.entries(NAV_MIN_ROLE)
+ .filter(([route]) => normalizedPath === route || normalizedPath.startsWith(`${route}/`))
+ .sort((a, b) => b[0].length - a[0].length)[0]?.[1];
+
+ if (!min) return !normalizedPath.startsWith('/dashboard');
+ return hasMinimumRole(role, min);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/lib/dashboard-roles.ts` around lines 62 - 67, canAccessRoute
currently returns true when NAV_MIN_ROLE[path] is undefined, which leaves
unmapped dashboard routes open; update it to fail-closed for dashboard paths and
perform nested route matching: compute the best matching min role by finding the
longest NAV_MIN_ROLE key that is a prefix of the requested path (or exact
match), then if a match is found call hasMinimumRole(role, min); if no match and
path startsWith('/dashboard') return false; otherwise (non-dashboard unmapped
paths) return true. Update the function canAccessRoute and reference
NAV_MIN_ROLE and hasMinimumRole accordingly.
|
| Filename | Overview |
|---|---|
| src/modules/backup.js | converted synchronous fs operations to async fs/promises - all functions properly await async operations, error handling preserved |
| src/api/routes/backup.js | route handlers updated to async/await pattern - all backup module calls properly awaited, error handling maintained |
| tests/modules/backup.test.js | test functions converted to async/await - all assertions properly use await, error tests use rejects.toThrow pattern |
Last reviewed commit: 579160e
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/routes/backup.js`:
- Around line 187-190: The async route handlers that call listBackups (and the
similar prune handler at lines 423-443) lack local error handling and can bypass
the route's JSON error contract; wrap the await calls in try/catch blocks inside
the route functions (the async (_req, res) => { ... } handler that calls
listBackups and the prune handler that calls the prune function), and on error
return the same JSON error shape used by neighboring endpoints (e.g.,
res.status(500).json({ error: 'backup_failed', message: err.message }) or call
next(err) if the router uses a centralized error formatter) so all failures are
normalized to the route-specific JSON error response.
In `@src/modules/backup.js`:
- Around line 285-291: Replace the separate access(filePath) check and
readFile(filePath, 'utf8') with a single readFile call and normalize errors:
call readFile(filePath, 'utf8') inside a try/catch, and if the caught error.code
=== 'ENOENT' throw new Error(`Backup not found: ${id}`) (otherwise rethrow or
wrap other errors); remove the prior access(...) branch so there is no TOCTOU
window between access and readFile for filePath.
- Around line 259-260: The current Promise.all(files.map(...)) in the backup
listing causes an unbounded stat fan-out; replace it with a bounded approach by
iterating files with a controlled concurrency or sequential loop to avoid
hitting FD limits: either use a simple for...of loop to await
parseBackupMeta(filename, dir) one-by-one and push non-null results into
results, or use a small concurrency pool (e.g., limit 5-10) around
parseBackupMeta to process files in batches; ensure you still filter(Boolean)
and sort the final backups as before (references: files, parseBackupMeta,
results, backups).
- Around line 402-409: The scheduled backup IIFE can overlap because it launches
async work and returns immediately; add an in-flight guard (e.g., a
module-scoped boolean like isBackupRunning or a simple mutex) checked at the
start of the interval handler to skip if a run is already active, set the flag
true before calling createBackup(backupDir) and pruneBackups(retention,
backupDir), and ensure the flag is cleared in a finally block so it is reset
even on errors; also log when a tick is skipped to aid observability.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/api/routes/backup.jssrc/modules/backup.jstests/modules/backup.test.js
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (6)
tests/**/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Maintain 80% test coverage threshold — Never lower the coverage requirement
Files:
tests/modules/backup.test.js
src/**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{js,ts,jsx,tsx}: Use ESM only — Useimport/export, no CommonJS
Use single quotes for strings — No double quotes except in JSON
Always require semicolons at end of statements
Use 2-space indent, enforced by Biome
Always use async/await for asynchronous operations and promise handling
Files:
src/api/routes/backup.jssrc/modules/backup.js
{src,web}/**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Winston logger from
src/logger.js, NEVER useconsole.*
Files:
src/api/routes/backup.jssrc/modules/backup.js
src/**/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use parameterized SQL queries — Never use string interpolation in database queries
Files:
src/api/routes/backup.jssrc/modules/backup.js
src/api/routes/**/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
src/api/routes/**/*.{js,ts}: All API routes must use auth middleware if handling sensitive data and be mounted insrc/api/server.js
API endpoints must include tests intests/api/directory
Files:
src/api/routes/backup.js
src/modules/**/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
src/modules/**/*.{js,ts}: Config section additions MUST be added toSAFE_CONFIG_KEYSinsrc/api/utils/configAllowlist.jsto enable API saves
Gate all community features behindconfig.<feature>.enabledconfiguration checks
Redis caching should usesrc/utils/cache.jsfor generic caching with Redis + in-memory fallback
Use Discord cache utilities —src/utils/discordCache.jsfor channels/roles/members,src/utils/reputationCache.jsfor leaderboard/rank data
Files:
src/modules/backup.js
🧠 Learnings (2)
📚 Learning: 2026-03-02T21:23:59.491Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-02T21:23:59.491Z
Learning: Applies to src/**/*.{js,ts,jsx,tsx} : Always use async/await for asynchronous operations and promise handling
Applied to files:
src/api/routes/backup.jssrc/modules/backup.js
📚 Learning: 2026-03-02T21:23:59.491Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-02T21:23:59.491Z
Learning: Applies to src/modules/**/*.{js,ts} : Config section additions MUST be added to `SAFE_CONFIG_KEYS` in `src/api/utils/configAllowlist.js` to enable API saves
Applied to files:
src/modules/backup.js
🧬 Code graph analysis (3)
tests/modules/backup.test.js (1)
src/modules/backup.js (11)
createBackup(188-210)backups(260-260)listBackups(249-263)payload(114-114)payload(139-139)payload(194-194)payload(308-308)readBackup(273-297)i(62-62)deleted(363-363)pruneBackups(335-377)
src/api/routes/backup.js (1)
src/modules/backup.js (11)
backups(260-260)listBackups(249-263)createBackup(188-210)payload(114-114)payload(139-139)payload(194-194)payload(308-308)readBackup(273-297)id(200-200)deleted(363-363)pruneBackups(335-377)
src/modules/backup.js (1)
src/api/routes/backup.js (5)
filename(64-64)filename(286-286)payload(63-63)payload(131-131)payload(285-285)
🔇 Additional comments (3)
src/modules/backup.js (1)
39-45: Clean async migration in core backup I/O paths.The conversion to promise-based fs operations is consistent and keeps these hot paths non-blocking.
Also applies to: 188-199, 219-227
tests/modules/backup.test.js (1)
194-291: Async test migration looks correct.These updates correctly await async backup operations and use async rejection assertions, which improves test reliability.
src/api/routes/backup.js (1)
232-235: Good async/await adoption with preserved response behavior.The create and download handlers now await module calls while keeping explicit response shaping.
Also applies to: 281-289
| async (_req, res) => { | ||
| const backups = await listBackups(); | ||
| res.json(backups); | ||
| }, |
There was a problem hiding this comment.
Normalize error responses for async list/prune handlers.
These handlers await filesystem-backed operations without local error handling, so failures can bypass the route-specific JSON error contract used by neighboring endpoints.
🧩 Suggested consistency fix
router.get(
'/',
(req, res, next) => requireGlobalAdmin('Backup access', req, res, next),
async (_req, res) => {
- const backups = await listBackups();
- res.json(backups);
+ try {
+ const backups = await listBackups();
+ return res.json(backups);
+ } catch (err) {
+ return res.status(500).json({ error: 'Failed to list backups', details: err.message });
+ }
},
);
@@
router.post(
'/prune',
(req, res, next) => requireGlobalAdmin('Backup access', req, res, next),
async (req, res) => {
@@
- const deleted = await pruneBackups(retention);
- return res.json({ deleted, count: deleted.length });
+ try {
+ const deleted = await pruneBackups(retention);
+ return res.json({ deleted, count: deleted.length });
+ } catch (err) {
+ return res.status(500).json({ error: 'Failed to prune backups', details: err.message });
+ }
},
);Also applies to: 423-443
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/routes/backup.js` around lines 187 - 190, The async route handlers
that call listBackups (and the similar prune handler at lines 423-443) lack
local error handling and can bypass the route's JSON error contract; wrap the
await calls in try/catch blocks inside the route functions (the async (_req,
res) => { ... } handler that calls listBackups and the prune handler that calls
the prune function), and on error return the same JSON error shape used by
neighboring endpoints (e.g., res.status(500).json({ error: 'backup_failed',
message: err.message }) or call next(err) if the router uses a centralized error
formatter) so all failures are normalized to the route-specific JSON error
response.
| const results = await Promise.all(files.map((filename) => parseBackupMeta(filename, dir))); | ||
| const backups = results.filter(Boolean).sort((a, b) => new Date(b.createdAt) - new Date(a.createdAt)); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid unbounded stat fan-out in backup listing.
Promise.all(files.map(...)) scales linearly with file count and can hit file descriptor limits under large backup directories. Prefer sequential iteration or a small concurrency cap.
♻️ Suggested bounded approach
- const results = await Promise.all(files.map((filename) => parseBackupMeta(filename, dir)));
- const backups = results.filter(Boolean).sort((a, b) => new Date(b.createdAt) - new Date(a.createdAt));
+ const backups = [];
+ for (const filename of files) {
+ const meta = await parseBackupMeta(filename, dir);
+ if (meta) backups.push(meta);
+ }
+ backups.sort((a, b) => new Date(b.createdAt) - new Date(a.createdAt));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/backup.js` around lines 259 - 260, The current
Promise.all(files.map(...)) in the backup listing causes an unbounded stat
fan-out; replace it with a bounded approach by iterating files with a controlled
concurrency or sequential loop to avoid hitting FD limits: either use a simple
for...of loop to await parseBackupMeta(filename, dir) one-by-one and push
non-null results into results, or use a small concurrency pool (e.g., limit
5-10) around parseBackupMeta to process files in batches; ensure you still
filter(Boolean) and sort the final backups as before (references: files,
parseBackupMeta, results, backups).
| try { | ||
| await access(filePath); | ||
| } catch { | ||
| throw new Error(`Backup not found: ${id}`); | ||
| } | ||
|
|
||
| const raw = readFileSync(filePath, 'utf8'); | ||
| const raw = await readFile(filePath, 'utf8'); |
There was a problem hiding this comment.
Remove TOCTOU between existence check and file read.
The access() then readFile() sequence can race; if the file is removed in-between, raw ENOENT escapes your normalized “not found” handling and can leak internal path details upstream.
🛠️ Suggested fix (single read path with normalized errors)
- try {
- await access(filePath);
- } catch {
- throw new Error(`Backup not found: ${id}`);
- }
-
- const raw = await readFile(filePath, 'utf8');
+ let raw;
+ try {
+ raw = await readFile(filePath, 'utf8');
+ } catch (err) {
+ if (err && typeof err === 'object' && 'code' in err && err.code === 'ENOENT') {
+ throw new Error(`Backup not found: ${id}`);
+ }
+ throw new Error(`Failed to read backup: ${id}`);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| await access(filePath); | |
| } catch { | |
| throw new Error(`Backup not found: ${id}`); | |
| } | |
| const raw = readFileSync(filePath, 'utf8'); | |
| const raw = await readFile(filePath, 'utf8'); | |
| let raw; | |
| try { | |
| raw = await readFile(filePath, 'utf8'); | |
| } catch (err) { | |
| if (err && typeof err === 'object' && 'code' in err && err.code === 'ENOENT') { | |
| throw new Error(`Backup not found: ${id}`); | |
| } | |
| throw new Error(`Failed to read backup: ${id}`); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/backup.js` around lines 285 - 291, Replace the separate
access(filePath) check and readFile(filePath, 'utf8') with a single readFile
call and normalize errors: call readFile(filePath, 'utf8') inside a try/catch,
and if the caught error.code === 'ENOENT' throw new Error(`Backup not found:
${id}`) (otherwise rethrow or wrap other errors); remove the prior access(...)
branch so there is no TOCTOU window between access and readFile for filePath.
| void (async () => { | ||
| try { | ||
| await createBackup(backupDir); | ||
| await pruneBackups(retention, backupDir); | ||
| } catch (err) { | ||
| logError('Scheduled backup failed', { error: err.message }); | ||
| } | ||
| })(); |
There was a problem hiding this comment.
Prevent overlapping scheduled backup runs.
The interval callback launches async work and returns immediately; if one run exceeds intervalMs, subsequent ticks can overlap and race create/prune operations.
🛡️ Suggested in-flight guard
let scheduledBackupInterval = null;
+let scheduledBackupInFlight = false;
@@
scheduledBackupInterval = setInterval(() => {
+ if (scheduledBackupInFlight) {
+ warn('Previous scheduled backup run is still in progress — skipping tick');
+ return;
+ }
void (async () => {
+ scheduledBackupInFlight = true;
try {
await createBackup(backupDir);
await pruneBackups(retention, backupDir);
} catch (err) {
logError('Scheduled backup failed', { error: err.message });
+ } finally {
+ scheduledBackupInFlight = false;
}
})();
}, intervalMs);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/backup.js` around lines 402 - 409, The scheduled backup IIFE can
overlap because it launches async work and returns immediately; add an in-flight
guard (e.g., a module-scoped boolean like isBackupRunning or a simple mutex)
checked at the start of the interval handler to skip if a run is already active,
set the flag true before calling createBackup(backupDir) and
pruneBackups(retention, backupDir), and ensure the flag is cleared in a finally
block so it is reset even on errors; also log when a tick is skipped to aid
observability.
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/routes/guilds.js (1)
472-505:⚠️ Potential issue | 🟠 Major
accessvalues changed but API contract docs are now stale.The payload now returns
'owner'(Line 472) and can return'viewer'(Line 504), while the OpenAPI enum still documents[admin, moderator, bot-owner]. This creates contract drift for generated clients.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/routes/guilds.js` around lines 472 - 505, The API now returns new access values like 'owner' and 'viewer' (see the access property set on ownerGuilds and in the userGuilds mapping using permissionsToDashboardRole and botGuilds) but the OpenAPI enum still lists [admin, moderator, bot-owner]; update the OpenAPI contract to include the new values (and remove/alias deprecated ones as appropriate) so generated clients match the runtime output, or alternatively map the runtime role strings back to the documented enum inside the GET /guilds logic (e.g., adjust permissionsToDashboardRole or the object assigned to access) to restore compatibility between fetchUserGuilds/permissionsToDashboardRole and the OpenAPI enum.
♻️ Duplicate comments (2)
src/api/routes/guilds.js (2)
269-278:⚠️ Potential issue | 🟠 MajorConvert deprecated role wrappers to
async/await.These wrappers still use
.then(...), which is inconsistent with the file and repo async pattern. Please convert both toasyncfunctions withawait.♻️ Proposed fix
/** `@deprecated` Use requireRole('admin') instead. Kept for backward compatibility. */ -function isOAuthGuildAdmin(user, guildId) { - return getOAuthDashboardRole(user, guildId).then((r) => r === 'owner' || r === 'admin'); +async function isOAuthGuildAdmin(user, guildId) { + const role = await getOAuthDashboardRole(user, guildId); + return role === 'owner' || role === 'admin'; } /** `@deprecated` Use requireRole('moderator') instead. Kept for backward compatibility. */ -function isOAuthGuildModerator(user, guildId) { - return getOAuthDashboardRole(user, guildId).then( - (r) => r === 'owner' || r === 'admin' || r === 'moderator', - ); +async function isOAuthGuildModerator(user, guildId) { + const role = await getOAuthDashboardRole(user, guildId); + return role === 'owner' || role === 'admin' || role === 'moderator'; }As per coding guidelines: “Always use async/await for asynchronous operations and promise handling”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/routes/guilds.js` around lines 269 - 278, The two deprecated wrapper functions isOAuthGuildAdmin and isOAuthGuildModerator should be converted to async functions using await instead of .then; change their declarations to async, call const r = await getOAuthDashboardRole(user, guildId) and return the same boolean checks (r === 'owner' || r === 'admin' for isOAuthGuildAdmin, and r === 'owner' || r === 'admin' || r === 'moderator' for isOAuthGuildModerator), preserving the deprecation comments and letting errors propagate naturally.
1635-1638:⚠️ Potential issue | 🟠 MajorShort-circuit API-secret auth before role/guild checks on actions endpoint.
Line 1636 rejects non-API-secret requests, but only after
requireRole('admin')andvalidateGuildrun. Move API-secret gating to a pre-middleware so invalid requests fail immediately.⚡ Proposed fix
+function requireApiSecret(req, res, next) { + if (req.authMethod !== 'api-secret') { + return res.status(403).json({ error: 'Actions endpoint requires API secret authentication' }); + } + return next(); +} + -router.post('/:id/actions', requireRole('admin'), validateGuild, async (req, res) => { - if (req.authMethod !== 'api-secret') { - return res.status(403).json({ error: 'Actions endpoint requires API secret authentication' }); - } +router.post('/:id/actions', requireApiSecret, validateGuild, async (req, res) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/routes/guilds.js` around lines 1635 - 1638, The route currently checks req.authMethod inside the handler after requireRole('admin') and validateGuild run; move this gate into a pre-middleware so API-secret auth fails fast: add a new middleware function (e.g., requireApiSecret) that checks req.authMethod === 'api-secret' and returns res.status(403).json({ error: 'Actions endpoint requires API secret authentication' }) when not, then use requireApiSecret as the first middleware in the router.post('/:id/actions', ...) chain (before requireRole and validateGuild) so the API-secret check runs before those validations.
🤖 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/006_command_usage.cjs`:
- Line 16: The migration uses gen_random_uuid() in the id column (id UUID
PRIMARY KEY DEFAULT gen_random_uuid()) but never ensures the pgcrypto extension
is enabled, causing fresh DB runs to fail; update the migration script that
contains that SQL (the 006_command_usage migration) to prepend a statement that
enables the pgcrypto extension (CREATE EXTENSION IF NOT EXISTS pgcrypto) before
creating the table so gen_random_uuid() is available during migration.
In `@src/api/routes/guilds.js`:
- Around line 24-54: The permissionsToDashboardRole function duplicates
role-mapping logic (ADMINISTRATOR_FLAG, MANAGE_GUILD_FLAG, MANAGE_MESSAGES_FLAG,
KICK_MEMBERS_FLAG, BAN_MEMBERS_FLAG, VIEW_CHANNEL_FLAG, DASHBOARD_ROLE_ORDER)
already implemented in the web code; extract this mapping into a shared utility
module (e.g., dashboardRoles.getRoleFromPermissions) and replace the local
permissionsToDashboardRole implementation with a thin wrapper that imports and
calls the shared function, ensuring both backend and frontend import the same
source (or publish a small shared package) so the constants and mapping logic
are centralized and cannot drift.
In `@src/utils/commandUsage.js`:
- Around line 24-27: The call to getPool() in trackCommandUsage can throw before
your error handling runs; wrap the getPool() invocation in the same try/catch
(or call it inside the try block) so the thrown error is caught and handled;
update the trackCommandUsage function to retrieve pool within the try (or guard
the getPool() call with a try/catch) and use the existing logging/fallback logic
when getPool() throws.
---
Outside diff comments:
In `@src/api/routes/guilds.js`:
- Around line 472-505: The API now returns new access values like 'owner' and
'viewer' (see the access property set on ownerGuilds and in the userGuilds
mapping using permissionsToDashboardRole and botGuilds) but the OpenAPI enum
still lists [admin, moderator, bot-owner]; update the OpenAPI contract to
include the new values (and remove/alias deprecated ones as appropriate) so
generated clients match the runtime output, or alternatively map the runtime
role strings back to the documented enum inside the GET /guilds logic (e.g.,
adjust permissionsToDashboardRole or the object assigned to access) to restore
compatibility between fetchUserGuilds/permissionsToDashboardRole and the OpenAPI
enum.
---
Duplicate comments:
In `@src/api/routes/guilds.js`:
- Around line 269-278: The two deprecated wrapper functions isOAuthGuildAdmin
and isOAuthGuildModerator should be converted to async functions using await
instead of .then; change their declarations to async, call const r = await
getOAuthDashboardRole(user, guildId) and return the same boolean checks (r ===
'owner' || r === 'admin' for isOAuthGuildAdmin, and r === 'owner' || r ===
'admin' || r === 'moderator' for isOAuthGuildModerator), preserving the
deprecation comments and letting errors propagate naturally.
- Around line 1635-1638: The route currently checks req.authMethod inside the
handler after requireRole('admin') and validateGuild run; move this gate into a
pre-middleware so API-secret auth fails fast: add a new middleware function
(e.g., requireApiSecret) that checks req.authMethod === 'api-secret' and returns
res.status(403).json({ error: 'Actions endpoint requires API secret
authentication' }) when not, then use requireApiSecret as the first middleware
in the router.post('/:id/actions', ...) chain (before requireRole and
validateGuild) so the API-secret check runs before those validations.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
migrations/006_command_usage.cjssrc/api/routes/guilds.jssrc/index.jssrc/utils/commandUsage.jsweb/src/types/analytics-validators.tsweb/src/types/analytics.ts
📜 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: Agent
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (7)
src/**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{js,ts,jsx,tsx}: Use ESM only — Useimport/export, no CommonJS
Use single quotes for strings — No double quotes except in JSON
Always require semicolons at end of statements
Use 2-space indent, enforced by Biome
Always use async/await for asynchronous operations and promise handling
Files:
src/index.jssrc/utils/commandUsage.jssrc/api/routes/guilds.js
{src,web}/**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Winston logger from
src/logger.js, NEVER useconsole.*
Files:
src/index.jsweb/src/types/analytics.tssrc/utils/commandUsage.jsweb/src/types/analytics-validators.tssrc/api/routes/guilds.js
src/{commands,index}.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use safe Discord message methods — Use
safeReply(),safeSend(), orsafeEditReply()instead of direct Discord.js methods
Files:
src/index.js
src/**/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use parameterized SQL queries — Never use string interpolation in database queries
Files:
src/index.jssrc/utils/commandUsage.jssrc/api/routes/guilds.js
web/**/*.{ts,tsx,jsx,js}
📄 CodeRabbit inference engine (AGENTS.md)
Use Next.js 16 App Router for web dashboard, with Discord OAuth2 authentication, dark/light theme support, and mobile-responsive design
Files:
web/src/types/analytics.tsweb/src/types/analytics-validators.ts
migrations/**/*.cjs
📄 CodeRabbit inference engine (AGENTS.md)
migrations/**/*.cjs: Use node-pg-migrate for database migrations with.cjsfile extension to avoid ESM conflicts
Use sequential migration numbering (001, 002, ...) for database migrations
Files:
migrations/006_command_usage.cjs
src/api/routes/**/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
src/api/routes/**/*.{js,ts}: All API routes must use auth middleware if handling sensitive data and be mounted insrc/api/server.js
API endpoints must include tests intests/api/directory
Files:
src/api/routes/guilds.js
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-02T21:23:59.491Z
Learning: Create modules in `src/modules/` for new features, add config section to `config.json`, update `SAFE_CONFIG_KEYS`, create slash command if needed, add database migration if needed, write tests, and update dashboard UI if configurable
📚 Learning: 2026-03-02T21:23:59.491Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-02T21:23:59.491Z
Learning: Applies to src/commands/**/*.{js,ts} : Slash command files must export a slash command builder and execute function
Applied to files:
src/index.jssrc/utils/commandUsage.js
📚 Learning: 2026-03-02T21:23:59.491Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-02T21:23:59.491Z
Learning: Applies to migrations/**/*.cjs : Use sequential migration numbering (001, 002, ...) for database migrations
Applied to files:
migrations/006_command_usage.cjs
📚 Learning: 2026-03-02T21:23:59.491Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-02T21:23:59.491Z
Learning: Applies to migrations/**/*.cjs : Use node-pg-migrate for database migrations with `.cjs` file extension to avoid ESM conflicts
Applied to files:
migrations/006_command_usage.cjs
📚 Learning: 2026-03-02T21:23:59.491Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-02T21:23:59.491Z
Learning: Create modules in `src/modules/` for new features, add config section to `config.json`, update `SAFE_CONFIG_KEYS`, create slash command if needed, add database migration if needed, write tests, and update dashboard UI if configurable
Applied to files:
src/utils/commandUsage.js
📚 Learning: 2026-03-02T21:23:59.491Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-02T21:23:59.491Z
Learning: Applies to src/modules/**/*.{js,ts} : Use Discord cache utilities — `src/utils/discordCache.js` for channels/roles/members, `src/utils/reputationCache.js` for leaderboard/rank data
Applied to files:
src/utils/commandUsage.jssrc/api/routes/guilds.js
📚 Learning: 2026-03-02T21:23:59.491Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-02T21:23:59.491Z
Learning: Applies to web/**/*.{ts,tsx,jsx,js} : Use Next.js 16 App Router for web dashboard, with Discord OAuth2 authentication, dark/light theme support, and mobile-responsive design
Applied to files:
src/api/routes/guilds.js
📚 Learning: 2026-03-02T21:23:59.491Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-02T21:23:59.491Z
Learning: Applies to src/api/routes/**/*.{js,ts} : All API routes must use auth middleware if handling sensitive data and be mounted in `src/api/server.js`
Applied to files:
src/api/routes/guilds.js
🧬 Code graph analysis (3)
src/index.js (3)
src/utils/commandUsage.js (1)
trackCommandUsage(24-44)tests/commands/review.test.js (1)
interaction(200-224)src/modules/challengeScheduler.js (1)
interaction(307-307)
src/utils/commandUsage.js (2)
src/db.js (1)
getPool(314-319)src/logger.js (1)
warn(238-240)
src/api/routes/guilds.js (3)
web/src/lib/dashboard-roles.ts (1)
permissionsToDashboardRole(26-39)src/api/utils/discordApi.js (2)
guilds(62-62)fetchUserGuilds(36-72)src/utils/commandUsage.js (1)
getAggregatedCommandUsage(59-89)
🔇 Additional comments (4)
src/api/routes/guilds.js (1)
1074-1081: Good analytics fallback behavior on command usage source.Using
getAggregatedCommandUsage(...)plussource: ... ? 'command_usage' : 'unavailable'is a solid resilience improvement for partial analytics failures.Also applies to: 1427-1427
web/src/types/analytics.ts (1)
83-83: Type widening is aligned with the new analytics source.Adding
'command_usage'to the union keeps the type in sync with the new backend source contract.web/src/types/analytics-validators.ts (1)
122-126: Validator update correctly mirrors the expanded source enum.This keeps runtime payload validation consistent with
DashboardAnalytics.commandUsage.source.src/index.js (1)
246-251: Good placement for non-blocking usage tracking.Recording analytics after successful command execution keeps command latency low and avoids counting rejected command attempts.
migrations/006_command_usage.cjs
Outdated
| exports.up = (pgm) => { | ||
| pgm.sql(` | ||
| CREATE TABLE IF NOT EXISTS command_usage ( | ||
| id UUID PRIMARY KEY DEFAULT gen_random_uuid(), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Listing all migration files ==="
git ls-files migrations/*.cjs 2>/dev/null || find . -path "./migrations/*.cjs" -type f | sort
echo -e "\n=== Searching for pgcrypto extension enablement in all migrations ==="
rg -n "CREATE EXTENSION.*pgcrypto|pgcrypto" --type-add 'cjs:*.cjs' --type cjs migrations/Repository: VolvoxLLC/volvox-bot
Length of output: 572
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Content of migrations/006_command_usage.cjs ==="
cat -n migrations/006_command_usage.cjs
echo -e "\n=== Content of migrations/001_initial-schema.cjs (checking for pgcrypto or UUID setup) ==="
cat -n migrations/001_initial-schema.cjsRepository: VolvoxLLC/volvox-bot
Length of output: 22922
Add pgcrypto extension enablement to migration 006 to prevent fresh database failures.
Line 16 uses gen_random_uuid(), which requires the pgcrypto extension. No prior migration enables this extension, so migration 006 will fail on clean database deployments. Add the extension before the table creation:
Required fix
exports.up = (pgm) => {
+ pgm.sql('CREATE EXTENSION IF NOT EXISTS pgcrypto');
pgm.sql(`
CREATE TABLE IF NOT EXISTS command_usage (
id UUID PRIMARY KEY DEFAULT gen_random_uuid(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@migrations/006_command_usage.cjs` at line 16, The migration uses
gen_random_uuid() in the id column (id UUID PRIMARY KEY DEFAULT
gen_random_uuid()) but never ensures the pgcrypto extension is enabled, causing
fresh DB runs to fail; update the migration script that contains that SQL (the
006_command_usage migration) to prepend a statement that enables the pgcrypto
extension (CREATE EXTENSION IF NOT EXISTS pgcrypto) before creating the table so
gen_random_uuid() is available during migration.
src/api/routes/guilds.js
Outdated
| /** Discord permission flags for dashboard role mapping (see PermissionFlagsBits) */ | ||
| const ADMINISTRATOR_FLAG = 0x8; | ||
| /** Discord MANAGE_GUILD permission flag */ | ||
| const MANAGE_GUILD_FLAG = 0x20; | ||
| const VIEW_CHANNEL_FLAG = 0x400; | ||
| const MANAGE_MESSAGES_FLAG = 0x2000; | ||
| const KICK_MEMBERS_FLAG = 0x2; | ||
| const BAN_MEMBERS_FLAG = 0x4; | ||
|
|
||
| /** Dashboard role tiers: viewer < moderator < admin < owner */ | ||
| const DASHBOARD_ROLE_ORDER = { viewer: 0, moderator: 1, admin: 2, owner: 3 }; | ||
|
|
||
| /** | ||
| * Map Discord guild permission bitfield to dashboard role (no bot-owner check). | ||
| * Admin = ADMINISTRATOR or MANAGE_GUILD; Moderator = MANAGE_MESSAGES or KICK_MEMBERS or BAN_MEMBERS; Viewer = VIEW_CHANNEL. | ||
| * @param {number} permissions - Guild permission bitfield from Discord API | ||
| * @returns {'admin'|'moderator'|'viewer'|null} Highest role granted, or null if no dashboard access | ||
| */ | ||
| function permissionsToDashboardRole(permissions) { | ||
| if (Number.isNaN(Number(permissions))) return null; | ||
| const p = Number(permissions); | ||
| if ((p & ADMINISTRATOR_FLAG) !== 0 || (p & MANAGE_GUILD_FLAG) !== 0) return 'admin'; | ||
| if ( | ||
| (p & MANAGE_MESSAGES_FLAG) !== 0 || | ||
| (p & KICK_MEMBERS_FLAG) !== 0 || | ||
| (p & BAN_MEMBERS_FLAG) !== 0 | ||
| ) { | ||
| return 'moderator'; | ||
| } | ||
| if ((p & VIEW_CHANNEL_FLAG) !== 0) return 'viewer'; | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider centralizing permissions→dashboard-role mapping to avoid drift.
This role-mapping logic duplicates the web implementation (src/web/src/lib/dashboard-roles.ts lines 25-38). A shared utility would prevent backend/frontend divergence.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/routes/guilds.js` around lines 24 - 54, The
permissionsToDashboardRole function duplicates role-mapping logic
(ADMINISTRATOR_FLAG, MANAGE_GUILD_FLAG, MANAGE_MESSAGES_FLAG, KICK_MEMBERS_FLAG,
BAN_MEMBERS_FLAG, VIEW_CHANNEL_FLAG, DASHBOARD_ROLE_ORDER) already implemented
in the web code; extract this mapping into a shared utility module (e.g.,
dashboardRoles.getRoleFromPermissions) and replace the local
permissionsToDashboardRole implementation with a thin wrapper that imports and
calls the shared function, ensuring both backend and frontend import the same
source (or publish a small shared package) so the constants and mapping logic
are centralized and cannot drift.
src/utils/commandUsage.js
Outdated
| export async function trackCommandUsage(opts) { | ||
| const pool = opts.pool ?? getPool(); | ||
| if (!pool) return; | ||
|
|
There was a problem hiding this comment.
getPool() can throw before your try/catch, causing unhandled rejections.
Line 25 calls getPool() outside the try. Since src/db.js throws when DB is uninitialized, this can reject before logging/fallback logic runs.
🐛 Proposed fix
export async function trackCommandUsage(opts) {
- const pool = opts.pool ?? getPool();
- if (!pool) return;
-
const { guildId, userId, commandName, channelId } = opts;
if (!guildId || !userId || !commandName) return;
try {
+ const pool = opts.pool ?? getPool();
await pool.query(
`INSERT INTO command_usage (guild_id, user_id, command_name, channel_id)
VALUES ($1, $2, $3, $4)`,
[guildId, userId, commandName, channelId ?? null],
);
} catch (err) {
warn('Failed to record command usage', {
guildId,
commandName,
- error: err.message,
+ error: err instanceof Error ? err.message : String(err),
});
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/commandUsage.js` around lines 24 - 27, The call to getPool() in
trackCommandUsage can throw before your error handling runs; wrap the getPool()
invocation in the same try/catch (or call it inside the try block) so the thrown
error is caught and handled; update the trackCommandUsage function to retrieve
pool within the try (or guard the getPool() call with a try/catch) and use the
existing logging/fallback logic when getPool() throws.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
web/src/types/discord.ts:22
GuildAccessRoleduplicates the role union that also exists asDashboardRoleinweb/src/lib/dashboard-roles.ts. Keeping two separate role type definitions risks drift (especially since comments indicate frontend/backed sync requirements). Consider re-exporting/importing a single role type so changes only need to be made in one place.
/** Dashboard role for the current user in this guild (from Discord permissions or backend). */
export type GuildAccessRole = 'viewer' | 'moderator' | 'admin' | 'owner';
export interface MutualGuild extends DiscordGuild {
botPresent: boolean;
/** Current user's dashboard role in this guild. Set when guild list is built. */
access?: GuildAccessRole;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/api/routes/guilds.js
Outdated
| function permissionsToDashboardRole(permissions) { | ||
| if (Number.isNaN(Number(permissions))) return null; | ||
| const p = Number(permissions); | ||
| if ((p & ADMINISTRATOR_FLAG) !== 0 || (p & MANAGE_GUILD_FLAG) !== 0) return 'admin'; | ||
| if ( | ||
| (p & MANAGE_MESSAGES_FLAG) !== 0 || | ||
| (p & KICK_MEMBERS_FLAG) !== 0 || | ||
| (p & BAN_MEMBERS_FLAG) !== 0 | ||
| ) { | ||
| return 'moderator'; | ||
| } | ||
| if ((p & VIEW_CHANNEL_FLAG) !== 0) return 'viewer'; | ||
| return null; | ||
| } |
There was a problem hiding this comment.
permissionsToDashboardRole casts the Discord permission bitfield to Number and uses bitwise ops. Because Discord permission values can exceed JS safe integers and bitwise operators coerce to 32-bit ints, this can miscompute roles and lead to incorrect authorization decisions. Parse permissions as BigInt and use ...n constants for masking to ensure access control is correct.
| const guildContext = useContext(DashboardGuildContext); | ||
| const role: DashboardRole | null = guildContext?.selectedGuild?.access ?? null; | ||
|
|
||
| const visibleNav = role === null ? navigation : navigation.filter((item) => canAccessRoute(role, item.href)); | ||
|
|
There was a problem hiding this comment.
visibleNav falls back to showing the full navigation when role is null. Since role can be null when the guild payload omits access (e.g., bot API unavailable or response shape changes), this makes role-based navigation ineffective and can surface admin-only links to users who shouldn't see them. Consider defaulting to the most restrictive set (or at least viewer) when role is null, and/or requiring access to be present in the guild context.
| const fetchedGuilds = data.filter( | ||
| (g): g is MutualGuild => | ||
| typeof g === 'object' && | ||
| g !== null && | ||
| typeof (g as Record<string, unknown>).id === 'string' && | ||
| typeof (g as Record<string, unknown>).name === 'string', | ||
| ); |
There was a problem hiding this comment.
The runtime shape check for /api/guilds only validates id and name, but the rest of the dashboard now relies on access being present to enforce role-based navigation. If access is missing, the selected guild will have access: undefined and the sidebar falls back to showing all items. Consider validating access (and its allowed string values) here and filtering out entries without it, or assigning a safe default role.
src/utils/commandUsage.js
Outdated
| export async function trackCommandUsage(opts) { | ||
| const pool = opts.pool ?? getPool(); | ||
| if (!pool) return; | ||
|
|
||
| const { guildId, userId, commandName, channelId } = opts; | ||
| if (!guildId || !userId || !commandName) return; | ||
|
|
||
| try { | ||
| await pool.query( | ||
| `INSERT INTO command_usage (guild_id, user_id, command_name, channel_id) | ||
| VALUES ($1, $2, $3, $4)`, | ||
| [guildId, userId, commandName, channelId ?? null], | ||
| ); | ||
| } catch (err) { | ||
| warn('Failed to record command usage', { | ||
| guildId, | ||
| commandName, | ||
| error: err.message, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Fetch aggregated command usage for a guild in a date range for analytics. | ||
| * Returns rows with command_name and uses, ordered by uses DESC. | ||
| * | ||
| * @param {Object} opts | ||
| * @param {import('pg').Pool} opts.pool - Database pool | ||
| * @param {string} opts.guildId - Guild ID | ||
| * @param {string} opts.from - ISO date string (inclusive) | ||
| * @param {string} opts.to - ISO date string (inclusive) | ||
| * @param {string} [opts.channelId] - Optional channel filter | ||
| * @param {number} [opts.limit=15] - Max number of commands to return | ||
| * @returns {Promise<{ rows: Array<{ command_name: string, uses: number }>, available: boolean }>} | ||
| */ | ||
| export async function getAggregatedCommandUsage(opts) { | ||
| const { pool, guildId, from, to, channelId, limit = 15 } = opts; | ||
| const values = [guildId, from, to]; | ||
| let whereClause = 'guild_id = $1 AND used_at >= $2 AND used_at <= $3'; | ||
| if (channelId) { | ||
| values.push(channelId); | ||
| whereClause += ` AND channel_id = $${values.length}`; | ||
| } | ||
| values.push(limit); | ||
|
|
||
| try { | ||
| const result = await pool.query( | ||
| `SELECT | ||
| COALESCE(NULLIF(command_name, ''), 'unknown') AS command_name, | ||
| COUNT(*)::int AS uses | ||
| FROM command_usage | ||
| WHERE ${whereClause} | ||
| GROUP BY command_name | ||
| ORDER BY uses DESC, command_name ASC | ||
| LIMIT $${values.length}`, | ||
| values, | ||
| ); | ||
| return { rows: result.rows, available: true }; | ||
| } catch (err) { | ||
| warn('Command usage analytics query failed', { | ||
| guildId, | ||
| error: err.message, | ||
| }); | ||
| return { rows: [], available: false }; | ||
| } |
There was a problem hiding this comment.
New utility functions trackCommandUsage and getAggregatedCommandUsage don’t appear to have accompanying unit tests. The repo has extensive coverage under tests/utils/*; adding tests for the success and failure paths (e.g., insert/query errors returning available:false) would help prevent regressions as analytics evolves.
migrations/006_command_usage.cjs
Outdated
| pgm.sql(` | ||
| CREATE TABLE IF NOT EXISTS command_usage ( | ||
| id UUID PRIMARY KEY DEFAULT gen_random_uuid(), | ||
| guild_id TEXT NOT NULL, | ||
| user_id TEXT NOT NULL, | ||
| command_name TEXT NOT NULL, | ||
| channel_id TEXT, | ||
| used_at TIMESTAMPTZ DEFAULT NOW() | ||
| ) |
There was a problem hiding this comment.
The migration uses gen_random_uuid() as the default UUID generator, but the migration does not ensure the required Postgres extension (typically pgcrypto) is enabled. On fresh databases this will fail at runtime. Consider creating the extension in this migration (or switching to a UUID generation strategy already guaranteed by your schema).
web/src/lib/dashboard-roles.ts
Outdated
| export function permissionsToDashboardRole(permissions: string): DashboardRole | null { | ||
| const p = Number(permissions); | ||
| if (Number.isNaN(p)) return null; | ||
| if ((p & ADMINISTRATOR) !== 0 || (p & MANAGE_GUILD) !== 0) return 'admin'; | ||
| if ( | ||
| (p & MANAGE_MESSAGES) !== 0 || | ||
| (p & KICK_MEMBERS) !== 0 || | ||
| (p & BAN_MEMBERS) !== 0 | ||
| ) { | ||
| return 'moderator'; | ||
| } | ||
| if ((p & VIEW_CHANNEL) !== 0) return 'viewer'; | ||
| return null; |
There was a problem hiding this comment.
permissionsToDashboardRole parses the Discord permissions bitfield with Number(...) and uses bitwise ops. Discord permission values can exceed Number.MAX_SAFE_INTEGER, and JS bitwise operators coerce to 32-bit ints, which can yield incorrect role derivation for some permission sets. Use BigInt parsing (e.g., const p = BigInt(permissions)) and 0x...n constants for these checks so role-based authorization is reliable.
|
❌ Failed to create PR with unit tests: AGENT_CHAT: Failed to open pull request |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| scheduledBackupInterval = setInterval(() => { | ||
| try { | ||
| createBackup(backupDir); | ||
| pruneBackups(retention, backupDir); | ||
| } catch (err) { | ||
| logError('Scheduled backup failed', { error: err.message }); | ||
| } | ||
| void (async () => { | ||
| try { | ||
| await createBackup(backupDir); | ||
| await pruneBackups(retention, backupDir); | ||
| } catch (err) { | ||
| logError('Scheduled backup failed', { error: err.message }); | ||
| } | ||
| })(); | ||
| }, intervalMs); |
There was a problem hiding this comment.
startScheduledBackups now kicks off an async IIFE inside setInterval without any in-flight guard. If createBackup/pruneBackups take longer than intervalMs, multiple runs can overlap, leading to concurrent backup/prune operations (race conditions, extra load, unexpected deletions). Consider serializing runs (e.g., a running flag that skips or queues the next tick, or using a recursive setTimeout scheduled after the prior run completes).
|
Hi @puppy-engineer, I noticed this PR has undergone several changes:
Current state: This PR now only contains backup-related changes ( Issue: These backup changes are essentially a duplicate of PR #237 which was just merged into main. The functionality you're implementing here (converting sync fs to async) is already in the codebase. Questions:
If you want to pursue the role-based access feature, I'd suggest creating a fresh branch from current main and re-applying those changes, since main has moved forward significantly. Happy to help if you need any clarification! |
Describe your changes
Please describe your changes in detail.
Issue ticket number and link
I fixed this issue