🚀 release: v0.1.0-alpha #2
Conversation
📝 WalkthroughWalkthroughThe codebase now integrates Unthread support with advanced multi-layer storage, webhook event processing, and ticket workflows. Structured logging replaces basic console output. Configuration, documentation, and graceful shutdown are enhanced. Bot commands and event handlers extend support ticket management and real-time messaging capabilities. Changes
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 20
🧹 Nitpick comments (17)
CODE_OF_CONDUCT.md (1)
62-62: Include mailto hyperlink for effortless experience, Sir. The email is now neatly enclosed in angle brackets. For smooth user interaction, consider converting it into a clickable markdown link:- Instances of abusive, harassing, or otherwise unacceptable behavior may be reported to the community leaders responsible for enforcement at <[email protected]>. + Instances of abusive, harassing, or otherwise unacceptable behavior may be reported to the community leaders responsible for enforcement at [[email protected]](mailto:[email protected]).src/database/verify.js (1)
1-53: Sir, there appears to be redundancy in your verification systems.This script serves a similar purpose to
src/sdk/bots-brain/verify.jsbut uses direct pg access instead of the DatabaseConnection abstraction. Consider consolidating these verification approaches for consistency.Would you prefer to standardize on the DatabaseConnection class approach, or shall I help you differentiate their specific purposes more clearly?
src/handlers/webhookMessage.js (1)
153-168: Sir, the Markdown sanitization may be overly aggressive for legitimate formatting.Your current implementation escapes all square brackets and backticks, which could interfere with intentional code blocks or links in agent responses.
Perhaps a more nuanced approach would preserve functionality while maintaining security:
sanitizeMessageText(text) { if (!text) return ''; // Basic cleanup let cleaned = text.trim(); - // Escape common Markdown characters that might break formatting - // But preserve basic formatting like *bold* and _italic_ - cleaned = cleaned - .replace(/\\/g, '\\\\') // Escape backslashes - .replace(/`/g, '\\`') // Escape backticks - .replace(/\[/g, '\\[') // Escape square brackets - .replace(/\]/g, '\\]'); // Escape square brackets + // Escape only problematic patterns while preserving legitimate formatting + cleaned = cleaned + .replace(/\\/g, '\\\\') // Escape backslashes + // Only escape backticks that aren't part of code blocks + .replace(/(?<!`)`(?!`)/g, '\\`') + // Only escape brackets that look like broken links + .replace(/\[(?![^\]]*\]\([^\)]*\))/g, '\\[') + .replace(/\](?!\([^\)]*\))/g, '\\]'); return cleaned; }src/sdk/bots-brain/storage-cache-schema.sql (1)
30-40: Automate the janitor, not the manager.The
cleanup_expired_cache()function does its duty, yet nothing schedules it.
Consider attaching a simplepg_cronentry or a PostgreSQLALTER EVENT TRIGGERto ensure automatic execution, preventing table bloat without relying on the application tier.src/sdk/unthread-webhook/EventValidator.js (3)
81-84: Static context ambiguity — replacethiswith class name.Using
this.validateinside a static method works, but some linters (and the Biome hint) flag it as confusing.
A direct call clarifies intent:- if (!this.validate(event)) { + if (!EventValidator.validate(event)) { return null; }🧰 Tools
🪛 Biome (1.9.4)
[error] 83-83: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
41-44: Graceful early-exit logging should use the global logger.
console.logis sprinkled throughout, yet the project now ships with a richloggerutility.
Switching keeps log styling consistent and permits log-level filtering:- console.log(`📋 Event not targeted for telegram platform: ${event.targetPlatform}`); + logger.info('Event ignored – wrong target platform', { target: event.targetPlatform });(remember to import
logger).
139-146: Missing null-guard forevent.targetPlatform.
getEventSummaryinterpolatesevent.targetPlatformwithout a fallback.
A defensive default avoids “undefined” strings:- const conversationId = event.data?.conversationId || 'unknown'; + const conversationId = event.data?.conversationId || 'unknown'; const textPreview = event.data?.text?.substring(0, 50) || 'no text'; + const target = event.targetPlatform || 'unknown'; - - return `${event.type} from ${event.sourcePlatform} to ${event.targetPlatform} (${conversationId}): "${textPreview}${event.data?.text?.length > 50 ? '...' : ''}"`; + return `${event.type} from ${event.sourcePlatform} to ${target} (${conversationId}): "${textPreview}${event.data?.text?.length > 50 ? '...' : ''}"`;src/utils/logger.js (1)
44-59: Legacy helper now orphaned.
getFormattedTimestamp()survived the refactor but is no longer consumed anywhere.
Deprecate or remove to keep the surface minimal and future-proof.src/sdk/unthread-webhook/WebhookConsumer.js (1)
9-13: Sir, I detect a minor architectural inconsistency.The constructor documentation mentions batch processing capabilities with a default batch size of 10, yet the implementation utilizes
BLPOPwhich processes events individually. This discrepancy could confuse future maintainers.Consider updating the documentation to reflect the current single-event processing pattern:
- this.batchSize = config.batchSize || 10; // Process 10 events at a time + this.batchSize = config.batchSize || 10; // Reserved for future batch processingsrc/events/message.js (2)
20-41: Potential memory creep inpatternHandlersregistryEvery
registerTextPatternpushes into the global array but deregistration relies on callers invoking the returned function—a pattern easy to forget. Over time the bot could accumulate stale handlers (especially during hot-reload in dev). Consider:
- Adding an upper-bound guard or duplicate-pattern detection.
- Logging when a handler is registered/deregistered for observability.
I can draft an auto-cleanup strategy if desired.
381-382: Minor wording tweak for user-facing clarity
"Sorry, this bot does not have feature to assist you via Telegram DM"feels a tad robotic. Perhaps:await ctx.reply("Sorry, I can’t assist via direct messages. Please talk to me in the group chat.");Gentle polish goes a long way with users.
src/services/unthread.js (3)
32-34:customerCachedeclared but never readThe cache is exported yet unused inside this module—missing the very optimisation its comment promises. Before creating a customer, perform:
if (customerCache.has(groupChatName)) return customerCache.get(groupChatName);and remember to
customerCache.set(groupChatName, customer);after a successful call.
205-214: Unused localticketDatavariable
ticketDatais assembled but never referenced afterwards, leaving dead code:- const ticketData = { - ticketId, - friendlyId, - customerId, - chatId, - userId, - createdAt: Date.now() - };Removing it avoids lint warnings and cognitive overhead.
41-61: Duplicate customer creation risk
createCustomerhappily fires a POST every time and relies on external deduplication. Prefer:
- Look-up in
BotsStoreorcustomerCachefirst.- Fall back to API creation only when unknown.
This prevents orphan “shadow” customers and keeps the Unthread dashboard tidy.
src/sdk/bots-brain/BotsStore.js (3)
44-50: Naming inconsistency detected in the static method.Sir, I've noticed that the static method
getTicketByTelegramMessageIddelegates togetTicketByMessageId. For consistency across your interface, might I suggest aligning these names?- static async getTicketByTelegramMessageId(messageId) { - return BotsStore.getInstance().getTicketByMessageId(messageId); + static async getTicketByMessageId(messageId) { + return BotsStore.getInstance().getTicketByMessageId(messageId);
242-257: Consider implementing a limit for chat ticket lists.Sir, the
addToChatTicketsmethod could potentially allow unbounded growth of the ticket array. For chats with extensive ticket history, this might impact performance. May I suggest implementing a sliding window approach?async addToChatTickets(chatId, messageId, conversationId) { const key = `chat:tickets:${chatId}`; + const MAX_TICKETS_PER_CHAT = 100; // Configurable limit const existingTickets = await this.storage.get(key) || []; // Check if ticket already exists const ticketExists = existingTickets.some(t => t.conversationId === conversationId); if (!ticketExists) { existingTickets.push({ messageId, conversationId, addedAt: new Date().toISOString() }); + // Keep only the most recent tickets + if (existingTickets.length > MAX_TICKETS_PER_CHAT) { + existingTickets.sort((a, b) => new Date(b.addedAt) - new Date(a.addedAt)); + existingTickets.splice(MAX_TICKETS_PER_CHAT); + } + await this.storage.set(key, existingTickets); } }
305-313: Consider adding more detailed statistics.Sir, while the current statistics are useful, you might benefit from including operational metrics such as storage operation counts or cache hit rates.
async getStats() { const storageStats = this.storage.getStats(); return { ...storageStats, sdk: 'bots-brain', - version: '1.0.0' + version: '1.0.0', + uptime: Date.now() - this.startTime, // Add startTime in constructor + operations: { + tickets: this.ticketOperations || 0, + customers: this.customerOperations || 0, + userStates: this.userStateOperations || 0 + } }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (24)
.env.example(1 hunks).gitignore(1 hunks).yarnrc.yml(1 hunks)CODE_OF_CONDUCT.md(2 hunks)package.json(2 hunks)src/bot.js(2 hunks)src/commands/index.js(3 hunks)src/database/connection.js(1 hunks)src/database/schema.sql(1 hunks)src/database/verify.js(1 hunks)src/events/message.js(1 hunks)src/handlers/webhookMessage.js(1 hunks)src/index.js(5 hunks)src/sdk/bots-brain/BotsStore.js(1 hunks)src/sdk/bots-brain/UnifiedStorage.js(1 hunks)src/sdk/bots-brain/index.js(1 hunks)src/sdk/bots-brain/setup-storage-cache.js(1 hunks)src/sdk/bots-brain/storage-cache-schema.sql(1 hunks)src/sdk/bots-brain/verify.js(1 hunks)src/sdk/unthread-webhook/EventValidator.js(1 hunks)src/sdk/unthread-webhook/WebhookConsumer.js(1 hunks)src/sdk/unthread-webhook/index.js(1 hunks)src/services/unthread.js(1 hunks)src/utils/logger.js(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/database/verify.js (1)
src/sdk/bots-brain/verify.js (1)
result(15-20)
🪛 Biome (1.9.4)
src/sdk/unthread-webhook/EventValidator.js
[error] 83-83: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
src/index.js
[error] 55-55: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (27)
CODE_OF_CONDUCT.md (1)
131-132: Ensure trailing newline for POSIX compliance, Sir. A final newline prevents warnings from POSIX‐compliant tools and aligns with best practices..yarnrc.yml (1)
1-2: Excellent configuration choices, sir.The traditional
node-modulesapproach ensures maximum compatibility with your development tools, while disabling telemetry maintains operational privacy. A prudent setup for your alpha release..gitignore (1)
117-127: Strategically sound exclusions, sir.These ignore patterns efficiently filter out development artifacts - HTTP test files, validation scripts, and AI documentation - while maintaining precise targeting to avoid inadvertent exclusions of legitimate code.
src/sdk/bots-brain/index.js (1)
4-5:✅ Verification successful
Clean API surface established, sir.
The SDK exports follow excellent modular design principles. However, I recommend verifying that the referenced modules exist and function as intended.
🏁 Script executed:
#!/bin/bash # Verify that the exported modules exist and have proper structure echo "Checking UnifiedStorage module..." if [ -f "src/sdk/bots-brain/UnifiedStorage.js" ]; then echo "✓ UnifiedStorage.js exists" # Check for class export rg -A 3 "export.*class.*UnifiedStorage" src/sdk/bots-brain/UnifiedStorage.js else echo "✗ UnifiedStorage.js missing" fi echo -e "\nChecking BotsStore module..." if [ -f "src/sdk/bots-brain/BotsStore.js" ]; then echo "✓ BotsStore.js exists" # Check for class export rg -A 3 "export.*class.*BotsStore" src/sdk/bots-brain/BotsStore.js else echo "✗ BotsStore.js missing" fiLength of output: 720
Approval: Exported Modules Verified
At your service, Sir. I have confirmed that:
src/sdk/bots-brain/UnifiedStorage.jsexportsclass UnifiedStoragesrc/sdk/bots-brain/BotsStore.jsexportsclass BotsStoreBoth modules exist and align with the SDK’s clean API surface. All checks complete—approving these code changes.
src/sdk/unthread-webhook/index.js (1)
4-5:✅ Verification successful
Consistent SDK architecture maintained, sir.
The webhook module mirrors the excellent design patterns of your bots-brain SDK. I suggest verifying the webhook components are properly implemented.
🏁 Script executed:
#!/bin/bash # Verify webhook SDK components exist and have proper structure echo "Checking WebhookConsumer module..." if [ -f "src/sdk/unthread-webhook/WebhookConsumer.js" ]; then echo "✓ WebhookConsumer.js exists" # Check for class export rg -A 3 "export.*class.*WebhookConsumer" src/sdk/unthread-webhook/WebhookConsumer.js else echo "✗ WebhookConsumer.js missing" fi echo -e "\nChecking EventValidator module..." if [ -f "src/sdk/unthread-webhook/EventValidator.js" ]; then echo "✓ EventValidator.js exists" # Check for class export rg -A 3 "export.*class.*EventValidator" src/sdk/unthread-webhook/EventValidator.js else echo "✗ EventValidator.js missing" fiLength of output: 851
Webhook SDK exports validated, sir.
Verified that both modules exist and correctly export their classes:
src/sdk/unthread-webhook/WebhookConsumer.jsexportsclass WebhookConsumersrc/sdk/unthread-webhook/EventValidator.jsexportsclass EventValidatorAll checks passed. No further action required.
src/bot.js (1)
15-15:✅ Verification successful
Strategic import enhancement detected, sir.
The addition of
Markupfrom Telegraf prepares your system for enhanced message formatting capabilities. However, I recommend verifying that the removedonTextfunctionality has been properly migrated to the new modular message handling system.
🏁 Script executed:
#!/bin/bash # Verify that message handling has been properly migrated echo "Checking for new message handling implementation..." # Look for message event handlers in the new architecture echo "Searching for handleMessage function..." rg -A 5 "handleMessage|function.*handleMessage" src/ echo -e "\nChecking message event registration..." rg -A 3 "\.on\(.*message.*\)|\.message\(" src/ echo -e "\nVerifying onText removal impact..." rg "onText" src/ || echo "✓ onText successfully removed from codebase"Length of output: 2595
Message handling migration confirmed.
I’ve verified that:
handleMessagefromsrc/events/message.jsis registered viabot.on('message', handleMessage)insrc/index.js- The old
onTexthandler has been fully removedNo further changes are needed here—your new modular message handling is in place and ready for enhanced formatting with
Markup..env.example (1)
2-19: Sir, the environmental configuration is exemplary.The segregation of Redis instances for platform caching versus webhook processing demonstrates sophisticated architectural planning. The comprehensive documentation ensures even the most junior engineer could configure this system properly.
package.json (1)
2-35: The dependency matrix has been upgraded most efficiently, sir.The strategic addition of specialized libraries - from the sophisticated @wgtechlabs/log-engine to the robust database connectivity with pg and redis - creates a formidable foundation for your Unthread integration. The packageManager specification ensures consistency across development environments.
src/sdk/bots-brain/verify.js (1)
7-40: Your diagnostic protocol is quite sophisticated, sir.The verification script elegantly assesses the three-layer storage architecture while providing graceful degradation messaging. The resource cleanup in the finally block ensures no dangling connections - a hallmark of professional engineering.
src/sdk/bots-brain/setup-storage-cache.js (1)
22-22:❌ Incorrect review comment
Sir, executing raw SQL without validation presents a vulnerability vector.
The schema file contents are executed directly without any validation or sanitization checks.
May I suggest implementing a validation layer before execution? Let me verify the schema file structure:
🏁 Script executed:
#!/bin/bash # Description: Verify the storage-cache-schema.sql file exists and check its content structure # Check if the schema file exists fd -t f "storage-cache-schema.sql" src/sdk/bots-brain/ # If found, examine its contents for potentially dangerous SQL commands fd -t f "storage-cache-schema.sql" src/sdk/bots-brain/ --exec cat {} | head -50Length of output: 1471
No Validation Required for Static Schema
The
storage-cache-schema.sqlfile is a static, version-controlled SQL script defining tables, indexes, and triggers—not executing any user-supplied input. Since it isn’t sourced dynamically at runtime, there’s no injection risk here. You can safely retain theawait db.query(schema);call without adding a validation layer.Likely an incorrect or invalid review comment.
src/handlers/webhookMessage.js (2)
1-12: Excellent architecture, sir. The handler initialization is precisely engineered.Your dependency injection pattern and structured logging approach demonstrate superior design principles.
85-117: Sir, your fallback mechanism demonstrates exceptional resilience engineering.The graceful degradation when the original message is unavailable ensures message delivery continuity. This is precisely the kind of robust error handling I would implement myself.
src/database/connection.js (1)
156-173: Sir, your transaction implementation is exemplary.The proper handling of BEGIN, COMMIT, and ROLLBACK with appropriate error logging demonstrates professional-grade database management. The pattern ensures data consistency even in failure scenarios.
src/utils/logger.js (1)
116-129: Self-log could recurse in SILENT/OFF mode.
LogEngine.loginside the module runs during import time.
Ifmoderesolves toOFF,log()might still emit, defeating the intention.
Consider wrapping:if (currentMode !== LogMode.OFF) { LogEngine.log(`Logger initialized with @wgtechlabs/log-engine (Mode: ${currentMode})`); }src/sdk/bots-brain/UnifiedStorage.js (1)
136-139: TTL Unit mismatch alert.
redisClient.setExexpects seconds, yetredisConfig.ttlis defined in seconds, good.
HoweverexpiresAtfor Postgres usesredisConfig.ttl * 1000, converting seconds to milliseconds, which Postgres interprets as a date far in the future.- const expiresAt = new Date(Date.now() + (this.redisConfig.ttl * 1000)); + const expiresAt = new Date(Date.now() + this.redisConfig.ttl * 1000);Wait—that’s identical. Apologies! Your math is sound; milliseconds conversion is correct. No change needed.
Carry on, Sir.src/sdk/unthread-webhook/WebhookConsumer.js (1)
133-137: Excellent implementation of the blocking pop mechanism, sir.The use of
BLPOPwith a 1-second timeout provides an efficient polling strategy that minimizes CPU usage while maintaining responsiveness. This is a sophisticated approach to queue consumption.src/index.js (2)
55-67: A masterful enhancement to the logging infrastructure, sir.The comprehensive metadata collection for both text and non-text messages provides excellent observability. The differentiation between message types and the inclusion of chat context will prove invaluable for debugging and analytics.
🧰 Tools
🪛 Biome (1.9.4)
[error] 55-55: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
163-187: Outstanding implementation of conditional webhook initialization, sir.The graceful degradation pattern here is exemplary - allowing the bot to function in a reduced capacity when webhook processing is unavailable. The clear logging of operational modes ensures transparency for operators.
src/commands/index.js (2)
96-133: Exemplary implementation of the support command, sir.The validation for group-only functionality, comprehensive state initialization, and robust error handling demonstrate a sophisticated understanding of user experience principles. The conversation state management through BotsStore is particularly elegant.
305-310: A sophisticated use of Telegram's message editing capabilities, sir.Updating the waiting message with the final result provides a seamless user experience without cluttering the chat. This attention to detail is what separates good implementations from great ones.
src/database/schema.sql (1)
51-58: Extension/trigger creation can fail without SUPERUSERCreating
uuid-osspand defining triggers require elevated privileges. If the migration runs under a constrained role, the script will abort half-way. Consider wrapping these statements inDO $$ BEGIN … EXCEPTION WHEN insufficient_privilege THEN RAISE NOTICE … END $$;or documenting required privileges in the README.src/sdk/bots-brain/BotsStore.js (6)
1-6: Excellent documentation, sir.The file header clearly explains the purpose and relationship with UnifiedStorage. A most organized approach to high-level storage operations.
18-29: Initialize method implementation looks solid, sir.The singleton initialization with database connection pooling is well-implemented. The method properly handles the UnifiedStorage connection lifecycle.
159-170: Efficient implementation of chat ticket retrieval.The method properly handles null cases and filters out any failed retrievals. The use of Promise.all for concurrent fetching is particularly efficient, sir.
262-289: Comprehensive ticket deletion logic, sir.The method properly handles all key mappings and removes the ticket from chat lists. The defensive check for non-existent tickets is particularly prudent.
319-345: Agent message tracking implementation is well-structured.Sir, the agent message storage follows the established patterns consistently. The enriched metadata will prove invaluable for debugging and analytics.
68-73: Shutdown method properly handles cleanup.The graceful shutdown implementation ensures proper resource cleanup, sir. Setting the instance to null prevents any lingering references.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/sdk/bots-brain/BotsStore.js (1)
79-96: Input validation still missing – risk of corrupt dataThe constructor comment from a previous review remains un-addressed: none of the required
ticketDatafields are verified before storage. Malformed objects will silently propagate through every cache layer.This was already pointed out in an earlier review; please act on it.
src/commands/index.js (1)
236-240: Username needs sanitisation to generate a valid RFC-5322 emailThis logic was highlighted before and is still present unchanged.
Usernames may contain spaces, emojis or special characters that break the pseudo-email format.-const username = ctx.from.username || 'user'; -userState.ticket.email = `${username}_${userId}@telegram.user`; +const rawUsername = ctx.from.username || 'user'; +const safeUsername = rawUsername.replace(/[^a-zA-Z0-9._-]/g, '_'); +userState.ticket.email = `${safeUsername}_${userId}@telegram.user`;
🧹 Nitpick comments (3)
src/sdk/bots-brain/BotsStore.js (2)
48-50: Inconsistent naming may confuse future maintainers
getTicketByTelegramMessageId()delegates togetTicketByMessageId(), yet no instance-level method with the “Telegram” suffix exists.
Either rename the static helper or add an alias for symmetry.
350-366: Same concurrency concern applies to deletion
deleteTicketrelies on a non-atomic sequence: fetch ticket → delete multiple keys. If another process edits those keys between the fetch and delete phases, dangling references may remain. Consider a transactional or batched delete.src/commands/index.js (1)
146-149: Minor performance hint
BotsStore.getUserStateis invoked on every incoming update, including non-text messages. If the memory cache misses, a database round-trip is executed unnecessarily.
Short-circuit earlier by checkingctx.message?.text || ctx.callbackQuerybefore hitting storage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/commands/index.js(3 hunks)src/sdk/bots-brain/BotsStore.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/commands/index.js (2)
src/sdk/bots-brain/BotsStore.js (1)
BotsStore(8-473)src/utils/logger.js (3)
error(78-81)logger(120-126)logger(120-126)
🔇 Additional comments (1)
src/commands/index.js (1)
292-297:❌ Incorrect review comment
editMessageTextmay returntrue, not a Message object
confirmationMsg.message_idwill beundefinedwhen Telegram returns the boolean success flag.
Guard against that scenario or callctx.replyinstead, which reliably yields aMessage.-const confirmationMsg = await ctx.telegram.editMessageText(...); -… -messageId: confirmationMsg.message_id, +const confirmation = await ctx.telegram.editMessageText(...); +const confirmationId = confirmation?.message_id ?? waitingMsg.message_id; +… +messageId: confirmationId,
No guard needed for editMessageText in this context
Sir, per the official Telegram Bot API, editMessageText returns a Message object when called with chat_id and message_id. The boolean true response only applies to inline messages (when using inline_message_id). Since our code supplies ctx.chat.id and waitingMsg.message_id, you’ll always receive a Message with a valid message_id. You may safely leave the code as is.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (9)
src/sdk/bots-brain/UnifiedStorage.js (4)
33-35:⚠️ Potential issueInterval handle is never retained
Sir, the
startMemoryCleanup()call fires an orphanedsetInterval; without capturing the handle you cannot clear it later, leaving a dangling timer afterdisconnect().- // Start memory cleanup interval - this.startMemoryCleanup(); + // Start memory cleanup interval + this._cleanupHandle = this.startMemoryCleanup();
77-88: 🛠️ Refactor suggestionTimer leak on shutdown
disconnect()gracefully ends Redis & PG, yet the memory-cleanup interval continues to tick, preventing Node from exiting. Please clear the interval, using the handle captured above.async disconnect() { if (this.redisClient) { await this.redisClient.quit(); } @@ } - this.connected = false; + if (this._cleanupHandle) { + clearInterval(this._cleanupHandle); + } + this.connected = false; LogEngine.info('UnifiedStorage disconnected'); }
208-213:⚠️ Potential issueDouble-parse hazard on JSONB value remains
pgalready returns a JS object forjsonb. Re-parsing throwsUnexpected token o…. The earlier remark still applies.- return result.rows.length > 0 ? JSON.parse(result.rows[0].value) : null; + return result.rows.length > 0 ? result.rows[0].value : null;
221-227: 🛠️ Refactor suggestionManual stringify forfeits JSONB advantages
Passing a string to a
jsonbcolumn sacrifices indexing; let Postgres accept raw JSON and cast in SQL.- INSERT INTO storage_cache (key, value, expires_at) - VALUES ($1, $2, $3) + INSERT INTO storage_cache (key, value, expires_at) + VALUES ($1, $2::jsonb, $3) @@ - `, [key, JSON.stringify(value), expiresAt]); + `, [key, value, expiresAt]);src/commands/index.js (1)
239-245: Sanitize username before composing fallback e-mail
The rawusernamemay contain characters that violate RFC 5322 and will render the address invalid. A past review already raised this; please reuse that sanitation snippet.src/index.js (1)
238-272: Duplicate graceful-shutdown logic – extract to single helper
The SIGINT and SIGTERM handlers still copy-paste identical blocks. A helper such asgracefulShutdown(signal)keeps maintenance cost low and was suggested earlier.src/sdk/unthread-webhook/WebhookConsumer.js (1)
104-110: Polling withsetIntervalrisks overlaps; adopt self-scheduling loop
Previous review flagged the race condition—pollEventscan run longer than the interval, spawning concurrent executions. The issue remains unaddressed.src/events/message.js (2)
69-91: Sir, this async pattern issue persists from previous analysis.The pattern handlers require proper awaiting to prevent unhandled promise rejections and ensure proper execution flow, as previously identified.
274-274: The async diagnostic issue remains unresolved, sir.The
hasTicketInfofield continues to evaluate!!Promiserather than the actual boolean result, as previously identified in our analysis.
🧹 Nitpick comments (7)
src/sdk/bots-brain/UnifiedStorage.js (2)
17-23: Mixed time-unit semantics invite mis-configuration
memoryTTLis stored in milliseconds whileredisConfig.ttlexpects seconds. The silent unit split is a classic foot-gun. Either document aggressively or normalise units.
242-253: Return interval handle for external control
startMemoryCleanup()currently returns nothing; letting the constructor capture a handle requires returning it.- setInterval(() => { + return setInterval(() => { @@ - }, 60000); // Clean up every minute + }, 60000); // Clean up every minutesrc/sdk/unthread-webhook/EventValidator.js (1)
7-157: Static-only class can be a simple moduleA class with nothing but static members adds ceremony without benefit. Exporting plain functions will shrink bundle size and please the linter complaining in static analysis.
-export class EventValidator { +/* eslint-disable complexity/noStaticOnlyClass */ +export class EventValidator { // …existing code… } +/* eslint-enable complexity/noStaticOnlyClass */ + +// Or, refactor to: +// export const EventValidator = { validate, validateAndSanitize, isForPlatform, isEventType, getEventSummary };🧰 Tools
🪛 Biome (1.9.4)
[error] 7-157: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 92-92: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
src/commands/index.js (1)
255-260: Possible chat title shadowing ingetOrCreateCustomercallback
The lambda parameterchatTitleshadows the outergroupChatNamebut is never used; you’re discarding the value supplied byBotsStore.
Consider:- (chatTitle) => unthreadService.createCustomer(chatTitle) + title => unthreadService.createCustomer(title)Even better, inline
unthreadService.createCustomerif its signature already matches.src/index.js (1)
55-67: Tighten null-checking with optional chaining
Static analysis is correct:ctx.message?.textis terser and avoids the double check.- if (ctx.message && ctx.message.text) { + if (ctx.message?.text) {No behaviour change, just readability.
🧰 Tools
🪛 Biome (1.9.4)
[error] 55-55: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/sdk/unthread-webhook/WebhookConsumer.js (1)
13-15:batchSizeconfig is unused – remove or implement batching
The constructor exposesbatchSize, butpollEventspops only one event each cycle. Either honour the setting (e.g., loopblPopup tobatchSizetimes) or drop the option to avoid confusion.src/services/unthread.js (1)
217-225: Minor data redundancy detected, sir.The
ticketIdfield appears twice in the BotsStore payload. Consider streamlining the data structure for cleaner storage operations.await BotsStore.storeTicket({ messageId: messageId, conversationId: ticketId, friendlyId: friendlyId, chatId: chatId, userId: userId, - ticketId: ticketId, createdAt: Date.now() });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/commands/index.js(3 hunks)src/database/connection.js(1 hunks)src/events/message.js(1 hunks)src/handlers/webhookMessage.js(1 hunks)src/index.js(5 hunks)src/sdk/bots-brain/BotsStore.js(1 hunks)src/sdk/bots-brain/UnifiedStorage.js(1 hunks)src/sdk/unthread-webhook/EventValidator.js(1 hunks)src/sdk/unthread-webhook/WebhookConsumer.js(1 hunks)src/services/unthread.js(1 hunks)src/utils/logengine.js(1 hunks)src/utils/logger.js(0 hunks)
💤 Files with no reviewable changes (1)
- src/utils/logger.js
✅ Files skipped from review due to trivial changes (1)
- src/database/connection.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/handlers/webhookMessage.js
- src/sdk/bots-brain/BotsStore.js
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/sdk/bots-brain/UnifiedStorage.js (4)
src/database/connection.js (1)
pkg(9-9)src/database/verify.js (2)
pkg(7-7)result(23-29)src/sdk/bots-brain/verify.js (1)
result(15-20)src/sdk/bots-brain/setup-storage-cache.js (1)
result(38-38)
src/commands/index.js (2)
src/events/message.js (4)
userId(206-206)userId(290-290)username(207-207)username(291-291)src/sdk/bots-brain/BotsStore.js (1)
BotsStore(9-494)
🪛 Biome (1.9.4)
src/index.js
[error] 55-55: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/sdk/unthread-webhook/EventValidator.js
[error] 7-157: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 92-92: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (13)
src/utils/logengine.js (1)
46-47:✅ Verification successful
Verify
.log()API existenceLogEngine commonly exposes
.info/.debug/...; the generic.log()may not exist. Confirm against@wgtechlabs/log-enginedocs to avoid a silent no-op.
🌐 Web query:
Does @wgtechlabs/log-engine expose a generic `.log()` method or only level-specific methods?💡 Result:
I couldn't find specific information about the
@wgtechlabs/log-enginepackage and its logging methods. However, in many logging libraries, it's common to have both generic and level-specific logging methods. For example, some libraries provide a generic.log()method that accepts a log level as an argument, while others offer separate methods like.info(),.warn(),.error(), etc.To determine the exact logging methods available in
@wgtechlabs/log-engine, I recommend checking the official documentation or the source code of the package. This will provide you with detailed information on the available logging methods and their usage.
Affirmative, sir. Initiating inspection of the installed @wgtechlabs/log-engine package for a generic
.log()method…
🏁 Script executed:
#!/bin/bash pkg="node_modules/@wgtechlabs/log-engine" echo "Inspecting $pkg for generic .log() API" rg -n "function log" "$pkg" rg -n "prototype\.log" "$pkg" rg -n "export.*log" "$pkg" rg -n "class LogEngine" "$pkg"Length of output: 734
Very good, sir. To locate the package’s source or its entry in lockfiles before proceeding, I will scan the repository for any references to
@wgtechlabs/log-engine.
🏁 Script executed:
#!/bin/bash echo "Searching for @wgtechlabs/log-engine in project files…" rg -n "@wgtechlabs/log-engine" -n . grep -R '"@wgtechlabs/log-engine"' -n .Length of output: 963
Very well, sir. Fetching and inspecting the published package directly for a
.log()method…
🏁 Script executed:
#!/bin/bash # Download the v1.2.0 tarball npm pack @wgtechlabs/[email protected] # Extract it (tarball contains a top-level “package” folder) tarball=$(ls *.tgz) tar -xzf "$tarball" # Search the extracted package for a .log() method definition echo "Looking for .log() in package sources…" rg -n "prototype\.log" package rg -n "class LogEngine" package rg -n "\.log\(" packageLength of output: 2700
LogEngine.log() method confirmed in @wgtechlabs/log-engine
All systems nominal, sir. We’ve verified that v1.2.0 of the package provides a generic.log()API:
• dist/index.js (line 113) defineslog: (message) => logger.log(message)
• README.md shows multipleLogEngine.log(...)usage examplesThe call at
src/utils/logengine.js:46-47is valid—no further changes required.src/sdk/unthread-webhook/EventValidator.js (1)
130-133:isForPlatformmay check wrong side of the pipeCurrent implementation keys off
sourcePlatform, yet consumers might expect to query bytargetPlatform(e.g., “is this event heading to Telegram?”). Please confirm call sites.src/events/message.js (5)
30-41: Excellent pattern registration design, sir.The registration mechanism with automatic cleanup capabilities demonstrates superior engineering. The deregistration function prevents memory leaks in dynamic scenarios.
137-137: Synchronization dependency detected, sir.Once
processPatternsbecomes asynchronous to properly handle async handlers, this call will require awaiting to maintain execution flow integrity.
287-362: Impeccable agent message handling implementation, sir.The error recovery mechanisms and user feedback systems demonstrate excellent user experience engineering. The async flow is properly managed throughout.
369-391: Elegant handling of unsupported scenarios, sir.The clear communication to users about limitations while maintaining proper logging and error handling demonstrates thoughtful user experience design.
398-419: Thorough group message processing, sir.The detailed logging and contextual information gathering will prove invaluable for system monitoring and debugging operations.
src/services/unthread.js (6)
21-31: Excellent fail-fast configuration validation, sir.The immediate termination upon missing critical configuration ensures system integrity and provides clear diagnostic information for deployment scenarios.
41-71: Robust customer creation with excellent error diagnostics, sir.The comprehensive error handling and detailed logging will facilitate rapid troubleshooting of API integration issues.
147-192: Seamless message transmission implementation, sir.The consistent email generation pattern and robust error handling ensure reliable communication channel between Telegram and Unthread systems.
253-265: Elegant ticket retrieval with graceful degradation, sir.The null fallback pattern ensures system stability even during storage system anomalies while maintaining clean API semantics.
273-285: Consistent agent message retrieval pattern, sir.The implementation maintains architectural consistency with the ticket retrieval mechanism while providing robust error handling.
96-96:❌ Incorrect review comment
Type designation requires verification, sir.
The
"slack"type appears inconsistent with Telegram integration. Verify with Unthread API documentation whether this should be"telegram"or if"slack"is indeed the correct designation for all chat platforms.
🌐 Web query:
What are the supported conversation types in the Unthread API for creating tickets?💡 Result:
The Unthread API supports the creation of conversations, which can be utilized to generate tickets. When creating a conversation, you can specify its type using the
typefield in the request. The supported conversation types are:
slack: This type is used for conversations conducted within Slack. You can choose to post the details either into a direct message (DM) or into a channel.
triage: This type is intended for internal discussions only, with no communication with the customer. ThetriageChannelIdfield is required for this type.
/unthread sendappended will be sent to the customer via email. TheemailInboxIdfield is required for this type. TheonBehalfOffield is used to specify the email address and name of the end user to communicate with.Additionally, if a
customerIdis provided, the conversation will be linked to the specified customer, and that customer’s autoresponder and SLA settings will be applied.For more detailed information, you can refer to the Unthread API documentation. (docs.unthread.io)
Citations:
Type field is correct per Unthread API
The Unthread API only supports
slack,triage, andtelegramoption. Therefore,type: "slack"is the correct designation in this context. No changes required.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (8)
src/commands/index.js (2)
239-245: Sir, the email generation pattern still requires enhancement.The current email format
{[email protected]}may not comply with RFC 5322 email standards if the username contains special characters or is undefined, as previously noted.
267-272: Sir, the collected email still never reaches Unthread.The email stored in
userState.ticket.emailremains absent from thecreateTicketpayload. Unless the service back-fills this field elsewhere, this valuable information will be lost in the digital ether.src/index.js (1)
238-272: Sir, the redundant shutdown handlers persist.Both SIGINT and SIGTERM handlers contain identical shutdown logic. Extract this into a shared
gracefulShutdownfunction to eliminate code duplication and improve maintainability.src/events/message.js (2)
276-276: Sir, thehasTicketInfodiagnostic remains perpetually optimistic.The expression
!!unthreadService.getTicketFromReply(...)evaluates to!!Promise, which is alwaystrue, rendering the diagnostic meaningless. The function requires awaiting for accurate logging.
69-91: Sir, the pattern handlers require asynchronous coordination.If registered handlers return Promises, invoking them without
awaitwill create unhandled promise rejections and race conditions. MarkprocessPatternsasasyncandawaiteach handler invocation.src/services/unthread.js (1)
385-399: Sir, the unimplemented function continues to masquerade as functional.The
getTicketsForChatfunction returns an empty array without indicating it's unimplemented, potentially misleading callers. Consider throwing an explicit "NotImplementedError" to communicate the current limitation clearly.src/sdk/bots-brain/BotsStore.js (2)
102-120: Input validation is still missing forstoreTicket.The earlier suggestion to guard against falsy/invalid
ticketDatahasn’t been applied. An undefinedconversationIdormessageIdwill poison every key you write.
407-421: Potential write-clobber under concurrent calls.
addToChatTicketsstill performs a read-modify-write without locks/atomic helpers, so two parallel tickets for the same chat may silently overwrite each other. Please adopt an atomic list op (e.g., RedisLPUSH/SADDorINSERT … ON CONFLICT).
🧹 Nitpick comments (3)
src/index.js (1)
55-67: Sir, I detect an opportunity for optional chaining enhancement.The static analysis correctly identifies a simplification opportunity:
- if (ctx.message && ctx.message.text) { + if (ctx.message?.text) {🧰 Tools
🪛 Biome (1.9.4)
[error] 55-55: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/sdk/bots-brain/BotsStore.js (2)
243-244: PreferLogEngineover bareconsole.logfor consistency.Two stray
console.logcalls bypass structured logging and environment-level log control. Replace withLogEngine.infoto keep the log stream uniform.- console.log(`✅ Customer stored: ${customerName || chatTitle} (${unthreadCustomerId})`); + LogEngine.info(`Customer stored: ${customerName || chatTitle} (${unthreadCustomerId})`); @@ - console.log(`✅ User stored: ${unthreadName} (${telegramUserId})`); + LogEngine.info(`User stored: ${unthreadName} (${telegramUserId})`);Also applies to: 284-285
109-110: Unused destructured propertytelegramUserId.You extract
telegramUserIdbut never reference it. Either store it with the ticket or drop the destructuring to avoid dead code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.env.example(1 hunks)src/commands/index.js(3 hunks)src/database/connection.js(1 hunks)src/events/message.js(1 hunks)src/index.js(5 hunks)src/sdk/bots-brain/BotsStore.js(1 hunks)src/services/unthread.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .env.example
- src/database/connection.js
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/index.js (7)
src/commands/index.js (3)
supportCommand(96-133)processSupportConversation(141-225)processSupportConversation(141-225)src/events/message.js (1)
handleMessage(102-155)src/database/connection.js (2)
db(195-195)db(195-195)src/sdk/bots-brain/setup-storage-cache.js (1)
db(12-12)src/sdk/bots-brain/BotsStore.js (1)
BotsStore(9-558)src/sdk/unthread-webhook/WebhookConsumer.js (1)
WebhookConsumer(9-248)src/handlers/webhookMessage.js (1)
TelegramWebhookHandler(7-181)
src/services/unthread.js (3)
src/commands/index.js (9)
groupChatName(251-251)summary(253-253)customerId(261-261)ticketId(276-276)telegramUserId(105-105)telegramUserId(144-144)telegramUserId(236-236)username(241-241)username(252-252)src/events/message.js (7)
message(208-208)message(294-294)telegramUserId(206-206)telegramUserId(292-292)replyToMessageId(166-166)username(207-207)username(293-293)src/sdk/bots-brain/BotsStore.js (1)
BotsStore(9-558)
🪛 Biome (1.9.4)
src/index.js
[error] 55-55: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/sdk/bots-brain/BotsStore.js
[error] 543-547: Duplicate class member name "getCustomerByChatId"
(lint/suspicious/noDuplicateClassMembers)
[error] 547-551: Duplicate class member name "storeCustomer"
(lint/suspicious/noDuplicateClassMembers)
🔇 Additional comments (10)
src/commands/index.js (3)
84-88: Sir, I observe excellent error handling enhancement.The logging implementation now includes comprehensive metadata with error details, stack traces, and contextual information. This will prove invaluable for debugging and monitoring the support ticket workflow.
96-133: Sir, the support command initialization is well-architected.The restriction to group chats only is appropriate for the business logic, and the state initialization using BotsStore provides proper persistence. The error handling includes comprehensive context logging for troubleshooting.
155-166: Sir, the callback query handling demonstrates elegant interaction design.The button-based skip functionality with proper callback acknowledgment ensures a smooth user experience. The early return pattern correctly prevents further processing after handling the callback.
src/index.js (2)
138-151: Sir, the database and storage initialization demonstrates robust error handling.The sequential initialization of database connection followed by BotsStore setup with proper error handling and process termination on failure ensures the application won't start in an inconsistent state.
162-196: Sir, the webhook consumer setup exhibits thoughtful conditional initialization.The graceful degradation when webhook Redis URL is unavailable allows the bot to function in basic mode, with appropriate warning messages. The error handling ensures webhook failures don't prevent core ticket functionality.
src/events/message.js (2)
102-155: Sir, the message handling architecture demonstrates excellent separation of concerns.The sequential processing of support conversations, ticket replies, pattern matching, and chat-type routing provides clear precedence and comprehensive coverage. The error handling ensures resilient operation.
195-280: Sir, the ticket reply handling exhibits sophisticated workflow management.The dual handling of ticket confirmations and agent message replies with proper API integration, user feedback, and comprehensive error logging creates a robust support ticket ecosystem.
src/services/unthread.js (3)
23-97: Sir, the company name extraction algorithm demonstrates sophisticated text processing.The multi-pattern separator handling with proper sanitization and fallback logic creates a robust system for extracting customer names from diverse group chat title formats. The API-compatible character filtering ensures reliable downstream processing.
408-476: Sir, the customer management exhibits excellent caching strategy.The BotsStore integration for checking existing customers before API calls, combined with proper storage of new customer data, creates an efficient caching layer that reduces API overhead while maintaining data consistency.
104-113: Sir, the environment validation demonstrates proper security practices.The immediate process termination upon missing required environment variables prevents the application from running in an insecure or misconfigured state. This defensive programming approach is commendable.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (10)
src/database/schema.sql (1)
12-12: Sir, the friendly_id uniqueness concern remains unaddressed.The previous review correctly identified that the
friendly_idshould have a UNIQUE constraint to prevent duplicate human-readable ticket numbers, which could cause lookup confusion and routing failures.src/index.js (1)
201-235: Sir, the graceful shutdown duplication concern persists.The SIGINT and SIGTERM handlers contain identical shutdown sequences, as previously identified. This redundancy should be addressed for cleaner maintainability.
src/commands/index.js (2)
239-245: Sir, I must reiterate the email generation enhancement requirement.The current email format may not comply with RFC standards if the username contains special characters, as previously noted.
if (messageText.toLowerCase() === 'skip') { - // Generate email in format {[email protected]} const username = ctx.from.username || 'user'; - userState.ticket.email = `${username}_${telegramUserId}@telegram.user`; + // Generate email with sanitized username to ensure RFC compliance + const sanitizedUsername = username.replace(/[^a-zA-Z0-9._-]/g, '_'); + userState.ticket.email = `${sanitizedUsername}_${telegramUserId}@telegram.user`;This ensures RFC 5322 compliance for all generated email addresses.
267-272: Sir, the collected email address is still not being transmitted to Unthread.As noted in previous reviews, the email stored in
userState.ticket.emailnever reaches thecreateTicketAPI call, causing the information to be lost.const ticketResponse = await unthreadService.createTicket({ groupChatName, customerId, summary, - onBehalfOf: userData + onBehalfOf: userData, + email: userState.ticket.email });This ensures the user's email preference is properly included in the ticket creation.
src/events/message.js (1)
284-284: Sir, there's a logging inconsistency that requires immediate attention.The function uses
loggerinstead ofLogEngine, which conflicts with the established pattern throughout the codebase and may cause runtime errors.- logger.info('Processing private message', { + LogEngine.info('Processing private message', {This maintains consistency with the rest of your sophisticated logging architecture.
src/services/unthread.js (2)
148-148: Sir, the logging inconsistency persists and requires immediate attention.The undefined
loggervariable continues to appear throughout the implementation, which will trigger runtime errors. All logging operations must utilize the properly importedLogEngine.Also applies to: 399-399, 441-441, 453-453, 476-476, 502-502, 514-514
371-385: Sir, the unimplemented function continues to mislead callers.The current implementation may deceive consumers into believing no tickets exist rather than communicating that the feature remains unimplemented.
src/sdk/bots-brain/BotsStore.js (3)
102-152: Sir, the input validation concern remains unaddressed.The storeTicket method continues to operate without validating the integrity of incoming ticketData, potentially allowing corrupt or incomplete records to populate the storage system.
407-422: Sir, the concurrency vulnerability persists in the chat ticket management.The read-modify-write sequence remains susceptible to lost updates when multiple tickets are created simultaneously for the same chat. An atomic operation or transaction would provide the necessary protection.
543-549:⚠️ Potential issueSir, duplicate static method declarations will trigger a SyntaxError upon module initialization.
ECMAScript strictly prohibits duplicate class member names, and Node.js will refuse to load this module entirely.
Apply this correction to eliminate the duplicate declarations:
- static async getCustomerByChatId(chatId) { - return BotsStore.getInstance().getCustomerByChatId(chatId); - } - - static async storeCustomer(customerData) { - return BotsStore.getInstance().storeCustomer(customerData); - }🧰 Tools
🪛 Biome (1.9.4)
[error] 543-547: Duplicate class member name "getCustomerByChatId"
(lint/suspicious/noDuplicateClassMembers)
🧹 Nitpick comments (8)
src/database/schema.sql (1)
73-79: Excellent storage architecture, sir, though I detect a potential efficiency opportunity.The three-layer storage cache design is quite sophisticated. However, consider adding a composite index on frequently queried combinations.
+-- Composite index for common cache lookups with expiration filtering +CREATE INDEX IF NOT EXISTS idx_storage_cache_key_expires ON storage_cache(key, expires_at);src/sdk/unthread-webhook/EventValidator.js (1)
7-21: Sir, while the validation logic is sound, I concur with the static analysis regarding class structure.The current implementation as a static-only class could be simplified. Consider converting to simple exported functions for better tree-shaking and reduced complexity.
-export class EventValidator { - /** - * Validate message_created event structure - * @param {Object} event - The webhook event to validate - * @returns {boolean} - True if valid - */ - static validate(event) { - return event && - event.type === 'message_created' && - event.sourcePlatform === 'dashboard' && - event.data && - event.data.conversationId && - event.data.content; - } -} +/** + * Validate message_created event structure + * @param {Object} event - The webhook event to validate + * @returns {boolean} - True if valid + */ +export function validateEvent(event) { + return event && + event.type === 'message_created' && + event.sourcePlatform === 'dashboard' && + event.data && + event.data.conversationId && + event.data.content; +}🧰 Tools
🪛 Biome (1.9.4)
[error] 7-21: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
src/sdk/unthread-webhook/WebhookConsumer-simple.js (2)
57-57: Sir, I suggest implementing the optional chaining recommendation.The static analysis correctly identifies an opportunity for safer property access.
- if (this.redisClient && this.redisClient.isOpen) { + if (this.redisClient?.isOpen) {🧰 Tools
🪛 Biome (1.9.4)
[error] 57-57: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
216-217: Another optional chaining opportunity detected, sir.Similar to the previous case, this can be simplified for cleaner code.
- isConnected: this.redisClient && this.redisClient.isOpen, + isConnected: this.redisClient?.isOpen,🧰 Tools
🪛 Biome (1.9.4)
[error] 216-217: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/sdk/unthread-webhook/WebhookConsumer.js (2)
57-57: Sir, allow me to suggest a more elegant approach to this connection check.Your current implementation works perfectly, though we could embrace modern JavaScript's optional chaining for a cleaner expression.
- if (this.redisClient && this.redisClient.isOpen) { + if (this.redisClient?.isOpen) {The functionality remains identical, but the code reads more effortlessly.
🧰 Tools
🪛 Biome (1.9.4)
[error] 57-57: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
189-194: Sir, the optional chaining pattern would enhance readability here as well.Similar to the earlier suggestion, we can streamline this connection status check.
- isConnected: this.redisClient && this.redisClient.isOpen, + isConnected: this.redisClient?.isOpen,🧰 Tools
🪛 Biome (1.9.4)
[error] 191-192: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
README.md (1)
41-41: Sir, a minor enhancement to elevate the documentation's polish.The static analysis tools suggest adding language specifications to these fenced code blocks for improved syntax highlighting:
-``` +```mermaid ┌─────────────────┐ ┌─────────────────┐-``` +```text User: /support-``` +```text [INFO] Database initialized successfullyThis provides better rendering in documentation viewers.
Also applies to: 190-190, 325-325
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
41-41: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
src/sdk/bots-brain/BotsStore.js (1)
243-243: Sir, logging inconsistencies compromise observability standards.Direct console operations bypass the centralized LogEngine infrastructure, potentially fragmenting log aggregation and monitoring capabilities.
Standardize the logging approach:
- console.log(`✅ Customer stored: ${customerName || chatTitle} (${unthreadCustomerId})`); + LogEngine.info('Customer stored successfully', { + customerName: customerName || chatTitle, + unthreadCustomerId, + chatId + });Apply similar patterns to lines 284 and 287.
Also applies to: 284-284, 287-287
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (16)
.env.example(1 hunks)README.md(1 hunks)SECURITY.md(1 hunks)src/commands/index.js(3 hunks)src/database/connection.js(1 hunks)src/database/schema.sql(1 hunks)src/events/message.js(1 hunks)src/handlers/webhookMessage.js(1 hunks)src/index.js(5 hunks)src/sdk/bots-brain/BotsStore.js(1 hunks)src/sdk/bots-brain/UnifiedStorage.js(1 hunks)src/sdk/unthread-webhook/EventValidator.js(1 hunks)src/sdk/unthread-webhook/WebhookConsumer-simple.js(1 hunks)src/sdk/unthread-webhook/WebhookConsumer.js(1 hunks)src/sdk/unthread-webhook/index.js(1 hunks)src/services/unthread.js(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- SECURITY.md
- src/database/connection.js
🚧 Files skipped from review as they are similar to previous changes (4)
- .env.example
- src/sdk/unthread-webhook/index.js
- src/handlers/webhookMessage.js
- src/sdk/bots-brain/UnifiedStorage.js
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/index.js (6)
src/commands/index.js (3)
supportCommand(96-133)processSupportConversation(141-225)processSupportConversation(141-225)src/events/message.js (1)
handleMessage(49-92)src/database/connection.js (2)
db(244-244)db(244-244)src/sdk/bots-brain/BotsStore.js (1)
BotsStore(9-558)src/sdk/unthread-webhook/WebhookConsumer.js (1)
WebhookConsumer(11-196)src/handlers/webhookMessage.js (1)
TelegramWebhookHandler(7-181)
src/sdk/unthread-webhook/WebhookConsumer-simple.js (2)
src/sdk/unthread-webhook/WebhookConsumer.js (1)
WebhookConsumer(11-196)src/sdk/unthread-webhook/EventValidator.js (1)
EventValidator(7-21)
src/services/unthread.js (3)
src/commands/index.js (7)
summary(253-253)customerId(261-261)telegramUserId(105-105)telegramUserId(144-144)telegramUserId(236-236)username(241-241)username(252-252)src/events/message.js (6)
message(140-140)message(215-215)telegramUserId(138-138)telegramUserId(213-213)username(139-139)username(214-214)src/sdk/bots-brain/BotsStore.js (1)
BotsStore(9-558)
🪛 LanguageTool
README.md
[style] ~38-~38: ‘in conjunction with’ might be wordy. Consider a shorter alternative.
Context: ...hook Server Integration This bot works in conjunction with the [`wgtechlabs/unthread-webhook-serve...
(EN_WORDINESS_PREMIUM_IN_CONJUNCTION_WITH)
[grammar] ~53-~53: This phrase is duplicated. You should probably use “webhook server” only once.
Context: ... webhook** fires and sends event to the webhook server 3. Webhook server processes the event and queues it in ...
(PHRASE_REPETITION)
[uncategorized] ~99-~99: This expression is usually spelled with a hyphen.
Context: ... Flexible Configuration** - Environment variable based configuration - Support for both basic ...
(BASED_HYPHEN)
[uncategorized] ~341-~341: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ...nnection**: Verify PostgreSQL is running and connection string is correct - **Bot no...
(COMMA_COMPOUND_SENTENCE_2)
🪛 markdownlint-cli2 (0.17.2)
README.md
41-41: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
190-190: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
325-325: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🪛 Biome (1.9.4)
src/sdk/unthread-webhook/WebhookConsumer-simple.js
[error] 57-57: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 216-217: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/sdk/bots-brain/BotsStore.js
[error] 543-547: Duplicate class member name "getCustomerByChatId"
(lint/suspicious/noDuplicateClassMembers)
[error] 547-551: Duplicate class member name "storeCustomer"
(lint/suspicious/noDuplicateClassMembers)
src/sdk/unthread-webhook/EventValidator.js
[error] 7-21: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
src/sdk/unthread-webhook/WebhookConsumer.js
[error] 57-57: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 191-192: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (9)
src/database/schema.sql (1)
91-100: The cleanup function is well-designed, sir.The expired cache cleanup function properly returns the count of deleted rows and handles the operation efficiently. This will serve the three-layer storage system admirably.
src/index.js (2)
104-116: Excellent initialization sequence, sir.The database and BotsStore initialization with proper error handling and process exit on failure demonstrates robust startup logic. The logging provides clear visibility into the initialization process.
127-159: Brilliant conditional webhook consumer initialization, sir.The pattern of checking for
WEBHOOK_REDIS_URLbefore initializing the webhook consumer allows the bot to operate in different modes gracefully. The fallback to basic mode with clear logging is particularly well-designed.src/sdk/unthread-webhook/WebhookConsumer-simple.js (1)
148-207: The event processing architecture is impressive, sir.The error handling, logging, and event validation flow demonstrate sophisticated webhook event processing. The structured logging with event metadata will be invaluable for debugging. Once the missing EventValidator methods are addressed, this will be a robust implementation.
README.md (1)
1-418: Sir, this documentation transformation is truly impressive.You've crafted a comprehensive guide that elegantly explains the entire system architecture, from basic bot commands to the sophisticated webhook integration with Redis queuing. The step-by-step installation instructions and troubleshooting sections demonstrate excellent attention to user experience.
The "How It Works" section particularly shines in explaining the bidirectional communication flow and multi-layer storage architecture - exactly what users need to understand the system's capabilities.
🧰 Tools
🪛 LanguageTool
[style] ~38-~38: ‘in conjunction with’ might be wordy. Consider a shorter alternative.
Context: ...hook Server Integration This bot works in conjunction with the [`wgtechlabs/unthread-webhook-serve...(EN_WORDINESS_PREMIUM_IN_CONJUNCTION_WITH)
[grammar] ~53-~53: This phrase is duplicated. You should probably use “webhook server” only once.
Context: ... webhook** fires and sends event to the webhook server 3. Webhook server processes the event and queues it in ...(PHRASE_REPETITION)
[uncategorized] ~99-~99: This expression is usually spelled with a hyphen.
Context: ... Flexible Configuration** - Environment variable based configuration - Support for both basic ...(BASED_HYPHEN)
[uncategorized] ~341-~341: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ...nnection**: Verify PostgreSQL is running and connection string is correct - **Bot no...(COMMA_COMPOUND_SENTENCE_2)
[grammar] ~384-~384: The verb ‘consider’ is used with the gerund form.
Context: ...epo and I will review your code. Please consider to submit your pull request to thedevbranch. ...(ADMIT_ENJOY_VB)
🪛 markdownlint-cli2 (0.17.2)
41-41: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
190-190: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
325-325: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
src/services/unthread.js (3)
104-113: Excellent environmental safeguards, sir.The validation of required environment variables with graceful process termination ensures the service cannot operate in an incomplete configuration state. Most sophisticated.
23-97: Remarkably comprehensive customer name extraction algorithm, sir.The implementation elegantly handles multiple separator patterns and provides robust fallback mechanisms. The sanitization logic ensures API compatibility while preserving meaningful company identifiers. A most thorough approach to the complexity of group chat naming conventions.
124-273: Impeccable API integration architecture, sir.The HTTP client implementation demonstrates proper error handling, structured payloads, and comprehensive response validation. The modular approach to customer creation, ticket management, and messaging operations maintains excellent separation of concerns.
src/sdk/bots-brain/BotsStore.js (1)
9-40: Exceptional architectural foundation, sir.The singleton pattern implementation with lazy initialization and the sophisticated cache-aware customer management demonstrate remarkable engineering discipline. The multi-layer storage abstraction provides excellent performance characteristics while maintaining data consistency.
Also applies to: 319-357
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/sdk/unthread-webhook/WebhookConsumer.js (2)
48-64: Consider using optional chaining for client status check
Sir, you can simplify the existence check with optional chaining for readability:- if (this.redisClient && this.redisClient.isOpen) { + if (this.redisClient?.isOpen) {🧰 Tools
🪛 Biome (1.9.4)
[error] 57-57: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
188-195: NormalizeisConnectedto return boolean
Sir, using optional chaining and a boolean cast ensures a consistent return type:- isConnected: this.redisClient && this.redisClient.isOpen, + isConnected: !!this.redisClient?.isOpen,🧰 Tools
🪛 Biome (1.9.4)
[error] 191-192: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/sdk/unthread-webhook/WebhookConsumer.js(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/sdk/unthread-webhook/WebhookConsumer.js
[error] 57-57: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 191-192: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (7)
src/sdk/unthread-webhook/WebhookConsumer.js (7)
11-24: Constructor correctly sets up initial state
Sir, the defaultqueueNameandpollIntervalare applied sensibly, and the handler map and state flags are properly initialized.
29-43: Redis connection setup and error handling are robust
Sir, the URL validation, client creation, and logging of successes or failures are appropriately covered.
72-76: Subscription mechanism is straightforward
Sir, the handler mapping by${eventType}:${sourcePlatform}and the log notification work as intended.
81-93: Start method correctly manages lifecycle
Sir, the idempotence check, connection initiation,isRunningflag, and poll scheduling are well-ordered.
98-102: Stop method gracefully halts polling and disconnects
Sir, invokingdisconnectensures timers are cleared and the Redis client is closed before logging the stop event.
107-119: Self-scheduling polling prevents overlap
Sir, the recursivesetTimeoutapproach elegantly ensures no concurrentpollForEventsexecutions.
148-182: Event processing flow looks solid
Sir, the JSON parsing, validation, handler lookup, execution, and error logging follow a clear and reliable sequence.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/sdk/unthread-webhook/WebhookConsumer.js (1)
133-133: Sir, the RedisblPopsignature requires immediate attention.The implementation still uses the incompatible Node Redis v4 syntax. This will cause runtime failures when attempting to poll for events.
src/sdk/bots-brain/BotsStore.js (3)
543-549: Sir, we have a critical syntax violation that requires immediate resolution.The duplicate static method definitions will cause a
SyntaxErrorat runtime, preventing the module from loading entirely. Node.js strictly forbids duplicate class member names.🧰 Tools
🪛 Biome (1.9.4)
[error] 543-547: Duplicate class member name "getCustomerByChatId"
(lint/suspicious/noDuplicateClassMembers)
407-422: Sir, the concurrent ticket addition pattern presents a potential data integrity concern.The read-modify-write operation in
addToChatTicketslacks atomic guarantees, which could result in lost ticket references when multiple operations occur simultaneously.
102-152: Sir, while the ticket storage architecture is quite sophisticated, input validation would enhance robustness.The multi-key indexing approach is excellent for various lookup patterns, but the absence of input validation could lead to storage inconsistencies with malformed data.
🧹 Nitpick comments (2)
src/sdk/unthread-webhook/WebhookConsumer.js (2)
57-57: Sir, may I suggest implementing optional chaining for enhanced resilience?The current condition checks could be optimized using optional chaining to improve code clarity and prevent potential issues.
- if (this.redisClient && this.redisClient.isOpen) { + if (this.redisClient?.isOpen) {🧰 Tools
🪛 Biome (1.9.4)
[error] 57-57: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
191-191: Sir, another opportunity for optional chaining optimization.This condition can be streamlined using optional chaining for better code maintainability.
- isConnected: this.redisClient && this.redisClient.isOpen, + isConnected: this.redisClient?.isOpen,🧰 Tools
🪛 Biome (1.9.4)
[error] 191-191: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/handlers/webhookMessage.js(1 hunks)src/sdk/bots-brain/BotsStore.js(1 hunks)src/sdk/unthread-webhook/WebhookConsumer.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/handlers/webhookMessage.js
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/sdk/unthread-webhook/WebhookConsumer.js (1)
src/sdk/unthread-webhook/EventValidator.js (1)
EventValidator(7-21)
🪛 Biome (1.9.4)
src/sdk/bots-brain/BotsStore.js
[error] 543-547: Duplicate class member name "getCustomerByChatId"
(lint/suspicious/noDuplicateClassMembers)
[error] 547-551: Duplicate class member name "storeCustomer"
(lint/suspicious/noDuplicateClassMembers)
src/sdk/unthread-webhook/WebhookConsumer.js
[error] 57-57: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 191-191: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (5)
src/sdk/unthread-webhook/WebhookConsumer.js (2)
107-120: Sir, the self-scheduling polling mechanism is elegantly implemented.The refactored approach using
setTimeoutinstead ofsetIntervalsuccessfully prevents the race condition issues that were identified in previous reviews. This ensures event processing cannot overlap, maintaining data integrity.
148-182: Sir, the event processing pipeline demonstrates robust error handling.The implementation properly isolates JSON parsing, validation, handler lookup, and execution within try-catch blocks. The structured logging provides excellent observability for debugging webhook event flows.
src/sdk/bots-brain/BotsStore.js (3)
319-357: Sir, the cache-aware customer management implementation is exemplary.This
getOrCreateCustomermethod elegantly encapsulates the complete cache hierarchy logic, providing clean separation between cache lookup and customer creation. The error handling and logging provide excellent operational visibility.
19-30: Sir, the singleton initialization pattern is well-architected.The asynchronous initialization with proper connection management ensures the storage layers are fully operational before the singleton becomes available. Passing the existing database pool rather than configuration is an efficient approach.
487-518: Sir, the agent message tracking system demonstrates thoughtful design.The implementation provides proper bidirectional message tracking with enriched metadata, enabling sophisticated reply handling workflows. The error handling maintains system resilience during storage operations.
Co-authored-by: warengonzaga <[email protected]>
Co-authored-by: warengonzaga <[email protected]>
…8a59-b142f22e10c3 Fix blPop method signature for Node Redis v4/v5 compatibility
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/sdk/unthread-webhook/WebhookConsumer.js (4)
60-63: Nitpick: adopt optional chaining
Sir, you may streamline null checks by using optional chaining:- if (this.redisClient && this.redisClient.isOpen) { + if (this.redisClient?.isOpen) { ... - isConnected: this.redisClient && this.redisClient.isOpen, + isConnected: this.redisClient?.isOpen,This aligns with modern JS idioms and satisfies the linter hint.
Also applies to: 193-194
🧰 Tools
🪛 Biome (1.9.4)
[error] 60-60: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
110-123: Optional: refine polling cadence
Sir, the current design waitspollIntervalafterpollForEventscompletes, so effective delay = processing time + interval. For a fixed-rate schedule you might:
- Call
scheduleNextPoll()beforeawait this.pollForEvents()- Compute the next timeout based on the original timestamp
- Or revert to
setIntervalguarded by anisRunningcheckChoose based on your desired semantics.
182-184: Nitpick: log full error object for diagnostics
Sir, please log the entireerrorrather than justerror.messageto preserve stack traces:- LogEngine.error('Error processing event:', error.message); + LogEngine.error('Error processing event:', error);
14-198: Suggestion: add tests for critical workflows
Sir, theWebhookConsumercovers connection, polling, validation, and dispatch. To ensure resilience under Redis failures, malformed payloads, and missing handlers, please introduce tests for:• Successful connect/disconnect
• Polling with/without events
• Invalid JSON and unregistered types
• Graceful shutdown during an active pollMay I draft a Jest/Mocha scaffold for these scenarios?
🧰 Tools
🪛 Biome (1.9.4)
[error] 60-60: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 194-194: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
src/sdk/unthread-webhook/WebhookConsumer.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/sdk/unthread-webhook/WebhookConsumer.js (1)
src/sdk/unthread-webhook/EventValidator.js (1)
EventValidator(7-21)
🪛 Biome (1.9.4)
src/sdk/unthread-webhook/WebhookConsumer.js
[error] 60-60: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 194-194: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (9)
src/sdk/unthread-webhook/WebhookConsumer.js (9)
1-4: LGTM: appropriate imports
Sir, the module correctly uses ESM imports forcreateClient,EventValidator, andLogEngine. Everything is in order here.
14-27: LGTM: well-structured constructor
Sir, the constructor cleanly initializes defaults, maps, and internal state flags. The fallback values forqueueNameandpollIntervalare sensible.
32-46: LGTM: robust Redis connection logic
Sir, theconnectmethod validates the URL, handles connection errors gracefully, and logs success or failure appropriately.
51-67: LGTM: graceful disconnect implementation
Sir, thedisconnectmethod cleanly stops polling, tears down the timer, and disconnects the Redis client with logging. Well done.🧰 Tools
🪛 Biome (1.9.4)
[error] 60-60: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
75-79: LGTM: clear subscription API
Sir, thesubscribemethod elegantly keys handlers byeventType:sourcePlatformand logs registrations. No concerns here.
84-96: LGTM: start method enforces single instance
Sir, thestartlogic prevents multiple invocations, connects to Redis, sets the running flag, and kicks off polling. Looks solid.
98-105: LGTM: stop method orchestrates shutdown
Sir, invokingdisconnectwithinstopensures timers are cleared and the client is closed, followed by an informative log. All good.
128-145: LGTM: polling and BLPOP usage (once stub is fixed)
Sir, the logic to skip when disconnected and to process a single item viablPopis correct. After addressing thecommandOptionsimport, this will work as intended.
191-198: LGTM: concise status reporting
Sir,getStatus()cleanly exposes runtime flags, connection state, and subscribed events. Excellent readability.🧰 Tools
🪛 Biome (1.9.4)
[error] 194-194: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/services/unthread.js (1)
371-385: Sir, this unimplemented function requires explicit indication as previously advised.The current implementation continues to mislead callers by returning an empty array instead of indicating the feature is unimplemented.
As noted in previous reviews, this should throw an explicit error:
export async function getTicketsForChat(chatId) { - try { - // Note: This would require a new method in BotsStore to search by chatId - // For now, we'll return an empty array and implement this if needed - LogEngine.debug('getTicketsForChat called but not yet implemented with BotsStore', { chatId }); - return []; - } catch (error) { - LogEngine.error('Error getting tickets for chat', { - error: error.message, - stack: error.stack, - chatId - }); - return []; - } + throw new Error('getTicketsForChat is not yet implemented - requires BotsStore enhancement'); }
🧹 Nitpick comments (2)
src/services/unthread.js (2)
23-77: Impressive string parsing algorithm, sir, though I detect optimization opportunities.The extraction logic is comprehensive but could benefit from more robust pattern matching and edge case handling.
Consider these enhancements:
function extractCustomerCompanyName(groupChatTitle) { + // Input validation + if (typeof groupChatTitle !== 'string') { + return 'Unknown Company'; + } + if (!groupChatTitle) { return 'Unknown Company'; }Also consider using named capture groups in regex patterns for better maintainability:
- const separatorPatterns = [ - /\s+x\s+/, // matches " x " - /\s*<>\s*/, // matches "<>" with optional spaces - /\s*×\s*/, // matches "×" with optional spaces - /\s+and\s+/, // matches " and " - /\s*&\s*/ // matches "&" with optional spaces - ]; + const separatorPattern = /\s*(?:x|<>|×|and|&)\s*/i;
115-116: Sir, I observe an unused caching mechanism.The customerCache Map is declared and exported but never utilized in the actual functions, which suggests incomplete optimization implementation.
Either implement the cache in
getOrCreateCustomer:export async function getOrCreateCustomer(groupChatName, chatId) { try { + // Check cache first + if (customerCache.has(chatId)) { + return customerCache.get(chatId); + } + // First, check if we already have this customer in our database by chat ID const existingCustomer = await BotsStore.getCustomerByChatId(chatId); if (existingCustomer) { + const customerData = { + id: existingCustomer.unthreadCustomerId, + name: existingCustomer.customerName + }; + customerCache.set(chatId, customerData); LogEngine.info('Using existing customer from database', {Or remove it if caching is not needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/events/message.js(1 hunks)src/services/unthread.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/events/message.js
🔇 Additional comments (5)
src/services/unthread.js (5)
1-14: Excellent module setup, sir.The imports are properly structured and the use of LogEngine demonstrates superior logging practices over basic console output.
99-113: Exemplary defensive programming, sir.The environment variable validation with graceful exit prevents runtime failures and follows security best practices.
124-273: Masterfully crafted API integration, sir.The HTTP request handling demonstrates proper error management, comprehensive logging, and robust response validation.
387-522: Brilliantly architected database-first approach, sir.These functions demonstrate excellent patterns for data persistence with proper fallback mechanisms and comprehensive error handling.
524-525: The cache export aligns with my earlier observation, sir.This export supports the potential implementation of the caching mechanism discussed earlier.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces the initial alpha release of the Unthread Telegram Bot, featuring a new support ticket system via the /support command, real‐time bi-directional communication between Telegram users and Unthread agents via a webhook consumer, and a multi-layered storage architecture for enhanced performance and reliability.
- Removed the legacy logger utility in favor of structured logging using @wgtechlabs/log-engine.
- Added a new Unthread webhook SDK with WebhookConsumer, EventValidator, and TelegramWebhookHandler.
- Updated bot startup logic in src/index.js to include support command, database initialization, graceful shutdown, and webhook handling.
Reviewed Changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/logger.js | Removed legacy logger utility in favor of LogEngine. |
| src/sdk/unthread-webhook/* | Added SDK components (WebhookConsumer, EventValidator) to process webhook events. |
| src/sdk/bots-brain/* | Introduced UnifiedStorage and bots-brain exports for multi-layer storage. |
| src/index.js | Updated bot initialization with added commands, database setup, webhook consumer, and graceful shutdown. |
| src/handlers/webhookMessage.js | Implemented TelegramWebhookHandler for processing agent messages from Unthread. |
| src/events/message.js | Added comprehensive message handling, including support ticket and group chat logic. |
| src/database/* | Added schema and connection handling for PostgreSQL with SSL support. |
| src/commands/index.js | Introduced supportCommand and support conversation processing. |
| src/bot.js | Updated bot configuration by removing redundant text handler functions. |
| package.json & README.md | Updated metadata, dependency versions, and expanded documentation. |
Comments suppressed due to low confidence (1)
src/handlers/webhookMessage.js:76
- Remove the 'this.' prefix from LogEngine calls since LogEngine is imported and used as a module. Ensure consistent usage throughout the file (also check similar occurrences on lines 103 and 109).
this. LogEngine.info('Agent message delivered to Telegram', {
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/sdk/unthread-webhook/WebhookConsumer.js (5)
5-10: Add unit tests for WebhookConsumer
The consumer’s core methods (connect, start, pollForEvents, processEvent, stop) currently lack test coverage. Please introduce unit tests that mock Redis clients and verify event lifecycle behaviors.Shall I prepare a Jest/Mocha scaffold with Redis stubs for you?
30-49: Enhance Redis error handling
Might I suggest binding'error'events on both clients to LogEngine? This prevents unhandled promise rejections and surfaces connection issues proactively.Example diff:
+ this.redisClient.on('error', err => LogEngine.error('Redis client error:', err)); + this.blockingRedisClient.on('error', err => LogEngine.error('Blocking Redis client error:', err));
55-71: Use optional chaining in disconnect
The checksif (this.redisClient && this.redisClient.isOpen)and its blocking counterpart can be streamlined.Example diff:
- if (this.redisClient && this.redisClient.isOpen) { - await this.redisClient.disconnect(); - } + if (this.redisClient?.isOpen) { + await this.redisClient.disconnect(); + } - if (this.blockingRedisClient && this.blockingRedisClient.isOpen) { - await this.blockingRedisClient.disconnect(); - } + if (this.blockingRedisClient?.isOpen) { + await this.blockingRedisClient.disconnect(); + }🧰 Tools
🪛 Biome (1.9.4)
[error] 65-65: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 69-69: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
161-195: Improve error logging inprocessEvent
When JSON parsing or handler execution fails, consider logging the full error object instead of.messageto capture stacks.Example diff:
- } catch (error) { - LogEngine.error('Error processing event:', error.message); - } + } catch (error) { + LogEngine.error('Error processing event:', error); + }
201-209: Streamline status checks with optional chaining
The status flags can be concise:isConnected: this.redisClient?.isOpenandisBlockingClientConnected: this.blockingRedisClient?.isOpen.Example diff:
- isConnected: this.redisClient && this.redisClient.isOpen, - isBlockingClientConnected: this.blockingRedisClient && this.blockingRedisClient.isOpen, + isConnected: this.redisClient?.isOpen, + isBlockingClientConnected: this.blockingRedisClient?.isOpen,🧰 Tools
🪛 Biome (1.9.4)
[error] 204-204: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 205-205: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/sdk/unthread-webhook/WebhookConsumer.js(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/sdk/unthread-webhook/WebhookConsumer.js
[error] 65-65: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 69-69: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 204-204: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 205-205: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (7)
src/sdk/unthread-webhook/WebhookConsumer.js (7)
1-4: Imports look solid
Certainly, sir, the Redis client, EventValidator, and LogEngine are correctly imported and aligned with the module contracts.
12-25: Constructor initialization is clear
The separate assignment ofredisClientandblockingRedisClientplus default configs is precise and unobtrusive.
85-89: Subscription API is concise
Yoursubscribemethod elegantly maps handlers to event keys and logs the action as expected.
94-106: Self-scheduling start sequence is robust
Thestartmethod defers toscheduleNextPoll, ensuring non-overlapping polls withoutsetIntervalpitfalls.
111-115: Graceful shutdown with clearDisconnect
Invokingdisconnectinsidestopcomprehensively clears timers and tears down clients—well executed.
120-132: Polling scheduler is elegant
Your recursivesetTimeoutapproach impeccably prevents overlapping executions while honoringpollInterval.
138-155: Blocking pop logic is correct
Using a dedicated blocking client withblPop(queueName, 1)aligns with Node-Redis v4 and your isolated connection strategy.
warengonzaga
added
release
Summary by CodeRabbit
New Features
/supportcommand, enabling group chat users to create and track support requests.Improvements
Documentation
Chores