Conversation
…, errorHandler) Co-authored-by: warengonzaga <15052701+warengonzaga@users.noreply.github.com>
…ndRegistry) Co-authored-by: warengonzaga <15052701+warengonzaga@users.noreply.github.com>
…eved Co-authored-by: warengonzaga <15052701+warengonzaga@users.noreply.github.com>
…ificant coverage improvements Co-authored-by: warengonzaga <15052701+warengonzaga@users.noreply.github.com>
…nd move to docs folder Co-authored-by: warengonzaga <15052701+warengonzaga@users.noreply.github.com>
…rtCommand - Major coverage increase Co-authored-by: warengonzaga <15052701+warengonzaga@users.noreply.github.com>
Co-authored-by: warengonzaga <15052701+warengonzaga@users.noreply.github.com>
Co-authored-by: warengonzaga <15052701+warengonzaga@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: warengonzaga <15052701+warengonzaga@users.noreply.github.com>
Co-authored-by: warengonzaga <15052701+warengonzaga@users.noreply.github.com>
…-87db-3b66780dc0f7 Update migration documentation to reflect latest codebase and move to docs folder
…vior Co-authored-by: warengonzaga <15052701+warengonzaga@users.noreply.github.com>
…-a780-10eca29d5930 Fix failing tests by aligning test expectations with actual BaseCommand implementation behavior
There was a problem hiding this comment.
Pull Request Overview
This PR marks the release of v1.0.1, primarily focused on enhancing test coverage and documentation. The changes transition the project from release candidate status to a stable release while adding comprehensive test suites for improved code quality and reliability.
- Added extensive test coverage across multiple command modules and utility functions
- Updated version number from "1.0.0-rc2" to "1.0.1" in package.json
- Refined documentation with updated migration guide and README references
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| src/tests/* | Added comprehensive test suites for commands, services, and utilities |
| package.json | Updated version to 1.0.1 |
| docs/migration.md | Updated migration guide for stable release |
| README.md | Fixed migration guide link reference |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThis PR migrates the repository from Yarn to pnpm, bumps package version to v1.0.1, updates CI/Docker/docs/tooling to pnpm, adjusts docker-compose images/networking, adds many unit tests, and includes small command and email-manager logic changes. No public API signature changes detected. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 23
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/migration.md (1)
99-104: Railway envs: document both Redis URLs (or same URL for both).Elsewhere you require both PLATFORM_REDIS_URL and WEBHOOK_REDIS_URL; Railway block shows only WEBHOOK_REDIS_URL. Add PLATFORM_REDIS_URL and note they can point to the same instance if only one Redis is used.
# Update Railway environment variables: ADMIN_USERS=123456789,987654321 BOT_USERNAME=your_bot_username MY_COMPANY_NAME=Your Company Name +PLATFORM_REDIS_URL="${{Redis.REDIS_URL}}" # Often the same Redis instance WEBHOOK_REDIS_URL="${{Redis.REDIS_URL}}" # If using Railway Redis
🧹 Nitpick comments (16)
package.json (2)
61-61: Avoid shipping Yarn as a runtime dependency."yarn" under dependencies bloats installs and is unnecessary since packageManager already enforces Yarn usage.
Apply this diff:
- "yarn": "1.22.22",
78-79: Pin Vitest to reduce CI flakiness.^2.1.8 allows upgrades up to <3.0.0. Prefer a strict pin (or ~2.1.8) for deterministic runs.
Based on learnings
- "vitest": "^2.1.8" + "vitest": "2.1.8"README.md (1)
26-36: Add Migration Guide to the docs list for discoverability.Even with the banner note, a docs bullet improves navigation.
- **[🛡️ Security Policy](./SECURITY.md)** - Security Features & Vulnerability Reporting + - **[🔄 Migration Guide](./docs/migration.md)** - Upgrading from Beta to v1.0.0src/__tests__/validationService.test.ts (1)
6-10: Prefer sorted named importsSort the mixed type/value imports for consistency with sort-imports.
-import { - ValidationService, - type ValidationCheck, - type ValidationResult -} from '../services/validationService.js'; +import { + type ValidationCheck, + type ValidationResult, + ValidationService +} from '../services/validationService.js';src/__tests__/adminCommands.test.ts (1)
320-323: Avoid explicit any on telegram mockUse a precise type to silence “Unexpected any”.
- mockContext.telegram = { - sendMessage: vi.fn().mockResolvedValue({ message_id: 123 }) - } as any; + mockContext.telegram = { + sendMessage: vi.fn().mockResolvedValue({ message_id: 123 }) + } as unknown as BotContext['telegram'];src/__tests__/baseCommand.test.ts (4)
6-12: Sort named imports for BaseCommand typesAlign with sort-imports rule.
-import { - BaseCommand, - type CommandMetadata, - type ICommand, - type IConversationProcessor, - type ICallbackProcessor -} from '../commands/base/BaseCommand.js'; +import { + BaseCommand, + type CommandMetadata, + type ICallbackProcessor, + type ICommand, + type IConversationProcessor +} from '../commands/base/BaseCommand.js';
236-237: Replace any-cast in spy with precise typingAvoid explicit any; cast to an interface exposing the protected member for spying.
- const handleErrorSpy = vi.spyOn(testCommand, 'handleError' as any); + const handleErrorSpy = vi.spyOn( + testCommand as unknown as { handleError(ctx: BotContext, error: unknown): Promise<void> }, + 'handleError' + );
246-250: Remove non-null assertion in completion log checkGuard the value instead of using “!”.
- const logCalls = vi.mocked(LogEngine.info).mock.calls; - const completionLog = logCalls.find(call => call[0].includes('completed')); - expect(completionLog).toBeDefined(); - expect(completionLog![1]).toHaveProperty('executionTime'); + const logCalls = vi.mocked(LogEngine.info).mock.calls; + const completionLog = logCalls.find(call => call[0]?.includes('completed')); + expect(completionLog).toBeDefined(); + if (completionLog) { + expect(completionLog[1]).toHaveProperty('executionTime'); + }
295-296: Avoid explicit any on chat mockUse a compatible contextual type instead.
- mockCtx.chat = { id: 456, type: 'group' } as any; + mockCtx.chat = { id: 456, type: 'group' } as unknown as BotContext['chat'];src/__tests__/errorHandler.test.ts (1)
4-4: Sort vitest named imports (alphabetically)Keeps ESLint happy.
-import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';src/__tests__/enhancedEmailManager.test.ts (2)
8-9: Sort vitest members and keep imports tidy-import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
146-153: Optional: Make env-domain test deterministicIf you intend to assert a custom domain, set env then re-import the module (vi.resetModules + dynamic import) to pick up the new config; otherwise, clarify expectation as "no effect" as you did. Example pattern:
vi.resetModules(); process.env.DUMMY_EMAIL_DOMAIN = 'custom.domain.com'; const { generateDummyEmail } = await import('../utils/emailManager.js'); expect(generateDummyEmail(12345, 'testuser')).toBe('testuser_12345@custom.domain.com');src/__tests__/viewEmailCommand.test.ts (1)
196-200: Stabilize date assertion to avoid locale-induced flakiness.toLocaleDateString varies by locale; asserting slashes is brittle. Relax the check.
-// The exact date format depends on locale, so we just check it's there -expect(message.match(/📅 \*\*Set on:\*\* \d+\/\d+\/\d+/)).toBeTruthy(); +// The exact date format depends on locale; just assert presence of a date-like value +expect(message).toMatch(/📅 \*\*Set on:\*\* [\d/.\-]+/);src/__tests__/supportCommand.test.ts (1)
7-7: Sort Vitest import members alphabetically to satisfy ESLint.-import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';src/__tests__/setEmailCommand.test.ts (2)
8-8: Sort Vitest import members alphabetically to satisfy ESLint.-import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
49-49: Remove unused import to fix ESLint no-unused-vars.escapeMarkdown is mocked for the SUT but not referenced in this test file. Drop the named import.
-import { escapeMarkdown } from '../utils/markdownEscape.js';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
README.md(1 hunks)docs/migration.md(7 hunks)package.json(1 hunks)src/__tests__/adminCommands.test.ts(1 hunks)src/__tests__/baseCommand.test.ts(1 hunks)src/__tests__/commandExecutor.test.ts(1 hunks)src/__tests__/commandRegistry.test.ts(1 hunks)src/__tests__/commands.index.test.ts(1 hunks)src/__tests__/enhancedEmailManager.test.ts(1 hunks)src/__tests__/errorHandler.test.ts(1 hunks)src/__tests__/infoCommands.test.ts(1 hunks)src/__tests__/setEmailCommand.test.ts(1 hunks)src/__tests__/stateCommands.test.ts(1 hunks)src/__tests__/supportCommand.test.ts(1 hunks)src/__tests__/validationService.test.ts(1 hunks)src/__tests__/viewEmailCommand.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-22T10:12:09.684Z
Learnt from: warengonzaga
PR: wgtechlabs/unthread-telegram-bot#24
File: docker-compose.yaml:29-29
Timestamp: 2025-06-22T10:12:09.684Z
Learning: The Unthread Telegram Bot project is currently in beta development phase (v1.0.0-beta.2) and does not yet have official versioned Docker builds available, making the use of :latest tags acceptable for development purposes in docker-compose.yaml.
Applied to files:
package.json
🧬 Code graph analysis (13)
src/__tests__/commandExecutor.test.ts (3)
src/types/index.ts (1)
BotContext(44-46)src/commands/utils/commandExecutor.ts (3)
createCommandExecutor(48-87)CommandExecutorOptions(26-35)createProcessorExecutor(96-109)src/utils/errorContextBuilder.ts (1)
ErrorContextBuilder(51-313)
src/__tests__/commands.index.test.ts (2)
src/types/index.ts (1)
BotContext(44-46)src/commands/index.ts (19)
initializeCommands(56-98)startCommand(148-148)helpCommand(149-149)versionCommand(150-150)aboutCommand(151-151)activateCommand(152-152)supportCommand(153-153)cancelCommand(154-154)resetCommand(155-155)setupCommand(156-156)templatesCommand(157-157)processSupportConversation(160-160)handleCallbackQuery(161-161)processSetupTextInput(162-162)processTemplateEditInput(163-163)handleTemplateEditCallback(166-168)handleTemplateCancelCallback(170-172)handleTemplateCancelEditCallback(174-176)handleTemplateBackMenuCallback(178-180)
src/__tests__/errorHandler.test.ts (1)
src/commands/utils/errorHandler.ts (4)
ERROR_CODES(52-76)getErrorDetails(325-425)logError(490-517)createUserErrorMessage(526-552)
src/__tests__/enhancedEmailManager.test.ts (2)
src/utils/emailManager.ts (4)
generateDummyEmail(148-160)getUserEmailPreferences(168-196)updateUserEmail(206-291)deliverPendingAgentMessages(373-410)src/sdk/types.ts (1)
UserData(109-124)
src/__tests__/adminCommands.test.ts (7)
src/types/index.ts (1)
BotContext(44-46)src/utils/permissions.ts (3)
validateAdminAccess(50-97)isBotAdmin(232-261)checkAndPromptBotAdmin(268-277)src/commands/admin/AdminCommands.ts (3)
ActivateCommand(22-112)SetupCommand(114-379)TemplatesCommand(381-533)src/sdk/types.ts (2)
AdminProfile(127-134)GroupConfig(180-194)src/commands/utils/errorHandler.ts (2)
createUserErrorMessage(526-552)logError(490-517)src/utils/adminManager.ts (1)
createDmSetupSession(288-332)src/utils/globalTemplateManager.ts (1)
GlobalTemplateManager(18-417)
src/__tests__/infoCommands.test.ts (3)
src/types/index.ts (1)
BotContext(44-46)src/commands/basic/InfoCommands.ts (4)
VersionCommand(128-163)StartCommand(51-71)HelpCommand(73-126)AboutCommand(165-199)src/config/env.ts (2)
getCompanyName(366-384)isAdminUser(354-357)
src/__tests__/stateCommands.test.ts (3)
src/types/index.ts (1)
BotContext(44-46)src/commands/basic/StateCommands.ts (2)
CancelCommand(15-44)ResetCommand(46-80)src/commands/utils/errorHandler.ts (1)
logError(490-517)
src/__tests__/supportCommand.test.ts (3)
src/commands/support/SupportCommandClean.ts (1)
SupportCommand(13-136)src/types/index.ts (1)
BotContext(44-46)src/sdk/types.ts (1)
UserState(163-177)
src/__tests__/setEmailCommand.test.ts (4)
src/commands/basic/SetEmailCommand.ts (1)
SetEmailCommand(23-196)src/types/index.ts (1)
BotContext(44-46)src/utils/messageContentExtractor.ts (1)
getMessageText(23-39)src/utils/emailManager.ts (3)
getUserEmailPreferences(168-196)updateUserEmail(206-291)deliverPendingAgentMessages(373-410)
src/__tests__/validationService.test.ts (2)
src/types/index.ts (1)
BotContext(44-46)src/services/validationService.ts (2)
ValidationService(29-166)ValidationCheck(12-17)
src/__tests__/viewEmailCommand.test.ts (3)
src/commands/basic/ViewEmailCommand.ts (1)
ViewEmailCommand(16-123)src/types/index.ts (1)
BotContext(44-46)src/utils/emailManager.ts (1)
getUserEmailPreferences(168-196)
src/__tests__/baseCommand.test.ts (4)
src/commands/base/BaseCommand.ts (4)
CommandMetadata(12-21)ICommand(23-27)IConversationProcessor(29-32)ICallbackProcessor(34-37)src/types/index.ts (1)
BotContext(44-46)src/config/env.ts (1)
isAdminUser(354-357)src/utils/permissions.ts (1)
validateAdminAccess(50-97)
src/__tests__/commandRegistry.test.ts (5)
src/commands/base/BaseCommand.ts (4)
ICommand(23-27)CommandMetadata(12-21)IConversationProcessor(29-32)ICallbackProcessor(34-37)src/types/index.ts (1)
BotContext(44-46)src/commands/base/CommandRegistry.ts (1)
CommandRegistry(16-352)src/utils/logConfig.ts (1)
StartupLogger(153-337)src/config/env.ts (1)
isAdminUser(354-357)
🪛 ESLint
src/__tests__/commandExecutor.test.ts
[error] 4-4: Member 'beforeEach' of the import declaration should be sorted alphabetically.
(sort-imports)
[error] 9-9: Member 'CommandExecutorOptions' of the import declaration should be sorted alphabetically.
(sort-imports)
src/__tests__/commands.index.test.ts
[error] 4-4: Member 'beforeEach' of the import declaration should be sorted alphabetically.
(sort-imports)
[error] 9-9: Member 'processCallback' of the import declaration should be sorted alphabetically.
(sort-imports)
src/__tests__/errorHandler.test.ts
[error] 4-4: Member 'beforeEach' of the import declaration should be sorted alphabetically.
(sort-imports)
[error] 8-8: Member 'createUserErrorMessage' of the import declaration should be sorted alphabetically.
(sort-imports)
[error] 12-12: 'ErrorDetails' is defined but never used. Allowed unused vars must match /^_/u.
(no-unused-vars)
[error] 12-12: 'ErrorDetails' is defined but never used.
(unused-imports/no-unused-imports)
src/__tests__/enhancedEmailManager.test.ts
[error] 8-8: Member 'expect' of the import declaration should be sorted alphabetically.
(sort-imports)
[error] 35-35: Member 'generateDummyEmail' of the import declaration should be sorted alphabetically.
(sort-imports)
[error] 40-40: 'EmailValidationResult' is defined but never used. Allowed unused vars must match /^_/u.
(no-unused-vars)
[error] 40-40: 'EmailValidationResult' is defined but never used.
(unused-imports/no-unused-imports)
[error] 41-41: 'UserEmailPreferences' is defined but never used. Allowed unused vars must match /^_/u.
(no-unused-vars)
[error] 41-41: 'UserEmailPreferences' is defined but never used.
(unused-imports/no-unused-imports)
[error] 407-407: 'originalConsole' is assigned a value but never used. Allowed unused vars must match /^_/u.
(no-unused-vars)
src/__tests__/adminCommands.test.ts
[error] 8-8: Member 'expect' of the import declaration should be sorted alphabetically.
(sort-imports)
[error] 72-72: 'getCompanyName' is defined but never used. Allowed unused vars must match /^_/u.
(no-unused-vars)
[error] 72-72: 'getCompanyName' is defined but never used.
(unused-imports/no-unused-imports)
[error] 74-74: 'ValidationService' is defined but never used. Allowed unused vars must match /^_/u.
(no-unused-vars)
[error] 74-74: 'ValidationService' is defined but never used.
(unused-imports/no-unused-imports)
[error] 75-75: 'SetupCallbackProcessor' is defined but never used. Allowed unused vars must match /^_/u.
(no-unused-vars)
[error] 75-75: 'SetupCallbackProcessor' is defined but never used.
(unused-imports/no-unused-imports)
src/__tests__/infoCommands.test.ts
[error] 4-4: Member 'beforeEach' of the import declaration should be sorted alphabetically.
(sort-imports)
[error] 8-8: Member 'HelpCommand' of the import declaration should be sorted alphabetically.
(sort-imports)
src/__tests__/stateCommands.test.ts
[error] 4-4: Member 'beforeEach' of the import declaration should be sorted alphabetically.
(sort-imports)
src/__tests__/supportCommand.test.ts
[error] 7-7: Member 'expect' of the import declaration should be sorted alphabetically.
(sort-imports)
src/__tests__/setEmailCommand.test.ts
[error] 8-8: Member 'expect' of the import declaration should be sorted alphabetically.
(sort-imports)
[error] 49-49: 'escapeMarkdown' is defined but never used. Allowed unused vars must match /^_/u.
(no-unused-vars)
[error] 49-49: 'escapeMarkdown' is defined but never used.
(unused-imports/no-unused-imports)
src/__tests__/validationService.test.ts
[error] 4-4: Member 'beforeEach' of the import declaration should be sorted alphabetically.
(sort-imports)
[error] 8-8: Member 'ValidationCheck' of the import declaration should be sorted alphabetically.
(sort-imports)
src/__tests__/viewEmailCommand.test.ts
[error] 4-4: Member 'beforeEach' of the import declaration should be sorted alphabetically.
(sort-imports)
src/__tests__/baseCommand.test.ts
[error] 4-4: Member 'beforeEach' of the import declaration should be sorted alphabetically.
(sort-imports)
[error] 11-11: Member 'ICallbackProcessor' of the import declaration should be sorted alphabetically.
(sort-imports)
[error] 132-132: 'adminCommand' is assigned a value but never used. Allowed unused vars must match /^_/u.
(no-unused-vars)
src/__tests__/commandRegistry.test.ts
[error] 4-4: Member 'beforeEach' of the import declaration should be sorted alphabetically.
(sort-imports)
[error] 12-12: Member 'ICallbackProcessor' of the import declaration should be sorted alphabetically.
(sort-imports)
[error] 44-44: 'metadata' is defined but never used. Allowed unused args must match /^_/u.
(no-unused-vars)
[error] 45-45: 'executeImpl' is assigned a value but never used. Allowed unused args must match /^_/u.
(no-unused-vars)
[error] 45-45: 'ctx' is defined but never used. Allowed unused args must match /^_/u.
(no-unused-vars)
[error] 59-59: 'canHandleImpl' is assigned a value but never used. Allowed unused args must match /^_/u.
(no-unused-vars)
[error] 59-59: 'ctx' is defined but never used. Allowed unused args must match /^_/u.
(no-unused-vars)
[error] 60-60: 'processImpl' is assigned a value but never used. Allowed unused args must match /^_/u.
(no-unused-vars)
[error] 60-60: 'ctx' is defined but never used. Allowed unused args must match /^_/u.
(no-unused-vars)
[error] 74-74: 'canHandleImpl' is assigned a value but never used. Allowed unused args must match /^_/u.
(no-unused-vars)
[error] 74-74: 'data' is defined but never used. Allowed unused args must match /^_/u.
(no-unused-vars)
[error] 75-75: 'processImpl' is assigned a value but never used. Allowed unused args must match /^_/u.
(no-unused-vars)
[error] 75-75: 'ctx' is defined but never used. Allowed unused args must match /^_/u.
(no-unused-vars)
[error] 75-75: 'data' is defined but never used. Allowed unused args must match /^_/u.
(no-unused-vars)
🪛 GitHub Actions: Validate
src/__tests__/adminCommands.test.ts
[error] 8-8: Member 'expect' of the import declaration should be sorted alphabetically. (sort-imports)
🪛 GitHub Check: Validate Changes
src/__tests__/adminCommands.test.ts
[warning] 322-322:
Unexpected any. Specify a different type
[failure] 75-75:
'SetupCallbackProcessor' is defined but never used
[failure] 75-75:
'SetupCallbackProcessor' is defined but never used. Allowed unused vars must match /^_/u
[failure] 74-74:
'ValidationService' is defined but never used
[failure] 74-74:
'ValidationService' is defined but never used. Allowed unused vars must match /^_/u
[failure] 72-72:
'getCompanyName' is defined but never used
[failure] 72-72:
'getCompanyName' is defined but never used. Allowed unused vars must match /^_/u
[failure] 8-8:
Member 'expect' of the import declaration should be sorted alphabetically
src/__tests__/baseCommand.test.ts
[warning] 295-295:
Unexpected any. Specify a different type
[warning] 249-249:
Forbidden non-null assertion
[warning] 236-236:
Unexpected any. Specify a different type
[failure] 4-4:
Member 'beforeEach' of the import declaration should be sorted alphabetically
🔇 Additional comments (7)
package.json (1)
3-3: Version bump acknowledged.All green on 1.0.1. Proceed to tag and publish as planned.
docs/migration.md (2)
201-203: Verify logging env var name (LOG_LEVEL vs LOG_MODE).Using @wgtechlabs/log-engine v2.2.0 favors “mode” over legacy “level”. If the app wires LOG_MODE, update the guide; if LOG_LEVEL is still supported, keep as-is.
Based on learnings
45-57: Nice clarity on required/optional envs.Explicit CRITICAL/OPTIONAL labels and rename note are clear. No changes requested.
README.md (1)
11-11: Migration link update looks good.Reads clean and points to the new docs path.
src/__tests__/infoCommands.test.ts (1)
155-166: Verify expected error message for missing userHelpCommand implementation replies “❌ Unable to determine user identity.”, while test expects “❌ Invalid command context. Please try again.” Adjust either the code or the assertion (e.g., check for the implemented text or use toContain).
src/__tests__/adminCommands.test.ts (1)
414-457: Reconfirm expected Templates UI messageThis test expects the fallback “Template management interface is being prepared” even when getGlobalTemplates resolves successfully. The implementation shows the fallback only on error. Please confirm intent and adjust either the mock to reject (to exercise the fallback) or the expectation to assert the real manager output.
src/__tests__/supportCommand.test.ts (1)
387-397: Affirmed, sir: "Group Setup Required" is present; no changes required.
- Updated Railway section in migration.md to include both PLATFORM_REDIS_URL and WEBHOOK_REDIS_URL - Fixed button text in BaseCommand.ts from 'Start Activation' to 'Activate Admin Access' - Updated test expectation in adminCommands.test.ts to match production button text - Fixed import path in baseCommand.test.ts (from ../../types to ../types) - Reordered imports in validationService.test.ts to satisfy sort-imports lint rule Co-authored-by: warengonzaga <15052701+warengonzaga@users.noreply.github.com>
- Sorted vitest imports alphabetically in all test files (afterEach, beforeEach, describe, expect, it, vi) - Fixed import sorting violations across 16 test files - All 737 tests still pass - All sort-imports lint errors in test files resolved Co-authored-by: warengonzaga <15052701+warengonzaga@users.noreply.github.com>
Fix migration docs, test expectations, and import paths
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/__tests__/setEmailCommand.test.ts (1)
13-23: Realign formatEmailForDisplay mock with production usageSir, the command under test imports
formatEmailForDisplayfrom../utils/markdownEscape.js, yet the suite still mocks and spies on the email manager module. Consequently everyvi.mocked(formatEmailForDisplay)assertion misses the actual call path, leaving the tests red. We can restore coherence by moving the mock/import over tomarkdownEscapeand lettingemailManagerexpose only the collaborators it truly owns.vi.mock('../utils/emailManager.js', () => ({ - deliverPendingAgentMessages: vi.fn(), - formatEmailForDisplay: vi.fn(), + deliverPendingAgentMessages: vi.fn(), getUserEmailPreferences: vi.fn(), updateUserEmail: vi.fn(), validateEmail: vi.fn(), })); vi.mock('../utils/markdownEscape.js', () => ({ - escapeMarkdown: vi.fn((text) => text), + escapeMarkdown: vi.fn((text) => text), + formatEmailForDisplay: vi.fn(), })); @@ -import { - deliverPendingAgentMessages, - formatEmailForDisplay, - getUserEmailPreferences, - updateUserEmail, - validateEmail -} from '../utils/emailManager.js'; -import { escapeMarkdown } from '../utils/markdownEscape.js'; +import { + deliverPendingAgentMessages, + getUserEmailPreferences, + updateUserEmail, + validateEmail +} from '../utils/emailManager.js'; +import { formatEmailForDisplay } from '../utils/markdownEscape.js';Also applies to: 42-48
src/__tests__/adminCommands.test.ts (1)
450-454: Match the success-path template manager responseAt present the happy-path reply begins with “📝 Message Template Manager” and ships an inline keyboard. Expecting the provisional “Template management interface is being prepared” payload forces the suite to fail. Let’s align the assertion with the live response and allow for the markup envelope.
- expect(mockContext.reply).toHaveBeenCalledWith( - expect.stringContaining('Template management interface is being prepared'), - { parse_mode: 'Markdown' } - ); + expect(mockContext.reply).toHaveBeenCalledWith( + expect.stringContaining('📝 **Message Template Manager**'), + expect.objectContaining({ + parse_mode: 'Markdown', + reply_markup: expect.any(Object) + }) + );
🧹 Nitpick comments (4)
src/__tests__/validationService.test.ts (2)
143-148: Consider testing private methods through the public API, if I may suggest.Sir, while the current approach of casting to
anyto accessbuildValidationMessagefunctions correctly, it does couple your tests to the implementation's internal structure. The effortless experience principle suggests we test observable behavior through the public interface—performSetupValidationalready exercises this method comprehensively.This approach offers several advantages: tests remain valid through refactoring, implementation details stay properly encapsulated, and the test suite focuses on the contract rather than the mechanics.
If you prefer to retain explicit coverage of message formatting edge cases, you might consider elevating
buildValidationMessageto a public static method or extracting it to a separate utility module where direct testing feels more natural.Also applies to: 167-172, 188-193, 203-208, 221-226
235-273: Interface tests provide limited value at runtime, sir.These tests validate that objects conforming to
ValidationCheckandValidationResultcan be instantiated—however, TypeScript's compiler already guarantees interface conformance at build time. Since interfaces are erased during transpilation, these assertions essentially verify JavaScript object creation rather than type safety.Should you wish to streamline the suite, removing these tests would eliminate redundancy without sacrificing any meaningful coverage. Your integration tests for
performSetupValidationalready confirm that the service produces correctly-shaped results.src/__tests__/supportCommand.test.ts (2)
13-22: Consider removing unused mock functions.I've observed several mock functions that remain untouched throughout the test suite:
storeUserState(line 16),clearUserState(line 17), andgetUserByTelegramId(line 20). While maintaining them as placeholders is harmless, removing unused mocks enhances clarity and reduces maintenance overhead.Apply this diff if these functions are indeed unnecessary:
vi.mock('../sdk/bots-brain/index.js', () => ({ BotsStore: { getUserState: vi.fn(), - storeUserState: vi.fn(), - clearUserState: vi.fn(), setUserState: vi.fn(), getGroupConfig: vi.fn(), - getUserByTelegramId: vi.fn(), } }));
248-258: Strengthen non-Error exception handling verification.Sir, the current test for non-Error exceptions relies on verifying that
error.messagebecomesundefinedwhen a string is thrown. This approach is rather fragile, as it tests implementation details (the type coercion in the catch block) rather than the observable behavior. Should the implementation adopt more robust error handling (such as normalizing thrown values to Error instances), this test would incorrectly fail.Consider refactoring to verify the essential behavior—that the error is logged and a user-facing message is displayed—without asserting on the specific logged error value:
it('should handle non-Error exceptions', async () => { vi.mocked(BotsStore.getUserState).mockRejectedValue('String error'); await supportCommand.execute(mockContext as BotContext); - expect(LogEngine.error).toHaveBeenCalledWith('Error in support command', { - error: undefined, - userId: 12345, - chatId: -67890 - }); + expect(LogEngine.error).toHaveBeenCalledWith( + 'Error in support command', + expect.objectContaining({ + userId: 12345, + chatId: -67890 + }) + ); + expect(mockContext.reply).toHaveBeenCalledWith( + expect.stringContaining('Error starting support ticket'), + { parse_mode: 'Markdown' } + ); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
docs/migration.md(9 hunks)src/__tests__/adminCommands.test.ts(1 hunks)src/__tests__/adminManager.test.ts(1 hunks)src/__tests__/attachmentDetection.test.ts(1 hunks)src/__tests__/baseCommand.test.ts(1 hunks)src/__tests__/bot.core.test.ts(1 hunks)src/__tests__/bots-brain.sdk.test.ts(1 hunks)src/__tests__/database.connection.test.ts(1 hunks)src/__tests__/emailManager.test.ts(1 hunks)src/__tests__/enhancedEmailManager.test.ts(1 hunks)src/__tests__/errorContextBuilder.test.ts(1 hunks)src/__tests__/logConfig.test.ts(1 hunks)src/__tests__/logging.test.ts(1 hunks)src/__tests__/messageAnalyzer.test.ts(1 hunks)src/__tests__/permissions.test.ts(1 hunks)src/__tests__/sdkTypes.test.ts(1 hunks)src/__tests__/setEmailCommand.test.ts(1 hunks)src/__tests__/supportCommand.test.ts(1 hunks)src/__tests__/validationService.test.ts(1 hunks)src/commands/base/BaseCommand.ts(1 hunks)
✅ Files skipped from review due to trivial changes (10)
- src/tests/bot.core.test.ts
- src/tests/emailManager.test.ts
- src/tests/logging.test.ts
- src/commands/base/BaseCommand.ts
- src/tests/permissions.test.ts
- src/tests/attachmentDetection.test.ts
- src/tests/bots-brain.sdk.test.ts
- src/tests/logConfig.test.ts
- src/tests/messageAnalyzer.test.ts
- src/tests/sdkTypes.test.ts
🧰 Additional context used
🧬 Code graph analysis (6)
src/__tests__/setEmailCommand.test.ts (4)
src/commands/basic/SetEmailCommand.ts (1)
SetEmailCommand(23-196)src/types/index.ts (1)
BotContext(44-46)src/utils/messageContentExtractor.ts (1)
getMessageText(23-39)src/utils/emailManager.ts (3)
getUserEmailPreferences(168-196)updateUserEmail(206-291)deliverPendingAgentMessages(373-410)
src/__tests__/supportCommand.test.ts (4)
src/commands/index.ts (1)
supportCommand(153-153)src/commands/support/SupportCommandClean.ts (1)
SupportCommand(13-136)src/types/index.ts (1)
BotContext(44-46)src/sdk/types.ts (1)
UserState(163-177)
src/__tests__/validationService.test.ts (2)
src/types/index.ts (1)
BotContext(44-46)src/services/validationService.ts (2)
ValidationService(29-166)ValidationCheck(12-17)
src/__tests__/enhancedEmailManager.test.ts (2)
src/utils/emailManager.ts (4)
generateDummyEmail(148-160)getUserEmailPreferences(168-196)updateUserEmail(206-291)deliverPendingAgentMessages(373-410)src/sdk/types.ts (1)
UserData(109-124)
src/__tests__/adminCommands.test.ts (7)
src/types/index.ts (1)
BotContext(44-46)src/utils/permissions.ts (3)
validateAdminAccess(50-97)isBotAdmin(232-261)checkAndPromptBotAdmin(268-277)src/commands/admin/AdminCommands.ts (3)
ActivateCommand(22-112)SetupCommand(114-379)TemplatesCommand(381-533)src/sdk/types.ts (2)
AdminProfile(127-134)GroupConfig(180-194)src/commands/utils/errorHandler.ts (2)
createUserErrorMessage(526-552)logError(490-517)src/utils/adminManager.ts (1)
createDmSetupSession(288-332)src/utils/globalTemplateManager.ts (1)
GlobalTemplateManager(18-417)
src/__tests__/baseCommand.test.ts (4)
src/commands/base/BaseCommand.ts (4)
CommandMetadata(12-21)ICommand(23-27)IConversationProcessor(29-32)ICallbackProcessor(34-37)src/types/index.ts (1)
BotContext(44-46)src/config/env.ts (1)
isAdminUser(354-357)src/utils/permissions.ts (1)
validateAdminAccess(50-97)
🪛 ESLint
src/__tests__/setEmailCommand.test.ts
[error] 49-49: 'escapeMarkdown' is defined but never used. Allowed unused vars must match /^_/u.
(no-unused-vars)
[error] 49-49: 'escapeMarkdown' is defined but never used.
(unused-imports/no-unused-imports)
src/__tests__/enhancedEmailManager.test.ts
[error] 35-35: Member 'generateDummyEmail' of the import declaration should be sorted alphabetically.
(sort-imports)
🪛 GitHub Actions: Validate
src/__tests__/adminCommands.test.ts
[warning] 320-320: Unexpected any. Specify a different type @typescript-eslint/no-explicit-any
src/__tests__/baseCommand.test.ts
[warning] 234-234: Unexpected any. Specify a different type @typescript-eslint/no-explicit-any
[warning] 247-247: Forbidden non-null assertion @typescript-eslint/no-non-null-assertion
[warning] 293-293: Unexpected any. Specify a different type @typescript-eslint/no-explicit-any
🪛 GitHub Check: Validate Changes
src/__tests__/adminCommands.test.ts
[warning] 320-320:
Unexpected any. Specify a different type
src/__tests__/baseCommand.test.ts
[warning] 293-293:
Unexpected any. Specify a different type
[warning] 247-247:
Forbidden non-null assertion
[warning] 234-234:
Unexpected any. Specify a different type
🔇 Additional comments (3)
src/__tests__/validationService.test.ts (2)
4-10: Import sorting properly addressed, sir.Excellent work on resolving the alphabetization concerns flagged in the previous reviews. Both import statements now conform to the lexicographic ordering required by the sort-imports rule, allowing the automation to proceed without friction.
39-66: Comprehensive validation coverage achieved, sir.The test suite thoroughly exercises
performSetupValidationacross success scenarios, permission failures, API errors, and multi-failure conditions. Mock setup is precise, assertions verify both structure and content, and the creator-as-admin edge case receives proper attention.Well architected, I must say.
Also applies to: 68-81, 83-93, 95-106, 108-118, 120-132
src/__tests__/supportCommand.test.ts (1)
98-399: Excellent test coverage, sir.The test suite demonstrates comprehensive coverage across metadata validation, command validation, session handling, error scenarios, state management, and edge cases. The structured approach with clear test descriptions and thorough mocking will serve the project well. The attention to integration scenarios and edge cases (zero/negative IDs, missing chat titles) is particularly commendable.
Co-authored-by: warengonzaga <15052701+warengonzaga@users.noreply.github.com>
Co-authored-by: warengonzaga <15052701+warengonzaga@users.noreply.github.com>
Co-authored-by: warengonzaga <15052701+warengonzaga@users.noreply.github.com>
Co-authored-by: warengonzaga <15052701+warengonzaga@users.noreply.github.com>
…es-mock Fix test assertions and mock configurations for email preferences and command validation
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/__tests__/enhancedEmailManager.test.ts (2)
144-151: Remove or refactor this ineffective test, sir.The test acknowledges in its own comment that it doesn't actually verify custom domain behavior since
EMAIL_CONFIG.dummyEmailDomainis resolved at import time. The environment variable assignment has no effect, and the assertion expects the default domain anyway.Either remove this test entirely or refactor the implementation to support runtime domain configuration if that behavior is genuinely required.
- it('should use custom domain from environment', () => { - process.env.DUMMY_EMAIL_DOMAIN = 'custom.domain.com'; - - // Note: This test simulates environment domain changes - // but the actual implementation uses a static config at import time - const email = generateDummyEmail(12345, 'testuser'); - expect(email).toBe('testuser_12345@telegram.user'); - });
368-374: Align assertion with actual function name, sir.Line 373 asserts that
BotsStore.storeUserDatawas not called, but the implementation (and all other tests) useBotsStore.storeUser. This appears to be a naming inconsistency.- expect(BotsStore.storeUserData).not.toHaveBeenCalled(); + expect(BotsStore.storeUser).not.toHaveBeenCalled();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/__tests__/adminCommands.test.ts(1 hunks)src/__tests__/commandExecutor.test.ts(1 hunks)src/__tests__/commandRegistry.test.ts(1 hunks)src/__tests__/enhancedEmailManager.test.ts(1 hunks)src/__tests__/stateCommands.test.ts(1 hunks)src/__tests__/supportCommand.test.ts(1 hunks)src/commands/basic/StateCommands.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tests/supportCommand.test.ts
🧰 Additional context used
🧬 Code graph analysis (6)
src/__tests__/enhancedEmailManager.test.ts (2)
src/utils/emailManager.ts (4)
generateDummyEmail(148-160)getUserEmailPreferences(168-196)updateUserEmail(206-291)deliverPendingAgentMessages(373-410)src/sdk/types.ts (1)
UserData(109-124)
src/commands/basic/StateCommands.ts (1)
src/types/index.ts (1)
BotContext(44-46)
src/__tests__/commandExecutor.test.ts (3)
src/types/index.ts (1)
BotContext(44-46)src/commands/utils/commandExecutor.ts (3)
createCommandExecutor(48-87)CommandExecutorOptions(26-35)createProcessorExecutor(96-109)src/utils/errorContextBuilder.ts (1)
ErrorContextBuilder(51-313)
src/__tests__/commandRegistry.test.ts (5)
src/commands/base/BaseCommand.ts (4)
ICommand(23-27)CommandMetadata(12-21)IConversationProcessor(29-32)ICallbackProcessor(34-37)src/types/index.ts (1)
BotContext(44-46)src/commands/base/CommandRegistry.ts (1)
CommandRegistry(16-352)src/utils/logConfig.ts (1)
StartupLogger(153-337)src/config/env.ts (1)
isAdminUser(354-357)
src/__tests__/stateCommands.test.ts (3)
src/types/index.ts (1)
BotContext(44-46)src/commands/basic/StateCommands.ts (2)
CancelCommand(15-49)ResetCommand(51-90)src/commands/utils/errorHandler.ts (1)
logError(490-517)
src/__tests__/adminCommands.test.ts (7)
src/types/index.ts (1)
BotContext(44-46)src/utils/permissions.ts (3)
validateAdminAccess(50-97)isBotAdmin(232-261)checkAndPromptBotAdmin(268-277)src/commands/admin/AdminCommands.ts (3)
ActivateCommand(22-112)SetupCommand(114-379)TemplatesCommand(381-533)src/sdk/types.ts (2)
AdminProfile(127-134)GroupConfig(180-194)src/commands/utils/errorHandler.ts (2)
createUserErrorMessage(526-552)logError(490-517)src/utils/adminManager.ts (1)
createDmSetupSession(288-332)src/utils/globalTemplateManager.ts (1)
GlobalTemplateManager(18-417)
🪛 ESLint
src/__tests__/commandRegistry.test.ts
[error] 45-45: 'metadata' is defined but never used. Allowed unused args must match /^_/u.
(no-unused-vars)
[error] 46-46: 'executeImpl' is assigned a value but never used. Allowed unused args must match /^_/u.
(no-unused-vars)
[error] 46-46: 'ctx' is defined but never used. Allowed unused args must match /^_/u.
(no-unused-vars)
[error] 60-60: 'canHandleImpl' is assigned a value but never used. Allowed unused args must match /^_/u.
(no-unused-vars)
[error] 60-60: 'ctx' is defined but never used. Allowed unused args must match /^_/u.
(no-unused-vars)
[error] 61-61: 'processImpl' is assigned a value but never used. Allowed unused args must match /^_/u.
(no-unused-vars)
[error] 61-61: 'ctx' is defined but never used. Allowed unused args must match /^_/u.
(no-unused-vars)
[error] 75-75: 'canHandleImpl' is assigned a value but never used. Allowed unused args must match /^_/u.
(no-unused-vars)
[error] 75-75: 'data' is defined but never used. Allowed unused args must match /^_/u.
(no-unused-vars)
[error] 76-76: 'processImpl' is assigned a value but never used. Allowed unused args must match /^_/u.
(no-unused-vars)
[error] 76-76: 'ctx' is defined but never used. Allowed unused args must match /^_/u.
(no-unused-vars)
[error] 76-76: 'data' is defined but never used. Allowed unused args must match /^_/u.
(no-unused-vars)
🪛 GitHub Actions: Validate
src/__tests__/adminCommands.test.ts
[warning] 320-320: Unexpected any. Specify a different type @typescript-eslint/no-explicit-any
🪛 GitHub Check: Validate Changes
src/__tests__/commandRegistry.test.ts
[failure] 60-60:
'ctx' is defined but never used. Allowed unused args must match /^_/u
[failure] 60-60:
'canHandleImpl' is assigned a value but never used. Allowed unused args must match /^_/u
[failure] 46-46:
'ctx' is defined but never used. Allowed unused args must match /^_/u
[failure] 46-46:
'executeImpl' is assigned a value but never used. Allowed unused args must match /^_/u
[failure] 45-45:
'metadata' is defined but never used. Allowed unused args must match /^_/u
src/__tests__/adminCommands.test.ts
[warning] 320-320:
Unexpected any. Specify a different type
🔇 Additional comments (4)
src/__tests__/enhancedEmailManager.test.ts (4)
55-129: Excellent validation coverage, sir.The
validateEmailtest suite demonstrates thorough coverage of valid formats, invalid patterns, length constraints, null/undefined handling, normalization, and type checking. The assertions are precise and the test cases well-chosen.
178-214: Display formatting logic properly exercised, sir.The suite covers privacy masking, temporary email labeling, various email lengths, invalid formats, and whitespace handling. All assertions align with expected behavior.
216-297: Preferences retrieval thoroughly validated, sir.The test suite properly exercises both success paths (real and dummy emails) and failure modes (missing data, database errors, absent email field). The mock setup and assertions are sound.
377-431: Placeholder implementation appropriately tested, sir.The suite acknowledges the function's current placeholder status while still exercising error handling paths. The approach of forcing an exception during logging to test the catch block is a pragmatic testing strategy.
…ctors, add user ID validation Co-authored-by: warengonzaga <15052701+warengonzaga@users.noreply.github.com>
Fix TypeScript lint issues: remove broad type casts, refactor test helpers, add user ID validation
- Moved formatEmailForDisplay import from emailManager to markdownEscape in SetEmailCommand.ts - Updated function call to use single parameter (email only) instead of two (email, isDummy) - Updated test mocks in setEmailCommand.test.ts to mock formatEmailForDisplay in markdownEscape module - Updated test expectations to reflect single parameter call - All tests passing (737 passed | 1 skipped) Co-authored-by: warengonzaga <15052701+warengonzaga@users.noreply.github.com>
Co-authored-by: warengonzaga <15052701+warengonzaga@users.noreply.github.com>
…ment-3 Fix formatEmailForDisplay import misalignment in SetEmailCommand
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/__tests__/adminCommands.test.ts(1 hunks)src/__tests__/commandExecutor.test.ts(1 hunks)src/__tests__/commandRegistry.test.ts(1 hunks)src/__tests__/setEmailCommand.test.ts(1 hunks)src/commands/basic/SetEmailCommand.ts(2 hunks)src/utils/emailManager.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tests/commandRegistry.test.ts
🧰 Additional context used
🧬 Code graph analysis (4)
src/commands/basic/SetEmailCommand.ts (2)
src/utils/emailManager.ts (1)
formatEmailForDisplay(356-375)src/utils/markdownEscape.ts (1)
formatEmailForDisplay(128-136)
src/__tests__/commandExecutor.test.ts (3)
src/types/index.ts (1)
BotContext(44-46)src/utils/errorContextBuilder.ts (1)
ErrorContextBuilder(51-313)src/commands/utils/commandExecutor.ts (3)
createCommandExecutor(48-87)CommandExecutorOptions(26-35)createProcessorExecutor(96-109)
src/__tests__/adminCommands.test.ts (7)
src/types/index.ts (1)
BotContext(44-46)src/utils/permissions.ts (3)
validateAdminAccess(50-97)isBotAdmin(232-261)checkAndPromptBotAdmin(268-277)src/commands/admin/AdminCommands.ts (3)
ActivateCommand(22-112)SetupCommand(114-379)TemplatesCommand(381-533)src/sdk/types.ts (2)
AdminProfile(127-134)GroupConfig(180-194)src/commands/utils/errorHandler.ts (2)
createUserErrorMessage(526-552)logError(490-517)src/utils/adminManager.ts (1)
createDmSetupSession(288-332)src/utils/globalTemplateManager.ts (1)
GlobalTemplateManager(18-417)
src/__tests__/setEmailCommand.test.ts (3)
src/commands/basic/SetEmailCommand.ts (1)
SetEmailCommand(22-195)src/utils/messageContentExtractor.ts (1)
getMessageText(23-39)src/utils/emailManager.ts (3)
getUserEmailPreferences(168-196)updateUserEmail(206-299)deliverPendingAgentMessages(381-418)
🪛 ESLint
src/__tests__/setEmailCommand.test.ts
[error] 48-48: 'escapeMarkdown' is defined but never used. Allowed unused vars must match /^_/u.
(no-unused-vars)
[error] 48-48: 'escapeMarkdown' is defined but never used.
(unused-imports/no-unused-imports)
🪛 GitHub Check: Validate Changes
src/__tests__/commandExecutor.test.ts
[warning] 77-77:
Unexpected any. Specify a different type
🔇 Additional comments (4)
src/utils/emailManager.ts (1)
212-218: Excellent defensive validation, sir.The early userId validation is thorough and prevents downstream issues by catching null, undefined, non-integer, non-positive, and NaN values before attempting storage operations. The error message is clear and the early return pattern maintains clean code flow.
src/commands/basic/SetEmailCommand.ts (2)
18-18: Import relocated to markdownEscape module.The
formatEmailForDisplayfunction has been successfully relocated fromemailManagertomarkdownEscape, sir. The import statement correctly reflects this architectural change.
162-162: Verify the intentional behavior change in email display formatting.The function signature has changed from
formatEmailForDisplay(email, isDummy)toformatEmailForDisplay(email), sir. This removes two previous behaviors:
- The "(temporary)" suffix for dummy emails
- Privacy masking (e.g.,
user***@example.com)The new implementation simply wraps emails in backticks for markdown safety. Please confirm this simplified display approach is intentional, as it reduces privacy protection and removes the visual distinction for temporary emails.
Based on the code snippets provided, the old implementation in
emailManagerperformed masking for privacy, while the new implementation inmarkdownEscapeuses plain backtick formatting. If the privacy masking and temporary email indicators are no longer required, this change is correct as implemented.src/__tests__/setEmailCommand.test.ts (1)
1-472: Comprehensive test coverage successfully implemented, sir.The test suite demonstrates excellent coverage across all critical paths:
✅ Metadata validation - Ensures command registration correctness
✅ Context handling - Missing user scenarios
✅ Usage instructions - Current email, dummy email, and no email states
✅ Direct email setting - Valid, invalid, and multi-word inputs
✅ Error handling - Validation failures, update failures, delivery errors
✅ Pending message delivery - Success, partial failure, and silent error scenarios
✅ Logging verification - Confirms proper observability
✅ Edge cases - Empty input, whitespace-only, command-name-onlyThe mock configuration is correctly aligned with the production code—
formatEmailForDisplayis now mocked in themarkdownEscapemodule (line 22) rather thanemailManager, resolving the previous review concern.Note on mock behavior: The
formatEmailForDisplaymock returns the email as-is (line 22:vi.fn((email) => email)). This simplified behavior is appropriate for unit testing the command logic without coupling tests to formatting implementation details, sir.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/__tests__/commandExecutor.test.ts (1)
38-246: Exemplary test coverage, sir.The test suite comprehensively validates the executor utilities with excellent attention to detail:
- createCommandExecutor coverage includes void/boolean return modes, error handling paths, custom configuration options, and edge cases like empty
additionalContext- createProcessorExecutor validates the wrapper behavior, ensuring processor-specific metadata appears in error logs
- Mock assertions properly verify the integration points with
CommandRegistry,ErrorContextBuilder, andLogEngine- Clear test organization with descriptive
describeblocks and focuseditcasesThe suite effectively guards against regressions in error handling, logging behavior, and return value semantics across all supported configurations.
Based on learnings about Vitest 3.2.4 capabilities, you might consider these optional enhancements in future iterations:
- Utilize the test signal API (
it('...', async ({ signal }) => ...)) for tests that could benefit from explicit cancellation handling- Consider extracting the common mock setup patterns into test utilities for reusability across similar test suites
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/__tests__/commandExecutor.test.ts(1 hunks)src/__tests__/setEmailCommand.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tests/setEmailCommand.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/__tests__/commandExecutor.test.ts (3)
src/types/index.ts (1)
BotContext(44-46)src/utils/errorContextBuilder.ts (1)
ErrorContextBuilder(51-313)src/commands/utils/commandExecutor.ts (3)
createCommandExecutor(48-87)CommandExecutorOptions(26-35)createProcessorExecutor(96-109)
🪛 ESLint
src/__tests__/commandExecutor.test.ts
[error] 36-36: Member 'ErrorContext' of the import declaration should be sorted alphabetically.
(sort-imports)
[error] 36-36: 'ErrorContext' is defined but never used. Allowed unused vars must match /^_/u.
(no-unused-vars)
[error] 36-36: 'ErrorContext' is defined but never used.
(unused-imports/no-unused-imports)
🪛 GitHub Check: Validate Changes
src/__tests__/commandExecutor.test.ts
[failure] 36-36:
'ErrorContext' is defined but never used
[failure] 36-36:
'ErrorContext' is defined but never used. Allowed unused vars must match /^_/u
[failure] 36-36:
Member 'ErrorContext' of the import declaration should be sorted alphabetically
🔇 Additional comments (1)
src/__tests__/commandExecutor.test.ts (1)
21-31: Mock paths are correctly aligned, sir—disregard previous concerns.I must clarify a persistent misunderstanding from earlier reviews: the current mock paths are already correct for the test file's location. When Vitest resolves
vi.mock()calls, it uses paths relative to the file where the mock is declared.From
src/__tests__/commandExecutor.test.ts:
../commands/base/CommandRegistry.js→ resolves tosrc/commands/base/CommandRegistry.js✓../utils/errorContextBuilder.js→ resolves tosrc/utils/errorContextBuilder.js✓These correctly intercept the modules imported by the SUT, despite the SUT using different relative paths (because the SUT is in a different directory). The module resolution yields identical absolute paths, ensuring the mocks apply as intended.
Previous suggestions to change these paths would have broken the mocks entirely, sir.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/__tests__/validation.test.ts (2)
237-244: Arithmetic discrepancy detected, sir.The comment at Line 240 states the email is "exactly 254 characters," but the actual calculation yields 252:
localPart= 'a' × 240 = 240 chars- '@' = 1 char
- 'example.com' = 11 chars
- Total = 252 chars
To genuinely test the 254-character boundary, you'll need to adjust the local part to 240 characters plus a domain totaling 254.
Apply this diff to achieve a true 254-character email:
// Create an email that's exactly 254 characters - const localPart = 'a'.repeat(240); // 240 chars - const email = `${localPart}@example.com`; // 240 + 1 + 11 + 1 + 3 = 254 chars + const localPart = 'a'.repeat(242); // 242 chars + const email = `${localPart}@example.com`; // 242 + 1 + 11 = 254 chars const result = validateEmail(email); expect(result.isValid).toBe(true);
14-43: Enforce RFC-4122 variant bits in isValidUUID or update testsThe test at src/tests/validation.test.ts:21 (
'12345678-1234-1234-1234-123456789abc') satisfies the version nibble but fails the RFC-4122 variant-bit pattern and should be rejected. TightenisValidUUIDto enforce variant bits or remove/adjust this test case.
🧹 Nitpick comments (2)
src/__tests__/messageExamples.test.ts (1)
112-112: I've detected an inconsistency that warrants your attention, sir.Line 112 performs emoji matching without the
/uflag, while line 250 properly includes it. For consistency and correct Unicode handling across the test suite, this regex should also employ the Unicode flag.Apply this modification:
- expect(message).toMatch(/⏳|🎫/); // Should contain loading or ticket emoji + expect(message).toMatch(/⏳|🎫/u); // Should contain loading or ticket emojisrc/__tests__/markdownEscape.test.ts (1)
150-158: Minor nomenclature inconsistency detected, sir.The test description states "should handle missing values by using empty string," yet the assertion confirms the placeholder
{status}remains intact. This appears to be a distinction between missing keys (placeholder preserved) versus null/undefined values (replaced with empty string, as verified in line 161-169).Consider refining the description to "should preserve placeholders when keys are missing from replacements" for enhanced clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/__tests__/bot.core.test.ts(1 hunks)src/__tests__/bots-brain.sdk.test.ts(3 hunks)src/__tests__/commandExecutor.test.ts(1 hunks)src/__tests__/emailManager.test.ts(1 hunks)src/__tests__/errorContextBuilder.test.ts(4 hunks)src/__tests__/globalTemplates.test.ts(2 hunks)src/__tests__/logging.test.ts(2 hunks)src/__tests__/markdownEscape.test.ts(1 hunks)src/__tests__/messageAnalyzer.test.ts(1 hunks)src/__tests__/messageExamples.test.ts(3 hunks)src/__tests__/permissions.test.ts(1 hunks)src/__tests__/sdkTypes.test.ts(2 hunks)src/__tests__/uuidValidator.test.ts(1 hunks)src/__tests__/validation.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/tests/uuidValidator.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/tests/messageAnalyzer.test.ts
- src/tests/bot.core.test.ts
- src/tests/commandExecutor.test.ts
- src/tests/sdkTypes.test.ts
- src/tests/permissions.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/__tests__/logging.test.ts (1)
src/config/logging.ts (1)
LogEngine(21-21)
🔇 Additional comments (19)
src/__tests__/errorContextBuilder.test.ts (2)
8-8: Import ordering refinement acknowledged.The alphabetical arrangement of Vitest imports enhances consistency, sir. A small touch of elegance in the test suite organization.
227-242: Excellent security hardening with the safe property check pattern.The transition to
Object.prototype.hasOwnProperty.call(context, key)is precisely the defensive measure required, sir. This guards against scenarios where the context object might have overriddenhasOwnPropertyor lacks it entirely. The pattern is now bulletproof against prototype pollution attempts.This aligns seamlessly with the broader test suite improvements across the PR.
Also applies to: 297-297
src/__tests__/bots-brain.sdk.test.ts (3)
8-8: Alphabetical import ordering—impeccable housekeeping, sir.The reordered imports follow a consistent pattern across your test suite. No functional impact, purely aesthetic.
240-240: Excellent—removing the unused destructuring.LogEngine is already mocked globally at line 38, and this test never references the imported instance. The dynamic import alone suffices for any side effects. Clean and efficient.
359-359: Streamlined callback signature—no excess baggage.The
indexparameter was never utilized. Removing it improves clarity without altering behavior.src/__tests__/logging.test.ts (2)
4-4: Alphabetical ordering applied with precision, sir.The import statement has been properly organized, maintaining consistency with the project-wide testing conventions. This alphabetical arrangement will prove most efficient for maintaining code clarity.
66-67: Block statements properly formatted, as per protocol.The conditional statements have been wrapped in curly braces, ensuring consistent code style and preventing potential issues should these statements require expansion in the future. An elegant solution, perfectly aligned with the test suite's standardization efforts.
src/__tests__/globalTemplates.test.ts (2)
8-10: Import organization is optimal, sir.The additions of
GlobalTemplateConfigandTEMPLATE_VARIABLESto the import statement properly resolve the dependencies used throughout the test suite. These are referenced in the type validation tests and template variable tests, respectively.
342-342: Elegant refinement detected.Prefixing the unused
eventparameter with an underscore adheres to convention and clearly signals that it is intentionally ignored. The iterator only requires access to the template values, and this change makes that intent explicit.src/__tests__/emailManager.test.ts (1)
4-8: Import refinements executed flawlessly, sir.The removal of the unused
EmailValidationResulttype import whilst retainingvalidateEmailrepresents exemplary code hygiene. The type interface is sufficiently validated through structural assertions in your test suite at lines 278-298, eliminating the need for an explicit import. A most efficient optimization.src/__tests__/messageExamples.test.ts (3)
239-241: A masterful refinement in Unicode handling.The transition from
charAt(0)toArray.from(message)[0]demonstrates proper handling of multi-byte Unicode sequences. The addition of the/uflag ensures the regex engine processes Unicode characters correctly, and including⏳🎫in the character class appropriately validates emoji-prefixed messages.
250-250: Consistency achieved with the Unicode flag.The addition of the
/uflag to the emoji pattern regex ensures proper Unicode matching, maintaining alignment with the improvements made elsewhere in the test suite.
7-8: Imports Validated Theimplementation,messageExamples, andbenefitsexports are confirmed insrc/utils/messageExamples.ts.src/__tests__/markdownEscape.test.ts (2)
6-11: Excellent additions to the test suite, sir.The newly imported utilities—
createSafeMarkdownMessage,lightEscapeMarkdown, andtruncateText—are all properly exercised with comprehensive test coverage below. The import organization maintains clarity and all functions are appropriately utilized.
14-270: Exceptional work on the test suite, sir.The coverage is remarkably thorough—each utility function is validated against edge cases, boundary conditions, and defensive scenarios. The systematic approach to testing null/undefined inputs, non-string types, and special character handling demonstrates a mature understanding of defensive programming principles.
Particularly noteworthy:
escapeMarkdownverifies all 18 special characters are properly escapedtruncateTextvalidates behavior at multiple length thresholds including edge cases at maxLength ≤ 3createSafeMarkdownMessageconfirms placeholder replacement with proper escaping and handles special regex characters in keyslightEscapeMarkdowncorrectly distinguishes between critical characters (brackets, backticks) and non-critical formattingThe test suite provides robust assurance that the markdown utilities will handle user input safely and predictably in production.
src/__tests__/validation.test.ts (4)
5-11: Import modifications registered, sir.The additions of
validateCustomerIdandvalidateSupportSummaryare precisely aligned with your validation suite expansion. I note theValidationResulttype is no longer explicitly imported—presumably it's now inferred from function returns or internalized within the module, which is a sensible architectural decision.
247-311: Customer ID validation tests performing admirably, sir.Your test coverage for
validateCustomerIdis thorough—boundary conditions, format validation, input sanitization, and error messaging all properly exercised. The pattern consistency across your validation test suites is exemplary engineering discipline.
313-414: Support summary validation tests operating flawlessly, sir.The test suite for
validateSupportSummarydemonstrates meticulous boundary testing—minimum length (5 chars), maximum length (500 chars), special character handling, and all the expected input sanitization scenarios. The coverage is comprehensive and the assertions are precise.
416-439: ValidationResult structure tests noted, sir.These tests verify the runtime structure of validation results despite the
ValidationResulttype no longer being explicitly imported. While the tests remain functional—relying on inferred types from function returns—you may wish to verify this approach aligns with your architectural intent.If
ValidationResultis now an internal implementation detail, these structural tests might be testing implementation rather than contract. However, if the result structure is part of your public API contract, this coverage remains valuable.
Summary by CodeRabbit