feat: webhooks implementation & queue system (BAL-2672) #3021
Conversation
|
WalkthroughThis update introduces a new Redis service with Docker Compose and adds its management commands to the package scripts. Several webhook-related modules and services have been refactored or removed, replacing direct HTTP calls with a centralized WebhooksService that integrates with a BullMQ-based queuing system. The environmental configuration has been extended to include Redis and queue settings, and the Incoming Webhooks module has been introduced for AML event processing. Minor naming corrections and test adjustments round out the changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Webhook Caller
participant WHService as WebhooksService
participant Queue as RetryableQueue
participant External as External Webhook Endpoint
Caller->>WHService: invokeWebhook(payload)
WHService->>Queue: Enqueue webhook job
Queue->>Queue: Process job with retries/backoff
Queue->>External: Send webhook request
External-->>Queue: Acknowledge response
Queue-->>WHService: Report job completion
sequenceDiagram
participant Client as Incoming Request
participant Controller as IncomingWebhooksController
participant Service as IncomingWebhooksService
participant EndUser as EndUserRepository
participant Workflow as WorkflowService
Client->>Controller: POST AML webhook data
Controller->>Service: handleIndividualAmlHit({ endUserId, data })
Service->>EndUser: Retrieve end-user details
Service->>Workflow: Initiate/update workflow runtime
Workflow-->>Service: Workflow created/updated
Service-->>Controller: Return success response
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
r4zendev
changed the title
![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
🔭 Outside diff range comments (3)
services/workflows-service/src/common/http-service/http-config.service.ts (1)
15-23: Align timeout values across functions.The timeout value in
createHttpOptions(5000ms) is different from the one ininitHttpModule(10000ms). This inconsistency could lead to confusion.Consider applying this diff to align the timeout values:
createHttpOptions(): HttpModuleOptions { return { - timeout: this.configService.get('HTTP_TIMEOUT_IN_MS', 5000), + timeout: this.configService.get('HTTP_TIMEOUT_IN_MS', 10_000), maxRedirects: this.configService.get('HTTP_MAX_REDIRECTS', 10), validateStatus: (status: number) => { return status < 500; // Resolve only if the status code is less than 500 }, }; }services/workflows-service/src/webhooks-incoming/webhooks-incoming.controller.ts (1)
34-56: Improve error handling and input validation.The function has several areas for improvement:
- Only individual entity type is handled, but business type is defined.
- Error logging could be more detailed.
- Input validation could be strengthened.
Consider applying this diff:
async amlHook( @common.Param() { entityType }: AmlWebhookInput, @common.Body() { data }: IndividualAmlWebhookInput, ) { + // Validate entity type + if (!Object.values(EntityType).includes(entityType as any)) { + const error = `Invalid entity type: ${entityType}`; + this.logger.error(error); + throw new BadRequestException(error); + } + if (!data?.endUserId) { - throw new BadRequestException('Missing endUserId'); + const error = 'Missing endUserId in request data'; + this.logger.error(error, { entityType, data }); + throw new BadRequestException(error); } try { if (entityType === EntityType.INDIVIDUAL) { return await this.incomingWebhooksService.handleIndividualAmlHit({ endUserId: data.endUserId, data, }); + } else if (entityType === EntityType.BUSINESS) { + // TODO: Implement business AML hit handling + const error = 'Business AML hit handling not implemented'; + this.logger.error(error); + throw new BadRequestException(error); } - this.logger.error(`Unknown entity type: ${entityType}`); - throw new BadRequestException('Unknown entity type'); + // This should never be reached due to entity type validation above + const error = `Unhandled entity type: ${entityType}`; + this.logger.error(error); + throw new BadRequestException(error); } catch (error) { - this.logger.error('amlHook::', { entityType, data, error }); + this.logger.error('Error processing AML webhook:', { + entityType, + endUserId: data?.endUserId, + error: error instanceof Error ? error.message : error, + }); throw error; } }services/workflows-service/package.json (1)
1-176: Lockfile Update Required to Resolve Pipeline Failure.
The CI pipeline error ("Cannot install with 'frozen-lockfile' because pnpm-lock.yaml is not up to date with package.json") indicates that the lockfile must be updated. Please run your package manager (e.g.,pnpm install) to synchronizepnpm-lock.yamlwith the current state ofpackage.json.🧰 Tools
🪛 GitHub Actions: CI
[error] 1-1: Cannot install with 'frozen-lockfile' because pnpm-lock.yaml is not up to date with package.json.
🧹 Nitpick comments (18)
services/workflows-service/src/webhooks/webhooks.module.ts (1)
14-24: Refactor to support registering queues dynamicallyThe new provider for the Bull Board instance initializes with an empty array of queues. If you plan to manage multiple queues, consider a dynamic queue registration approach (e.g., retrieving them from environment variables or a centralized registry).
services/workflows-service/src/webhooks/retryable-queue.ts (2)
9-44: Refine type usage for job ID and dataWhen adding the job to the DLQ, casting
job.idandjob.datatoanycould mask type mismatches. Use narrower type assertions or avoid casting altogether:- (job.id ?? `dlq-${Date.now()}`) as any, - { ...job.data, error: err } as any, + (job.id ?? `dlq-${Date.now()}`), + { ...job.data, error: err },
46-53: Clarify the return typeThe method returns
Promise<void[]>, which can be confusing. Consider returningPromise<Array<unknown>>or adjusting the signature to reflect actual usage for improved readability and linter compatibility.-async shutdown(): Promise<void[]> { +async shutdown(): Promise<Array<unknown>> {🧰 Tools
🪛 Biome (1.9.4)
[error] 46-46: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
services/workflows-service/src/webhooks-incoming/webhooks-incoming.service.ts (1)
35-125: Consider robust error handling for external calls.Currently, if any of the external service calls (e.g.,
endUserService.updateById,workflowService.createOrUpdateWorkflowRuntime) fail, the method will throw and not handle partial failures gracefully. Enclosing these calls in a try/catch block or introducing a transactional approach could improve resilience and clarity when multiple external services are involved.services/workflows-service/src/webhooks/webhooks.service.ts (2)
16-21: Potential duplication of thealertWebhookFailureutility.This function is defined in multiple places (e.g.,
webhooks-incoming.service.ts). If it is intended for shared use, consider extracting it into a shared utility file to follow DRY principles.
122-129: Assess possibility of duplicate webhook deliveries.When the queue is unavailable, the fallback calls Axios directly. If the queue later recovers, there’s a risk of re-adding the same job or re-sending an identical request. To avoid duplicates, consider adding idempotency checks or unique job constraints.
services/workflows-service/src/events/workflow-completed-webhook-caller.ts (1)
90-173: Refactor duplicated webhook payload creation logic.There are two nearly identical payload-block sections (lines 101-128 and 136-172). Consider extracting the common logic into a helper function to improve maintainability and reduce redundancy.
services/workflows-service/src/events/document-changed-webhook-caller.ts (2)
33-33: Constructor injection forwebhooksServiceis a clean design choice.
InjectingwebhooksServicerather than handling HTTP calls directly promotes better separation of concerns. Just ensure any additional error handling (if needed) is implemented inwebhooksService.
196-201: Consider adding error handling or retry logic.
WhilewebhooksService.invokeWebhookmay handle certain errors internally, it might be beneficial to catch and log errors here to enable better traceability at the caller level. If multiple webhooks fail, you can isolate each failure for targeted analysis.services/workflows-service/src/webhooks/types/webhook.ts (3)
3-15: Consider strengthening type definitions.The type definitions could be improved for better type safety:
datafield usesRecord<string, any>which is too permissive.environmentcould be a union of specific environment strings.Consider applying this diff:
- environment: string | undefined; - data: Record<string, any>; + environment: 'development' | 'staging' | 'production' | undefined; + data: Record<string, unknown>;
17-30: Extract assignee type to a reusable interface.The
assigneetype inworkflow.context.document.changedcould be extracted to a reusable interface for better maintainability.Consider applying this diff:
+export interface Assignee { + id: string; + firstName: string; + lastName: string; + email: string; +} + export type OutgoingWebhookPayloads = { 'workflow.completed': BaseOutgoingWebhookPayload & { eventName: 'workflow.completed'; }; 'workflow.state.changed': BaseOutgoingWebhookPayload & { eventName: 'workflow.state.changed'; state: string | null; }; 'workflow.context.document.changed': BaseOutgoingWebhookPayload & { eventName: 'workflow.context.document.changed'; - assignee: { id: string; firstName: string; lastName: string; email: string } | null; + assignee: Assignee | null; assignedAt: Date | null; }; };
32-39: Strengthen method type with HTTP method literals.The
methodfield could be more specific using HTTP method literals.Consider applying this diff:
export type OutgoingWebhookJobData = { url: string; - method: string; + method: 'GET' | 'POST' | 'PUT' | 'PATCH' | 'DELETE'; headers?: RawAxiosRequestHeaders; data: OutgoingWebhookPayloads[keyof OutgoingWebhookPayloads]; timeout?: number; secret?: string; };services/workflows-service/src/common/http-service/http-config.service.ts (2)
44-44: Update comment to reflect actual timeout value.The comment mentions 3 seconds but the actual timeout is configurable and defaults to 10 seconds.
Consider applying this diff:
- // 👇️ aborts request after 3 seconds (connectivity issues) + // 👇️ aborts request after configured timeout (default: 10 seconds) for connectivity issues
26-52: Extract validateStatus function to avoid duplication.The
validateStatusfunction is duplicated betweencreateHttpOptionsandinitHttpModule.Consider applying this diff:
+const validateHttpStatus = (status: number) => status < 500; + export class HttpConfigService implements HttpModuleOptionsFactory { createHttpOptions(): HttpModuleOptions { return { timeout: this.configService.get('HTTP_TIMEOUT_IN_MS', 10_000), maxRedirects: this.configService.get('HTTP_MAX_REDIRECTS', 10), - validateStatus: (status: number) => { - return status < 500; - }, + validateStatus: validateHttpStatus, }; } } export const initHttpModule = () => HttpModule.registerAsync({ // ... useFactory: (configService: ConfigService) => { // ... return { timeout, maxRedirects: configService.get('HTTP_MAX_REDIRECTS', 10), retryAttempts: configService.get('HTTP_RETRY_ATTEMPTS', 0), signal: newAbortSignal(timeout), - validateStatus: (status: number) => { - return status < 500; - }, + validateStatus: validateHttpStatus, }; }, });services/workflows-service/src/alert/alert.module.ts (1)
65-104: Improve Axios retry configuration.The Axios retry configuration could be improved:
- Retry delay value is hardcoded.
- Retry condition logic could be simplified.
Consider applying this diff:
+const RETRY_DELAY_MS = 1500; +const RETRYABLE_STATUS_CODES = [429, 500, 501]; + export class AlertModule { onModuleInit() { const _axios = this.httpService.axiosRef; interceptAxiosRequests(this.logger, _axios, AlertModule.name); axiosRetry(_axios, { retries: 3, - retryDelay: (...arg) => axiosRetry.exponentialDelay(...arg, 1500), + retryDelay: (...arg) => axiosRetry.exponentialDelay(...arg, RETRY_DELAY_MS), retryCondition(error: unknown) { if (error && isAxiosError(error)) { if (error.response) { - switch (error.response.status) { - //retry only if status: 429, 500, 501 - case 429: - case 500: - case 501: - return true; - default: - return false; - } + return RETRYABLE_STATUS_CODES.includes(error.response.status); } else if (getHttpStatusFromAxiosError(error.code) === HttpStatus.INTERNAL_SERVER_ERROR) { return false; } } return true; },services/workflows-service/src/env.ts (1)
97-103: LGTM! Redis configuration looks good.The Redis configuration includes all necessary parameters with appropriate validation:
- Required host and port settings
- Optional password for flexibility
- Queue system toggle for controlled rollout
Consider adding these environment variables to your deployment documentation and CI/CD pipeline configuration.
services/workflows-service/src/workflow/workflow.service.unit.test.ts (1)
179-185: Consider creating a mock WebhooksService instead of using empty object.Using an empty object cast to
anyreduces type safety and makes the test less maintainable. Consider creating a proper mock for the WebhooksService.- {} as any, + { + sendWebhook: jest.fn().mockResolvedValue({ success: true }), + // Add other methods as needed + } as jest.Mocked<WebhooksService>,services/workflows-service/docker-compose.redis.yml (1)
17-23: Volume and Network Declaration Follow Best Practices.
The volumeredis-datais explicitly set withdriver: local, which aligns with the retrieved learning for local development. Similarly, the custom networkapp-networkuses thebridgedriver as expected. Consider adding inline comments for clarity in the future if additional customizations are planned.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (25)
services/workflows-service/docker-compose.redis.yml(1 hunks)services/workflows-service/package.json(5 hunks)services/workflows-service/prisma/data-migrations(1 hunks)services/workflows-service/src/alert/alert.module.ts(3 hunks)services/workflows-service/src/alert/webhook-manager/types.ts(0 hunks)services/workflows-service/src/alert/webhook-manager/webhook-event-emitter.service.ts(0 hunks)services/workflows-service/src/alert/webhook-manager/webhook-http.service.ts(0 hunks)services/workflows-service/src/alert/webhook-manager/webhook-manager.service.ts(0 hunks)services/workflows-service/src/app.module.ts(3 hunks)services/workflows-service/src/business-report/business-report.controller.external.ts(1 hunks)services/workflows-service/src/common/http-service/http-config.service.ts(1 hunks)services/workflows-service/src/env.ts(1 hunks)services/workflows-service/src/events/document-changed-webhook-caller.ts(3 hunks)services/workflows-service/src/events/workflow-completed-webhook-caller.ts(3 hunks)services/workflows-service/src/events/workflow-state-changed-webhook-caller.ts(2 hunks)services/workflows-service/src/webhooks-incoming/webhooks-incoming.controller.ts(3 hunks)services/workflows-service/src/webhooks-incoming/webhooks-incoming.module.ts(1 hunks)services/workflows-service/src/webhooks-incoming/webhooks-incoming.service.ts(1 hunks)services/workflows-service/src/webhooks/retryable-queue.ts(1 hunks)services/workflows-service/src/webhooks/types/bull.ts(1 hunks)services/workflows-service/src/webhooks/types/webhook.ts(1 hunks)services/workflows-service/src/webhooks/webhooks.module.ts(1 hunks)services/workflows-service/src/webhooks/webhooks.service.ts(1 hunks)services/workflows-service/src/workflow/workflow.module.ts(2 hunks)services/workflows-service/src/workflow/workflow.service.unit.test.ts(1 hunks)
💤 Files with no reviewable changes (4)
- services/workflows-service/src/alert/webhook-manager/webhook-http.service.ts
- services/workflows-service/src/alert/webhook-manager/webhook-manager.service.ts
- services/workflows-service/src/alert/webhook-manager/webhook-event-emitter.service.ts
- services/workflows-service/src/alert/webhook-manager/types.ts
✅ Files skipped from review due to trivial changes (2)
- services/workflows-service/prisma/data-migrations
- services/workflows-service/src/business-report/business-report.controller.external.ts
🧰 Additional context used
📓 Learnings (1)
services/workflows-service/docker-compose.redis.yml (1)
Learnt from: alonp99
PR: ballerine-io/ballerine#2735
File: services/workflows-service/docker-compose.redis.yml:17-23
Timestamp: 2024-11-12T05:57:16.983Z
Learning: For Redis volumes in local development, prefer using `driver: local` in Docker Compose configurations.
🪛 Biome (1.9.4)
services/workflows-service/src/webhooks/retryable-queue.ts
[error] 46-46: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🪛 GitHub Actions: CI
services/workflows-service/package.json
[error] 1-1: Cannot install with 'frozen-lockfile' because pnpm-lock.yaml is not up to date with package.json.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (25)
services/workflows-service/src/webhooks/webhooks.module.ts (4)
1-2: Logical integration of Bull BoardThe additional import statements from
@bull-board/apiand@bull-board/expresseffectively introduce the capability to manage and monitor Bull queues.
4-8: Clean reorganization of importsImports for
MiddlewareConsumer,AppLoggerModule,BULLBOARD_INSTANCE_INJECTION_TOKEN, andWebhooksServiceappear consistent with the newly introduced queuing architecture. Verify there are no remaining references to older or removed imports elsewhere in the codebase.Also applies to: 11-11
26-26: Exporting WebhooksServiceExposing
WebhooksServiceensures it can be leveraged by other modules. This aligns with NestJS best practices for modular design.
29-36: Confirm router path usageThe
configuremethod applies the Bull Board's router at/queues. Ensure no conflicts exist with other service routes, and verify alignment with the overall API prefix or gateway.services/workflows-service/src/webhooks/retryable-queue.ts (1)
1-8: Decoupled queue structureDefining distinct properties for the primary queue, worker, DLQ, and DLQ worker clarifies responsibilities for both normal and failed jobs.
services/workflows-service/src/events/workflow-state-changed-webhook-caller.ts (3)
10-10: Modularizing webhook logicImporting
WebhooksServiceconsolidates HTTP logic, promoting cleaner separation of concerns.
19-19: Dependency injection for WebhooksServiceReplacing direct HTTP calls with a dedicated service clarifies responsibilities and fosters testability.
86-93: Confirm robust error handlingCalling
this.webhooksService.invokeWebhookcentralizes webhook dispatch, but ensure any failures or retries are accessible here if needed for logging, alerts, or additional fallback logic.services/workflows-service/src/webhooks-incoming/webhooks-incoming.service.ts (3)
1-4: All imports appear valid.
14-21: Review Sentry logging scope for sensitive data.The captured Axios error objects might contain personally identifiable information (PII). Please verify that no sensitive data is included in these logs or consider redacting fields before sending them to Sentry.
23-33: Constructor dependency injection looks correct.services/workflows-service/src/events/document-changed-webhook-caller.ts (3)
7-7: Import usage looks good.
ImportingDefaultContextSchemaandgetDocumentIdfrom@ballerine/commonis consistent with the usage below where you retrieve document IDs.
14-14: NewWebhooksServiceimport aligns with modular webhook handling.
This replacement ofHttpServicewith a dedicatedWebhooksServiceimproves maintainability by centralizing all webhook-related logic in one service.
172-195: Validate payload for security or PII exposure.
Though marking this object asas constis helpful for type safety, confirm that no sensitive information is unnecessarily shared in the webhook payload, such as personally identifiable data beyond what’s intended.Would you like to run a codebase analysis script to scan for potential references to sensitive fields in the context data?
services/workflows-service/src/webhooks/types/bull.ts (1)
1-9: Bull Board integration is set up correctly.
Defining a clear injection token (BULLBOARD_INSTANCE_INJECTION_TOKEN) and typed interface (BullBoardInjectedInstance) makes the Bull Board instance more discoverable and maintainable. This approach follows NestJS best practices for dependency injection.services/workflows-service/src/webhooks-incoming/webhooks-incoming.module.ts (1)
1-24: New module structure looks solid.
Consolidating the imports (logger, customer, end-user, workflow, and webhooks modules) provides a coherent setup for the newIncomingWebhooksModule. Verify the path spelling forworkflow-defintionis correct and update if needed. Otherwise, this module comprehensively encapsulates its concerns and fosters clarity.services/workflows-service/src/workflow/workflow.module.ts (1)
49-49: LGTM! WebhooksModule integration is clean.The module is properly imported and positioned correctly in the imports array.
Also applies to: 58-58
services/workflows-service/src/app.module.ts (3)
35-35: LGTM! Import statements are properly fixed.The imports are correctly updated:
- Fixed typo in initHttpModule import
- Added IncomingWebhooksModule import
Also applies to: 43-43
47-47: LGTM! Zod import is now using named import.The import statement now correctly uses named import syntax for zod.
96-96: LGTM! IncomingWebhooksModule properly integrated.The module is correctly added to the imports array.
services/workflows-service/docker-compose.redis.yml (2)
1-1: Basic File Declaration is Correct.
The declared Compose version (3.8) is appropriate for the configuration.
2-16: Redis Service Definition Looks Solid.
The Redis service is configured with theredis:alpineimage, correct port mapping using${REDIS_PORT}, and persistent storage via theredis-datavolume. The command section correctly enables authentication using--requirepass ${REDIS_PASSWORD}and activates AOF persistence with--appendonly yes.services/workflows-service/package.json (3)
8-8: Setup Script Incorporates Redis Management.
The updated"setup"command now includes stopping and starting the Redis container (withnpm run docker:redis:downandnpm run docker:redis), ensuring that the Redis service is properly reset alongside the database. Verify that the sequence—stopping the service before starting a new instance—is as intended in all deployment environments.
35-36: New Redis Docker Commands Added.
The scripts"docker:redis"and"docker:redis:down"correctly reference the new Docker Compose file (docker-compose.redis.yml). The use of the--waitflag in the"docker:redis"command should help ensure that the container is fully initialized before further operations commence.
58-59: Queue and Webhooks Dependencies Introduced.
New dependencies for the queue and webhooks systems—@bull-board/api,@bull-board/express,bullmq, andioredis—have been added. These align with the overall PR objectives to implement a robust queue system and webhooks service. Please double-check compatibility with existing modules and ensure that version constraints work well with your overall dependency ecosystem.Also applies to: 88-88, 101-101
r4zendev
force-pushed
the
bullmq-webhooks-2
branch
from
0dd7015 to
e1adfc2
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
services/workflows-service/src/common/http-service/http-config.service.ts (1)
15-23: Align timeout defaults with initHttpModule or consider removing unused class.The
createHttpOptionsmethod uses a different default timeout (5000ms) compared toinitHttpModule(10000ms). Additionally, the class appears to be unused as it's commented out in the module registration.Consider either:
- Removing the class if it's no longer needed, or
- Aligning the configuration with
initHttpModuleand reusing the logic:createHttpOptions(): HttpModuleOptions { return { - timeout: this.configService.get('HTTP_TIMEOUT_IN_MS', 5000), + timeout: this.configService.get('HTTP_TIMEOUT_IN_MS', 10_000), maxRedirects: this.configService.get('HTTP_MAX_REDIRECTS', 10), + retryAttempts: this.configService.get('HTTP_RETRY_ATTEMPTS', 0), validateStatus: (status: number) => { return status < 500; }, }; }services/workflows-service/src/events/workflow-completed-webhook-caller.ts (1)
189-190: Fix TypeScript type error in tags handling.Instead of using
@ts-expect-error, consider properly typing the tags array.- // @ts-expect-error - error from Prisma types fix - workflowRuntimeData.tags?.some((tag: string) => { + (workflowRuntimeData.tags as string[] | null)?.some((tag) => {
♻️ Duplicate comments (1)
services/workflows-service/src/webhooks/webhooks.service.ts (1)
134-149:⚠️ Potential issuePotential sensitive data exposure in error logs.
The error logging includes potentially sensitive data from the webhook payload.
-const { id, state, entityId, correlationId, runtimeData } = data as Record<string, any>; +const { id, state } = data as Record<string, any>; this.logger.error('Failed to send webhook', { id, message: isAxiosError(error) ? error.response?.data ?? error.message : (error as Error).message, error, }); -this.logger.log('Webhook error data:: ', { - state, - entityId, - correlationId, - id: runtimeData.id, -}); +this.logger.log('Webhook error state:: ', { state });
🧹 Nitpick comments (5)
services/workflows-service/src/common/http-service/http-config.service.ts (1)
6-6: Consider utilizing the defined RETRY_DELAY_IN_MS constant.The
RETRY_DELAY_IN_MSconstant is defined but not utilized in the retry configuration. Consider adding aretryDelayoption to make use of this constant.return { timeout, maxRedirects: configService.get('HTTP_MAX_REDIRECTS', 10), retryAttempts: configService.get('HTTP_RETRY_ATTEMPTS', 0), + retryDelay: configService.get('HTTP_RETRY_DELAY_IN_MS', RETRY_DELAY_IN_MS), signal: newAbortSignal(timeout), validateStatus: (status: number) => { return status < 500; }, };services/workflows-service/src/webhooks/webhooks.module.ts (1)
28-28: Add module documentation.Consider adding JSDoc comments to document the module's purpose, configuration options, and usage examples. This would help other developers understand how to use the webhooks system and Bull Board integration.
Add documentation like this:
+/** + * WebhooksModule provides webhook processing capabilities with Bull Board integration for queue monitoring. + * + * The module: + * - Integrates Bull Board UI at /api/queues for monitoring queue status + * - Exports WebhooksService for handling webhook operations + * + * @example + * ```typescript + * @Module({ + * imports: [WebhooksModule], + * }) + * export class AppModule {} + * ``` + */ export class WebhooksModule {services/workflows-service/src/webhooks/retryable-queue.ts (1)
32-43: Consider enhancing error handling in the failed job handler.The failed job handler could benefit from additional error context and logging before moving to DLQ.
this.worker.on('failed', async (job, err) => { if (!job) { return; } + const attempts = job.opts.attempts ?? 1; + const attemptsMade = job.attemptsMade; + this.logger.warn(`Job ${job.id} failed (attempt ${attemptsMade}/${attempts})`, { + error: err, + jobData: job.data, + }); - if (job.attemptsMade >= (job.opts.attempts ?? 1)) { + if (attemptsMade >= attempts) { return await this.dlq.add( (job.id ?? `dlq-${Date.now()}`) as any, { ...job.data, error: err } as any, ); } });services/workflows-service/src/webhooks/webhooks.service.ts (1)
36-61: Consider implementing reconnection backoff limits.The Redis connection setup uses a fixed retry delay. Consider implementing a maximum retry limit or exponential backoff with maximum delay to prevent infinite reconnection attempts.
const redis = new IORedis({ ...connection, - retryStrategy: () => 10_000, + retryStrategy: (times) => { + const maxDelay = 30_000; // 30 seconds + const maxRetries = 10; + + if (times > maxRetries) { + return null; // Stop retrying + } + + const delay = Math.min(times * 10_000, maxDelay); + return delay; + }, connectTimeout: 5_000, maxRetriesPerRequest: null, });services/workflows-service/package.json (1)
8-8: Verify Redis port availability before setup.The setup script assumes Redis will be available on the default port. Consider:
- Adding port verification to prevent conflicts.
- Making the Redis port configurable.
- "setup": "npm run docker:db:down && npm run docker:db && wait-on tcp:5432 && npm run docker:redis:down && npm run docker:redis && npm run db:reset && npm run seed", + "setup": "npm run docker:db:down && npm run docker:db && wait-on tcp:5432 && npm run docker:redis:down && npm run docker:redis && wait-on tcp:6379 && npm run db:reset && npm run seed",Also applies to: 35-36
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (25)
services/workflows-service/docker-compose.redis.yml(1 hunks)services/workflows-service/package.json(5 hunks)services/workflows-service/prisma/data-migrations(1 hunks)services/workflows-service/src/alert/alert.module.ts(3 hunks)services/workflows-service/src/alert/webhook-manager/types.ts(0 hunks)services/workflows-service/src/alert/webhook-manager/webhook-event-emitter.service.ts(0 hunks)services/workflows-service/src/alert/webhook-manager/webhook-http.service.ts(0 hunks)services/workflows-service/src/alert/webhook-manager/webhook-manager.service.ts(0 hunks)services/workflows-service/src/app.module.ts(3 hunks)services/workflows-service/src/business-report/business-report.controller.external.ts(1 hunks)services/workflows-service/src/common/http-service/http-config.service.ts(1 hunks)services/workflows-service/src/env.ts(1 hunks)services/workflows-service/src/events/document-changed-webhook-caller.ts(3 hunks)services/workflows-service/src/events/workflow-completed-webhook-caller.ts(3 hunks)services/workflows-service/src/events/workflow-state-changed-webhook-caller.ts(2 hunks)services/workflows-service/src/webhooks-incoming/webhooks-incoming.controller.ts(3 hunks)services/workflows-service/src/webhooks-incoming/webhooks-incoming.module.ts(1 hunks)services/workflows-service/src/webhooks-incoming/webhooks-incoming.service.ts(1 hunks)services/workflows-service/src/webhooks/retryable-queue.ts(1 hunks)services/workflows-service/src/webhooks/types/bull.ts(1 hunks)services/workflows-service/src/webhooks/types/webhook.ts(1 hunks)services/workflows-service/src/webhooks/webhooks.module.ts(1 hunks)services/workflows-service/src/webhooks/webhooks.service.ts(1 hunks)services/workflows-service/src/workflow/workflow.module.ts(2 hunks)services/workflows-service/src/workflow/workflow.service.unit.test.ts(1 hunks)
💤 Files with no reviewable changes (4)
- services/workflows-service/src/alert/webhook-manager/webhook-http.service.ts
- services/workflows-service/src/alert/webhook-manager/webhook-event-emitter.service.ts
- services/workflows-service/src/alert/webhook-manager/webhook-manager.service.ts
- services/workflows-service/src/alert/webhook-manager/types.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- services/workflows-service/src/workflow/workflow.service.unit.test.ts
- services/workflows-service/src/business-report/business-report.controller.external.ts
- services/workflows-service/prisma/data-migrations
- services/workflows-service/src/webhooks/types/bull.ts
- services/workflows-service/src/webhooks-incoming/webhooks-incoming.module.ts
- services/workflows-service/docker-compose.redis.yml
- services/workflows-service/src/workflow/workflow.module.ts
- services/workflows-service/src/env.ts
- services/workflows-service/src/app.module.ts
- services/workflows-service/src/webhooks/types/webhook.ts
- services/workflows-service/src/alert/alert.module.ts
- services/workflows-service/src/webhooks-incoming/webhooks-incoming.service.ts
- services/workflows-service/src/webhooks-incoming/webhooks-incoming.controller.ts
🧰 Additional context used
🪛 Biome (1.9.4)
services/workflows-service/src/webhooks/retryable-queue.ts
[error] 46-46: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test_windows
- GitHub Check: test_linux
- GitHub Check: spell_check
- GitHub Check: build (windows-latest)
- GitHub Check: Analyze (javascript)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: lint
🔇 Additional comments (9)
services/workflows-service/src/common/http-service/http-config.service.ts (1)
26-52: LGTM! Well-structured HTTP module configuration.The implementation includes good practices:
- Configurable timeout with abort signal mechanism
- Retry attempts configuration
- Consistent status validation
- Clear and maintainable code structure
services/workflows-service/src/webhooks/webhooks.module.ts (2)
1-9: LGTM! Dependencies are well organized.The imports are properly structured and include all necessary dependencies for Bull Board integration and NestJS module configuration.
10-27: Verify the empty queues configuration in Bull Board setup.The Bull Board is initialized with an empty queue array. While this might be intentional if queues are added dynamically, please verify this is the expected behavior.
✅ Verification successful
Bull Board Queue Configuration is as Expected
The module intentionally initializes Bull Board with an empty queues array. Queue registration is handled dynamically in
webhooks.service.tsvia calls tosetQueues(), which confirms the expected behavior.
- Queues are dynamically set with adapters in
services/workflows-service/src/webhooks/webhooks.service.ts.- An empty array is also used in
degradeQueueSystemto clear or reset the queues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for queue registrations in the codebase # Expected: Find where queues are dynamically registered with Bull Board # Search for queue registrations rg -A 5 "boardInstance|serverAdapter" --type ts # Search for Bull queue definitions ast-grep --pattern 'Queue($$$)'Length of output: 2614
services/workflows-service/src/webhooks/retryable-queue.ts (2)
3-8: LGTM! Well-structured class with proper type definitions.The class is well-designed with clear separation between main queue and DLQ, using proper TypeScript generics for type safety.
46-53: LGTM! Proper resource cleanup.The shutdown method correctly closes all queues and workers to prevent resource leaks.
🧰 Tools
🪛 Biome (1.9.4)
[error] 46-46: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
services/workflows-service/src/events/workflow-state-changed-webhook-caller.ts (1)
72-86: LGTM! Clean payload construction with proper typing.The webhook payload is well-structured with proper type assertions using
as const.services/workflows-service/src/events/document-changed-webhook-caller.ts (2)
7-7: LGTM!The import changes align with the refactoring to use
WebhooksServicefor webhook handling.Also applies to: 14-14
33-33: LGTM!The constructor injection of
WebhooksServicealigns with the refactoring to use a queue-based system for webhook handling.services/workflows-service/package.json (1)
58-60: Verify package versions for vulnerabilities.Please verify that the following package versions are secure and up-to-date:
@bull-board/[email protected]@bull-board/[email protected][email protected][email protected]Also applies to: 88-88, 101-101
services/workflows-service/src/events/workflow-completed-webhook-caller.ts
Outdated
Show resolved
Hide resolved
services/workflows-service/src/events/document-changed-webhook-caller.ts
Show resolved
Hide resolved
services/workflows-service/src/events/document-changed-webhook-caller.ts
Show resolved
Hide resolved
r4zendev
force-pushed
the
bullmq-webhooks-2
branch
from
e1adfc2 to
7bcd4c6
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
services/workflows-service/src/events/workflow-state-changed-webhook-caller.ts (1)
24-26: Replace console.error with logger service.Using console.error for error logging is not recommended in production environments.
Apply this diff to use the logger service:
} catch (error) { - console.error(error); + this.logger.error('Failed to handle workflow event', { error }); alertWebhookFailure(error); }
♻️ Duplicate comments (4)
services/workflows-service/src/webhooks/webhooks.module.ts (1)
34-35:⚠️ Potential issueFix route path mismatch in middleware configuration.
The base path is set to
/api/queuesbut the middleware is configured for/queues. This mismatch could lead to routing issues.Apply this diff to fix the route path:
- consumer?.apply(this.bullBoard.serverAdapter.getRouter()).forRoutes('/queues'); + consumer?.apply(this.bullBoard.serverAdapter.getRouter()).forRoutes('/api/queues');services/workflows-service/src/webhooks/webhooks.service.ts (1)
73-77:⚠️ Potential issueSensitive Data Exposure in Webhook Error Logs
The error log for failed jobs passes the full
job.datato the logger without any sanitization. This could expose sensitive data if any sensitive fields are present injob.data.Consider redacting or masking sensitive fields before logging:
- this.logger.error('Failed to send webhook through queue system.', { - id: job.id, - jobData: job.data, - error: err, - }); + this.logger.error('Failed to send webhook through queue system.', { + id: job.id, + jobData: { + url: job.data.url, + method: job.data.method, + // Exclude sensitive data from headers and payload + headers: Object.keys(job.data.headers || {}), + }, + error: err, + });services/workflows-service/src/events/workflow-completed-webhook-caller.ts (1)
99-134:⚠️ Potential issueRemove duplicate webhook sending code.
The same webhook sending logic is duplicated within the same method. The second block appears to be an older version that wasn't properly removed during refactoring.
services/workflows-service/src/events/document-changed-webhook-caller.ts (1)
170-192:⚠️ Potential issueSensitive Data Handling in Webhook Payload
The payload still includes unmasked assignee details. The shell script search did not reveal any masking, redaction, or sanitization of these PII fields.
🧹 Nitpick comments (4)
services/workflows-service/src/common/http-service/http-config.service.ts (1)
26-52: Well-structured implementation with good error handling.The implementation includes several good practices:
- Abort signal mechanism for handling connectivity issues
- Configurable retry attempts
- Clear validation of HTTP status codes
Consider extracting the duplicated
validateStatuslogic into a shared constant:+const validateHttpStatus = (status: number) => status < 500; + export const initHttpModule = () => HttpModule.registerAsync({ // ... other code ... useFactory: (configService: ConfigService) => ({ // ... other options ... - validateStatus: (status: number) => { - return status < 500; - }, + validateStatus: validateHttpStatus, }), });services/workflows-service/src/webhooks/retryable-queue.ts (1)
53-53: Fix return type annotation.The static analysis tool correctly flags the use of
voidin the return type.Apply this diff to fix the return type:
- async shutdown(): Promise<void[]> { + async shutdown(): Promise<undefined[]> {🧰 Tools
🪛 Biome (1.9.4)
[error] 53-53: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
services/workflows-service/src/webhooks-incoming/webhooks-incoming.service.ts (1)
14-21: Enhance error context in Sentry alerts.The error context sent to Sentry could be more informative.
Add more context to Sentry alerts:
export const alertWebhookFailure = (error: unknown) => { const errorToAlert = new Error('Failed to send a webhook', { cause: error }); - const context = isAxiosError(error) ? { ...error } : {}; + const context = isAxiosError(error) + ? { + response: error.response?.data, + status: error.response?.status, + headers: error.response?.headers, + url: error.config?.url, + } + : {}; Sentry.captureException(errorToAlert, { extra: context, + tags: { + type: isAxiosError(error) ? 'axios_error' : 'unknown_error', + }, }); };services/workflows-service/src/webhooks/webhooks.service.ts (1)
36-61: Enhance Redis connection error handling.While the Redis connection setup is good, consider improving error handling:
- Log the actual error details in the error handler.
- Consider implementing a reconnection mechanism with exponential backoff.
Apply this diff to enhance error handling:
redis.on('error', err => { + this.logger.error('Redis connection error', { + error: err, + host: env.REDIS_HOST, + port: env.REDIS_PORT, + }); this.degradeQueueSystem(err); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (25)
services/workflows-service/docker-compose.redis.yml(1 hunks)services/workflows-service/package.json(5 hunks)services/workflows-service/prisma/data-migrations(1 hunks)services/workflows-service/src/alert/alert.module.ts(3 hunks)services/workflows-service/src/alert/webhook-manager/types.ts(0 hunks)services/workflows-service/src/alert/webhook-manager/webhook-event-emitter.service.ts(0 hunks)services/workflows-service/src/alert/webhook-manager/webhook-http.service.ts(0 hunks)services/workflows-service/src/alert/webhook-manager/webhook-manager.service.ts(0 hunks)services/workflows-service/src/app.module.ts(3 hunks)services/workflows-service/src/business-report/business-report.controller.external.ts(1 hunks)services/workflows-service/src/common/http-service/http-config.service.ts(1 hunks)services/workflows-service/src/env.ts(1 hunks)services/workflows-service/src/events/document-changed-webhook-caller.ts(3 hunks)services/workflows-service/src/events/workflow-completed-webhook-caller.ts(2 hunks)services/workflows-service/src/events/workflow-state-changed-webhook-caller.ts(2 hunks)services/workflows-service/src/webhooks-incoming/webhooks-incoming.controller.ts(3 hunks)services/workflows-service/src/webhooks-incoming/webhooks-incoming.module.ts(1 hunks)services/workflows-service/src/webhooks-incoming/webhooks-incoming.service.ts(1 hunks)services/workflows-service/src/webhooks/retryable-queue.ts(1 hunks)services/workflows-service/src/webhooks/types/bull.ts(1 hunks)services/workflows-service/src/webhooks/types/webhook.ts(1 hunks)services/workflows-service/src/webhooks/webhooks.module.ts(1 hunks)services/workflows-service/src/webhooks/webhooks.service.ts(1 hunks)services/workflows-service/src/workflow/workflow.module.ts(2 hunks)services/workflows-service/src/workflow/workflow.service.unit.test.ts(1 hunks)
💤 Files with no reviewable changes (4)
- services/workflows-service/src/alert/webhook-manager/webhook-event-emitter.service.ts
- services/workflows-service/src/alert/webhook-manager/webhook-http.service.ts
- services/workflows-service/src/alert/webhook-manager/types.ts
- services/workflows-service/src/alert/webhook-manager/webhook-manager.service.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- services/workflows-service/src/webhooks/types/bull.ts
- services/workflows-service/src/workflow/workflow.module.ts
- services/workflows-service/src/workflow/workflow.service.unit.test.ts
- services/workflows-service/docker-compose.redis.yml
- services/workflows-service/src/webhooks-incoming/webhooks-incoming.module.ts
- services/workflows-service/prisma/data-migrations
- services/workflows-service/src/webhooks/types/webhook.ts
- services/workflows-service/src/business-report/business-report.controller.external.ts
- services/workflows-service/src/app.module.ts
- services/workflows-service/src/env.ts
- services/workflows-service/src/webhooks-incoming/webhooks-incoming.controller.ts
🧰 Additional context used
🪛 Biome (1.9.4)
services/workflows-service/src/webhooks/retryable-queue.ts
[error] 53-53: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test_windows
- GitHub Check: test_linux
- GitHub Check: build (windows-latest)
🔇 Additional comments (10)
services/workflows-service/src/alert/alert.module.ts (3)
22-22: LGTM!The import of WebhooksModule aligns with the centralization of webhook handling.
31-31: LGTM!The WebhooksModule is correctly integrated into the imports array.
56-56: Verify the impact of removing WebhookEventEmitterService from exports.The removal of WebhookEventEmitterService from exports is a breaking change. Let's verify that all dependent modules have been updated accordingly.
✅ Verification successful
Verified the removal impact of WebhookEventEmitterService from exports is safe.
The search for any imports or dependency injections of WebhookEventEmitterService in non-test files returned no output, indicating that no dependent modules are still referencing this service.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any remaining references to WebhookEventEmitterService that might be affected by its removal from exports # Search for imports of WebhookEventEmitterService rg "import.*WebhookEventEmitterService.*from.*alert" -g "!*.{spec,test}.*" # Search for usage of WebhookEventEmitterService in dependency injection rg "WebhookEventEmitterService.*\)" -g "!*.{spec,test}.*"Length of output: 136
services/workflows-service/src/common/http-service/http-config.service.ts (2)
8-24: Consider removing unused HttpConfigService class.The commented code on line 29 suggests that this class is not being used. If
initHttpModuleis the preferred approach, consider removing this class to avoid confusion and maintain cleaner code.Run the following script to verify if the class is used elsewhere:
✅ Verification successful
Unused HttpConfigService is confirmed to be unused.
The grep search shows that the only occurrence of "HttpConfigService" is in its own file. This indicates that the class isn’t referenced anywhere else in the codebase, so removing it to reduce confusion is appropriate.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usages of HttpConfigService across the codebase. rg -l "HttpConfigService"Length of output: 99
15-23: Inconsistent timeout defaults between createHttpOptions and initHttpModule.The default timeout in
createHttpOptionsis 5000ms whileinitHttpModuleuses 10000ms. This inconsistency could lead to confusion.Apply this diff to align the timeout defaults:
- timeout: this.configService.get('HTTP_TIMEOUT_IN_MS', 5000), + timeout: this.configService.get('HTTP_TIMEOUT_IN_MS', 10_000),services/workflows-service/src/webhooks/webhooks.service.ts (4)
16-21: LGTM! Good error handling.The function properly captures webhook failures in Sentry with appropriate context and error details.
99-104: LGTM! Good graceful degradation handling.The function properly handles queue system degradation by logging the event and cleaning up resources.
171-173: LGTM! Good cleanup.The function properly handles queue cleanup during module destruction.
150-163:⚠️ Potential issueSensitive Data Exposure in Error Logs
The error logs expose potentially sensitive data from the webhook payload.
Consider redacting or masking sensitive fields before logging:
- this.logger.error('Failed to send webhook', { - id, - message: isAxiosError(error) - ? error.response?.data ?? error.message - : (error as Error).message, - error, - }); - - this.logger.log('Webhook error data:: ', { - state, - entityId, - correlationId, - id: runtimeData.id, - }); + this.logger.error('Failed to send webhook', { + id, + message: isAxiosError(error) + ? error.response?.statusText ?? error.message + : (error as Error).message, + status: isAxiosError(error) ? error.response?.status : undefined, + }); + + this.logger.log('Webhook error metadata:: ', { + correlationId, + id: runtimeData.id, + });Likely invalid or redundant comment.
services/workflows-service/src/events/document-changed-webhook-caller.ts (1)
194-199: Add error handling for webhook invocation.Consider adding error handling to capture and log webhook failures.
services/workflows-service/src/events/workflow-state-changed-webhook-caller.ts
Show resolved
Hide resolved
services/workflows-service/src/webhooks-incoming/webhooks-incoming.service.ts
Show resolved
Hide resolved
services/workflows-service/src/webhooks-incoming/webhooks-incoming.service.ts
Show resolved
Hide resolved
r4zendev
force-pushed
the
bullmq-webhooks-2
branch
from
7bcd4c6 to
0a69cb4
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
services/workflows-service/src/webhooks/retryable-queue.ts (2)
38-38:⚠️ Potential issueReplace console.log with structured logging.
Using console.log for error logging is not recommended in production environments.
Apply this diff to use structured logging:
- console.log('err', err); + this.logger.error('Job processing failed', { error: err, jobId: job.id });
43-46:⚠️ Potential issueImprove type safety in DLQ job handling.
The code uses unsafe type casting and has a potentially unsafe job ID generation.
Apply this diff to improve type safety:
- (job.id ?? `dlq-${Date.now()}`) as any, - { ...job.data, error: err } as any, + `dlq-${job.id ?? Date.now()}-${Math.random().toString(36).slice(2)}`, + { ...job.data, error: { message: err.message, stack: err.stack } } as T,services/workflows-service/src/events/workflow-state-changed-webhook-caller.ts (1)
86-91:⚠️ Potential issueAdd URL validation and request timeout in webhook invocation.
The code passes the URL to invokeWebhook without ensuring it's secure or imposing a timeout.
Apply this diff to add URL validation and timeout:
await this.webhooksService.invokeWebhook(payload.eventName, { url, method: 'POST', data: payload, secret: webhookSharedSecret, + timeout: 5000, + validateUrlPattern: /^https:\/\//, });services/workflows-service/src/webhooks/webhooks.service.ts (1)
73-77:⚠️ Potential issueAdd data sanitization before logging.
The error log passes the full job data without sanitization, which could expose sensitive information.
Apply this diff to sanitize sensitive data:
- this.logger.error('Failed to send webhook through queue system.', { - id: job.id, - jobData: job.data, - error: err, - }); + this.logger.error('Failed to send webhook through queue system.', { + id: job.id, + jobData: this.sanitizeWebhookData(job.data), + error: this.sanitizeError(err), + });services/workflows-service/src/events/document-changed-webhook-caller.ts (2)
170-192:⚠️ Potential issueSensitive data exposure in webhook payload.
The assignee information in the payload contains PII (firstName, lastName, email). This is a duplicate of a previously identified issue.
const payload = { id, eventName: 'workflow.context.document.changed', apiVersion, timestamp: new Date().toISOString(), assignee: data.assignee ? { id: data.assignee.id, - firstName: data.assignee.firstName, - lastName: data.assignee.lastName, - email: data.assignee.email, + // Only include non-PII fields + role: data.assignee.role, } : null, assignedAt: data.assignedAt, // ... rest of the payload } as const;
194-199: 🛠️ Refactor suggestionAdd error handling for webhook invocation.
This is a duplicate of a previously identified issue regarding error handling.
services/workflows-service/package.json (1)
35-36:⚠️ Potential issueSecure Redis configuration in Docker commands.
This is a duplicate of a previously identified issue regarding Redis security.
🧹 Nitpick comments (5)
services/workflows-service/src/webhooks/bull-board.auth.middleware.ts (1)
11-12: Consider moving environment and role checks to configuration.Hard-coded values for environment and role checks could be moved to configuration for better maintainability.
Consider adding these to your configuration:
// config/auth.config.ts export const authConfig = { production: { environment: 'production', adminRole: 'admin', }, };Then update the middleware to use these values:
- if ( - this.config.get('NODE_ENV') === 'production' && - req.session?.passport.user.type !== 'admin' - ) + const authConfig = this.config.get('auth'); + if ( + this.config.get('NODE_ENV') === authConfig.production.environment && + userType !== authConfig.production.adminRole + )services/workflows-service/src/webhooks-incoming/webhooks-incoming.service.ts (2)
14-21: Enhance type safety in error handling.Consider using type narrowing for better type safety and error handling.
-export const alertWebhookFailure = (error: unknown) => { +export const alertWebhookFailure = (error: Error | unknown) => { const errorToAlert = new Error('Failed to send a webhook', { cause: error }); - const context = isAxiosError(error) ? { ...error } : {}; + const context = isAxiosError(error) + ? { + status: error.response?.status, + data: error.response?.data, + url: error.config?.url, + } + : error instanceof Error + ? { message: error.message, stack: error.stack } + : {}; Sentry.captureException(errorToAlert, { extra: context, }); };
23-33: Consider breaking down the service for better maintainability.The service has 7 dependencies, which might indicate it's handling too many responsibilities. Consider splitting it into smaller, more focused services:
- AML processing service
- Workflow management service
- User data service
services/workflows-service/src/events/workflow-completed-webhook-caller.ts (1)
99-126: Consider sanitizing sensitive data in the webhook payload.The payload includes potentially sensitive data from the runtime context. Consider implementing data sanitization or masking for sensitive fields.
const payload = { id, eventName: 'workflow.completed', apiVersion, timestamp: new Date().toISOString(), workflowCreatedAt: createdAt, workflowResolvedAt: resolvedAt, workflowDefinitionId, workflowRuntimeId: runtimeDataId, workflowStatus: data.runtimeData.status, workflowFinalState: data.runtimeData.state, ballerineEntityId: entityId, correlationId, environment, data: { - ...restRuntimeData.context, + ...sanitizeContext(restRuntimeData.context), childWorkflowsRuntimeData, }, } as const;services/workflows-service/package.json (1)
58-60: Pin dependency versions for security.The new dependencies use caret ranges (^) which could lead to unexpected updates. Consider pinning specific versions for better stability and security.
- "@bull-board/api": "^6.7.1", - "@bull-board/express": "^6.7.1", - "bullmq": "^5.13.2", - "ioredis": "^5.4.2", + "@bull-board/api": "6.7.1", + "@bull-board/express": "6.7.1", + "bullmq": "5.13.2", + "ioredis": "5.4.2",Also applies to: 88-89, 101-101
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (26)
services/workflows-service/docker-compose.redis.yml(1 hunks)services/workflows-service/package.json(5 hunks)services/workflows-service/prisma/data-migrations(1 hunks)services/workflows-service/src/alert/alert.module.ts(3 hunks)services/workflows-service/src/alert/webhook-manager/types.ts(0 hunks)services/workflows-service/src/alert/webhook-manager/webhook-event-emitter.service.ts(0 hunks)services/workflows-service/src/alert/webhook-manager/webhook-http.service.ts(0 hunks)services/workflows-service/src/alert/webhook-manager/webhook-manager.service.ts(0 hunks)services/workflows-service/src/app.module.ts(3 hunks)services/workflows-service/src/business-report/business-report.controller.external.ts(1 hunks)services/workflows-service/src/common/http-service/http-config.service.ts(1 hunks)services/workflows-service/src/env.ts(1 hunks)services/workflows-service/src/events/document-changed-webhook-caller.ts(3 hunks)services/workflows-service/src/events/workflow-completed-webhook-caller.ts(2 hunks)services/workflows-service/src/events/workflow-state-changed-webhook-caller.ts(2 hunks)services/workflows-service/src/webhooks-incoming/webhooks-incoming.controller.ts(3 hunks)services/workflows-service/src/webhooks-incoming/webhooks-incoming.module.ts(1 hunks)services/workflows-service/src/webhooks-incoming/webhooks-incoming.service.ts(1 hunks)services/workflows-service/src/webhooks/bull-board.auth.middleware.ts(1 hunks)services/workflows-service/src/webhooks/retryable-queue.ts(1 hunks)services/workflows-service/src/webhooks/types/bull.ts(1 hunks)services/workflows-service/src/webhooks/types/webhook.ts(1 hunks)services/workflows-service/src/webhooks/webhooks.module.ts(1 hunks)services/workflows-service/src/webhooks/webhooks.service.ts(1 hunks)services/workflows-service/src/workflow/workflow.module.ts(2 hunks)services/workflows-service/src/workflow/workflow.service.unit.test.ts(1 hunks)
💤 Files with no reviewable changes (4)
- services/workflows-service/src/alert/webhook-manager/webhook-http.service.ts
- services/workflows-service/src/alert/webhook-manager/webhook-manager.service.ts
- services/workflows-service/src/alert/webhook-manager/webhook-event-emitter.service.ts
- services/workflows-service/src/alert/webhook-manager/types.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- services/workflows-service/src/workflow/workflow.service.unit.test.ts
- services/workflows-service/src/business-report/business-report.controller.external.ts
- services/workflows-service/prisma/data-migrations
- services/workflows-service/src/workflow/workflow.module.ts
- services/workflows-service/src/webhooks/types/bull.ts
- services/workflows-service/src/env.ts
- services/workflows-service/src/app.module.ts
- services/workflows-service/src/common/http-service/http-config.service.ts
- services/workflows-service/docker-compose.redis.yml
- services/workflows-service/src/alert/alert.module.ts
- services/workflows-service/src/webhooks-incoming/webhooks-incoming.module.ts
- services/workflows-service/src/webhooks/types/webhook.ts
- services/workflows-service/src/webhooks-incoming/webhooks-incoming.controller.ts
🧰 Additional context used
🪛 Biome (1.9.4)
services/workflows-service/src/webhooks/retryable-queue.ts
[error] 53-53: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test_windows
- GitHub Check: test_linux
- GitHub Check: Analyze (javascript)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: lint
🔇 Additional comments (10)
services/workflows-service/src/webhooks/bull-board.auth.middleware.ts (1)
1-8: LGTM! Clean implementation following NestJS best practices.The class structure and dependency injection are well implemented, following NestJS conventions.
services/workflows-service/src/webhooks-incoming/webhooks-incoming.service.ts (3)
1-13: LGTM!The imports are well-organized and all dependencies are properly utilized in the implementation.
109-113: Fix type incompatibility instead of suppressing the error.Using @ts-expect-error is a temporary solution that should be properly fixed.
43-72: Implement data sanitization for sensitive information.The service processes sensitive user data without proper sanitization.
services/workflows-service/src/webhooks/webhooks.module.ts (1)
35-39: LGTM! Authentication and routing have been properly configured.The implementation now correctly:
- Uses BullBoardAuthMiddleware for securing the Bull Board UI
- Configures the routes with proper middleware chain
services/workflows-service/src/webhooks/webhooks.service.ts (4)
36-61: LGTM! Robust Redis connection handling.The implementation includes:
- Proper connection configuration
- Retry strategy with reasonable timeout
- Error handling for connection failures
63-97: LGTM! Well-structured queue setup with proper error handling.The implementation includes:
- Proper queue configuration
- DLQ handling
- Retry mechanism with logging
- Bull Board integration
106-169: LGTM! Comprehensive webhook invocation with fallback.The implementation includes:
- Proper headers and HMAC signing
- Queue-based processing with direct fallback
- Comprehensive error handling
148-163:⚠️ Potential issueAdd data sanitization before logging webhook errors.
The error and webhook data logs could expose sensitive information.
Apply this diff to sanitize sensitive data:
- this.logger.error('Failed to send webhook', { - id, - message: isAxiosError(error) - ? error.response?.data ?? error.message - : (error as Error).message, - error, - }); + this.logger.error('Failed to send webhook', { + id, + message: isAxiosError(error) + ? this.sanitizeErrorResponse(error.response?.data) ?? error.message + : (error as Error).message, + error: this.sanitizeError(error), + }); - this.logger.log('Errored webhook data:: ', { - state, - entityId, - correlationId, - id: runtimeData.id, - }); + this.logger.log('Errored webhook data:: ', { + state, + entityId, + correlationId, + id: runtimeData.id, + // Add only necessary non-sensitive fields + });Likely invalid or redundant comment.
services/workflows-service/src/events/workflow-completed-webhook-caller.ts (1)
25-25: LGTM! WebhooksService injection.The transition from HttpService to WebhooksService aligns with the centralized webhook handling approach.
services/workflows-service/src/events/workflow-completed-webhook-caller.ts
Show resolved
Hide resolved
r4zendev
force-pushed
the
bullmq-webhooks-2
branch
from
0a69cb4 to
bd62098
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
services/workflows-service/src/events/workflow-state-changed-webhook-caller.ts (1)
86-91:⚠️ Potential issueAdd URL validation and request timeout in webhook invocation.
The webhook invocation should include security measures:
- Add a timeout to prevent hanging requests
- Validate the URL to ensure only secure endpoints (HTTPS) are accepted
Apply this diff to enhance security:
await this.webhooksService.invokeWebhook(payload.eventName, { url, method: 'POST', data: payload, secret: webhookSharedSecret, + timeout: 5000, + validateUrlPattern: /^https:\/\//, });services/workflows-service/src/events/document-changed-webhook-caller.ts (2)
170-192:⚠️ Potential issueSensitive Data Handling in Webhook Payload
The payload still includes unmasked assignee details (id, firstName, lastName, email). Consider adding PII sanitization or configurable masking logic.
194-199: 🛠️ Refactor suggestionAdd error handling for webhook invocation
Consider adding error handling to capture and log webhook failures.
- await this.webhooksService.invokeWebhook(payload.eventName, { + try { + await this.webhooksService.invokeWebhook(payload.eventName, { + url, + method: 'POST', + data: payload, + secret: webhookSharedSecret, + }); + } catch (error) { + this.logger.error('Failed to invoke webhook', { + url, + eventName: payload.eventName, + error, + workflowRuntimeId: payload.workflowRuntimeId, + }); + throw error; + } - url, - method: 'POST', - data: payload, - secret: webhookSharedSecret, - });
🧹 Nitpick comments (4)
services/workflows-service/src/events/workflow-completed-webhook-caller.ts (1)
99-126: Consider simplifying the payload construction.The payload construction can be simplified by using object destructuring and spread operators more effectively.
- // Omit from data properties already sent as part of the webhook payload - const { runtimeData, correlationId, entityId, childWorkflowsRuntimeData, ...restData } = data; - const { - createdAt, - resolvedAt, - workflowDefinitionId, - id: runtimeDataId, - ...restRuntimeData - } = runtimeData; - const payload = { - id, - eventName: 'workflow.completed', - apiVersion, - timestamp: new Date().toISOString(), - workflowCreatedAt: createdAt, - workflowResolvedAt: resolvedAt, - workflowDefinitionId, - workflowRuntimeId: runtimeDataId, - workflowStatus: data.runtimeData.status, - workflowFinalState: data.runtimeData.state, - ballerineEntityId: entityId, - correlationId, - environment, - data: { - ...restRuntimeData.context, - childWorkflowsRuntimeData, - }, - } as const; + const { + runtimeData: { + createdAt, + resolvedAt, + workflowDefinitionId, + id: runtimeDataId, + status, + state, + context, + }, + correlationId, + entityId, + childWorkflowsRuntimeData, + } = data; + + const payload = { + id, + eventName: 'workflow.completed', + apiVersion, + timestamp: new Date().toISOString(), + workflowCreatedAt: createdAt, + workflowResolvedAt: resolvedAt, + workflowDefinitionId, + workflowRuntimeId: runtimeDataId, + workflowStatus: status, + workflowFinalState: state, + ballerineEntityId: entityId, + correlationId, + environment, + data: { + ...context, + childWorkflowsRuntimeData, + }, + } as const;services/workflows-service/src/webhooks-incoming/webhooks-incoming.service.ts (1)
14-21: Enhance error context for better debugging.While the error handling is good, consider enhancing the error context for better debugging:
export const alertWebhookFailure = (error: unknown) => { - const errorToAlert = new Error('Failed to send a webhook', { cause: error }); - const context = isAxiosError(error) ? { ...error } : {}; + const errorToAlert = new Error( + `Webhook failure: ${error instanceof Error ? error.message : 'Unknown error'}`, + { cause: error } + ); + const context = { + ...(isAxiosError(error) ? { + status: error.response?.status, + statusText: error.response?.statusText, + data: error.response?.data, + url: error.config?.url, + method: error.config?.method, + } : {}), + timestamp: new Date().toISOString(), + }; Sentry.captureException(errorToAlert, { extra: context, }); };services/workflows-service/src/webhooks/retryable-queue.ts (1)
48-48: Fix return type of shutdown method.The
voidreturn type in the array is confusing. UseundefinedorPromise<void>instead.Apply this diff to fix the return type:
- async shutdown(): Promise<void[]> { + async shutdown(): Promise<undefined[]> {🧰 Tools
🪛 Biome (1.9.4)
[error] 48-48: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
services/workflows-service/src/webhooks/webhooks.service.ts (1)
49-54: Improve Redis retry strategy.The current retry strategy is simplistic. Consider implementing an exponential backoff strategy with a maximum retry limit.
Apply this diff to improve the retry strategy:
const redis = new IORedis({ ...connection, - retryStrategy: () => 10_000, + retryStrategy: (times) => { + const delay = Math.min(times * 2000, 30000); + return times <= 20 ? delay : null; + }, connectTimeout: 5_000, maxRetriesPerRequest: null, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (26)
services/workflows-service/docker-compose.redis.yml(1 hunks)services/workflows-service/package.json(5 hunks)services/workflows-service/prisma/data-migrations(1 hunks)services/workflows-service/src/alert/alert.module.ts(3 hunks)services/workflows-service/src/alert/webhook-manager/types.ts(0 hunks)services/workflows-service/src/alert/webhook-manager/webhook-event-emitter.service.ts(0 hunks)services/workflows-service/src/alert/webhook-manager/webhook-http.service.ts(0 hunks)services/workflows-service/src/alert/webhook-manager/webhook-manager.service.ts(0 hunks)services/workflows-service/src/app.module.ts(3 hunks)services/workflows-service/src/business-report/business-report.controller.external.ts(1 hunks)services/workflows-service/src/common/http-service/http-config.service.ts(1 hunks)services/workflows-service/src/env.ts(1 hunks)services/workflows-service/src/events/document-changed-webhook-caller.ts(3 hunks)services/workflows-service/src/events/workflow-completed-webhook-caller.ts(2 hunks)services/workflows-service/src/events/workflow-state-changed-webhook-caller.ts(2 hunks)services/workflows-service/src/webhooks-incoming/webhooks-incoming.controller.ts(3 hunks)services/workflows-service/src/webhooks-incoming/webhooks-incoming.module.ts(1 hunks)services/workflows-service/src/webhooks-incoming/webhooks-incoming.service.ts(1 hunks)services/workflows-service/src/webhooks/bull-board.auth.middleware.ts(1 hunks)services/workflows-service/src/webhooks/retryable-queue.ts(1 hunks)services/workflows-service/src/webhooks/types/bull.ts(1 hunks)services/workflows-service/src/webhooks/types/webhook.ts(1 hunks)services/workflows-service/src/webhooks/webhooks.module.ts(1 hunks)services/workflows-service/src/webhooks/webhooks.service.ts(1 hunks)services/workflows-service/src/workflow/workflow.module.ts(2 hunks)services/workflows-service/src/workflow/workflow.service.unit.test.ts(1 hunks)
💤 Files with no reviewable changes (4)
- services/workflows-service/src/alert/webhook-manager/webhook-http.service.ts
- services/workflows-service/src/alert/webhook-manager/webhook-manager.service.ts
- services/workflows-service/src/alert/webhook-manager/webhook-event-emitter.service.ts
- services/workflows-service/src/alert/webhook-manager/types.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- services/workflows-service/src/workflow/workflow.module.ts
- services/workflows-service/src/business-report/business-report.controller.external.ts
- services/workflows-service/src/workflow/workflow.service.unit.test.ts
- services/workflows-service/src/webhooks-incoming/webhooks-incoming.module.ts
- services/workflows-service/src/app.module.ts
- services/workflows-service/prisma/data-migrations
- services/workflows-service/src/env.ts
- services/workflows-service/src/webhooks/types/bull.ts
- services/workflows-service/src/alert/alert.module.ts
- services/workflows-service/src/common/http-service/http-config.service.ts
- services/workflows-service/src/webhooks/types/webhook.ts
- services/workflows-service/src/webhooks/bull-board.auth.middleware.ts
- services/workflows-service/src/webhooks-incoming/webhooks-incoming.controller.ts
- services/workflows-service/docker-compose.redis.yml
🧰 Additional context used
🪛 Biome (1.9.4)
services/workflows-service/src/webhooks/retryable-queue.ts
[error] 48-48: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test_windows
- GitHub Check: test_linux
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Analyze (javascript)
- GitHub Check: lint
🔇 Additional comments (13)
services/workflows-service/src/events/workflow-state-changed-webhook-caller.ts (2)
10-10: LGTM! Clean dependency injection implementation.The WebhooksService integration follows dependency injection best practices with proper immutability.
Also applies to: 19-19
84-84: LGTM! Good type safety improvement.The
as constassertion ensures immutability of the payload object's properties.services/workflows-service/src/events/document-changed-webhook-caller.ts (2)
7-7: LGTM!The new imports are correctly added and align with the changes in the code.
Also applies to: 14-14
33-33: LGTM!The constructor changes align with the architectural shift to using a dedicated WebhooksService.
services/workflows-service/src/events/workflow-completed-webhook-caller.ts (2)
128-133: Add error handling for webhook invocation.The webhook invocation should include error handling to properly log and handle failures.
150-151: Fix the Prisma type issue instead of using @ts-expect-error.The
@ts-expect-errorcomment suggests a known type issue with Prisma. Consider fixing the underlying type issue instead of suppressing it.✅ Verification successful
Action Required: Address the Root Prisma Type Definition
The Prisma schema defines thetagsfield inWorkflowRuntimeDataasJson?, yet the code assumes it to be an array of strings. This type mismatch forces the use of@ts-expect-erroracross multiple files. Instead of suppressing the error, please adjust the type definitions or add appropriate type guards/casts to accurately reflect thattagsshould be an array of strings.
• Adjust the Prisma schema/client generation or modify the TypeScript interfaces to enforce the expected array type fortags.
• Verify that similar usage patterns in other files are similarly resolved rather than suppressed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the Prisma schema defines tags field correctly # and if there are any type issues in similar usage patterns # Check Prisma schema for tags field definition echo "Checking Prisma schema for tags field definition..." fd -e prisma | xargs cat # Check for similar @ts-expect-error usage patterns echo "Checking for similar @ts-expect-error usage patterns..." rg "@ts-expect-error.*Prisma" -A 2Length of output: 36017
services/workflows-service/src/webhooks-incoming/webhooks-incoming.service.ts (3)
43-72: Implement data sanitization for sensitive information.The service processes sensitive user data without proper sanitization.
The previous suggestion for implementing data sanitization is still valid and should be addressed for security purposes.
109-113: Fix type incompatibility instead of suppressing the error.Using @ts-expect-error is a temporary solution that should be properly fixed.
The previous suggestion for creating a proper type conversion utility is still valid and should be implemented.
35-123: Add proper error handling to the webhook handler.The method should handle potential errors that might occur during database operations or workflow creation.
The previous suggestion for implementing proper error handling with try-catch blocks is still valid and should be implemented.
services/workflows-service/src/webhooks/webhooks.module.ts (2)
21-21: Initialize queue list with a non-empty array.The queue list is initialized empty, which might cause issues if queues are added later. Consider initializing it with a default queue or handling the empty state.
37-38:⚠️ Potential issueFix route path mismatch in middleware configuration.
The base path is set to
/api/queuesbut the middleware is configured for/queues. This mismatch could lead to routing issues.Apply this diff to fix the route path:
- .forRoutes('/queues'); + .forRoutes('/api/queues');Likely invalid or redundant comment.
services/workflows-service/src/webhooks/retryable-queue.ts (1)
41-41: 🛠️ Refactor suggestionImprove job ID generation for DLQ.
The current job ID generation could lead to collisions. Consider using a more robust ID generation method.
Apply this diff to improve ID generation:
- return await this.dlq.add(job.id ?? `dlq-${Date.now()}`, { ...job.data, error }); + return await this.dlq.add( + `dlq-${job.id ?? Date.now()}-${Math.random().toString(36).slice(2)}`, + { ...job.data, error: { message: error.message, stack: error.stack } }, + );Likely invalid or redundant comment.
services/workflows-service/src/webhooks/webhooks.service.ts (1)
83-87:⚠️ Potential issueSanitize sensitive data in error logs.
The error log for failed jobs passes the full
job.datato the logger without any sanitization. This could expose sensitive data.Apply this diff to sanitize sensitive data:
- this.logger.error('Failed to send webhook through queue system.', { - id: job.id, - jobData: job.data, - error: job.data.error, - }); + const { url, method } = job.data; + this.logger.error('Failed to send webhook through queue system.', { + id: job.id, + url, + method, + error: job.data.error, + });Likely invalid or redundant comment.
services/workflows-service/src/webhooks-incoming/webhooks-incoming.service.ts
Show resolved
Hide resolved
services/workflows-service/src/webhooks-incoming/webhooks-incoming.service.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
services/workflows-service/src/webhooks/webhooks.service.ts (1)
83-87:⚠️ Potential issueSensitive Data Exposure in Webhook Error Logs
The error log for failed jobs passes the full
job.datato the logger without any sanitization. This could expose sensitive data if any sensitive fields are present injob.data.this.logger.error('Failed to send webhook through queue system.', { id: job.id, - jobData: job.data, + jobData: { + url: job.data.url, + method: job.data.method, + // Add other non-sensitive fields as needed + }, error: job.data.error, });
🧹 Nitpick comments (4)
services/workflows-service/src/webhooks-incoming/webhooks-incoming.service.ts (1)
72-91: Add type safety for AML hits mapping.Consider adding type definitions for the hit structure to ensure type safety during mapping.
+interface AmlHit { + // Add relevant fields based on your data structure + vendor?: string; +} + -const hits = data?.hits ?? []; +const hits: AmlHit[] = data?.hits ?? []; const amlHits = hits.map(hit => ({ ...hit, vendor: data?.vendor, }));services/workflows-service/src/webhooks/webhooks.service.ts (3)
16-23: Consider enhancing error context in Sentry reporting.The helper function could be improved by:
- Including more structured error context
- Adding error tags for better filtering
const captureWebhookFailureWithSentry = (errorPayload: Record<string, unknown>) => { + const { url, method, ...errorDetails } = errorPayload; Sentry.captureException( new Error('Failed to send a webhook', { cause: errorPayload, }), - { extra: errorPayload }, + { + extra: errorDetails, + tags: { + type: 'webhook_failure', + method, + endpoint: url, + }, + }, ); };
38-63: Consider increasing connection timeout.The current 5-second connection timeout might be too aggressive for some environments, especially in containerized setups where services might take longer to be fully available.
const redis = new IORedis({ ...connection, retryStrategy: () => 10_000, - connectTimeout: 5_000, + connectTimeout: 15_000, maxRetriesPerRequest: null, });
178-180: Enhance module cleanup robustness.Consider adding a timeout to the queue shutdown to prevent hanging.
async onModuleDestroy() { - await this.queue?.shutdown(); + try { + await Promise.race([ + this.queue?.shutdown(), + new Promise((_, reject) => + setTimeout(() => reject(new Error('Queue shutdown timeout')), 5000) + ), + ]); + } catch (error) { + this.logger.error('Error during queue shutdown:', { error }); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
services/workflows-service/src/webhooks-incoming/webhooks-incoming.service.ts(1 hunks)services/workflows-service/src/webhooks/webhooks.service.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test_windows
- GitHub Check: test_linux
- GitHub Check: build (windows-latest)
- GitHub Check: lint
- GitHub Check: Analyze (javascript)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (7)
services/workflows-service/src/webhooks-incoming/webhooks-incoming.service.ts (4)
1-22: LGTM! Well-structured service with proper dependency injection.The service follows best practices with clear dependency injection and includes all necessary services for handling webhooks.
24-31: Add input validation for webhook data.The method should validate the structure of the input data before processing.
32-61: Implement data sanitization for sensitive information.The service processes sensitive user data without proper sanitization.
Wrap database operations in a transaction.
Multiple database operations should be wrapped in a transaction to maintain data consistency.
93-112: Fix type incompatibility instead of suppressing the error.Using @ts-expect-error is a temporary solution that should be properly fixed.
services/workflows-service/src/webhooks/webhooks.service.ts (3)
25-36: LGTM! Clean dependency injection setup.The constructor properly injects required dependencies and delegates initialization to a separate method.
113-118: LGTM! Proper queue system degradation handling.The implementation gracefully handles Redis unavailability by clearing the queue and board instance.
166-172:⚠️ Potential issueSanitize error logging.
Similar to the queue error logging, this error log could expose sensitive data.
this.logger.error('Failed to send webhook', { id: runtimeData.id ?? id, - error: errorPayload, + error: { + status: errorPayload.status, + message: errorPayload.message, + url: errorPayload.url, + method: errorPayload.method, + }, state, entityId, correlationId, });Likely invalid or redundant comment.
| const { config } = await this.customerService.getByProjectId(projectId, { | ||
| select: { config: true }, | ||
| }); | ||
|
|
||
| if (!config?.ongoingWorkflowDefinitionId) { | ||
| this.logger.error('No ongoing workflow definition found for project', { projectId }); | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling for missing workflow definition.
Instead of just logging and silently returning, consider throwing a specific exception that can be caught and handled by the caller.
if (!config?.ongoingWorkflowDefinitionId) {
this.logger.error('No ongoing workflow definition found for project', { projectId });
- return;
+ throw new Error(`No ongoing workflow definition found for project ${projectId}`);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { config } = await this.customerService.getByProjectId(projectId, { | |
| select: { config: true }, | |
| }); | |
| if (!config?.ongoingWorkflowDefinitionId) { | |
| this.logger.error('No ongoing workflow definition found for project', { projectId }); | |
| return; | |
| } | |
| const { config } = await this.customerService.getByProjectId(projectId, { | |
| select: { config: true }, | |
| }); | |
| if (!config?.ongoingWorkflowDefinitionId) { | |
| this.logger.error('No ongoing workflow definition found for project', { projectId }); | |
| throw new Error(`No ongoing workflow definition found for project ${projectId}`); | |
| } |
| try { | ||
| return await this.httpService.axiosRef.request(requestData); | ||
| } catch (error) { | ||
| const { id, state, entityId, correlationId, runtimeData } = data as Record<string, any>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety for data extraction.
The type casting to Record<string, any> could be unsafe. Consider using a type guard or optional chaining.
-const { id, state, entityId, correlationId, runtimeData } = data as Record<string, any>;
+const { id, state, entityId, correlationId, runtimeData } = data as Partial<{
+ id: string;
+ state: string;
+ entityId: string;
+ correlationId: string;
+ runtimeData: { id?: string };
+}>;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { id, state, entityId, correlationId, runtimeData } = data as Record<string, any>; | |
| const { id, state, entityId, correlationId, runtimeData } = data as Partial<{ | |
| id: string; | |
| state: string; | |
| entityId: string; | |
| correlationId: string; | |
| runtimeData: { id?: string }; | |
| }>; |
Summary by CodeRabbit
New Features
Refactor