Skip to content

Commit 66c54ac

Browse files
committed
fix: address all CodeRabbit + Bugbot review comments
src/db.js: - Add pool.on('error') handler to prevent unhandled errors crashing process - Make SSL configurable via DB_SSL_REJECT_UNAUTHORIZED env var (defaults to rejectUnauthorized:true instead of insecure false) - Add re-entrancy guard to initDb to prevent double initialization - Clean up pool on init failure so getPool() doesn't return broken instance - Wrap closeDb in try/catch so shutdown errors don't crash src/modules/config.js: - Remove process.exit from loadConfigFromFile — always throw instead (no runtime code path should hard-exit the process) - Remove unused getConfigSection export (dead code) - Remove unused sectionConfig variable (dead store in setConfigValue) - Fix cache-before-DB-write: write to database first, then update cache - Fix concurrent section writes: use transaction with FOR UPDATE row lock - Fix deep nested paths: read-modify-write in JS instead of jsonb_set (jsonb_set silently drops updates when intermediate keys don't exist) - Fix parseValue: preserve large numeric strings beyond MAX_SAFE_INTEGER - Wrap DB seeding in transaction to prevent partial config loss on restart src/commands/config.js: - Fix collectConfigPaths to emit leaf-only entries (was emitting branches too) - Derive valid sections from live config instead of hardcoded VALID_SECTIONS - Fix 'New Value' display: traverse full dot-path instead of only first key (path.split('.').slice(1)[0] broke for paths deeper than section.key) - Add embed 6000-char limit guard with truncation notice - Switch section options to autocomplete (derived from live config) src/index.js: - Wrap closeDb in try/catch in gracefulShutdown README.md: - Change json code block to jsonc (has // comments)
1 parent 84f5ed3 commit 66c54ac

5 files changed

Lines changed: 318 additions & 150 deletions

File tree

README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ AI-powered Discord bot for the Volvox community.
5151

5252
## Config
5353

54-
```json
54+
```jsonc
5555
{
5656
"ai": {
5757
"enabled": true,

src/commands/config.js

Lines changed: 77 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,6 @@
66
import { SlashCommandBuilder, EmbedBuilder } from 'discord.js';
77
import { getConfig, setConfigValue, resetConfig } from '../modules/config.js';
88

9-
const VALID_SECTIONS = ['ai', 'chimeIn', 'welcome', 'moderation', 'logging', 'permissions'];
10-
119
export const data = new SlashCommandBuilder()
1210
.setName('config')
1311
.setDescription('View or manage bot configuration (Admin only)')
@@ -20,14 +18,7 @@ export const data = new SlashCommandBuilder()
2018
.setName('section')
2119
.setDescription('Specific config section to view')
2220
.setRequired(false)
23-
.addChoices(
24-
{ name: 'AI Settings', value: 'ai' },
25-
{ name: 'Chime In', value: 'chimeIn' },
26-
{ name: 'Welcome Messages', value: 'welcome' },
27-
{ name: 'Moderation', value: 'moderation' },
28-
{ name: 'Logging', value: 'logging' },
29-
{ name: 'Permissions', value: 'permissions' }
30-
)
21+
.setAutocomplete(true)
3122
)
3223
)
3324
.addSubcommand(subcommand =>
@@ -57,38 +48,30 @@ export const data = new SlashCommandBuilder()
5748
.setName('section')
5849
.setDescription('Section to reset (omit to reset all)')
5950
.setRequired(false)
60-
.addChoices(
61-
{ name: 'AI Settings', value: 'ai' },
62-
{ name: 'Chime In', value: 'chimeIn' },
63-
{ name: 'Welcome Messages', value: 'welcome' },
64-
{ name: 'Moderation', value: 'moderation' },
65-
{ name: 'Logging', value: 'logging' },
66-
{ name: 'Permissions', value: 'permissions' }
67-
)
51+
.setAutocomplete(true)
6852
)
6953
);
7054

7155
export const adminOnly = true;
7256

