fix: AI channel blocklist - rebase on main#192
Conversation
|
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. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR introduces an AI feedback tracking system with channel blocklist enforcement. It adds database infrastructure for recording user reactions to AI messages, API routes for retrieving feedback statistics, event handlers to capture feedback via reactions, and UI components for visualization and configuration management. Changes
Possibly Related PRs
Suggested Reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
Pull request overview
Rebases and carries forward the original feature to add a per-guild AI channel blocklist, preventing the AI assistant from responding (and accumulating triage context) in configured channels and their threads.
Changes:
- Adds
ai.blockedChannelIdsto config types, validation schema, and defaultconfig.json. - Introduces
isChannelBlocked()and enforces the blocklist in the message-create event handler (including thread-parent blocking). - Extends the dashboard config editor with a channel picker for blocked channels and adds/updates unit tests.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/modules/ai.js |
Adds isChannelBlocked() helper to evaluate blocklist membership (including thread parent checks). |
src/modules/events.js |
Applies blocklist gating in the AI message handling path. |
src/api/utils/configValidation.js |
Extends config schema to allow ai.blockedChannelIds. |
web/src/types/config.ts |
Updates TypeScript config types to include blockedChannelIds. |
web/src/components/dashboard/config-editor.tsx |
Adds a “Blocked Channels” UI section using ChannelSelector. |
tests/modules/ai.test.js |
Adds unit tests for isChannelBlocked(). |
tests/modules/events.test.js |
Adds event-handler tests to ensure blocked channels/threads suppress AI behavior. |
config.json |
Adds default ai.blockedChannelIds and updates seed config content formatting/encoding. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
| Filename | Overview |
|---|---|
| src/api/utils/configValidation.js | Added schema validation for blockedChannelIds array |
| src/modules/ai.js | Implemented isChannelBlocked with proper error handling and fail-open behavior |
| src/modules/events.js | Integrated blocklist check in message handler, blocks AI responses in configured channels |
| web/src/components/dashboard/config-editor.tsx | Added UI card with ChannelSelector for configuring blocked channels |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Message Received] --> B{AI Enabled?}
B -->|No| Z[Ignore]
B -->|Yes| C{Mentioned or Reply?}
C -->|No| D[Accumulate for Context]
C -->|Yes| E{In Allowed Channels?}
E -->|No| Z
E -->|Yes| F{Channel Blocked?}
F -->|Yes| Z
F -->|No| G[Accumulate Message]
G --> H[Show Typing Indicator]
H --> I[Evaluate & Respond]
style F fill:#f96,stroke:#333,stroke-width:2px
style Z fill:#ddd
Last reviewed commit: 648caff
| router.get( | ||
| '/stats', | ||
| feedbackRateLimit, | ||
| requireGuildAdmin, |
Check failure
Code scanning / CodeQL
Missing rate limiting High
Copilot Autofix
AI 13 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| router.get( | ||
| '/recent', | ||
| feedbackRateLimit, | ||
| requireGuildAdmin, |
Check failure
Code scanning / CodeQL
Missing rate limiting High
Copilot Autofix
AI 13 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config.json`:
- Around line 13-16: Add documentation for the new AI config keys introduced in
config.json: document ai.blockedChannelIds (array of channel IDs to ignore) and
ai.feedback.enabled (boolean to toggle feedback) in the README.md config
reference section; update the config reference table/section to include key
names, types, default values, and a short description for ai.blockedChannelIds
and ai.feedback.enabled so readers can discover and understand these new
settings.
In `@migrations/003_ai_feedback.cjs`:
- Line 19: The migration defines the feedback timestamp column as created_at
TIMESTAMP NOT NULL DEFAULT NOW(); update the column to be timezone-aware by
changing its type to TIMESTAMPTZ (i.e., created_at TIMESTAMPTZ NOT NULL DEFAULT
NOW()) so feedback event times are stored with timezone information; locate the
created_at definition in migrations/003_ai_feedback.cjs and replace TIMESTAMP
with TIMESTAMPTZ (ensure any downstream code that assumes TIMESTAMP handles
timestamptz values).
- Around line 24-39: Add a composite index on (guild_id, created_at) to speed
guild-scoped time queries: in the migration's exports.up where pgm.sql creates
the two single-column indexes for ai_feedback, add a pgm.sql call to CREATE
INDEX IF NOT EXISTS idx_ai_feedback_guild_id_created_at ON ai_feedback(guild_id,
created_at); and in exports.down add a corresponding DROP INDEX IF EXISTS
idx_ai_feedback_guild_id_created_at; so the migration creates and removes the
composite index alongside the existing idx_ai_feedback_guild_id and
idx_ai_feedback_message_id.
In `@src/api/routes/ai-feedback.js`:
- Around line 103-135: The route handler currently sends responses directly;
instead import and throw the app's custom errors (e.g., ServiceUnavailableError
and InternalServerError from src/utils/errors.js) so the centralized error
middleware enforces the API contract. Replace the manual return
res.status(503)... when getDbPool(req) is falsy with: throw new
ServiceUnavailableError('Database not available') and in the catch block call
logError('Failed to fetch AI feedback stats', { guildId: req.params.id, error:
err.message, err }) then re-throw or throw new InternalServerError('Failed to
fetch AI feedback stats', { cause: err }) so the global error handler formats
the response; apply the same pattern to the other handler referenced (lines
208-233) and remove direct res.status/res.json usage for error cases.
In `@src/api/utils/configValidation.js`:
- Line 20: The schema entry for blockedChannelIds is too permissive and should
only accept an array of channel ID strings; update the config validation schema
(the blockedChannelIds property in src/api/utils/configValidation.js) to enforce
items: { type: 'string' } and add uniqueItems: true (and optionally a pattern or
minLength if you need numeric/string format constraints) so invalid element
types no longer persist and block matching remains reliable.
In `@src/modules/aiFeedback.js`:
- Around line 177-181: The code sets negative = row?.negative || 1 which
incorrectly treats a legitimate zero as one; change the fallback to preserve
zero by using a nullish/coalescing check (e.g., with ??) or explicit undefined
check so negative falls back to 0 (or the intended default) when row?.negative
is undefined, updating the assignment for negative in the block that reads
result.rows[0] (referencing row and negative in src/modules/aiFeedback.js).
In `@src/modules/events.js`:
- Around line 224-228: Move the channel blocklist guard earlier inside the AI
handling branch so it runs before any reply-reference or message-fetch logic;
specifically, compute parentId (using message.channel.isThread?.() and
message.channel.parentId) and call isChannelBlocked(message.channel.id,
parentId, message.guild.id) as the first early-return check in the AI path to
avoid performing any Discord fetches when the channel is blocked.
- Around line 355-366: The current handler calls deleteFeedback unconditionally
when a feedback reaction is removed; change it to only delete when the user has
no remaining feedback reactions on that message. Specifically, in the
reaction-remove branch (where guildConfig.ai?.feedback?.enabled,
isAiMessage(reaction.message.id), FEEDBACK_EMOJI and deleteFeedback are used),
determine the opposite emoji (FEEDBACK_EMOJI.positive vs .negative) and check
the message's reactions/users to see if the user still has the opposite reaction
(use reaction.message.reactions.cache or fetch the reaction users and test for
user.id); only call deleteFeedback({ messageId: reaction.message.id, userId:
user.id }) when the user does not have either feedback reaction remaining.
In `@tests/api/routes/ai-feedback.test.js`:
- Around line 176-194: The mocked DB rows in the test for the
/api/v1/guilds/:id/ai-feedback/recent route are using snake_case keys, but
getRecentFeedback (and the route handler) returns camelCase fields; update the
fakeRows fixture in tests/api/routes/ai-feedback.test.js to use camelCase keys
(e.g., messageId, channelId, userId, feedbackType, createdAt) so the mocked
query shape matches the module's output and the existing assertions (messageId,
feedbackType) remain valid.
In `@tests/modules/ai.test.js`:
- Around line 210-257: Add a test that exercises isChannelBlocked with a guildId
to verify guild-scoped configs: mock getConfig to return { ai: {
blockedChannelIds: [...] } } when called with that guildId, call
isChannelBlocked(channelId, parentId, 'guild-123'), assert the returned boolean
matches expectation (true/false based on blockedChannelIds), and assert
getConfig was called with 'guild-123' (e.g.,
expect(getConfig).toHaveBeenCalledWith('guild-123')). Ensure the test references
isChannelBlocked, getConfig, and blockedChannelIds so guild-specific fallback
regressions are detected.
In `@tests/modules/aiFeedback.test.js`:
- Around line 59-178: Add unit tests for deleteFeedback mirroring other suites:
(1) "does nothing when no pool is configured" — call deleteFeedback({ messageId,
channelId, guildId, userId }) without setting a pool and assert mockPool.query
was not called; (2) "deletes feedback via pool query" — setPool(mockPool) with
mockPool.query.mockResolvedValue({ rows: [] }), call deleteFeedback(...) and
assert mockPool.query was called with a SQL string containing 'DELETE FROM
ai_feedback' and the correct params [messageId, channelId, guildId, userId]; (3)
"handles DB errors gracefully without throwing" — setPool(mockPool) with
mockPool.query.mockRejectedValue(new Error('DB error')), call
deleteFeedback(...) and assert the promise resolves to undefined (does not
throw). Optionally add a test using _setPoolGetter(() => altPool) to validate DI
like other tests.
In `@web/src/components/dashboard/ai-feedback-stats.tsx`:
- Around line 45-71: fetchStats can suffer from stale responses overwriting
state when selectedGuild changes; modify fetchStats/useEffect to cancel or
ignore out-of-date requests by using an AbortController or a per-request id.
Specifically, in the effect that calls fetchStats, create an AbortController (or
increment a local requestId counter) and pass its signal (or id) into
fetchStats; inside fetchStats check the signal.aborted (or compare requestId
before calling setStats/setLoading/setError) and skip state updates if
cancelled/mismatched; also ensure fetch is called with the controller.signal and
that you call controller.abort() (or discard the id) in the effect cleanup so
only the latest guild’s response can update stats.
In `@web/src/components/dashboard/config-editor.tsx`:
- Around line 771-779: Replace the direct-fetch ChannelSelector usage with the
dashboard's Zustand-backed channel source: read the cached channels for guildId
from the discord-entities store (e.g., useDiscordEntitiesStore or
discordEntitiesStore selector that returns channels for a guild) and pass that
channel list into ChannelSelector instead of allowing it to perform its own
fetch; keep the same props (id "ai-blocked-channels",
selected={(draftConfig.ai?.blockedChannelIds ?? []) as string[]},
onChange={updateAiBlockedChannels}, placeholder, disabled={saving},
filter="text") but supply the store-backed channels via the component’s
channels/options prop (or a source prop used by our other selectors) so
ChannelSelector uses the cached Zustand data for guildId.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
config.jsonmigrations/003_ai_feedback.cjssrc/api/index.jssrc/api/routes/ai-feedback.jssrc/api/utils/configValidation.jssrc/modules/ai.jssrc/modules/aiFeedback.jssrc/modules/events.jssrc/modules/triage-respond.jstests/api/routes/ai-feedback.test.jstests/modules/ai.test.jstests/modules/aiFeedback.test.jstests/modules/events.test.jsweb/src/components/dashboard/ai-feedback-stats.tsxweb/src/components/dashboard/config-editor.tsxweb/src/types/config.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 (9)
**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
**/*.js: Use ESM modules — useimport/export, neverrequire()
Always usenode:protocol for Node.js builtins (e.g.import { readFileSync } from 'node:fs')
Always use semicolons in JavaScript code
Use single quotes for strings (enforced by Biome)
Use 2-space indentation (enforced by Biome)
Files:
src/api/utils/configValidation.jstests/modules/events.test.jssrc/api/routes/ai-feedback.jstests/modules/aiFeedback.test.jssrc/modules/triage-respond.jssrc/modules/ai.jstests/modules/ai.test.jssrc/modules/events.jssrc/modules/aiFeedback.jssrc/api/index.jstests/api/routes/ai-feedback.test.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: Always use Winston logger —import { info, warn, error } from '../logger.js'. NEVER useconsole.log,console.warn,console.error, or anyconsole.*method in src/ files — replace any existing console calls with Winston equivalents
Pass structured metadata to Winston logs:info('Message processed', { userId, channelId })
Files:
src/api/utils/configValidation.jssrc/api/routes/ai-feedback.jssrc/modules/triage-respond.jssrc/modules/ai.jssrc/modules/events.jssrc/modules/aiFeedback.jssrc/api/index.js
tests/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
All new code must include tests. Test coverage must maintain an 80% threshold on statements, branches, functions, and lines. Run
pnpm testbefore every commit
Files:
tests/modules/events.test.jstests/modules/aiFeedback.test.jstests/modules/ai.test.jstests/api/routes/ai-feedback.test.js
src/api/routes/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Always use custom error classes from
src/utils/errors.jsand log errors with context before re-throwing
Files:
src/api/routes/ai-feedback.js
web/src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript with type safety. Share contracts between dashboard UI and API responses via
web/src/types/analytics.tsand similar type definition files
Files:
web/src/types/config.ts
src/modules/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Per-request modules (AI, spam, moderation) should call
getConfig(guildId)on every invocation for automatic config changes. Stateful resources should useonConfigChangelisteners for reactive updates
Files:
src/modules/triage-respond.jssrc/modules/ai.jssrc/modules/events.jssrc/modules/aiFeedback.js
web/src/components/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Component files should integrate with Zustand stores for state management (e.g., discord-entities store for caching Discord channels and roles per guild)
Files:
web/src/components/dashboard/ai-feedback-stats.tsxweb/src/components/dashboard/config-editor.tsx
migrations/*.cjs
📄 CodeRabbit inference engine (AGENTS.md)
Database migrations must use CommonJS (.cjs) format and follow naming convention
NNN_description.cjswhere NNN is a zero-padded sequence number
Files:
migrations/003_ai_feedback.cjs
config.json
📄 CodeRabbit inference engine (AGENTS.md)
When adding a new config section or key, document it in README.md's config reference section
Files:
config.json
🧠 Learnings (2)
📚 Learning: 2026-03-01T06:03:34.399Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-01T06:03:34.399Z
Learning: Applies to tests/**/*.js : All new code must include tests. Test coverage must maintain an 80% threshold on statements, branches, functions, and lines. Run `pnpm test` before every commit
Applied to files:
tests/modules/aiFeedback.test.jstests/api/routes/ai-feedback.test.js
📚 Learning: 2026-03-01T06:03:34.399Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-01T06:03:34.399Z
Learning: Applies to src/modules/*.js : Per-request modules (AI, spam, moderation) should call `getConfig(guildId)` on every invocation for automatic config changes. Stateful resources should use `onConfigChange` listeners for reactive updates
Applied to files:
src/modules/ai.jstests/modules/ai.test.jssrc/modules/events.jssrc/modules/aiFeedback.js
🧬 Code graph analysis (11)
tests/modules/events.test.js (1)
src/modules/triage.js (4)
evaluateNow(648-714)accumulateMessage(565-635)config(60-60)client(51-51)
src/api/routes/ai-feedback.js (1)
src/modules/aiFeedback.js (5)
pool(101-101)pool(135-135)getFeedbackStats(162-188)getFeedbackTrend(196-223)getRecentFeedback(231-256)
tests/modules/aiFeedback.test.js (1)
src/modules/aiFeedback.js (8)
clearAiMessages(54-56)setPool(78-80)_setPoolGetter(67-69)registerAiMessage(26-40)isAiMessage(47-49)recordFeedback(100-125)getFeedbackStats(162-188)getFeedbackTrend(196-223)
src/modules/triage-respond.js (1)
src/modules/aiFeedback.js (4)
first(36-36)registerAiMessage(26-40)FEEDBACK_EMOJI(10-13)FEEDBACK_EMOJI(10-13)
src/modules/ai.js (1)
src/modules/config.js (1)
getConfig(282-313)
tests/modules/ai.test.js (2)
src/modules/config.js (1)
getConfig(282-313)src/modules/ai.js (1)
isChannelBlocked(40-52)
src/modules/events.js (2)
src/modules/ai.js (1)
isChannelBlocked(40-52)src/modules/aiFeedback.js (4)
FEEDBACK_EMOJI(10-13)FEEDBACK_EMOJI(10-13)recordFeedback(100-125)deleteFeedback(134-155)
src/modules/aiFeedback.js (4)
src/api/routes/ai-feedback.js (2)
pool(105-105)pool(210-210)src/db.js (1)
getPool(314-319)src/logger.js (2)
warn(238-240)info(231-233)src/modules/config.js (1)
err(94-94)
src/api/index.js (3)
src/api/routes/ai-feedback.js (1)
router(14-14)src/api/middleware/auth.js (1)
requireAuth(36-70)src/api/middleware/auditLog.js (1)
auditLogMiddleware(139-243)
web/src/components/dashboard/config-editor.tsx (2)
web/src/lib/config-utils.ts (1)
GuildConfig(4-4)web/src/components/ui/channel-selector.tsx (1)
ChannelSelector(168-413)
tests/api/routes/ai-feedback.test.js (1)
src/api/server.js (1)
createApp(28-91)
🪛 GitHub Check: CodeQL
src/api/routes/ai-feedback.js
[failure] 101-101: Missing rate limiting
This route handler performs authorization, but is not rate-limited.
[failure] 206-206: Missing rate limiting
This route handler performs authorization, but is not rate-limited.
src/api/index.js
[failure] 41-41: Missing rate limiting
This route handler performs authorization, but is not rate-limited.
This route handler performs authorization, but is not rate-limited.
🔇 Additional comments (6)
web/src/types/config.ts (1)
13-13:AiConfigcontract update looks correct.The new
blockedChannelIdsfield is correctly typed and aligned with the backend blocklist feature.tests/modules/events.test.js (1)
358-439: Good blocklist coverage in event-flow tests.These cases validate blocked channels, blocked thread parents, and allowed-channel behavior with blocklist configured.
src/modules/triage-respond.js (1)
19-47: Feedback reaction integration is clean and non-blocking.Good separation: feature-gated reactions, AI message registration, and first-chunk-only behavior avoids reaction spam.
Also applies to: 299-300
src/modules/ai.js (1)
40-52:isChannelBlockedimplementation matches the intended contract.Channel + parent-channel checks and per-guild config resolution are implemented correctly.
src/api/index.js (1)
41-41: Rate limiting is already in place for the AI feedback route.The endpoint is protected by both a global rate limiter applied to
/api/v1(src/api/server.js line 66) and a route-specific rate limiter of 60 requests per minute per IP (src/api/routes/ai-feedback.js line 16). No action needed.Likely an incorrect or invalid review comment.
web/src/components/dashboard/config-editor.tsx (1)
417-425:updateAiBlockedChannelsis clean and consistent.The immutable update shape matches the rest of the editor and correctly scopes the change to
ai.blockedChannelIds.
| "blockedChannelIds": [], | ||
| "feedback": { | ||
| "enabled": false | ||
| } |
There was a problem hiding this comment.
Document new AI config keys in the config reference.
ai.blockedChannelIds and ai.feedback.enabled are new config keys; please ensure README config reference is updated in this PR.
As per coding guidelines, "config.json: When adding a new config section or key, document it in README.md's config reference section".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config.json` around lines 13 - 16, Add documentation for the new AI config
keys introduced in config.json: document ai.blockedChannelIds (array of channel
IDs to ignore) and ai.feedback.enabled (boolean to toggle feedback) in the
README.md config reference section; update the config reference table/section to
include key names, types, default values, and a short description for
ai.blockedChannelIds and ai.feedback.enabled so readers can discover and
understand these new settings.
| guild_id TEXT NOT NULL, | ||
| user_id TEXT NOT NULL, | ||
| feedback_type TEXT NOT NULL CHECK (feedback_type IN ('positive', 'negative')), | ||
| created_at TIMESTAMP NOT NULL DEFAULT NOW(), |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use TIMESTAMPTZ for feedback event time.
created_at should be timezone-aware to keep trend/recent analytics consistent across environments.
Suggested migration tweak
- created_at TIMESTAMP NOT NULL DEFAULT NOW(),
+ created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(),📝 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.
| created_at TIMESTAMP NOT NULL DEFAULT NOW(), | |
| created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@migrations/003_ai_feedback.cjs` at line 19, The migration defines the
feedback timestamp column as created_at TIMESTAMP NOT NULL DEFAULT NOW(); update
the column to be timezone-aware by changing its type to TIMESTAMPTZ (i.e.,
created_at TIMESTAMPTZ NOT NULL DEFAULT NOW()) so feedback event times are
stored with timezone information; locate the created_at definition in
migrations/003_ai_feedback.cjs and replace TIMESTAMP with TIMESTAMPTZ (ensure
any downstream code that assumes TIMESTAMP handles timestamptz values).
| pgm.sql(` | ||
| CREATE INDEX IF NOT EXISTS idx_ai_feedback_guild_id | ||
| ON ai_feedback(guild_id) | ||
| `); | ||
|
|
||
| pgm.sql(` | ||
| CREATE INDEX IF NOT EXISTS idx_ai_feedback_message_id | ||
| ON ai_feedback(message_id) | ||
| `); | ||
| }; | ||
|
|
||
| /** @param {import('node-pg-migrate').MigrationBuilder} pgm */ | ||
| exports.down = (pgm) => { | ||
| pgm.sql(`DROP INDEX IF EXISTS idx_ai_feedback_message_id`); | ||
| pgm.sql(`DROP INDEX IF EXISTS idx_ai_feedback_guild_id`); | ||
| pgm.sql(`DROP TABLE IF EXISTS ai_feedback`); |
There was a problem hiding this comment.
Add a composite index for guild-scoped time queries.
For feedback stats/trend/recent workloads, missing (guild_id, created_at) will force expensive sorts/scans as data grows.
Suggested index additions
pgm.sql(`
CREATE INDEX IF NOT EXISTS idx_ai_feedback_message_id
ON ai_feedback(message_id)
`);
+
+ pgm.sql(`
+ CREATE INDEX IF NOT EXISTS idx_ai_feedback_guild_created_at
+ ON ai_feedback(guild_id, created_at DESC)
+ `);
};
/** `@param` {import('node-pg-migrate').MigrationBuilder} pgm */
exports.down = (pgm) => {
+ pgm.sql(`DROP INDEX IF EXISTS idx_ai_feedback_guild_created_at`);
pgm.sql(`DROP INDEX IF EXISTS idx_ai_feedback_message_id`);
pgm.sql(`DROP INDEX IF EXISTS idx_ai_feedback_guild_id`);
pgm.sql(`DROP TABLE IF EXISTS ai_feedback`);
};📝 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.
| pgm.sql(` | |
| CREATE INDEX IF NOT EXISTS idx_ai_feedback_guild_id | |
| ON ai_feedback(guild_id) | |
| `); | |
| pgm.sql(` | |
| CREATE INDEX IF NOT EXISTS idx_ai_feedback_message_id | |
| ON ai_feedback(message_id) | |
| `); | |
| }; | |
| /** @param {import('node-pg-migrate').MigrationBuilder} pgm */ | |
| exports.down = (pgm) => { | |
| pgm.sql(`DROP INDEX IF EXISTS idx_ai_feedback_message_id`); | |
| pgm.sql(`DROP INDEX IF EXISTS idx_ai_feedback_guild_id`); | |
| pgm.sql(`DROP TABLE IF EXISTS ai_feedback`); | |
| pgm.sql(` | |
| CREATE INDEX IF NOT EXISTS idx_ai_feedback_guild_id | |
| ON ai_feedback(guild_id) | |
| `); | |
| pgm.sql(` | |
| CREATE INDEX IF NOT EXISTS idx_ai_feedback_message_id | |
| ON ai_feedback(message_id) | |
| `); | |
| pgm.sql(` | |
| CREATE INDEX IF NOT EXISTS idx_ai_feedback_guild_created_at | |
| ON ai_feedback(guild_id, created_at DESC) | |
| `); | |
| }; | |
| /** `@param` {import('node-pg-migrate').MigrationBuilder} pgm */ | |
| exports.down = (pgm) => { | |
| pgm.sql(`DROP INDEX IF EXISTS idx_ai_feedback_guild_created_at`); | |
| pgm.sql(`DROP INDEX IF EXISTS idx_ai_feedback_message_id`); | |
| pgm.sql(`DROP INDEX IF EXISTS idx_ai_feedback_guild_id`); | |
| pgm.sql(`DROP TABLE IF EXISTS ai_feedback`); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@migrations/003_ai_feedback.cjs` around lines 24 - 39, Add a composite index
on (guild_id, created_at) to speed guild-scoped time queries: in the migration's
exports.up where pgm.sql creates the two single-column indexes for ai_feedback,
add a pgm.sql call to CREATE INDEX IF NOT EXISTS
idx_ai_feedback_guild_id_created_at ON ai_feedback(guild_id, created_at); and in
exports.down add a corresponding DROP INDEX IF EXISTS
idx_ai_feedback_guild_id_created_at; so the migration creates and removes the
composite index alongside the existing idx_ai_feedback_guild_id and
idx_ai_feedback_message_id.
| async (req, res, next) => { | ||
| try { | ||
| const pool = getDbPool(req); | ||
| if (!pool) { | ||
| return res.status(503).json({ error: 'Database not available' }); | ||
| } | ||
|
|
||
| const guildId = req.params.id; | ||
|
|
||
| let days = 30; | ||
| if (req.query.days !== undefined) { | ||
| const parsed = Number.parseInt(req.query.days, 10); | ||
| if (!Number.isNaN(parsed) && parsed >= 1 && parsed <= 90) { | ||
| days = parsed; | ||
| } | ||
| } | ||
|
|
||
| const [stats, trend] = await Promise.all([ | ||
| getFeedbackStats(guildId, pool), | ||
| getFeedbackTrend(guildId, days, pool), | ||
| ]); | ||
|
|
||
| res.json({ | ||
| ...stats, | ||
| trend, | ||
| }); | ||
| } catch (err) { | ||
| logError('Failed to fetch AI feedback stats', { | ||
| guildId: req.params.id, | ||
| error: err.message, | ||
| }); | ||
| res.status(500).json({ error: 'Failed to fetch AI feedback stats' }); | ||
| } |
There was a problem hiding this comment.
Route handlers bypass required custom error classes.
Both handlers manually send 503/500 responses instead of throwing/passing typed errors from src/utils/errors.js. That bypasses the shared API error contract and duplicates response logic in-route.
As per coding guidelines, "src/api/routes/*.js: Always use custom error classes from src/utils/errors.js and log errors with context before re-throwing."
Also applies to: 208-233
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/routes/ai-feedback.js` around lines 103 - 135, The route handler
currently sends responses directly; instead import and throw the app's custom
errors (e.g., ServiceUnavailableError and InternalServerError from
src/utils/errors.js) so the centralized error middleware enforces the API
contract. Replace the manual return res.status(503)... when getDbPool(req) is
falsy with: throw new ServiceUnavailableError('Database not available') and in
the catch block call logError('Failed to fetch AI feedback stats', { guildId:
req.params.id, error: err.message, err }) then re-throw or throw new
InternalServerError('Failed to fetch AI feedback stats', { cause: err }) so the
global error handler formats the response; apply the same pattern to the other
handler referenced (lines 208-233) and remove direct res.status/res.json usage
for error cases.
| enabled: { type: 'boolean' }, | ||
| systemPrompt: { type: 'string' }, | ||
| channels: { type: 'array' }, | ||
| blockedChannelIds: { type: 'array' }, |
There was a problem hiding this comment.
blockedChannelIds validation is too loose.
Allowing any array element type can persist invalid config and silently break block matching.
Suggested schema fix
- blockedChannelIds: { type: 'array' },
+ blockedChannelIds: { type: 'array', items: { type: '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.
| blockedChannelIds: { type: 'array' }, | |
| blockedChannelIds: { type: 'array', items: { type: 'string' } }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/utils/configValidation.js` at line 20, The schema entry for
blockedChannelIds is too permissive and should only accept an array of channel
ID strings; update the config validation schema (the blockedChannelIds property
in src/api/utils/configValidation.js) to enforce items: { type: 'string' } and
add uniqueItems: true (and optionally a pattern or minLength if you need
numeric/string format constraints) so invalid element types no longer persist
and block matching remains reliable.
| const fakeRows = [ | ||
| { | ||
| id: 1, | ||
| message_id: 'msg-1', | ||
| channel_id: 'ch-1', | ||
| user_id: 'u-1', | ||
| feedback_type: 'positive', | ||
| created_at: '2026-03-01T12:00:00Z', | ||
| }, | ||
| ]; | ||
| mockPool.query.mockResolvedValueOnce({ rows: fakeRows }); | ||
|
|
||
| const res = await authed(request(app).get(`/api/v1/guilds/${GUILD_ID}/ai-feedback/recent`)); | ||
|
|
||
| expect(res.status).toBe(200); | ||
| expect(res.body.feedback).toHaveLength(1); | ||
| expect(res.body.feedback[0].messageId).toBe('msg-1'); | ||
| expect(res.body.feedback[0].feedbackType).toBe('positive'); | ||
| }); |
There was a problem hiding this comment.
/recent mock row schema is inconsistent with route/module contract.
getRecentFeedback aliases DB fields to camelCase. This fixture uses snake_case keys, but the assertion expects camelCase (messageId, feedbackType). Use camelCase in mocked rows to mirror real query output.
🛠️ Suggested fix
const fakeRows = [
{
id: 1,
- message_id: 'msg-1',
- channel_id: 'ch-1',
- user_id: 'u-1',
- feedback_type: 'positive',
- created_at: '2026-03-01T12:00:00Z',
+ messageId: 'msg-1',
+ channelId: 'ch-1',
+ userId: 'u-1',
+ feedbackType: 'positive',
+ createdAt: '2026-03-01T12:00:00Z',
},
];📝 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 fakeRows = [ | |
| { | |
| id: 1, | |
| message_id: 'msg-1', | |
| channel_id: 'ch-1', | |
| user_id: 'u-1', | |
| feedback_type: 'positive', | |
| created_at: '2026-03-01T12:00:00Z', | |
| }, | |
| ]; | |
| mockPool.query.mockResolvedValueOnce({ rows: fakeRows }); | |
| const res = await authed(request(app).get(`/api/v1/guilds/${GUILD_ID}/ai-feedback/recent`)); | |
| expect(res.status).toBe(200); | |
| expect(res.body.feedback).toHaveLength(1); | |
| expect(res.body.feedback[0].messageId).toBe('msg-1'); | |
| expect(res.body.feedback[0].feedbackType).toBe('positive'); | |
| }); | |
| const fakeRows = [ | |
| { | |
| id: 1, | |
| messageId: 'msg-1', | |
| channelId: 'ch-1', | |
| userId: 'u-1', | |
| feedbackType: 'positive', | |
| createdAt: '2026-03-01T12:00:00Z', | |
| }, | |
| ]; | |
| mockPool.query.mockResolvedValueOnce({ rows: fakeRows }); | |
| const res = await authed(request(app).get(`/api/v1/guilds/${GUILD_ID}/ai-feedback/recent`)); | |
| expect(res.status).toBe(200); | |
| expect(res.body.feedback).toHaveLength(1); | |
| expect(res.body.feedback[0].messageId).toBe('msg-1'); | |
| expect(res.body.feedback[0].feedbackType).toBe('positive'); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/api/routes/ai-feedback.test.js` around lines 176 - 194, The mocked DB
rows in the test for the /api/v1/guilds/:id/ai-feedback/recent route are using
snake_case keys, but getRecentFeedback (and the route handler) returns camelCase
fields; update the fakeRows fixture in tests/api/routes/ai-feedback.test.js to
use camelCase keys (e.g., messageId, channelId, userId, feedbackType, createdAt)
so the mocked query shape matches the module's output and the existing
assertions (messageId, feedbackType) remain valid.
| describe('isChannelBlocked', () => { | ||
| it('should return false when blockedChannelIds is not set', () => { | ||
| getConfig.mockReturnValue({ ai: {} }); | ||
| expect(isChannelBlocked('ch1')).toBe(false); | ||
| }); | ||
|
|
||
| it('should return false when blockedChannelIds is empty', () => { | ||
| getConfig.mockReturnValue({ ai: { blockedChannelIds: [] } }); | ||
| expect(isChannelBlocked('ch1')).toBe(false); | ||
| }); | ||
|
|
||
| it('should return true when channelId is in blockedChannelIds', () => { | ||
| getConfig.mockReturnValue({ ai: { blockedChannelIds: ['ch1', 'ch2'] } }); | ||
| expect(isChannelBlocked('ch1')).toBe(true); | ||
| expect(isChannelBlocked('ch2')).toBe(true); | ||
| }); | ||
|
|
||
| it('should return false when channelId is not in blockedChannelIds', () => { | ||
| getConfig.mockReturnValue({ ai: { blockedChannelIds: ['ch1'] } }); | ||
| expect(isChannelBlocked('ch3')).toBe(false); | ||
| }); | ||
|
|
||
| it('should return true when parentId is in blockedChannelIds (thread in blocked parent)', () => { | ||
| getConfig.mockReturnValue({ ai: { blockedChannelIds: ['parent-ch'] } }); | ||
| expect(isChannelBlocked('thread-ch', 'parent-ch')).toBe(true); | ||
| }); | ||
|
|
||
| it('should return true when channelId matches even if parentId is not blocked', () => { | ||
| getConfig.mockReturnValue({ ai: { blockedChannelIds: ['thread-ch'] } }); | ||
| expect(isChannelBlocked('thread-ch', 'parent-ch')).toBe(true); | ||
| }); | ||
|
|
||
| it('should return false when neither channelId nor parentId is in blockedChannelIds', () => { | ||
| getConfig.mockReturnValue({ ai: { blockedChannelIds: ['other-ch'] } }); | ||
| expect(isChannelBlocked('thread-ch', 'parent-ch')).toBe(false); | ||
| }); | ||
|
|
||
| it('should return false when parentId is null and channelId is not blocked', () => { | ||
| getConfig.mockReturnValue({ ai: { blockedChannelIds: ['ch1'] } }); | ||
| expect(isChannelBlocked('ch2', null)).toBe(false); | ||
| }); | ||
|
|
||
| it('should fail open (return false) when getConfig throws', () => { | ||
| getConfig.mockImplementation(() => { | ||
| throw new Error('Config not loaded'); | ||
| }); | ||
| expect(isChannelBlocked('ch1')).toBe(false); | ||
| }); |
There was a problem hiding this comment.
Add guild-scoped assertions for isChannelBlocked.
These cases validate matching logic, but they don’t verify guild scoping. Since the helper now accepts guildId, please add at least one case that passes a guild ID and asserts getConfig is called with it, so global-fallback regressions are caught.
✅ Suggested test addition
+ it('uses guild-scoped config lookup', () => {
+ getConfig.mockReturnValue({ ai: { blockedChannelIds: ['ch1'] } });
+ expect(isChannelBlocked('ch1', null, 'guild-1')).toBe(true);
+ expect(getConfig).toHaveBeenCalledWith('guild-1');
+ });📝 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.
| describe('isChannelBlocked', () => { | |
| it('should return false when blockedChannelIds is not set', () => { | |
| getConfig.mockReturnValue({ ai: {} }); | |
| expect(isChannelBlocked('ch1')).toBe(false); | |
| }); | |
| it('should return false when blockedChannelIds is empty', () => { | |
| getConfig.mockReturnValue({ ai: { blockedChannelIds: [] } }); | |
| expect(isChannelBlocked('ch1')).toBe(false); | |
| }); | |
| it('should return true when channelId is in blockedChannelIds', () => { | |
| getConfig.mockReturnValue({ ai: { blockedChannelIds: ['ch1', 'ch2'] } }); | |
| expect(isChannelBlocked('ch1')).toBe(true); | |
| expect(isChannelBlocked('ch2')).toBe(true); | |
| }); | |
| it('should return false when channelId is not in blockedChannelIds', () => { | |
| getConfig.mockReturnValue({ ai: { blockedChannelIds: ['ch1'] } }); | |
| expect(isChannelBlocked('ch3')).toBe(false); | |
| }); | |
| it('should return true when parentId is in blockedChannelIds (thread in blocked parent)', () => { | |
| getConfig.mockReturnValue({ ai: { blockedChannelIds: ['parent-ch'] } }); | |
| expect(isChannelBlocked('thread-ch', 'parent-ch')).toBe(true); | |
| }); | |
| it('should return true when channelId matches even if parentId is not blocked', () => { | |
| getConfig.mockReturnValue({ ai: { blockedChannelIds: ['thread-ch'] } }); | |
| expect(isChannelBlocked('thread-ch', 'parent-ch')).toBe(true); | |
| }); | |
| it('should return false when neither channelId nor parentId is in blockedChannelIds', () => { | |
| getConfig.mockReturnValue({ ai: { blockedChannelIds: ['other-ch'] } }); | |
| expect(isChannelBlocked('thread-ch', 'parent-ch')).toBe(false); | |
| }); | |
| it('should return false when parentId is null and channelId is not blocked', () => { | |
| getConfig.mockReturnValue({ ai: { blockedChannelIds: ['ch1'] } }); | |
| expect(isChannelBlocked('ch2', null)).toBe(false); | |
| }); | |
| it('should fail open (return false) when getConfig throws', () => { | |
| getConfig.mockImplementation(() => { | |
| throw new Error('Config not loaded'); | |
| }); | |
| expect(isChannelBlocked('ch1')).toBe(false); | |
| }); | |
| describe('isChannelBlocked', () => { | |
| it('should return false when blockedChannelIds is not set', () => { | |
| getConfig.mockReturnValue({ ai: {} }); | |
| expect(isChannelBlocked('ch1')).toBe(false); | |
| }); | |
| it('should return false when blockedChannelIds is empty', () => { | |
| getConfig.mockReturnValue({ ai: { blockedChannelIds: [] } }); | |
| expect(isChannelBlocked('ch1')).toBe(false); | |
| }); | |
| it('should return true when channelId is in blockedChannelIds', () => { | |
| getConfig.mockReturnValue({ ai: { blockedChannelIds: ['ch1', 'ch2'] } }); | |
| expect(isChannelBlocked('ch1')).toBe(true); | |
| expect(isChannelBlocked('ch2')).toBe(true); | |
| }); | |
| it('should return false when channelId is not in blockedChannelIds', () => { | |
| getConfig.mockReturnValue({ ai: { blockedChannelIds: ['ch1'] } }); | |
| expect(isChannelBlocked('ch3')).toBe(false); | |
| }); | |
| it('should return true when parentId is in blockedChannelIds (thread in blocked parent)', () => { | |
| getConfig.mockReturnValue({ ai: { blockedChannelIds: ['parent-ch'] } }); | |
| expect(isChannelBlocked('thread-ch', 'parent-ch')).toBe(true); | |
| }); | |
| it('should return true when channelId matches even if parentId is not blocked', () => { | |
| getConfig.mockReturnValue({ ai: { blockedChannelIds: ['thread-ch'] } }); | |
| expect(isChannelBlocked('thread-ch', 'parent-ch')).toBe(true); | |
| }); | |
| it('should return false when neither channelId nor parentId is in blockedChannelIds', () => { | |
| getConfig.mockReturnValue({ ai: { blockedChannelIds: ['other-ch'] } }); | |
| expect(isChannelBlocked('thread-ch', 'parent-ch')).toBe(false); | |
| }); | |
| it('should return false when parentId is null and channelId is not blocked', () => { | |
| getConfig.mockReturnValue({ ai: { blockedChannelIds: ['ch1'] } }); | |
| expect(isChannelBlocked('ch2', null)).toBe(false); | |
| }); | |
| it('should fail open (return false) when getConfig throws', () => { | |
| getConfig.mockImplementation(() => { | |
| throw new Error('Config not loaded'); | |
| }); | |
| expect(isChannelBlocked('ch1')).toBe(false); | |
| }); | |
| it('uses guild-scoped config lookup', () => { | |
| getConfig.mockReturnValue({ ai: { blockedChannelIds: ['ch1'] } }); | |
| expect(isChannelBlocked('ch1', null, 'guild-1')).toBe(true); | |
| expect(getConfig).toHaveBeenCalledWith('guild-1'); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/modules/ai.test.js` around lines 210 - 257, Add a test that exercises
isChannelBlocked with a guildId to verify guild-scoped configs: mock getConfig
to return { ai: { blockedChannelIds: [...] } } when called with that guildId,
call isChannelBlocked(channelId, parentId, 'guild-123'), assert the returned
boolean matches expectation (true/false based on blockedChannelIds), and assert
getConfig was called with 'guild-123' (e.g.,
expect(getConfig).toHaveBeenCalledWith('guild-123')). Ensure the test references
isChannelBlocked, getConfig, and blockedChannelIds so guild-specific fallback
regressions are detected.
| describe('recordFeedback', () => { | ||
| it('does nothing when no pool is configured', async () => { | ||
| await recordFeedback({ | ||
| messageId: 'm1', | ||
| channelId: 'c1', | ||
| guildId: 'g1', | ||
| userId: 'u1', | ||
| feedbackType: 'positive', | ||
| }); | ||
| expect(mockPool.query).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('inserts feedback via pool query', async () => { | ||
| mockPool.query.mockResolvedValue({ rows: [] }); | ||
| setPool(mockPool); | ||
|
|
||
| await recordFeedback({ | ||
| messageId: 'm1', | ||
| channelId: 'c1', | ||
| guildId: 'g1', | ||
| userId: 'u1', | ||
| feedbackType: 'positive', | ||
| }); | ||
|
|
||
| expect(mockPool.query).toHaveBeenCalledWith( | ||
| expect.stringContaining('INSERT INTO ai_feedback'), | ||
| ['m1', 'c1', 'g1', 'u1', 'positive'], | ||
| ); | ||
| }); | ||
|
|
||
| it('handles DB errors gracefully without throwing', async () => { | ||
| mockPool.query.mockRejectedValue(new Error('DB error')); | ||
| setPool(mockPool); | ||
|
|
||
| await expect( | ||
| recordFeedback({ | ||
| messageId: 'm1', | ||
| channelId: 'c1', | ||
| guildId: 'g1', | ||
| userId: 'u1', | ||
| feedbackType: 'positive', | ||
| }), | ||
| ).resolves.toBeUndefined(); | ||
| }); | ||
|
|
||
| it('uses _setPoolGetter for DI', async () => { | ||
| const altPool = { query: vi.fn().mockResolvedValue({ rows: [] }) }; | ||
| _setPoolGetter(() => altPool); | ||
|
|
||
| await recordFeedback({ | ||
| messageId: 'm2', | ||
| channelId: 'c2', | ||
| guildId: 'g2', | ||
| userId: 'u2', | ||
| feedbackType: 'negative', | ||
| }); | ||
|
|
||
| expect(altPool.query).toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
|
|
||
| // ── getFeedbackStats ──────────────────────────────────────────────────────── | ||
|
|
||
| describe('getFeedbackStats', () => { | ||
| it('returns zeros when no pool', async () => { | ||
| const stats = await getFeedbackStats('g1'); | ||
| expect(stats).toEqual({ positive: 0, negative: 0, total: 0, ratio: null }); | ||
| }); | ||
|
|
||
| it('returns aggregated stats from DB', async () => { | ||
| mockPool.query.mockResolvedValue({ | ||
| rows: [{ positive: 5, negative: 2, total: 7 }], | ||
| }); | ||
| setPool(mockPool); | ||
|
|
||
| const stats = await getFeedbackStats('g1'); | ||
|
|
||
| expect(stats.positive).toBe(5); | ||
| expect(stats.negative).toBe(2); | ||
| expect(stats.total).toBe(7); | ||
| expect(stats.ratio).toBe(71); | ||
| }); | ||
|
|
||
| it('returns null ratio when total is 0', async () => { | ||
| mockPool.query.mockResolvedValue({ | ||
| rows: [{ positive: 0, negative: 0, total: 0 }], | ||
| }); | ||
| setPool(mockPool); | ||
|
|
||
| const stats = await getFeedbackStats('g1'); | ||
| expect(stats.ratio).toBeNull(); | ||
| }); | ||
| }); | ||
|
|
||
| // ── getFeedbackTrend ──────────────────────────────────────────────────────── | ||
|
|
||
| describe('getFeedbackTrend', () => { | ||
| it('returns empty array when no pool', async () => { | ||
| const trend = await getFeedbackTrend('g1', 7); | ||
| expect(trend).toEqual([]); | ||
| }); | ||
|
|
||
| it('returns daily trend rows from DB', async () => { | ||
| mockPool.query.mockResolvedValue({ | ||
| rows: [ | ||
| { date: '2026-03-01', positive: 3, negative: 1 }, | ||
| { date: '2026-03-02', positive: 2, negative: 0 }, | ||
| ], | ||
| }); | ||
| setPool(mockPool); | ||
|
|
||
| const trend = await getFeedbackTrend('g1', 30); | ||
|
|
||
| expect(trend).toHaveLength(2); | ||
| expect(trend[0].date).toBe('2026-03-01'); | ||
| expect(trend[0].positive).toBe(3); | ||
| expect(trend[0].negative).toBe(1); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Add tests for deleteFeedback behavior.
The suite covers insert/stats/trend well, but the reaction-removal path (deleteFeedback) is not exercised. Please add cases for no-pool, successful delete query, and DB error handling so stale feedback cleanup is validated.
As per coding guidelines, "All new code must include tests. Test coverage must maintain an 80% threshold on statements, branches, functions, and lines."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/modules/aiFeedback.test.js` around lines 59 - 178, Add unit tests for
deleteFeedback mirroring other suites: (1) "does nothing when no pool is
configured" — call deleteFeedback({ messageId, channelId, guildId, userId })
without setting a pool and assert mockPool.query was not called; (2) "deletes
feedback via pool query" — setPool(mockPool) with
mockPool.query.mockResolvedValue({ rows: [] }), call deleteFeedback(...) and
assert mockPool.query was called with a SQL string containing 'DELETE FROM
ai_feedback' and the correct params [messageId, channelId, guildId, userId]; (3)
"handles DB errors gracefully without throwing" — setPool(mockPool) with
mockPool.query.mockRejectedValue(new Error('DB error')), call
deleteFeedback(...) and assert the promise resolves to undefined (does not
throw). Optionally add a test using _setPoolGetter(() => altPool) to validate DI
like other tests.
| const fetchStats = useCallback(async () => { | ||
| if (!selectedGuild || !apiBase) return; | ||
|
|
||
| setLoading(true); | ||
| setError(null); | ||
|
|
||
| try { | ||
| const res = await fetch(`${apiBase}/guilds/${selectedGuild.id}/ai-feedback/stats?days=30`, { | ||
| credentials: 'include', | ||
| }); | ||
|
|
||
| if (!res.ok) { | ||
| throw new Error(`HTTP ${res.status}`); | ||
| } | ||
|
|
||
| const data = (await res.json()) as FeedbackStats; | ||
| setStats(data); | ||
| } catch (err) { | ||
| setError(err instanceof Error ? err.message : 'Failed to load feedback stats'); | ||
| } finally { | ||
| setLoading(false); | ||
| } | ||
| }, [selectedGuild, apiBase]); | ||
|
|
||
| useEffect(() => { | ||
| void fetchStats(); | ||
| }, [fetchStats]); |
There was a problem hiding this comment.
Guard against stale fetch responses on guild changes.
If guild selection changes quickly, an older in-flight request can resolve after the new one and overwrite stats with the wrong guild’s data. Add cancellation or a request-id guard in this effect.
🧩 Suggested pattern
- const fetchStats = useCallback(async () => {
+ const fetchStats = useCallback(async (signal?: AbortSignal) => {
if (!selectedGuild || !apiBase) return;
@@
- const res = await fetch(`${apiBase}/guilds/${selectedGuild.id}/ai-feedback/stats?days=30`, {
+ const res = await fetch(`${apiBase}/guilds/${selectedGuild.id}/ai-feedback/stats?days=30`, {
credentials: 'include',
+ signal,
});
@@
- useEffect(() => {
- void fetchStats();
- }, [fetchStats]);
+ useEffect(() => {
+ const controller = new AbortController();
+ void fetchStats(controller.signal);
+ return () => controller.abort();
+ }, [fetchStats]);📝 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 fetchStats = useCallback(async () => { | |
| if (!selectedGuild || !apiBase) return; | |
| setLoading(true); | |
| setError(null); | |
| try { | |
| const res = await fetch(`${apiBase}/guilds/${selectedGuild.id}/ai-feedback/stats?days=30`, { | |
| credentials: 'include', | |
| }); | |
| if (!res.ok) { | |
| throw new Error(`HTTP ${res.status}`); | |
| } | |
| const data = (await res.json()) as FeedbackStats; | |
| setStats(data); | |
| } catch (err) { | |
| setError(err instanceof Error ? err.message : 'Failed to load feedback stats'); | |
| } finally { | |
| setLoading(false); | |
| } | |
| }, [selectedGuild, apiBase]); | |
| useEffect(() => { | |
| void fetchStats(); | |
| }, [fetchStats]); | |
| const fetchStats = useCallback(async (signal?: AbortSignal) => { | |
| if (!selectedGuild || !apiBase) return; | |
| setLoading(true); | |
| setError(null); | |
| try { | |
| const res = await fetch(`${apiBase}/guilds/${selectedGuild.id}/ai-feedback/stats?days=30`, { | |
| credentials: 'include', | |
| signal, | |
| }); | |
| if (!res.ok) { | |
| throw new Error(`HTTP ${res.status}`); | |
| } | |
| const data = (await res.json()) as FeedbackStats; | |
| setStats(data); | |
| } catch (err) { | |
| setError(err instanceof Error ? err.message : 'Failed to load feedback stats'); | |
| } finally { | |
| setLoading(false); | |
| } | |
| }, [selectedGuild, apiBase]); | |
| useEffect(() => { | |
| const controller = new AbortController(); | |
| void fetchStats(controller.signal); | |
| return () => controller.abort(); | |
| }, [fetchStats]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/dashboard/ai-feedback-stats.tsx` around lines 45 - 71,
fetchStats can suffer from stale responses overwriting state when selectedGuild
changes; modify fetchStats/useEffect to cancel or ignore out-of-date requests by
using an AbortController or a per-request id. Specifically, in the effect that
calls fetchStats, create an AbortController (or increment a local requestId
counter) and pass its signal (or id) into fetchStats; inside fetchStats check
the signal.aborted (or compare requestId before calling
setStats/setLoading/setError) and skip state updates if cancelled/mismatched;
also ensure fetch is called with the controller.signal and that you call
controller.abort() (or discard the id) in the effect cleanup so only the latest
guild’s response can update stats.
| <ChannelSelector | ||
| id="ai-blocked-channels" | ||
| guildId={guildId} | ||
| selected={(draftConfig.ai?.blockedChannelIds ?? []) as string[]} | ||
| onChange={updateAiBlockedChannels} | ||
| placeholder="Select channels to block AI in..." | ||
| disabled={saving} | ||
| filter="text" | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use the store-backed channel source for this picker.
This newly added blocked-channel selector should follow the dashboard’s cached Discord-entities flow (Zustand) rather than a direct-fetch path, to avoid repeated fetches and stale channel lists across sections.
As per coding guidelines, "Component files should integrate with Zustand stores for state management (e.g., discord-entities store for caching Discord channels and roles per guild)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/dashboard/config-editor.tsx` around lines 771 - 779,
Replace the direct-fetch ChannelSelector usage with the dashboard's
Zustand-backed channel source: read the cached channels for guildId from the
discord-entities store (e.g., useDiscordEntitiesStore or discordEntitiesStore
selector that returns channels for a guild) and pass that channel list into
ChannelSelector instead of allowing it to perform its own fetch; keep the same
props (id "ai-blocked-channels", selected={(draftConfig.ai?.blockedChannelIds ??
[]) as string[]}, onChange={updateAiBlockedChannels}, placeholder,
disabled={saving}, filter="text") but supply the store-backed channels via the
component’s channels/options prop (or a source prop used by our other selectors)
so ChannelSelector uses the cached Zustand data for guildId.
|
|
||
| const row = result.rows[0]; | ||
| const positive = row?.positive || 0; | ||
| const negative = row?.negative || 1; |
There was a problem hiding this comment.
default value should be 0, not 1 - when there are zero negative reactions, this will incorrectly report 1
| const negative = row?.negative || 1; | |
| const negative = row?.negative || 0; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/modules/aiFeedback.js
Line: 179
Comment:
default value should be `0`, not `1` - when there are zero negative reactions, this will incorrectly report 1
```suggestion
const negative = row?.negative || 0;
```
How can I resolve this? If you propose a fix, please make it concise.11a9bb5 to
648caff
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // For threads, parentId is also checked so blocking the parent channel | ||
| // blocks all its child threads. | ||
| const parentId = message.channel.isThread?.() ? message.channel.parentId : null; | ||
| if (isChannelBlocked(message.channel.id, parentId, message.guild.id)) return; |
There was a problem hiding this comment.
isChannelBlocked() calls getConfig(guildId) internally, but this handler already computed guildConfig = getConfig(message.guild.id) for the same message. This introduces an extra getConfig() + deep clone on the hot message path. Consider reading guildConfig.ai?.blockedChannelIds directly here, or refactor isChannelBlocked to accept the already-resolved guildConfig/blocklist array instead of re-fetching config.
| if (isChannelBlocked(message.channel.id, parentId, message.guild.id)) return; | |
| const blockedChannels = guildConfig.ai?.blockedChannelIds || []; | |
| const isBlockedChannel = | |
| blockedChannels.includes(message.channel.id) || | |
| (parentId !== null && blockedChannels.includes(parentId)); | |
| if (isBlockedChannel) { | |
| return; | |
| } |
| * @param {string} guildId - The guild ID for per-guild configuration. | ||
| * @returns {boolean} `true` when the channel is blocked, `false` otherwise. | ||
| */ | ||
| export function isChannelBlocked(channelId, parentId = null, guildId) { | ||
| try { | ||
| const config = getConfig(guildId); |
There was a problem hiding this comment.
isChannelBlocked(channelId, parentId = null, guildId) has a required guildId parameter after an optional parameter. This makes the API easy to misuse (e.g., a 2-arg call can’t supply guildId without passing a null placeholder) and conflicts with the JSDoc implying guildId is required. Consider reordering parameters, using an options object, or making guildId explicitly optional in both the signature and docs (with a documented fallback behavior).
| * @param {string} guildId - The guild ID for per-guild configuration. | |
| * @returns {boolean} `true` when the channel is blocked, `false` otherwise. | |
| */ | |
| export function isChannelBlocked(channelId, parentId = null, guildId) { | |
| try { | |
| const config = getConfig(guildId); | |
| * @param {string} [guildId] - Optional guild ID for per-guild configuration. When omitted, | |
| * the global/default configuration is used via {@link getConfig} with no arguments. | |
| * @returns {boolean} `true` when the channel is blocked, `false` otherwise. | |
| */ | |
| export function isChannelBlocked(channelId, parentId = null, guildId = null) { | |
| try { | |
| // When guildId is provided, use per-guild configuration; otherwise fall back | |
| // to the global/default configuration, matching the behavior of other helpers | |
| // such as getHistoryLength and getHistoryTTLDays. | |
| const config = guildId ? getConfig(guildId) : getConfig(); |
| describe('isChannelBlocked', () => { | ||
| it('should return false when blockedChannelIds is not set', () => { | ||
| getConfig.mockReturnValue({ ai: {} }); | ||
| expect(isChannelBlocked('ch1')).toBe(false); | ||
| }); | ||
|
|
||
| it('should return false when blockedChannelIds is empty', () => { | ||
| getConfig.mockReturnValue({ ai: { blockedChannelIds: [] } }); | ||
| expect(isChannelBlocked('ch1')).toBe(false); | ||
| }); | ||
|
|
||
| it('should return true when channelId is in blockedChannelIds', () => { | ||
| getConfig.mockReturnValue({ ai: { blockedChannelIds: ['ch1', 'ch2'] } }); | ||
| expect(isChannelBlocked('ch1')).toBe(true); | ||
| expect(isChannelBlocked('ch2')).toBe(true); | ||
| }); | ||
|
|
||
| it('should return false when channelId is not in blockedChannelIds', () => { | ||
| getConfig.mockReturnValue({ ai: { blockedChannelIds: ['ch1'] } }); | ||
| expect(isChannelBlocked('ch3')).toBe(false); | ||
| }); | ||
|
|
||
| it('should return true when parentId is in blockedChannelIds (thread in blocked parent)', () => { | ||
| getConfig.mockReturnValue({ ai: { blockedChannelIds: ['parent-ch'] } }); | ||
| expect(isChannelBlocked('thread-ch', 'parent-ch')).toBe(true); | ||
| }); | ||
|
|
||
| it('should return true when channelId matches even if parentId is not blocked', () => { | ||
| getConfig.mockReturnValue({ ai: { blockedChannelIds: ['thread-ch'] } }); | ||
| expect(isChannelBlocked('thread-ch', 'parent-ch')).toBe(true); | ||
| }); | ||
|
|
||
| it('should return false when neither channelId nor parentId is in blockedChannelIds', () => { | ||
| getConfig.mockReturnValue({ ai: { blockedChannelIds: ['other-ch'] } }); | ||
| expect(isChannelBlocked('thread-ch', 'parent-ch')).toBe(false); | ||
| }); |
There was a problem hiding this comment.
The new isChannelBlocked behavior is intended to be per-guild (it accepts guildId and calls getConfig(guildId)), but the added unit tests only call it without guildId. Add at least one test that passes a guildId and asserts getConfig is called with that guild ID (and that the returned value is respected), so regressions don’t silently fall back to global config.
Rebased #187 on latest main to resolve BEHIND status.
This is a simple rebase fix - the original PR adds an config option to control which channels the AI assistant is allowed to respond in.
Changes: