fix: AI response feedback review comments#190
Merged
BillChirico merged 2 commits intomainfrom Mar 2, 2026
Merged
Conversation
- Add ai_feedback table migration (003_ai_feedback.cjs) - Add aiFeedback.js module with recordFeedback, getFeedbackStats, getFeedbackTrend - Register AI message IDs in-memory (LRU-capped at 2000) for reaction filtering - Add 👍👎 reactions to first chunk of AI responses (opt-in via ai.feedback.enabled) - Handle feedback reactions in registerReactionHandlers (events.js) - Add /api/v1/guilds/:id/ai-feedback/stats and /recent endpoints - Add AiFeedbackStats dashboard component with pie chart + bar trend - Set ai.feedback.enabled: false (default opt-in) in config.json - 26 new tests (unit + API) all passing - Lint clean (1 pre-existing Biome noArrayIndexKey warning only) Closes #182
- Fix LRU logic in registerAiMessage - Add deleteFeedback function for reaction-remove handling - Update API routes to use module functions - Add proper error handling - Simplify by removing getConfig checks from module (config gating at API/events level)
Contributor
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughIntroduces a complete AI feedback system enabling users to provide positive/negative feedback on AI-generated Discord messages via reactions. Includes database schema, API endpoints for stats retrieval, core feedback management module, event handling, message response integration, and dashboard analytics component. Changes
Possibly related PRs
🚥 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| * "503": | ||
| * $ref: "#/components/responses/ServiceUnavailable" | ||
| */ | ||
| router.get('/stats', feedbackRateLimit, requireGuildAdmin, validateGuild, async (req, res, next) => { |
Check failure
Code scanning / CodeQL
Missing rate limiting High
Copilot Autofix
AI 12 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.
| * "503": | ||
| * $ref: "#/components/responses/ServiceUnavailable" | ||
| */ | ||
| router.get('/recent', feedbackRateLimit, requireGuildAdmin, validateGuild, async (req, res, next) => { |
Check failure
Code scanning / CodeQL
Missing rate limiting High
Copilot Autofix
AI 12 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.
![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: 11
🤖 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-15: Add documentation for the new ai.feedback.enabled
configuration key to README.md under the "Configuration" section's "AI Chat
(`ai`)" table: add a row for "feedback.enabled" with type "boolean" and a brief
description such as "Enable or disable user feedback collection for AI responses
(default: false)"; ensure the key is shown nested under the ai prefix (e.g.,
ai.feedback.enabled), include the default value and any notes about required
features or impact, and keep formatting consistent with the existing table
entries in the AI Chat (`ai`) documentation.
In `@migrations/003_ai_feedback.cjs`:
- Around line 24-32: Add a composite index to optimize queries filtering by
guild and recent timestamps: create an index on ai_feedback(guild_id,
created_at) (suggested name idx_ai_feedback_guild_created_at) to support WHERE
guild_id = $1 AND created_at >= ... and avoid large range scans; in the
migration (the block that currently creates idx_ai_feedback_guild_id and
idx_ai_feedback_message_id) add a CREATE INDEX IF NOT EXISTS for this composite
key so DB query planner can use it for trend/stats paths.
In `@src/api/routes/ai-feedback.js`:
- Around line 109-111: Replace the bare next(err) in the two catch blocks with a
contextual logError(...) call and forward a wrapped project error from
src/utils/errors.js: import logError and an appropriate class (e.g.,
InternalServerError) from src/utils/errors.js at the top, then inside each catch
call logError({ err, context: 'ai-feedback route', handler: '<describe handler
name or HTTP method>' }) and call next(new InternalServerError('Failed handling
ai-feedback request', { cause: err })) instead of next(err); apply this change
to both catch sites (the catch at lines ~109-111 and the one at ~192-194).
In `@src/modules/aiFeedback.js`:
- Around line 100-125: The module functions must fetch and check per-guild
config on each call: update recordFeedback to call getConfig(guildId) at the
start and return early if falsy (logging a warn), and modify deleteFeedback (and
other public functions in this file such as any functions around lines 162-188,
196-223, 231-256) to accept guildId if missing, call getConfig(guildId) each
invocation, and gate execution when config is absent; ensure you import/get
getConfig and keep existing DB logic unchanged beyond the added config check and
early return/logging.
- Around line 177-181: The code wrongly treats a legitimate zero negative count
as one because it uses logical OR fallbacks; update the fallbacks to use nullish
coalescing so zeros are preserved (e.g., change positive = row?.positive || 0
and negative = row?.negative || 1 and total = row?.total || 0 to use ?? with 0
defaults, and specifically change negative's default from 1 to 0) so that
variables positive, negative, and total reflect true zero values and ratio
computation (ratio in this snippet) remains correct.
In `@src/modules/events.js`:
- Around line 348-357: The current reaction-remove handler always calls
deleteFeedback when either feedback emoji is removed, which can erase valid
feedback if the user still has the opposite reaction; update the handler (the
block using isAiMessage, FEEDBACK_EMOJI and deleteFeedback) to first check the
message's reactions for the opposite feedback emoji and whether that user still
has that reaction before deleting: query reaction.message.reactions.cache (or
fetch reactions if cache is empty), get the Reaction objects for
FEEDBACK_EMOJI.positive and FEEDBACK_EMOJI.negative, and only call
deleteFeedback when neither reaction remains by that user (i.e., the user does
not appear in the remaining reaction's users), otherwise do nothing. Ensure to
handle missing caches by fetching reaction.users when necessary.
In `@src/modules/triage-respond.js`:
- Around line 44-46: The current .catch(() => { /* Reaction permission errors
are non-fatal */ }) silently swallows failures; replace it with a low-noise
warning that logs the error and minimal message/guild metadata so
permission/config regressions are visible. In the catch for the reaction promise
in src/modules/triage-respond.js, change the anonymous empty handler to accept
the error (e.g., err) and call your logger.warn (or console.warn) with a short
message plus err.message/stack and context: guild id (or guild?.id), channel id,
and message id (from the message object used when reacting), so failures are
recorded but remain non-fatal.
In `@tests/api/routes/ai-feedback.test.js`:
- Around line 49-64: Tests are tightly coupled to DB internals by asserting on
mockPool.query call order; change them to mock the route module's DB dependency
at the module boundary instead of inspecting mockPool.query positions.
Specifically, stop relying on mockPool and instead stub or vi.mock the exported
data-access functions or adapter used by the routes (the module that the route
handlers import) and inject those stubs when creating the app via createApp;
update assertions to verify route responses and that the route-level adapter
methods (the mocked exported functions) were called with expected args rather
than checking mockPool.query call indices. Ensure use of the same identifiers
from the test setup (createApp, mockGuild) while replacing mockPool.query-based
assertions with adapter-level mocks.
In `@tests/modules/aiFeedback.test.js`:
- Around line 142-150: The test only checks stats.ratio when total is 0; update
the assertion in the it block for getFeedbackStats to validate the entire
returned object shape (positive, negative, total and ratio) instead of only
ratio so incorrect defaults are caught — e.g., assert that
getFeedbackStats('g1') returns an object with positive: 0, negative: 0, total: 0
and ratio: null by replacing expect(stats.ratio).toBeNull() with a full equality
assertion against that object.
- Around line 14-23: Add tests that exercise the new deleteFeedback flow by
importing deleteFeedback into tests/modules/aiFeedback.test.js alongside
registerAiMessage, recordFeedback, getFeedbackStats, and getFeedbackTrend;
create an AI message with registerAiMessage, add feedback via recordFeedback,
then call deleteFeedback (simulating reaction-remove) and assert that
getFeedbackStats and getFeedbackTrend reflect the removal (counts/trend drop
back), and that isAiMessage/state remains consistent. Ensure the test covers
both successful deletion and a no-op delete of a non-existent feedback id to
validate branch behavior and maintain coverage thresholds.
In `@web/src/components/dashboard/ai-feedback-stats.tsx`:
- Around line 41-67: The component-level state and fetch logic (stats, setStats,
loading, setLoading, error, setError and the fetchStats function) should be
moved into a Zustand-backed hook (e.g., create/useAIFeedbackStore) so the
component consumes store state instead of owning it; implement a store that
holds FeedbackStats | null, loading:boolean, error:string|null and an async
action fetchStats(guildId, apiBase) that performs the fetch logic currently in
fetchStats (respecting selectedGuild.id and apiBase), sets loading/error/stats
appropriately, and export selectors/hooks (e.g., useAIFeedbackStore(state =>
...)) for the component to read stats/loading/error and to trigger fetchStats;
update the component to remove local useState/useCallback usage and use the new
store hook with selectedGuild and apiBase as inputs to call the store action.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
config.jsonmigrations/003_ai_feedback.cjssrc/api/index.jssrc/api/routes/ai-feedback.jssrc/modules/aiFeedback.jssrc/modules/events.jssrc/modules/triage-respond.jstests/api/routes/ai-feedback.test.jstests/modules/aiFeedback.test.jsweb/src/components/dashboard/ai-feedback-stats.tsx
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Agent
- GitHub Check: Greptile Review
- GitHub Check: Docker Build Validation
🧰 Additional context used
📓 Path-based instructions (8)
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
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.tsx
**/*.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/modules/events.jstests/modules/aiFeedback.test.jssrc/api/routes/ai-feedback.jssrc/modules/triage-respond.jstests/api/routes/ai-feedback.test.jssrc/api/index.jssrc/modules/aiFeedback.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/modules/events.jssrc/api/routes/ai-feedback.jssrc/modules/triage-respond.jssrc/api/index.jssrc/modules/aiFeedback.js
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/events.jssrc/modules/triage-respond.jssrc/modules/aiFeedback.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/aiFeedback.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
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 migrations/*.cjs : Database migrations must use CommonJS (.cjs) format and follow naming convention `NNN_description.cjs` where NNN is a zero-padded sequence number
Applied to files:
migrations/003_ai_feedback.cjs
📚 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/events.jssrc/modules/aiFeedback.js
🧬 Code graph analysis (7)
src/modules/events.js (1)
src/modules/aiFeedback.js (5)
isAiMessage(47-49)FEEDBACK_EMOJI(10-13)FEEDBACK_EMOJI(10-13)recordFeedback(100-125)deleteFeedback(134-155)
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/api/routes/ai-feedback.js (3)
src/api/middleware/rateLimit.js (1)
rateLimit(15-61)src/api/routes/guilds.js (1)
validateGuild(302-312)src/modules/aiFeedback.js (3)
getFeedbackStats(162-188)getFeedbackTrend(196-223)getRecentFeedback(231-256)
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)
tests/api/routes/ai-feedback.test.js (1)
src/api/server.js (1)
createApp(28-91)
src/api/index.js (1)
src/api/routes/ai-feedback.js (1)
router(14-14)
src/modules/aiFeedback.js (2)
src/db.js (1)
getPool(314-319)src/logger.js (2)
warn(238-240)info(231-233)
🪛 GitHub Check: CodeQL
src/api/routes/ai-feedback.js
[failure] 88-88: Missing rate limiting
This route handler performs authorization, but is not rate-limited.
[failure] 178-178: 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.
🪛 GitHub Check: Test (Vitest Coverage)
tests/api/routes/ai-feedback.test.js
[failure] 145-145: tests/api/routes/ai-feedback.test.js > ai-feedback routes > GET /api/v1/guilds/:id/ai-feedback/stats > ignores out-of-range days param (uses default 30)
TypeError: Cannot read properties of undefined (reading '1')
❯ tests/api/routes/ai-feedback.test.js:145:23
[failure] 134-134: tests/api/routes/ai-feedback.test.js > ai-feedback routes > GET /api/v1/guilds/:id/ai-feedback/stats > accepts custom days param
TypeError: Cannot read properties of undefined (reading '1')
❯ tests/api/routes/ai-feedback.test.js:134:23
[failure] 103-103: tests/api/routes/ai-feedback.test.js > ai-feedback routes > GET /api/v1/guilds/:id/ai-feedback/stats > returns aggregate stats with default 30-day window
AssertionError: expected +0 to be 10 // Object.is equality
- Expected
- Received
- 10
- 0
❯ tests/api/routes/ai-feedback.test.js:103:33
[failure] 85-85: tests/api/routes/ai-feedback.test.js > ai-feedback routes > GET /api/v1/guilds/:id/ai-feedback/stats > returns 503 when DB is unavailable
AssertionError: expected 200 to be 503 // Object.is equality
- Expected
- Received
- 503
- 200
❯ tests/api/routes/ai-feedback.test.js:85:26
🔇 Additional comments (4)
migrations/003_ai_feedback.cjs (1)
10-22: Table definition and constraints look solid.
feedback_typeandcreated_atare properly constrained/defaulted, and the uniqueness rule matches per-user per-message semantics.src/api/index.js (1)
40-41: Route wiring is correct and consistently protected.Good placement and middleware stack (
requireAuth()+auditLogMiddleware()) align with the protected guild route pattern.src/api/routes/ai-feedback.js (1)
88-88: Rate limiting is correctly applied before authorization.
feedbackRateLimitis in the middleware chain for both protected endpoints, so the CodeQL warning here is not reproducible from this code path.Also applies to: 178-178
src/modules/aiFeedback.js (1)
26-40: LRU recency refresh behavior looks correct.Existing IDs are refreshed (
delete+add) and eviction only applies when capacity is reached.
Comment on lines
+13
to
15
| "feedback": { | ||
| "enabled": false | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if README.md exists and search for the config documentation
if [ -f README.md ]; then
echo "=== Searching for ai.feedback documentation in README.md ==="
rg -n "ai\.feedback|feedback.*enabled|config.*feedback" README.md -C 3 || echo "No matches found for ai.feedback patterns"
echo ""
echo "=== Looking for config reference section ==="
rg -n "config|Configuration" README.md -i | head -20
else
echo "README.md not found"
fiRepository: VolvoxLLC/volvox-bot
Length of output: 1427
🏁 Script executed:
# Read the Configuration section from README.md
sed -n '125,250p' README.md | head -80Repository: VolvoxLLC/volvox-bot
Length of output: 4831
Document the new ai.feedback.enabled config key in README.md's configuration reference.
This adds a new config key ai.feedback.enabled to the ai section of config.json, but the corresponding documentation in README.md's Configuration section is missing. Add an entry to the "AI Chat (ai)" table with the key, type, and description.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config.json` around lines 13 - 15, Add documentation for the new
ai.feedback.enabled configuration key to README.md under the "Configuration"
section's "AI Chat (`ai`)" table: add a row for "feedback.enabled" with type
"boolean" and a brief description such as "Enable or disable user feedback
collection for AI responses (default: false)"; ensure the key is shown nested
under the ai prefix (e.g., ai.feedback.enabled), include the default value and
any notes about required features or impact, and keep formatting consistent with
the existing table entries in the AI Chat (`ai`) documentation.
Comment on lines
+24
to
+32
| 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) | ||
| `); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a composite index for trend/stats query paths.
Current indexes don’t optimize WHERE guild_id = $1 AND created_at >= .... Add (guild_id, created_at) to avoid full/large range scans as data grows.
♻️ Proposed migration addition
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)
+ `);🤖 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 - 32, Add a composite index
to optimize queries filtering by guild and recent timestamps: create an index on
ai_feedback(guild_id, created_at) (suggested name
idx_ai_feedback_guild_created_at) to support WHERE guild_id = $1 AND created_at
>= ... and avoid large range scans; in the migration (the block that currently
creates idx_ai_feedback_guild_id and idx_ai_feedback_message_id) add a CREATE
INDEX IF NOT EXISTS for this composite key so DB query planner can use it for
trend/stats paths.
Comment on lines
+109
to
+111
| } catch (err) { | ||
| next(err); | ||
| } |
Contributor
There was a problem hiding this comment.
Use route-standard error handling in catch blocks.
Both handlers forward raw errors with next(err) but do not log route context and do not wrap/rethrow using custom classes from src/utils/errors.js. Please add contextual logError(...) and forward a project error class instead of raw errors.
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: 192-194
🤖 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 109 - 111, Replace the bare
next(err) in the two catch blocks with a contextual logError(...) call and
forward a wrapped project error from src/utils/errors.js: import logError and an
appropriate class (e.g., InternalServerError) from src/utils/errors.js at the
top, then inside each catch call logError({ err, context: 'ai-feedback route',
handler: '<describe handler name or HTTP method>' }) and call next(new
InternalServerError('Failed handling ai-feedback request', { cause: err }))
instead of next(err); apply this change to both catch sites (the catch at lines
~109-111 and the one at ~192-194).
Comment on lines
+100
to
+125
| export async function recordFeedback({ messageId, channelId, guildId, userId, feedbackType }) { | ||
| const pool = getPool(); | ||
| if (!pool) { | ||
| warn('No DB pool — cannot record AI feedback', { messageId, userId, feedbackType }); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| await pool.query( | ||
| `INSERT INTO ai_feedback (message_id, channel_id, guild_id, user_id, feedback_type) | ||
| VALUES ($1, $2, $3, $4, $5) | ||
| ON CONFLICT (message_id, user_id) | ||
| DO UPDATE SET feedback_type = EXCLUDED.feedback_type, created_at = NOW()`, | ||
| [messageId, channelId, guildId, userId, feedbackType], | ||
| ); | ||
|
|
||
| info('AI feedback recorded', { messageId, userId, feedbackType, guildId }); | ||
| } catch (err) { | ||
| logError('Failed to record AI feedback', { | ||
| messageId, | ||
| userId, | ||
| feedbackType, | ||
| error: err.message, | ||
| }); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Enforce getConfig(guildId) checks inside module entrypoints.
These public module functions rely on callers for gating. Per repo rule, per-request module functions should read config on each invocation so runtime config changes are consistently honored (including future call sites). This likely requires adding guildId to deleteFeedback input as well.
Based on learnings: "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".
Also applies to: 162-188, 196-223, 231-256
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/aiFeedback.js` around lines 100 - 125, The module functions must
fetch and check per-guild config on each call: update recordFeedback to call
getConfig(guildId) at the start and return early if falsy (logging a warn), and
modify deleteFeedback (and other public functions in this file such as any
functions around lines 162-188, 196-223, 231-256) to accept guildId if missing,
call getConfig(guildId) each invocation, and gate execution when config is
absent; ensure you import/get getConfig and keep existing DB logic unchanged
beyond the added config check and early return/logging.
Comment on lines
+177
to
+181
| const row = result.rows[0]; | ||
| const positive = row?.positive || 0; | ||
| const negative = row?.negative || 1; | ||
| const total = row?.total || 0; | ||
| const ratio = total > 0 ? Math.round((positive / total) * 100) : null; |
Contributor
There was a problem hiding this comment.
negative fallback currently fabricates one negative vote.
row?.negative || 1 converts a legitimate 0 count into 1, which corrupts returned stats.
Proposed fix
- const negative = row?.negative || 1;
+ const negative = row?.negative || 0;📝 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
| const row = result.rows[0]; | |
| const positive = row?.positive || 0; | |
| const negative = row?.negative || 1; | |
| const total = row?.total || 0; | |
| const ratio = total > 0 ? Math.round((positive / total) * 100) : null; | |
| const row = result.rows[0]; | |
| const positive = row?.positive || 0; | |
| const negative = row?.negative || 0; | |
| const total = row?.total || 0; | |
| const ratio = total > 0 ? Math.round((positive / total) * 100) : null; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/aiFeedback.js` around lines 177 - 181, The code wrongly treats a
legitimate zero negative count as one because it uses logical OR fallbacks;
update the fallbacks to use nullish coalescing so zeros are preserved (e.g.,
change positive = row?.positive || 0 and negative = row?.negative || 1 and total
= row?.total || 0 to use ?? with 0 defaults, and specifically change negative's
default from 1 to 0) so that variables positive, negative, and total reflect
true zero values and ratio computation (ratio in this snippet) remains correct.
Comment on lines
+44
to
+46
| .catch(() => { | ||
| // Reaction permission errors are non-fatal | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Don’t silently swallow reaction failures.
Swallowing errors here makes permission/config regressions invisible. Log a low-noise warning with message/guild metadata.
🪵 Minimal observability patch
- .catch(() => {
- // Reaction permission errors are non-fatal
- });
+ .catch((err) => {
+ warn('Failed to add AI feedback reactions', {
+ messageId: first.id,
+ channelId: first.channelId,
+ guildId: first.guildId,
+ 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
| .catch(() => { | |
| // Reaction permission errors are non-fatal | |
| }); | |
| .catch((err) => { | |
| warn('Failed to add AI feedback reactions', { | |
| messageId: first.id, | |
| channelId: first.channelId, | |
| guildId: first.guildId, | |
| error: err?.message, | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/triage-respond.js` around lines 44 - 46, The current .catch(() =>
{ /* Reaction permission errors are non-fatal */ }) silently swallows failures;
replace it with a low-noise warning that logs the error and minimal
message/guild metadata so permission/config regressions are visible. In the
catch for the reaction promise in src/modules/triage-respond.js, change the
anonymous empty handler to accept the error (e.g., err) and call your
logger.warn (or console.warn) with a short message plus err.message/stack and
context: guild id (or guild?.id), channel id, and message id (from the message
object used when reacting), so failures are recorded but remain non-fatal.
Comment on lines
+49
to
+64
| beforeEach(() => { | ||
| vi.stubEnv('BOT_API_SECRET', TEST_SECRET); | ||
|
|
||
| mockPool = { | ||
| query: vi.fn().mockResolvedValue({ rows: [] }), | ||
| connect: vi.fn(), | ||
| }; | ||
|
|
||
| const client = { | ||
| guilds: { cache: new Map([[GUILD_ID, mockGuild]]) }, | ||
| ws: { status: 0, ping: 42 }, | ||
| user: { tag: 'Bot#1234' }, | ||
| }; | ||
|
|
||
| app = createApp(client, mockPool); | ||
| }); |
Contributor
There was a problem hiding this comment.
Tests are still coupled to pre-refactor DB internals.
After the route refactor, these specs should validate route-module contracts, not mockPool.query call positions. The current coupling is the root cause of the reported failures (undefined calls and zeroed stats).
🧪 Stabilize tests by mocking route dependencies at module boundary
+vi.mock('../../../src/modules/aiFeedback.js', () => ({
+ getFeedbackStats: vi.fn(),
+ getFeedbackTrend: vi.fn(),
+ getRecentFeedback: vi.fn(),
+}));
// in tests
-const trendCall = mockPool.query.mock.calls[1];
-expect(trendCall[1]).toContain(7);
+expect(getFeedbackTrend).toHaveBeenCalledWith(GUILD_ID, 7);
-// DB-unavailable path via createApp(client, null)
+// DB-unavailable path should be simulated by mocked module throwing/rejecting
+getFeedbackStats.mockRejectedValueOnce(new Error('DB unavailable'));Also applies to: 93-155, 196-212
🤖 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 49 - 64, Tests are tightly
coupled to DB internals by asserting on mockPool.query call order; change them
to mock the route module's DB dependency at the module boundary instead of
inspecting mockPool.query positions. Specifically, stop relying on mockPool and
instead stub or vi.mock the exported data-access functions or adapter used by
the routes (the module that the route handlers import) and inject those stubs
when creating the app via createApp; update assertions to verify route responses
and that the route-level adapter methods (the mocked exported functions) were
called with expected args rather than checking mockPool.query call indices.
Ensure use of the same identifiers from the test setup (createApp, mockGuild)
while replacing mockPool.query-based assertions with adapter-level mocks.
Comment on lines
+14
to
+23
| import { | ||
| _setPoolGetter, | ||
| clearAiMessages, | ||
| getFeedbackStats, | ||
| getFeedbackTrend, | ||
| isAiMessage, | ||
| recordFeedback, | ||
| registerAiMessage, | ||
| setPool, | ||
| } from '../../src/modules/aiFeedback.js'; |
Contributor
There was a problem hiding this comment.
Add tests for deleteFeedback behavior.
This suite validates insert/stats/trend, but not the newly added delete path used by reaction-remove handling. That leaves a critical state transition untested.
As per coding guidelines tests/**/*.js: “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 14 - 23, Add tests that
exercise the new deleteFeedback flow by importing deleteFeedback into
tests/modules/aiFeedback.test.js alongside registerAiMessage, recordFeedback,
getFeedbackStats, and getFeedbackTrend; create an AI message with
registerAiMessage, add feedback via recordFeedback, then call deleteFeedback
(simulating reaction-remove) and assert that getFeedbackStats and
getFeedbackTrend reflect the removal (counts/trend drop back), and that
isAiMessage/state remains consistent. Ensure the test covers both successful
deletion and a no-op delete of a non-existent feedback id to validate branch
behavior and maintain coverage thresholds.
Comment on lines
+142
to
+150
| 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(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Strengthen the zero-total stats assertion to validate full shape.
Only asserting ratio can miss incorrect defaults for positive/negative/total. Assert the full object in this case.
✅ Suggested assertion update
const stats = await getFeedbackStats('g1');
- expect(stats.ratio).toBeNull();
+ expect(stats).toEqual({ positive: 0, negative: 0, total: 0, ratio: null });📝 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
| 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(); | |
| }); | |
| 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).toEqual({ positive: 0, negative: 0, total: 0, ratio: null }); | |
| }); |
🤖 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 142 - 150, The test only
checks stats.ratio when total is 0; update the assertion in the it block for
getFeedbackStats to validate the entire returned object shape (positive,
negative, total and ratio) instead of only ratio so incorrect defaults are
caught — e.g., assert that getFeedbackStats('g1') returns an object with
positive: 0, negative: 0, total: 0 and ratio: null by replacing
expect(stats.ratio).toBeNull() with a full equality assertion against that
object.
Comment on lines
+41
to
+67
| const [stats, setStats] = useState<FeedbackStats | null>(null); | ||
| const [loading, setLoading] = useState(false); | ||
| const [error, setError] = useState<string | null>(null); | ||
|
|
||
| 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]); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move stats/loading/error state to a Zustand store-backed hook.
This component currently owns fetch/state locally, which diverges from the repository’s store-driven state pattern for web components.
As per coding guidelines web/src/components/**/*.tsx: “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/ai-feedback-stats.tsx` around lines 41 - 67, The
component-level state and fetch logic (stats, setStats, loading, setLoading,
error, setError and the fetchStats function) should be moved into a
Zustand-backed hook (e.g., create/useAIFeedbackStore) so the component consumes
store state instead of owning it; implement a store that holds FeedbackStats |
null, loading:boolean, error:string|null and an async action fetchStats(guildId,
apiBase) that performs the fetch logic currently in fetchStats (respecting
selectedGuild.id and apiBase), sets loading/error/stats appropriately, and
export selectors/hooks (e.g., useAIFeedbackStore(state => ...)) for the
component to read stats/loading/error and to trigger fetchStats; update the
component to remove local useState/useCallback usage and use the new store hook
with selectedGuild and apiBase as inputs to call the store action.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the AI feedback (👍/👎 reactions) feature by introducing a dedicated aiFeedback module, wiring reaction add/remove handling, exposing API endpoints for feedback stats/recent entries, and adding supporting tests and migration/config updates.
Changes:
- Added
src/modules/aiFeedback.jsfor registering AI message IDs and recording/deleting/querying feedback. - Updated Discord reaction handlers to record feedback on add and delete it on remove.
- Added new API routes for feedback stats/trends and recent feedback, plus updated tests and a DB migration/config toggle.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/components/dashboard/ai-feedback-stats.tsx | Adds a dashboard card UI for AI feedback stats/trend. |
| tests/modules/aiFeedback.test.js | Adds unit tests for the new aiFeedback module interface. |
| tests/api/routes/ai-feedback.test.js | Adds route tests for the new AI feedback API endpoints. |
| src/modules/triage-respond.js | Adds auto-reactions (👍/👎) to the first AI response chunk and registers the message ID. |
| src/modules/events.js | Records feedback on reaction add and deletes feedback on reaction remove. |
| src/modules/aiFeedback.js | Implements in-memory AI message LRU tracking + DB persistence/query helpers. |
| src/api/routes/ai-feedback.js | Adds /stats and /recent endpoints for dashboard consumption. |
| src/api/index.js | Mounts the new AI feedback router under /guilds/:id/ai-feedback. |
| migrations/003_ai_feedback.cjs | Creates ai_feedback table + indexes. |
| config.json | Adds ai.feedback.enabled config flag and normalizes some unicode in strings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+109
to
+111
| } catch (err) { | ||
| next(err); | ||
| } |
There was a problem hiding this comment.
Error handling here uses next(err), but the API server’s error middleware returns a generic { error: 'Internal server error' } for 5xx. That conflicts with the route tests/OpenAPI which expect a specific 500 message (e.g. “Failed to fetch AI feedback stats”) and also leaves logError unused (lint). Handle DB/handler failures locally like other routes: log with context and respond with a stable 500 JSON error message.
Comment on lines
+178
to
+191
| router.get('/recent', feedbackRateLimit, requireGuildAdmin, validateGuild, async (req, res, next) => { | ||
| try { | ||
| const guildId = req.params.id; | ||
|
|
||
| let limit = 25; | ||
| if (req.query.limit !== undefined) { | ||
| const parsed = Number.parseInt(req.query.limit, 10); | ||
| if (!Number.isNaN(parsed) && parsed >= 1 && parsed <= 100) { | ||
| limit = parsed; | ||
| } | ||
| } | ||
|
|
||
| const feedback = await getRecentFeedback(guildId, limit); | ||
| res.json({ feedback }); |
There was a problem hiding this comment.
Same as /stats: this handler doesn’t check for req.app.locals.dbPool, so it can’t return the documented/tested 503 when DB is unavailable, and it risks always returning an empty list if the aiFeedback module pool isn’t initialized from the app’s pool. Add the pool check and ensure the DB queries run against the app’s pool.
| 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.
This migration uses created_at TIMESTAMP ... while the rest of the schema consistently uses TIMESTAMPTZ for created_at. Using a non-timezone timestamp can lead to ambiguous interpretation across environments and makes queries inconsistent with other tables. Consider switching created_at to TIMESTAMPTZ NOT NULL DEFAULT NOW() to match existing conventions.
Suggested change
| created_at TIMESTAMP NOT NULL DEFAULT NOW(), | |
| created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), |
Comment on lines
+127
to
+155
| /** | ||
| * Delete user feedback for an AI message (when reaction is removed). | ||
| * @param {Object} opts | ||
| * @param {string} opts.messageId - Discord message ID | ||
| * @param {string} opts.userId - Discord user ID | ||
| * @returns {Promise<void>} | ||
| */ | ||
| export async function deleteFeedback({ messageId, userId }) { | ||
| const pool = getPool(); | ||
| if (!pool) { | ||
| warn('No DB pool — cannot delete AI feedback', { messageId, userId }); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| await pool.query( | ||
| `DELETE FROM ai_feedback WHERE message_id = $1 AND user_id = $2`, | ||
| [messageId, userId], | ||
| ); | ||
|
|
||
| info('AI feedback deleted', { messageId, userId }); | ||
| } catch (err) { | ||
| logError('Failed to delete AI feedback', { | ||
| messageId, | ||
| userId, | ||
| error: err.message, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
deleteFeedback is new behavior that affects reaction-remove handling, but it isn’t covered by the module tests (which already cover other aiFeedback functions). Add tests for deleteFeedback success/no-pool/error cases so regressions don’t silently break feedback removal.
Comment on lines
+39
to
+53
| export function AiFeedbackStats() { | ||
| const { selectedGuild, apiBase } = useGuildSelection(); | ||
| const [stats, setStats] = useState<FeedbackStats | null>(null); | ||
| const [loading, setLoading] = useState(false); | ||
| const [error, setError] = useState<string | null>(null); | ||
|
|
||
| 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', |
There was a problem hiding this comment.
useGuildSelection (from @/hooks/use-guild-selection) returns a string | null guildId, but this component destructures { selectedGuild, apiBase }. As written, this won’t type-check/compile and the fetch URL/auth pattern doesn’t match other dashboard components (which call Next.js proxy routes under /api/guilds/:guildId/*). Update this component to use the guildId return value and align the request path with the existing proxy approach (or introduce the missing proxy route if you intend to call the bot API directly).
|
|
||
| const row = result.rows[0]; | ||
| const positive = row?.positive || 0; | ||
| const negative = row?.negative || 1; |
There was a problem hiding this comment.
negative defaults to 1 when the query returns no row/value (const negative = row?.negative || 1;). This inflates negative counts and makes total/ratio inconsistent. Default should be 0 (matching positive and total).
Suggested change
| const negative = row?.negative || 1; | |
| const negative = row?.negative || 0; |
Comment on lines
+58
to
+85
| // ── Pool injection ──────────────────────────────────────────────────── | ||
|
|
||
| /** @type {Function|null} */ | ||
| let _getPoolFn = null; | ||
|
|
||
| /** | ||
| * Set a pool getter function (for dependency injection / testing). | ||
| * @param {Function} fn | ||
| */ | ||
| export function _setPoolGetter(fn) { | ||
| _getPoolFn = fn; | ||
| } | ||
|
|
||
| /** @type {import('pg').Pool|null} */ | ||
| let _poolRef = null; | ||
|
|
||
| /** | ||
| * Set the database pool reference. | ||
| * @param {import('pg').Pool|null} pool | ||
| */ | ||
| export function setPool(pool) { | ||
| _poolRef = pool; | ||
| } | ||
|
|
||
| function getPool() { | ||
| if (_getPoolFn) return _getPoolFn(); | ||
| return _poolRef; | ||
| } |
There was a problem hiding this comment.
The aiFeedback module only uses its internal pool reference (setPool / _setPoolGetter), but nothing in this PR wires it up to the application’s actual DB pool. In production this will cause feedback writes/reads to silently no-op ("No DB pool" warnings), and in API route tests createApp(client, mockPool) won’t be able to exercise DB queries. Consider resolving the pool from the shared src/db.js getter (as other modules do) or ensure setPool(dbPool) is called during startup and when constructing the API app/router so module functions have access to the pool.
Comment on lines
+88
to
+104
| router.get('/stats', feedbackRateLimit, requireGuildAdmin, validateGuild, async (req, res, next) => { | ||
| try { | ||
| 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), | ||
| getFeedbackTrend(guildId, days), | ||
| ]); | ||
|
|
There was a problem hiding this comment.
These route handlers don’t check req.app.locals.dbPool and therefore can’t return the documented/tested 503 when the DB is unavailable. They also call module helpers that currently don’t use app.locals.dbPool, so even when createApp(..., dbPool) is provided, the endpoint will behave as if no DB exists. Add a pool availability check (consistent with other routes) and make sure the module queries use that same pool (e.g., pass the pool through or initialize the module pool from app.locals.dbPool).
|
| Filename | Overview |
|---|---|
| migrations/003_ai_feedback.cjs | Creates ai_feedback table with proper NOT NULL constraints, defaults, unique constraint, and indexes |
| src/modules/aiFeedback.js | Implements AI feedback tracking with LRU cache, DB operations, and stats - contains logic bug on line 179 |
| src/api/routes/ai-feedback.js | API routes delegate to module functions with proper error handling via next(err) pattern |
| src/modules/events.js | Added feedback tracking on reaction add/remove with proper config checks and fire-and-forget pattern |
| tests/modules/aiFeedback.test.js | Covers core module functions but missing tests for deleteFeedback and getRecentFeedback |
Last reviewed commit: 83fdcdd
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| const row = result.rows[0]; | ||
| const positive = row?.positive || 0; | ||
| const negative = row?.negative || 1; |
There was a problem hiding this comment.
Default should be 0, not 1 — when there are no negative feedbacks, this will incorrectly report negative: 1.
Suggested change
| 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 should be 0, not 1 — when there are no negative feedbacks, this will incorrectly report `negative: 1`.
```suggestion
const negative = row?.negative || 0;
```
How can I resolve this? If you propose a fix, please make it concise.
BillChirico
added a commit
that referenced
this pull request
Mar 2, 2026
Fixes three performance bottlenecks identified in code review of recently merged features (PR #121 conversations viewer, PR #190 AI feedback). ## Changes ### migrations/004_performance_indexes.cjs (new) Four new indexes targeting hot query paths: - idx_ai_feedback_guild_created (guild_id, created_at DESC) getFeedbackTrend() and getRecentFeedback() filtered by guild_id AND created_at but only had a single-column guild_id index, forcing a full guild scan + sort on every trend/recent call. - idx_conversations_content_trgm (GIN, pg_trgm) content ILIKE '%...%' search was a sequential scan. GIN/trgm index reduces this from O(n) to O(log n * trigram matches). Requires pg_trgm extension (added idempotently). - idx_conversations_guild_created (guild_id, created_at DESC) Default 30-day listing query filters guild_id + created_at. The existing 3-column (guild_id, channel_id, created_at) composite is suboptimal when channel_id is not in the predicate. - idx_flagged_messages_guild_message (guild_id, message_id) Conversation detail + flag endpoints query flagged_messages by guild_id AND message_id = ANY(...). Existing index only covers (guild_id, status). ### src/api/routes/conversations.js **GET / — Replace in-memory pagination with SQL CTE grouping** Before: fetched up to 10,000 message rows into Node memory, grouped them in JavaScript (O(n) time + memory), then sliced for pagination. Every page request loaded the full 10k row dataset. After: single SQL query using window functions (LAG + SUM OVER) to identify conversation boundaries and aggregate summaries directly. COUNT(*) OVER() provides total count without a second query. Pagination happens at the DB with LIMIT/OFFSET on summary rows. Memory overhead is now proportional to page size (default 25), not total conversation volume. Removed now-unused buildConversationSummary() helper (logic inlined into the SQL-side aggregation). **POST /:conversationId/flag — Parallel verification queries** Before: msgCheck and anchorCheck ran sequentially (~2× RTT). After: both run in parallel via Promise.all (1× RTT for verification). ### tests/api/routes/conversations.test.js Updated 'should return paginated conversations' test to mock the new SQL CTE response shape (pre-aggregated summary rows) instead of raw message rows. All 41 conversation tests pass.
BillChirico
added a commit
that referenced
this pull request
Mar 2, 2026
Fixes three performance bottlenecks identified in code review of recently merged features (PR #121 conversations viewer, PR #190 AI feedback). ## Changes ### migrations/004_performance_indexes.cjs (new) Four new indexes targeting hot query paths: - idx_ai_feedback_guild_created (guild_id, created_at DESC) getFeedbackTrend() and getRecentFeedback() filtered by guild_id AND created_at but only had a single-column guild_id index, forcing a full guild scan + sort on every trend/recent call. - idx_conversations_content_trgm (GIN, pg_trgm) content ILIKE '%...%' search was a sequential scan. GIN/trgm index reduces this from O(n) to O(log n * trigram matches). Requires pg_trgm extension (added idempotently). - idx_conversations_guild_created (guild_id, created_at DESC) Default 30-day listing query filters guild_id + created_at. The existing 3-column (guild_id, channel_id, created_at) composite is suboptimal when channel_id is not in the predicate. - idx_flagged_messages_guild_message (guild_id, message_id) Conversation detail + flag endpoints query flagged_messages by guild_id AND message_id = ANY(...). Existing index only covers (guild_id, status). ### src/api/routes/conversations.js **GET / — Replace in-memory pagination with SQL CTE grouping** Before: fetched up to 10,000 message rows into Node memory, grouped them in JavaScript (O(n) time + memory), then sliced for pagination. Every page request loaded the full 10k row dataset. After: single SQL query using window functions (LAG + SUM OVER) to identify conversation boundaries and aggregate summaries directly. COUNT(*) OVER() provides total count without a second query. Pagination happens at the DB with LIMIT/OFFSET on summary rows. Memory overhead is now proportional to page size (default 25), not total conversation volume. Removed now-unused buildConversationSummary() helper (logic inlined into the SQL-side aggregation). **POST /:conversationId/flag — Parallel verification queries** Before: msgCheck and anchorCheck ran sequentially (~2× RTT). After: both run in parallel via Promise.all (1× RTT for verification). ### tests/api/routes/conversations.test.js Updated 'should return paginated conversations' test to mock the new SQL CTE response shape (pre-aggregated summary rows) instead of raw message rows. All 41 conversation tests pass.
BillChirico
added a commit
that referenced
this pull request
Mar 2, 2026
Fixes three performance bottlenecks identified in code review of recently merged features (PR #121 conversations viewer, PR #190 AI feedback). ## Changes ### migrations/004_performance_indexes.cjs (new) Four new indexes targeting hot query paths: - idx_ai_feedback_guild_created (guild_id, created_at DESC) getFeedbackTrend() and getRecentFeedback() filtered by guild_id AND created_at but only had a single-column guild_id index, forcing a full guild scan + sort on every trend/recent call. - idx_conversations_content_trgm (GIN, pg_trgm) content ILIKE '%...%' search was a sequential scan. GIN/trgm index reduces this from O(n) to O(log n * trigram matches). Requires pg_trgm extension (added idempotently). - idx_conversations_guild_created (guild_id, created_at DESC) Default 30-day listing query filters guild_id + created_at. The existing 3-column (guild_id, channel_id, created_at) composite is suboptimal when channel_id is not in the predicate. - idx_flagged_messages_guild_message (guild_id, message_id) Conversation detail + flag endpoints query flagged_messages by guild_id AND message_id = ANY(...). Existing index only covers (guild_id, status). ### src/api/routes/conversations.js **GET / — Replace in-memory pagination with SQL CTE grouping** Before: fetched up to 10,000 message rows into Node memory, grouped them in JavaScript (O(n) time + memory), then sliced for pagination. Every page request loaded the full 10k row dataset. After: single SQL query using window functions (LAG + SUM OVER) to identify conversation boundaries and aggregate summaries directly. COUNT(*) OVER() provides total count without a second query. Pagination happens at the DB with LIMIT/OFFSET on summary rows. Memory overhead is now proportional to page size (default 25), not total conversation volume. Removed now-unused buildConversationSummary() helper (logic inlined into the SQL-side aggregation). **POST /:conversationId/flag — Parallel verification queries** Before: msgCheck and anchorCheck ran sequentially (~2× RTT). After: both run in parallel via Promise.all (1× RTT for verification). ### tests/api/routes/conversations.test.js Updated 'should return paginated conversations' test to mock the new SQL CTE response shape (pre-aggregated summary rows) instead of raw message rows. All 41 conversation tests pass.
BillChirico
added a commit
that referenced
this pull request
Mar 2, 2026
Fixes three performance bottlenecks identified in code review of recently merged features (PR #121 conversations viewer, PR #190 AI feedback). ## Changes ### migrations/004_performance_indexes.cjs (new) Four new indexes targeting hot query paths: - idx_ai_feedback_guild_created (guild_id, created_at DESC) getFeedbackTrend() and getRecentFeedback() filtered by guild_id AND created_at but only had a single-column guild_id index, forcing a full guild scan + sort on every trend/recent call. - idx_conversations_content_trgm (GIN, pg_trgm) content ILIKE '%...%' search was a sequential scan. GIN/trgm index reduces this from O(n) to O(log n * trigram matches). Requires pg_trgm extension (added idempotently). - idx_conversations_guild_created (guild_id, created_at DESC) Default 30-day listing query filters guild_id + created_at. The existing 3-column (guild_id, channel_id, created_at) composite is suboptimal when channel_id is not in the predicate. - idx_flagged_messages_guild_message (guild_id, message_id) Conversation detail + flag endpoints query flagged_messages by guild_id AND message_id = ANY(...). Existing index only covers (guild_id, status). ### src/api/routes/conversations.js **GET / — Replace in-memory pagination with SQL CTE grouping** Before: fetched up to 10,000 message rows into Node memory, grouped them in JavaScript (O(n) time + memory), then sliced for pagination. Every page request loaded the full 10k row dataset. After: single SQL query using window functions (LAG + SUM OVER) to identify conversation boundaries and aggregate summaries directly. COUNT(*) OVER() provides total count without a second query. Pagination happens at the DB with LIMIT/OFFSET on summary rows. Memory overhead is now proportional to page size (default 25), not total conversation volume. Removed now-unused buildConversationSummary() helper (logic inlined into the SQL-side aggregation). **POST /:conversationId/flag — Parallel verification queries** Before: msgCheck and anchorCheck ran sequentially (~2× RTT). After: both run in parallel via Promise.all (1× RTT for verification). ### tests/api/routes/conversations.test.js Updated 'should return paginated conversations' test to mock the new SQL CTE response shape (pre-aggregated summary rows) instead of raw message rows. All 41 conversation tests pass.
BillChirico
added a commit
that referenced
this pull request
Mar 2, 2026
* security: escape user content in triage prompt delimiters (#164) Add escapePromptDelimiters() to HTML-encode < and > in user-supplied message content before it is inserted between XML-style section tags in the LLM prompt. Without escaping, a crafted message containing the literal text `</messages-to-evaluate>` could break out of the user-content section and inject attacker-controlled instructions into the prompt structure. Changes: - Add escapePromptDelimiters(text) utility exported from triage-prompt.js - Apply escape to m.content and m.replyTo.content in buildConversationText() - Add 13 new tests covering the escape function and injection scenarios Closes #164 * security: escape & chars and author fields in prompt delimiters * fix(security): escape & in prompt delimiters and escape author fields - Add & → & escape first in escapePromptDelimiters() to prevent HTML entity bypass attacks (e.g. </messages-to-evaluate>) - Also escape m.author and m.replyTo.author since Discord display names are user-controlled and can contain < / > characters Addresses review feedback on PR #204. * fix: guard replyTo.content before .slice() to handle null/undefined * perf: SQL-based conversation pagination + missing DB indexes (#221) Fixes three performance bottlenecks identified in code review of recently merged features (PR #121 conversations viewer, PR #190 AI feedback). ## Changes ### migrations/004_performance_indexes.cjs (new) Four new indexes targeting hot query paths: - idx_ai_feedback_guild_created (guild_id, created_at DESC) getFeedbackTrend() and getRecentFeedback() filtered by guild_id AND created_at but only had a single-column guild_id index, forcing a full guild scan + sort on every trend/recent call. - idx_conversations_content_trgm (GIN, pg_trgm) content ILIKE '%...%' search was a sequential scan. GIN/trgm index reduces this from O(n) to O(log n * trigram matches). Requires pg_trgm extension (added idempotently). - idx_conversations_guild_created (guild_id, created_at DESC) Default 30-day listing query filters guild_id + created_at. The existing 3-column (guild_id, channel_id, created_at) composite is suboptimal when channel_id is not in the predicate. - idx_flagged_messages_guild_message (guild_id, message_id) Conversation detail + flag endpoints query flagged_messages by guild_id AND message_id = ANY(...). Existing index only covers (guild_id, status). ### src/api/routes/conversations.js **GET / — Replace in-memory pagination with SQL CTE grouping** Before: fetched up to 10,000 message rows into Node memory, grouped them in JavaScript (O(n) time + memory), then sliced for pagination. Every page request loaded the full 10k row dataset. After: single SQL query using window functions (LAG + SUM OVER) to identify conversation boundaries and aggregate summaries directly. COUNT(*) OVER() provides total count without a second query. Pagination happens at the DB with LIMIT/OFFSET on summary rows. Memory overhead is now proportional to page size (default 25), not total conversation volume. Removed now-unused buildConversationSummary() helper (logic inlined into the SQL-side aggregation). **POST /:conversationId/flag — Parallel verification queries** Before: msgCheck and anchorCheck ran sequentially (~2× RTT). After: both run in parallel via Promise.all (1× RTT for verification). ### tests/api/routes/conversations.test.js Updated 'should return paginated conversations' test to mock the new SQL CTE response shape (pre-aggregated summary rows) instead of raw message rows. All 41 conversation tests pass. * feat: channel-level quiet mode via bot mention (#173) (#213) * feat: quiet mode per-channel via bot mention (#173) - Add quietMode.js module with Redis+memory storage - Parse duration from natural language (30m, 1 hour, etc.) - Permission gated via config.quietMode.allowedRoles - Commands: quiet, unquiet, status - Suppress AI responses during quiet mode in events.js - Add quietMode section to config.json (disabled by default) - Add quietMode to configAllowlist.js for dashboard editing * test: add quiet mode tests (41 tests, all passing) * style: fix biome formatting in quietMode.js, events.js, and test * fix(web): fix ai-feedback-stats TypeScript and formatting errors * fix: gate quiet mode checks on enabled flag, validate TTL, honor maxDurationMinutes config - events.js: Wrap isQuietMode() calls in guildConfig.quietMode?.enabled check to avoid unnecessary Redis lookups and prevent stale records from suppressing AI responses when the feature is disabled (PRRT_kwDORICdSM5xdbmp, PRRT_kwDORICdSM5xdbmx) - quietMode.js: Add TTL validation in setQuiet() to guard against 0, negative, or NaN values that would error in Redis (PRRT_kwDORICdSM5xdbm3) - quietMode.js: Update parseDurationFromContent() to accept config parameter and honor guildConfig.quietMode.maxDurationMinutes. Also clamp defaultSeconds to the effective max (PRRT_kwDORICdSM5xdbm_) - configValidation.js: Add quietMode schema entry with enabled, maxDurationMinutes, and allowedRoles properties (PRRT_kwDORICdSM5xdbnH) * style: fix biome formatting in quietMode.js and ai-feedback-stats.tsx * feat: audit log improvements — CSV/JSON export and real-time WebSocket stream (#215) * feat: audit log improvements — CSV/JSON export, real-time WebSocket stream - Add GET /:id/audit-log/export endpoint (CSV and JSON, up to 10k rows) - Add /ws/audit-log WebSocket server for real-time audit entry broadcast - Refactor buildFilters() shared helper to eliminate duplication - Hook broadcastAuditEntry() into insertAuditEntry (RETURNING id+created_at) - Wire setupAuditStream/stopAuditStream into startServer/stopServer lifecycle - Add escapeCsvValue/rowsToCsv helpers with full test coverage - 30 route tests + 17 WebSocket stream tests, all green Closes #136 * fix: PR #215 review feedback - audit stream fixes - ws.ping() crash: guard with readyState check + try/catch to avoid crashing heartbeat interval when socket not OPEN - stopAuditStream race: make setupAuditStream async and await stopAuditStream() to prevent concurrent WebSocketServer creation - Query param array coercion: add typeof === 'string' checks for startDate/endDate to handle Express string|string[]|undefined - CSV CRLF quoting: add \r to RFC 4180 special-char check for proper Windows line ending handling - Test timeouts: make AUTH_TIMEOUT_MS configurable via AUDIT_STREAM_AUTH_TIMEOUT_MS env var, use 100ms in tests * feat: voice channel activity tracking — join/leave/move, leaderboard, export (#212) * feat: add voice_sessions migration (#135) * feat: add voice tracking module — join/leave/move/flush/leaderboard (#135) * feat: wire voiceStateUpdate handler into event registration (#135) * feat: add /voice command — leaderboard, stats, export subcommands (#135) * feat: add voice config defaults to config.json (#135) * feat: wire voice flush start/stop into bot lifecycle (#135) * feat: add voice to config API allowlist (#135) * fix: SQL UPDATE subquery for closeSession, fix import order (#135) * fix(voice): resolve race conditions and missing config schema - Fix openSession: update in-memory state only AFTER DB INSERT succeeds - Fix closeSession: delete from in-memory state only AFTER DB UPDATE succeeds - Fix: allow closeSession on leave/move even when feature is disabled - Fix migration: add UNIQUE constraint to partial index to prevent duplicates - Fix: move 'Voice join' log to after openSession succeeds - Add voice config to CONFIG_SCHEMA for validation --------- Co-authored-by: Bill <[email protected]> * feat(dashboard): auto-save config with 500ms debounce (#199) * feat(dashboard): replace manual save with auto-save (500ms debounce) - Remove 'Save Changes' button; saving now fires automatically 500ms after the last config change (no changes → no network call) - Add saveStatus state ('idle' | 'saving' | 'saved' | 'error') with AutoSaveStatus component showing spinner, check, or error+retry - Add isLoadingConfigRef guard so the initial fetchConfig load never triggers a spurious PATCH - Ctrl+S still works: clears debounce timer and saves immediately - Keep 'beforeunload' warning for validation errors that block save - Replace yellow unsaved-changes banner with a destructive validation error banner (only shown when save is actually blocked) - Error state shows 'Save failed' + 'Retry' button for user recovery Closes #189 * test(dashboard): add auto-save tests for ConfigEditor - No PATCH on initial config load - Validation error banner suppresses auto-save - 'Saving...' spinner visible while PATCH in-flight - 'Save failed' + Retry button on PATCH error * fix(dashboard): prevent fetchConfig from overwriting saveStatus after successful save Add skipSaveStatusReset parameter to fetchConfig so that post-save reloads preserve the 'saved' status indicator instead of immediately resetting to 'idle'. * test(dashboard): use fake timers, restore vi.stubGlobal, fix assertions, add idle/saved coverage - Replace real setTimeout delays with vi.useFakeTimers() + vi.advanceTimersByTimeAsync() for deterministic, fast debounce tests - Add afterEach cleanup: vi.unstubAllGlobals() + vi.useRealTimers() - Replace toBeTruthy() with toBeInTheDocument() for Testing Library queries - Add idle state test (no status indicator shown after load) - Add saved state test (shows 'Saved' after successful save) - Update file-level comment to list all four states --------- Co-authored-by: Bill Chirico <[email protected]> * feat: Reaction role menus (#162) (#205) * feat: reaction role menus - core module, command, event hooks, migration Implements issue #162: reaction role menus. - Add migration 004 creating reaction_role_menus and reaction_role_entries tables - Add src/modules/reactionRoles.js with DB helpers, embed builder, event handlers - Add src/commands/reactionrole.js with /reactionrole create|add|remove|delete|list - Wire handleReactionRoleAdd/Remove into registerReactionHandlers in events.js Roles are granted on reaction add and revoked on reaction remove. All mappings persist in PostgreSQL across bot restarts. * test: reaction role menus - 40 tests covering module and command - tests/modules/reactionRoles.test.js: resolveEmojiString, buildReactionRoleEmbed, all DB helpers, handleReactionRoleAdd, handleReactionRoleRemove - tests/commands/reactionrole.test.js: all 5 subcommands (create, add, remove, delete, list) including error paths and guild ownership checks - Fix biome lint: import sort order + unused import removal * fix: remove unused import in reactionrole command --------- Co-authored-by: Bill Chirico <[email protected]> * fix(security): validate GitHub owner/repo format before gh CLI call (#198) * fix(security): validate GitHub owner/repo format before gh CLI call Prevents API path traversal by validating owner/repo segments against a strict allowlist regex before interpolating them into the gh CLI invocation. Adds: - VALID_GH_NAME regex (/^[a-zA-Z0-9._-]+$/) - isValidGhRepo() helper (exported for testing) - Guard in fetchRepoEvents() — returns [] and warns on invalid input - Strengthened guard in pollGuildFeed() split logic Fixes #160 * test(security): add validation tests for GitHub owner/repo format Covers isValidGhRepo(), VALID_GH_NAME regex, and fetchRepoEvents() validation guard introduced in fix for #160. 19 new tests verify: - Valid alphanumeric/dot/hyphen/underscore names pass - Path traversal (../../etc/passwd) is rejected at both entry points - Slashes, empty strings, non-strings, spaces all rejected - Shell metacharacters (; && $()) blocked - gh CLI is NOT invoked when validation fails - warn() fires with the invalid values (observable audit trail) - Valid owner/repo still reach gh CLI unchanged * fix(security): reject pure-dot owner/repo names to prevent path traversal * test(githubFeed): add tests for pure-dot path traversal bypass --------- Co-authored-by: Bill Chirico <[email protected]> --------- Co-authored-by: Bill <[email protected]> Co-authored-by: Bill Chirico <[email protected]>
BillChirico
added a commit
that referenced
this pull request
Mar 2, 2026
* feat: add role_menu_templates migration (#135) * feat: add roleMenuTemplates module with built-ins, CRUD, and share (#135) * feat: add /rolemenu command with template CRUD, apply, share (#135) * feat: seed built-in role menu templates on startup (#135) * test: add roleMenuTemplates tests — 36 passing (#135) * test: add /rolemenu command tests — 19 passing (#135) * fix: typo hasModeatorPerms → hasModeratorPerms * perf: SQL-based conversation pagination + missing DB indexes (#221) Fixes three performance bottlenecks identified in code review of recently merged features (PR #121 conversations viewer, PR #190 AI feedback). ## Changes ### migrations/004_performance_indexes.cjs (new) Four new indexes targeting hot query paths: - idx_ai_feedback_guild_created (guild_id, created_at DESC) getFeedbackTrend() and getRecentFeedback() filtered by guild_id AND created_at but only had a single-column guild_id index, forcing a full guild scan + sort on every trend/recent call. - idx_conversations_content_trgm (GIN, pg_trgm) content ILIKE '%...%' search was a sequential scan. GIN/trgm index reduces this from O(n) to O(log n * trigram matches). Requires pg_trgm extension (added idempotently). - idx_conversations_guild_created (guild_id, created_at DESC) Default 30-day listing query filters guild_id + created_at. The existing 3-column (guild_id, channel_id, created_at) composite is suboptimal when channel_id is not in the predicate. - idx_flagged_messages_guild_message (guild_id, message_id) Conversation detail + flag endpoints query flagged_messages by guild_id AND message_id = ANY(...). Existing index only covers (guild_id, status). ### src/api/routes/conversations.js **GET / — Replace in-memory pagination with SQL CTE grouping** Before: fetched up to 10,000 message rows into Node memory, grouped them in JavaScript (O(n) time + memory), then sliced for pagination. Every page request loaded the full 10k row dataset. After: single SQL query using window functions (LAG + SUM OVER) to identify conversation boundaries and aggregate summaries directly. COUNT(*) OVER() provides total count without a second query. Pagination happens at the DB with LIMIT/OFFSET on summary rows. Memory overhead is now proportional to page size (default 25), not total conversation volume. Removed now-unused buildConversationSummary() helper (logic inlined into the SQL-side aggregation). **POST /:conversationId/flag — Parallel verification queries** Before: msgCheck and anchorCheck ran sequentially (~2× RTT). After: both run in parallel via Promise.all (1× RTT for verification). ### tests/api/routes/conversations.test.js Updated 'should return paginated conversations' test to mock the new SQL CTE response shape (pre-aggregated summary rows) instead of raw message rows. All 41 conversation tests pass. * feat: channel-level quiet mode via bot mention (#173) (#213) * feat: quiet mode per-channel via bot mention (#173) - Add quietMode.js module with Redis+memory storage - Parse duration from natural language (30m, 1 hour, etc.) - Permission gated via config.quietMode.allowedRoles - Commands: quiet, unquiet, status - Suppress AI responses during quiet mode in events.js - Add quietMode section to config.json (disabled by default) - Add quietMode to configAllowlist.js for dashboard editing * test: add quiet mode tests (41 tests, all passing) * style: fix biome formatting in quietMode.js, events.js, and test * fix(web): fix ai-feedback-stats TypeScript and formatting errors * fix: gate quiet mode checks on enabled flag, validate TTL, honor maxDurationMinutes config - events.js: Wrap isQuietMode() calls in guildConfig.quietMode?.enabled check to avoid unnecessary Redis lookups and prevent stale records from suppressing AI responses when the feature is disabled (PRRT_kwDORICdSM5xdbmp, PRRT_kwDORICdSM5xdbmx) - quietMode.js: Add TTL validation in setQuiet() to guard against 0, negative, or NaN values that would error in Redis (PRRT_kwDORICdSM5xdbm3) - quietMode.js: Update parseDurationFromContent() to accept config parameter and honor guildConfig.quietMode.maxDurationMinutes. Also clamp defaultSeconds to the effective max (PRRT_kwDORICdSM5xdbm_) - configValidation.js: Add quietMode schema entry with enabled, maxDurationMinutes, and allowedRoles properties (PRRT_kwDORICdSM5xdbnH) * style: fix biome formatting in quietMode.js and ai-feedback-stats.tsx * Fix: unterminated string in rolemenu.js * Fix: lint issues and formatting * fix: deterministic template lookup and correct roleId precedence - Add ORDER BY to getTemplateByName for deterministic results - Fix roleId precedence to preserve existing roleIds during merge - Truncate Discord embed field values to 1024 chars * fix: test assertion matches comment intent The test expected template roleId to win, but the comment said existing should take precedence. Fixed assertion to match documented behavior. * fix: filter empty roleIds and only enable when valid options exist - Filter out options with empty roleIds before saving - Only enable role menu for non-built-in templates with valid options - Add user-facing note when options are filtered * chore: remove unused _MAX_DESCRIPTION_LEN constant * fix: case-insensitive unique index for template names Use LOWER(name) in unique index to match case-insensitive queries and prevent duplicate templates differing only by case. * fix(roleMenuTemplates): add type validation for roleId and description - validateTemplateOptions now validates that optional roleId and description fields are strings when present - Update JSDoc @see reference from issue #135 (voice tracking) to issue #216 (role menu templates) - Update ON CONFLICT clause to use constraint name for consistency with the new LOWER(name) index --------- Co-authored-by: Bill <[email protected]> Co-authored-by: Bill Chirico <[email protected]>
13 tasks
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.
Resolves all CodeRabbit and CodeQL review comments from #184.
Changes
Fixes all 11 CodeRabbit actionable comments and CodeQL security warnings.