7357
/**
74-
* Recursively collect dot-notation paths for a config object.
75-
* Includes object paths, leaf paths, and nested array/object paths.
58+
* Recursively collect leaf-only dot-notation paths for a config object.
59+
* Only emits paths that point to non-object values (leaves).
7660
* @param {*} source - Config value to traverse
7761
* @param {string} [prefix] - Current path prefix
7862
* @param {string[]} [paths] - Accumulator array
79-
* @returns {string[]} Dot-notation config paths
63+
* @returns {string[]} Dot-notation config paths (leaf-only)
8064
*/
8165
function collectConfigPaths(source, prefix = '', paths = []) {
8266
if (Array.isArray(source)) {
8367
source.forEach((value, index) => {
8468
const path = prefix ? `${prefix}.${index}` : String(index);
85-
paths.push(path);
86-
8769
if (value && typeof value === 'object') {
8870
collectConfigPaths(value, path, paths);
71+
} else {
72+
paths.push(path);
8973
}
9074
});
91-
9275
return paths;
9376
}
9477

@@ -98,45 +81,52 @@ function collectConfigPaths(source, prefix = '', paths = []) {
9881

9982
for (const [key, value] of Object.entries(source)) {
10083
const path = prefix ? `${prefix}.${key}` : key;
101-
paths.push(path);
102-
10384
if (value && typeof value === 'object') {
10485
collectConfigPaths(value, path, paths);
86+
} else {
87+
paths.push(path);
10588
}
10689
}
10790

10891
return paths;
10992
}
11093

11194
/**
112-
* Handle autocomplete for config paths
95+
* Handle autocomplete for config paths and section names
11396
* @param {Object} interaction - Discord interaction
11497
*/
11598
export async function autocomplete(interaction) {
116-
const focusedValue = interaction.options.getFocused().toLowerCase().trim();
99+
const focusedOption = interaction.options.getFocused(true);
100+
const focusedValue = focusedOption.value.toLowerCase().trim();
117101
const config = getConfig();
118102

119-
const paths = collectConfigPaths(config);
120-
121-
const filtered = paths
122-
.filter(p => p.toLowerCase().includes(focusedValue))
123-
.sort((a, b) => {
124-
const aLower = a.toLowerCase();
125-
const bLower = b.toLowerCase();
126-
const aStartsWithFocus = aLower.startsWith(focusedValue);
127-
const bStartsWithFocus = bLower.startsWith(focusedValue);
128-
129-
if (aStartsWithFocus !== bStartsWithFocus) {
130-
return aStartsWithFocus ? -1 : 1;
131-
}
132-
133-
return aLower.localeCompare(bLower);
134-
})
135-
.slice(0, 25);
103+
let choices;
104+
if (focusedOption.name === 'section') {
105+
// Autocomplete section names from live config
106+
choices = Object.keys(config)
107+
.filter(s => s.toLowerCase().includes(focusedValue))
108+
.slice(0, 25)
109+
.map(s => ({ name: s, value: s }));
110+
} else {
111+
// Autocomplete dot-notation paths (leaf-only)
112+
const paths = collectConfigPaths(config);
113+
choices = paths
114+
.filter(p => p.toLowerCase().includes(focusedValue))
115+
.sort((a, b) => {
116+
const aLower = a.toLowerCase();
117+
const bLower = b.toLowerCase();
118+
const aStartsWithFocus = aLower.startsWith(focusedValue);
119+
const bStartsWithFocus = bLower.startsWith(focusedValue);
120+
if (aStartsWithFocus !== bStartsWithFocus) {
121+
return aStartsWithFocus ? -1 : 1;
122+
}
123+
return aLower.localeCompare(bLower);
124+
})
125+
.slice(0, 25)
126+
.map(p => ({ name: p, value: p }));
127+
}
136128

137-
await interaction.respond(
138-
filtered.map(p => ({ name: p, value: p }))
139-
);
129+
await interaction.respond(choices);
140130
}
141131

142132
/**
@@ -159,6 +149,9 @@ export async function execute(interaction) {
159149
}
160150
}
161151

152+
/** @type {number} Discord embed total character limit */
153+
const EMBED_CHAR_LIMIT = 6000;
154+
162155
/**
163156
* Handle /config view
164157
*/
@@ -183,25 +176,46 @@ async function handleView(interaction) {
183176
}
184177

185178
embed.setDescription(`**${section.toUpperCase()} Configuration**`);
179+
const sectionJson = JSON.stringify(sectionData, null, 2);
186180
embed.addFields({
187181
name: 'Settings',
188-
value: '```json\n' + JSON.stringify(sectionData, null, 2) + '\n```'
182+
value: '```json\n' + (sectionJson.length > 1000 ? sectionJson.slice(0, 997) + '...' : sectionJson) + '\n```'
189183
});
190184
} else {
191185
embed.setDescription('Current bot configuration');
192186

187+
// Track cumulative embed size to stay under Discord's 6000-char limit
188+
let totalLength = embed.data.title.length + embed.data.description.length;
189+
let truncated = false;
190+
193191
for (const [key, value] of Object.entries(config)) {
194192
const jsonStr = JSON.stringify(value, null, 2);
195-
const truncated = jsonStr.length > 1000
196-
? jsonStr.slice(0, 997) + '...'
197-
: jsonStr;
198-
193+
const fieldValue = '```json\n' + (jsonStr.length > 1000 ? jsonStr.slice(0, 997) + '...' : jsonStr) + '\n```';
194+
const fieldName = key.toUpperCase();
195+
const fieldLength = fieldName.length + fieldValue.length;
196+
197+
if (totalLength + fieldLength > EMBED_CHAR_LIMIT - 200) {
198+
// Reserve space for a truncation notice
199+
embed.addFields({
200+
name: '⚠️ Truncated',
201+
value: 'Use `/config view section:<name>` to see remaining sections.',
202+
inline: false
203+
});
204+
truncated = true;
205+
break;
206+
}
207+
208+
totalLength += fieldLength;
199209
embed.addFields({
200-
name: `${key.toUpperCase()}`,
201-
value: '```json\n' + truncated + '\n```',
210+
name: fieldName,
211+
value: fieldValue,
202212
inline: false
203213
});
204214
}
215+
216+
if (truncated) {
217+
embed.setFooter({ text: 'Some sections omitted • Use /config view section:<name> for details' });
218+
}
205219
}
206220

