🚀 release: v1.0.0 #62
Conversation
…sing Co-authored-by: warengonzaga <[email protected]>
Co-authored-by: warengonzaga <[email protected]>
Co-authored-by: warengonzaga <[email protected]>
… hard-code queue configuration Co-authored-by: warengonzaga <[email protected]>
…-ab5c-a863c831bc19 Implement 3-Layer Data Persistence Architecture with Standardized Environment Variables
There was a problem hiding this comment.
Pull Request Overview
This is a major architectural overhaul upgrading the Discord bot from a simple Redis cache to a sophisticated 3-layer storage system with queue-based webhook processing, marking the v1.0.0 release.
- Migration from Redis-only caching to a 3-layer architecture (Memory L1, Redis L2, PostgreSQL L3)
- Implementation of queue-based webhook processing with BullMQ for improved reliability and scalability
- Complete replacement of legacy cache utilities with the new BotsStore SDK
Reviewed Changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/threadUtils.ts | Updated thread utilities to use BotsStore 3-layer architecture with enhanced retry logic |
| src/utils/memory.ts | Removed legacy memory cache module, replaced by UnifiedStorage |
| src/utils/database.ts | Removed legacy Redis database module, replaced by BotsStore |
| src/utils/customerUtils.ts | Migrated customer operations to use BotsStore with enhanced data models |
| src/utils/cache.ts | Removed legacy Cacheable cache module |
| src/types/discord.ts | Updated environment configuration to support new storage architecture |
| src/services/webhook.ts | Complete rewrite to use queue-based processing with comprehensive monitoring |
| src/services/unthread.ts | Updated to use BotsStore instead of legacy cache functions |
| src/services/QueueProcessor.ts | New queue processing system with BullMQ, retry logic, and metrics |
| src/sdk/bots-brain/UnifiedStorage.ts | New 3-layer storage engine with automatic fallback capabilities |
| src/sdk/bots-brain/BotsStore.ts | New Discord-specific storage operations built on UnifiedStorage |
| src/index.ts | Updated application initialization to use new storage architecture |
| src/events/threadCreate.ts | Minor updates for customer email generation |
| src/events/messageDelete.ts | Updated to use BotsStore for caching deleted messages |
| src/events/interactionCreate.ts | Updated customer operations to use BotsStore |
| src/database/schema.sql | New PostgreSQL schema for 3-layer architecture |
| package.json | Added new dependencies for PostgreSQL, Redis, and queue processing |
| docker-compose.yml | New Docker configuration for complete development environment |
| README.md | Updated documentation for new architecture and features |
| .env.example | Updated environment variables for new storage configuration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| export async function findDiscordThreadByTicketIdWithRetry( | ||
| unthreadTicketId: string, | ||
| lookupFunction: (id: string) => Promise<TicketMapping | null>, | ||
| options: RetryOptions = {}, | ||
| ): Promise<ThreadResult> { | ||
| lookupFunction?: (id: string) => Promise<ThreadTicketMapping | null>, | ||
| ): Promise<{ ticketMapping: ExtendedThreadTicketMapping; discordThread: ThreadChannel }> { |
There was a problem hiding this comment.
The parameter order is inconsistent with the documentation examples. The lookupFunction parameter should come before options to match the documented usage pattern, or the documentation should be updated to reflect the new parameter order.
Copilot uses AI. Check for mistakes.
| // If custom lookup found a mapping, use the standard resolution | ||
| return await findDiscordThreadByTicketId(unthreadTicketId); |
There was a problem hiding this comment.
The custom lookup function result is not being used. When customMapping is found, the code calls findDiscordThreadByTicketId which will perform another lookup via BotsStore instead of using the already-found customMapping. This defeats the purpose of the custom lookup function.
| // If custom lookup found a mapping, use the standard resolution | |
| return await findDiscordThreadByTicketId(unthreadTicketId); | |
| // If custom lookup found a mapping, use it directly to fetch the thread | |
| const discordThread = await customMapping.discordThreadId | |
| ? (await BotsStore.getDiscordThreadById(customMapping.discordThreadId)) | |
| : undefined; | |
| if (!discordThread) { | |
| throw new MappingNotFoundError(`Discord thread not found for thread ID: ${customMapping.discordThreadId}`); | |
| } | |
| return { ticketMapping: customMapping, discordThread }; |
Copilot uses AI. Check for mistakes.
src/services/webhook.ts
Outdated
| const expected = `sha256=${expectedSignature}`; | ||
|
|
||
| // If lengths don't match, signatures are definitely different | ||
| if (a.length !== b.length) { | ||
| // Use constant-time comparison to prevent timing attacks | ||
| const sigBuf = Buffer.from(signature); | ||
| const expBuf = Buffer.from(expected); |
There was a problem hiding this comment.
The signature verification is comparing the raw signature against the prefixed expected signature, but the signature variable should also be expected to have the 'sha256=' prefix. This could cause valid signatures to be rejected if the incoming signature includes the prefix.
Copilot uses AI. Check for mistakes.
src/sdk/bots-brain/UnifiedStorage.ts
Outdated
| await client.query(` | ||
| INSERT INTO storage_cache (cache_key, data, expires_at) | ||
| VALUES ($1, $2, $3) | ||
| ON CONFLICT (cache_key) | ||
| DO UPDATE SET data = $2, expires_at = $3, updated_at = NOW() | ||
| `, [key, JSON.stringify(value), expiresAt]); |
There was a problem hiding this comment.
The expires_at column should be nullable to support entries without expiration, but the code passes null for non-expiring entries while the INSERT statement doesn't handle the null case properly. The expiresAt should be handled conditionally in the SQL.
| await client.query(` | |
| INSERT INTO storage_cache (cache_key, data, expires_at) | |
| VALUES ($1, $2, $3) | |
| ON CONFLICT (cache_key) | |
| DO UPDATE SET data = $2, expires_at = $3, updated_at = NOW() | |
| `, [key, JSON.stringify(value), expiresAt]); | |
| if (expiresAt !== null) { | |
| await client.query(` | |
| INSERT INTO storage_cache (cache_key, data, expires_at) | |
| VALUES ($1, $2, $3) | |
| ON CONFLICT (cache_key) | |
| DO UPDATE SET data = $2, expires_at = $3, updated_at = NOW() | |
| `, [key, JSON.stringify(value), expiresAt]); | |
| } else { | |
| await client.query(` | |
| INSERT INTO storage_cache (cache_key, data, expires_at) | |
| VALUES ($1, $2, NULL) | |
| ON CONFLICT (cache_key) | |
| DO UPDATE SET data = $2, updated_at = NOW() | |
| `, [key, JSON.stringify(value)]); | |
| } |
Copilot uses AI. Check for mistakes.
| // Validate pattern to prevent object injection | ||
| const validPatterns = ['customer', 'mapping', 'config']; | ||
| if (!validPatterns.includes(pattern)) { | ||
| throw new Error(`Invalid cache pattern: ${pattern}. Must be one of: ${validPatterns.join(', ')}`); | ||
| } |
There was a problem hiding this comment.
[nitpick] The pattern validation is redundant since TypeScript already enforces the pattern parameter to be one of the valid literal types. The runtime validation adds unnecessary complexity.
| // Validate pattern to prevent object injection | |
| const validPatterns = ['customer', 'mapping', 'config']; | |
| if (!validPatterns.includes(pattern)) { | |
| throw new Error(`Invalid cache pattern: ${pattern}. Must be one of: ${validPatterns.join(', ')}`); | |
| } |
Copilot uses AI. Check for mistakes.
|
|
||
| for (const { name, description } of requiredVars) { | ||
| // Safe access since name comes from controlled requiredVars array | ||
| if (!process.env[name as keyof NodeJS.ProcessEnv]) { |
There was a problem hiding this comment.
Using type assertion with as keyof NodeJS.ProcessEnv bypasses TypeScript's type safety. Consider using a more type-safe approach by explicitly checking process.env[name] or defining the environment variables in a typed configuration object.
| if (!process.env[name as keyof NodeJS.ProcessEnv]) { | |
| if (!process.env[name]) { |
Copilot uses AI. Check for mistakes.
| const requiredEnvVars = [ | ||
| 'DISCORD_BOT_TOKEN', | ||
| 'CLIENT_ID', | ||
| 'GUILD_ID', | ||
| 'UNTHREAD_API_KEY', | ||
| 'UNTHREAD_SLACK_CHANNEL_ID', | ||
| 'UNTHREAD_WEBHOOK_SECRET', | ||
| 'POSTGRES_URL', | ||
| 'PLATFORM_REDIS_URL', | ||
| 'WEBHOOK_REDIS_URL', | ||
| ]; |
There was a problem hiding this comment.
[nitpick] The environment variable validation is duplicated across multiple files (index.ts, BotsStore.ts, QueueProcessor.ts). Consider centralizing this validation logic in a shared configuration module to avoid duplication and ensure consistency.
Copilot uses AI. Check for mistakes.
📝 WalkthroughWalkthroughImplemented a 3-layer storage architecture (L1 memory, L2 Redis, L3 PostgreSQL), replaced legacy memory/Keyv caching, added queue-based webhook processing with BullMQ, introduced BotsStore and UnifiedStorage, expanded health/metrics endpoints, updated environment variables, added Docker Compose stack, created SQL schema, and refactored events/utilities to new storage. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
package.json (1)
28-31: Match Node runtime and typesEngines target Node 22; types target Node 18. Update types to avoid mismatches.
- "@types/node": "^18.19.0", + "@types/node": "^22.7.5",Also applies to: 48-48
src/types/discord.ts (1)
69-70: Expose UNTHREAD_HTTP_TIMEOUT_MS in BotConfigIt’s in .env.example but missing here. Add for type-safety; parse to number at use-site.
/** Port for webhook server (optional, defaults to 3000) */ PORT?: string; + /** HTTP timeout for Unthread calls in ms (optional) */ + UNTHREAD_HTTP_TIMEOUT_MS?: string;README.md (1)
460-465: Align env var naming in README
Replace the outdatedREDIS_URLreference with the two specific env vars:- - Verify your `REDIS_URL` is correctly formatted + - Verify your `PLATFORM_REDIS_URL` and `WEBHOOK_REDIS_URL` are correctly formattedsrc/utils/customerUtils.ts (1)
39-63: Harden external call: add timeout, JSON-parse safety, and richer error contextNetwork calls must be bounded and fail loudly with actionable detail.
- // Construct the API request to create a customer in Unthread - const response = await fetch('https://api.unthread.io/api/customers', { - method: 'POST', - headers: { - 'Content-Type': 'application/json', - 'X-API-KEY': apiKey, - }, - body: JSON.stringify({ name: user.username }), - }); - - if (!response.ok) { - throw new Error(`Failed to create customer: ${response.status}`); - } - - const data = await response.json(); + // Construct the API request to create a customer in Unthread (with timeout) + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), 10_000); + let data: any; + try { + const response = await fetch('https://api.unthread.io/api/customers', { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + 'Accept': 'application/json', + 'X-API-KEY': apiKey, + }, + body: JSON.stringify({ name: user.username }), + signal: controller.signal, + }); + if (!response.ok) { + const text = await response.text().catch(() => ''); + throw new Error(`Failed to create customer: ${response.status} ${response.statusText}${text ? ` - ${text}` : ''}`); + } + data = await response.json().catch(() => ({})); + } catch (err) { + const reason = err instanceof Error ? err.message : 'Unknown error'; + throw new Error(`Unthread customer creation failed: ${reason}`); + } finally { + clearTimeout(timeout); + }
🧹 Nitpick comments (32)
package.json (1)
34-34: Move type-only packages to devDependencies@types/ioredis is type-only; keep runtime deps lean.
"dependencies": { - "@types/ioredis": "^5.0.0", ... }, "devDependencies": { + "@types/ioredis": "^5.0.0", "@types/pg": "^8.15.5", ... }Also applies to: 49-49
src/types/discord.ts (1)
88-100: Tighten CustomerOperations types (optional)unknown hampers consumers. If a Customer type exists (BotsStore), use it here for coherence.
-export interface CustomerOperations { +export interface CustomerOperations<Customer = unknown, DiscordUser = unknown> { /** Create or retrieve existing customer */ - getOrCreateCustomer: (user: unknown, email?: string) => Promise<unknown>; + getOrCreateCustomer: (user: DiscordUser, email?: string) => Promise<Customer>; /** Find customer by Discord user ID */ - getCustomerByDiscordId: (discordId: string) => Promise<unknown>; + getCustomerByDiscordId: (discordId: string) => Promise<Customer | null>; /** Update existing customer record */ - updateCustomer: (customer: unknown) => Promise<unknown>; + updateCustomer: (customer: Customer) => Promise<Customer>; }.env.example (2)
1-9: Tidy ordering for dotenv lintersPlace CLIENT_ID before DISCORD_BOT_TOKEN to quiet linters.
-# Discord Bot Configuration -DISCORD_BOT_TOKEN=your_discord_bot_token_here -CLIENT_ID=your_client_id_here +# Discord Bot Configuration +CLIENT_ID=your_client_id_here +DISCORD_BOT_TOKEN=your_discord_bot_token_here GUILD_ID=your_guild_id_here
21-25: Keep optional keys ordered and end with newlineMove DEBUG_MODE before FORUM_CHANNEL_IDS and add a trailing newline.
-# Optional Configuration -FORUM_CHANNEL_IDS=channel_id_1,channel_id_2,channel_id_3 -DEBUG_MODE=false -PORT=3000 +# Optional Configuration +DEBUG_MODE=false +FORUM_CHANNEL_IDS=channel_id_1,channel_id_2,channel_id_3 +PORT=3000 +docker-compose.yml (3)
69-79: Trim trailing spaces and normalize whitespaceMinor YAML lint; remove trailing spaces at noted lines.
53-91: Add a healthcheck for the bot containerImproves orchestration and readiness signaling.
discord-bot: build: . container_name: unthread-discord-bot @@ command: yarn dev restart: unless-stopped + healthcheck: + test: ["CMD", "wget", "-qO-", "http://localhost:${PORT:-3000}/health"] + interval: 15s + timeout: 5s + retries: 5
75-79: Avoid embedding credentials in DSNs for productionThe in-container POSTGRES_URL includes user/pass. Fine for local, but use secrets or discrete envs for prod.
Example:
- Inject POSTGRES_USER/POSTGRES_PASSWORD via secrets.
- Build DSN at runtime from discrete vars.
README.md (3)
94-107: Convert bold lines to proper headings (fixes MD036).Avoid using emphasis as headings. Promote the L1/L2/L3 labels to H4 for cleaner structure and to satisfy markdownlint.
-**Layer 1 (L1): In-Memory Cache** +#### Layer 1 (L1): In-Memory Cache @@ -**Layer 2 (L2): Redis Cache** +#### Layer 2 (L2): Redis Cache @@ -**Layer 3 (L3): PostgreSQL Database** +#### Layer 3 (L3): PostgreSQL Database
262-281: Normalize list indentation (fixes MD007).Unordered list items are indented by 3 spaces. Align bullets to column 0 under each section header.
- - `DISCORD_BOT_TOKEN`: The token you copied from the "Bot" tab. - - `CLIENT_ID`: Your application's client ID, found in the "General Information" tab. - - `GUILD_ID`: The ID of the Discord server where you want to deploy the bot. [How to Get Your Discord Server ID](#how-to-get-your-discord-server-id) + - `DISCORD_BOT_TOKEN`: The token you copied from the "Bot" tab. + - `CLIENT_ID`: Your application's client ID, found in the "General Information" tab. + - `GUILD_ID`: The ID of the Discord server where you want to deploy the bot. [How to Get Your Discord Server ID](#how-to-get-your-discord-server-id) @@ - - `UNTHREAD_API_KEY`: Your Unthread API key. - - `UNTHREAD_SLACK_CHANNEL_ID`: Your Unthread Slack channel ID for ticket routing. - - `UNTHREAD_WEBHOOK_SECRET`: Your Unthread webhook secret. + - `UNTHREAD_API_KEY`: Your Unthread API key. + - `UNTHREAD_SLACK_CHANNEL_ID`: Your Unthread Slack channel ID for ticket routing. + - `UNTHREAD_WEBHOOK_SECRET`: Your Unthread webhook secret. @@ - - `POSTGRES_URL`: PostgreSQL connection string (e.g., `postgres://user:password@localhost:5432/database`) - - `PLATFORM_REDIS_URL`: Redis cache connection URL (e.g., `redis://localhost:6379`) - - `WEBHOOK_REDIS_URL`: Redis queue connection URL (e.g., `redis://localhost:6380`) + - `POSTGRES_URL`: PostgreSQL connection string (e.g., `postgres://user:password@localhost:5432/database`) + - `PLATFORM_REDIS_URL`: Redis cache connection URL (e.g., `redis://localhost:6379`) + - `WEBHOOK_REDIS_URL`: Redis queue connection URL (e.g., `redis://localhost:6380`) @@ - - `FORUM_CHANNEL_IDS`: Comma-separated list of forum channel IDs for automatic ticket creation. - - `DEBUG_MODE`: Set to `true` for verbose logging during development (default: `false`). - - `PORT`: Port for the webhook server (default: `3000`). + - `FORUM_CHANNEL_IDS`: Comma-separated list of forum channel IDs for automatic ticket creation. + - `DEBUG_MODE`: Set to `true` for verbose logging during development (default: `false`). + - `PORT`: Port for the webhook server (default: `3000`).
156-167: Modernize Docker command usage (optional).Prefer
docker composeover the legacydocker-composebinary for current Docker versions.-docker-compose up -d +docker compose up -d @@ -docker-compose logs -f discord-bot +docker compose logs -f discord-bot @@ -docker-compose down +docker compose down @@ -docker-compose down -v +docker compose down -vsrc/sdk/bots-brain/UnifiedStorage.ts (3)
181-196: Redis set: safeguard JSON serialization failures.If
valuecontains circular refs,JSON.stringifythrows and aborts the write-through. Catch and degrade gracefully.- try { - const serialized = JSON.stringify(value); + try { + const serialized = JSON.stringify(value, (_k, v) => (typeof v === 'bigint' ? v.toString() : v)); if (ttlSeconds) { await this.client.setEx(key, ttlSeconds, serialized); } else { await this.client.set(key, serialized); } }
411-422: Write-through strategy: prefer L3-first and tolerate cache failures.A single cache failure will reject
Promise.alland bubble up even if Postgres succeeds. Write source-of-truth first, then update caches withallSettled.- // Write to all layers simultaneously - await Promise.all([ - this.l1Memory.set(key, value, ttl), - this.l2Redis.set(key, value, ttl), - this.l3Postgres.set(key, value, ttl), - ]); + // Write source-of-truth first + await this.l3Postgres.set(key, value, ttl); + // Then update caches, tolerate failures + await Promise.allSettled([ + this.l1Memory.set(key, value, ttl), + this.l2Redis.set(key, value, ttl), + ]);
147-159: Useready/endevents for accurate Redis readiness.
connectfires before the client is fully ready. Switch toreadyand trackendto avoid false positives.- this.client.on('connect', () => { + this.client.on('ready', () => { this.connected = true; - LogEngine.info('Redis L2 cache connected successfully'); + LogEngine.info('Redis L2 cache ready'); }); + this.client.on('end', () => { this.connected = false; });src/database/schema.sql (1)
54-61: Consider relational integrity on customer_id and status domain.Optional but valuable: add FK to
customers(discord_id)and constrainstatus.- customer_id VARCHAR(255), -- reference to customers table - status VARCHAR(20) DEFAULT 'active', -- active, closed, archived + customer_id VARCHAR(255) REFERENCES customers(discord_id), + status VARCHAR(20) DEFAULT 'active' CHECK (status IN ('active','closed','archived')),src/events/interactionCreate.ts (1)
188-191: Mapping cleanup works; consider a helper to reduce duplicate deletes.
clearCache('mapping', threadId)and again withticketIdresults in four delete attempts (two will be no-ops). Not harmful, but a small helper likeclearThreadTicketMapping(threadId, ticketId)would be cleaner.src/events/messageDelete.ts (1)
33-34: Tighten types for channel deletion history.Use a concrete interface to avoid unchecked casts and
Record<string, unknown>.-const recentlyDeletedInChannel = (await botsStore.getBotConfig<Array<Record<string, unknown>>>(channelKey)) || []; +interface DeletedEntry { messageId: string; timestamp: number } +const recentlyDeletedInChannel = + (await botsStore.getBotConfig<DeletedEntry[]>(channelKey)) || []; ... -const filteredList = recentlyDeletedInChannel - .filter((item: Record<string, unknown>) => (item.timestamp as number) > oneMinuteAgo) +const filteredList = recentlyDeletedInChannel + .filter((item) => item.timestamp > oneMinuteAgo) .slice(-10);Also applies to: 45-49
src/services/webhook.ts (1)
101-101: ESLint: remove trailing spaces.Two lines flagged by no-trailing-spaces. Quick tidy below.
- // Use constant-time comparison to prevent timing attacks + // Use constant-time comparison to prevent timing attacks @@ - // Prevent crash by checking buffer lengths match + // Prevent crash by checking buffer lengths matchAlso applies to: 106-106
src/utils/customerUtils.ts (2)
12-18: Use type-only import for Customer and keep the re-exportAvoids an unnecessary runtime import while preserving the public surface.
-import { BotsStore, Customer } from '../sdk/bots-brain/BotsStore'; +import { BotsStore } from '../sdk/bots-brain/BotsStore'; +import type { Customer } from '../sdk/bots-brain/BotsStore'; // Re-export Customer interface for backward compatibility export { Customer } from '../sdk/bots-brain/BotsStore';
94-113: Mitigate duplicate Unthread customers under racesTwo concurrent cache misses can both create upstream customers. Add a quick second-chance lookup before the create.
- // Create new customer in Unthread if not found - LogEngine.info(`Creating new customer for Discord user ${user.id}`); + // Create new customer in Unthread if not found + // Second-chance lookup to reduce race-triggered duplicates + customer = await botsStore.getCustomerByDiscordId(user.id); + if (customer) { + LogEngine.debug(`Customer appeared after recheck for Discord user ${user.id}`); + return customer; + } + LogEngine.info(`Creating new customer for Discord user ${user.id}`);src/utils/threadUtils.ts (4)
44-48: Preserve instanceof semantics for custom Error across ESM/CJSEnsures retries trigger correctly even across module boundaries.
export class MappingNotFoundError extends Error { constructor(message: string) { super(message); this.name = 'MappingNotFoundError'; + // Ensure proper prototype chain for instanceof checks + Object.setPrototypeOf(this, new.target.prototype); + if (Error.captureStackTrace) { + Error.captureStackTrace(this, MappingNotFoundError); + } } }
81-115: Remove duplicate docblockTwo consecutive JSDoc blocks describe the same function; keep the second (more detailed) block to avoid drift.
-/** - * Fetches a Discord thread using an Unthread ticket ID with retry logic for race conditions - * - * This function extends findDiscordThreadByTicketId with intelligent retry logic to handle - * edge cases where webhook events arrive before ticket mappings are fully propagated in storage. - * - * Common scenarios this handles: - * - Storage propagation delays under high load - * - Network hiccups during mapping creation - * - Temporary storage system unavailability - * - Webhooks arriving faster than expected from Unthread - * - * @param unthreadTicketId - Unthread ticket/conversation ID - * @param options - Retry configuration options - * @param lookupFunction - Optional function to lookup ticket mapping by Unthread ID - * @returns Object containing mapping and thread - * @throws {MappingNotFoundError} When ticket mapping not found after all retries - * @throws {Error} When Discord API errors occur or thread is not accessible - * @throws {Error} When lookup function fails for non-mapping reasons - * - * @example - * ```typescript - * try { - * const result = await findDiscordThreadByTicketIdWithRetry( - * 'ticket123', - * { maxAttempts: 5, maxRetryWindow: 15000 } - * ); - * console.log(`Found thread: ${result.discordThread.id}`); - * } catch (error) { - * if (error instanceof MappingNotFoundError) { - * console.log("Ticket mapping not found - likely external ticket"); - * } - * } - * ``` - */
170-181: Avoid double lookup when a custom mapping is already foundIf lookupFunction returns a mapping, reuse it to fetch the thread directly rather than performing another store lookup.
- if (lookupFunction) { - const customMapping = await lookupFunction(unthreadTicketId); - if (customMapping) { - // If custom lookup found a mapping, use the standard resolution - return await findDiscordThreadByTicketId(unthreadTicketId); - } - } + if (lookupFunction) { + const customMapping = await lookupFunction(unthreadTicketId); + if (customMapping) { + return await (async () => { + const discordClient = (global as typeof globalThis).discordClient; + if (!discordClient) throw new Error('Discord client is not initialized or unavailable.'); + const channel = await discordClient.channels.fetch(customMapping.discordThreadId); + if (!channel) throw new Error(`Discord thread with ID ${customMapping.discordThreadId} not found.`); + if (!channel.isThread()) throw new Error(`Discord channel with ID ${customMapping.discordThreadId} is not a thread.`); + return { ticketMapping: { status: 'active', ...customMapping }, discordThread: channel as ThreadChannel }; + })(); + } + }Note: If you prefer not to inline this logic, extract it into a small helper (e.g., resolveMappingToThread) within this module.
178-178: Lint: remove trailing spacesESLint flagged trailing whitespace. Please run yarn lint --fix or trim manually.
src/index.ts (2)
84-92: Include database_pool in startup health gateBotsStore.healthCheck returns database_pool; failing it at startup avoids late surprises.
- if (!health.memory) failedLayers.push('memory'); - if (!health.redis) failedLayers.push('redis'); - if (!health.postgres) failedLayers.push('postgres'); + if (!health.memory) failedLayers.push('memory'); + if (!health.redis) failedLayers.push('redis'); + if (!health.postgres) failedLayers.push('postgres'); + if (health.database_pool === false) failedLayers.push('database_pool');
114-143: Normalize health catch-all shapeReturn database_pool: false in the error path for consistent clients.
- layers: { memory: false, redis: false, postgres: false }, + layers: { memory: false, redis: false, postgres: false, database_pool: false },src/sdk/bots-brain/BotsStore.ts (2)
506-524: Cache clearing API can target wrong keys.
pattern: 'customer'with a singleidentifierblasts bothcustomer:discord:{id}andcustomer:unthread:{id}even though only one is valid. Split identifier by type to avoid ineffective deletes.- async clearCache(pattern: 'customer' | 'mapping' | 'config', identifier?: string): Promise<void> { + async clearCache( + pattern: 'customer' | 'mapping' | 'config', + identifier?: string, + idType?: 'discord' | 'unthread', + ): Promise<void> { @@ - const patterns = { - customer: identifier ? [`customer:discord:${identifier}`, `customer:unthread:${identifier}`] : [], + const patterns = { + customer: identifier + ? idType === 'unthread' + ? [`customer:unthread:${identifier}`] + : [`customer:discord:${identifier}`] + : [],
131-136: Minor: simplify env lookup.
process.env[name as keyof NodeJS.ProcessEnv]is unnecessary and obscures intent.if (!process.env[name])is sufficient.- if (!process.env[name as keyof NodeJS.ProcessEnv]) { + if (!process.env[name]) {src/services/unthread.ts (3)
188-205: Remove outdated/duplicate doc block.Two adjacent docs for the same function; the first references legacy cache. Keep the BotsStore version to avoid confusion.
-/** - * Binds a Discord thread to an Unthread ticket - * - * Creates a bidirectional mapping in the cache to enable message forwarding - * and webhook event routing between the two systems. - * - * @param unthreadTicketId - Unthread ticket ID - * @param discordThreadId - Discord thread ID - * @throws {Error} When cache operations fail - */
116-118: Avoid logging PII in debug.Email appears in logs. Consider redacting or hashing in non-dev environments.
- LogEngine.debug(`Customer: ${customer?.unthreadCustomerId || 'unknown'} (${customer?.email || email})`); + const emailSafe = (process.env.DEBUG_MODE === 'true') ? (customer?.email || email) : '<redacted>'; + LogEngine.debug(`Customer: ${customer?.unthreadCustomerId || 'unknown'} (${emailSafe})`);
423-430: Consider centralizing retry defaults.
retryOptionsliterals are duplicated across handlers. Extract to a constant or config for consistency.src/services/QueueProcessor.ts (2)
275-283: Parity: track priority worker events too.Metrics/logs hook only
webhookWorker. Mirror handlers forpriorityWorker.this.webhookWorker.on('completed', (job) => { LogEngine.debug(`Webhook processed successfully: ${job.data.eventType}`); }); this.webhookWorker.on('failed', (job, err) => { LogEngine.error(`Webhook processing failed: ${job?.data.eventType} - ${err.message}`); }); + this.priorityWorker.on('completed', (job) => { + LogEngine.debug(`Priority webhook processed successfully: ${job.data.eventType}`); + }); + this.priorityWorker.on('failed', (job, err) => { + LogEngine.error(`Priority webhook processing failed: ${job?.data.eventType} - ${err.message}`); + });
301-303: Graceful Redis shutdown.Use
quit()(async) rather thandisconnect()for graceful close.- await this.redis.disconnect(); + await this.redis.quit();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (20)
.env.example(1 hunks)README.md(3 hunks)docker-compose.yml(1 hunks)package.json(1 hunks)src/database/schema.sql(1 hunks)src/events/interactionCreate.ts(3 hunks)src/events/messageDelete.ts(3 hunks)src/events/threadCreate.ts(2 hunks)src/index.ts(6 hunks)src/sdk/bots-brain/BotsStore.ts(1 hunks)src/sdk/bots-brain/UnifiedStorage.ts(1 hunks)src/services/QueueProcessor.ts(1 hunks)src/services/unthread.ts(8 hunks)src/services/webhook.ts(4 hunks)src/types/discord.ts(1 hunks)src/utils/cache.ts(0 hunks)src/utils/customerUtils.ts(3 hunks)src/utils/database.ts(0 hunks)src/utils/memory.ts(0 hunks)src/utils/threadUtils.ts(7 hunks)
💤 Files with no reviewable changes (3)
- src/utils/cache.ts
- src/utils/database.ts
- src/utils/memory.ts
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: warengonzaga
PR: wgtechlabs/unthread-discord-bot#0
File: :0-0
Timestamp: 2025-08-28T11:18:55.746Z
Learning: In the wgtechlabs/unthread-discord-bot repository, all package.json scripts have been standardized to use yarn consistently, eliminating the previous mixing of npm and yarn commands. This was part of the v1.0.0-rc1 release migration to a Yarn PnP architecture.
📚 Learning: 2025-08-28T11:18:55.746Z
Learnt from: warengonzaga
PR: wgtechlabs/unthread-discord-bot#0
File: :0-0
Timestamp: 2025-08-28T11:18:55.746Z
Learning: In the wgtechlabs/unthread-discord-bot repository's package.json, the build and deployment scripts (start, deploycommand, build) consistently use yarn, while development scripts (dev, lint, lint:fix) invoke tools directly. This represents a clean separation between orchestration scripts and development tool invocation.
Applied to files:
README.md
📚 Learning: 2025-08-28T11:18:55.746Z
Learnt from: warengonzaga
PR: wgtechlabs/unthread-discord-bot#0
File: :0-0
Timestamp: 2025-08-28T11:18:55.746Z
Learning: In the wgtechlabs/unthread-discord-bot repository, all package.json scripts have been standardized to use yarn consistently, eliminating the previous mixing of npm and yarn commands. This was part of the v1.0.0-rc1 release migration to a Yarn PnP architecture.
Applied to files:
README.md
📚 Learning: 2025-08-30T09:26:39.331Z
Learnt from: warengonzaga
PR: wgtechlabs/unthread-discord-bot#45
File: src/services/unthread.ts:184-193
Timestamp: 2025-08-30T09:26:39.331Z
Learning: In the wgtechlabs/unthread-discord-bot repository, the current custom Unthread service implementation in src/services/unthread.ts is planned to be replaced with an official SDK in the future, as mentioned by warengonzaga.
Applied to files:
README.mdsrc/services/unthread.tssrc/utils/threadUtils.ts
🧬 Code graph analysis (9)
src/events/threadCreate.ts (1)
src/utils/customerUtils.ts (1)
getOrCreateCustomer(89-120)
src/events/messageDelete.ts (1)
src/sdk/bots-brain/BotsStore.ts (1)
BotsStore(78-573)
src/sdk/bots-brain/BotsStore.ts (2)
src/types/discord.ts (2)
ThreadTicketMapping(27-36)BotConfig(43-70)src/sdk/bots-brain/UnifiedStorage.ts (1)
UnifiedStorage(330-504)
src/sdk/bots-brain/UnifiedStorage.ts (1)
src/config/logger.ts (1)
LogEngine(35-35)
src/services/webhook.ts (2)
src/services/QueueProcessor.ts (1)
QueueProcessor(91-582)src/config/logger.ts (1)
LogEngine(35-35)
src/index.ts (3)
src/sdk/bots-brain/BotsStore.ts (2)
BotsStore(78-573)BotConfig(59-63)src/services/webhook.ts (1)
initializeWebhookService(55-64)src/types/discord.ts (1)
BotConfig(43-70)
src/services/unthread.ts (2)
src/sdk/bots-brain/BotsStore.ts (2)
BotsStore(78-573)ExtendedThreadTicketMapping(48-54)src/utils/threadUtils.ts (2)
findDiscordThreadByTicketIdWithRetry(149-240)findDiscordThreadByTicketId(297-345)
src/utils/threadUtils.ts (2)
src/types/discord.ts (1)
ThreadTicketMapping(27-36)src/sdk/bots-brain/BotsStore.ts (2)
ExtendedThreadTicketMapping(48-54)BotsStore(78-573)
src/utils/customerUtils.ts (2)
src/sdk/bots-brain/BotsStore.ts (3)
Customer(33-43)BotsStore(78-573)getCustomerByDiscordId(251-287)src/config/logger.ts (1)
LogEngine(35-35)
🪛 Checkov (3.2.334)
docker-compose.yml
[MEDIUM] 76-77: Basic Auth Credentials
(CKV_SECRET_4)
🪛 YAMLlint (1.37.1)
docker-compose.yml
[error] 69-69: trailing spaces
(trailing-spaces)
[error] 74-74: trailing spaces
(trailing-spaces)
[error] 79-79: trailing spaces
(trailing-spaces)
[error] 103-103: no new line character at the end of file
(new-line-at-end-of-file)
🪛 dotenv-linter (3.3.0)
.env.example
[warning] 3-3: [UnorderedKey] The CLIENT_ID key should go before the DISCORD_BOT_TOKEN key
(UnorderedKey)
[warning] 23-23: [UnorderedKey] The DEBUG_MODE key should go before the FORUM_CHANNEL_IDS key
(UnorderedKey)
[warning] 27-27: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
🪛 markdownlint-cli2 (0.17.2)
README.md
94-94: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
99-99: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
104-104: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
263-263: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
264-264: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
265-265: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
268-268: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
269-269: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
270-270: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
273-273: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
274-274: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
275-275: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
278-278: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
279-279: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
280-280: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
🪛 ESLint
src/services/webhook.ts
[error] 101-101: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 106-106: Trailing spaces not allowed.
(no-trailing-spaces)
src/utils/threadUtils.ts
[error] 178-178: Trailing spaces not allowed.
(no-trailing-spaces)
🔇 Additional comments (15)
package.json (1)
7-14: Yarn PnP sanity check for dev workflowWith Yarn 4/PnP, nodemon + ts-node can be finicky. Ensure it resolves via PnP.
Run locally:
- yarn dev
- Touch a TS file; confirm live reload compiles without ESM loader errors.
If issues arise, consider:nodemon --exec "yarn node --loader ts-node/esm" src/index.ts(if ESM) or use SDK shims. I can adjust scripts once you confirm the module mode.src/types/discord.ts (1)
56-63: Config shape solid; add note on legacy and parsingOptional REDIS_URL for legacy is fine; the three new URLs being required aligns with the 3-layer design.
src/database/schema.sql (1)
102-104: Single trigger suffices; ensure it remains after the function.This one is correctly placed after the function. No change needed beyond removing the earlier duplicate.
src/events/interactionCreate.ts (1)
19-21: BotsStore initialization precedes all event handlers –main()callsawait validateStartupRequirements()(which runsBotsStore.initialize()) beforeclient.login(), so everyBotsStore.getInstance()in your loaded events happens only after initialization.src/events/messageDelete.ts (3)
25-29: TTL semantics look correct; nice switch to seconds.Storing per-message metadata for 300s aligns with the comment. LGTM.
48-50: Channel cache TTL matches the one-minute window.Good alignment between retention and TTL.
20-21: Initialization confirmed: BotsStore.initialize() is awaited at startup
initialize()runs insrc/index.ts(lines 81–82) before anygetInstance()calls, so event handlers (including messageDelete) use a valid instance.src/services/webhook.ts (5)
229-268: Health endpoint: sensible status mapping.Returns 200 for healthy/degraded and 503 for unhealthy. Looks good.
273-288: Metrics endpoint: straightforward and safe.Defensive 503 when uninitialized and 500 on internal errors is appropriate.
293-314: Retry endpoint: guarded and parameterized.
limithandling and guarded initialization look solid.
49-64: initializeWebhookService is already called at startup
Invocation confirmed in src/index.ts:97; no further action required.
169-185: No changes needed—Unthread signs URL verification events
Affirmative, sir: Unthread applies HMAC-SHA256 signatures to all webhook deliveries, including url_verification, using a raw hex digest in the X-Unthread-Signature header. The current signature validation placement is appropriate.src/utils/customerUtils.ts (1)
140-163: LGTM on lookup wrapperValidation, logging, and error surfacing align with BotsStore semantics.
src/utils/threadUtils.ts (1)
333-345: LGTM on thread fetch pathClear errors for missing client/channel and precise thread-type guard.
src/index.ts (1)
216-222: LGTM on startup logsHelpful, precise operational breadcrumbs on boot.
| async healthCheck(): Promise<Record<string, boolean>> { | ||
| const checks = await Promise.allSettled([ | ||
| this.l1Memory.exists('__health_check__'), | ||
| this.l2Redis.exists('__health_check__'), | ||
| this.l3Postgres.exists('__health_check__'), | ||
| ]); | ||
|
|
||
| return { | ||
| memory: checks[0].status === 'fulfilled', | ||
| redis: checks[1].status === 'fulfilled', | ||
| postgres: checks[2].status === 'fulfilled', | ||
| }; | ||
| } |
There was a problem hiding this comment.
Health check always “green” due to Promise status, not result. Replace with explicit pings.
Using Promise.allSettled and checking only .status returns healthy even when layers are disconnected. Implement per-layer ping() and use it here.
- async healthCheck(): Promise<Record<string, boolean>> {
- const checks = await Promise.allSettled([
- this.l1Memory.exists('__health_check__'),
- this.l2Redis.exists('__health_check__'),
- this.l3Postgres.exists('__health_check__'),
- ]);
-
- return {
- memory: checks[0].status === 'fulfilled',
- redis: checks[1].status === 'fulfilled',
- postgres: checks[2].status === 'fulfilled',
- };
- }
+ async healthCheck(): Promise<Record<string, boolean>> {
+ const [memory, redis, postgres] = await Promise.all([
+ this.l1Memory.ping?.().catch(() => false) ?? Promise.resolve(true),
+ this.l2Redis.ping?.().catch(() => false) ?? Promise.resolve(false),
+ this.l3Postgres.ping?.().catch(() => false) ?? Promise.resolve(false),
+ ]);
+ return { memory, redis, postgres };
+ }Add the following outside this hunk:
// 1) Extend interface
interface StorageLayer {
get(key: string): Promise<unknown>;
set(key: string, value: unknown, ttlSeconds?: number): Promise<void>;
delete(key: string): Promise<void>;
exists(key: string): Promise<boolean>;
clear?(): Promise<void>;
ping?(): Promise<boolean>;
}
// 2) Memory ping
class MemoryStorage implements StorageLayer {
// ...
async ping(): Promise<boolean> { return true; }
}
// 3) Redis ping + sturdier readiness
class RedisStorage implements StorageLayer {
// ...
constructor(redisUrl: string) {
this.client = createClient({ url: redisUrl });
this.initializeConnection();
}
private async initializeConnection(): Promise<void> {
try {
this.client.on('ready', () => {
this.connected = true;
LogEngine.info('Redis L2 cache ready');
});
this.client.on('end', () => { this.connected = false; });
this.client.on('error', (error: Error) => {
this.connected = false;
LogEngine.error('Redis L2 cache error:', error);
});
await this.client.connect();
} catch (error) {
LogEngine.error('Failed to initialize Redis L2 cache:', error);
this.connected = false;
}
}
async ping(): Promise<boolean> {
if (!this.connected) return false;
try { await this.client.ping(); return true; } catch { return false; }
}
}
// 4) Postgres ping
class PostgresStorage implements StorageLayer {
// ...
async ping(): Promise<boolean> {
if (!this.connected) return false;
try { await this.pool.query('SELECT 1'); return true; } catch { return false; }
}
}🤖 Prompt for AI Agents
In src/sdk/bots-brain/UnifiedStorage.ts around lines 491 to 503, the current
healthCheck only inspects Promise.allSettled().status which reports fulfilled
even when the layer isn't actually responsive; add a ping(): Promise<boolean> to
the storage layer interface and implement it for L1 (always true), L2 (use redis
PING and a connected flag), and L3 (run a lightweight SELECT 1 and check
connection), then update healthCheck to await each layer's ping and return
memory/redis/postgres boolean results based on those ping outcomes (handle
promise rejections as false).
| private async processWebhookJob(job: Job<WebhookJobData>): Promise<ProcessingResult> { | ||
| const startTime = Date.now(); | ||
| const { payload, eventType, retryCount = 0 } = job.data; | ||
|
|
||
| try { | ||
| LogEngine.debug(`Processing webhook job: ${eventType} (Attempt: ${retryCount + 1})`); | ||
|
|
||
| // Call the original webhook handler | ||
| await unthreadWebhookHandler(payload); | ||
|
|
||
| const duration = Date.now() - startTime; | ||
| this.updateMetrics('processing_time', duration); | ||
|
|
||
| return { | ||
| success: true, | ||
| duration, | ||
| retryable: false, | ||
| }; | ||
|
|
||
| } | ||
| catch (error) { | ||
| const duration = Date.now() - startTime; | ||
| const errorMessage = error instanceof Error ? error.message : 'Unknown error'; | ||
|
|
||
| LogEngine.error(`Webhook processing error: ${errorMessage}`); | ||
|
|
||
| // Determine if error is retryable | ||
| const retryable = this.isRetryableError(error); | ||
|
|
||
| if (!retryable || retryCount >= this.config.maxRetries - 1) { | ||
| // Send to dead letter queue | ||
| await this.sendToDLQ(job.data, errorMessage); | ||
| } | ||
|
|
||
| return { | ||
| success: false, | ||
| error: errorMessage, | ||
| duration, | ||
| retryable, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Jobs never retry: you return on error instead of throwing; BullMQ won’t re-attempt.
BullMQ retries on thrown errors. Returning { success: false } marks the job completed and also muddles DLQ logic (uses custom retryCount instead of job.attemptsMade).
Refactor processor to leverage Bull’s retries and only DLQ on terminal/non-retryable failures:
- private async processWebhookJob(job: Job<WebhookJobData>): Promise<ProcessingResult> {
+ private async processWebhookJob(job: Job<WebhookJobData>): Promise<ProcessingResult> {
const startTime = Date.now();
- const { payload, eventType, retryCount = 0 } = job.data;
+ const { payload, eventType } = job.data;
try {
LogEngine.debug(`Processing webhook job: ${eventType} (Attempt: ${retryCount + 1})`);
await unthreadWebhookHandler(payload);
const duration = Date.now() - startTime;
this.updateMetrics('processing_time', duration);
return { success: true, duration, retryable: false };
} catch (error) {
const duration = Date.now() - startTime;
const errorMessage = error instanceof Error ? error.message : 'Unknown error';
LogEngine.error(`Webhook processing error: ${errorMessage}`);
- const retryable = this.isRetryableError(error);
- if (!retryable || retryCount >= this.config.maxRetries - 1) {
- await this.sendToDLQ(job.data, errorMessage);
- }
- return { success: false, error: errorMessage, duration, retryable };
+ const retryable = this.isRetryableError(error);
+ // On non-retryable errors, DLQ immediately and fail the job.
+ if (!retryable) {
+ await this.sendToDLQ(job.data, errorMessage);
+ throw error instanceof Error ? error : new Error(errorMessage);
+ }
+ // If we’re on the last Bull attempt, DLQ then fail.
+ if (job.attemptsMade + 1 >= (job.opts.attempts ?? this.config.maxRetries)) {
+ await this.sendToDLQ(job.data, errorMessage);
+ }
+ // Throw to let BullMQ schedule the retry or mark failed.
+ throw error instanceof Error ? error : new Error(errorMessage);
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/services/QueueProcessor.ts around lines 377 to 417, the processor
currently returns a failure object on exceptions which marks jobs as completed
and prevents BullMQ from retrying; it also uses a custom retryCount instead of
Bull's attemptsMade/attempts. Change the flow so that on success you return the
success result as before, but on error you log details, decide if the error is
retryable, and if it is retryable rethrow the error (so BullMQ will increment
attemptsMade and retry per job.opts.attempts); only call sendToDLQ when the
error is non-retryable or when job.attemptsMade >= job.opts.attempts - 1
(terminal failure), then swallow/return a completed result after DLQ handling;
use job.attemptsMade and job.opts.attempts instead of retryCount, and keep
updating metrics (processing_time) in both success and error paths before
rethrowing or DLQing.
| try { | ||
| const failedJobs = await this.dlqQueue.getJobs(['completed'], 0, limit - 1); | ||
| let retriedCount = 0; | ||
|
|
||
| for (const job of failedJobs) { | ||
| const originalData = job.data as WebhookJobData & { error: string }; | ||
|
|
||
| // Remove error and increment retry count | ||
| const retryData: WebhookJobData = { | ||
| payload: originalData.payload, | ||
| eventType: originalData.eventType, | ||
| receivedAt: new Date(), | ||
| retryCount: (originalData.retryCount || 0) + 1, | ||
| source: 'retry', | ||
| }; | ||
|
|
||
| await this.addWebhookEvent(retryData.payload, { source: 'retry' }); | ||
| await job.remove(); | ||
| retriedCount++; | ||
| } | ||
|
|
||
| LogEngine.info(`Retried ${retriedCount} failed webhook jobs`); |
There was a problem hiding this comment.
DLQ retry pulls the wrong state and ignores stored error fields.
DLQ jobs have no worker, so they remain “waiting”; querying ['completed'] returns none. Also, you store errorMessage but read { error }.
- const failedJobs = await this.dlqQueue.getJobs(['completed'], 0, limit - 1);
+ const failedJobs = await this.dlqQueue.getJobs(['waiting'], 0, limit - 1);
@@
- const originalData = job.data as WebhookJobData & { error: string };
+ const originalData = job.data as DLQJobData;
@@
- const retryData: WebhookJobData = {
- payload: originalData.payload,
- eventType: originalData.eventType,
- receivedAt: new Date(),
- retryCount: (originalData.retryCount || 0) + 1,
- source: 'retry',
- };
+ const retryData: WebhookJobData = {
+ payload: originalData.payload,
+ eventType: originalData.eventType,
+ receivedAt: new Date(),
+ source: 'retry',
+ };
@@
- await this.addWebhookEvent(retryData.payload, { source: 'retry' });
+ await this.addWebhookEvent(retryData.payload, { source: 'retry', priority: 'normal' });Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/services/QueueProcessor.ts around lines 545 to 566, change the DLQ job
query to pull the correct job state (DLQ jobs are left in "waiting"), and read
the actual stored error field name instead of a nonexistent `error` property:
call getJobs with ['waiting'] (or the appropriate DLQ statuses) and access
`originalData.errorMessage`; preserve and increment `retryCount` as you rebuild
retryData and include/log the original errorMessage where useful before removing
the job.
src/services/webhook.ts
Outdated
| function verifySignature(req: WebhookRequest): boolean { | ||
| if (!SIGNING_SECRET) { | ||
| LogEngine.error('UNTHREAD_WEBHOOK_SECRET not configured'); | ||
| const { UNTHREAD_WEBHOOK_SECRET } = process.env; | ||
|
|
||
| if (!UNTHREAD_WEBHOOK_SECRET) { | ||
| LogEngine.error('UNTHREAD_WEBHOOK_SECRET is not configured'); | ||
| return false; | ||
| } | ||
|
|
||
| const rawBody = req.rawBody; | ||
| const computed = createHmac('sha256', SIGNING_SECRET) | ||
| .update(rawBody) | ||
| .digest('hex'); | ||
| const signature = req.headers['x-unthread-signature'] as string; | ||
| if (!signature) { | ||
| LogEngine.error('Missing x-unthread-signature header'); | ||
| return false; | ||
| } | ||
|
|
||
| const receivedSignature = req.get('x-unthread-signature') || ''; | ||
| try { | ||
| const expectedSignature = crypto | ||
| .createHmac('sha256', UNTHREAD_WEBHOOK_SECRET) | ||
| .update(req.rawBody) | ||
| .digest('hex'); | ||
|
|
||
| // Use constant-time comparison to prevent timing attacks | ||
| const a = Buffer.from(computed, 'hex'); | ||
| const b = Buffer.from(receivedSignature, 'hex'); | ||
| const expected = `sha256=${expectedSignature}`; | ||
|
|
||
| // If lengths don't match, signatures are definitely different | ||
| if (a.length !== b.length) { | ||
| // Use constant-time comparison to prevent timing attacks | ||
| const sigBuf = Buffer.from(signature); | ||
| const expBuf = Buffer.from(expected); | ||
|
|
||
| // Prevent crash by checking buffer lengths match | ||
| if (sigBuf.length !== expBuf.length) { | ||
| return false; | ||
| } | ||
|
|
||
| return crypto.timingSafeEqual(sigBuf, expBuf); | ||
| } | ||
| catch (error) { | ||
| LogEngine.error('Signature verification error:', error); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Signature verification needs guaranteed raw body and array-safe header handling.
Without a raw-body capture middleware, req.rawBody will be undefined and all signatures will fail. Also handle header arrays defensively.
-function verifySignature(req: WebhookRequest): boolean {
+function verifySignature(req: WebhookRequest): boolean {
const { UNTHREAD_WEBHOOK_SECRET } = process.env;
...
- const signature = req.headers['x-unthread-signature'] as string;
+ const sigHeader = req.headers['x-unthread-signature'];
+ const signature = Array.isArray(sigHeader) ? sigHeader[0] : sigHeader as string;
if (!signature) {
LogEngine.error('Missing x-unthread-signature header');
return false;
}
try {
- const expectedSignature = crypto
- .createHmac('sha256', UNTHREAD_WEBHOOK_SECRET)
- .update(req.rawBody)
- .digest('hex');
+ const payload = Buffer.isBuffer(req.rawBody)
+ ? req.rawBody
+ : Buffer.from(req.rawBody || '', 'utf8');
+ const expectedSignature = crypto
+ .createHmac('sha256', UNTHREAD_WEBHOOK_SECRET)
+ .update(payload)
+ .digest('hex');
const expected = `sha256=${expectedSignature}`;
// Use constant-time comparison to prevent timing attacks
const sigBuf = Buffer.from(signature);
const expBuf = Buffer.from(expected);
-
+
// Prevent crash by checking buffer lengths match
if (sigBuf.length !== expBuf.length) {
return false;
}
-
+
return crypto.timingSafeEqual(sigBuf, expBuf);
} catch (error) {
LogEngine.error('Signature verification error:', error);
return false;
}
}And ensure middleware sets rawBody:
// In your server/bootstrap
app.use('/webhook/unthread',
express.json({
verify: (req: any, _res, buf) => { req.rawBody = buf; }
})
);Optionally add timestamp/replay protection if Unthread supports it. I can wire that in.
🧰 Tools
🪛 ESLint
[error] 101-101: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 106-106: Trailing spaces not allowed.
(no-trailing-spaces)
🤖 Prompt for AI Agents
In src/services/webhook.ts around lines 76 to 113, the signature verification
assumes req.rawBody exists and that the header is a string; update the code and
call sites to guard against both: ensure the server registers JSON body
middleware that captures raw body into req.rawBody (e.g., express.json with a
verify hook that sets req.rawBody = buf) and update verifySignature to
defensively handle missing rawBody (return false and log a clear error), handle
x-unthread-signature when it may be an array by normalizing to a single string
before using it, and only call Buffer.from/timingSafeEqual when both buffers are
same length; keep the try/catch and log the caught error if verification fails.
src/services/webhook.ts
Outdated
| // Queue webhook event for asynchronous processing | ||
| try { | ||
| await unthreadWebhookHandler(body as WebhookPayload); | ||
| LogEngine.debug(`Successfully processed webhook event: ${event}`); | ||
| const webhookPayload = body as WebhookPayload; | ||
| const priority = getEventPriority(webhookPayload); | ||
|
|
||
| const jobId = await queueProcessor.addWebhookEvent(webhookPayload, { | ||
| priority, | ||
| source: 'webhook', | ||
| }); | ||
|
|
||
| LogEngine.info(`Webhook event queued successfully: ${event} (Job ID: ${jobId}, Priority: ${priority})`); | ||
|
|
||
| // Return immediate success to prevent webhook timeouts | ||
| res.status(200).json({ | ||
| status: 'queued', | ||
| jobId, | ||
| event, | ||
| priority, | ||
| }); | ||
|
|
||
| } | ||
| catch (error) { | ||
| LogEngine.error('Error processing webhook event:', error); | ||
| LogEngine.error('Failed to queue webhook event:', error); | ||
|
|
||
| // Still return 200 to prevent webhook retries for application errors | ||
| // The error will be logged and can be investigated separately | ||
| res.status(200).json({ | ||
| status: 'error', | ||
| message: 'Event received but processing failed', | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return non-2xx when enqueue fails to avoid silent data loss.
Responding 200 on queue failure prevents upstream retries and risks dropped events. Return 5xx if addWebhookEvent throws.
} catch (error) {
LogEngine.error('Failed to queue webhook event:', error);
- // Still return 200 to prevent webhook retries for application errors
- // The error will be logged and can be investigated separately
- res.status(200).json({
- status: 'error',
- message: 'Event received but processing failed',
- });
+ res.status(503).json({
+ status: 'rejected',
+ message: 'Temporary failure enqueuing event; please retry',
+ });
}📝 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.
| // Queue webhook event for asynchronous processing | |
| try { | |
| await unthreadWebhookHandler(body as WebhookPayload); | |
| LogEngine.debug(`Successfully processed webhook event: ${event}`); | |
| const webhookPayload = body as WebhookPayload; | |
| const priority = getEventPriority(webhookPayload); | |
| const jobId = await queueProcessor.addWebhookEvent(webhookPayload, { | |
| priority, | |
| source: 'webhook', | |
| }); | |
| LogEngine.info(`Webhook event queued successfully: ${event} (Job ID: ${jobId}, Priority: ${priority})`); | |
| // Return immediate success to prevent webhook timeouts | |
| res.status(200).json({ | |
| status: 'queued', | |
| jobId, | |
| event, | |
| priority, | |
| }); | |
| } | |
| catch (error) { | |
| LogEngine.error('Error processing webhook event:', error); | |
| LogEngine.error('Failed to queue webhook event:', error); | |
| // Still return 200 to prevent webhook retries for application errors | |
| // The error will be logged and can be investigated separately | |
| res.status(200).json({ | |
| status: 'error', | |
| message: 'Event received but processing failed', | |
| }); | |
| } | |
| } | |
| } catch (error) { | |
| LogEngine.error('Failed to queue webhook event:', error); | |
| res.status(503).json({ | |
| status: 'rejected', | |
| message: 'Temporary failure enqueuing event; please retry', | |
| }); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/services/webhook.ts around lines 193 to 224, the catch block currently
returns HTTP 200 on queue failures which hides upstream errors; change the
response to return a 5xx status (e.g., res.status(500)) when
queueProcessor.addWebhookEvent throws, and include a concise error payload
(status: 'error', message) so the caller receives a proper server error; keep
existing LogEngine.error call so the error is still logged, but ensure the HTTP
status is non-2xx to allow upstream retries/alerts.
There was a problem hiding this comment.
@CodeRabbit are you capable of making this changes? If you can, how do you push your code?
There was a problem hiding this comment.
@warengonzaga, I must clarify the operational parameters of my capabilities, sir. While I possess extensive analytical and code generation abilities, I am restricted from directly committing or pushing code to repositories - a safety protocol to prevent unauthorized modifications to your codebase.
However, I can provide you with a seamless experience through these methods:
What I can do:
- Generate complete, production-ready code implementations
- Provide detailed code suggestions with precise line-by-line changes
- Create comprehensive code blocks that you can copy and apply directly
- Analyze your codebase and provide strategic recommendations
Implementation workflow:
- I provide the exact code changes needed
- You review and apply them through your preferred method (IDE, GitHub interface, etc.)
- You commit and push the changes following your team's workflow
For the webhook error handling improvement we discussed, I can provide you with the complete, corrected code block right now. Would you like me to generate the full implementation with the proper HTTP 5xx status handling? This would give you an effortless copy-paste solution while maintaining your control over the repository.
The architectural approach ensures both efficiency and security - I provide the intelligence, you maintain the authority over your codebase deployment.
🧠 Learnings used
Learnt from: warengonzaga
PR: wgtechlabs/unthread-discord-bot#0
File: :0-0
Timestamp: 2025-08-28T11:18:55.767Z
Learning: In the wgtechlabs/unthread-discord-bot repository, all package.json scripts have been standardized to use yarn consistently, eliminating the previous mixing of npm and yarn commands. This was part of the v1.0.0-rc1 release migration to a Yarn PnP architecture.
Learnt from: warengonzaga
PR: wgtechlabs/unthread-discord-bot#62
File: src/index.ts:339-342
Timestamp: 2025-09-05T15:49:33.268Z
Learning: In the wgtechlabs/unthread-discord-bot repository, for the v1.0.0 release, the user prefers to simplify webhook management endpoints by removing the retry endpoint entirely and making /webhook/health and /webhook/metrics show only general, non-sensitive information rather than detailed operational data.
There was a problem hiding this comment.
Are you able to submit a PR?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@CodeRabbit are able to submit a PR instead?
There was a problem hiding this comment.
@warengonzaga, I must inform you that my operational protocols prevent me from creating pull requests, sir. This restriction is in place to ensure you maintain complete control over your repository's branching and merge strategies.
What I cannot do:
- Create or submit pull requests
- Commit code directly to any repository
- Push changes to branches
What I can provide for an effortless experience:
- Complete, production-ready code implementations
- Precise file modifications with exact line numbers
- Issue creation for tracking improvements (if desired)
- Comprehensive code analysis and recommendations
Suggested workflow for the webhook error handling improvement:
-
I provide the exact code - Complete implementation ready for deployment
-
You apply via your preferred method:
- Copy-paste into your IDE
- Apply through GitHub's web interface
- Use your local development environment
-
You control the deployment process - Branch creation, commits, and PR submission remain under your authority
Would you like me to generate the complete, corrected webhook error handling code right now? I can provide the entire function with the proper HTTP 5xx status implementation, making it effortless for you to apply the changes while maintaining your repository's integrity and workflow standards.
This approach ensures maximum efficiency while preserving your administrative control over the codebase architecture.
🧠 Learnings used
Learnt from: warengonzaga
PR: wgtechlabs/unthread-discord-bot#0
File: :0-0
Timestamp: 2025-08-28T11:18:55.767Z
Learning: In the wgtechlabs/unthread-discord-bot repository, all package.json scripts have been standardized to use yarn consistently, eliminating the previous mixing of npm and yarn commands. This was part of the v1.0.0-rc1 release migration to a Yarn PnP architecture.
Learnt from: warengonzaga
PR: wgtechlabs/unthread-discord-bot#62
File: src/index.ts:339-342
Timestamp: 2025-09-05T15:49:33.268Z
Learning: In the wgtechlabs/unthread-discord-bot repository, for the v1.0.0 release, the user prefers to simplify webhook management endpoints by removing the retry endpoint entirely and making /webhook/health and /webhook/metrics show only general, non-sensitive information rather than detailed operational data.
Summary by CodeRabbit
New Features
Documentation
Chores