fix: address code review findings from PRs #12–#18

[AnExiledDev]

Summary

• Error handling on Discord sends — Wrapped the messageCreate AI-response block in try-catch with a getUserFriendlyMessage() fallback so Discord API failures (rate limits, missing permissions, deleted channels) no longer surface as unhandled rejections
• Safe-by-default error display — Inverted the NODE_ENV check in config.js so error details only leak when explicitly set to development, not whenever production is unset
• Duplicate unhandledRejection handler — Removed the handler from index.js; the canonical one in events.js remains
• Dead shutdown code — Removed registerPendingRequest/removePendingRequest and the unused pending-request wait loop from graceful shutdown
• ChimeIn AbortController cleanup — Null out buf.abortController in the finally block so it doesn't linger until the next resetCounter()
• Welcome activity Map eviction — Added throttled pruneStaleActivity() every 50 calls to recordCommunityActivity() so stale guild data doesn't accumulate indefinitely

Test plan

• All 337 tests pass (npm test)
• New tests for Discord send failure (reply + split paths)
• New test for NODE_ENV-unset error hiding
• New test for welcome activity pruning
• Removed tests for deleted dead code (pending requests, duplicate handler)

[coderabbitai]

📝 Walkthrough

Summary by CodeRabbit

• Bug Fixes

  • Improved AI response error handling with user-friendly fallbacks and graceful failure behavior.
  • Cleared lingering per-channel processing state after evaluations.
  • Simplified shutdown sequence to avoid waiting on in-flight AI requests.

• New Features

  • Automatic eviction/pruning of stale activity tracking data.
  • Environment-aware error messages: detailed in development, generic in production.

• Tests

  • Added/updated tests for error sanitization, response-send failures, and eviction behavior.

Walkthrough

Sanitized config error messages by environment, removed pending-request helpers and unhandledRejection shutdown handling, cleared AbortController references after evaluation, added robust AI-response send error handling with user-friendly replies, and implemented throttled pruning of stale community activity; tests added or updated accordingly.

Changes

Cohort / File(s)	Summary
Config error sanitization src/commands/config.js, tests/commands/config.test.js	Error messages now expose err.message only when NODE_ENV === 'development'; otherwise use a generic "An internal error occurred." Added tests asserting sanitization when NODE_ENV is unset.
Shutdown / exports removed src/index.js, tests/index.test.js	Removed pending-request tracking and exported helpers registerPendingRequest / removePendingRequest, and removed unhandledRejection handling and related graceful-shutdown wait/logging. Corresponding tests that asserted those behaviors were removed.
AbortController cleanup src/modules/chimeIn.js	Ensure buf.abortController is reset to null in accumulate's finally block so no lingering AbortController remains after evaluation.
AI response error handling src/modules/events.js, tests/modules/events.test.js	Wrapped message-create AI flow in try/catch; split responses are sent via channel.send and single responses via message.reply. On send failures, log the error and attempt a user-friendly reply via getUserFriendlyMessage; tests added/mocked for message.reply() and channel.send() rejection scenarios.
Activity eviction / pruning src/modules/welcome.js, tests/modules/welcome.test.js	Added EVICTION_INTERVAL, activityCallCount, and pruneStaleActivity(cutoff) to periodically evict stale per-channel activity timestamps; introduced test-only helpers __getCommunityActivityState(guildId) and __resetCommunityActivityState() and a test that verifies eviction behavior.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and concisely describes the main objective of the PR: addressing code review findings from multiple prior PRs.
Description check	✅ Passed	The description is well-structured and directly related to the changeset, clearly explaining each of the six main improvements with specific implementation details.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🤖 Fix all issues with AI agents

In `@tests/commands/config.test.js`:
- Around line 173-195: The test restores NODE_ENV incorrectly (setting it to the
string "undefined") — update the teardown to restore env safely by checking
originalEnv: if originalEnv === undefined then delete process.env.NODE_ENV else
set process.env.NODE_ENV = originalEnv; additionally move the test logic into a
try/finally around the call to execute (and the mocks) so the environment is
always restored; reference the test block that calls execute, uses getConfig
mock, and uses originalEnv.

In `@tests/modules/welcome.test.js`:
- Around line 139-174: The test currently only ensures no crash when eviction
runs but doesn't assert pruning occurred; update the test around
recordCommunityActivity and the stale channel to verify pruning by using or
adding a public inspector (e.g., getCommunitySnapshot or similar) to read the
stored timestamps for guild 'prune-guild' and channels 'prune-ch' and 'fresh-ch'
after the simulated time jump and repeated calls; specifically, after the
vi.restoreAllMocks() call invoke getCommunitySnapshot() (or add/export such a
helper from the welcome module) and assert that 'prune-ch' has only fresh
timestamps (i.e., no timestamps older than the activityWindowMinutes) or that a
subsequent call to recordCommunityActivity for 'prune-ch' creates a new entry
rather than reusing old timestamps, ensuring deterministic behavior regardless
of module-level activityCallCount.

📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c5b484b and b936877.

📒 Files selected for processing (9)

• src/commands/config.js
• src/index.js
• src/modules/chimeIn.js
• src/modules/events.js
• src/modules/welcome.js
• tests/commands/config.test.js
• tests/index.test.js
• tests/modules/events.test.js
• tests/modules/welcome.test.js

💤 Files with no reviewable changes (1)

• tests/index.test.js

🧰 Additional context used

📓 Path-based instructions (4)

src/**/*.js

📄 CodeRabbit inference engine (AGENTS.md)

src/**/*.js: Use ESM syntax with import/export; never use require()
Always use node: protocol when importing Node.js builtins (e.g., import { readFileSync } from 'node:fs')
Always use semicolons in code
Use single quotes for strings (enforced by Biome)
Use 2-space indentation (enforced by Biome)
Always use Winston logging via import { info, warn, error } from '../logger.js' — NEVER use console.log(), console.warn(), console.error(), or any console.* method in src/ files
Pass structured metadata when logging: info('Message processed', { userId, channelId })
Use custom error classes from src/utils/errors.js for error handling
Always log errors with context before re-throwing
Use getConfig() from src/modules/config.js to read config; use setConfigValue(key, value) to update at runtime
Use splitMessage() utility for messages exceeding Discord's 2000-character limit
No TypeScript — use plain JavaScript with JSDoc comments for documentation

Files:

• src/modules/chimeIn.js
• src/index.js
• src/modules/welcome.js
• src/modules/events.js
• src/commands/config.js

src/modules/*.js

📄 CodeRabbit inference engine (AGENTS.md)

Check config.yourModule.enabled before processing in module handlers

Files:

• src/modules/chimeIn.js
• src/modules/welcome.js
• src/modules/events.js

tests/**/*.{js,test.js}

📄 CodeRabbit inference engine (AGENTS.md)

Use Vitest for testing with pnpm test; test coverage provider is @vitest/coverage-v8

Files:

• tests/modules/welcome.test.js
• tests/modules/events.test.js
• tests/commands/config.test.js

src/commands/*.js

📄 CodeRabbit inference engine (AGENTS.md)

Slash commands must export data (SlashCommandBuilder) and execute(interaction) function

Files:

• src/commands/config.js

🧠 Learnings (4)

📚 Learning: 2026-02-11T17:18:29.603Z

Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-11T17:18:29.603Z
Learning: Graceful shutdown is handled in `src/index.js`; use error classes and logging before throwing errors in other modules

Applied to files:

• src/index.js

📚 Learning: 2026-02-11T17:18:29.603Z

Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-11T17:18:29.603Z
Learning: Applies to src/**/*.js : Use `splitMessage()` utility for messages exceeding Discord's 2000-character limit

Applied to files:

• src/modules/events.js
• tests/modules/events.test.js

📚 Learning: 2026-02-11T17:18:29.603Z

Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-11T17:18:29.603Z
Learning: Module handlers must be registered in `src/modules/events.js` to wire them to Discord events

Applied to files:

• tests/modules/events.test.js

📚 Learning: 2026-02-11T17:18:29.603Z

Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-11T17:18:29.603Z
Learning: Applies to src/**/*.js : Use custom error classes from `src/utils/errors.js` for error handling

Applied to files:

• tests/modules/events.test.js

🧬 Code graph analysis (5)

src/index.js (3)

src/modules/ai.js (1)

• stopConversationCleanup (335-341)

src/logger.js (2)

• info (216-218)
• error (230-232)

src/db.js (1)

• closeDb (149-160)

src/modules/events.js (3)

src/utils/errors.js (2)

• message (46-46)
• getUserFriendlyMessage (114-159)

src/modules/ai.js (4)

• response (406-417)
• generateResponse (379-450)
• config (34-34)
• config (49-49)

src/utils/splitMessage.js (3)

• needsSplitting (59-61)
• chunks (29-29)
• splitMessage (24-51)

src/commands/config.js (1)

src/modules/config.js (1)

• err (30-30)

tests/modules/events.test.js (3)

src/modules/welcome.js (1)

• message (117-123)

src/utils/errors.js (2)

• message (46-46)
• getUserFriendlyMessage (114-159)

src/utils/splitMessage.js (2)

• needsSplitting (59-61)
• splitMessage (24-51)

tests/commands/config.test.js (2)

src/modules/config.js (1)

• getConfig (130-132)

src/commands/config.js (3)

• execute (158-178)
• content (315-315)
• content (350-350)

🔇 Additional comments (7)

src/modules/chimeIn.js (1)

283-286: LGTM — proper cleanup of abortController in the finally block.

This ensures the AbortController reference doesn't linger after evaluation completes normally, complementing the abort-and-null in resetCounter.

src/commands/config.js (1)

258-265: LGTM — consistent environment-aware error messaging across all three handlers.

