feat(redis): Comprehensive Redis integration (#177)#220
Conversation
Phase 1 of #177: Core infrastructure - src/redis.js: Centralized Redis client with graceful degradation, reconnect strategy, connection stats, and hit/miss tracking - src/utils/cache.js: High-level cache helpers (get/set/del/pattern/getOrSet) with Redis backend and in-memory LRU fallback - Configurable TTLs via environment variables - Non-blocking SCAN for pattern deletes
- Update index.js to initialize Redis on startup and close on shutdown - Add stopCacheCleanup() to graceful shutdown sequence - Update redisClient.js to be a thin re-export from centralized redis.js - Add Redis stats (connected, hit/miss, hitRate) to /health endpoint Part of #177
- discordCache.js: Cached fetchers for channels, roles, members, guild channels with TTL-based expiration and cache invalidation helpers - reputationCache.js: XP/level/leaderboard/rank caching with proper invalidation on XP gain events Part of #177
- Replace client.channels.fetch() with fetchChannelCached() in 12 modules: scheduler, reminderHandler, pollHandler, challengeScheduler, githubFeed, moderation, rateLimit, linkFilter, reviewHandler, starboard, welcome, triage-respond - Add leaderboard DB query caching in community.js (TTL 5min) - Add reputation cache invalidation on XP gain in reputation.js - All fetches use Redis when available, in-memory LRU fallback otherwise Part of #177
- Add cache invalidation listeners in config-listeners.js for welcome, starboard, and reputation config changes - Add comprehensive tests: redis.test.js (8 tests), cache.test.js (20 tests), discordCache.test.js (10 tests), reputationCache.test.js (6 tests) - Update 16 existing test files with discordCache mock to prevent import resolution errors from fetchChannelCached - Fix community.test.js to mock cacheGetOrSet pass-through - Update redisClient.coverage.test.js for US spelling - Update config-listeners.test.js for new listener count (8→11) - Update welcome.coverage.test.js for graceful null return behavior All 2869 passing tests pass. 6 pre-existing failures in triage.coverage.test.js (unrelated to this PR). Part of #177
- redisRateLimit.js: Sliding window rate limiting using Redis INCR/PTTL with automatic fallback to in-memory when Redis is unavailable - Atomic multi/exec pipeline for consistent counting - Same API contract as existing rateLimit.js (drop-in replacement) - 5 tests covering Redis path, fallback, 429 response, error recovery Part of #177
- Add Redis service to railway.toml (free plan) - Add Redis container to docker-compose.yml with healthcheck - Add REDIS_URL env vars to bot and web services - Add redisdata volume for persistence
multi().exec() returns [[err, value], ...] tuples. Now destructures and checks each command's error before using its value; falls back to in-memory rate limiter on pipeline failure. Also removes the unused windowSec variable.
Only cacheGetOrSet and TTL are used; cacheGet and cacheSet were dead imports.
reputationCache was deleting the base key 'leaderboard:${guildId}'
but the paginated API caches under 'leaderboard:${guildId}:${page}:${limit}'.
Now uses cacheDelPattern('leaderboard:${guildId}:*') consistently so
all paginated leaderboard entries are properly invalidated.
An import statement had leaked into the JSDoc block, creating invalid comment syntax. Cleaned up the header to be proper JSDoc.
Cache was being invalidated BEFORE the level update write, allowing stale data to be re-cached in the gap. Now invalidates AFTER all reputation writes (XP upsert + level update) are complete.
The in-memory fallback for cacheDelPattern converted glob patterns to regex without escaping special regex chars like '.', '+', etc. Now escapes all metacharacters before substituting '*' and '?'.
flushdb() wipes ALL Redis keys — dangerous in shared environments. Now scans and deletes only known app-prefixed keys (rl:*, reputation:*, rank:*, leaderboard:*, discord:*, config:*, session:*).
If client was non-null when _resetRedis() was called, the existing connection was silently abandoned. Now calls client.quit() (with disconnect() fallback) before resetting state.
Channel metadata was stored in Redis but never returned when the
DJS cache missed — the code only re-checked DJS. Now returns the
cached {id, name, type, guildId} object directly, avoiding an
unnecessary Discord API call.
cacheDelPattern was dynamically imported via await import('./cache.js')
but cache.js is already statically imported at the top. Added it to
the existing static import instead.
If an assertion failed before vi.useRealTimers() was called, fake timers would leak into subsequent tests. Wrapped in try/finally to ensure timers are always restored.
Tests were using the old base key 'leaderboard:${guildId}' format.
Updated to use paginated keys matching the new pattern-based invalidation.
…urn type
- config-listeners: change leaderboard invalidation pattern from
`leaderboard:${guildId}:*` to `leaderboard:${guildId}*` so it
matches the actual stored key format (no colon suffix)
- discordCache: fetchChannelCached no longer returns a plain metadata
object when a Redis cache hit is found. Cached data is used only for
the existence recheck; function always falls through to Discord API
fetch so callers receive a real Discord.js Channel (or null)
Fixes review threads PRRT_kwDORICdSM5xceQX, PRRT_kwDORICdSM5xchrZ
_resetRedisClient() was calling the async _resetRedis() without await, meaning callers had no way to wait for the reset to complete, leading to potential in-flight connection races in tests and shutdown sequences. Fixes review thread PRRT_kwDORICdSM5xchrL
…tener keys redis.test.js: - Make beforeEach/afterEach async and await _resetRedis() to prevent cross-test flakiness from in-flight connections config-listeners.test.js: - Add explicit toContain assertions for welcome.*, starboard.*, and reputation.* so the test will fail if a cache-invalidation listener is removed Fixes review threads PRRT_kwDORICdSM5xchrQ, PRRT_kwDORICdSM5xchrU
- Switch server.js and community.js from in-memory rateLimit to redisRateLimit for distributed rate limiting across bot instances - Wire /rank command to use getReputationCached + setReputationCache + getRankCached, avoiding redundant DB queries on hot command paths - Wire /leaderboard command to use getLeaderboardCached (TTL cached) - Extend fetchChannelCached to commands/welcome.js, commands/review.js, modules/welcomeOnboarding.js, and modules/reviewHandler.js - Update tests: mock reputationCache in leaderboard/rank tests to isolate cache layer; mock discordCache in review/welcomeOnboarding tests; restore ioredis constructor mock syntax broken by biome formatter Closes #177
|
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 a comprehensive caching and Redis-backed rate limiting infrastructure across the application. It replaces in-process rate limiting with Redis-backed rate limiting, adds caching layers for leaderboard, rank, and channel lookups, and updates cache invalidation patterns and test mocks accordingly. Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| Filename | Overview |
|---|---|
| src/index.js | CRITICAL: Merge conflict resolution removed multiple features (aliases, perf monitoring, bot status, backups, temp roles, webhooks) |
| src/modules/triage-respond.js | CRITICAL: Removed AI feedback reactions, protected target checks, and webhook notifications |
| src/api/server.js | Switched to redisRateLimit for distributed rate limiting with proper fallback |
| src/api/routes/community.js | Switched to redisRateLimit with appropriate keyPrefix for community endpoints |
| src/commands/rank.js | Added reputation and rank caching with proper cache-aside pattern and fallback to DB |
| src/commands/leaderboard.js | Added leaderboard caching using getLeaderboardCached wrapper |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
Start[API Request / Command Execution] --> RateLimit{Redis Available?}
RateLimit -->|Yes| RedisRL[Redis Rate Limiter<br/>Sliding Window]
RateLimit -->|No| MemoryRL[In-Memory Fallback]
RedisRL --> CheckLimit{Under Limit?}
MemoryRL --> CheckLimit
CheckLimit -->|No| Reject[429 Too Many Requests]
CheckLimit -->|Yes| Handler[Request Handler]
Handler --> NeedData{Need Data?}
NeedData -->|Reputation| RepCache{Cache Hit?}
NeedData -->|Leaderboard| LbCache{Cache Hit?}
NeedData -->|Discord API| DiscCache{Cache Hit?}
RepCache -->|Hit| ReturnCache1[Return Cached]
RepCache -->|Miss| QueryDB1[Query PostgreSQL]
QueryDB1 --> CacheDB1[Cache Result]
CacheDB1 --> ReturnCache1
LbCache -->|Hit| ReturnCache2[Return Cached]
LbCache -->|Miss| QueryDB2[Query PostgreSQL]
QueryDB2 --> CacheDB2[Cache Result]
CacheDB2 --> ReturnCache2
DiscCache -->|Hit| ReturnCache3[Fetch from Discord API]
DiscCache -->|Miss| QueryAPI[Call Discord API]
QueryAPI --> CacheAPI[Cache Metadata]
CacheAPI --> ReturnCache3
ReturnCache1 --> Response[Generate Response]
ReturnCache2 --> Response
ReturnCache3 --> Response
Response --> End[Return to User]
Last reviewed commit: e0c1bf4
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/leaderboard.js`:
- Around line 34-44: The leaderboard code uses interaction.guildId directly as a
cache key and SQL filter which can be undefined; add a guild-context guard at
the start of the command handler to abort early (e.g., send an ephemeral error
like "This command must be used in a server") and do not call
getLeaderboardCached or pool.query when interaction.guildId is falsy. Locate the
call to getLeaderboardCached and the SQL query (references:
getLeaderboardCached, pool.query and the SQL selecting from reputation) and wrap
them behind a check for interaction.guildId (or throw/return before reaching
them) so you never create or read from a "leaderboard:undefined" cache bucket.
In `@src/modules/triage-respond.js`:
- Around line 354-355: The code currently swallows all exceptions from
fetchChannelCached(client, channelId) by using .catch(() => null); change this
to a try/catch so you can log unexpected errors while still falling back to
null: call fetchChannelCached(client, channelId) inside a try block, assign the
result to channel, and in catch inspect the error and call the existing logger
(e.g., processLogger or logger) to record the error with context (include
channelId and client/guild context) before setting channel = null so guildId =
channel?.guildId continues to work; keep any expected-not-found handling as a
null fallback but do not silence other exceptions.
In `@src/utils/discordCache.js`:
- Around line 37-40: The debug message currently hardcodes "Redis cache hit..."
which is misleading when the metadata may come from an in-memory fallback;
update the debug(...) call that logs "Redis cache hit for channel metadata;
fetching real channel from API" (the debug invocation that uses channelId) to
use neutral wording such as "cache hit for channel metadata; fetching real
channel from API" or include the actual source if available (e.g., add a source
variable like cacheSource and log it along with channelId) so logs no longer
misattribute hits to Redis.
In `@tests/commands/rank.test.js`:
- Around line 26-30: Add a new test in tests/commands/rank.test.js that
simulates a cache hit by having the mocked getReputationCached return a
reputation row (instead of null) and call the /rank command handler in
src/commands/rank.js; assert that the code path that queries the database for
reputation is not invoked (i.e., the repository/DB method used by rank.js to
fetch reputation is not called) and that the response uses the cached value—use
the existing mocks getReputationCached, getRankCached, and setReputationCache to
set up and verify the behavior.
In `@tests/modules/welcomeOnboarding.test.js`:
- Line 16: The current expression chains .catch() directly after an optional
call to client?.channels?.fetch?.(channelId), which can be undefined and cause a
TypeError; update the mock so you first obtain the result of
client?.channels?.fetch?.(channelId) into a variable (or use an additional
optional check) and only call .catch() if that result is a Promise, e.g. get
const result = client?.channels?.fetch?.(channelId) and return result ?
result.catch(() => null) : Promise.resolve(null); ensure you reference the
existing client, channels, fetch and channelId symbols when applying the change.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (17)
src/api/routes/community.jssrc/api/server.jssrc/commands/leaderboard.jssrc/commands/rank.jssrc/commands/review.jssrc/commands/welcome.jssrc/config-listeners.jssrc/modules/reviewHandler.jssrc/modules/triage-respond.jssrc/modules/welcomeOnboarding.jssrc/utils/discordCache.jssrc/utils/reputationCache.jstests/commands/leaderboard.test.jstests/commands/rank.test.jstests/commands/review.test.jstests/config-listeners.test.jstests/modules/welcomeOnboarding.test.js
💤 Files with no reviewable changes (1)
- src/utils/reputationCache.js
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Greptile Review
- GitHub Check: Docker Build Validation
🧰 Additional context used
📓 Path-based instructions (6)
**/*.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:
tests/commands/leaderboard.test.jssrc/modules/triage-respond.jssrc/commands/welcome.jssrc/commands/review.jssrc/modules/reviewHandler.jstests/commands/review.test.jssrc/modules/welcomeOnboarding.jssrc/commands/leaderboard.jssrc/api/routes/community.jstests/commands/rank.test.jssrc/config-listeners.jstests/modules/welcomeOnboarding.test.jssrc/commands/rank.jstests/config-listeners.test.jssrc/utils/discordCache.jssrc/api/server.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/commands/leaderboard.test.jstests/commands/review.test.jstests/commands/rank.test.jstests/modules/welcomeOnboarding.test.jstests/config-listeners.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/modules/triage-respond.jssrc/commands/welcome.jssrc/commands/review.jssrc/modules/reviewHandler.jssrc/modules/welcomeOnboarding.jssrc/commands/leaderboard.jssrc/api/routes/community.jssrc/config-listeners.jssrc/commands/rank.jssrc/utils/discordCache.jssrc/api/server.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/triage-respond.jssrc/modules/reviewHandler.jssrc/modules/welcomeOnboarding.js
src/commands/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/commands/*.js: Slash commands must exportdata(SlashCommandBuilder) andexecute(interaction)function. ExportadminOnly = truefor mod-only commands
Moderation commands must follow the shared pattern:deferReply({ ephemeral: true }), validate inputs,sendDmNotification(), execute Discord action,createCase(),sendModLogEmbed(),checkEscalation()
Files:
src/commands/welcome.jssrc/commands/review.jssrc/commands/leaderboard.jssrc/commands/rank.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/community.js
🧠 Learnings (3)
📚 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/commands/*.js : Moderation commands must follow the shared pattern: `deferReply({ ephemeral: true })`, validate inputs, `sendDmNotification()`, execute Discord action, `createCase()`, `sendModLogEmbed()`, `checkEscalation()`
Applied to files:
src/commands/welcome.jssrc/commands/review.jssrc/modules/welcomeOnboarding.jssrc/commands/leaderboard.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/commands/*.js : Slash commands must export `data` (SlashCommandBuilder) and `execute(interaction)` function. Export `adminOnly = true` for mod-only commands
Applied to files:
src/commands/review.jssrc/commands/leaderboard.jssrc/commands/rank.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/config-listeners.js
🧬 Code graph analysis (12)
src/modules/triage-respond.js (1)
src/utils/discordCache.js (2)
channel(45-45)fetchChannelCached(22-65)
src/commands/welcome.js (1)
src/utils/discordCache.js (1)
fetchChannelCached(22-65)
src/commands/review.js (3)
src/utils/discordCache.js (1)
fetchChannelCached(22-65)tests/commands/review.test.js (1)
interaction(200-224)src/modules/reviewHandler.js (1)
reviewChannelId(302-302)
src/modules/reviewHandler.js (1)
src/utils/discordCache.js (2)
channel(45-45)fetchChannelCached(22-65)
src/modules/welcomeOnboarding.js (1)
src/utils/discordCache.js (1)
fetchChannelCached(22-65)
src/commands/leaderboard.js (1)
src/utils/reputationCache.js (1)
getLeaderboardCached(59-61)
src/api/routes/community.js (1)
src/api/middleware/redisRateLimit.js (1)
redisRateLimit(22-78)
src/config-listeners.js (1)
src/utils/cache.js (1)
cacheDelPattern(178-211)
tests/modules/welcomeOnboarding.test.js (1)
src/modules/welcomeOnboarding.js (1)
introChannel(177-177)
src/commands/rank.js (2)
src/utils/reputationCache.js (3)
getReputationCached(18-21)setReputationCache(31-35)getRankCached(71-73)src/commands/leaderboard.js (2)
pool(33-33)rows(34-44)
src/utils/discordCache.js (2)
src/utils/cache.js (1)
cacheGet(76-115)src/logger.js (1)
debug(224-226)
src/api/server.js (2)
src/api/middleware/redisRateLimit.js (1)
redisRateLimit(22-78)tests/api/middleware/redisRateLimit.test.js (1)
redisRateLimit(27-27)
🔇 Additional comments (13)
src/api/server.js (1)
9-9: LGTM!The Redis rate limiter integration is correctly wired:
- Import path is valid
- Type annotation properly updated
- Default configuration (15min window, 100 requests,
rlprefix) is reasonable for the global API limiter- Existing cleanup logic via
destroy()remains compatible with the new middlewareAlso applies to: 18-19, 65-66
src/api/routes/community.js (1)
15-15: LGTM!The Redis rate limiter configuration is well-designed:
- Aggressive 30 req/min limit is appropriate for public, unauthenticated endpoints
keyPrefix: 'rl:community'correctly namespaces these rate limit keys separately from the globalrlprefix inserver.js, preventing cross-contamination between global and route-specific limits- The limiter is properly applied to all routes via
router.use()Also applies to: 20-25
src/config-listeners.js (1)
106-109: Scoped leaderboard cache invalidation looks correct.The move to
leaderboard:${guildId}:*tightens invalidation to per-guild leaderboard keys and avoids broader unintended matches.tests/config-listeners.test.js (1)
449-450: Test expectation update is aligned with runtime behavior.This assertion now correctly tracks the narrowed leaderboard invalidation pattern.
tests/commands/leaderboard.test.js (1)
17-19: Cache mock strategy is solid for deterministic command tests.Executing the provided factory directly is a clean way to bypass cache state in this suite.
src/commands/review.js (1)
163-171: Good fallback-preserving migration tofetchChannelCached.This keeps the command resilient: it prefers configured channel resolution and cleanly falls back to the interaction channel when unavailable.
src/commands/welcome.js (1)
40-61: Channel fetch helper wiring is correct and safely guarded.Using
fetchChannelCachedhere is a good fit, and theisTextBasedchecks still prevent invalid send targets.tests/commands/review.test.js (1)
30-38: Mock coverage fordiscordCacheintegration is well structured.This prevents unrelated cache-layer behavior from destabilizing
/reviewcommand tests while still exercising the channel-resolution path.src/modules/welcomeOnboarding.js (2)
9-9: Good cache-aware channel resolution integration.Using
fetchChannelCachedhere keeps channel lookup behavior consistent with the new shared Discord cache path and preserves existing null/text-channel safety checks.Also applies to: 177-177
122-122: Role resolution fallback looks solid.The cache-first role lookup with API fetch-on-miss is concise and safe for onboarding role assignment flows.
tests/modules/welcomeOnboarding.test.js (1)
95-100: Nice fixture update forinteraction.client.channels.This aligns the test interaction shape with the new production lookup path (
fetchChannelCached(interaction.client, ...)).src/modules/reviewHandler.js (1)
13-13: Good consolidation on shared channel caching helper.Switching both call sites to
fetchChannelCachedimproves consistency and keeps the same graceful-failure behavior.Also applies to: 127-127, 307-307
src/commands/rank.js (1)
14-18: Cache-first reputation/rank integration is implemented cleanly.The fallback flow (cache → DB → cache write) and rank cache wrapper are wired correctly while preserving command response behavior.
Also applies to: 51-63, 78-87
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
BillChirico
force-pushed
the
feat/issue-177
branch
from
84a227b to
e0c1bf4
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
src/index.js:227
- This change removes the alias → canonical command resolution flow (and the corresponding permission lookup), which effectively disables per-guild command aliases while keeping the rest of the command execution pipeline intact. If command aliases are still a supported feature, restore alias resolution (and permission checks against the resolved target), otherwise consider removing the alias feature end-to-end (incl. DB load / docs) in a separate PR to avoid an unexpected breaking behavior change in a Redis-focused PR.
// Permission check
const guildConfig = getConfig(interaction.guildId);
if (!hasPermission(member, commandName, guildConfig)) {
const permLevel = guildConfig.permissions?.allowedCommands?.[commandName] || 'administrator';
await safeReply(interaction, {
content: getPermissionError(commandName, permLevel),
ephemeral: true,
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Execute command from collection | ||
| const command = client.commands.get(resolvedCommandName); | ||
| const command = client.commands.get(commandName); |
There was a problem hiding this comment.
This change removes the alias → canonical command resolution flow (and the corresponding permission lookup), which effectively disables per-guild command aliases while keeping the rest of the command execution pipeline intact. If command aliases are still a supported feature, restore alias resolution (and permission checks against the resolved target), otherwise consider removing the alias feature end-to-end (incl. DB load / docs) in a separate PR to avoid an unexpected breaking behavior change in a Redis-focused PR.
| @@ -139,31 +106,6 @@ export async function sendModerationLog(client, classification, snapshot, channe | |||
| // Find target messages from the snapshot | |||
| const targets = snapshot.filter((m) => classification.targetMessageIds?.includes(m.messageId)); | |||
|
|
|||
There was a problem hiding this comment.
The protected-target gating logic was removed from sendModerationLog, so moderation logs will now be generated even when the target is an admin/mod/owner (previously explicitly skipped). If that protection is still a requirement, reintroduce the check (ideally still allowing an explicit config override) so sensitive moderation actions aren’t logged for protected roles.
| // Skip logging when any protected target (admin/mod/owner/staff) is involved, | |
| // unless explicitly overridden via config.triage.logProtectedTargets === true. | |
| const allowProtectedLogs = config.triage?.logProtectedTargets === true; | |
| const hasProtectedTarget = targets.some( | |
| (t) => t?.isProtected || t?.isStaff || t?.isMod || t?.isAdmin || t?.isOwner, | |
| ); | |
| if (!allowProtectedLogs && hasProtectedTarget) { | |
| return; | |
| } |
|
|
||
| try { | ||
| const channel = await client.channels.fetch(review.channel_id).catch(() => null); | ||
| const channel = await fetchChannelCached(client, review.channel_id); |
There was a problem hiding this comment.
Error-handling for fetchChannelCached is inconsistent in this PR (e.g., triage-respond wraps it with .catch(() => null) but this call site does not). Either standardize on catching/recovering at each call site or guarantee (and document) that fetchChannelCached never rejects so callers can safely await it without try/catch.
| const channel = await fetchChannelCached(client, review.channel_id); | |
| const channel = await fetchChannelCached(client, review.channel_id).catch(() => null); |
| // Fetch reputation row (cached) | ||
| const cachedRep = await getReputationCached(interaction.guildId, target.id); | ||
| let repRow = cachedRep; | ||
| if (!repRow) { | ||
| const { rows } = await pool.query( | ||
| 'SELECT xp, level, messages_count FROM reputation WHERE guild_id = $1 AND user_id = $2', | ||
| [interaction.guildId, target.id], | ||
| ); | ||
| repRow = rows[0] ?? null; | ||
| if (repRow) { | ||
| await setReputationCache(interaction.guildId, target.id, repRow); | ||
| } | ||
| } |
There was a problem hiding this comment.
The new cache branches (rep cache hit vs miss; rank cache factory invocation) aren’t directly asserted in tests. Add coverage in tests/commands/rank.test.js to validate: (1) on cache hit, pool.query is not called and setReputationCache is not called; (2) on cache miss, pool.query is called and setReputationCache is called with the fetched row; (3) getRankCached bypasses the DB query when returning a cached value.
| // Rank position in guild (cached) | ||
| const rank = await getRankCached(interaction.guildId, target.id, async () => { | ||
| const rankRow = await pool.query( | ||
| `SELECT COUNT(*) + 1 AS rank | ||
| FROM reputation | ||
| WHERE guild_id = $1 AND xp > $2`, | ||
| [interaction.guildId, xp], | ||
| ); | ||
| return { rank: Number(rankRow.rows[0]?.rank ?? 1) }; | ||
| }).then((r) => r?.rank ?? 1); |
There was a problem hiding this comment.
The new cache branches (rep cache hit vs miss; rank cache factory invocation) aren’t directly asserted in tests. Add coverage in tests/commands/rank.test.js to validate: (1) on cache hit, pool.query is not called and setReputationCache is not called; (2) on cache miss, pool.query is called and setReputationCache is called with the fetched row; (3) getRankCached bypasses the DB query when returning a cached value.
| const rank = await getRankCached(interaction.guildId, target.id, async () => { | ||
| const rankRow = await pool.query( | ||
| `SELECT COUNT(*) + 1 AS rank | ||
| FROM reputation | ||
| WHERE guild_id = $1 AND xp > $2`, | ||
| [interaction.guildId, xp], | ||
| ); | ||
| return { rank: Number(rankRow.rows[0]?.rank ?? 1) }; | ||
| }).then((r) => r?.rank ?? 1); |
There was a problem hiding this comment.
Mixing await with a chained .then(...) makes the flow harder to read and slightly harder to debug. Prefer awaiting the cached result into a variable and then deriving rank from that value in a separate statement.
| const rows = await getLeaderboardCached(interaction.guildId, async () => { | ||
| const result = await pool.query( | ||
| `SELECT user_id, xp, level | ||
| FROM reputation | ||
| WHERE guild_id = $1 | ||
| ORDER BY xp DESC | ||
| LIMIT 10`, | ||
| [interaction.guildId], | ||
| ); | ||
| return result.rows; | ||
| }); |
There was a problem hiding this comment.
Add/update tests in tests/commands/leaderboard.test.js to assert the caching wrapper is used (e.g., getLeaderboardCached called with the guild id) and to validate behavior on cache hit vs miss (DB query executed only when the cache factory runs). This ensures the new integration actually prevents redundant DB calls.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| startConversationCleanup, | ||
| stopConversationCleanup, | ||
| } from './modules/ai.js'; | ||
| import { startScheduledBackups, stopScheduledBackups } from './modules/backup.js'; | ||
| import { startBotStatus, stopBotStatus } from './modules/botStatus.js'; | ||
| import { loadAliasesFromDb, resolveAlias } from './modules/commandAliases.js'; | ||
| import { getConfig, loadConfig } from './modules/config.js'; | ||
| import { registerEventHandlers } from './modules/events.js'; | ||
| import { startGithubFeed, stopGithubFeed } from './modules/githubFeed.js'; | ||
| import { checkMem0Health, markUnavailable } from './modules/memory.js'; | ||
| import { startTempbanScheduler, stopTempbanScheduler } from './modules/moderation.js'; | ||
| import { loadOptOuts } from './modules/optout.js'; | ||
| import { PerformanceMonitor } from './modules/performanceMonitor.js'; | ||
| import { startScheduler, stopScheduler } from './modules/scheduler.js'; | ||
| import { startTempRoleScheduler, stopTempRoleScheduler } from './modules/tempRoleHandler.js'; | ||
| import { startTriage, stopTriage } from './modules/triage.js'; | ||
| import { startVoiceFlush, stopVoiceFlush } from './modules/voice.js'; | ||
| import { fireEventAllGuilds } from './modules/webhookNotifier.js'; | ||
| import { closeRedisClient as closeRedis, initRedis } from './redis.js'; |
There was a problem hiding this comment.
CRITICAL: Merge conflict resolution removed imports for recently-merged features (backup, botStatus, commandAliases, performanceMonitor, tempRoleHandler). These modules still exist in the codebase but are no longer being initialized.
The commit message "fix: resolve merge conflicts with main" suggests this was unintentional. These features need to be restored:
- Command aliases (enhancement: refactor events.js into single InteractionCreate dispatcher #166/feat: custom command aliases per guild (#166) #203)
- Performance monitoring (feat: bot performance monitoring #218)
- Bot status/activity (🎯 Community Engagement Features #40/feat: bot status/custom activity #210)
- Server backups (feat: server config backup and restore #209)
- Temp role assignment (test: end-to-end tests for web dashboard with Playwright #128/feat: Temporary role assignment (#128) #208)
| startConversationCleanup, | |
| stopConversationCleanup, | |
| } from './modules/ai.js'; | |
| import { startScheduledBackups, stopScheduledBackups } from './modules/backup.js'; | |
| import { startBotStatus, stopBotStatus } from './modules/botStatus.js'; | |
| import { loadAliasesFromDb, resolveAlias } from './modules/commandAliases.js'; | |
| import { getConfig, loadConfig } from './modules/config.js'; | |
| import { registerEventHandlers } from './modules/events.js'; | |
| import { startGithubFeed, stopGithubFeed } from './modules/githubFeed.js'; | |
| import { checkMem0Health, markUnavailable } from './modules/memory.js'; | |
| import { startTempbanScheduler, stopTempbanScheduler } from './modules/moderation.js'; | |
| import { loadOptOuts } from './modules/optout.js'; | |
| import { PerformanceMonitor } from './modules/performanceMonitor.js'; | |
| import { startScheduler, stopScheduler } from './modules/scheduler.js'; | |
| import { startTempRoleScheduler, stopTempRoleScheduler } from './modules/tempRoleHandler.js'; | |
| import { startTriage, stopTriage } from './modules/triage.js'; | |
| import { startVoiceFlush, stopVoiceFlush } from './modules/voice.js'; | |
| import { fireEventAllGuilds } from './modules/webhookNotifier.js'; | |
| import { closeRedisClient as closeRedis, initRedis } from './redis.js'; | |
| } from './modules/ai.js'; | |
| import { startScheduledBackups, stopScheduledBackups } from './modules/backup.js'; | |
| import { startBotStatus, stopBotStatus } from './modules/botStatus.js'; | |
| import { loadAliasesFromDb, resolveAlias } from './modules/commandAliases.js'; | |
| import { getConfig, loadConfig } from './modules/config.js'; | |
| import { registerEventHandlers } from './modules/events.js'; | |
| import { startGithubFeed, stopGithubFeed } from './modules/githubFeed.js'; | |
| import { checkMem0Health, markUnavailable } from './modules/memory.js'; | |
| import { startTempbanScheduler, stopTempbanScheduler } from './modules/moderation.js'; | |
| import { loadOptOuts } from './modules/optout.js'; | |
| import { PerformanceMonitor } from './modules/performanceMonitor.js'; | |
| import { startScheduler, stopScheduler } from './modules/scheduler.js'; | |
| import { startTempRoleScheduler, stopTempRoleScheduler } from './modules/tempRoleHandler.js'; | |
| import { startTriage, stopTriage } from './modules/triage.js'; | |
| import { startVoiceFlush, stopVoiceFlush } from './modules/voice.js'; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/index.js
Line: 43-56
Comment:
CRITICAL: Merge conflict resolution removed imports for recently-merged features (backup, botStatus, commandAliases, performanceMonitor, tempRoleHandler). These modules still exist in the codebase but are no longer being initialized.
The commit message "fix: resolve merge conflicts with main" suggests this was unintentional. These features need to be restored:
- Command aliases (#166/#203)
- Performance monitoring (#218)
- Bot status/activity (#40/#210)
- Server backups (#209)
- Temp role assignment (#128/#208)
```suggestion
} from './modules/ai.js';
import { startScheduledBackups, stopScheduledBackups } from './modules/backup.js';
import { startBotStatus, stopBotStatus } from './modules/botStatus.js';
import { loadAliasesFromDb, resolveAlias } from './modules/commandAliases.js';
import { getConfig, loadConfig } from './modules/config.js';
import { registerEventHandlers } from './modules/events.js';
import { startGithubFeed, stopGithubFeed } from './modules/githubFeed.js';
import { checkMem0Health, markUnavailable } from './modules/memory.js';
import { startTempbanScheduler, stopTempbanScheduler } from './modules/moderation.js';
import { loadOptOuts } from './modules/optout.js';
import { PerformanceMonitor } from './modules/performanceMonitor.js';
import { startScheduler, stopScheduler } from './modules/scheduler.js';
import { startTempRoleScheduler, stopTempRoleScheduler } from './modules/tempRoleHandler.js';
import { startTriage, stopTriage } from './modules/triage.js';
import { startVoiceFlush, stopVoiceFlush } from './modules/voice.js';
```
How can I resolve this? If you propose a fix, please make it concise.| @@ -270,7 +230,7 @@ client.on('interactionCreate', async (interaction) => { | |||
| } | |||
There was a problem hiding this comment.
Command alias resolution was removed but the commandAliases module still exists. Aliases allow per-guild custom command names and should be restored.
| try { | |
| info('Slash command received', { command: commandName, user: interaction.user.tag }); | |
| // Resolve alias → target command (per-guild custom aliases). | |
| // Do this early so permission checks and command lookup both use the | |
| // resolved (canonical) command name rather than the alias name. | |
| const resolvedCommandName = resolveAlias(interaction.guildId, commandName) || commandName; | |
| // Permission check (using resolved command name so alias permissions mirror target) | |
| const guildConfig = getConfig(interaction.guildId); | |
| if (!hasPermission(member, resolvedCommandName, guildConfig)) { | |
| const permLevel = | |
| guildConfig.permissions?.allowedCommands?.[resolvedCommandName] || 'administrator'; | |
| await safeReply(interaction, { | |
| content: getPermissionError(commandName, permLevel), |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/index.js
Line: 217-230
Comment:
Command alias resolution was removed but the `commandAliases` module still exists. Aliases allow per-guild custom command names and should be restored.
```suggestion
try {
info('Slash command received', { command: commandName, user: interaction.user.tag });
// Resolve alias → target command (per-guild custom aliases).
// Do this early so permission checks and command lookup both use the
// resolved (canonical) command name rather than the alias name.
const resolvedCommandName = resolveAlias(interaction.guildId, commandName) || commandName;
// Permission check (using resolved command name so alias permissions mirror target)
const guildConfig = getConfig(interaction.guildId);
if (!hasPermission(member, resolvedCommandName, guildConfig)) {
const permLevel =
guildConfig.permissions?.allowedCommands?.[resolvedCommandName] || 'administrator';
await safeReply(interaction, {
content: getPermissionError(commandName, permLevel),
```
How can I resolve this? If you propose a fix, please make it concise.| stopTriage(); | ||
| stopConversationCleanup(); | ||
| stopTempbanScheduler(); | ||
| stopTempRoleScheduler(); | ||
| stopScheduler(); | ||
| stopGithubFeed(); |
There was a problem hiding this comment.
Missing shutdown calls for removed features (tempRoleScheduler, scheduledBackups, botStatus, voiceFlush, perfMonitor).
| stopTriage(); | |
| stopConversationCleanup(); | |
| stopTempbanScheduler(); | |
| stopTempRoleScheduler(); | |
| stopScheduler(); | |
| stopGithubFeed(); | |
| stopTriage(); | |
| stopConversationCleanup(); | |
| stopTempbanScheduler(); | |
| stopTempRoleScheduler(); | |
| stopScheduler(); | |
| stopGithubFeed(); | |
| stopScheduledBackups(); | |
| perfMonitor.stop(); | |
| stopBotStatus(); | |
| stopVoiceFlush(); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/index.js
Line: 282-286
Comment:
Missing shutdown calls for removed features (tempRoleScheduler, scheduledBackups, botStatus, voiceFlush, perfMonitor).
```suggestion
stopTriage();
stopConversationCleanup();
stopTempbanScheduler();
stopTempRoleScheduler();
stopScheduler();
stopGithubFeed();
stopScheduledBackups();
perfMonitor.stop();
stopBotStatus();
stopVoiceFlush();
```
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/modules/triage-respond.js
Line: 6-13
Comment:
Removed imports for AI feedback reactions and protected target checks. These features (aiFeedback module for 👍/👎 reactions, isProtectedTarget to prevent moderating admins/mods) were present in the base branch and should not be removed as part of Redis integration.
```suggestion
import { EmbedBuilder } from 'discord.js';
import { info, error as logError, warn } from '../logger.js';
import { buildDebugEmbed, extractStats, logAiUsage } from '../utils/debugFooter.js';
import { fetchChannelCached } from '../utils/discordCache.js';
import { safeSend } from '../utils/safeSend.js';
import { splitMessage } from '../utils/splitMessage.js';
import { addToHistory } from './ai.js';
import { FEEDBACK_EMOJI, registerAiMessage } from './aiFeedback.js';
import { isProtectedTarget } from './moderation.js';
import { resolveMessageId, sanitizeText } from './triage-filter.js';
import { fireEvent } from './webhookNotifier.js';
```
How can I resolve this? If you propose a fix, please make it concise. |
Summary
Completes feature #177 — comprehensive Redis integration for caching, distributed rate limiting, and graceful degradation.
What's already implemented (prior commits on this branch)
src/redis.js) with singleton pattern, graceful degradation when Redis is unavailable, auto-reconnect with backoff, and metrics tracking (hit rate, connections)src/utils/cache.js) — Redis-backed with in-memory LRU fallback; transparent to callerssrc/utils/discordCache.js) —fetchChannelCached,fetchGuildChannelsCached,fetchGuildRolesCached,fetchMemberCachedwith appropriate TTLssrc/utils/reputationCache.js) —getReputationCached,getLeaderboardCached,getRankCachedwith invalidation hookssrc/api/middleware/redisRateLimit.js) — Redis sliding window with in-memory fallback/api/v1/healthsrc/config-listeners.js) — cache eviction on config changes via DB LISTEN/NOTIFYWhat this PR adds (final wiring)
server.js+community.js: Switch from in-memoryrateLimittoredisRateLimitfor distributed rate limiting/rankcommand: UsesgetReputationCached+setReputationCache+getRankCached— avoids redundant DB queries/leaderboardcommand: UsesgetLeaderboardCached— TTL-cached queryfetchChannelCachednow wired incommands/welcome.js,commands/review.js,modules/welcomeOnboarding.js,modules/reviewHandler.jsTest results
2874 passing, 6 failing (all pre-existing
triage.coverage.test.jsfailures unrelated to this feature).Acceptance criteria