chore: deferred tech debt and reviewer-flagged improvements (#144)#306
Open
BillChirico wants to merge 42 commits intomainfrom
Open
chore: deferred tech debt and reviewer-flagged improvements (#144)#306BillChirico wants to merge 42 commits intomainfrom
BillChirico wants to merge 42 commits intomainfrom
Conversation
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
- Upgrade web/src/lib/logger.ts browser runtime from no-op to structured console wrapper with [VolvoxDash] prefix and level tags - Browser logger suppresses debug/info in production, surfaces warn/error - Replace console.error in ErrorBoundary with logger.error + sonner toast - Replace console.warn in config-categories with logger.warn - Remove all raw console.* calls from browser code - Update src/logger.js TODO to reflect browser shim is implemented
- Extract slash command dispatch (interactionCreate) into
src/modules/events/commandInteraction.js
- Extract ClientReady slash command registration into
src/modules/events/clientReady.js
- Move shardDisconnect handler into events/errors.js alongside
existing error handlers (was duplicated in index.js)
- Remove duplicate client.on('error') handler from index.js
(already handled by registerErrorHandlers)
- Clean up unused imports (Events, getConfig, debug) from index.js
- Update events.js dispatcher with organized handler groups
- index.js reduced from 529 to 413 lines
…ions - Move fetchMembers into members-store with abort signal support - Move fetchStats, fetchCases, fetchUserHistory into moderation-store - Delete use-moderation-cases, use-moderation-stats, use-user-history hooks - Pages now use store actions directly instead of local useState/useEffect - Add unified 'ok' | 'unauthorized' | 'error' return type for fetch actions - Moderation store clearUserHistory now resets data/error state inline
- 15 tests for members-store: sync actions, fetchMembers success/error/401/abort - 24 tests for moderation-store: sync actions, fetchStats/fetchCases/fetchUserHistory - Covers URL construction, filter params, sort reversal, error extraction
…onsiveness to dashboard pages - Wrap all dashboard pages with ErrorBoundary component - Add reusable skeleton components (StatCardSkeleton, TableSkeleton, etc.) - Replace alert()/confirm() with sonner toast + Dialog in temp-roles - Add toast feedback to performance threshold saves - Make filter bars stack vertically on mobile (flex-col → sm:flex-row) - Add responsive column hiding to temp-roles table - Improve search input width behavior at small breakpoints Part of #144 (Dashboard UX)
- Reject amount=0 (meaningless operation) - Enforce ±1,000,000 bounds on XP adjustments - Cap reason length at 500 characters - Add comprehensive test suite (15 tests) - Fix @vitejs/plugin-react version (^6 needs vite 8, we have 7)
- Test all reusable skeleton components (StatCard, Table, Chart, etc.) - Test ErrorBoundary rendering, custom fallback, and reset behavior - 15 passing tests Part of #144
- Add tests/modules/events/commandInteraction.test.js (10 tests) - Add tests/modules/events/clientReady.test.js (3 tests) - Remove moved handler tests from tests/index.test.js - All 189 test files, 3869 tests passing
…aints - Add min/max to numeric schema fields (historyLength, retentionDays, etc.) - Add maxLength to string fields (systemPrompt capped at 4000 chars) - Enforce enum values for ai.defaultChannelMode - Validate min/max and maxLength in validateValue() - Add 20 new tests covering range, length, enum, and openProperties
- Remove unused imports in test files - Fix import ordering (biome auto-fix) - Remove unused eslint-disable comment from logger.ts - Fix ternary formatting in logger.ts
railway-app
bot
temporarily deployed
to
volvox-bot / volvox-bot-pr-306
|
🚅 Deployed to the volvox-bot-pr-306 environment in volvox-bot
|
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (36)
📝 WalkthroughWalkthroughThis PR centralizes Discord bot event handler registration by extracting slash-command and interaction-handling logic from src/index.js into dedicated modules (clientReady.js, commandInteraction.js) within a new registerEventHandlers lifecycle. It also implements a proper browser-compatible logger with console prefixing and development-aware logging. Tests are reorganized to reflect the modular structure. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerLet us write the prompt for your AI agent so you can ship faster (with fewer bugs). View plan for ticket: ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
added 5 commits
March 15, 2026 13:55
- Replace spinner in ticket detail with card/content skeleton - Replace 'Loading conversation...' text with chat bubble skeletons - Wrap detail pages (ticket, conversation) with ErrorBoundary - Add ErrorBoundary import to member detail page Part of #144
…nd cleanup - Add sweep() method to manually trigger stale entry removal - Add size() method to inspect tracked IP count - Add 5 new tests covering sweep, size, selective removal, and interval sweep
…README - Rewrite 012_placeholder with full history of the naming conflict - Add migrations/README.md with commands, numbering guide, and anomaly docs
- Add overflow-x-auto to conversations, tickets, and audit-log tables - Make filter select triggers full-width on mobile - Prevents horizontal overflow at 375px viewport Part of #144
…day window The community stats endpoint was filtering user_stats by last_active >= 7 days, which made totalMessagesSent report only messages from recently-active users. This was misleading since the field name implies an all-time aggregate. - Remove the 7-day last_active filter from the messages query - Use ::bigint instead of ::int to handle large totals safely - Add OpenAPI description clarifying all-time scope - Update test comment to reflect corrected behavior
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses deferred tech-debt items by extracting core Discord event handlers into dedicated modules, improving dashboard-side logging, and relocating event-handler tests out of the src/index.js harness.
Changes:
- Extracted
clientReady+ slash commandinteractionCreatehandling intosrc/modules/events/*and wired them viaregisterEventHandlers. - Implemented a real browser logger shim for the dashboard and replaced direct
console.*usage withlogger. - Moved related tests from
tests/index.test.jsinto focusedtests/modules/events/*suites.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/lib/logger.ts | Implements browser logger wrapper (level-aware) and keeps server-side stdout/stderr logging. |
| web/src/components/ui/error-boundary.tsx | Switches from console.error to logger and adds toast notification on caught errors. |
| web/src/components/dashboard/config-workspace/config-categories.ts | Replaces console.warn with logger.warn for missing config category IDs. |
| tests/modules/events/commandInteraction.test.js | New unit tests for extracted slash command + autocomplete handler. |
| tests/modules/events/clientReady.test.js | New unit tests for extracted ClientReady command registration handler. |
| tests/index.test.js | Removes inline event-handler tests from the index.js test harness and adds pointers to new locations. |
| src/modules/events/errors.js | Enhances Discord error logging and adds shard disconnect warning behavior. |
| src/modules/events/commandInteraction.js | New module for slash command dispatch + autocomplete handling. |
| src/modules/events/clientReady.js | New module for registering slash commands on ClientReady. |
| src/modules/events.js | Becomes a cleaner dispatcher that composes event handler registrations. |
| src/logger.js | Updates module header comment to reference the dashboard logger shim. |
| src/index.js | Removes inline handlers now registered via registerEventHandlers(). |
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+525
to
526
| // Discord client error tests moved to tests/modules/events/errors.test.js | ||
|
|
| * @param {import('discord.js').Client} client - Discord client | ||
| */ | ||
| export function registerCommandInteractionHandler(client) { | ||
| client.on('interactionCreate', async (interaction) => { |
Comment on lines
+4
to
+7
| * Browser runtime: thin wrapper around console methods that adds a | ||
| * `[VolvoxDash]` prefix and timestamp for consistent structured logging | ||
| * visible in DevTools. Only `warn` and `error` are active in production | ||
| * builds; `debug` and `info` are suppressed unless `NODE_ENV === 'development'`. |
Comment on lines
+51
to
+52
| toast.error('Something went wrong', { | ||
| description: error.message, |
|
| Filename | Overview |
|---|---|
| TASK2.md | Internal development task document that should not be committed to the repository — contains shell commands, internal workflow notes, and references to disabling code review tools. |
| src/modules/events.js | Refactored to a clean dispatcher: all inline handlers extracted to events/ subdirectory, re-exported for backward compatibility, and registered via a single registerEventHandlers() call. |
| src/modules/events/errors.js | New error handler module; async uncaughtException handler returns an unawaited Promise, and the module-level processHandlersRegistered guard could misbehave under Vitest module resets. |
| src/api/routes/community.js | Public community API with improved OpenAPI docs and caching on the leaderboard endpoint; showcases route uses string-interpolated ORDER BY derived from user query input, which violates the project's parameterized-query rule even though it's safe as written. |
| src/api/utils/configValidation.js | New shared config validation utility centralising CONFIG_SCHEMA and validateValue/validateSingleValue; comprehensive schema coverage with proper type, range, and enum checks. |
| web/src/lib/logger.ts | New browser/server logger shim: suppresses debug/info in production, adds structured [VolvoxDash] prefix in browser, falls back to stderr/stdout on server without importing Winston; biome-ignore lint annotation is correctly scoped. |
| web/src/app/api/guilds/[guildId]/members/[userId]/xp/route.ts | XP proxy route with thorough defense-in-depth validation: type, finiteness, zero-check, magnitude cap, reason string length, and unknown-key stripping before forwarding. |
| web/src/stores/moderation-store.ts | New Zustand store for moderation page; all three fetch methods read res.json() before checking res.ok, which will throw a JSON-parse error instead of a useful message when the server returns a non-JSON error body. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["src/index.js\nBot entry point"] --> B["registerEventHandlers(client)"]
B --> C["events.js\nDispatcher"]
C --> D["events/clientReady.js\nregisterClientReadyHandler"]
C --> E["events/commandInteraction.js\nregisterCommandInteractionHandler"]
C --> F["events/errors.js\nregisterErrorHandlers"]
C --> G["events/interactionCreate.js\nButton/modal handlers"]
C --> H["events/messageCreate.js\nregisterMessageCreateHandler"]
C --> I["Other handlers\n(ready, guildMemberAdd, reactions, voiceState)"]
D -->|"ClientReady"| D1["registerCommands()"]
E -->|"interactionCreate"| E1{"isAutocomplete?"}
E1 -->|Yes| E2["command.autocomplete()"]
E1 -->|No| E3{"hasPermission?"}
E3 -->|No| E4["safeReply: permission denied"]
E3 -->|Yes| E5["command.execute()"]
E5 --> E6["logCommandUsage() fire-and-forget"]
F --> F1["client.on(Error)"]
F --> F2["client.on(ShardDisconnect)"]
F --> F3{"processHandlersRegistered?"}
F3 -->|No| F4["process.on(unhandledRejection)\nprocess.on(uncaughtException)"]
F3 -->|Yes| F5["Skip — already registered"]
Prompt To Fix All With AI
This is a comment left during a code review.
Path: TASK2.md
Line: 1-69
Comment:
**Internal task document committed to the repository**
`TASK2.md` is an internal development task-tracking document and should not live in the repository. It contains shell commands for codebase investigation, notes about disabling specific code review tools (including Greptile), and workflow instructions that are better suited to an issue tracker or project board.
Please remove this file from the commit — or add it to `.gitignore` if it's used as a local scratchpad.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/api/routes/community.js
Line: 328-347
Comment:
**SQL string interpolation for `ORDER BY` clause**
`orderBy` is derived from a strict ternary and is never directly the raw user input, so this isn't exploitable as written. However, it violates the project's rule of never interpolating user-supplied (or user-influenced) values into SQL strings.
The `req.query.sort` value influences `orderBy`, and the current pattern (string interpolation in a template literal inside the query string) can silently become a real injection vector if someone later extends the conditional without the same care.
A safe, canonical alternative for dynamic `ORDER BY` in PostgreSQL is a `CASE` expression with parameterised column references:
```js
const orderBy =
sort === 'recent'
? 's.created_at DESC'
: 's.upvotes DESC, s.created_at DESC';
pool.query(
`SELECT ...
ORDER BY ${orderBy} -- ← only two pre-approved literals reach here
LIMIT $2 OFFSET $3`,
[guildId, limit, offset],
)
```
If you ever expand the sort options to dynamic column names, switch to a `CASE` expression or an explicit allowlist map to keep the parameterised-query invariant intact.
**Rule Used:** Always use parameterized queries ($1, $2) with pg.... ([source](https://app.greptile.com/review/custom-context?memory=parameterized-queries))
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/modules/events/errors.js
Line: 40-52
Comment:
**`async` `uncaughtException` handler returns an unawaited Promise**
`process.on('uncaughtException', async (err) => { ... })` registers an asynchronous handler. Node.js calls it synchronously from the event-emitter perspective — it does **not** await the returned Promise. This means:
1. As soon as the handler hits the first `await` (the dynamic `import`), control returns to the event loop and the process is considered to have "handled" the exception.
2. If a second uncaught exception fires while the Sentry flush is still in progress (between `await import(...)` and `process.exit(1)`), the handler will be entered a second time, resulting in two concurrent async handlers racing to call `process.exit(1)`.
The try/catch and `process.exit(1)` guarantee the process eventually terminates, so this is not a crash risk, but the handler's async nature is worth noting. A common mitigation is to pre-import Sentry at module load time so the handler can remain synchronous:
```js
// Top of file — eagerly resolved so the handler can stay synchronous
import * as Sentry from '../../sentry.js';
process.on('uncaughtException', (err) => {
logError('Uncaught exception — shutting down', {
error: err?.message || String(err),
stack: err?.stack,
});
try {
Sentry.flush(2000).finally(() => process.exit(1));
} catch {
process.exit(1);
}
});
```
If the dynamic import is intentional (to avoid circular-dependency issues at startup), the current code is acceptable — just be aware of the above edge case.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: web/src/stores/moderation-store.ts
Line: 130-133
Comment:
**`fetchStats` reads the response body before checking `res.ok`**
The current order is:
```ts
const payload: unknown = await res.json();
if (!res.ok) {
const msg = extractErrorMessage(payload, 'Failed to fetch stats');
throw new Error(msg);
}
```
This works as long as the server always returns valid JSON on error responses. If the server ever returns a non-JSON error body (e.g., a 500 HTML page from a gateway), `res.json()` will throw before `res.ok` is checked, resulting in a generic JSON-parse error instead of a meaningful message.
The same pattern appears in `fetchCases` (lines 176–180) and `fetchUserHistory` (lines 215–219). Consider guarding with `res.ok` first, or wrapping `res.json()` in a try/catch to fall back gracefully:
```ts
let payload: unknown;
try {
payload = await res.json();
} catch {
payload = null;
}
if (!res.ok) {
const msg = extractErrorMessage(payload, 'Failed to fetch stats');
throw new Error(msg);
}
```
This issue appears in all three fetch methods in this store.
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 22341da
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| source: 'discord_client', | ||
| }); | ||
| }); | ||
| // Discord client error tests moved to tests/modules/events/errors.test.js |
There was a problem hiding this comment.
Missing errors.test.js test file
This comment says the Discord client error and shard disconnect tests were moved to tests/modules/events/errors.test.js, but that file does not exist in this PR. The original test for 'Discord client error' logging (with error, stack, code, and source fields) was removed from this file, and the new ShardDisconnect handler added in src/modules/events/errors.js also has no test coverage.
Please add tests/modules/events/errors.test.js to cover:
- The
Events.Errorhandler (previously tested here) - The new
Events.ShardDisconnecthandler (non-1000 code logs a warning, code 1000 is silently ignored)
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/index.test.js
Line: 525
Comment:
**Missing `errors.test.js` test file**
This comment says the Discord client error and shard disconnect tests were moved to `tests/modules/events/errors.test.js`, but that file does not exist in this PR. The original test for `'Discord client error'` logging (with `error`, `stack`, `code`, and `source` fields) was removed from this file, and the new `ShardDisconnect` handler added in `src/modules/events/errors.js` also has no test coverage.
Please add `tests/modules/events/errors.test.js` to cover:
1. The `Events.Error` handler (previously tested here)
2. The new `Events.ShardDisconnect` handler (non-1000 code logs a warning, code 1000 is silently ignored)
How can I resolve this? If you propose a fix, please make it concise.…onsiveness to dashboard pages - Wrap all dashboard pages with ErrorBoundary component - Add reusable skeleton components (StatCardSkeleton, TableSkeleton, etc.) - Replace alert()/confirm() with sonner toast + Dialog in temp-roles - Add toast feedback to performance threshold saves - Make filter bars stack vertically on mobile (flex-col → sm:flex-row) - Add responsive column hiding to temp-roles table - Improve search input width behavior at small breakpoints Part of #144 (Dashboard UX)
- Test all reusable skeleton components (StatCard, Table, Chart, etc.) - Test ErrorBoundary rendering, custom fallback, and reset behavior - 15 passing tests Part of #144
- Replace spinner in ticket detail with card/content skeleton - Replace 'Loading conversation...' text with chat bubble skeletons - Wrap detail pages (ticket, conversation) with ErrorBoundary - Add ErrorBoundary import to member detail page Part of #144
- Add overflow-x-auto to conversations, tickets, and audit-log tables - Make filter select triggers full-width on mobile - Prevents horizontal overflow at 375px viewport Part of #144
- Auto-format all dashboard pages to pass biome check - Remove unused ErrorBoundary import from member detail page Part of #144
… chore/issue-144-tech-debt
Resolves merge conflicts between Zustand store migration and infrastructure validation branches. Accepts state branch's dashboard page changes (Zustand integration) and infra branch's backend validation changes.
Resolves conflicts between dashboard UX improvements and infra validation changes. Takes dashboard's skeleton/error boundary/toast additions for detail pages.
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses deferred tech debt from #144 by refactoring Discord bot event wiring into dedicated modules and improving the web dashboard’s UX/state management and client-side logging/validation.
Changes:
- Extracted Discord
clientReady+ command interaction handling intosrc/modules/events/*and updated tests accordingly. - Added/expanded dashboard UX primitives (error boundaries, skeletons, toasts) and migrated moderation/members pages toward Zustand-based state + fetch helpers.
- Hardened validation and operational utilities (XP proxy payload validation, config schema ranges, rate-limit map sweeping, migration docs/placeholder).
Reviewed changes
Copilot reviewed 46 out of 47 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/stores/moderation-store.ts | Adds fetch/state management for moderation data via Zustand. |
| web/tests/stores/moderation-store.test.ts | Adds unit tests for moderation store behavior. |
| web/src/stores/members-store.ts | Adds fetch/state management for members list via Zustand. |
| web/tests/stores/members-store.test.ts | Adds unit tests for members store behavior. |
| web/src/lib/logger.ts | Implements a browser logger shim (console wrapper) with dev/prod level gating. |
| web/src/components/dashboard/skeletons.tsx | Introduces reusable dashboard skeleton components. |
| web/tests/components/dashboard/skeletons.test.tsx | Tests skeleton rendering behavior. |
| web/tests/components/dashboard/error-boundary.test.tsx | Tests the new/updated error boundary component behavior. |
| web/src/components/dashboard/performance-dashboard.tsx | Adds toast notifications around threshold saves (and related UI updates). |
| web/src/components/dashboard/config-workspace/config-categories.ts | Replaces console.warn with the dashboard logger. |
| web/src/app/dashboard/* | Wraps multiple pages in ErrorBoundary, improves mobile table overflow, adds skeletons/toasts/confirm dialogs. |
| web/src/app/api/guilds/[guildId]/members/[userId]/xp/route.ts | Adds defense-in-depth validation + sanitization for XP proxy payloads. |
| web/tests/api/xp-route.test.ts | Tests XP proxy route validation and sanitization behavior. |
| web/package.json | Adjusts dependency version for @vitejs/plugin-react. |
| src/modules/events.js | Registers extracted event modules from a central dispatcher. |
| src/modules/events/clientReady.js | New handler for slash command registration on ready. |
| src/modules/events/commandInteraction.js | New handler for slash commands + autocomplete. |
| src/modules/events/errors.js | Improves discord error logging and adds shard disconnect warnings. |
| src/index.js | Removes inline event handler code now delegated to registerEventHandlers. |
| src/api/utils/configValidation.js | Adds min/max/maxLength constraints and validates them. |
| tests/api/utils/configValidation.test.js | Adds tests for new config validation constraints. |
| src/api/middleware/rateLimit.js | Adds sweep()/size() helpers and keeps interval cleanup. |
| tests/api/middleware/rateLimit.test.js | Adds tests for sweep()/size()/destroy() behaviors. |
| src/api/routes/community.js | Adjusts totalMessagesSent to be all-time and uses bigint conversion. |
| tests/api/routes/community.test.js | Updates tests/comments to match new stats semantics. |
| migrations/README.md | Documents migration numbering anomaly and placeholder usage. |
| migrations/012_placeholder.cjs | Expands placeholder migration documentation (still no-op). |
| tests/modules/events/clientReady.test.js | Adds focused tests for the extracted clientReady handler. |
| tests/modules/events/commandInteraction.test.js | Adds focused tests for the extracted command interaction handler. |
| tests/index.test.js | Removes moved event tests and points to new test locations. |
| TASK2.md | Adds a checklist-style doc for remaining #144 items. |
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+336
to
+340
| it('fetchUserHistory silently handles AbortError', async () => { | ||
| const abortError = new DOMException('Aborted', 'AbortError'); | ||
| vi.spyOn(globalThis, 'fetch').mockRejectedValueOnce(abortError); | ||
|
|
||
| const result = await useModerationStore.getState().fetchUserHistory('guild1', 'user123', 1); |
Comment on lines
+145
to
+149
| } catch (err) { | ||
| if (isAbortError(err)) return 'ok'; | ||
| set({ | ||
| statsError: err instanceof Error ? err.message : 'Failed to fetch stats', | ||
| statsLoading: false, |
Comment on lines
+222
to
+226
| } catch (err) { | ||
| if (isAbortError(err)) return 'ok'; | ||
| set({ | ||
| userHistoryError: err instanceof Error ? err.message : 'Failed to fetch user history', | ||
| userHistoryLoading: false, |
| "@types/react": "^19.2.14", | ||
| "@types/react-dom": "^19.2.3", | ||
| "@vitejs/plugin-react": "^6.0.0", | ||
| "@vitejs/plugin-react": "^4.7.0", |
Comment on lines
+188
to
+192
| } catch (err) { | ||
| if (isAbortError(err)) return 'ok'; | ||
| set({ | ||
| casesError: err instanceof Error ? err.message : 'Failed to fetch cases', | ||
| casesLoading: false, |
Comment on lines
+16
to
+19
| export function registerCommandInteractionHandler(client) { | ||
| client.on('interactionCreate', async (interaction) => { | ||
| // Handle autocomplete | ||
| if (interaction.isAutocomplete()) { |
Comment on lines
16
to
18
| } from 'recharts'; | ||
| import { toast } from 'sonner'; | ||
| import { Badge } from '@/components/ui/badge'; |
Comment on lines
+4
to
+7
| * Browser runtime: thin wrapper around console methods that adds a | ||
| * `[VolvoxDash]` prefix and timestamp for consistent structured logging | ||
| * visible in DevTools. Only `warn` and `error` are active in production | ||
| * builds; `debug` and `info` are suppressed unless `NODE_ENV === 'development'`. |
Comment on lines
+207
to
+211
| it('fetchStats silently handles AbortError', async () => { | ||
| const abortError = new DOMException('Aborted', 'AbortError'); | ||
| vi.spyOn(globalThis, 'fetch').mockRejectedValueOnce(abortError); | ||
|
|
||
| const result = await useModerationStore.getState().fetchStats('guild1'); |
Comment on lines
+1
to
+69
| # TASK2: Remaining Issue #144 Items | ||
|
|
||
| Already done (skip these): | ||
| - Loading skeletons ✅ | ||
| - Error boundaries ✅ | ||
| - Toast notifications ✅ | ||
| - XP proxy validation ✅ | ||
| - Rate limit stale cleanup ✅ | ||
|
|
||
| ## Items to implement now | ||
|
|
||
| ### 1. Zustand store for member page | ||
| - File: `web/src/app/dashboard/[guildId]/members/page.tsx` (or similar) | ||
| - Find all useState/useEffect hooks managing member state | ||
| - Create `web/src/stores/members-store.ts` with Zustand | ||
| - Migrate: member list, loading state, search query, selected member, pagination | ||
| - Keep component using the store hooks | ||
|
|
||
| ### 2. Zustand store for moderation page | ||
| - File: `web/src/app/dashboard/[guildId]/moderation/page.tsx` (or similar) | ||
| - Same pattern — create `web/src/stores/moderation-store.ts` | ||
| - Migrate: cases list, filters, loading, pagination | ||
|
|
||
| ### 3. console.error cleanup in browser code | ||
| - `rg -rn "console\.error" web/src/` — find all occurrences in client components | ||
| - Replace with: toast.error() for user-facing errors, or keep console.error where it's truly logging-only | ||
| - Do NOT replace server-side console.error (only client components) | ||
|
|
||
| ### 4. events.js function extraction | ||
| - File: `src/modules/events/` — check if there's a monolithic events file or inline handlers in interactionCreate.js | ||
| - Run: `wc -l src/modules/events/interactionCreate.js` to check size | ||
| - If handlers are inline (ticket_open, ticket_close, poll, etc.), extract each to separate files in `src/modules/handlers/` | ||
| - Update interactionCreate.js to import from handlers | ||
|
|
||
| ### 5. Mobile responsiveness — critical fixes only | ||
| - `rg -rn "grid-cols-2\|grid-cols-3\|table\b" web/src/components/dashboard/ --include="*.tsx" | head -30` | ||
| - Add `sm:grid-cols-2` fallbacks where fixed grid-cols are used | ||
| - Add `overflow-x-auto` wrapper around data tables | ||
| - Focus on: member table, moderation cases table, config sections with grids | ||
|
|
||
| ### 6. Migration gap — add placeholder comment | ||
| - Create `migrations/012_placeholder.cjs` with a comment explaining the gap | ||
| - Content: migration that logs a warning about the gap, does nothing (no-op) | ||
|
|
||
| ### 7. Fix totalMessagesSent stat accuracy | ||
| - File: find where community page stats are calculated | ||
| - `rg -rn "totalMessagesSent\|messages_sent" web/src/ src/` | ||
| - Add a comment explaining the known limitation, or filter to last 30 days | ||
| - If it's a simple query change, fix it; if architectural, add a TODO comment | ||
|
|
||
| ### 8. Review bot consolidation (GitHub config) | ||
| - Disable Copilot and Greptile PR reviewers — keep Claude (coderabbitai style) + CodeRabbit | ||
| - Check `.github/` for reviewer config files | ||
| - Check `CODEOWNERS` or `.github/pull_request_review_protection.yml` | ||
| - If via GitHub API: `gh api repos/VolvoxLLC/volvox-bot/automated-security-fixes` | ||
| - Note: Greptile may be configured in `.greptile.yaml` or via webhook — find and disable | ||
|
|
||
| ## Architectural items (document only, don't implement) | ||
| These need separate planning — just add TODO comments: | ||
| - Server-side member search (needs Discord member cache or DB index) | ||
| - Conversation search pagination (needs full-text search index) | ||
| - Config patch deep validation (needs schema per config key) | ||
| - Logger browser shim (nice-to-have, low impact) | ||
|
|
||
| ## Rules | ||
| - Commit each fix separately with conventional commits | ||
| - Run `pnpm format && pnpm lint && pnpm test` | ||
| - Run `pnpm --prefix web lint && pnpm --prefix web typecheck` | ||
| - Do NOT push |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/lib/logger.ts (1)
18-25:⚠️ Potential issue | 🟡 MinorPreserve
Errordetails in the server logger.
JSON.stringify(new Error('x'))returns{}, so passing a realErrorinto this logger drops the message and stack completely.Suggested fix
function formatArg(arg: unknown): string { if (typeof arg === 'string') return arg; + if (arg instanceof Error) { + return JSON.stringify({ + name: arg.name, + message: arg.message, + stack: arg.stack, + }); + } try { return JSON.stringify(arg); } catch { return String(arg);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/lib/logger.ts` around lines 18 - 25, The formatArg function currently loses Error details because JSON.stringify(new Error(...)) yields "{}", so update formatArg in web/src/lib/logger.ts to special-case Error instances (use instanceof Error) and return a string that includes the error name, message and stack (falling back to message or String(arg) if stack is missing) instead of JSON.stringify; keep the existing string/JSON/string fallback behavior for non-Error values so other types are unchanged.
🤖 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/modules/events/commandInteraction.js`:
- Around line 69-75: The fire-and-forget call to logCommandUsage({ guildId:
interaction.guildId, userId: interaction.user.id, commandName, channelId:
interaction.channelId }) should explicitly handle promise rejections; keep it
non-awaited but append a .catch(...) to the call and log the error (using your
existing logger or console.error) so failures are recorded at the call site
(reference: function logCommandUsage and variables interaction and commandName).
- Around line 19-28: The autocomplete branch may return without acknowledging
the AutocompleteInteraction; update the logic around
interaction.isAutocomplete() so that if a command handler is missing
(command?.autocomplete is falsy) or if command.autocomplete throws, you call
interaction.respond([]) before returning; when command.autocomplete exists,
await it inside the try/catch and on catch call interaction.respond([]) and log
the error (use error('Autocomplete error', { command: interaction.commandName,
error: err.message })), ensuring you only call interaction.respond once per
interaction. Use the existing symbols interaction.isAutocomplete(),
client.commands.get(interaction.commandName), command.autocomplete, and
interaction.respond to locate and modify the code.
In `@tests/modules/events/commandInteraction.test.js`:
- Around line 51-54: The afterEach hook currently calls vi.restoreAllMocks(),
which doesn't clear automocked/module mocks like hasPermission; replace
vi.restoreAllMocks() with vi.clearAllMocks() in the afterEach block to properly
reset mock call history and isolate tests while keeping the
hasPermission.mockReturnValue(true) reestablishment intact.
In `@web/src/components/dashboard/config-workspace/config-categories.ts`:
- Around line 361-365: The warning says we fall back to the declared default but
the code still returns CONFIG_CATEGORIES[0]; change getCategoryById to return
the actual declared default category instead of hardcoding index 0—locate the
project's default identifier/constant (e.g., DEFAULT_CONFIG_CATEGORY,
CONFIG_CATEGORIES_DEFAULT, or DEFAULT_CONFIG_CATEGORY_ID) and use it to find the
matching entry in CONFIG_CATEGORIES (e.g., CONFIG_CATEGORIES.find(c => c.id ===
DEFAULT_...)); if that lookup somehow fails, then fall back to
CONFIG_CATEGORIES[0] as a last resort so behavior matches the logged message.
In `@web/src/components/ui/error-boundary.tsx`:
- Around line 51-53: The toast in ErrorBoundary currently displays raw exception
text (toast.error call in web/src/components/ui/error-boundary.tsx), which can
leak internals; change the user-facing toast description to a generic message
(e.g., "An unexpected error occurred") and move the detailed error output to
logging/monitoring (console.error or your logger) inside the same error handler
so the full error.message and stack are recorded but not shown to users; update
the toast invocation in the ErrorBoundary/onError handler accordingly.
---
Outside diff comments:
In `@web/src/lib/logger.ts`:
- Around line 18-25: The formatArg function currently loses Error details
because JSON.stringify(new Error(...)) yields "{}", so update formatArg in
web/src/lib/logger.ts to special-case Error instances (use instanceof Error) and
return a string that includes the error name, message and stack (falling back to
message or String(arg) if stack is missing) instead of JSON.stringify; keep the
existing string/JSON/string fallback behavior for non-Error values so other
types are unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1212f1e7-034e-4ecc-bade-ba141e4b2940
📒 Files selected for processing (12)
src/index.jssrc/logger.jssrc/modules/events.jssrc/modules/events/clientReady.jssrc/modules/events/commandInteraction.jssrc/modules/events/errors.jstests/index.test.jstests/modules/events/clientReady.test.jstests/modules/events/commandInteraction.test.jsweb/src/components/dashboard/config-workspace/config-categories.tsweb/src/components/ui/error-boundary.tsxweb/src/lib/logger.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts,tsx}: Use single quotes for strings (except in JSON files); no double quotes
Always include semicolons at the end of statements
Use 2-space indentation (spaces, not tabs)
Always include trailing commas in multi-line arrays, objects, and function parameters
Maintain a maximum line width of 100 characters
Files:
web/src/components/dashboard/config-workspace/config-categories.tsweb/src/components/ui/error-boundary.tsxsrc/modules/events/clientReady.jssrc/modules/events/commandInteraction.jssrc/logger.jstests/modules/events/clientReady.test.jssrc/index.jstests/modules/events/commandInteraction.test.jsweb/src/lib/logger.tstests/index.test.jssrc/modules/events.jssrc/modules/events/errors.js
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Never use
console.*methods in web dashboard code; use appropriate logging mechanisms for React applications
Files:
web/src/components/dashboard/config-workspace/config-categories.tsweb/src/components/ui/error-boundary.tsxweb/src/lib/logger.ts
**/*.{js,ts,tsx,mjs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,tsx,mjs}: Use ESM syntax (import/export) — CommonJS is not allowed
Use single quotes for strings — double quotes only allowed in JSON files
Always include semicolons at end of statements
Use 2-space indentation for all code
Use Winston logger fromsrc/logger.js— never useconsole.*methods
Files:
web/src/components/dashboard/config-workspace/config-categories.tsweb/src/components/ui/error-boundary.tsxsrc/modules/events/clientReady.jssrc/modules/events/commandInteraction.jssrc/logger.jstests/modules/events/clientReady.test.jssrc/index.jstests/modules/events/commandInteraction.test.jsweb/src/lib/logger.tstests/index.test.jssrc/modules/events.jssrc/modules/events/errors.js
**/*.js
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESM-only syntax:
import/export, neverrequire()/module.exports
Files:
src/modules/events/clientReady.jssrc/modules/events/commandInteraction.jssrc/logger.jstests/modules/events/clientReady.test.jssrc/index.jstests/modules/events/commandInteraction.test.jstests/index.test.jssrc/modules/events.jssrc/modules/events/errors.js
src/**/*.js
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.js: Never useconsole.*methods; use the Winston logger instead viaimport logger from '../logger.js'(adjust path as needed), then calllogger.info(),logger.warn(),logger.error(), orlogger.debug()
Always usesafeReply(),safeSend(), orsafeEditReply()instead of raw Discord.js methods for safe Discord messaging that handles errors gracefully
Files:
src/modules/events/clientReady.jssrc/modules/events/commandInteraction.jssrc/logger.jssrc/index.jssrc/modules/events.jssrc/modules/events/errors.js
src/modules/**/*.js
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Create feature modules in
src/modules/and add corresponding config sections toconfig.json
Files:
src/modules/events/clientReady.jssrc/modules/events/commandInteraction.jssrc/modules/events.jssrc/modules/events/errors.js
{src/commands/**/*.{js,ts},src/modules/**/*.{js,ts},src/index.js}
📄 CodeRabbit inference engine (AGENTS.md)
Use
safeReply(),safeSend(), orsafeEditReply()for Discord messages — never send unsafe messages directly
Files:
src/modules/events/clientReady.jssrc/modules/events/commandInteraction.jssrc/index.jssrc/modules/events.jssrc/modules/events/errors.js
tests/**/*.test.js
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
tests/**/*.test.js: Write bot tests using Vitest 4 with thenodeenvironment, matching thesrc/structure in thetests/directory
Maintain test coverage thresholds: statements 85%, branches 82%, functions 85%, lines 85%; never lower thresholds—add tests to cover new code instead
Files:
tests/modules/events/clientReady.test.jstests/modules/events/commandInteraction.test.jstests/index.test.js
🧠 Learnings (13)
📓 Common learnings
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-03-12T02:03:36.476Z
Learning: Applies to src/commands/**/*.js : Create slash command definitions in `src/commands/`, exporting a slash command builder and an `execute` function
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T20:36:29.483Z
Learning: Applies to src/**/{startup,command-register,reload}*.{js,ts} : Remove process.env.GUILD_ID runtime reads from bot startup and reload command registration
📚 Learning: 2026-03-10T23:21:49.730Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T23:21:49.730Z
Learning: Applies to web/src/components/dashboard/config-workspace/**/*.{ts,tsx} : Web dashboard config editor should use category workspace navigation with reusable SettingsFeatureCard pattern (header + master toggle + Basic/Advanced blocks)
Applied to files:
web/src/components/dashboard/config-workspace/config-categories.ts
📚 Learning: 2026-03-12T02:03:36.476Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-03-12T02:03:36.476Z
Learning: Applies to web/src/**/*.{ts,tsx} : Never use `console.*` methods in web dashboard code; use appropriate logging mechanisms for React applications
Applied to files:
web/src/components/dashboard/config-workspace/config-categories.tsweb/src/components/ui/error-boundary.tsxsrc/logger.jsweb/src/lib/logger.ts
📚 Learning: 2026-03-12T02:03:36.476Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-03-12T02:03:36.476Z
Learning: Applies to src/**/*.js : Never use `console.*` methods; use the Winston logger instead via `import logger from '../logger.js'` (adjust path as needed), then call `logger.info()`, `logger.warn()`, `logger.error()`, or `logger.debug()`
Applied to files:
web/src/components/dashboard/config-workspace/config-categories.tssrc/logger.jsweb/src/lib/logger.ts
📚 Learning: 2026-03-12T02:03:52.689Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-12T02:03:52.689Z
Learning: Applies to **/*.{js,ts,tsx,mjs} : Use Winston logger from `src/logger.js` — never use `console.*` methods
Applied to files:
web/src/components/dashboard/config-workspace/config-categories.tssrc/logger.jsweb/src/lib/logger.ts
📚 Learning: 2026-03-12T02:03:36.476Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-03-12T02:03:36.476Z
Learning: Applies to src/commands/**/*.js : Create slash command definitions in `src/commands/`, exporting a slash command builder and an `execute` function
Applied to files:
src/modules/events/clientReady.jssrc/modules/events/commandInteraction.jssrc/index.jstests/modules/events/commandInteraction.test.js
📚 Learning: 2026-03-10T20:36:29.483Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T20:36:29.483Z
Learning: Applies to src/**/{startup,command-register,reload}*.{js,ts} : Remove process.env.GUILD_ID runtime reads from bot startup and reload command registration
Applied to files:
src/modules/events/clientReady.jssrc/index.jstests/index.test.js
📚 Learning: 2026-03-10T20:36:29.483Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T20:36:29.483Z
Learning: Applies to web/src/pages/dashboard/**/*.{ts,tsx} : Use shared title helpers from web/src/lib/page-titles.ts for setting browser titles in dashboard pages
Applied to files:
src/logger.js
📚 Learning: 2026-03-12T02:03:52.689Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-12T02:03:52.689Z
Learning: Applies to web/src/app/**/*.{ts,tsx} : Use `DashboardTitleSync` component and `getDashboardDocumentTitle()` for client-side navigation title updates in the dashboard
Applied to files:
src/logger.js
📚 Learning: 2026-03-12T02:03:36.476Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-03-12T02:03:36.476Z
Learning: Applies to web/src/app/dashboard/**/*.tsx : For dashboard routes, add a matcher entry to `dashboardTitleMatchers` in `web/src/lib/page-titles.ts`: use exact equality for leaf routes (`pathname === '/dashboard/my-route'`) and subtree checks (`pathname.startsWith('/dashboard/my-route/')`); export `metadata` using `createPageMetadata(title)` for SSR entry points
Applied to files:
src/logger.js
📚 Learning: 2026-03-12T02:03:36.476Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-03-12T02:03:36.476Z
Learning: Applies to src/**/*.js : Always use `safeReply()`, `safeSend()`, or `safeEditReply()` instead of raw Discord.js methods for safe Discord messaging that handles errors gracefully
Applied to files:
src/index.jssrc/modules/events/errors.js
📚 Learning: 2026-03-12T02:03:36.476Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-03-12T02:03:36.476Z
Learning: Applies to src/utils/cache.js : Use `src/utils/cache.js` for generic caching with Redis primary + in-memory fallback; use `src/utils/discordCache.js` for Discord API response caching (channels, roles, members); use `src/utils/reputationCache.js` for leaderboard and user reputation data; all caches auto-invalidate when config changes
Applied to files:
src/index.js
📚 Learning: 2026-03-12T02:03:36.476Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-03-12T02:03:36.476Z
Learning: Applies to tests/**/*.test.js : Write bot tests using Vitest 4 with the `node` environment, matching the `src/` structure in the `tests/` directory
Applied to files:
tests/modules/events/commandInteraction.test.js
🪛 GitHub Check: SonarCloud Code Analysis
web/src/lib/logger.ts
[warning] 57-57: Unexpected negated condition.
src/modules/events.js
[warning] 32-32: Use export…from to re-export registerClientReadyHandler.
[warning] 33-33: Use export…from to re-export registerCommandInteractionHandler.
🔇 Additional comments (2)
src/modules/events/errors.js (1)
26-33:⚠️ Potential issue | 🟠 MajorEscalate fatal shard disconnects instead of logging and returning.
This handler only logs non-1000 closes and returns. For non-reconnectable gateway close codes (4004, 4010–4014 per Discord spec), that leaves the process, API server, and schedulers running while the bot stays permanently disconnected from Discord. The graceful shutdown path exists in
src/index.js, so this handler should trigger it on fatal codes rather than just warning.The suggested approach is to pass an escalation callback to
registerErrorHandlersand invoke it when a fatal code is detected, threading that callback from the startup path insrc/modules/events.jsthrough togracefulShutdown()(which will require exporting it fromsrc/index.js).⛔ Skipped due to learnings
Learnt from: CR Repo: VolvoxLLC/volvox-bot PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2026-03-12T02:03:36.476Z Learning: Applies to src/**/*.js : Always use `safeReply()`, `safeSend()`, or `safeEditReply()` instead of raw Discord.js methods for safe Discord messaging that handles errors gracefullytests/modules/events/clientReady.test.js (1)
44-64: Good coverage for extractedclientReadyflowThis validates both registration and failure handling paths after modularization, which protects the refactor well.
As per coding guidelines, this follows: "Write bot tests using Vitest 4 with the
nodeenvironment, matching thesrc/structure in thetests/directory".
Comment on lines
+19
to
+28
| if (interaction.isAutocomplete()) { | ||
| const command = client.commands.get(interaction.commandName); | ||
| if (command?.autocomplete) { | ||
| try { | ||
| await command.autocomplete(interaction); | ||
| } catch (err) { | ||
| error('Autocomplete error', { command: interaction.commandName, error: err.message }); | ||
| } | ||
| } | ||
| return; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
discord.js 14 AutocompleteInteraction respond required when autocomplete handler throws or command is missing
💡 Result:
Yes. An AutocompleteInteraction must be acknowledged by calling interaction.respond(...) within ~3 seconds, and you cannot defer autocomplete responses. If you don’t respond (e.g., because the command is missing or your autocomplete handler throws), Discord will show an autocomplete error to the user. [1]
Fix: always “fail closed” by responding with an empty array ([]) if you can’t produce suggestions (missing command, exception, etc.). An empty array shows “No options match your search”. [1]
client.on(Events.InteractionCreate, async (interaction) => {
if (!interaction.isAutocomplete()) return;
const command = interaction.client.commands.get(interaction.commandName);
// Command missing -> still respond
if (!command?.autocomplete) {
await interaction.respond([]); // always acknowledge
return;
}
try {
await command.autocomplete(interaction); // should call interaction.respond([...])
} catch (err) {
console.error(err);
// Handler threw -> still respond (but only if not already responded)
if (!interaction.responded) await interaction.respond([]);
}
});Notes:
AutocompleteInteraction#respond(choices)is the required response method. [2]interaction.respondedlets you avoid double-responding if your handler already responded before failing. [2]
Sources: discord.js Guide autocomplete docs [1], discord.js AutocompleteInteraction API docs (respond, responded) [2].
Always respond to autocomplete interactions, even on failure.
The current code misses three scenarios where it returns without calling interaction.respond(). Discord's API requires acknowledgment of an AutocompleteInteraction within ~3 seconds; without it, the user sees a broken autocomplete error instead of an empty result list. When command?.autocomplete is missing or throws, respond with an empty array [].
Suggested fix
if (interaction.isAutocomplete()) {
const command = client.commands.get(interaction.commandName);
- if (command?.autocomplete) {
- try {
- await command.autocomplete(interaction);
- } catch (err) {
- error('Autocomplete error', { command: interaction.commandName, error: err.message });
- }
+ if (!command?.autocomplete) {
+ await interaction.respond([]).catch(() => {});
+ return;
+ }
+
+ try {
+ await command.autocomplete(interaction);
+ } catch (err) {
+ error('Autocomplete error', { command: interaction.commandName, error: err.message });
+ await interaction.respond([]).catch(() => {});
}
return;
}📝 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.
Suggested change
| if (interaction.isAutocomplete()) { | |
| const command = client.commands.get(interaction.commandName); | |
| if (command?.autocomplete) { | |
| try { | |
| await command.autocomplete(interaction); | |
| } catch (err) { | |
| error('Autocomplete error', { command: interaction.commandName, error: err.message }); | |
| } | |
| } | |
| return; | |
| if (interaction.isAutocomplete()) { | |
| const command = client.commands.get(interaction.commandName); | |
| if (!command?.autocomplete) { | |
| await interaction.respond([]).catch(() => {}); | |
| return; | |
| } | |
| try { | |
| await command.autocomplete(interaction); | |
| } catch (err) { | |
| error('Autocomplete error', { command: interaction.commandName, error: err.message }); | |
| await interaction.respond([]).catch(() => {}); | |
| } | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/events/commandInteraction.js` around lines 19 - 28, The
autocomplete branch may return without acknowledging the
AutocompleteInteraction; update the logic around interaction.isAutocomplete() so
that if a command handler is missing (command?.autocomplete is falsy) or if
command.autocomplete throws, you call interaction.respond([]) before returning;
when command.autocomplete exists, await it inside the try/catch and on catch
call interaction.respond([]) and log the error (use error('Autocomplete error',
{ command: interaction.commandName, error: err.message })), ensuring you only
call interaction.respond once per interaction. Use the existing symbols
interaction.isAutocomplete(), client.commands.get(interaction.commandName),
command.autocomplete, and interaction.respond to locate and modify the code.
Comment on lines
+69
to
+75
| // Log command usage to dedicated analytics table (fire-and-forget) | ||
| logCommandUsage({ | ||
| guildId: interaction.guildId, | ||
| userId: interaction.user.id, | ||
| commandName, | ||
| channelId: interaction.channelId, | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
fd 'commandUsage\.(js|ts)$' . -x sed -n '1,220p' {}
rg -nP '\blogCommandUsage\s*\(' -C2Repository: VolvoxLLC/volvox-bot
Length of output: 5991
Consider adding explicit error handling for the fire-and-forget analytics write.
While logCommandUsage() internally catches errors, the call at line 70 is not awaited or caught. Adding .catch() provides defense-in-depth error logging at the call site and makes the fire-and-forget intent explicit:
Suggested fix
- logCommandUsage({
+ void logCommandUsage({
guildId: interaction.guildId,
userId: interaction.user.id,
commandName,
channelId: interaction.channelId,
- });
+ }).catch((err) => {
+ logError('Failed to record command usage', {
+ command: commandName,
+ error: err.message,
+ });
+ });📝 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.
Suggested change
| // Log command usage to dedicated analytics table (fire-and-forget) | |
| logCommandUsage({ | |
| guildId: interaction.guildId, | |
| userId: interaction.user.id, | |
| commandName, | |
| channelId: interaction.channelId, | |
| }); | |
| // Log command usage to dedicated analytics table (fire-and-forget) | |
| void logCommandUsage({ | |
| guildId: interaction.guildId, | |
| userId: interaction.user.id, | |
| commandName, | |
| channelId: interaction.channelId, | |
| }).catch((err) => { | |
| logger.error('Failed to record command usage', { | |
| command: commandName, | |
| error: err.message, | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/events/commandInteraction.js` around lines 69 - 75, The
fire-and-forget call to logCommandUsage({ guildId: interaction.guildId, userId:
interaction.user.id, commandName, channelId: interaction.channelId }) should
explicitly handle promise rejections; keep it non-awaited but append a
.catch(...) to the call and log the error (using your existing logger or
console.error) so failures are recorded at the call site (reference: function
logCommandUsage and variables interaction and commandName).
Comment on lines
+51
to
+54
| afterEach(() => { | ||
| vi.restoreAllMocks(); | ||
| hasPermission.mockReturnValue(true); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Vitest 4, for mocks created with vi.mock(..., () => ({ fn: vi.fn() })), does vi.restoreAllMocks()reset call history/implementations, or shouldvi.clearAllMocks()/vi.resetAllMocks() be used for per-test isolation?
💡 Result:
vi.restoreAllMocks() is not the right tool for per-test isolation of vi.fn() mocks returned from a vi.mock(..., () => ({ fn: vi.fn() })) factory in Vitest 4.
vi.restoreAllMocks(): only restores original implementations for spies created withvi.spyOnand does not clear call history or reset mock implementations. It also doesn’t affect automocked/module mocks. (main.vitest.dev)- For per-test isolation, use:
vi.clearAllMocks()to clear call history only (keeps implementations). (main.vitest.dev)vi.resetAllMocks()to clear call history + reset implementations. (main.vitest.dev)
So: for vi.fn() in your vi.mock factory, prefer vi.clearAllMocks() / vi.resetAllMocks() (or their config equivalents) rather than vi.restoreAllMocks(). (main.vitest.dev)
Citations:
- 1: https://main.vitest.dev/api/vi
- 2: https://main.vitest.dev/api/vi
- 3: https://main.vitest.dev/api/vi
- 4: https://main.vitest.dev/api/vi
Replace restoreAllMocks() with clearAllMocks() for proper module-mock isolation
vi.restoreAllMocks() only restores original implementations for spies created with vi.spyOn() and does not affect automocked/module mocks created via vi.mock(). Use vi.clearAllMocks() instead to properly clear call history and prevent cross-test state contamination.
🔧 Suggested change
afterEach(() => {
- vi.restoreAllMocks();
+ vi.clearAllMocks();
hasPermission.mockReturnValue(true);
+ getPermissionError.mockReturnValue('Permission denied');
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/modules/events/commandInteraction.test.js` around lines 51 - 54, The
afterEach hook currently calls vi.restoreAllMocks(), which doesn't clear
automocked/module mocks like hasPermission; replace vi.restoreAllMocks() with
vi.clearAllMocks() in the afterEach block to properly reset mock call history
and isolate tests while keeping the hasPermission.mockReturnValue(true)
reestablishment intact.
Comment on lines
361
to
365
| export function getCategoryById(categoryId: ConfigCategoryId): ConfigCategoryMeta { | ||
| const found = CONFIG_CATEGORIES.find((category) => category.id === categoryId); | ||
| if (!found) { | ||
| // biome-ignore lint/suspicious/noConsole: intentional warning for unexpected/missing categoryId | ||
| console.warn(`getCategoryById: unknown categoryId "${categoryId}", falling back to default.`); | ||
| logger.warn(`getCategoryById: unknown categoryId "${categoryId}", falling back to default.`); | ||
| return CONFIG_CATEGORIES[0]; |
Contributor
There was a problem hiding this comment.
Make the fallback match the declared default.
The new warning says this path falls back to the default category, but it still returns CONFIG_CATEGORIES[0]. Those only stay equivalent while the array order never changes.
Suggested fix
export function getCategoryById(categoryId: ConfigCategoryId): ConfigCategoryMeta {
const found = CONFIG_CATEGORIES.find((category) => category.id === categoryId);
if (!found) {
logger.warn(`getCategoryById: unknown categoryId "${categoryId}", falling back to default.`);
- return CONFIG_CATEGORIES[0];
+ return (
+ CONFIG_CATEGORIES.find((category) => category.id === DEFAULT_CONFIG_CATEGORY) ??
+ CONFIG_CATEGORIES[0]
+ );
}
return found;
}📝 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.
Suggested change
| export function getCategoryById(categoryId: ConfigCategoryId): ConfigCategoryMeta { | |
| const found = CONFIG_CATEGORIES.find((category) => category.id === categoryId); | |
| if (!found) { | |
| // biome-ignore lint/suspicious/noConsole: intentional warning for unexpected/missing categoryId | |
| console.warn(`getCategoryById: unknown categoryId "${categoryId}", falling back to default.`); | |
| logger.warn(`getCategoryById: unknown categoryId "${categoryId}", falling back to default.`); | |
| return CONFIG_CATEGORIES[0]; | |
| export function getCategoryById(categoryId: ConfigCategoryId): ConfigCategoryMeta { | |
| const found = CONFIG_CATEGORIES.find((category) => category.id === categoryId); | |
| if (!found) { | |
| logger.warn(`getCategoryById: unknown categoryId "${categoryId}", falling back to default.`); | |
| return ( | |
| CONFIG_CATEGORIES.find((category) => category.id === DEFAULT_CONFIG_CATEGORY) ?? | |
| CONFIG_CATEGORIES[0] | |
| ); | |
| } | |
| return found; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/dashboard/config-workspace/config-categories.ts` around
lines 361 - 365, The warning says we fall back to the declared default but the
code still returns CONFIG_CATEGORIES[0]; change getCategoryById to return the
actual declared default category instead of hardcoding index 0—locate the
project's default identifier/constant (e.g., DEFAULT_CONFIG_CATEGORY,
CONFIG_CATEGORIES_DEFAULT, or DEFAULT_CONFIG_CATEGORY_ID) and use it to find the
matching entry in CONFIG_CATEGORIES (e.g., CONFIG_CATEGORIES.find(c => c.id ===
DEFAULT_...)); if that lookup somehow fails, then fall back to
CONFIG_CATEGORIES[0] as a last resort so behavior matches the logged message.
Comment on lines
+51
to
+53
| toast.error('Something went wrong', { | ||
| description: error.message, | ||
| duration: 5000, |
Contributor
There was a problem hiding this comment.
Avoid exposing raw exception messages in user toasts
On Line 52, description: error.message can leak internal details to end users in production. Keep detailed data in logs and show a generic user-facing message instead.
🔧 Suggested change
componentDidCatch(error: Error, info: ErrorInfo) {
logger.error('[ErrorBoundary] Caught error:', error, info.componentStack);
toast.error('Something went wrong', {
- description: error.message,
+ description:
+ process.env.NODE_ENV === 'development'
+ ? error.message
+ : 'An unexpected error occurred. Please try again.',
duration: 5000,
});
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/ui/error-boundary.tsx` around lines 51 - 53, The toast in
ErrorBoundary currently displays raw exception text (toast.error call in
web/src/components/ui/error-boundary.tsx), which can leak internals; change the
user-facing toast description to a generic message (e.g., "An unexpected error
occurred") and move the detailed error output to logging/monitoring
(console.error or your logger) inside the same error handler so the full
error.message and stack are recorded but not shown to users; update the toast
invocation in the ErrorBoundary/onError handler accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses items from #144 — deferred tech debt collected from PR reviews, code audits, and session notes.
Workstreams
✅ Code Quality
events.jshandlers — Pulled inline handlers (clientReady, commandInteraction) into separate modules undersrc/modules/events/. Main events file is now a clean dispatcher.web/src/lib/logger.tswith proper client-side logging (structured, level-aware)console.errorcleanup — Replaced raw console calls in dashboard components with logger🔄 In Progress (will merge as subagents complete)
Not Included
Tests
Closes #144