feat: deep config autocomplete + dynamic welcome messages#15
feat: deep config autocomplete + dynamic welcome messages#15BillChirico merged 1 commit intomainfrom
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbit
WalkthroughImplements dynamic, activity-aware welcome system with timezone and milestone tracking. Adds database-backed configuration management with persistent caching. Refactors config command to support autocomplete and multiple subcommands. Enhances database layer with re-entrancy guards and SSL configuration. Changes
🚥 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)
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📥 CommitsReviewing files that changed from the base of the PR and between 20f7f8ee48704052380b5f3a7d7c4a853a0eb383 and 60e8c4d. 📒 Files selected for processing (8)
✏️ Tip: You can disable this entire section by setting 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
54-81:⚠️ Potential issue | 🟡 MinorJSON code block contains
//comments, which are invalid JSON.Users copy-pasting this block into
config.jsonwill get parse errors. Either switch to JSONC/JSON5 fencing or move the annotation outside the code block.📝 Proposed fix
-```json +```jsonc { ... "welcome": { "enabled": true, "channelId": "...", "message": "Welcome, {user}!", // used when dynamic.enabled=falseAlternatively, remove the inline comment and add a note in prose above the block.
🤖 Fix all issues with AI agents
In `@src/commands/config.js`:
- Around line 81-109: The collectConfigPaths function currently adds both branch
and leaf entries which can cause `/config set` to overwrite whole sections;
update collectConfigPaths to emit only leaf paths by pushing a path only when
the corresponding value is not an object (i.e., primitive or null), and for
objects/arrays recurse without adding the branch path; reference the function
name collectConfigPaths and ensure callers (e.g., autocomplete or setConfigValue
usage) continue to use the returned list of leaf-only paths.
- Line 9: VALID_SECTIONS is a hardcoded duplicate and causes runtime validation
in handleSet to reject newly added config sections; change handleSet to derive
valid sections from the live config by calling getConfig() (or equivalent) and
using Object.keys(config) to validate the incoming section parameter, while
keeping the static VALID_SECTIONS for command-definition addChoices usage;
update the validation logic in handleSet (and any error messages) to use that
derived keys set instead of VALID_SECTIONS and ensure you still fall back to a
safe error path if getConfig() returns null/undefined.
- Line 242: The "New Value" field currently uses path.split('.').slice(1)[0]
which only picks the second segment and breaks for deeper paths (e.g.,
welcome.dynamic.enabled); add a helper getNestedValue(obj, keys) that traverses
keys with reduce (returning undefined if any segment is missing), then replace
the current expression in the embed field that constructs the New Value (the
object referenced as updatedSection and the path variable) with a call like
getNestedValue(updatedSection, path.split('.').slice(1)) to extract the actual
leaf value before JSON.stringify.
In `@src/db.js`:
- Around line 18-53: initDb currently creates a new Pool on every call and can
leak one if called concurrently; add a re-entrancy guard by introducing a
module-level "initPromise" (or similar) and short-circuit: if pool is already
set return it immediately; if an initPromise exists await and return the
resolved pool; when starting initDb assign initPromise before creating the
Pool/connecting so concurrent callers wait on it; on success resolve and keep
pool, and on error clear initPromise and rethrow so future calls can retry.
Ensure the symbols mentioned are used: initDb, pool, Pool, and the new
initPromise (or initializing flag).
- Line 30: The ssl config currently sets rejectUnauthorized: false which
disables TLS verification; change src/db.js so the ssl property defaults to
secure verification and uses a platform-provided CA when available: read a CA
cert from an environment variable (e.g., PG_SSL_ROOT_CERT or RAILWAY_CA_CERT)
and set ssl to { rejectUnauthorized: true, ca: <cert> } (or true if no custom CA
is needed), and only allow rejectUnauthorized: false when an explicit opt-out
env var (e.g., PG_SSL_INSECURE='true') is set; update the logic that builds the
ssl property (the ssl field that uses connectionString) and document the new
opt-out env var.
In `@src/index.js`:
- Around line 250-252: closeDb() can throw (e.g., pool.end() rejecting) and
would prevent client.destroy() from running; wrap the await closeDb() call in a
try/catch (or ensure closeDb() itself catches pool.end() errors) and log the
error (e.g., using error(...) or processLogger.error), then ensure
client.destroy() is executed in a finally block (or after the catch) so it
always runs even if closing the DB fails; reference the closeDb() call and
client.destroy() to locate where to add the try/catch/finally.
In `@src/modules/config.js`:
- Around line 150-152: resetConfig currently calls loadConfigFromFile() and
getPool() on every invocation causing sync I/O and duplicate throws; change the
module to cache the on-disk config once (e.g., populate a module-scoped
cachedConfig via loadConfigFromFile() at startup or expose an initConfig that
sets cachedConfig) and have resetConfig use cachedConfig instead of calling
loadConfigFromFile() repeatedly; also align getPool usage with setConfigValue by
guarding against missing DB config (either check cachedConfig for DB settings
before calling getPool() or reuse the same validation logic used in
setConfigValue) so resetConfig no longer triggers unexpected synchronous I/O or
duplicate errors.
- Around line 198-222: The numeric parsing in parseValue currently converts any
digit-string via /^\-?\d+(\.\d+)?$/ to Number, which will silently lose
precision for integers outside Number.MIN_SAFE_INTEGER..Number.MAX_SAFE_INTEGER;
update parseValue (the function named parseValue and the numeric-regex branch)
to detect integer-only numeric strings and if the integer magnitude exceeds
Number.MAX_SAFE_INTEGER (or is less than Number.MIN_SAFE_INTEGER) return the
original string instead of Number (or alternatively return BigInt if you prefer
and callers can handle it) so large integers are not silently truncated.
- Around line 134-139: setConfigValue currently calls getPool() without handling
the case where no DB exists; wrap the getPool() call in a try/catch (or check
for a falsy return) inside setConfigValue and, if no pool is available, do not
let the low-level error bubble up — instead update configCache in-memory if you
want transient changes or throw a clear user-facing error like "Config
persistence requires a database" so callers (e.g. the CLI `/config set`) receive
a friendly message; reference setConfigValue, getPool, configCache and
loadConfig when making the change.
- Around line 180-185: The current logic in the config update branch uses object
spread on value which can silently produce {} for primitives or throw for null;
update the handling in the block that touches configCache[key] so you first
guard that value is a non-null object (e.g., typeof value === 'object' && value
!== null) before using Object.assign or spreading into configCache[key]; if
value is a primitive or null, replace configCache[key] directly with value (or
delete/overwrite the existing object) instead of spreading, ensuring the
branches in the code that reference configCache, key and value behave correctly
for both object and non-object config entries.
- Around line 114-115: Remove the dead variable `sectionConfig` or use it
consistently: either delete the unused declaration `const sectionConfig = {
...(configCache[section] || {}) };` and continue mutating `configCache[section]`
as currently done, or refactor so you modify the shallow copy (`sectionConfig`),
perform updates against `sectionConfig`, then assign it back into
`configCache[section]` (e.g., `configCache[section] = sectionConfig`) to avoid
mutating the original object directly; locate the declaration of `sectionConfig`
and subsequent mutations of `configCache[section]` to apply one of these fixes.
- Around line 57-65: The seeding loop that inserts fileConfig entries when
rows.length === 0 must run inside a DB transaction to avoid partial seeding;
obtain a client from pool (e.g., pool.connect()), BEGIN the transaction, perform
the inserts using client.query (still using the existing INSERT ... ON CONFLICT
statement for each key/value), COMMIT on success and ROLLBACK on error, and
ensure the client is released in a finally block so the seeding is atomic and
won't leave the config table partially populated on crash.
In `@src/modules/welcome.js`:
- Line 36: The code creates a new Set on every message by doing const
excludedChannels = new Set(welcomeDynamic.excludeChannels || []); inside
recordCommunityActivity (or its per-message path); instead, avoid allocating per
message by caching the Set on the welcomeDynamic object (e.g., store
welcomeDynamic._excludedChannelsSet and update it only when
welcomeDynamic.excludeChannels changes) or simply use Array.prototype.includes
for small lists (use welcomeDynamic.excludeChannels.includes(channelId)) so you
only allocate when the config changes rather than on every call; update
references to use the cached Set or the includes check and ensure cache
invalidation when welcomeDynamic.excludeChannels is reassigned.
- Around line 146-153: getCommunitySnapshot mutates the shared timestamps arrays
by shifting out stale entries; move that pruning logic out of
getCommunitySnapshot so the function only reads a snapshot. Specifically, remove
the while(timestamps.length && timestamps[0] < cutoff) { timestamps.shift(); }
loop from getCommunitySnapshot and ensure stale-entry removal is performed in
recordCommunityActivity (or a new pruneActivityForChannel helper) where
activityMap and timestamps are already updated; if you must keep some pruning in
the reader, clearly document that getCommunitySnapshot mutates timestamps. Keep
references to activityMap, timestamps, getCommunitySnapshot, and
recordCommunityActivity when making the change so callers and maintainers can
find related logic.
- Line 74: The current check for useDynamic (config.welcome?.dynamic?.enabled
!== false) treats an absent dynamic block as enabled; change the logic so
dynamic mode is only enabled on explicit opt-in (i.e., only true when
config.welcome.dynamic.enabled is explicitly true). Update the evaluation of
useDynamic in src/modules/welcome.js (the useDynamic variable and the
config.welcome?.dynamic?.enabled reference) to require explicit true and ensure
downstream code that reads timezone/highlight channels still treats missing
dynamic config as disabled.
- Line 245: The return statement in the welcome module uses a single-quoted
string containing unescaped apostrophes ("you're" and "we're"), causing a
SyntaxError; update the literal in the return inside the affected function or
export in src/modules/welcome.js (the line that currently returns 'Say hey and
tell us what you’re building — we’re glad you’re here.') to a backtick template
literal or escape the apostrophes so the string is valid (e.g., use backticks
around the whole string or replace internal apostrophes with \').
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
README.mdconfig.jsonpackage.jsonsrc/commands/config.jssrc/db.jssrc/index.jssrc/modules/config.jssrc/modules/events.jssrc/modules/welcome.js
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-10T15:05:26.145Z
Learnt from: CR
Repo: BillChirico/LUA-Obfuscator PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-10T15:05:26.145Z
Learning: Applies to package.json : Only add new packages when absolutely necessary or explicitly requested
Applied to files:
package.json
📚 Learning: 2026-02-04T02:20:09.131Z
Learnt from: CR
Repo: VolvoxCommunity/Volvox.Website PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-04T02:20:09.131Z
Learning: Applies to {package.json,.npmrc,pnpm-lock.yaml} : Use pnpm package manager (pinned to v10.23.0) with `.npmrc` configured for strict peer dependencies and disabled shamefully-hoist
Applied to files:
package.json
🧬 Code graph analysis (6)
src/modules/config.js (3)
src/index.js (1)
__dirname(30-30)src/db.js (2)
pool(12-12)getPool(60-65)src/logger.js (1)
info(218-220)
src/index.js (4)
src/db.js (3)
client(34-34)closeDb(70-76)initDb(18-53)src/modules/config.js (1)
loadConfig(40-82)src/modules/events.js (1)
registerEventHandlers(147-152)src/commands/status.js (1)
healthMonitor(58-58)
src/modules/welcome.js (3)
src/commands/config.js (2)
config(117-117)config(167-167)src/index.js (1)
config(40-40)src/modules/chimeIn.js (1)
channelId(202-202)
src/db.js (2)
src/modules/config.js (4)
pool(44-44)pool(55-55)pool(135-135)pool(152-152)src/logger.js (1)
info(218-220)
src/modules/events.js (1)
src/modules/welcome.js (2)
recordCommunityActivity(31-59)message(76-82)
src/commands/config.js (1)
src/modules/config.js (5)
getConfig(88-90)section(114-114)sectionData(165-165)setConfigValue(108-143)resetConfig(150-191)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (6)
src/modules/events.js (1)
70-71: LGTM!Placement after spam filtering and before AI handling is appropriate.
recordCommunityActivityis synchronous and has its own defensive guards, so this integrates cleanly.config.json (1)
21-32: LGTM — sensible defaults for the dynamic welcome configuration.The structure is clean and the values are reasonable. Channel IDs are server-specific as expected for a seed config.
src/index.js (2)
300-322: Startup sequencing looks solid.The conditional DB init, async config load with fallback, in-place config mutation for live reference propagation, and deferred event-handler registration are all well-structured. The shutdown sequence (Lines 226-261) mirrors startup in reverse order — good.
162-175: LGTM — autocomplete dispatch with proper error isolation.Errors in individual command autocomplete handlers are caught and logged without crashing the interaction pipeline.
package.json (1)
16-16: LGTM —pgdependency is justified for the new DB-backed config persistence.The specified version
^8.18.0is valid and recently released.src/modules/config.js (1)
68-74: No bug: Thevaluecolumn isJSONB, not TEXT, and node-postgres automatically parses JSONB columns.The schema in
src/db.js(line 46) definesvalue JSONB NOT NULL. When the pg driver retrieves rows with JSONB columns, it automatically parses them back to JavaScript objects. The pattern ofJSON.stringify()on writes and direct assignment on reads is correct for JSONB columns—no explicitJSON.parse()is needed. Line 72 receives an already-parsed object.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/commands/config.js`:
- Around line 186-213: The current embed size calculation uses
embed.data.title.length + embed.data.description.length which can throw if
embed.data.title or embed.data.description is undefined; update the logic in the
block that computes totalLength (around the code using EMBED_CHAR_LIMIT and
variables totalLength/truncated) to safely read lengths with fallbacks (e.g.,
treat missing title or description as empty string) before summing, then proceed
with the existing field loop and truncation logic; reference embed.data.title,
embed.data.description, totalLength, EMBED_CHAR_LIMIT, and the embed.addFields
calls when applying the defensive checks.
In `@src/modules/config.js`:
- Around line 127-214: The setConfigValue function repeats the same
"walk-and-create intermediate objects, then assign leaf" logic three times (when
building sectionClone, mutating dbSection, and updating configCache); extract
that into a small helper (e.g., setNestedValue(obj, keys, value)) that walks
keys, creates intermediate objects, and sets the leaf, then replace the three
loops in setConfigValue with calls like setNestedValue(sectionClone,
parts.slice(1), parsedVal), setNestedValue(dbSection, parts.slice(1),
parsedVal), and setNestedValue(configCache[section], parts.slice(1), parsedVal);
keep the helper in the same module (non-exported) and ensure it mutates the
passed object in-place.
- Around line 267-269: The catch block in resetConfig currently performs await
client.query('ROLLBACK') which can throw and overwrite the original txErr;
change it to perform the rollback in a safe way (e.g., await
client.query('ROLLBACK').catch(() => {}) or wrap in its own try/catch) and then
rethrow the original txErr so the original error context from resetConfig is
preserved; update the catch handling around client.query('ROLLBACK') and ensure
you still await/close the client as appropriate.
- Around line 86-88: The catch block that currently does `await
client.query('ROLLBACK'); throw txErr;` can itself throw and mask the original
error; change it to guard the ROLLBACK so the original `txErr` is always
re-thrown (same approach used in `setConfigValue`): call
`client.query('ROLLBACK')` inside a safe catch (e.g., `await
client.query('ROLLBACK').catch(()=>{})` or a nested try/catch) and then re-throw
`txErr` so the rollback failure cannot replace the original error.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
README.mdsrc/commands/config.jssrc/db.jssrc/index.jssrc/modules/config.js
🧰 Additional context used
🧬 Code graph analysis (3)
src/modules/config.js (2)
src/logger.js (1)
info(218-220)src/db.js (2)
pool(12-12)getPool(90-95)
src/db.js (2)
src/modules/config.js (8)
pool(52-52)pool(66-66)pool(154-154)pool(224-224)client(74-74)client(155-155)client(159-162)client(257-257)src/logger.js (1)
info(218-220)
src/commands/config.js (1)
src/modules/config.js (6)
getConfig(116-118)section(133-133)sectionData(244-244)err(25-25)setConfigValue(127-214)resetConfig(221-288)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (13)
src/db.js (2)
21-83: Well-structured init lifecycle with proper cleanup.The re-entrancy guard, connection test with proper
client.release()infinally, and pool cleanup on init failure are all well done. The SSL configuration now defaults to secure (rejectUnauthorized: true) with an explicit opt-out, which addresses prior review feedback.
100-111: LGTM —closeDbis defensive and idempotent.The try/catch/finally ensures the pool reference is always nulled out regardless of errors during
pool.end(), which prevents stale references.src/modules/config.js (2)
68-91: Seeding transaction and fallback logic look solid.The transactional seeding,
ON CONFLICTupsert, and properclient.release()infinallyaddress the prior review concern. The layered fallback (DB → file → error) is well-structured.
304-333:parseValuesafely handles edge cases.Large integers are now preserved as strings (line 318), and the
Number.isFiniteguard (line 317) catches Infinity/NaN. The JSON parse attempt for arrays/objects with a silent fallback is appropriate.src/commands/config.js (3)
65-92:collectConfigPathsnow correctly emits leaf-only paths.The recursive traversal handles both arrays and plain objects, only pushing when a value is a non-object primitive. This addresses the prior concern about branch paths being selectable in autocomplete.
98-130: Autocomplete implementation is clean and well-bounded.Prefix matches are prioritized, results are capped at 25, and the section vs. path distinction is handled via
focusedOption.name.
247-273: Leaf-value display fix looks correct; minor note onJSON.stringify(undefined).Line 253's
reducetraversal properly walks deep paths, addressing the prior review. Note that if the path doesn't resolve (e.g., cache is stale),leafValuewill beundefined, andJSON.stringify(undefined)returns JSundefined— the??fallback to the rawvaluestring is a reasonable UX choice here.src/index.js (4)
304-326: Startup sequence is well-ordered.DB init → config load → state restore → event registration → commands → login. Notably,
registerEventHandlers(client, config, ...)receives the sameconfigCacheobject reference thatsetConfigValue/resetConfigmutate in-place, so event handlers always see the latest config without re-binding. Good design.
250-256:closeDb()in try/catch during shutdown — addresses prior review.This ensures that a failed pool teardown doesn't prevent the Discord client from being destroyed.
163-175: Autocomplete dispatch looks correct.The early
returnafter handling autocomplete prevents fallthrough to the chat-input-command handler. The?.optional chaining oncommand.autocompletegracefully handles commands without autocomplete support.
39-40: Module-scopedconfigis empty untilstartup()completes — safe becauseclient.login()is the last startup step.Interaction handlers are registered at module evaluation time (line 163), before
startup()runs. In theory, if an interaction arrived before config was loaded,hasPermissionwould receive{}. This is safe in practice because Discord only delivers interactions after login, which is the last step of startup. Just noting this for awareness.README.md (2)
54-81: Documentation updates are clear and consistent with the new config shape.The
jsoncblock correctly enables inline comments, and the newdynamicsection documents the key fields. The comment on line 66 clarifying whenmessageis used (dynamic.enabled=false) is helpful context.
8-8: Feature description updated to reflect dynamic welcome.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
✅ Review Comments AddressedAll 11 review threads have been resolved:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Round 4 — Bugbot Review Fixes ✅All 5 review threads resolved: 1. Remove unused
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/modules/config.js`:
- Around line 301-310: setNestedValue currently writes nested properties without
guarding against prototype pollution; update setNestedValue to validate path
segments internally (in addition to validatePathSegments used by setConfigValue)
by rejecting or throwing on dangerous keys like "__proto__", "prototype", and
"constructor" (and any other blacklist your project uses) before creating
objects, so calls from resetConfig or any other caller cannot mutate prototypes;
reference function names: setNestedValue (and note callers setConfigValue and
resetConfig) and ensure the function either filters/throws on invalid segments
or uses a safe object creation strategy (e.g., Object.create(null)) for
intermediate nodes.
- Around line 242-273: The full-reset path in resetConfig() currently only
upserts fileConfig entries so orphaned DB rows and runtime-added keys remain;
update the transaction that uses pool/client to delete any config rows whose key
is NOT IN the set of Object.keys(fileConfig) (or perform a single DELETE with
WHERE key <> ALL($1) using the fileConfig keys) before committing, and after the
DB transaction prune configCache by removing any top-level keys not present in
fileConfig (i.e., iterate Object.keys(configCache) and delete those not in
Object.keys(fileConfig)); reference symbols: resetConfig(), fileConfig,
configCache, pool/client, and setConfigValue for context.
In `@src/modules/welcome.js`:
- Around line 272-283: Hoist the recurring Set creation out of getMilestoneLine
into a module-level constant to avoid allocating notableMilestones on every
call; replace the in-function "const notableMilestones = new
Set([10,25,50,100,250,500,1000])" with a top-level "const NOTABLE_MILESTONES =
new Set(...)" and update getMilestoneLine to reference NOTABLE_MILESTONES while
keeping the existing logic and the function signature unchanged.
- Around line 231-236: The second branch currently always uses "people are" and
thus prints "1 people are" when snapshot.voiceParticipants === 1; update the
string in the branch that checks snapshot.voiceChannels (the hasChannels branch)
to use the same singular/plural conditional as the first branch by replacing
"people are" with `${snapshot.voiceParticipants === 1 ? 'person is' : 'people
are'}` so the returned template becomes `${snapshot.voiceParticipants}
${snapshot.voiceParticipants === 1 ? 'person is' : 'people are'} hanging out in
voice right now, and ${channelText} is waking up.`; use the existing variables
snapshot.voiceParticipants, snapshot.voiceChannels, hasChannels, and
channelText.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/commands/config.jssrc/db.jssrc/index.jssrc/modules/config.jssrc/modules/welcome.js
🧰 Additional context used
🧬 Code graph analysis (3)
src/modules/config.js (2)
src/logger.js (1)
info(218-220)src/db.js (2)
pool(12-12)getPool(118-123)
src/commands/config.js (1)
src/modules/config.js (6)
getConfig(122-124)section(142-142)sectionData(232-232)err(30-30)setConfigValue(133-202)resetConfig(209-276)
src/index.js (6)
src/commands/config.js (2)
config(101-101)config(160-160)src/logger.js (4)
config(27-27)error(232-234)info(218-220)warn(225-227)src/modules/config.js (6)
client(80-80)client(157-157)client(161-164)client(245-245)err(30-30)loadConfig(47-116)src/db.js (3)
client(82-82)closeDb(128-139)initDb(54-111)src/modules/events.js (1)
registerEventHandlers(147-152)src/commands/status.js (1)
healthMonitor(58-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (12)
src/index.js (3)
39-40: LGTM — async config initialization with stable reference pattern.The module-level
configis reassigned once duringstartup()to theconfigCacheobject returned byloadConfig(), and subsequent mutations viasetConfigValuehappen in-place on that same object. This keeps the reference passed toregisterEventHandlers(line 322) and used inhasPermission(line 186) consistent.
164-176: Autocomplete handling looks correct.Early return after autocomplete prevents falling through to the chat-input-command handler. Error is caught and logged without attempting to reply (Discord silently shows no suggestions on failure, which is the right UX).
305-332: Startup sequence is well-structured.DB init is correctly gated on
DATABASE_URLpresence, config load follows, and event handlers are registered with the live config reference before login. The.catch()onstartup()ensures a clean exit on failure.src/db.js (2)
54-111: Re-entrancy guard is functional but could lose concurrent callers.The boolean
initializingguard correctly prevents double-init, but concurrent callers get an error instead of waiting for the in-progress initialization. Given thatinitDb()is only called once during startup (line 308 ofsrc/index.js), this is acceptable. If this ever needs to be called from multiple paths, consider switching to a cached promise pattern.
128-139:closeDbproperly handles errors and cleans up.The
finallyblock ensurespoolis nulled even ifpool.end()throws, and errors are logged without re-throwing — correct for a shutdown path.src/modules/config.js (2)
26-40: File config caching looks correct.
loadConfigFromFile()caches the parsedconfig.jsonon first read and returns it on subsequent calls, avoiding redundant synchronous I/O. Sinceconfig.jsondoesn't change at runtime, this is safe.
133-202:setConfigValuegracefully degrades without DB — good resilience pattern.The try/catch around the DB write path (lines 155-192) logs the error and falls through to the in-memory update, so
/config setworks even when the database is unavailable. ThedbPersistedflag is also surfaced in the log for observability.src/modules/welcome.js (2)
34-66: Activity recording with cached excluded-channels Set — well implemented.The key-based invalidation pattern (line 40-43) avoids per-message
Setconstruction while still reacting to config changes. The pruning and capping logic on the write path is appropriate.
144-189:getCommunitySnapshotno longer mutates shared arrays — good fix.The
filter()on line 154 creates a new array instead of mutating the stored timestamps. Activity data integrity is preserved.src/commands/config.js (3)
65-92:collectConfigPathsexposes array-index paths (e.g.,permissions.allowedCommands.0).Autocomplete will surface paths like
permissions.allowedCommands.0for array entries. WhilesetConfigValuecan technically handle these (JS arrays support string-index assignment), it's worth noting thatsetNestedValuecreates{}for missing intermediates, not[]. For existing arrays this works, but setting a path that requires creating a new intermediate array-like structure would produce an object instead. Since autocomplete only surfaces existing paths, this is safe in practice.
98-130: Autocomplete implementation is clean — prefix-biased sorting with 25-choice cap.Good use of
startsWithprioritization so that typingai.immediately surfacesai.*paths before unrelated matches. Theslice(0, 25)respects Discord's limit.
233-274:handleSeterror handling correctly distinguishes deferred vs. non-deferred state.The early validation reply (line 241) happens before
deferReply, and the catch block (line 268) checksinteraction.deferredto pick the right response method. This avoids the common discord.js pitfall of replying to an already-deferred interaction.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
This comment has been minimized.
This comment has been minimized.
Round 5 Review Fixes — All 6 Threads Resolved ✅Changes Made (5 commits for 6 review threads):
|
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@src/commands/config.js`:
- Around line 147-161: The switch in execute currently handles 'view', 'set',
and 'reset' but lacks a default branch; add a default case to the switch in
execute that calls interaction.reply (or interaction.followUp) with an ephemeral
error message indicating an unknown subcommand so the user always receives a
response—use the same interaction object and ensure the reply is ephemeral to
avoid leaking info; this change references the execute function and preserves
existing calls to handleView, handleSet, and handleReset.
In `@src/modules/config.js`:
- Around line 305-322: The helper setNestedValue currently allows an empty
pathParts array which causes current[undefined] = value; add an explicit guard
at the start of setNestedValue to validate that pathParts is a non-empty array
(or has length > 0) and throw a clear Error like "Invalid config path: pathParts
must contain at least one segment" if it is empty; keep the existing
DANGEROUS_KEYS checks and behavior for normal segments and leaf assignment
(i.e., perform the guard before any prototype-pollution checks or assignments).
- Around line 18-19: The JSDoc for fileConfigCache is misleading because the
cache is never reset; update the comment for the variable fileConfigCache to
reflect that config.json is immutable at runtime and the cache is permanent (or,
alternatively, implement explicit invalidation where appropriate); locate the
declaration of fileConfigCache and replace "invalidated on reset" with wording
like "cached permanently for the process lifetime (config.json is immutable at
runtime)" so future maintainers are not confused.
- Around line 64-68: The shallow spread assignment configCache = { ...fileConfig
} creates shared nested references with fileConfig/fileConfigCache and allows
mutations from setConfigValue (which uses setNestedValue) to corrupt the cached
file config; replace the shallow copy with a deep clone using structuredClone
wherever you currently spread fileConfig (the occurrences that set configCache
at the block shown and the similar instances around the assignments referenced
on Lines ~91 and ~112) so configCache is an independent deep copy and
resetConfig can safely restore from fileConfigCache without shared-reference
mutation.
In `@src/modules/welcome.js`:
- Around line 333-337: In the afternoon welcome messages inside the afternoon
array in src/modules/welcome.js, one template string is hardcoded to "the Volvox
corner of the internet" instead of using the server name; update that array
entry to interpolate ctx.server (e.g., use ${ctx.server}) so the message reads
like the others (keep the surrounding template string and <@${ctx.id}> mention
intact) so non-Volvox servers display their actual name.
- Around line 225-228: In the 'busy' case return (in src/modules/welcome.js)
avoid showing "0 in voice" by conditionally including the voice stat: when
hasChannels is false, only append the `${snapshot.voiceParticipants} in voice`
fragment if snapshot.voiceParticipants > 0; otherwise return the shorter string
that only mentions `${snapshot.messageCount}`. Update the template logic around
the 'busy' case (referencing hasChannels, snapshot.messageCount,
snapshot.voiceParticipants, and channelText) to use a conditional/ternary to
include or omit the voice portion.
- Around line 53-68: The per-guild activity Map (guildActivity) never evicts
stale channel entries, causing unbounded growth; modify recordCommunityActivity
(and/or getCommunitySnapshot) to prune channel keys whose timestamps arrays
become empty or whose last timestamp is older than cutoff: after trimming
timestamps in the block that updates activityMap (using
activityMap.get/message.channel.id and the timestamps array), if
timestamps.length === 0 (or lastTimestamp < cutoff) call
activityMap.delete(message.channel.id); additionally, if activityMap is empty
after deletions, remove the guild key from guildActivity to avoid retaining
empty guild maps; ensure this logic runs immediately after the existing
trimming/splice logic and still respects MAX_EVENTS_PER_CHANNEL.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/commands/config.jssrc/modules/config.jssrc/modules/welcome.js
🧰 Additional context used
🧬 Code graph analysis (3)
src/modules/config.js (2)
src/logger.js (1)
info(218-220)src/db.js (2)
pool(12-12)getPool(118-123)
src/commands/config.js (1)
src/modules/config.js (5)
getConfig(122-124)section(142-142)err(30-30)setConfigValue(133-202)resetConfig(209-280)
src/modules/welcome.js (3)
src/commands/config.js (2)
config(112-112)config(171-171)src/index.js (1)
config(40-40)src/modules/config.js (1)
err(30-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (20)
src/modules/welcome.js (7)
6-14: Module-level constants and caches look good.NOTABLE_MILESTONES is properly hoisted, and the excludedChannelsCache avoids per-message
Setallocation. The shared module-level state is appropriate for this use case.
77-99: LGTM — dynamic opt-in and static fallback are clean.The
=== truecheck properly requires explicit opt-in, and the fallback to static rendering is solid.
107-139: Dynamic message assembly is well-structured.The composition of greeting, milestone/member-count, vibe, and CTA lines is clean and readable.
147-192: Non-mutating snapshot with filter — nicely addresses the earlier review.The snapshot logic correctly reads without side effects, and the voice channel aggregation is clean.
306-319: LGTM — graceful timezone handling with sensible fallback.The
try/catchandNumber.isFiniteguard handle invalid timezones and unexpected formatting well.
360-373: LGTM — channel suggestion logic is clean.Deduplication, guild-existence check, and the 3-channel cap are well handled.
390-404: LGTM — utility functions are concise and defensive.
getActivityWindowMsenforces a sensible minimum, andpickFromhandles the empty-array edge case.src/modules/config.js (7)
26-40: LGTM — caching and error handling look correct.The synchronous file read is guarded by the
fileConfigCachecheck, preventing repeated disk I/O. Error paths are clear and descriptive.
118-124: LGTM.Simple accessor returning the in-memory cache. The by-reference return is intentional per the codebase's design for in-place mutation.
133-202: Well-structured transactional write with proper fallback.Row-level locking (
SELECT ... FOR UPDATE), ROLLBACK guard (.catch(() => {})), prototype-pollution defense, and graceful in-memory fallback are all in place. ThestructuredClonefor the INSERT path and the extractedsetNestedValuehelper address prior review concerns cleanly.
209-280: Reset logic is thorough with proper in-place mutation and deep cloning.The
structuredCloneusage, reference-preserving clear-and-assign pattern, transactional full-reset with guarded ROLLBACK, and the deliberate comment about preserving runtime-added sections all look correct. The only dependency is onfileConfigCacheintegrity, which is addressed by the shallow-spread fix noted above.
282-296: LGTM — clean prototype-pollution guard.
329-331: LGTM.Standard plain-object check covering the null and Array edge cases.
338-367: LGTM —parseValuehandles edge cases well.Safe-integer guard for large numbers, JSON parse with fallback, and boolean/null literals are all covered correctly.
src/commands/config.js (6)
9-55: LGTM — command definition is well-structured.Subcommands with appropriate required/optional options and autocomplete flags are correctly wired.
65-103: LGTM — leaf-only path collection is correct.Handles arrays (with index-based paths), empty containers, and null values properly. The recursion is safe given JSON-sourced config objects.
109-141: LGTM — autocomplete logic is clean.Prefix-prioritized sorting, case-insensitive matching, and the 25-choice Discord cap are all handled correctly.
169-239: LGTM — view handler with embed-size tracking and truncation is solid.Field truncation stays within Discord's 1024-char field limit, cumulative size tracking guards the 6000-char embed limit, and optional chaining on
embed.datafields is in place.
244-285: LGTM — set handler correctly validates, persists, and displays the update.Live-config section validation, deferred reply for async DB work, leaf-value traversal via
reduce, and theJSON.stringify(…) ?? valuefallback all look correct.
290-318: LGTM — reset handler is clean and consistent with the set handler pattern.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
PR #15 Round 6 — Review Fixes SummaryAll 7 review threads addressed and resolved: Fixes
All 7 review threads resolved. ✅ |
This comment has been minimized.
This comment has been minimized.
Round 7 — Array Safety Fix ✅Thread Resolved:
|
This comment has been minimized.
This comment has been minimized.
🔨 Round 8 — Review Comments AddressedThread Resolved (1/1)
Commit: |
This comment has been minimized.
This comment has been minimized.
🔨 PR #15 — Round 9 SummaryResolved Threads (1/1)
Commit: |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/commands/config.js`:
- Around line 257-261: The error message interpolates raw section into the reply
(validSections, section, interaction.reply) which can trigger markdown/mentions;
before using it in the reply, sanitize/escape section (e.g., escape backticks
and `@-mentions`) and use that escaped value in the interaction.reply message
(replace the direct `${section}` with the sanitized variable) so the displayed
section is safe.
- Around line 186-194: The error reply embeds the raw user-provided section
variable directly into the message; update the check around sectionData to
sanitize or safely format the section before replying (e.g., wrap the section
value in inline code backticks or escape Discord markdown/mention characters) so
malicious input like `@everyone` or markdown cannot trigger mentions or
formatting; modify the block that references section and the call to
interaction.reply to use the sanitized/wrapped value instead of the raw section
string (keep the existing check for !sectionData and only change the displayed
content).
In `@src/modules/config.js`:
- Around line 175-181: The INSERT branch in setConfigValue performs a blind
INSERT into the config table which races when two callers create the same
section; change the insert logic to an upsert (use INSERT ... ON CONFLICT (key)
DO UPDATE SET value = EXCLUDED.value) so concurrent creators resolve safely, and
update the error handling around client.query for the INSERT/UPSERT to avoid
logging such conflicts as generic "Database unavailable" (log the actual
error/constraint info instead); locate the INSERT into config and replace it
with an ON CONFLICT upsert tied to the key and adjust the catch/logging nearby.
- Around line 219-241: The single-section reset currently calls pool.query and
will abort the in-memory reset if the DB write throws; wrap the DB write inside
a try/catch around the pool.query call (the same way setConfigValue does), log
the error (using info/error logger) but do not rethrow, and then proceed to
mutate configCache (the sectionData assignment/structuredClone block) so the
in-memory reset always happens even when pool.query fails; reference pool.query,
section, configCache, and setConfigValue to mirror its error-handling pattern.
In `@src/modules/welcome.js`:
- Around line 147-198: getCommunitySnapshot currently filters timestamps into
the local variable recent but never writes that filtered array back to the
stored activityMap, leaving expired timestamps in memory; update
getCommunitySnapshot so that after computing recent you replace the original
array in activityMap.set(channelId, recent) and if recent is empty call
activityMap.delete(channelId) (and ensure you still evict the whole guild from
guildActivity when activityMap.size === 0); this keeps the stored arrays pruned
between recordCommunityActivity calls and prevents stale accumulation (refer to
variables: getCommunitySnapshot, activityMap, timestamps, recent, guildActivity,
recordCommunityActivity, MAX_EVENTS_PER_CHANNEL).
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/commands/config.jssrc/modules/config.jssrc/modules/welcome.js
🧰 Additional context used
🧬 Code graph analysis (3)
src/modules/welcome.js (4)
src/commands/config.js (2)
config(112-112)config(177-177)src/index.js (1)
config(40-40)src/modules/config.js (1)
err(30-30)src/modules/chimeIn.js (1)
channelId(202-202)
src/commands/config.js (1)
src/modules/config.js (6)
getConfig(122-124)section(142-142)sectionData(232-232)err(30-30)setConfigValue(133-202)resetConfig(209-280)
src/modules/config.js (2)
src/logger.js (1)
info(218-220)src/db.js (2)
pool(12-12)getPool(118-123)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (14)
src/modules/config.js (4)
15-19: LGTM on caching and documentation.The
fileConfigCacheis properly cached once and the JSDoc accurately reflects the behavior.structuredCloneis used consistently to prevent shared-reference corruption.
47-116: LGTM — solid fallback chain with transactional seeding.The load flow correctly handles all combinations of DB/file availability, seeds atomically, and uses
structuredClonethroughout.
305-331: LGTM — well-defendedsetNestedValuehelper.Empty path guard, prototype-pollution checks, and array-aware traversal are all in place.
358-391: LGTM —parseValuecovers edge cases well.Safe integer range check, JSON escape hatch, and graceful fallback to plain strings are solid.
src/modules/welcome.js (6)
37-69: LGTM — activity tracking with cached exclusion and bounded storage.The cached
Setwith key-based invalidation, rolling window pruning, and per-channel cap are well implemented.
84-84: LGTM — explicit opt-in for dynamic welcome.
=== truecorrectly prevents accidental activation when the config block is absent.
220-252: LGTM —buildVibeLinehandles all edge cases.Singular/plural voice text, empty channels guard, and the
busycase conditionally omitting "0 in voice" are all properly addressed.
259-273: LGTM —buildCtaLineand string escaping.Template literals used throughout, no unescaped apostrophes.
366-379: LGTM —getSuggestedChannelsvalidates channel existence before suggesting.Deduplication via
Set, cache-presence filter, and bounded slice are all correct.
312-325:getHourInTimezonemay return 24 for midnight in some locales/environments.
Intl.DateTimeFormatwithhour: '2-digit', hour12: falsecan return"24"instead of"00"for midnight depending on the locale and runtime.Number("24")passes theisFinitecheck, which would then not match any time-of-day bucket ingetTimeOfDay(falls through to'night'). This happens to be correct for midnight, so it's benign, but worth being aware of.src/commands/config.js (4)
65-103: LGTM —collectConfigPathscorrectly emits leaf-only paths with empty container support.The recursive traversal handles arrays, objects, and empty containers correctly, making all leaf paths discoverable in autocomplete.
109-141: LGTM — autocomplete with prefix prioritization and 25-choice cap.The sort-by-startsWith approach provides good UX and respects Discord's limit.
147-167: LGTM — default case added for unknown subcommands.Good defensive handling.
250-291: LGTM —handleSetwith live section validation and proper leaf display.The
reducetraversal to leaf value and the deferred/non-deferred error reply branching are both correct.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| function getMilestoneLine(memberCount, settings) { | ||
| if (!memberCount) return null; | ||
|
|
||
| const interval = Number(settings.milestoneInterval) || 25; |
There was a problem hiding this comment.
Falsy || prevents disabling interval milestones with zero
Low Severity
Number(settings.milestoneInterval) || 25 treats a user-configured value of 0 as falsy and silently replaces it with 25. The interval > 0 guard on line 286 shows the intent was for 0 to disable interval-based milestones, but the || fallback defeats that. Using ?? 25 (nullish coalescing) would preserve 0 while still defaulting for undefined/null.
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
Or push these changes by commenting: |
🔨 Round 10 — All 5 Review Threads AddressedCommits: 4 fixes, pushed to Changes
All 5 coderabbitai review threads resolved. ✅ |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@src/commands/config.js`:
- Around line 307-335: The error path in handleReset forwards raw err.message
(which includes the user-provided section) to Discord; update the catch block to
sanitize the section before displaying by either escaping the inline code like
handleView does (use escapeInlineCode(section) when building the message) or by
constructing a safe message that does not interpolate raw err.message; ensure
references to handleReset, resetConfig, and err.message are updated so the
displayed content uses the sanitized section value (or a generic error text)
instead of the raw err.message.
- Around line 283-293: The "New Value" embed field currently uses
JSON.stringify(leafValue, null, 2) which can exceed Discord's 1024-char limit;
update the embed construction (around EmbedBuilder usage that creates embed and
fields, and before interaction.editReply) to truncate the stringified value
using the same truncation logic used by handleView (i.e., convert leafValue to a
pretty JSON string, enforce a max length of 1024 minus any wrapper/backticks,
and append an ellipsis when truncated) and then use that truncated string in the
'New Value' field so interaction.editReply won't fail for large objects.
In `@src/modules/config.js`:
- Around line 237-244: The code assigns arrays from fileConfig into configCache
by reference (because isPlainObject returns false for arrays), risking cache
corruption; update the logic around sectionData, configCache, and the other
branch handling (including the similar block at lines ~272–278) to use
structuredClone for any non-primitive value instead of only for plain
objects—i.e., when assigning fileConfig[section] or value to
configCache[section] or sectionData, check for non-primitive (object/array) and
use structuredClone(fileConfig[section] / value) so arrays are cloned rather
than referenced.
In `@src/modules/welcome.js`:
- Around line 107-139: In buildDynamicWelcomeMessage, avoid emitting "member `#0`"
when guild.memberCount is unavailable: detect when member.guild?.memberCount is
undefined/null (instead of defaulting to 0) and pass a nullable/undefined count
into getMilestoneLine; then only add the fallback "You just rolled in as member
..." line when a real count exists (use memberContext.memberCount truthiness or
an explicit isNumber check) — update the memberContext creation and the
conditional around the fallback so getMilestoneLine(memberContext.memberCount,
...) and the fallback line are skipped if the count is missing.
- Around line 246-248: The sentence uses a singular verb "is" with channelText
which may refer to multiple channels; update the template to pick the verb based
on the number of channels (use snapshot.voiceChannels) instead of always "is" —
e.g. replace the hardcoded "is" in the string containing ${channelText} with a
conditional like snapshot.voiceChannels === 1 ? 'is' : 'are' (apply the same
change to the other similar line that currently uses "is where people are
checking in"), referencing snapshot.voiceChannels, snapshot.voiceParticipants
and channelText to determine singular vs plural.
- Around line 95-97: Replace the raw console calls in the welcome handler with
the project logger: import the winston-based logger from src/logger.js (e.g.,
const { info, logError } = require('../logger')) at the top of
src/modules/welcome.js, then change console.log(`[WELCOME] ${member.user.tag}
joined ${member.guild.name}`) to info(`[WELCOME] ${member.user.tag} joined
${member.guild.name}`) and change console.error('Welcome error:', err.message)
to logError('Welcome error', err) (or use logger.error with the error object) so
logging goes through the centralized logger and includes full error
details/stack.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between a27fa6c and 20f7f8ee48704052380b5f3a7d7c4a853a0eb383.
📒 Files selected for processing (3)
src/commands/config.jssrc/modules/config.jssrc/modules/welcome.js
🧰 Additional context used
🧬 Code graph analysis (2)
src/modules/config.js (2)
src/logger.js (1)
info(218-220)src/db.js (2)
pool(12-12)getPool(118-123)
src/commands/config.js (1)
src/modules/config.js (6)
getConfig(122-124)section(142-142)sectionData(236-236)err(30-30)setConfigValue(133-202)resetConfig(209-284)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (15)
src/modules/config.js (5)
26-40: LGTM — file config caching and error handling look solid.The caching of
fileConfigCacheavoids repeated disk reads,existsSyncguard prevents unclear JSON parse errors, and the typed error code on missing config is a nice touch for callers.
47-116: LGTM — loadConfig resilience and fallback chain are well structured.Uses
structuredCloneconsistently, transaction-wrapped seeding with safe ROLLBACK, and graceful DB-unavailable fallback. Past review concerns have been addressed.
133-202: LGTM —setConfigValueis well-hardened.Transaction with row-level locking,
ON CONFLICTfor concurrent inserts,ROLLBACKguarded with.catch, prototype-pollution defense, and graceful fallback to in-memory-only writes. Good work addressing the prior review feedback.
309-335: LGTM —setNestedValuehandles edge cases well.Empty
pathPartsguard, DANGEROUS_KEYS checks at both intermediate and leaf levels, and array-aware traversal with numeric index detection are all solid defensive additions.
362-395: LGTM —parseValuecoercion is well-documented and safe.Safe integer guard, finite check, JSON escape hatch for literal strings, and clear JSDoc. Past review concerns addressed.
src/modules/welcome.js (5)
37-69: LGTM — activity recording is clean and well-guarded.Cached excluded-channels Set avoids per-message allocation, timestamp pruning keeps arrays bounded, and the MAX_EVENTS_PER_CHANNEL cap prevents runaway growth. Past review concerns on this segment are addressed.
147-201: LGTM — snapshot pruning and eviction are solid.Filtered arrays written back to
activityMap, empty channels deleted, and guild entries evicted when no channels remain. This addresses the prior stale-data and memory-growth concerns.
315-328: LGTM — timezone-aware hour extraction with robust fallbacks.
Intl.DateTimeFormatis the right approach for server-side timezone handling, and the double fallback (parse failure →Date.getHours(), thrown error →Date.getHours()) is well considered.
335-360: LGTM — greeting templates are well-structured.All use backtick literals (apostrophe-safe), dynamic
ctx.server/ctx.idinterpolation, and a safe fallback toafternoonfor unexpectedtimeOfDayvalues. Past review concerns (hardcoded "Volvox", unescaped apostrophes) are resolved.
369-382: LGTM — channel suggestions are properly deduplicated and validated.
Setfor dedup,guild.channels.cache.has(id)to filter deleted/inaccessible channels before slicing, and a sensible merge order (active → configured → legacy) are all good.src/commands/config.js (5)
14-16: LGTM —escapeInlineCodeis a clean, focused sanitizer.
74-112: LGTM —collectConfigPathscorrectly emits only leaf paths with discoverable empty containers.The recursive traversal is clean, handles arrays and objects separately, and ensures empty arrays/objects surface in autocomplete. Past review concerns are resolved.
118-150: LGTM — autocomplete with prefix-biased sorting and 25-choice cap.Good UX:
startsWithmatches surface first, then alphabetical. Section autocomplete uses.includesfor broader fuzzy matching.
156-176: LGTM —defaultcase for unknown subcommands.Past review concern addressed. The ephemeral error reply prevents silent timeout.
184-255: LGTM —handleViewembed construction is well-guarded.Defensive
?.length || 0, per-field size tracking, truncation notice, andescapeInlineCodefor user-provided section. Past review concerns addressed.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| async function handleReset(interaction) { | ||
| const section = interaction.options.getString('section'); | ||
|
|
||
| try { | ||
| await interaction.deferReply({ ephemeral: true }); | ||
|
|
||
| await resetConfig(section || undefined); | ||
|
|
||
| const embed = new EmbedBuilder() | ||
| .setColor(0xFEE75C) | ||
| .setTitle('🔄 Config Reset') | ||
| .setDescription( | ||
| section | ||
| ? `Section **${section}** has been reset to defaults from config.json.` | ||
| : 'All configuration has been reset to defaults from config.json.' | ||
| ) | ||
| .setFooter({ text: 'Changes take effect immediately' }) | ||
| .setTimestamp(); | ||
|
|
||
| await interaction.editReply({ embeds: [embed] }); | ||
| } catch (err) { | ||
| const content = `❌ Failed to reset config: ${err.message}`; | ||
| if (interaction.deferred) { | ||
| await interaction.editReply({ content }); | ||
| } else { | ||
| await interaction.reply({ content, ephemeral: true }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Unsanitized user input in error path — err.message from resetConfig embeds the raw section name.
When an invalid section is provided, resetConfig throws Section '${section}' not found in config.json defaults. This raw error (containing the unescaped user input) is forwarded to Discord at Line 328 via err.message. Unlike handleView (which uses escapeInlineCode), this path does not sanitize the section.
Proposed fix — sanitize before display
} catch (err) {
- const content = `❌ Failed to reset config: ${err.message}`;
+ const safeMessage = escapeInlineCode(err.message);
+ const content = `❌ Failed to reset config: ${safeMessage}`;
if (interaction.deferred) {📝 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.
| async function handleReset(interaction) { | |
| const section = interaction.options.getString('section'); | |
| try { | |
| await interaction.deferReply({ ephemeral: true }); | |
| await resetConfig(section || undefined); | |
| const embed = new EmbedBuilder() | |
| .setColor(0xFEE75C) | |
| .setTitle('🔄 Config Reset') | |
| .setDescription( | |
| section | |
| ? `Section **${section}** has been reset to defaults from config.json.` | |
| : 'All configuration has been reset to defaults from config.json.' | |
| ) | |
| .setFooter({ text: 'Changes take effect immediately' }) | |
| .setTimestamp(); | |
| await interaction.editReply({ embeds: [embed] }); | |
| } catch (err) { | |
| const content = `❌ Failed to reset config: ${err.message}`; | |
| if (interaction.deferred) { | |
| await interaction.editReply({ content }); | |
| } else { | |
| await interaction.reply({ content, ephemeral: true }); | |
| } | |
| } | |
| } | |
| async function handleReset(interaction) { | |
| const section = interaction.options.getString('section'); | |
| try { | |
| await interaction.deferReply({ ephemeral: true }); | |
| await resetConfig(section || undefined); | |
| const embed = new EmbedBuilder() | |
| .setColor(0xFEE75C) | |
| .setTitle('🔄 Config Reset') | |
| .setDescription( | |
| section | |
| ? `Section **${section}** has been reset to defaults from config.json.` | |
| : 'All configuration has been reset to defaults from config.json.' | |
| ) | |
| .setFooter({ text: 'Changes take effect immediately' }) | |
| .setTimestamp(); | |
| await interaction.editReply({ embeds: [embed] }); | |
| } catch (err) { | |
| const safeMessage = escapeInlineCode(err.message); | |
| const content = `❌ Failed to reset config: ${safeMessage}`; | |
| if (interaction.deferred) { | |
| await interaction.editReply({ content }); | |
| } else { | |
| await interaction.reply({ content, ephemeral: true }); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/commands/config.js` around lines 307 - 335, The error path in handleReset
forwards raw err.message (which includes the user-provided section) to Discord;
update the catch block to sanitize the section before displaying by either
escaping the inline code like handleView does (use escapeInlineCode(section)
when building the message) or by constructing a safe message that does not
interpolate raw err.message; ensure references to handleReset, resetConfig, and
err.message are updated so the displayed content uses the sanitized section
value (or a generic error text) instead of the raw err.message.
| console.log(`[WELCOME] ${member.user.tag} joined ${member.guild.name}`); | ||
| } catch (err) { | ||
| console.error('Welcome error:', err.message); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent logging: console.log/console.error here vs. structured info/logError in config.js.
The rest of the modules use the winston-based logger (src/logger.js). Using console here bypasses any log-level filtering, formatting, and transport configuration.
Proposed fix
+import { info, error as logError } from '../logger.js';
+
// ... inside sendWelcomeMessage:
await channel.send(message);
- console.log(`[WELCOME] ${member.user.tag} joined ${member.guild.name}`);
+ info(`Welcome message sent`, { user: member.user.tag, guild: member.guild.name });
} catch (err) {
- console.error('Welcome error:', err.message);
+ logError('Welcome message error', { error: err.message });
}🤖 Prompt for AI Agents
In `@src/modules/welcome.js` around lines 95 - 97, Replace the raw console calls
in the welcome handler with the project logger: import the winston-based logger
from src/logger.js (e.g., const { info, logError } = require('../logger')) at
the top of src/modules/welcome.js, then change console.log(`[WELCOME]
${member.user.tag} joined ${member.guild.name}`) to info(`[WELCOME]
${member.user.tag} joined ${member.guild.name}`) and change
console.error('Welcome error:', err.message) to logError('Welcome error', err)
(or use logger.error with the error object) so logging goes through the
centralized logger and includes full error details/stack.
| function buildDynamicWelcomeMessage(member, config) { | ||
| const welcomeDynamic = config?.welcome?.dynamic || {}; | ||
| const timezone = welcomeDynamic.timezone || 'America/New_York'; | ||
|
|
||
| const memberContext = { | ||
| id: member.id, | ||
| username: member.user?.username || 'Unknown', | ||
| server: member.guild?.name || 'the server', | ||
| memberCount: member.guild?.memberCount || 0, | ||
| }; | ||
|
|
||
| const timeOfDay = getTimeOfDay(timezone); | ||
| const snapshot = getCommunitySnapshot(member.guild, welcomeDynamic); | ||
| const milestoneLine = getMilestoneLine(memberContext.memberCount, welcomeDynamic); | ||
| const suggestedChannels = getSuggestedChannels(member, config, snapshot); | ||
|
|
||
| const greeting = pickFrom(getGreetingTemplates(timeOfDay), memberContext); | ||
| const vibeLine = buildVibeLine(snapshot, suggestedChannels); | ||
| const ctaLine = buildCtaLine(suggestedChannels); | ||
|
|
||
| const lines = [greeting]; | ||
|
|
||
| if (milestoneLine) { | ||
| lines.push(milestoneLine); | ||
| } else { | ||
| lines.push(`You just rolled in as member **#${memberContext.memberCount}**.`); | ||
| } | ||
|
|
||
| lines.push(vibeLine); | ||
| lines.push(ctaLine); | ||
|
|
||
| return lines.join('\n\n'); | ||
| } |
There was a problem hiding this comment.
buildDynamicWelcomeMessage can produce "member #0" when guild.memberCount is unavailable.
Line 115 defaults memberCount to 0 if member.guild?.memberCount is falsy. getMilestoneLine(0, ...) returns null (line 285), so the fallback at line 132 emits "You just rolled in as member **#0**.". Consider guarding against this, e.g., by omitting the member-count line entirely when the count is unavailable.
Proposed fix
if (milestoneLine) {
lines.push(milestoneLine);
- } else {
+ } else if (memberContext.memberCount > 0) {
lines.push(`You just rolled in as member **#${memberContext.memberCount}**.`);
}📝 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.
| function buildDynamicWelcomeMessage(member, config) { | |
| const welcomeDynamic = config?.welcome?.dynamic || {}; | |
| const timezone = welcomeDynamic.timezone || 'America/New_York'; | |
| const memberContext = { | |
| id: member.id, | |
| username: member.user?.username || 'Unknown', | |
| server: member.guild?.name || 'the server', | |
| memberCount: member.guild?.memberCount || 0, | |
| }; | |
| const timeOfDay = getTimeOfDay(timezone); | |
| const snapshot = getCommunitySnapshot(member.guild, welcomeDynamic); | |
| const milestoneLine = getMilestoneLine(memberContext.memberCount, welcomeDynamic); | |
| const suggestedChannels = getSuggestedChannels(member, config, snapshot); | |
| const greeting = pickFrom(getGreetingTemplates(timeOfDay), memberContext); | |
| const vibeLine = buildVibeLine(snapshot, suggestedChannels); | |
| const ctaLine = buildCtaLine(suggestedChannels); | |
| const lines = [greeting]; | |
| if (milestoneLine) { | |
| lines.push(milestoneLine); | |
| } else { | |
| lines.push(`You just rolled in as member **#${memberContext.memberCount}**.`); | |
| } | |
| lines.push(vibeLine); | |
| lines.push(ctaLine); | |
| return lines.join('\n\n'); | |
| } | |
| function buildDynamicWelcomeMessage(member, config) { | |
| const welcomeDynamic = config?.welcome?.dynamic || {}; | |
| const timezone = welcomeDynamic.timezone || 'America/New_York'; | |
| const memberContext = { | |
| id: member.id, | |
| username: member.user?.username || 'Unknown', | |
| server: member.guild?.name || 'the server', | |
| memberCount: member.guild?.memberCount || 0, | |
| }; | |
| const timeOfDay = getTimeOfDay(timezone); | |
| const snapshot = getCommunitySnapshot(member.guild, welcomeDynamic); | |
| const milestoneLine = getMilestoneLine(memberContext.memberCount, welcomeDynamic); | |
| const suggestedChannels = getSuggestedChannels(member, config, snapshot); | |
| const greeting = pickFrom(getGreetingTemplates(timeOfDay), memberContext); | |
| const vibeLine = buildVibeLine(snapshot, suggestedChannels); | |
| const ctaLine = buildCtaLine(suggestedChannels); | |
| const lines = [greeting]; | |
| if (milestoneLine) { | |
| lines.push(milestoneLine); | |
| } else if (memberContext.memberCount > 0) { | |
| lines.push(`You just rolled in as member **#${memberContext.memberCount}**.`); | |
| } | |
| lines.push(vibeLine); | |
| lines.push(ctaLine); | |
| return lines.join('\n\n'); | |
| } |
🤖 Prompt for AI Agents
In `@src/modules/welcome.js` around lines 107 - 139, In
buildDynamicWelcomeMessage, avoid emitting "member `#0`" when guild.memberCount is
unavailable: detect when member.guild?.memberCount is undefined/null (instead of
defaulting to 0) and pass a nullable/undefined count into getMilestoneLine; then
only add the fallback "You just rolled in as member ..." line when a real count
exists (use memberContext.memberCount truthiness or an explicit isNumber check)
— update the memberContext creation and the conditional around the fallback so
getMilestoneLine(memberContext.memberCount, ...) and the fallback line are
skipped if the count is missing.
| if (snapshot.voiceChannels > 0) { | ||
| return `${snapshot.voiceParticipants} ${snapshot.voiceParticipants === 1 ? 'person is' : 'people are'} hanging out in voice right now, and ${channelText} is waking up.`; | ||
| } |
There was a problem hiding this comment.
Grammar: "is waking up" should be "are waking up" when two channels are listed.
channelText can be a compound subject like <#A> + <#B>. Using "is" reads as "channel1 + channel2 is waking up." The same issue applies on Line 250 ("is where people are checking in").
Proposed fix
if (snapshot.voiceChannels > 0) {
- return `${snapshot.voiceParticipants} ${snapshot.voiceParticipants === 1 ? 'person is' : 'people are'} hanging out in voice right now, and ${channelText} is waking up.`;
+ return `${snapshot.voiceParticipants} ${snapshot.voiceParticipants === 1 ? 'person is' : 'people are'} hanging out in voice right now, and ${channelText} ${channelList.length === 1 ? 'is' : 'are'} waking up.`;
}
return hasChannels
- ? `It's a chill moment, but ${channelText} is where people are checking in.`
+ ? `It's a chill moment, but ${channelText} ${channelList.length === 1 ? 'is' : 'are'} where people are checking in.`
: `It's a chill moment — perfect time to say hello.`;🤖 Prompt for AI Agents
In `@src/modules/welcome.js` around lines 246 - 248, The sentence uses a singular
verb "is" with channelText which may refer to multiple channels; update the
template to pick the verb based on the number of channels (use
snapshot.voiceChannels) instead of always "is" — e.g. replace the hardcoded "is"
in the string containing ${channelText} with a conditional like
snapshot.voiceChannels === 1 ? 'is' : 'are' (apply the same change to the other
similar line that currently uses "is where people are checking in"), referencing
snapshot.voiceChannels, snapshot.voiceParticipants and channelText to determine
singular vs plural.
Rebased PR #15 changes on top of main (post PR #14 merge). Deep config path autocomplete: - Nested key traversal with dot-notation (e.g. ai.model, welcome.enabled) - Array-aware setNestedValue with numeric index support - Dynamic section autocomplete from live config (not static choices) - collectConfigPaths handles arrays, empty objects/arrays Dynamic welcome messages: - Community snapshots with member/channel/voice activity stats - GuildVoiceStates intent for voice channel tracking - Welcome message module with configurable templates Review fixes (Rounds 1-10): - structuredClone for safe config cloning (no shared refs) - Array.isArray guards in section reset and cache updates - Channel validity filtering - Section name sanitization (escapeInlineCode) - INSERT ON CONFLICT for concurrent section inserts - resetConfig error handling with graceful DB fallback - Re-entrancy guard in initDb - getSslConfig helper with DATABASE_SSL env var support - fileConfigCache for single config.json load - Stale timestamp pruning in getCommunitySnapshot
BillChirico
force-pushed
the
feat/deep-autocomplete-dynamic-welcome
branch
from
20f7f8e to
60e8c4d
Compare
| } catch (txErr) { | ||
| try { await client.query('ROLLBACK'); } catch { /* ignore rollback failure */ } | ||
| throw txErr; | ||
| logError('Database error during full config reset — updating in-memory only', { error: txErr.message }); |
There was a problem hiding this comment.
Full config reset silently swallows DB transaction errors
Medium Severity
The full resetConfig path changed from throw txErr to logError(...), silently swallowing database transaction failures. The handleReset caller then sends a success embed ("All configuration has been reset to defaults") even though the database still holds the old values. On bot restart, the unreset config reloads from the database, making the reset temporary without the admin knowing.
Additional Locations (1)
| } catch { | ||
| return new Date().getHours(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Midnight returns hour 24 due to h24 hourCycle
Low Severity
getHourInTimezone uses hour12: false with the en-US locale, which resolves to the h24 hourCycle. This is a well-documented JavaScript quirk that formats midnight as "24" instead of "00", so the function returns 24 rather than 0. The current getTimeOfDay logic happens to classify 24 as 'night' correctly, but the return value is outside the expected 0–23 range. Replacing hour12: false with hourCycle: 'h23' would produce the correct range.
- Test refreshDiscordToken: success, failure, token rotation, no refresh token (issue #15) - Test fetchWithRateLimit: 429 response, retry-after parsing, max retries exhaustion (issue #16) - Test proxy: actual redirect behavior for unauthenticated requests, passthrough for valid tokens, callbackUrl preservation (issue #17) - Fix mock types: complete Account/Token objects with all required fields (issue #18) - Update guilds API test to use getToken() instead of session.accessToken - Update landing page tests for conditional invite button rendering - Update header/dashboard-shell/server-selector tests for refactored components - Add health check endpoint test - All 82 tests passing, lint clean, build succeeds
- Differentiate requireGuildAdmin (ADMINISTRATOR only) from requireGuildModerator (ADMINISTRATOR | MANAGE_GUILD), aligning REST admin check with slash-command isAdmin (#1, #2, #12) - Add botOwners startup warning when using default upstream ID (#3) - Add SESSION_SECRET, DISCORD_CLIENT_SECRET, DISCORD_REDIRECT_URI to README deployment table (#4) - Pass actual permission level to getPermissionError so modlog denial says 'moderator' not 'administrator' (#5) - Guard _seedOAuthState with NODE_ENV production check (#6) - Add test: valid JWT with no server-side session (#7) - Add DiscordApiError class with HTTP status (#8) - Add moderatorRoleId support to isModerator (#9) - Remove no-op delete override from SessionStore (#10) - Cap oauthStates at 10k entries (#11) - Fix hasOAuthGuildPermission docstring for bitwise OR semantics (#12) - Handle dashboard URL fragment collision (#13) - Cap guildCache at 10k entries (#14) - Add SESSION_TTL_MS co-location comment with JWT expiry (#15) - Cache SESSION_SECRET via lazy getter in verifyJwt (#16) - Remove PII (username) from OAuth auth log (#17)
- Differentiate requireGuildAdmin (ADMINISTRATOR only) from requireGuildModerator (ADMINISTRATOR | MANAGE_GUILD), aligning REST admin check with slash-command isAdmin (#1, #2, #12) - Add botOwners startup warning when using default upstream ID (#3) - Add SESSION_SECRET, DISCORD_CLIENT_SECRET, DISCORD_REDIRECT_URI to README deployment table (#4) - Pass actual permission level to getPermissionError so modlog denial says 'moderator' not 'administrator' (#5) - Guard _seedOAuthState with NODE_ENV production check (#6) - Add test: valid JWT with no server-side session (#7) - Add DiscordApiError class with HTTP status (#8) - Add moderatorRoleId support to isModerator (#9) - Remove no-op delete override from SessionStore (#10) - Cap oauthStates at 10k entries (#11) - Fix hasOAuthGuildPermission docstring for bitwise OR semantics (#12) - Handle dashboard URL fragment collision (#13) - Cap guildCache at 10k entries (#14) - Add SESSION_TTL_MS co-location comment with JWT expiry (#15) - Cache SESSION_SECRET via lazy getter in verifyJwt (#16) - Remove PII (username) from OAuth auth log (#17)
… (#83) * feat(dashboard): add Discord entity pickers Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(dashboard): add diff view and polish - ConfigDiff.tsx: visual diff component showing before/after config changes with green additions and red deletions using the `diff` library - SystemPromptEditor.tsx: textarea with real-time character count, max length warning indicator, and accessible labeling - Toast notifications via sonner: success/error toasts on save, load failures, and reset actions positioned bottom-right - ResetDefaultsButton with confirmation dialog using Radix UI Dialog - ConfigEditor.tsx: full config editing page with AI, welcome message, and moderation sections; PATCH-based save with diff preview - Config API proxy route (GET/PATCH) following established analytics proxy pattern with guild admin authorization - Dialog UI component (shadcn/ui new-york style) - Added lint script to web/package.json Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * polish(config-editor): improve UX, accessibility, and edge case handling - Replace native checkboxes with styled toggle switches using proper role="switch" and aria-checked attributes - Add unsaved changes guard (beforeunload warning + yellow banner) - Add Ctrl/Cmd+S keyboard shortcut for saving - Block save when system prompt exceeds character limit - Rename misleading "Reset to Defaults" to "Discard Changes" with accurate dialog copy (reverts to last saved state, not factory defaults) - Add diff summary counts (+N / -N) to the pending changes card - Improve accessibility throughout: aria-labels on loading spinner, aria-describedby linking textareas to their hints, aria-invalid on over-limit prompt, aria-live on character counter, aria-hidden on decorative icons, role="region" on diff view - Memoize hasChanges and hasValidationErrors to avoid redundant JSON.stringify on every render - Validate PATCH body shape in API route before proxying upstream - Fix stale "bills-bot" prefix in guild-selection localStorage keys (missed during volvox rename) Closes #31 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(api): add config endpoints for Issue #31 Add REST API endpoints for managing bot configuration: - GET /api/v1/config - Retrieve current config (ai, welcome, spam, moderation) - PUT /api/v1/config - Update config with schema validation Features: - Type-safe schema validation for config sections - Flatten nested objects to dot-notation paths for persistence - requireGlobalAdmin middleware (API-secret or bot-owner OAuth) - Proper HTTP error codes (400 for validation, 401/403 for auth, 500 for errors) - Added PUT to CORS methods Tests: - 35 comprehensive tests covering auth, validation, types, edge cases - Tests for validateConfigSchema and flattenToLeafPaths exports Closes #31 * feat(api): add webhook notifications for config changes - Add notifyDashboardWebhook() fire-and-forget sender to PATCH /:id/config - POST /webhooks/config-update endpoint for dashboard to push config changes - Webhook uses DASHBOARD_WEBHOOK_URL env var with 5s timeout - Add comprehensive tests for webhook functionality * feat(dashboard): add config editor with Zustand state management Add a full config editor UI at /dashboard/config with: - Proxy API routes (GET + PATCH) for bot config at /api/guilds/:guildId/config - Zustand store for config state with fetch, update, and debounced saves - Accordion-based sections for ai, welcome, spam, moderation (read-only), triage - Recursive field renderer supporting booleans, numbers, strings, arrays, objects - shadcn/ui components: accordion, input, label, switch, textarea Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(dashboard): enhance Discord entity pickers with multi-select and Zustand - Add multi-select mode to ChannelSelector and RoleSelector (multiple?: boolean prop) - Create Zustand store for caching channels/roles per guild - Add dedicated bot API endpoints: GET /:id/channels and GET /:id/roles - Add Next.js proxy routes for channels and roles - Update AGENTS.md with new key files Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: remove unused import in webhooks.js * fix: resolve all linting errors across codebase - Migrate biome config from 2.3.14 to 2.4.0 - Fix unused imports (triage.js, modAction.test.js) - Fix import ordering (auth.js, lock.js, unlock.js, ai.js, triage-respond.js, modAction.js, modAction.test.js) - Fix formatting across 19 files - Replace O(n²) spread in reduce with push (cli-process.test.js) - Use Object.hasOwn() instead of Object.prototype.hasOwnProperty (config-guild.test.js) All 1310 tests pass. * feat: add full config editing support for moderation and triage - Add moderation and triage to SAFE_CONFIG_KEYS in guilds.js, webhooks.js, and config.js making them writable via PATCH/PUT endpoints - Expand READABLE_CONFIG_KEYS to include all sections: ai, welcome, spam, moderation, triage, logging, memory, permissions - Add CONFIG_SCHEMA definitions for moderation and triage sections with full type validation - Update WritableConfigSection type to include moderation and triage - Remove moderation from READ_ONLY_SECTIONS in config-section.tsx - Update config-store.ts writable keys check - Add editable moderation section in dashboard config-editor with toggles for enabled, autoDelete, DM notifications, and escalation - Add editable triage section with fields for models, budgets, intervals, streaming, debug footer, and moderation log channel - Update all test assertions to reflect new writable sections Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(security): add webhook URL validation, schema validation on all write paths, atomic writes - Add shared validateWebhookUrl() utility that blocks SSRF (localhost, private IPs, link-local, IPv6 loopback) and enforces https in production - Wire URL validation into config.js notifyWebhook and guilds.js notifyDashboardWebhook - Export validateSingleValue() from config.js and apply it to the PATCH endpoint in guilds.js and the POST /config-update webhook endpoint - Add path length (<=200 chars) and depth (<=10 segments) limits to guilds.js PATCH and webhooks.js POST endpoints - Refactor PUT handler in config.js to track per-field write results: returns 200 on full success, 207 on partial failure, 500 when all fail - Add comprehensive tests for all new validations and error responses Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: extract shared config allowlist, webhook utility, and proxy helpers; remove dead code Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(frontend): batch saves, fix race conditions, DRY constants, localStorage safety - Batch saveChanges into parallel PATCH requests grouped by section instead of sequential individual PATCHes (I5) - Add request sequence counter to Zustand config store to prevent stale PATCH responses from clobbering newer state (I6) - Centralize SYSTEM_PROMPT_MAX_LENGTH constant in types/config.ts and import in config-editor and system-prompt-editor (M2) - Wrap localStorage.getItem in try/catch for SSR safety (M3) - Fix channels.length / roles.length truthiness bug — use !== undefined instead of .length which is falsy for 0 (M5) - Replace JSON.stringify deep comparison with recursive deepEqual utility function (M8) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(security): mask sensitive config fields, block IPv4-mapped IPv6 SSRF, reject unknown config paths - Add SENSITIVE_FIELDS set and maskSensitiveFields utility to strip triage API keys (classifyApiKey, respondApiKey) from all GET config responses - Block SSRF via IPv4-mapped IPv6 addresses (::ffff:127.0.0.1, hex form ::ffff:7f00:1, and cloud metadata ::ffff:169.254.169.254) - Reject unknown config paths in validateSingleValue instead of silently accepting them without type checking - Add cache size limit (100 entries) to webhook URL validation cache - Guard flattenToLeafPaths against __proto__/constructor/prototype keys Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(backend): extract shared validators and getBotOwnerIds, add webhook utility tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(frontend): remove dead code, fix save flow, harden inputs and type guards - C4: delete 10 unused files (stores, UI components, dashboard selectors) and remove zustand, @radix-ui/react-accordion, @radix-ui/react-label, @radix-ui/react-switch from package.json - m8: replace ~85-line local GuildConfig interface with DeepPartial<BotConfig> (add DeepPartial utility to types/config.ts) - m4: harden isGuildConfig type guard to verify at least one expected section key (ai, welcome, spam) instead of just typeof === "object" - M6: fix computePatches to include top-level paths (remove incorrect fullPath.includes(".") guard that silently dropped top-level field changes) - M7: fix partial save to merge only succeeded sections into savedConfig on partial failure, preserving draft edits for failed sections; only call fetchConfig() on full success - m5: add min constraints to triage number inputs (budgets min=0, timeouts min=1, buffer/context sizes min=1) - m9: add e.returnValue = "" to beforeunload handler for modern browser support Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test: fix 10-segment test to use valid schema path after strict validation * fix: wire default-personality.md as system prompt fallback Replace generic 'You are a helpful Discord bot.' fallback with the existing default-personality.md template, which provides the intended Volvox Bot personality, role constraints, and anti-abuse guardrails. * fix: merge default-personality into responder system prompt, remove dead template Merge personality, role, constraints, and anti-abuse from the unused default-personality.md into triage-respond-system.md (the responder's actual system prompt). Revert the fallback wiring in triage-prompt.js since personality now lives in the system prompt file where it belongs. Delete default-personality.md — no longer needed. * 📝 Add docstrings to `feat/config-editor-combined` Docstrings generation was requested by @BillChirico. The following files were modified: * `src/api/routes/auth.js` * `src/api/routes/config.js` * `src/api/routes/guilds.js` * `src/api/utils/configAllowlist.js` * `src/api/utils/validateConfigPatch.js` * `src/api/utils/validateWebhookUrl.js` * `src/commands/lock.js` * `src/commands/slowmode.js` * `src/commands/unlock.js` * `src/modules/memory.js` * `src/modules/triage-prompt.js` * `src/modules/triage-respond.js` * `src/modules/triage.js` * `src/utils/debugFooter.js` * `src/utils/permissions.js` * `web/src/app/api/guilds/[guildId]/channels/route.ts` * `web/src/app/api/guilds/[guildId]/config/route.ts` * `web/src/app/api/guilds/[guildId]/roles/route.ts` * `web/src/app/dashboard/config/page.tsx` * `web/src/components/dashboard/config-diff.tsx` * `web/src/components/dashboard/config-editor.tsx` * `web/src/components/dashboard/reset-defaults-button.tsx` * `web/src/components/dashboard/system-prompt-editor.tsx` * `web/src/components/providers.tsx` * `web/src/lib/bot-api-proxy.ts` These files were kept as they were: * `src/api/server.js` * `src/api/utils/webhook.js` These files were ignored: * `tests/api/routes/config.test.js` * `tests/api/routes/guilds.test.js` * `tests/api/routes/webhooks.test.js` * `tests/api/utils/validateWebhookUrl.test.js` * `tests/api/utils/webhook.test.js` * `tests/commands/tempban.test.js` * `tests/config-listeners.test.js` * `tests/modules/cli-process.test.js` * `tests/modules/config-guild.test.js` * `tests/utils/modAction.test.js` These file types are not supported: * `.env.example` * `AGENTS.md` * `biome.json` * `src/prompts/triage-respond-system.md` * `web/package.json` * fix(prompts): replace ambiguous 'classified' with 'triaged' (Thread #11) * fix(prompts): define concrete 'flagging' mechanism for moderation (Thread #12) * fix(prompts): add PII/credentials constraint to prevent echoing secrets (Thread #13) * fix: remove bracketed-IPv6 dead code in webhook URL validator URL.hostname already strips brackets from IPv6 addresses (new URL('http://[::1]').hostname === '::1'), so the hostname.startsWith('[') branch was unreachable dead code. Remove the bracketed-IPv6 branch and the '[::1]' entry from BLOCKED_IPV6 since it could never match. Addresses CodeRabbit review thread #9. * fix: sanitize webhook URL in warning logs to prevent credential exposure Strip query string and fragment from webhook URLs before including them in warning log messages. If a webhook URL contains tokens or API keys as query parameters, they would previously appear in logs. Now logs only origin + pathname (e.g. 'https://example.com/hook') instead of the full URL with sensitive query params. Addresses CodeRabbit review thread #10. * test(config): rename misleading 'Infinity' test name (Thread #14) * fix: normalize validateSingleValue to always return string[] Unknown config paths previously returned [{path, message}] objects while validateValue returned string[]. Callers now receive a consistent string[] in both cases, eliminating the need to handle two different shapes. * test(config): move PUT partial write handling inside config routes suite (Thread #15) * refactor: extract getGuildChannels helper to remove duplication GET /:id and GET /:id/channels shared identical channel-fetching loops with a duplicated MAX_CHANNELS constant. Extracted into a single getGuildChannels(guild) helper used by both handlers. * feat: include section (topLevelKey) in PATCH config webhook payload Downstream consumers of the DASHBOARD_WEBHOOK_URL can now use the 'section' field to selectively reload only the affected config section rather than refreshing the entire config. * fix: return promise in webhook config-update handler (async/await) The previous .then()/.catch() chain was not returned, so Express 5 could not auto-forward rejected promises to the error handler. Converted to async/await so errors propagate correctly. * feat: make JSON body size limit configurable via API_BODY_LIMIT env var Defaults to '100kb' when the env var is not set, preserving existing behaviour. Operators can now tune the limit without code changes. * test(validateWebhookUrl): strengthen cache eviction test to verify re-evaluation (Thread #16) * test(webhook): move vi.useRealTimers() to afterEach to prevent timer leak (Thread #17) * refactor: move CONFIG_SCHEMA/validateValue/validateSingleValue to utils/configValidation.js validateConfigPatch.js (utils) was importing validateSingleValue from routes/config.js — an inverted dependency. Created src/api/utils/configValidation.js as the canonical home for CONFIG_SCHEMA, validateValue, and validateSingleValue. - config.js now imports from ../utils/configValidation.js and re-exports validateSingleValue for backward compatibility with existing callers. - validateConfigPatch.js now imports from ./configValidation.js directly. * perf: split path string once in validateConfigPatchBody path.split('.') was called twice — once to extract topLevelKey and again for segments. Moved the single split to the top and derived topLevelKey from segments[0], avoiding the redundant allocation. * fix(#18): change lint script from tsc to biome check * fix(#19,#20): simplify params type; add PATCH body value check * fix(#21): add metadata export to config page * fix(#22): compute addedCount/removedCount inside useMemo loop * fix(#23,#24,#25,#26): tighten isGuildConfig; extract inputClasses; guard number inputs; rename DiscardChangesButton * fix(#27): change aria-live from polite to off on char counter * fix(#28): change Toaster theme from dark to system * fix(#29,#30): export BotApiConfig; return 504 on AbortError/TimeoutError * fix(#31): add one-time localStorage key migration from old key * fix(#32,#33,#34): SpamConfig JSDoc; collapse WritableConfigSection; fix SYSTEM_PROMPT_MAX_LENGTH JSDoc * fix: remove unused validateSingleValue import, fix biome formatting in config.js After the refactor, validateSingleValue is re-exported directly via 'export { } from' and no longer needs a local import binding. Also removed an extra blank line that biome flagged as a format error. * fix: add DNS resolution validation to prevent SSRF via DNS rebinding Add async validateDnsResolution() that resolves a webhook hostname via DNS and checks all resolved IPs against the existing blocked ranges before fetch. This closes the TOCTOU gap where a hostname could pass string-based validation then resolve to a private IP at request time (DNS rebinding attack). Changes: - Add validateDnsResolution() with resolve4/resolve6 checks - Integrate DNS check in fireAndForgetWebhook before fetch - Normalize IPv6 hostnames by stripping brackets (Node.js v22 retains them in URL.hostname, contrary to WHATWG spec) - Add comprehensive test coverage for DNS rebinding scenarios - Update webhook tests for async DNS validation flow Addresses CodeRabbit review thread #8. * fix: update test assertions for string[] return type from validateSingleValue * fix: mock validateDnsResolution in webhook integration tests After adding DNS resolution pinning in fireAndForgetWebhook, the config and guilds route tests need to mock validateDnsResolution to return true so fetch is actually called. * fix: address minor code review feedback - JSDoc, tests, caps * fix(frontend): address code review feedback - HTML, types, perf * fix(backend): address code review feedback - validation, logging, exports * fix: correct IPv6 validation for public addresses and literals * fix: restore classifier invocation in triage module * fix: address test failures in validateConfigPatch and triage-respond - Check for empty path segments before topLevelKey validation - Fix test to use valid nullable path (welcome.channelId) - Add mock cleanup between triage-respond tests * fix(validation): handle alertChannelId nullable and DNS edge cases * fix(security): prevent mask sentinel write-back and auth secret override 1. configAllowlist: Add isMasked() and stripMaskedWrites() to detect and filter out writes where sensitive fields contain the mask sentinel ('••••••••'). Prevents clients from accidentally overwriting real secrets with the placeholder returned by maskSensitiveFields(). 2. bot-api-proxy: Reorder header spread so x-api-secret is always set AFTER spreading options.headers, preventing caller-provided headers from overriding the server-side auth secret. Both fixes include comprehensive tests. * test: add missing test cases for mask sentinel, prototype pollution, DNS edge cases * refactor: simplify webhook validation for internal-only use * refactor: remove unused SSRF validation code Deleted validateWebhookUrl.js and its tests since webhooks are internal-only. Simplified webhook.js to just check URL format. * fix: prevent WebSearch notification failures from aborting response * fix: correct safeSend mock setup in triage-respond tests * fix(security): use own-property checks in config validation * fix: export MASK constant and clean up orphaned JSDoc * fix: report written sections in webhook, add roles endpoint test * fix: address remaining PR review feedback - Add nullable: true to triage.moderationLogChannel and debugFooterLevel - Add evalClient param to runResponder JSDoc - Convert SAFE_CONFIG_KEYS to Set for O(1) lookups - Reorder validation checks for consistent 400 responses - Update tests for Set-based SAFE_CONFIG_KEYS --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: AnExiledDev <AnExiledDev@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
Changes
Deep Config Autocomplete
/config setwith recursive traversal viacollectConfigPaths()chimeIn.maxBufferSize,permissions.allowedCommands.configDynamic Welcome Messages
welcome.jswelcome.dynamicconfig defaults inconfig.jsonFiles Changed
src/commands/config.js— recursive autocompletesrc/modules/welcome.js— dynamic welcome systemsrc/modules/events.js— activity trackingconfig.json— welcome.dynamic defaultsREADME.md— updated docs