207221
await interaction.reply({ embeds: [embed], ephemeral: true });
@@ -220,11 +234,12 @@ async function handleSet(interaction) {
220234
const path = interaction.options.getString('path');
221235
const value = interaction.options.getString('value');
222236

223-
// Validate section exists
237+
// Validate section exists in live config
224238
const section = path.split('.')[0];
225-
if (!VALID_SECTIONS.includes(section)) {
239+
const validSections = Object.keys(getConfig());
240+
if (!validSections.includes(section)) {
226241
return await interaction.reply({
227-
content: `❌ Invalid section '${section}'. Valid sections: ${VALID_SECTIONS.join(', ')}`,
242+
content: `❌ Invalid section '${section}'. Valid sections: ${validSections.join(', ')}`,
228243
ephemeral: true
229244
});
230245
}
@@ -234,12 +249,15 @@ async function handleSet(interaction) {
234249

235250
const updatedSection = await setConfigValue(path, value);
236251

252+
// Traverse to the actual leaf value for display
253+
const leafValue = path.split('.').slice(1).reduce((obj, k) => obj?.[k], updatedSection);
254+
237255
const embed = new EmbedBuilder()
238256
.setColor(0x57F287)
239257
.setTitle('✅ Config Updated')
240258
.addFields(
241259
{ name: 'Path', value: `\`${path}\``, inline: true },
242-
{ name: 'New Value', value: `\`${JSON.stringify(updatedSection[path.split('.').slice(1)[0]], null, 2) ?? value}\``, inline: true }
260+
{ name: 'New Value', value: `\`${JSON.stringify(leafValue, null, 2) ?? value}\``, inline: true }
243261
)
244262
.setFooter({ text: 'Changes take effect immediately' })
245263
.setTimestamp();

src/db.js

Lines changed: 67 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -11,45 +11,75 @@ const { Pool } = pg;
1111
/** @type {pg.Pool | null} */
1212
let pool = null;
1313

14+
/** @type {boolean} Re-entrancy guard for initDb */
15+
let initializing = false;
16+
1417
/**
1518
* Initialize the database connection pool and create schema
1619
* @returns {Promise<pg.Pool>} The connection pool
1720
*/
1821
export async function initDb() {
19-
const connectionString = process.env.DATABASE_URL;
20-
if (!connectionString) {
21-
throw new Error('DATABASE_URL environment variable is not set');
22+
if (pool) return pool;
23+
if (initializing) {
24+
throw new Error('initDb is already in progress');
2225
}
2326

24-
pool = new Pool({
25-
connectionString,
26-
max: 5,
27-
idleTimeoutMillis: 30000,
28-
connectionTimeoutMillis: 10000,
29-
// Railway internal connections don't need SSL
30-
ssl: connectionString.includes('railway.internal') ? false : { rejectUnauthorized: false },
31-
});
32-
33-
// Test connection
34-
const client = await pool.connect();
27+
initializing = true;
3528
try {
36-
await client.query('SELECT NOW()');
37-
info('Database connected');
38-
} finally {
39-
client.release();
40-
}
29+
const connectionString = process.env.DATABASE_URL;
30+
if (!connectionString) {
31+
throw new Error('DATABASE_URL environment variable is not set');
32+
}
4133

42-
// Create schema
43-
await pool.query(`
44-
CREATE TABLE IF NOT EXISTS config (
45-
key TEXT PRIMARY KEY,
46-
value JSONB NOT NULL,
47-
updated_at TIMESTAMPTZ DEFAULT NOW()
48-
)
49-
`);
34+
pool = new Pool({
35+
connectionString,
36+
max: 5,
37+
idleTimeoutMillis: 30000,
38+
connectionTimeoutMillis: 10000,
39+
// Railway internal connections don't need SSL; others default to verified TLS
40+
ssl: connectionString.includes('railway.internal')
41+
? false
42+
: process.env.DB_SSL_REJECT_UNAUTHORIZED === 'false'
43+
? { rejectUnauthorized: false }
44+
: { rejectUnauthorized: true },
45+
});
5046

51-
info('Database schema initialized');
52-
return pool;
47+
// Prevent unhandled pool errors from crashing the process
48+
pool.on('error', (err) => {
49+
logError('Unexpected database pool error', { error: err.message });
50+
});
51+
52+
try {
53+
// Test connection
54+
const client = await pool.connect();
55+
try {
56+
await client.query('SELECT NOW()');
57+
info('Database connected');
58+
} finally {
59+
client.release();
60+
}
61+
62+
// Create schema
63+
await pool.query(`
64+
CREATE TABLE IF NOT EXISTS config (
65+
key TEXT PRIMARY KEY,
66+
value JSONB NOT NULL,
67+
updated_at TIMESTAMPTZ DEFAULT NOW()
68+
)
69+
`);
70+
71+
info('Database schema initialized');
72+
} catch (err) {
73+
// Clean up the pool so getPool() doesn't return an unusable instance
74+
await pool.end().catch(() => {});
75+
pool = null;
76+
throw err;
77+
}
78+
79+
return pool;
80+
} finally {
81+
initializing = false;
82+
}
5383
}
5484

5585
/**
@@ -69,8 +99,13 @@ export function getPool() {
6999
*/
70100
export async function closeDb() {
71101
if (pool) {
72-
await pool.end();
73-
pool = null;
74-
info('Database pool closed');
102+
try {
103+
await pool.end();
104+
info('Database pool closed');
105+
} catch (err) {
106+
logError('Error closing database pool', { error: err.message });
107+
} finally {
108+
pool = null;
109+
}
75110
}
76111
}

src/index.js

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -249,7 +249,11 @@ async function gracefulShutdown(signal) {
249249

250250
// 3. Close database pool
251251
info('Closing database connection');
252-
await closeDb();
252+
try {
253+
await closeDb();
254+
} catch (err) {
255+
error('Failed to close database pool', { error: err.message });
256+
}
253257

254258
// 4. Destroy Discord client
255259
info('Disconnecting from Discord');

0 commit comments

Comments
 (0)