The inverted check correctly restricts error detail exposure to explicit development mode only, preventing accidental leakage of internal details (e.g., database connection strings) in production or unset environments.

src/modules/welcome.js (1)

75-100: LGTM — throttled eviction prevents unbounded Map growth.

The approach of pruning every 50 calls with the current activity window cutoff is clean and avoids per-call overhead. Deleting from a Map during for...of iteration is safe in JavaScript.

src/modules/events.js (1)

97-133: Solid error handling for Discord send failures.

The try/catch correctly wraps all Discord API calls in the AI response path, and the nested fallback with silent catch on line 130 is appropriate — if the initial send failed due to permissions or a deleted channel, the fallback will also fail gracefully.

src/index.js (1)

203-228: Clean simplification of the shutdown sequence.

The removal of the pending-request wait loop streamlines shutdown without losing essential steps (stop timers → save state → close DB → disconnect → exit). The ordering is correct.

tests/modules/events.test.js (2)

247-281: Good coverage of the new error paths.

Both tests correctly exercise the failure scenarios — reply rejection triggering the getUserFriendlyMessage fallback, and split-send rejection where both send and the fallback reply fail. The assertions match the implementation behavior.

---

34-37: LGTM — mock for getUserFriendlyMessage properly set up.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

[coderabbitai]

Actionable comments posted: 2

🤖 Fix all issues with AI agents

In `@src/modules/welcome.js`:
- Around line 22-43: The two test-only helpers __getCommunityActivityState and
__resetCommunityActivityState are exported unconditionally; gate their exports
so they are only available in non-production environments (e.g., when
process.env.NODE_ENV !== 'production') or behind a clearly named dev-only flag.
Update the module to conditionally export these functions (or export a single
devHelpers object) based on NODE_ENV, and keep normal runtime exports unchanged
so production consumers cannot call __getCommunityActivityState or
__resetCommunityActivityState.

In `@tests/modules/welcome.test.js`:
- Around line 141-188: The test currently calls vi.restoreAllMocks() and
__resetCommunityActivityState() at the end which won't run if an assertion
throws; ensure cleanup always runs by moving those calls into an afterEach hook
(or wrapping the test body in try/finally) so mocks are restored and community
state reset regardless of failures — update the test that uses
recordCommunityActivity, __getCommunityActivityState, vi.spyOn(Date, 'now'),
vi.restoreAllMocks, and __resetCommunityActivityState to use afterEach cleanup
or try/finally.

📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7661eb6 and 68e16cf.

📒 Files selected for processing (2)

• src/modules/welcome.js
• tests/modules/welcome.test.js

🧰 Additional context used

📓 Path-based instructions (3)

src/**/*.js

📄 CodeRabbit inference engine (AGENTS.md)

src/**/*.js: Use ESM syntax with import/export; never use require()
Always use node: protocol when importing Node.js builtins (e.g., import { readFileSync } from 'node:fs')
Always use semicolons in code
Use single quotes for strings (enforced by Biome)
Use 2-space indentation (enforced by Biome)
Always use Winston logging via import { info, warn, error } from '../logger.js' — NEVER use console.log(), console.warn(), console.error(), or any console.* method in src/ files
Pass structured metadata when logging: info('Message processed', { userId, channelId })
Use custom error classes from src/utils/errors.js for error handling
Always log errors with context before re-throwing
Use getConfig() from src/modules/config.js to read config; use setConfigValue(key, value) to update at runtime
Use splitMessage() utility for messages exceeding Discord's 2000-character limit
No TypeScript — use plain JavaScript with JSDoc comments for documentation

Files:

• src/modules/welcome.js

src/modules/*.js

📄 CodeRabbit inference engine (AGENTS.md)

Check config.yourModule.enabled before processing in module handlers

Files:

• src/modules/welcome.js

tests/**/*.{js,test.js}

📄 CodeRabbit inference engine (AGENTS.md)

Use Vitest for testing with pnpm test; test coverage provider is @vitest/coverage-v8

Files:

• tests/modules/welcome.test.js

🧬 Code graph analysis (1)

tests/modules/welcome.test.js (1)

src/modules/welcome.js (3)

• __resetCommunityActivityState (39-43)
• recordCommunityActivity (66-105)
• __getCommunityActivityState (27-34)

🔇 Additional comments (4)

src/modules/welcome.js (3)

11-14: LGTM!

The throttled eviction constants are well-defined. The module-level counter with a constant interval is a clean approach for periodic pruning.

---

98-105: LGTM!

The periodic eviction trigger is clean — counter increment, threshold check, reset, and prune. The cutoff value reuses the already-computed threshold from the current call, which is correct.

---

111-123: LGTM!

The pruning logic is sound. Since timestamps are appended chronologically, checking only the last element against the cutoff is sufficient to determine staleness. Deleting from a Map during for...of iteration is safe per the ES6 specification.

tests/modules/welcome.test.js (1)

12-13: LGTM!

New test helper imports align with the exports added in the source module.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}