chore: tech debt — skeletons, error boundaries, toasts, validation, rate limit cleanup#257
chore: tech debt — skeletons, error boundaries, toasts, validation, rate limit cleanup#257BillChirico merged 18 commits intomainfrom
Conversation
BillChirico
added
priority: medium
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds planning documentation, refactors Discord interaction handlers into modular components, introduces Zustand stores for members and moderation data management, improves UI with skeleton loading states and error boundaries, enhances mobile responsiveness through grid layout adjustments, and includes infrastructure updates to rate limiting exports and API validation. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/api/routes/guilds.js (1)
1188-1202:⚠️ Potential issue | 🟠 MajorAll-time metrics mixed with time-scoped analytics creates misleading data.
The
userEngagementmetrics (and similarlyxpEconomyat lines 1210-1227) return all-time cumulative values, while the rest of the/guilds/:id/analyticsendpoint respects therangequery parameter (today, week, month, custom). This creates a significant inconsistency: when a consumer requests "last 7 days" analytics, they receive time-bounded message volume, AI usage, and channel activity, buttotalMessagesSent,totalReactionsGiven, and XP stats reflect all-time values. The API response provides no indication that these metrics have different scoping, likely causing dashboards to misrepresent engagement trends.Consider one of these approaches:
- Omit these metrics until proper time-series data is available (breaking change but prevents misleading data)
- Add a clear indicator in the response, e.g.:
"userEngagement": { "scope": "all-time", "trackedUsers": 150, ... }- Implement the time-series solution now by adding a
message_eventsoruser_activity_logtable with timestamps, then aggregate within the requested rangeOption 2: Add scope indicator
userEngagement: userEngagementResult.rows[0] ? { + scope: 'all-time', trackedUsers: Number(userEngagementResult.rows[0].tracked_users || 0), totalMessagesSent: Number(userEngagementResult.rows[0].total_messages_sent || 0),And similarly for
xpEconomy:xpEconomy: xpEconomyResult.rows[0] ? { + scope: 'all-time', totalUsers: Number(xpEconomyResult.rows[0].total_users || 0),🤖 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 1188 - 1202, The userEngagement and xpEconomy objects returned by the /guilds/:id/analytics endpoint mix all-time aggregates (queries against user_stats) with time-scoped metrics, so add an explicit scope indicator to each impacted object instead of changing the data source: update the code that builds the userEngagement and xpEconomy response objects (look for the SQL that SELECTs from user_stats and the places constructing userEngagement and xpEconomy) to include a "scope": "all-time" field alongside trackedUsers/totalMessagesSent/totalReactionsGiven and XP fields, and ensure any API docs/tests reflecting these objects are updated to expect the new scope property so consumers can tell these metrics are not time-bounded.web/src/app/dashboard/tickets/page.tsx (1)
385-445:⚠️ Potential issue | 🟡 MinorSuppress the empty state when the request failed.
When
erroris set and there are no rows yet, this branch renders both the destructive alert and “No tickets found.” / “No tickets match your filters.” That makes a failed fetch look like a valid empty result.Proposed fix
- ) : ( + ) : error ? null : ( <div className="flex h-48 items-center justify-center rounded-lg border border-dashed"> <p className="text-sm text-muted-foreground"> {statusFilter || debouncedSearch ? 'No tickets match your filters.' : 'No tickets found.'} </p> </div> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/app/dashboard/tickets/page.tsx` around lines 385 - 445, The empty-state branch is rendered even when the fetch failed (error is set), causing the “No tickets…” message to show alongside the error alert; update the rendering conditions so the final empty-state block only renders when there is no error. Concretely, change the ternary that decides between TicketsSkeleton / table / empty message to include an error check (use the existing loading, tickets, and error symbols) so the empty message (the block that uses statusFilter/debouncedSearch) is only shown when !error and tickets.length === 0; keep TicketsSkeleton for loading and the table for tickets.length > 0 so the destructive alert (rendered elsewhere when error is truthy) is not duplicated by the empty state.
🤖 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/012_placeholder.cjs`:
- Around line 1-21: The placeholder migration doesn't resolve duplicate
migration IDs — instead rename the conflicting files
(004_performance_indexes.cjs and 004_voice_sessions.cjs) to unique, sequential
IDs so there are no duplicate "004_" prefixes: choose the next available numbers
in sequence, update the filenames accordingly (preserving the rest of each
filename and their exports.up/exports.down), remove this no-op placeholder if
you used 012 for one of them, and run your migration tooling to verify no
out-of-order/duplicate-ID errors remain.
In `@src/api/utils/validateConfigPatch.js`:
- Around line 83-87: Add deep per-key JSON Schema validation before accepting
config patches: create a JSON Schema registry keyed by top-level config sections
and integrate an AJV validator (or equivalent) into the patch acceptance flow in
validateConfigPatch.js so that, for each incoming patch path you first select
the matching top-level schema and run full schema.validate against the patch
object; keep validateSingleValue for existing type/range checks but reject
patches that fail the AJV schema validation and return clear validation errors.
In `@src/modules/handlers/pollHandler.js`:
- Around line 29-42: The logError call currently accesses err.message directly
which can throw if the caught value isn't an Error; update the error extraction
in the catch block around logError in poll vote handler to use a defensive
lookup such as err?.message || String(err) (or JSON.stringify for objects) and
pass that value into the error field, so replace the direct err.message usage in
the logError payload (referencing logError, interaction, and safeReply) with the
safe extraction.
In `@src/modules/handlers/reminderHandler.js`:
- Around line 37-49: The catch block in the reminder button handler uses
err.message directly which can throw if a non-Error (or null/undefined) was
thrown; update the catch in the Reminder button handler to defensively extract
an error message (e.g., check instanceof Error or coerce with String(err) /
fallback like 'unknown error') before calling logError, and ensure logError is
passed a stable string for the error field; keep the existing safeReply call
(safeReply(interaction, ...)) and the replied/deferred guard intact so the
user-facing fallback still runs when err is not an Error.
In `@src/modules/handlers/showcaseHandler.js`:
- Line 25: Change the feature-gate checks from explicit false comparisons to the
negation pattern used by other handlers: replace occurrences of
"guildConfig.showcase?.enabled === false" with "!guildConfig.showcase?.enabled"
(update both checks around the showcase handler, e.g., the checks referencing
guildConfig.showcase?.enabled at the start of the handler and later near the
secondary guard) so the showcase feature defaults to disabled when the config is
missing or undefined.
In `@TASK.md`:
- Around line 9-47: Add a blank line before and after each Markdown heading in
TASK.md to satisfy MD022 (e.g., ensure lines around "### 1. Loading skeletons
for dashboard pages", "### 2. Error boundaries for dashboard", "### 3. Toast
notifications for actions", "### 4. XP proxy route validation", and "### 5.
Stale rate limit cleanup" have an empty line above and below); update all other
headings in the file similarly for consistent spacing and re-run the markdown
linter to confirm the MD022 warning is resolved.
In `@tests/modules/rateLimit.test.js`:
- Line 361: The import of startRateLimitCleanup and stopRateLimitCleanup is
placed mid-file; move that import so it is grouped with the other imports from
rateLimit.js at the top of the test file, combine it into the existing rateLimit
import statement (so startRateLimitCleanup and stopRateLimitCleanup are imported
alongside other symbols from rateLimit.js), and delete the now-redundant import
line that currently appears after the first tests.
In `@web/src/app/api/guilds/`[guildId]/members/[userId]/xp/route.ts:
- Around line 40-72: The client-side validation in this route currently checks
payload, amount type, and reason but omits the backend's additional constraints
(reject zero and enforce magnitude <= 1_000_000), so update the validation
around payload.amount (the block that currently checks Number.isFinite and
Number.isInteger) to also return 400 when payload.amount === 0 and when
Math.abs(payload.amount) > 1_000_000, using NextResponse.json with clear error
messages consistent with the backend; ensure sanitized remains created only
after these checks and that body = JSON.stringify(sanitized) is unchanged.
In `@web/src/app/dashboard/members/page.tsx`:
- Around line 84-86: The hook currently doesn't clear persisted members when
revisiting the page with the same guild, so stale entries can remain visible if
the initial re-fetch fails; update the logic so that either onGuildChange is
invoked on mount or—preferably—modify fetchMembers to clear the stored members
when called with append: false (e.g., set members = [] / call resetAll at the
start of fetchMembers) so the UI doesn't show stale members while the fresh
request is in-flight; reference the fetchMembers function and the
resetAll/onGuildChange handlers to implement this change.
In `@web/src/app/dashboard/moderation/page.tsx`:
- Around line 87-101: The callbacks handleUserHistorySearch and
handleClearUserHistory mix store actions (setLookupUserId, setUserHistoryPage,
clearUserHistory) with hook state setters (setUserHistoryData,
setUserHistoryError), which splits user-history state between the store and the
useUserHistory hook; either consolidate or document that split. To fix: choose
one approach — (A) consolidate into the store by adding/using store actions like
setUserHistoryData and setUserHistoryError and replace the hook setters in
handleUserHistorySearch/handleClearUserHistory with those store actions, or (B)
keep fetched data in the hook and move UI state into the hook by exposing
setters for lookupUserId and userHistoryPage from useUserHistory and replace
store calls in the handlers; update imports/usages of
setLookupUserId/setUserHistoryPage/clearUserHistory or
setUserHistoryData/setUserHistoryError accordingly and add a short inline
comment above handleUserHistorySearch/handleClearUserHistory documenting the
chosen ownership model.
In `@web/src/stores/moderation-store.ts`:
- Line 7: The property actionFilter in moderation-store.ts is typed too loosely
as string; tighten it to a union type (e.g., type ActionFilter = 'all' | 'warn'
| 'ban' | 'mute' | ...) and use that type for the actionFilter property to
prevent invalid values and enable editor autocomplete; update the declaration
(actionFilter: ActionFilter), export the ActionFilter type if consumed
elsewhere, and adjust any places that set or read actionFilter (including
default values and setter functions) to use the new union members or a mapped
constant list.
---
Outside diff comments:
In `@src/api/routes/guilds.js`:
- Around line 1188-1202: The userEngagement and xpEconomy objects returned by
the /guilds/:id/analytics endpoint mix all-time aggregates (queries against
user_stats) with time-scoped metrics, so add an explicit scope indicator to each
impacted object instead of changing the data source: update the code that builds
the userEngagement and xpEconomy response objects (look for the SQL that SELECTs
from user_stats and the places constructing userEngagement and xpEconomy) to
include a "scope": "all-time" field alongside
trackedUsers/totalMessagesSent/totalReactionsGiven and XP fields, and ensure any
API docs/tests reflecting these objects are updated to expect the new scope
property so consumers can tell these metrics are not time-bounded.
In `@web/src/app/dashboard/tickets/page.tsx`:
- Around line 385-445: The empty-state branch is rendered even when the fetch
failed (error is set), causing the “No tickets…” message to show alongside the
error alert; update the rendering conditions so the final empty-state block only
renders when there is no error. Concretely, change the ternary that decides
between TicketsSkeleton / table / empty message to include an error check (use
the existing loading, tickets, and error symbols) so the empty message (the
block that uses statusFilter/debouncedSearch) is only shown when !error and
tickets.length === 0; keep TicketsSkeleton for loading and the table for
tickets.length > 0 so the destructive alert (rendered elsewhere when error is
truthy) is not duplicated by the empty state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5ff4d968-46fe-444f-8c88-1e27d9358ab9
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (37)
TASK.mdTASK2.mdmigrations/012_placeholder.cjssrc/api/routes/conversations.jssrc/api/routes/guilds.jssrc/api/routes/members.jssrc/api/utils/validateConfigPatch.jssrc/logger.jssrc/modules/events/interactionCreate.jssrc/modules/handlers/challengeHandler.jssrc/modules/handlers/pollHandler.jssrc/modules/handlers/reminderHandler.jssrc/modules/handlers/reviewHandler.jssrc/modules/handlers/showcaseHandler.jssrc/modules/handlers/ticketHandler.jssrc/modules/handlers/welcomeOnboardingHandler.jssrc/modules/rateLimit.jstests/modules/rateLimit.test.jsweb/package.jsonweb/src/app/api/guilds/[guildId]/members/[userId]/xp/route.tsweb/src/app/dashboard/audit-log/page.tsxweb/src/app/dashboard/conversations/page.tsxweb/src/app/dashboard/members/[userId]/page.tsxweb/src/app/dashboard/members/page.tsxweb/src/app/dashboard/moderation/page.tsxweb/src/app/dashboard/page.tsxweb/src/app/dashboard/temp-roles/page.tsxweb/src/app/dashboard/tickets/page.tsxweb/src/components/dashboard/ai-feedback-stats.tsxweb/src/components/dashboard/config-editor.tsxweb/src/components/dashboard/config-sections/CommunitySettingsSection.tsxweb/src/components/dashboard/config-sections/TriageSection.tsxweb/src/components/ui/channel-selector.tsxweb/src/components/ui/error-boundary.tsxweb/src/components/ui/role-selector.tsxweb/src/stores/members-store.tsweb/src/stores/moderation-store.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). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{js,cjs,mjs}
📄 CodeRabbit inference engine (AGENTS.md)
Use ESM only with
import/exportsyntax, never CommonJS except in migration files (.cjs)
Files:
src/api/utils/validateConfigPatch.jssrc/api/routes/conversations.jssrc/logger.jssrc/modules/handlers/reviewHandler.jssrc/modules/rateLimit.jssrc/modules/handlers/welcomeOnboardingHandler.jssrc/modules/handlers/challengeHandler.jssrc/modules/handlers/showcaseHandler.jssrc/modules/handlers/pollHandler.jssrc/api/routes/guilds.jssrc/modules/handlers/ticketHandler.jssrc/modules/handlers/reminderHandler.jsmigrations/012_placeholder.cjssrc/api/routes/members.jstests/modules/rateLimit.test.jssrc/modules/events/interactionCreate.js
**/*.{js,mjs,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,mjs,jsx,ts,tsx}: Use single quotes for strings in code, double quotes only allowed in JSON files
Always end statements with semicolons
Use 2-space indentation, enforced by Biome
Files:
src/api/utils/validateConfigPatch.jsweb/src/stores/moderation-store.tssrc/api/routes/conversations.jssrc/logger.jsweb/src/app/api/guilds/[guildId]/members/[userId]/xp/route.tssrc/modules/handlers/reviewHandler.jssrc/modules/rateLimit.jssrc/modules/handlers/welcomeOnboardingHandler.jssrc/modules/handlers/challengeHandler.jsweb/src/app/dashboard/members/[userId]/page.tsxsrc/modules/handlers/showcaseHandler.jsweb/src/components/ui/role-selector.tsxweb/src/app/dashboard/page.tsxsrc/modules/handlers/pollHandler.jsweb/src/components/dashboard/config-editor.tsxweb/src/components/dashboard/config-sections/CommunitySettingsSection.tsxsrc/api/routes/guilds.jssrc/modules/handlers/ticketHandler.jsweb/src/components/dashboard/ai-feedback-stats.tsxweb/src/app/dashboard/conversations/page.tsxsrc/modules/handlers/reminderHandler.jsweb/src/app/dashboard/tickets/page.tsxweb/src/components/dashboard/config-sections/TriageSection.tsxweb/src/app/dashboard/temp-roles/page.tsxsrc/api/routes/members.jsweb/src/app/dashboard/moderation/page.tsxweb/src/stores/members-store.tsweb/src/components/ui/channel-selector.tsxweb/src/components/ui/error-boundary.tsxtests/modules/rateLimit.test.jssrc/modules/events/interactionCreate.jsweb/src/app/dashboard/members/page.tsxweb/src/app/dashboard/audit-log/page.tsx
src/**/*.{js,mjs,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{js,mjs,jsx,ts,tsx}: Usesrc/logger.jsWinston logger singleton, never useconsole.*methods
Use safe Discord message methods:safeReply(),safeSend(),safeEditReply()instead of direct Discord.js methods
Use parameterized SQL queries, never string interpolation for database queries
Files:
src/api/utils/validateConfigPatch.jssrc/api/routes/conversations.jssrc/logger.jssrc/modules/handlers/reviewHandler.jssrc/modules/rateLimit.jssrc/modules/handlers/welcomeOnboardingHandler.jssrc/modules/handlers/challengeHandler.jssrc/modules/handlers/showcaseHandler.jssrc/modules/handlers/pollHandler.jssrc/api/routes/guilds.jssrc/modules/handlers/ticketHandler.jssrc/modules/handlers/reminderHandler.jssrc/api/routes/members.jssrc/modules/events/interactionCreate.js
{.env*,README.md,src/**/!(*.test).{js,ts}}
📄 CodeRabbit inference engine (CLAUDE.md)
Remove
GUILD_IDfrom shared environment variables in production/deployment configurations; preserve dev-only guild-scoped deploy support via CLI flag--guild-id <guild_id>
Files:
src/api/utils/validateConfigPatch.jssrc/api/routes/conversations.jssrc/logger.jssrc/modules/handlers/reviewHandler.jssrc/modules/rateLimit.jssrc/modules/handlers/welcomeOnboardingHandler.jssrc/modules/handlers/challengeHandler.jssrc/modules/handlers/showcaseHandler.jssrc/modules/handlers/pollHandler.jssrc/api/routes/guilds.jssrc/modules/handlers/ticketHandler.jssrc/modules/handlers/reminderHandler.jssrc/api/routes/members.jssrc/modules/events/interactionCreate.js
web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Next.js 16 web dashboard uses App Router with Discord OAuth2 authentication, dark/light theme support, and mobile-responsive design
Files:
web/src/stores/moderation-store.tsweb/src/app/api/guilds/[guildId]/members/[userId]/xp/route.tsweb/src/app/dashboard/members/[userId]/page.tsxweb/src/components/ui/role-selector.tsxweb/src/app/dashboard/page.tsxweb/src/components/dashboard/config-editor.tsxweb/src/components/dashboard/config-sections/CommunitySettingsSection.tsxweb/src/components/dashboard/ai-feedback-stats.tsxweb/src/app/dashboard/conversations/page.tsxweb/src/app/dashboard/tickets/page.tsxweb/src/components/dashboard/config-sections/TriageSection.tsxweb/src/app/dashboard/temp-roles/page.tsxweb/src/app/dashboard/moderation/page.tsxweb/src/stores/members-store.tsweb/src/components/ui/channel-selector.tsxweb/src/components/ui/error-boundary.tsxweb/src/app/dashboard/members/page.tsxweb/src/app/dashboard/audit-log/page.tsx
src/api/routes/**/*.{js,mjs}
📄 CodeRabbit inference engine (AGENTS.md)
Create API routes with proper authentication middleware, mount in
src/api/server.js, and document in OpenAPI spec
Files:
src/api/routes/conversations.jssrc/api/routes/guilds.jssrc/api/routes/members.js
src/modules/**/*.{js,mjs}
📄 CodeRabbit inference engine (AGENTS.md)
Create new modules for features with corresponding config sections in
config.jsonand entries inSAFE_CONFIG_KEYS
Files:
src/modules/handlers/reviewHandler.jssrc/modules/rateLimit.jssrc/modules/handlers/welcomeOnboardingHandler.jssrc/modules/handlers/challengeHandler.jssrc/modules/handlers/showcaseHandler.jssrc/modules/handlers/pollHandler.jssrc/modules/handlers/ticketHandler.jssrc/modules/handlers/reminderHandler.jssrc/modules/events/interactionCreate.js
migrations/**/*.cjs
📄 CodeRabbit inference engine (AGENTS.md)
Use
.cjsfile extension for database migrations, use sequential migration numbering (001, 002, etc.) with node-pg-migrate
Files:
migrations/012_placeholder.cjs
migrations/[0-9]*_*.cjs
📄 CodeRabbit inference engine (CLAUDE.md)
Database migrations must be sequentially numbered with non-conflicting IDs; rename conflicting migration files to resolve out-of-order execution errors (e.g.,
migrations/004_*.cjs→migrations/007_*.cjs, etc.)
Files:
migrations/012_placeholder.cjs
tests/**/*.{js,mjs,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Maintain 80% code coverage threshold minimum, never lower this threshold
Files:
tests/modules/rateLimit.test.js
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-05T18:07:15.752Z
Learning: Applies to web/**/*.{ts,tsx} : Next.js 16 web dashboard uses App Router with Discord OAuth2 authentication, dark/light theme support, and mobile-responsive design
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-07T15:34:56.495Z
Learning: Applies to web/src/components/dashboard/config-workspace/**/*.{js,jsx,ts,tsx} : Config editor save contract must maintain: global save/discard, diff-modal confirmation, per-section PATCH batching, and partial-failure behavior
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-07T15:34:56.495Z
Learning: Applies to web/src/components/dashboard/config-workspace/**/*.{js,jsx,ts,tsx} : Refactor config feature presentation to use reusable `SettingsFeatureCard` pattern with structure: header + master toggle + Basic/Advanced blocks
📚 Learning: 2026-03-05T18:07:15.752Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-05T18:07:15.752Z
Learning: Applies to src/**/*.{js,mjs,jsx,ts,tsx} : Use `src/logger.js` Winston logger singleton, never use `console.*` methods
Applied to files:
src/logger.js
📚 Learning: 2026-03-05T18:07:15.752Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-05T18:07:15.752Z
Learning: Applies to src/modules/**/*.{js,mjs} : Create new modules for features with corresponding config sections in `config.json` and entries in `SAFE_CONFIG_KEYS`
Applied to files:
src/modules/handlers/welcomeOnboardingHandler.js
📚 Learning: 2026-03-05T18:07:15.752Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-05T18:07:15.752Z
Learning: Applies to web/**/*.{ts,tsx} : Next.js 16 web dashboard uses App Router with Discord OAuth2 authentication, dark/light theme support, and mobile-responsive design
Applied to files:
web/src/app/dashboard/members/[userId]/page.tsxweb/src/components/dashboard/config-editor.tsxTASK.mdTASK2.mdweb/src/components/dashboard/config-sections/TriageSection.tsxweb/src/app/dashboard/moderation/page.tsxweb/src/app/dashboard/members/page.tsxweb/src/app/dashboard/audit-log/page.tsx
📚 Learning: 2026-03-07T15:34:56.495Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-07T15:34:56.495Z
Learning: Applies to web/src/components/dashboard/config-workspace/**/*.{js,jsx,ts,tsx} : Refactor config feature presentation to use reusable `SettingsFeatureCard` pattern with structure: header + master toggle + Basic/Advanced blocks
Applied to files:
web/src/components/dashboard/config-editor.tsxweb/src/components/dashboard/config-sections/CommunitySettingsSection.tsxweb/src/components/dashboard/config-sections/TriageSection.tsxweb/src/app/dashboard/moderation/page.tsxweb/src/app/dashboard/members/page.tsx
📚 Learning: 2026-03-07T15:34:56.495Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-07T15:34:56.495Z
Learning: Applies to web/src/components/dashboard/config-workspace/**/*.{js,jsx,ts,tsx} : Config editor must implement metadata-driven config search with cross-category quick jump, focus/scroll targeting, and auto-open advanced sections when search hits advanced controls
Applied to files:
web/src/components/dashboard/config-editor.tsxweb/src/components/dashboard/config-sections/CommunitySettingsSection.tsxweb/src/components/dashboard/config-sections/TriageSection.tsx
📚 Learning: 2026-03-07T15:34:56.495Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-07T15:34:56.495Z
Learning: Applies to web/src/components/dashboard/config-workspace/**/*.{js,jsx,ts,tsx} : Config editor save contract must maintain: global save/discard, diff-modal confirmation, per-section PATCH batching, and partial-failure behavior
Applied to files:
web/src/components/dashboard/config-editor.tsxTASK.mdweb/src/components/dashboard/config-sections/CommunitySettingsSection.tsxTASK2.mdweb/src/components/dashboard/config-sections/TriageSection.tsxweb/src/app/dashboard/moderation/page.tsx
📚 Learning: 2026-03-07T15:34:56.495Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-07T15:34:56.495Z
Learning: Applies to web/src/components/dashboard/config-workspace/**/*.{js,jsx,ts,tsx} : Web dashboard config editor must use category workspace navigation with categories: `AI & Automation`, `Onboarding & Growth`, `Moderation & Safety`, `Community Tools`, `Support & Integrations` located in `web/src/components/dashboard/config-workspace/`
Applied to files:
web/src/components/dashboard/config-editor.tsxTASK.mdweb/src/components/dashboard/config-sections/CommunitySettingsSection.tsxweb/src/components/dashboard/config-sections/TriageSection.tsx
📚 Learning: 2026-03-07T15:34:56.495Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-07T15:34:56.495Z
Learning: Applies to web/src/components/dashboard/config-workspace/**/*.test.{js,jsx,ts,tsx} : Config editor tests must cover manual-save workspace behavior (not autosave assumptions), category switching, search functionality, and dirty badges
Applied to files:
web/src/components/dashboard/config-editor.tsxTASK.mdTASK2.md
📚 Learning: 2026-03-07T15:34:56.495Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-07T15:34:56.495Z
Learning: Applies to migrations/[0-9]*_*.cjs : Database migrations must be sequentially numbered with non-conflicting IDs; rename conflicting migration files to resolve out-of-order execution errors (e.g., `migrations/004_*.cjs` → `migrations/007_*.cjs`, etc.)
Applied to files:
migrations/012_placeholder.cjs
📚 Learning: 2026-03-05T18:07:15.752Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-05T18:07:15.752Z
Learning: Applies to migrations/**/*.cjs : Use `.cjs` file extension for database migrations, use sequential migration numbering (001, 002, etc.) with node-pg-migrate
Applied to files:
migrations/012_placeholder.cjs
📚 Learning: 2026-03-05T18:07:15.752Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-05T18:07:15.752Z
Learning: Applies to src/utils/discordCache.js : Cache Discord API responses for channels, roles, and members with auto-invalidation on config changes
Applied to files:
src/api/routes/members.js
🪛 LanguageTool
TASK2.md
[uncategorized] ~53-~53: The official name of this software platform is spelled with a capital “H”.
Context: ...oderabbitai style) + CodeRabbit - Check .github/ for reviewer config files - Check `CO...
(GITHUB)
🪛 markdownlint-cli2 (0.21.0)
TASK.md
[warning] 12-12: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 19-19: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 24-24: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 29-29: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 35-35: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 41-41: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 45-45: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🔇 Additional comments (39)
src/api/routes/conversations.js (1)
224-227: Helpful performance note here.Documenting the current trigram-index path and the planned move to full-text search/keyset pagination makes the tradeoff explicit without changing behavior. Nice addition.
web/src/app/dashboard/temp-roles/page.tsx (1)
14-14: LGTM! Skeleton loading implementation is clean.The skeleton placeholder provides good UX feedback during initial data fetch. The implementation correctly renders only when
loading && rows.length === 0, avoiding flicker on subsequent fetches.Also applies to: 203-213
web/src/components/ui/channel-selector.tsx (1)
28-28: LGTM! Skeleton loading in dropdown is well-structured.The skeleton layout mirrors the actual channel item structure (icon + text), providing a faithful placeholder during loading. Good use of
CommandEmptyfor conditional rendering.Also applies to: 322-329
src/api/routes/members.js (1)
317-323: LGTM! Helpful documentation of API limitations.The TODO clearly documents the member search cache limitation and proposes concrete approaches (DB table with bulk sync, Discord HTTP API). This aids future maintainers in understanding the tradeoff.
web/src/components/dashboard/config-sections/TriageSection.tsx (1)
88-88: LGTM! Responsive grid layout improvement.The change from fixed
grid-cols-2to responsivegrid-cols-1 sm:grid-cols-2ensures form fields stack properly on mobile viewports, aligning with the project's mobile-responsive design goals.Also applies to: 106-106, 122-122
src/logger.js (1)
9-14: LGTM! Useful documentation for browser compatibility.The TODO clearly explains why this module cannot be used in browser contexts and provides a concrete path forward (thin shim in
web/src/lib/logger.ts). This prevents future confusion when adding client-side logging.web/src/components/dashboard/config-sections/CommunitySettingsSection.tsx (1)
290-290: LGTM! Consistent responsive layout update.Matches the same responsive pattern applied in
TriageSection.tsx, ensuring visual consistency across config sections on mobile devices.web/src/components/dashboard/ai-feedback-stats.tsx (1)
93-93: LGTM! Responsive summary grid.The three stat cards (Positive/Negative/Approval Rate) will now stack on narrow viewports and display as a row on
sm:and larger, improving mobile readability.web/src/app/dashboard/conversations/page.tsx (2)
27-68: LGTM! Well-structured skeleton component.The
ConversationsSkeletonfollows the established pattern from other dashboard pages, with appropriate skeleton widths matching expected content sizes and responsive classes (hidden md:table-cell) matching the actual table structure.
371-444: Consistent loading and empty-state handling.The conditional rendering logic properly:
- Shows skeleton only on initial load (when
loading && conversations.length === 0)- Preserves stale data during pagination/refresh
- Provides context-aware empty state messages
src/modules/handlers/challengeHandler.js (1)
1-64: Clean handler implementation following established patterns.The module correctly:
- Gates on
challenges?.enabledconfig- Validates the challenge index with
Number.isNaNcheck- Uses the Winston logger (
warn,error) instead of console methods- Uses
safeReplyfor error responses- Checks reply/deferred state before attempting error reply
This aligns well with the other handler modules in this PR.
web/src/components/dashboard/config-editor.tsx (2)
1352-1383: Good mobile-first responsive refactor.Changing from fixed
grid-cols-2togrid-cols-1 gap-4 sm:grid-cols-2ensures proper stacking on mobile devices while maintaining the multi-column layout on larger screens.
1384-1431: Consistent responsive pattern applied.The 3-column grid for mute settings now properly collapses to single-column on mobile with
grid-cols-1 gap-4 sm:grid-cols-3.src/modules/handlers/welcomeOnboardingHandler.js (1)
24-75: Handler correctly passes guildConfig to delegate functions.Per the context snippets from
src/modules/welcomeOnboarding.js, bothhandleRulesAcceptButtonandhandleRoleMenuSelectionexpect(interaction, config)where config is the full guild config object. The handler correctly:
- Passes
guildConfig(notguildConfig.welcome)- Uses
safeEditReplysince the delegate functions calldeferReplyinternally- Swallows secondary errors to prevent unhandled rejections
web/src/app/dashboard/audit-log/page.tsx (2)
66-107: Well-structured skeleton matching table layout.The skeleton widths and responsive visibility classes (
hidden md:table-cell,hidden lg:table-cell) correctly mirror the actual table structure, ensuring a smooth loading experience.
388-465: Consistent loading state handling.The rendering logic follows the same pattern as other dashboard pages, showing skeleton on initial load while preserving data during pagination.
tests/modules/rateLimit.test.js (1)
643-706: Good test coverage for the stale entry cleanup interval.The test properly:
- Manages fake/real timer transitions in beforeEach/afterEach
- Restarts the cleanup interval under fake timers to capture it
- Uses short retention windows (1s) for fast test execution
- Advances time past both the cleanup interval (5 min) and retention window
- Verifies the entry count drops to 0
This validates the interval-based sweep behavior documented in the PR objectives.
src/modules/handlers/reviewHandler.js (1)
18-48: Handler correctly passes only interaction to delegate.Per the context snippet from
src/modules/reviewHandler.js:148-158,handleReviewClaimonly takes theinteractionparameter (it acquires the pool internally). The handler correctly:
- Gates on
review?.enabled- Passes only
interactiontohandleReviewClaim- Follows the established error handling pattern
src/modules/handlers/showcaseHandler.js (2)
27-40: Lazy pool acquisition handles DB unavailability gracefully.The dynamic import pattern for
getPool()with try-catch properly handles cases where the database module isn't initialized, providing user feedback before early return.Also applies to: 79-92
51-56: Smart reply method selection based on interaction state.The logic to choose between
safeEditReplyandsafeReplybased oninteraction.deferred || interaction.repliedensures proper error responses regardless of the delegate function's reply state.Also applies to: 98-103
web/src/components/ui/error-boundary.tsx (1)
1-92: LGTM! Well-structured reusable error boundary.The implementation follows React error boundary best practices with proper lifecycle methods, a clean default UI, and dev-mode debugging. The
biome-ignorecomment correctly justifiesconsole.errorusage in the browser context.web/src/app/dashboard/page.tsx (1)
5-12: LGTM! Clean error boundary integration.The custom title and description provide helpful context for analytics failures, and the suggestion to "select a different server" gives users an actionable recovery path.
src/modules/handlers/ticketHandler.js (3)
1-68: LGTM! Well-structured ticket open handler with proper error handling.The handler correctly validates ticket system status, uses safe Discord methods, and logs errors appropriately. The empty catch block at line 37-39 is acceptable for ignoring reply failures when the system is disabled.
76-122: LGTM! Modal handler follows robust patterns.Good use of
deferReplybefore async operations, proper error logging with context, and nested try/catch for fallback error replies.
130-180: LGTM! Close handler with proper channel type validation.The validation at lines 147-155 correctly handles both thread and channel modes as documented in the relevant code snippets from
closeTicket. Error handling is comprehensive.src/modules/events/interactionCreate.js (1)
1-21: LGTM! Clean refactor to modular handler exports.The barrel re-export pattern maintains API compatibility while enabling better separation of concerns. Each handler type now lives in its own focused module.
web/src/stores/moderation-store.ts (1)
1-73: LGTM! Clean Zustand store implementation.The store follows Zustand 5 conventions correctly with clear state/action separation. The
resetOnGuildChangeaction comprehensively resets all state fields.web/src/stores/members-store.ts (1)
1-75: LGTM! Well-organized members store.The store is cleanly structured with extracted
initialStatefor reset functionality. TheappendMembersaction correctly uses a functional update for immutable state modification.web/package.json (1)
37-38: Zustand^5.0.11is the latest available version.The dependency is correctly specified and compatible with the store implementation using the
create<State>((set) => ...)pattern.web/src/app/dashboard/moderation/page.tsx (4)
5-15: LGTM — clean import additions for store and ErrorBoundary.The imports are properly organized, bringing in
useCallback(already needed),ErrorBoundaryfor render error handling, anduseModerationStorefor centralized state management.
20-42: Good migration to store-driven state.The destructured state and actions from
useModerationStorecleanly replace what would have been multipleuseStatecalls. TheonGuildChangecallback correctly depends onresetOnGuildChange, which is stable from Zustand.
142-144: ErrorBoundary correctly wraps render errors.Good separation of concerns: the
ErrorBoundarycatches JavaScript exceptions during rendering, whilestatsError(passed as a prop) handles API-level errors. Thetitleprop provides a user-friendly message.
159-162: Store callbacks correctly match CaseTableProps interface.Per
CaseTableProps(context snippet 3),onSortToggleandonClearFiltersexpect() => voidsignatures. The store'stoggleSortDescandclearFiltersmatch exactly, making this a clean pass-through.web/src/app/dashboard/members/page.tsx (6)
36-60: Good store integration with comprehensive state destructuring.The large destructuring block clearly documents all state and actions used by this component. This makes the component's data dependencies explicit and aligns with the Zustand pattern of selecting specific slices.
69-75: Debounce effect correctly includessetDebouncedSearchin deps.The dependency array properly includes both
search(triggers the effect) andsetDebouncedSearch(used in the effect). Since Zustand actions are stable references, this won't cause unnecessary re-runs.
136-165: Fetch logic correctly uses store actions with proper dependency array.The
appendMembersandsetMembersactions are correctly used based on theappendflag. All store setters used insidefetchMembersare included in the dependency array, and since Zustand actions are stable, this won't cause unnecessary re-creations.
181-192: Sort handler correctly resets pagination.Calling
resetPagination()when sort changes ensures fresh data loads from the beginning rather than trying to paginate with stale cursor state. The dependency array is complete.
206-217: Refresh handler correctly resets pagination before fetching.Calling
resetPagination()clears the members array andnextAftercursor, then immediately fetches fresh data. This ensures no stale pagination state interferes with the refresh.
227-230: Clear search correctly bypasses debounce.Setting both
searchanddebouncedSearchto empty immediately triggers the fetch effect without waiting for the 300ms debounce. The debounce effect (lines 69-75) will also run, but sincedebouncedSearchis already'', Zustand's shallow equality check prevents a redundant state update and double fetch.
| /** | ||
| * Migration 012 — Placeholder (gap filler) | ||
| * | ||
| * There are two migrations numbered 004 in this project: | ||
| * - 004_performance_indexes.cjs | ||
| * - 004_voice_sessions.cjs | ||
| * | ||
| * This no-op migration occupies the 012 slot to make the numbering sequence | ||
| * explicit going forward. No schema changes are made here. | ||
| */ | ||
|
|
||
| /** @type {import('node-pg-migrate').MigrationBuilder} */ | ||
| exports.up = async (pgm) => { | ||
| // No-op — this migration exists only to document the sequence gap | ||
| pgm.noTransaction(); | ||
| }; | ||
|
|
||
| /** @type {import('node-pg-migrate').MigrationBuilder} */ | ||
| exports.down = async (_pgm) => { | ||
| // Nothing to undo | ||
| }; |
There was a problem hiding this comment.
This placeholder does not fix the duplicate migration IDs.
The header documents that the repo already has two 004_* migrations. Filling the 012 slot leaves that conflict in place, so node-pg-migrate can still hit out-of-order or duplicate-ID problems. The fix needs to renumber the conflicting migration file(s), not add a no-op gap marker.
As per coding guidelines, "Database migrations must be sequentially numbered with non-conflicting IDs; rename conflicting migration files to resolve out-of-order execution errors."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@migrations/012_placeholder.cjs` around lines 1 - 21, The placeholder
migration doesn't resolve duplicate migration IDs — instead rename the
conflicting files (004_performance_indexes.cjs and 004_voice_sessions.cjs) to
unique, sequential IDs so there are no duplicate "004_" prefixes: choose the
next available numbers in sequence, update the filenames accordingly (preserving
the rest of each filename and their exports.up/exports.down), remove this no-op
placeholder if you used 012 for one of them, and run your migration tooling to
verify no out-of-order/duplicate-ID errors remain.
| // TODO: Deep per-key schema validation — currently validateSingleValue only checks | ||
| // type/range for known paths. Unknown paths pass through without structural validation. | ||
| // For full coverage, add a per-key JSON schema registry (one schema per top-level config | ||
| // section) and run deep validation against it here before accepting the patch. | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
TODO acknowledged — schema validation gap documented.
The comment clearly describes the current limitation (unknown paths bypass structural validation) and proposes a reasonable solution (per-key JSON schema registry). This is good documentation of technical debt.
Would you like me to help by:
- Opening an issue to track this schema validation enhancement, or
- Drafting an implementation approach using a library like
ajvfor JSON schema validation?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/utils/validateConfigPatch.js` around lines 83 - 87, Add deep per-key
JSON Schema validation before accepting config patches: create a JSON Schema
registry keyed by top-level config sections and integrate an AJV validator (or
equivalent) into the patch acceptance flow in validateConfigPatch.js so that,
for each incoming patch path you first select the matching top-level schema and
run full schema.validate against the patch object; keep validateSingleValue for
existing type/range checks but reject patches that fail the AJV schema
validation and return clear validation errors.
| } catch (err) { | ||
| logError('Poll vote handler failed', { | ||
| customId: interaction.customId, | ||
| userId: interaction.user?.id, | ||
| error: err.message, | ||
| }); | ||
|
|
||
| // Try to send an ephemeral error if we haven't replied yet | ||
| if (!interaction.replied && !interaction.deferred) { | ||
| try { | ||
| await safeReply(interaction, { | ||
| content: '❌ Something went wrong processing your vote.', | ||
| ephemeral: true, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 'catch \(err\)|error:\s*err\.message' src/modules/handlers/pollHandler.jsRepository: VolvoxLLC/volvox-bot
Length of output: 323
🏁 Script executed:
#!/bin/bash
# Find all handler files to compare error handling patterns
fd -e js src/modules/handlers | head -20Repository: VolvoxLLC/volvox-bot
Length of output: 449
🏁 Script executed:
#!/bin/bash
# Search for catch blocks and error logging patterns in all handlers
rg -n 'catch\s*\(\w+\)' src/modules/handlers/ -A 5 | head -100Repository: VolvoxLLC/volvox-bot
Length of output: 6497
Use optional chaining or defensive check for error message access.
The direct access to err.message here can fail if the caught value is not an Error object. While some handlers in the codebase use err?.message for safety, JavaScript catch bindings are not guaranteed to be Error instances—they can be primitives, null, or other objects.
Suggested fix
} catch (err) {
+ const errorMessage = err instanceof Error ? err.message : String(err);
logError('Poll vote handler failed', {
customId: interaction.customId,
userId: interaction.user?.id,
- error: err.message,
+ error: errorMessage,
});📝 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.
| } catch (err) { | |
| logError('Poll vote handler failed', { | |
| customId: interaction.customId, | |
| userId: interaction.user?.id, | |
| error: err.message, | |
| }); | |
| // Try to send an ephemeral error if we haven't replied yet | |
| if (!interaction.replied && !interaction.deferred) { | |
| try { | |
| await safeReply(interaction, { | |
| content: '❌ Something went wrong processing your vote.', | |
| ephemeral: true, | |
| }); | |
| } catch (err) { | |
| const errorMessage = err instanceof Error ? err.message : String(err); | |
| logError('Poll vote handler failed', { | |
| customId: interaction.customId, | |
| userId: interaction.user?.id, | |
| error: errorMessage, | |
| }); | |
| // Try to send an ephemeral error if we haven't replied yet | |
| if (!interaction.replied && !interaction.deferred) { | |
| try { | |
| await safeReply(interaction, { | |
| content: '❌ Something went wrong processing your vote.', | |
| ephemeral: true, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/handlers/pollHandler.js` around lines 29 - 42, The logError call
currently accesses err.message directly which can throw if the caught value
isn't an Error; update the error extraction in the catch block around logError
in poll vote handler to use a defensive lookup such as err?.message ||
String(err) (or JSON.stringify for objects) and pass that value into the error
field, so replace the direct err.message usage in the logError payload
(referencing logError, interaction, and safeReply) with the safe extraction.
| } catch (err) { | ||
| logError('Reminder button handler failed', { | ||
| customId: interaction.customId, | ||
| userId: interaction.user?.id, | ||
| error: err.message, | ||
| }); | ||
|
|
||
| if (!interaction.replied && !interaction.deferred) { | ||
| try { | ||
| await safeReply(interaction, { | ||
| content: '❌ Something went wrong processing your request.', | ||
| ephemeral: true, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 'catch \(err\)|error:\s*err\.message' src/modules/handlers/reminderHandler.jsRepository: VolvoxLLC/volvox-bot
Length of output: 336
🏁 Script executed:
# Check if TypeScript is used and search for catch block patterns
rg -n 'catch\s*\(\s*\w+\s*\)' src/modules/ -A2 | head -40Repository: VolvoxLLC/volvox-bot
Length of output: 2149
🏁 Script executed:
# Check the handler structure and see if there's a shared pattern
git ls-files | grep -E '\.(ts|tsx|js)$' | head -5
cat tsconfig.json 2>/dev/null || echo "No tsconfig found"Repository: VolvoxLLC/volvox-bot
Length of output: 212
🏁 Script executed:
# Look at logger implementation to understand what it expects
rg -n 'export.*logError|function logError' src/ -A5Repository: VolvoxLLC/volvox-bot
Length of output: 46
🏁 Script executed:
# Find and read the logger file
fd logger.js src/ -x cat -n {}Repository: VolvoxLLC/volvox-bot
Length of output: 11487
🏁 Script executed:
# Read the full reminderHandler.js to understand context
head -80 src/modules/handlers/reminderHandler.js | cat -nRepository: VolvoxLLC/volvox-bot
Length of output: 2254
🏁 Script executed:
# Check if this handler is called in an event listener that might catch non-Error values
rg -n 'handleReminderSnooze|handleReminderDismiss|InteractionCreate' src/modules/handlers/reminderHandler.js -B5 -A2Repository: VolvoxLLC/volvox-bot
Length of output: 949
Guard error logging against non-Error throws.
Line 41 accesses err.message without type checking. If the catch block receives null, undefined, or a non-Error value, this will fail silently and skip the user-facing fallback reply.
Fix
} catch (err) {
+ const errorMessage = err instanceof Error ? err.message : String(err);
logError('Reminder button handler failed', {
customId: interaction.customId,
userId: interaction.user?.id,
- error: err.message,
+ error: errorMessage,
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/handlers/reminderHandler.js` around lines 37 - 49, The catch
block in the reminder button handler uses err.message directly which can throw
if a non-Error (or null/undefined) was thrown; update the catch in the Reminder
button handler to defensively extract an error message (e.g., check instanceof
Error or coerce with String(err) / fallback like 'unknown error') before calling
logError, and ensure logError is passed a stable string for the error field;
keep the existing safeReply call (safeReply(interaction, ...)) and the
replied/deferred guard intact so the user-facing fallback still runs when err is
not an Error.
|
|
||
| // Gate on showcase feature being enabled for this guild | ||
| const guildConfig = getConfig(interaction.guildId); | ||
| if (guildConfig.showcase?.enabled === false) return; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how other handlers gate on feature enablement to confirm consistency
rg -n '\.enabled\)? return' src/modules/handlers/ --type jsRepository: VolvoxLLC/volvox-bot
Length of output: 518
Fix inconsistent feature gate check—use negation pattern matching other handlers.
The showcase handler uses guildConfig.showcase?.enabled === false on lines 25 and 77, which enables the feature by default when config is missing or undefined. Every other handler in src/modules/handlers/ uses !guildConfig.feature?.enabled to default-disable the feature when config is missing. Change to if (!guildConfig.showcase?.enabled) return; for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/handlers/showcaseHandler.js` at line 25, Change the feature-gate
checks from explicit false comparisons to the negation pattern used by other
handlers: replace occurrences of "guildConfig.showcase?.enabled === false" with
"!guildConfig.showcase?.enabled" (update both checks around the showcase
handler, e.g., the checks referencing guildConfig.showcase?.enabled at the start
of the handler and later near the secondary guard) so the showcase feature
defaults to disabled when the config is missing or undefined.
| const json: unknown = await request.json(); | ||
|
|
||
| // Validate payload shape before forwarding (defense-in-depth) | ||
| if (typeof json !== 'object' || json === null || Array.isArray(json)) { | ||
| return NextResponse.json({ error: 'Body must be a JSON object' }, { status: 400 }); | ||
| } | ||
| const payload = json as Record<string, unknown>; | ||
|
|
||
| if (!('amount' in payload)) { | ||
| return NextResponse.json({ error: 'Missing required field: amount' }, { status: 400 }); | ||
| } | ||
| if ( | ||
| typeof payload.amount !== 'number' || | ||
| !Number.isFinite(payload.amount) || | ||
| !Number.isInteger(payload.amount) | ||
| ) { | ||
| return NextResponse.json( | ||
| { error: 'Field "amount" must be a finite integer' }, | ||
| { status: 400 }, | ||
| ); | ||
| } | ||
| if ('reason' in payload && payload.reason !== undefined && typeof payload.reason !== 'string') { | ||
| return NextResponse.json( | ||
| { error: 'Field "reason" must be a string when provided' }, | ||
| { status: 400 }, | ||
| ); | ||
| } | ||
|
|
||
| // Only forward the known fields — strip unknown keys | ||
| const sanitized: { amount: number; reason?: string } = { amount: payload.amount as number }; | ||
| if (typeof payload.reason === 'string') sanitized.reason = payload.reason; | ||
|
|
||
| body = JSON.stringify(sanitized); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider aligning validation with the backend for fail-fast behavior.
The validation correctly ensures amount is a finite integer and strips unknown fields. However, the backend (src/api/routes/members.js:906-917) has two additional checks that this proxy doesn't replicate:
amount !== 0— zero is rejectedMath.abs(amount) <= 1_000_000— magnitude limit
Invalid requests will still fail at the backend, but users receive a less specific error after a network round-trip. For better UX, consider adding these checks here:
♻️ Suggested validation alignment
if (
typeof payload.amount !== 'number' ||
!Number.isFinite(payload.amount) ||
!Number.isInteger(payload.amount)
) {
return NextResponse.json(
{ error: 'Field "amount" must be a finite integer' },
{ status: 400 },
);
}
+ if (payload.amount === 0) {
+ return NextResponse.json(
+ { error: 'Field "amount" must be non-zero' },
+ { status: 400 },
+ );
+ }
+ if (Math.abs(payload.amount) > 1_000_000) {
+ return NextResponse.json(
+ { error: 'Field "amount" must be between -1000000 and 1000000' },
+ { status: 400 },
+ );
+ }
if ('reason' in payload && payload.reason !== undefined && typeof payload.reason !== 'string') {📝 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 json: unknown = await request.json(); | |
| // Validate payload shape before forwarding (defense-in-depth) | |
| if (typeof json !== 'object' || json === null || Array.isArray(json)) { | |
| return NextResponse.json({ error: 'Body must be a JSON object' }, { status: 400 }); | |
| } | |
| const payload = json as Record<string, unknown>; | |
| if (!('amount' in payload)) { | |
| return NextResponse.json({ error: 'Missing required field: amount' }, { status: 400 }); | |
| } | |
| if ( | |
| typeof payload.amount !== 'number' || | |
| !Number.isFinite(payload.amount) || | |
| !Number.isInteger(payload.amount) | |
| ) { | |
| return NextResponse.json( | |
| { error: 'Field "amount" must be a finite integer' }, | |
| { status: 400 }, | |
| ); | |
| } | |
| if ('reason' in payload && payload.reason !== undefined && typeof payload.reason !== 'string') { | |
| return NextResponse.json( | |
| { error: 'Field "reason" must be a string when provided' }, | |
| { status: 400 }, | |
| ); | |
| } | |
| // Only forward the known fields — strip unknown keys | |
| const sanitized: { amount: number; reason?: string } = { amount: payload.amount as number }; | |
| if (typeof payload.reason === 'string') sanitized.reason = payload.reason; | |
| body = JSON.stringify(sanitized); | |
| const json: unknown = await request.json(); | |
| // Validate payload shape before forwarding (defense-in-depth) | |
| if (typeof json !== 'object' || json === null || Array.isArray(json)) { | |
| return NextResponse.json({ error: 'Body must be a JSON object' }, { status: 400 }); | |
| } | |
| const payload = json as Record<string, unknown>; | |
| if (!('amount' in payload)) { | |
| return NextResponse.json({ error: 'Missing required field: amount' }, { status: 400 }); | |
| } | |
| if ( | |
| typeof payload.amount !== 'number' || | |
| !Number.isFinite(payload.amount) || | |
| !Number.isInteger(payload.amount) | |
| ) { | |
| return NextResponse.json( | |
| { error: 'Field "amount" must be a finite integer' }, | |
| { status: 400 }, | |
| ); | |
| } | |
| if (payload.amount === 0) { | |
| return NextResponse.json( | |
| { error: 'Field "amount" must be non-zero' }, | |
| { status: 400 }, | |
| ); | |
| } | |
| if (Math.abs(payload.amount) > 1_000_000) { | |
| return NextResponse.json( | |
| { error: 'Field "amount" must be between -1000000 and 1000000' }, | |
| { status: 400 }, | |
| ); | |
| } | |
| if ('reason' in payload && payload.reason !== undefined && typeof payload.reason !== 'string') { | |
| return NextResponse.json( | |
| { error: 'Field "reason" must be a string when provided' }, | |
| { status: 400 }, | |
| ); | |
| } | |
| // Only forward the known fields — strip unknown keys | |
| const sanitized: { amount: number; reason?: string } = { amount: payload.amount as number }; | |
| if (typeof payload.reason === 'string') sanitized.reason = payload.reason; | |
| body = JSON.stringify(sanitized); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/app/api/guilds/`[guildId]/members/[userId]/xp/route.ts around lines
40 - 72, The client-side validation in this route currently checks payload,
amount type, and reason but omits the backend's additional constraints (reject
zero and enforce magnitude <= 1_000_000), so update the validation around
payload.amount (the block that currently checks Number.isFinite and
Number.isInteger) to also return 400 when payload.amount === 0 and when
Math.abs(payload.amount) > 1_000_000, using NextResponse.json with clear error
messages consistent with the backend; ensure sanitized remains created only
after these checks and that body = JSON.stringify(sanitized) is unchanged.
| const onGuildChange = useCallback(() => { | ||
| setMembers([]); | ||
| setNextAfter(null); | ||
| setTotal(0); | ||
| setFilteredTotal(null); | ||
| setError(null); | ||
| }, []); | ||
| resetAll(); | ||
| }, [resetAll]); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider stale data visibility on page revisit.
The useGuildSelection hook only calls onGuildChange when the guild ID actually changes via localStorage or CustomEvent. On component mount when revisiting the page, if the same guild is still selected, onGuildChange is NOT called.
Combined with the Zustand store that persists state across component unmount/remount, this means old members data will briefly render before the fetch effect (lines 169-179) refreshes it.
This is a valid stale-while-revalidate (SWR) pattern, but be aware that if the initial fetch fails, stale data remains visible alongside the error message. If this UX is undesirable, consider clearing members at the start of fetchMembers when append: false.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/app/dashboard/members/page.tsx` around lines 84 - 86, The hook
currently doesn't clear persisted members when revisiting the page with the same
guild, so stale entries can remain visible if the initial re-fetch fails; update
the logic so that either onGuildChange is invoked on mount or—preferably—modify
fetchMembers to clear the stored members when called with append: false (e.g.,
set members = [] / call resetAll at the start of fetchMembers) so the UI doesn't
show stale members while the fresh request is in-flight; reference the
fetchMembers function and the resetAll/onGuildChange handlers to implement this
change.
| // Cases filters & pagination | ||
| page: number; | ||
| sortDesc: boolean; | ||
| actionFilter: string; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider: Type actionFilter more strictly.
actionFilter: string could be a union type (e.g., 'all' | 'warn' | 'ban' | ...) to prevent invalid values and improve autocomplete. This is optional if the valid values are dynamic or defined elsewhere.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/stores/moderation-store.ts` at line 7, The property actionFilter in
moderation-store.ts is typed too loosely as string; tighten it to a union type
(e.g., type ActionFilter = 'all' | 'warn' | 'ban' | 'mute' | ...) and use that
type for the actionFilter property to prevent invalid values and enable editor
autocomplete; update the declaration (actionFilter: ActionFilter), export the
ActionFilter type if consumed elsewhere, and adjust any places that set or read
actionFilter (including default values and setter functions) to use the new
union members or a mapped constant list.
Replace raw loading text with animated Skeleton components on: - Audit log, tickets, conversations, temp-roles pages (skeleton table rows) - Role selector and channel selector popovers (skeleton list items) Partially addresses #144
- Create ErrorBoundary class component in web/src/components/ui/ - Shows friendly error card with AlertTriangle icon and 'Try Again' button - Supports custom fallback render prop for flexible error UIs - Wrap AnalyticsDashboard (main dashboard page) and ModerationStats - Dev mode surfaces the raw error message for easier debugging Partially addresses #144
Use sonner toasts to surface success/error feedback for: - XP adjustment (toast.success / toast.error alongside inline state) - Member CSV export (toast.success on download, toast.error on failure) Config editor already had comprehensive toasts. Partially addresses #144
Validate the incoming JSON body before forwarding to the bot API: - Reject non-object payloads with 400 - Require 'amount' field as a finite number - Reject non-string 'reason' values - Strip unknown keys (only forward amount + reason) Defense-in-depth: catches bad clients before they hit the backend. Partially addresses #144
- Export startRateLimitCleanup() for testing and graceful re-init - Add 'stale entry cleanup (interval sweep)' test that verifies the 5-minute setInterval sweep actually removes entries whose last activity is older than their retention window (uses fake timers) The cleanup itself was already implemented; this adds test coverage and makes the function available for external callers. Partially addresses #144
- Replace console.error in ErrorBoundary with project logger
- Use 'skeleton-${i}' key prefix instead of bare index (noArrayIndexKey)
- Organize imports in tickets and conversations pages
- Format pass (biome)
…config validation, logger shim)
…o-op server logger
BillChirico
force-pushed
the
chore/issue-144
branch
from
9a5b24a to
c444978
Compare
🧹 Preview Environment Cleaned UpThe Railway preview environment for this PR has been removed. Environment: |
Summary
Selected improvements from #144 tech debt backlog.
Changes
ErrorBoundarycomponent with retry button; wraps analytics and moderation statsstartRateLimitCleanup(), added stale-sweep test with fake timersPartially addresses #144