fix: resolve button handling crash and logging errors#19
Conversation
WalkthroughMigrates tests from Vitest to Bun, adds/expands Telegram-related test suites, refactors utils to validate and filter buttons with logging, standardizes structured error logging in message processing, adds/verifies exports for TelegramService and default plugin, and bumps package/dependency versions. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant convertToTelegramButtons as Converter
participant Logger
Caller->>Converter: convertToTelegramButtons(buttons)
alt buttons null/undefined/empty
Converter-->>Caller: return []
else
loop for each button
Converter->>Converter: if button == null -> skip & Logger.warn
Converter->>Converter: validate text && url
alt invalid (missing text or url)
Converter->>Logger: warn about invalid button
Converter-->>Converter: skip
else valid
Converter->>Converter: switch kind (url/login/other)
alt kind == url
Converter-->>Caller: add url button
else kind == login
Converter-->>Caller: add login button
else unknown
Converter->>Logger: warn unknown kind (default to url)
Converter-->>Caller: add url button
end
end
end
Converter-->>Caller: return validated button array
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/utils.ts (1)
92-101: Broken link regex — links will never match.The pattern appears corrupted and won’t detect
[text](url). Fix to standard link regex.- converted = converted.replace( - /$begin:math:display$([^$end:math:display$]+)]$begin:math:text$([^)]+)$end:math:text$/g, - (_match, text, url) => { + converted = converted.replace( + /\[([^\]]+)\]\(([^)]+)\)/g, + (_match, text, url) => { // For link text we escape as plain text. const formattedText = escapePlainText(text); const escapedURL = escapeUrl(url); const formatted = `[${formattedText}](${escapedURL})`; return storeReplacement(formatted); } );Please also add/restore unit tests covering link conversion.
package.json (1)
29-39: Test runner mismatch: tests use Bun, scripts run Vitest.Update scripts to run Bun tests using the
bun testcommand (or revert tests back to Vitest). Current setup will fail CI/local testing. Apply:"scripts": { - "build": "tsup && tsc", - "dev": "tsup --watch", - "test": "vitest run", - "test:watch": "vitest", + "build": "tsup && tsc", + "dev": "tsup --watch", + "test": "bun test", + "test:watch": "bun test --watch", "lint": "prettier --write ./src",Optionally remove Vitest from devDependencies if unused elsewhere.
src/messageManager.ts (1)
332-340: Add URL validation before calling sendMedia; empty strings will cause failures.When document processing fails, attachments are created with
url: ''(lines 334 and 345). These empty strings are then passed tosendMedia()at line 416. ThesendMediamethod at line 468 checks if the mediaPath is an HTTP(S) URL; empty strings fail this check and are treated as local file paths. At line 492,fs.existsSync('')returns false, triggering an error that gets re-thrown, causing message sending to fail.Add validation before line 416 to either skip calling
sendMediafor empty URLs or populate the URL field with actual values. Alternatively, add explicit empty-string handling insendMediato log and gracefully skip invalid attachments.
🧹 Nitpick comments (11)
package.json (1)
20-28: Build-time deps under runtime dependencies.typescript and @types/node should be devDependencies to avoid leaking to consumers.
"dependencies": { "@elizaos/core": "^1.0.19", "@telegraf/types": "7.1.0", - "@types/node": "^24.0.10", "strip-literal": "^3.0.0", "telegraf": "4.16.3", "type-detect": "^4.1.0", - "typescript": "^5.8.3" + "typescript": "^5.8.3" }, "devDependencies": { "prettier": "3.5.3", "tsup": "8.4.0", - "vitest": "1.6.1" + "vitest": "1.6.1", + "@types/node": "^24.0.10" },src/environment.ts (1)
31-36: Use shared logger for consistency.Prefer
logger.warnfrom @elizaos/core overconsole.warnto integrate with existing logging.- console.warn(`Telegram configuration validation failed:\n${errorMessages}`); + logger.warn(`Telegram configuration validation failed:\n${errorMessages}`);src/utils.ts (2)
189-195: Minor: newline accounting in splitMessage.Avoid adding 1 for the newline when
currentChunkis empty to prevent needless early splits.- if (currentChunk.length + line.length + 1 <= maxLength) { - currentChunk += (currentChunk ? '\n' : '') + line; + const sep = currentChunk ? 1 : 0; + if (currentChunk.length + line.length + sep <= maxLength) { + currentChunk += (currentChunk ? '\n' : '') + line;
206-235: Button conversion logic LGTM; consider accepting nullable entries in type.Implementation filters invalid entries, defaults unknown kinds, and logs. Optionally widen the input type to avoid casts at call sites.
-export function convertToTelegramButtons(buttons?: Button[] | null): InlineKeyboardButton[] { +export function convertToTelegramButtons( + buttons?: Array<Button | null | undefined> | null +): InlineKeyboardButton[] {__tests__/button-conversion.test.ts (2)
1-1: Unused import.
spyOnis imported but unused; remove to keep tests clean.-import { afterEach, describe, expect, it, mock, spyOn } from 'bun:test'; +import { afterEach, describe, expect, it, mock } from 'bun:test';
24-31: Consolidate duplicate test suites.
__tests__/button-conversion.test.tsduplicates all 248 lines of__tests__/utils.test.tsincluding identical describe blocks and test implementations forsplitMessage,convertMarkdownToTelegram,convertToTelegramButtons, andcleanText. Remove the redundant test file and maintain a single test suite to reduce maintenance overhead.__tests__/utils.test.ts (2)
1-1: Unused import.
spyOnis imported but not used; remove it.-import { afterEach, describe, expect, it, mock, spyOn } from 'bun:test'; +import { afterEach, describe, expect, it, mock } from 'bun:test';
125-247: Duplicate coverage with button-conversion.test.ts.Consolidate overlapping button/markdown/split tests into one file to avoid drift.
__tests__/messageManager.test.ts (1)
145-151: Call count assertion is brittle.
sendMessageMock.mock.calls.length >= 2may be sensitive to internal changes (typing action, chunking). Prefer explicit expectations on chunks returned and at-least-once message send.- expect(sendMessageMock.mock.calls.length).toBeGreaterThanOrEqual(2); + expect(sendMessageMock).toHaveBeenCalled();src/messageManager.ts (2)
105-109: Remove redundant PDF check.The condition
!message.document?.mime_type?.startsWith('application/pdf')is redundant. If a MIME type starts with'image/', it cannot also start with'application/pdf'.Apply this diff to simplify the logic:
- } else if ( - 'document' in message && - message.document?.mime_type?.startsWith('image/') && - !message.document?.mime_type?.startsWith('application/pdf') - ) { + } else if ( + 'document' in message && + message.document?.mime_type?.startsWith('image/') + ) {
320-327: Consider using documentInfo for source determination.The code re-checks
document.mime_type?.startsWith('application/pdf')to determine the source type, even thoughdocumentInfois already available from theprocessDocumentcall. While the current approach is functional and clear, you could optionally add atypefield toDocumentProcessingResultto avoid re-checking the MIME type.For example, you could extend the interface:
interface DocumentProcessingResult { title: string; fullText: string; formattedDescription: string; fileName: string; mimeType: string | undefined; fileSize: number | undefined; documentType?: 'PDF' | 'Text' | 'Document'; // Add this field error?: string; }Then update processors to set this field and use it here instead of re-checking the MIME type.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (8)
__tests__/button-conversion.test.ts(1 hunks)__tests__/messageManager.test.ts(1 hunks)__tests__/telegramClient.test.ts(1 hunks)__tests__/utils.test.ts(4 hunks)package.json(1 hunks)src/environment.ts(2 hunks)src/messageManager.ts(11 hunks)src/utils.ts(3 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
src/utils.ts
[error] 244-245: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🔇 Additional comments (10)
src/environment.ts (1)
20-23: Signature/flow LGTM.
error.issuesis the correct Zod property; returning null on failure is appropriate.__tests__/telegramClient.test.ts (2)
1-9: LGTM on export checks.Bun test runner import and dynamic imports look correct.
12-19: Plugin metadata assertions OK.Name/description checks are straightforward and low-flake.
__tests__/messageManager.test.ts (2)
257-295: Enum value checks OK.Good to lock MediaType string values.
43-53: fs mock shape correctly matches the import style—no issue.The source imports
fsasimport fs from 'fs'(default import), and the mock correctly provides{ default: { existsSync, createReadStream, ... } }. The mock structure is compatible with this import style. No changes needed.Likely an incorrect or invalid review comment.
package.json (1)
21-27: Version pairing verified: telegraf 4.16.3 + @telegraf/types 7.1.0 is correct for Bot API 7.1 support.Telegraf v4.16.3 supports Telegram Bot API 7.1 with matching @telegraf/types. The dependency versions in package.json are appropriate.
src/messageManager.ts (4)
173-191: LGTM! Pattern-based MIME type matching improves flexibility.The refactor to use
startsWithpattern matching is a good improvement. It allows handling multiple MIME type variants (e.g.,text/plain,text/csv,text/markdown) with a single pattern, making the code more maintainable and flexible.
165-165: LGTM! Structured error logging improves observability.The migration to structured logging format (e.g.,
logger.error({ error }, 'message')) is a good improvement. This format is better suited for log aggregation systems and makes error tracking more effective.Also applies to: 232-232, 269-269, 330-330
196-242: LGTM! Document processing methods are well-structured.The
processPdfDocumentandprocessTextDocumentmethods have consistent error handling patterns, appropriate fallbacks (emptyfullTexton errors), and structured logging. The refactoring maintains clarity while improving maintainability.Also applies to: 247-279
288-376: LGTM! Message processing integrates well with document handling.The
processMessagemethod correctly integrates with the refactored document processing flow. The attachment structure includes appropriate metadata, and error handling ensures that attachments are created even when processing fails, which aids in debugging.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Resolved merge conflicts in src/messageManager.ts by keeping structured logging approach with pino-style error logging.
Upgraded @elizaos/core and related dependencies in package.json and bun.lock, removing unused packages and updating versions for better compatibility. Refactored src/utils.ts to align with new dependency versions and improve code maintainability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/messageManager.ts (1)
121-121: Consider standardizing error logging patterns across the file.The file currently uses three different error logging patterns:
console.error(line 121){ error }(the updated lines){ originalError: error }or{ error: errorMessage, originalError: error }(lines 512-515, 815-820, 898-902)Standardizing to a single pattern would improve consistency and make log analysis easier.
Consider applying this pattern uniformly:
- console.error('❌ Error processing image:', error); + logger.error({ error }, 'Error processing image');And for other locations, decide whether to use
{ error }or{ originalError: error }consistently throughout the file.Also applies to: 512-515, 815-820, 898-902
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
src/messageManager.ts(4 hunks)
🔇 Additional comments (1)
src/messageManager.ts (1)
164-164: LGTM! Structured error logging improves observability.The migration to structured logging format (
logger.error({ error }, 'message')) is excellent. This pattern enables better log aggregation, filtering, and monitoring in production systems.Also applies to: 231-231, 268-268, 329-329
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/utils.ts (1)
206-235: LGTM: Safe button conversion with comprehensive validation.The refactored logic correctly validates button inputs, skips invalid entries with warnings, and defaults unknown
kindvalues to URL buttons. The validation ensurestextandurlare present before constructing Telegram buttons, preventing runtime errors.Optional enhancement: The validation warning on line 213 could be more specific:
- logger.warn({ button }, 'Invalid button configuration, skipping'); + logger.warn({ button }, 'Button missing required text or url field, skipping');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
package.json(2 hunks)src/utils.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🔇 Additional comments (2)
src/utils.ts (2)
4-4: LGTM: Logger import supports structured logging.The logger import enables structured logging for button validation warnings, aligning with the PR's goal to improve error handling and logging consistency.
242-246: LGTM: Implements suggested lint fix.The split/join approach correctly removes NULL characters while avoiding the Biome linter error for control characters in regex literals. The implementation matches the suggestion from the previous review.
2 participants
Note
Safely converts Telegram buttons with validation and fallbacks, improves structured logging, expands test coverage, and updates package/core versions.
convertToTelegramButtons: validate inputs, skip invalid buttons, default unknownkindtourl, and log warnings.cleanText: remove NULLs via split/join to avoid regex control-char issues.logger.error({ error }, ...)in document processing paths.1.6.2and upgrade@elizaos/coreto^1.6.2.Written by Cursor Bugbot for commit 1b28012. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Tests
Chores
Bug Fixes