feat: ledger id null trigger #1526
Conversation
Signed-off-by: Krishna Waske <[email protected]>
Signed-off-by: Krishna Waske <[email protected]>
WalkthroughAdds a DB trigger that NOTIFYs when org_agents.ledgerId becomes NULL; API Gateway LISTENs and publishes a NATS alert when enabled and threshold exceeded; Utility microservice receives the message, rate‑limits and retries email alerts; DTO, DI modules, env samples, and pg dependency updated. Changes
Sequence Diagram(s)sequenceDiagram
participant DB as PostgreSQL
participant Trigger as DB Trigger
participant API as API Gateway (UtilitiesService)
participant NATS as NATS
participant Utility as Utility Microservice
participant Email as EmailService
DB->>Trigger: UPDATE org_agents (ledgerId non-NULL → NULL)
Trigger->>DB: NOTIFY ledger_null (agentId, orgId, ts)
DB-->>API: LISTEN notification delivered
API->>DB: Query totals (total, ledgerId IS NULL)
API->>API: Compute % NULL, check DB_ALERT_ENABLE & threshold
alt Alert required
API->>NATS: publish { cmd: 'alert-db-ledgerId-null', emailDto }
NATS->>Utility: deliver message
Utility->>Utility: enforce cooldown & isSendingAlert guard
Utility->>Email: send email(payload) with retries/backoff
Email-->>Utility: delivery result
else No alert
API-->>API: no further action
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
.env.demo (1)
226-226: Add a trailing newline for consistency.Per dotenv-linter, files should end with a blank line.
# DB_ALERT_EMAILS= +.env.sample (1)
245-247: Good documentation for the new configuration.The comment clearly explains the purpose of
DB_ALERT_EMAILS. Add a trailing newline per dotenv-linter.#Comma separated emails that needs to be alerted in case the 'ledgerId' is set to null # DB_ALERT_EMAILS= +libs/prisma-service/prisma/migrations/20251125062851_add_ledger_null_trigger/migration.sql (1)
4-11: Redundant NULL check inside function.The
IF NEW."ledgerId" IS NULLcheck (Line 5) is redundant since the trigger'sWHENclause (Line 20) already guarantees this condition. The function will only execute whenNEW."ledgerId" IS NULL.BEGIN - IF NEW."ledgerId" IS NULL THEN - PERFORM pg_notify('ledger_null', json_build_object( - 'agentId', NEW.id, - 'orgId', NEW."orgId", - 'timestamp', now() - )::text); - END IF; + PERFORM pg_notify('ledger_null', json_build_object( + 'agentId', NEW.id, + 'orgId', NEW."orgId", + 'timestamp', now() + )::text); RETURN NEW; END;libs/prisma-service/src/prisma-service.service.ts (2)
106-114: Add error handling for email dispatch.If
sendEmailfails (network issue, misconfiguration), the error will propagate and could crash the notification handler. Wrap in try-catch and log the failure.- await this.emailService.sendEmail({ - emailFrom: process.env.PUBLIC_PLATFORM_SUPPORT_EMAIL, - emailTo: alertEmails, - emailSubject: '[ALERT] More than 30% org_agents ledgerId is NULL', - emailText: `ALERT: ${percent.toFixed(2)}% of org_agents records currently have ledgerId = NULL.`, - emailHtml: `<p><strong>ALERT:</strong> ${percent.toFixed( - 2 - )}% of <code>org_agents</code> have <code>ledgerId</code> = NULL.</p>` - }); - - this.logger.log('ALERT EMAIL SENT, ledgerId WAS SET TO NULL'); + try { + await this.emailService.sendEmail({ + emailFrom: process.env.PUBLIC_PLATFORM_SUPPORT_EMAIL, + emailTo: alertEmails, + emailSubject: '[ALERT] More than 30% org_agents ledgerId is NULL', + emailText: `ALERT: ${percent.toFixed(2)}% of org_agents records currently have ledgerId = NULL.`, + emailHtml: `<p><strong>ALERT:</strong> ${percent.toFixed( + 2 + )}% of <code>org_agents</code> have <code>ledgerId</code> = NULL.</p>` + }); + this.logger.log('ALERT EMAIL SENT, ledgerId WAS SET TO NULL'); + } catch (error) { + this.logger.error('Failed to send ledger alert email', error); + }
72-74: Add error handling forpg.end()in cleanup.If
pg.end()throws, it could prevent graceful shutdown. Consider wrapping in try-catch.async onModuleDestroy(): Promise<void> { - await this.pg?.end(); + try { + await this.pg?.end(); + } catch (error) { + this.logger.error('Failed to close PgClient connection', error); + } }
📜 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 (7)
.env.demo(1 hunks).env.sample(1 hunks)libs/common/src/dtos/email.dto.ts(1 hunks)libs/prisma-service/prisma/migrations/20251125062851_add_ledger_null_trigger/migration.sql(1 hunks)libs/prisma-service/src/prisma-service.module.ts(1 hunks)libs/prisma-service/src/prisma-service.service.ts(3 hunks)package.json(1 hunks)
🧰 Additional context used
🪛 dotenv-linter (4.0.0)
.env.demo
[warning] 226-226: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
.env.sample
[warning] 247-247: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
🪛 GitHub Check: SonarCloud Code Analysis
libs/prisma-service/src/prisma-service.service.ts
[warning] 11-11: Member 'pg' is never reassigned; mark it as readonly.
🔇 Additional comments (3)
libs/common/src/dtos/email.dto.ts (1)
1-8: Type change enables multi-recipient support.The
emailTo: string | string[]change supports the new alerting workflow that sends to multiple recipients fromDB_ALERT_EMAILS. Consider adding class-validator decorators for runtime validation if this DTO is used with user input elsewhere.libs/prisma-service/prisma/migrations/20251125062851_add_ledger_null_trigger/migration.sql (1)
17-21: Trigger only fires on UPDATE, not INSERT.The trigger is configured for
AFTER UPDATEonly. If a neworg_agentsrecord is inserted withledgerId = NULL, no notification will be sent. Verify this is the intended behavior—if agents can be created without a ledger initially, you may want anAFTER INSERTtrigger as well.package.json (1)
109-109: Dependency upgrade verified as safe—no breaking changes.The
pgupgrade from ^8.11.2 to ^8.16.3 introduces only backward-compatible features (queryMode, per-query timeouts, SCRAM-SHA-256-PLUS, ESM support, min pool size). The codebase uses CommonJS, so the ESM support addition does not impact it. The upgrade aligns with the newPgClientusage for PostgreSQL LISTEN/NOTIFY and is ready to merge.
Signed-off-by: Krishna Waske <[email protected]>
Signed-off-by: Krishna Waske <[email protected]>
Signed-off-by: Krishna Waske <[email protected]>
Signed-off-by: Krishna Waske <[email protected]>
Signed-off-by: Krishna Waske <[email protected]>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (8)
libs/common/src/resend-helper-file.ts (2)
9-15: Shared Resend client + env guard look fine, but consider fail-fast tradeoffsCreating a single
Resendinstance with a pre-check onRESEND_API_KEYis efficient and keeps misuse from slipping into runtime, but it also means any import of this helper will crash the process if the key is missing (including tests or services that never send email). If you want non-email code paths to work without this env set, consider deferring the check andnew Resend(...)intosendWithResend(lazy init) or guarding usage behind a feature flag.
16-30: Clarify error-handling contract ofsendWithResendRight now failures are logged and surfaced only as
falseto callers, which makes it hard for upstream code to distinguish configuration errors, transient issues, or bad input. Consider either:
- returning a richer result (e.g.,
{ ok: boolean; error?: unknown }), or- throwing on unexpected failures and letting higher layers decide how to handle them,
while still keeping the
Logger.errorcall.apps/api-gateway/src/utilities/utilities.service.ts (3)
10-10: Markpgasreadonly.The field is assigned once in the constructor and never reassigned.
- private pg: PgClient; + private readonly pg: PgClient;
8-20: Implement lifecycle interfaces for type safety.Add
OnModuleInitandOnModuleDestroyinterfaces. This ensures the lifecycle methods have correct signatures and enables better IDE support.-import { Inject, Injectable } from '@nestjs/common'; +import { Inject, Injectable, OnModuleInit, OnModuleDestroy } from '@nestjs/common'; ... -export class UtilitiesService extends BaseService { +export class UtilitiesService extends BaseService implements OnModuleInit, OnModuleDestroy {
35-75: Consider adding rate limiting for alert emails.Every PostgreSQL trigger notification can potentially send an email. If multiple
ledgerIdvalues become NULL in quick succession (e.g., during a batch operation or data migration), this could result in a flood of duplicate alerts.Consider implementing a debounce or cooldown mechanism:
private lastAlertSent: number = 0; private readonly ALERT_COOLDOWN_MS = 60000; // 1 minute // Inside notification handler, before sending: const now = Date.now(); if (now - this.lastAlertSent < this.ALERT_COOLDOWN_MS) { this.logger.debug('Alert cooldown active, skipping email'); return; } this.lastAlertSent = now;.env.sample (1)
246-249: Consider reordering the comments for logical flow.Placing the DB_ALERT_ENABLE explanation and placeholder before DB_ALERT_EMAILS would be more intuitive, as users typically enable a feature before configuring its details.
Apply this diff:
-# Comma separated emails that need to be alerted in case the 'ledgerId' is set to null -# DB_ALERT_EMAILS= # Boolean: to enable/disable db alerts. This needs the 'utility' microservice # DB_ALERT_ENABLE= +# Comma separated emails that need to be alerted in case the 'ledgerId' is set to null +# DB_ALERT_EMAILS=apps/utility/src/utilities.service.ts (1)
77-77: Improve timestamp readability in log message.The log message includes
${this.lastAlertTime}which is milliseconds since epoch. Consider formatting it as a human-readable date for better debugging.Apply this diff:
- this.logger.log(`ALERT EMAIL ALREADY SENT at ${this.lastAlertTime}, ledgerId WAS SET TO NULL`); + this.logger.log(`ALERT EMAIL ALREADY SENT at ${new Date(this.lastAlertTime).toISOString()}, ledgerId WAS SET TO NULL`);apps/utility/src/utilities.controller.ts (1)
35-46: Simplify void method handling.Since
utilitiesService.handleLedgerAlert()returnsPromise<void>, theresultvariable (line 39) is alwaysundefinedand returning it (line 41) is unnecessary.Apply this diff:
@MessagePattern({ cmd: 'alert-db-ledgerId-null' }) async handleLedgerAlert(payload: { emailDto: EmailDto }): Promise<void> { try { this.logger.debug('Received msg in alert-db-service'); - const result = await this.utilitiesService.handleLedgerAlert(payload.emailDto); + await this.utilitiesService.handleLedgerAlert(payload.emailDto); this.logger.debug('Received result in alert-db-service'); - return result; } catch (error) { this.logger.error(error); throw error; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.env.demo(1 hunks).env.sample(1 hunks)apps/api-gateway/src/main.ts(1 hunks)apps/api-gateway/src/utilities/utilities.service.ts(1 hunks)apps/ledger/src/ledger.module.ts(2 hunks)apps/utility/src/utilities.controller.ts(2 hunks)apps/utility/src/utilities.service.ts(1 hunks)libs/common/src/resend-helper-file.ts(1 hunks)libs/org-roles/src/org-roles.module.ts(1 hunks)libs/prisma-service/src/prisma-service.module.ts(1 hunks)libs/prisma-service/src/prisma-service.service.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/prisma-service/src/prisma-service.module.ts
- libs/prisma-service/src/prisma-service.service.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-17T08:03:16.180Z
Learnt from: KambleSahil3
Repo: credebl/platform PR: 1518
File: libs/prisma-service/prisma/data/credebl-master-table/credebl-master-table.json:11-20
Timestamp: 2025-11-17T08:03:16.180Z
Learning: In the credebl/platform repository, seed data files like libs/prisma-service/prisma/data/credebl-master-table/credebl-master-table.json intentionally contain empty strings for sensitive fields (email, username, passwords, API keys, etc.) because these values are populated from .env file at runtime, not hardcoded in the JSON.
Applied to files:
.env.demo
🧬 Code graph analysis (3)
apps/api-gateway/src/main.ts (1)
libs/logger/src/logger.interface.ts (1)
Logger(6-20)
apps/utility/src/utilities.controller.ts (1)
libs/common/src/dtos/email.dto.ts (1)
EmailDto(1-8)
apps/api-gateway/src/utilities/utilities.service.ts (1)
libs/service/base.service.ts (1)
BaseService(2-9)
🪛 dotenv-linter (4.0.0)
.env.demo
[warning] 227-227: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
.env.sample
[warning] 249-249: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
🪛 GitHub Check: SonarCloud Code Analysis
apps/api-gateway/src/utilities/utilities.service.ts
[warning] 10-10: Member 'pg' is never reassigned; mark it as readonly.
🔇 Additional comments (6)
libs/org-roles/src/org-roles.module.ts (1)
1-11: PrismaServiceModule integration in OrgRolesModule looks consistentImporting
PrismaServiceModuleand dropping a directPrismaServiceprovider keeps the DI surface clean while preservingOrgRolesServiceas the sole export. AssumingPrismaServiceModulecorrectly exportsPrismaService, this module wiring is sound.apps/ledger/src/ledger.module.ts (1)
5-35: LedgerModule DI refactor to PrismaServiceModule looks correctAdding
PrismaServiceModuletoimportsand keeping onlyLedgerService,LedgerRepository, andLoggerinprovidersaligns this module with the new Prisma DI pattern without changing its public API.apps/api-gateway/src/utilities/utilities.service.ts (1)
79-81: LGTM!Proper cleanup with optional chaining to handle edge cases where
pgmight not be initialized.apps/utility/src/utilities.controller.ts (1)
35-46: LGTM with minor suggestions above.The message pattern handler follows existing conventions and includes appropriate error handling. The structure is consistent with other handlers in the controller.
apps/api-gateway/src/main.ts (2)
124-130: All DB alert feature components are properly wired end-to-end.Verification confirms:
- ✓ PostgreSQL trigger (
alert_ledger_null) is defined in migration20251125062851_add_ledger_null_triggerand creates theledger_null_triggeron theorg_agentstable- ✓ UtilitiesService subscribes to the notification in
onModuleInit()withLISTEN ledger_null(utilities.service.ts:26) and handles incoming notifications via thepg.on('notification')event handler- ✓ Utility microservice properly receives the NATS message with
@MessagePattern({ cmd: 'alert-db-ledgerId-null' })handler (utilities.controller.ts:35)- ✓
DB_ALERT_EMAILSenvironment variable is properly parsed, trimmed, and filtered in utilities.service.ts:53The log at lines 124-130 in main.ts is a confirmation message and does not indicate incomplete wiring.
122-122: The review comment is incorrect. The second parameter is a context string, not a log level.The NestJS
Logger.log()method signature acceptslog(message: any, context?: string), where the second parameter is an optional context string used to identify the source of the log message. Using'Success'and'DB alert enabled'as context strings is valid and follows standard NestJS Logger usage patterns. No custom log level is being used here.
Signed-off-by: Krishna Waske <[email protected]>
Signed-off-by: Krishna Waske <[email protected]>
Signed-off-by: Krishna Waske <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
apps/api-gateway/src/utilities/utilities.service.ts (3)
41-49: [CRITICAL] Fix division by zero whenorg_agentstable is empty.If the
org_agentstable contains no records,totalwill be 0, causing division by zero on line 49. This results inpercentbeingInfinityorNaN, which will bypass the threshold check and likely cause downstream errors when sending the alert.This issue was flagged in the previous review as critical but has not been addressed.
Apply this diff to guard against division by zero:
const totalRes = await this.pg.query('SELECT COUNT(*) FROM org_agents'); const total = Number(totalRes.rows[0].count); + + if (total === 0) { + this.logger.debug('No org_agents records found, skipping alert check'); + return; + } // Step 2: Count NULL ledgerId records const nullRes = await this.pg.query('SELECT COUNT(*) FROM org_agents WHERE "ledgerId" IS NULL'); const nullCount = Number(nullRes.rows[0].count); // Step 3: Calculate % const percent = (nullCount / total) * 100;
56-69: [MINOR] Validate required environment variables before constructing email DTO.If
DB_ALERT_EMAILSis empty orPUBLIC_PLATFORM_SUPPORT_EMAILis undefined, the email will be sent with no recipients or sender, likely causing a silent failure or error in the email service. Validating these environment variables before sending the alert provides clearer error messages.This issue was flagged in the previous review as minor but has not been addressed.
Apply this diff to add validation:
const alertEmails = process.env.DB_ALERT_EMAILS?.split(',') .map((e) => e.trim()) .filter((e) => 0 < e.length) || []; + if (alertEmails.length === 0) { + this.logger.warn('DB_ALERT_EMAILS is empty or not configured, skipping alert'); + return; + } + + if (!process.env.PUBLIC_PLATFORM_SUPPORT_EMAIL) { + this.logger.warn('PUBLIC_PLATFORM_SUPPORT_EMAIL not configured, skipping alert'); + return; + } const emailDto = {
39-80: [MAJOR] Wrap all database queries in try-catch to prevent unhandled rejections.The database queries on lines 41 and 45 can throw errors that will propagate as unhandled promise rejections, potentially crashing the process or leaving the notification listener in an inconsistent state. While the NATS send operation is wrapped in try-catch (lines 71-78), the queries themselves are not protected.
This issue was flagged in the previous review as major but has not been addressed.
Apply this diff to wrap the entire notification handler logic in try-catch:
if ('ledger_null' === msg.channel) { + try { // Step 1: Count total records const totalRes = await this.pg.query('SELECT COUNT(*) FROM org_agents'); const total = Number(totalRes.rows[0].count); + + if (total === 0) { + this.logger.debug('No org_agents records found, skipping alert check'); + return; + } // Step 2: Count NULL ledgerId records const nullRes = await this.pg.query('SELECT COUNT(*) FROM org_agents WHERE "ledgerId" IS NULL'); const nullCount = Number(nullRes.rows[0].count); // Step 3: Calculate % const percent = (nullCount / total) * 100; // Condition: > 30% if (30 >= percent) { return; } const alertEmails = process.env.DB_ALERT_EMAILS?.split(',') .map((e) => e.trim()) .filter((e) => 0 < e.length) || []; const emailDto = { emailFrom: process.env.PUBLIC_PLATFORM_SUPPORT_EMAIL, emailTo: alertEmails, emailSubject: '[ALERT] More than 30% org_agents ledgerId is NULL', emailText: `ALERT: ${percent.toFixed(2)}% of org_agents records currently have ledgerId = NULL.`, emailHtml: `<p><strong>ALERT:</strong> ${percent.toFixed( 2 )}% of <code>org_agents</code> have <code>ledgerId</code> = NULL.</p>` }; - try { - const result = await this.natsClient.sendNatsMessage(this.serviceProxy, 'alert-db-ledgerId-null', { - emailDto - }); - this.logger.debug('Received result', JSON.stringify(result, null, 2)); - } catch (err) { - this.logger.error(err?.message ?? 'Some error occurred while sending prisma ledgerId alert email'); - } + const result = await this.natsClient.sendNatsMessage(this.serviceProxy, 'alert-db-ledgerId-null', { + emailDto + }); + this.logger.debug('Received result', JSON.stringify(result, null, 2)); + } catch (err) { + this.logger.error(`Error in ledger_null notification handler: ${err?.message}`, err?.stack); + } }
🧹 Nitpick comments (2)
apps/api-gateway/src/main.ts (1)
122-130: DB alert bootstrap logging is correct; consider refining log context labelsThe new logs and the
DB_ALERT_ENABLEflag handling look correct and give useful visibility into DB alert configuration. To make these logs easier to filter/search, consider using more specific, stable context strings instead of generic ones like'Success'and'DB alert enabled'(for example,'API_GATEWAY_BOOTSTRAP'and'DB_ALERT_LEDGER_NULL'), aligned with how other logs are tagged in this service.apps/api-gateway/src/utilities/utilities.service.ts (1)
10-10: Consider markingpgasreadonly.The
pgfield is assigned once in the constructor and never reassigned. Marking it asreadonlyimproves type safety and documents the immutability constraint.Based on static analysis hints.
Apply this diff:
- private pg: PgClient; + private readonly pg: PgClient;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api-gateway/src/main.ts(1 hunks)apps/api-gateway/src/utilities/utilities.service.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/api-gateway/src/main.ts (1)
libs/logger/src/logger.interface.ts (1)
Logger(6-20)
🪛 GitHub Check: SonarCloud Code Analysis
apps/api-gateway/src/utilities/utilities.service.ts
[warning] 10-10: Member 'pg' is never reassigned; mark it as readonly.
🔇 Additional comments (2)
apps/api-gateway/src/utilities/utilities.service.ts (2)
22-31: LGTM! Error handling properly implemented.The connection and LISTEN setup are now wrapped in try-catch with clear logging and proper error propagation. This addresses the previous review feedback.
83-85: LGTM! Proper cleanup of PostgreSQL connection.The optional chaining ensures safe cleanup even if the connection was never established, and using the lifecycle hook is the appropriate pattern for resource cleanup.
Signed-off-by: Krishna Waske <[email protected]>
Signed-off-by: Krishna Waske <[email protected]>
Signed-off-by: Krishna Waske <[email protected]>
Signed-off-by: Krishna Waske <[email protected]>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Signed-off-by: Krishna Waske <[email protected]>
Signed-off-by: Krishna Waske <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/utility/src/utilities.service.ts (1)
76-129: Email alert retry flow is correct, but in‑memory rate limiting is still fragile across restarts/instances.The new
handleLedgerAlert+sendWithRetryflow looks logically sound:
isSendingAlertprevents overlapping retry flows.lastAlertTimeis updated only after a successful send.- The handler returns immediately without blocking the caller.
However, the existing concerns about the in‑memory rate limit remain:
- The 2‑hour window is hard‑coded (
2 * 60 * 60 * 1000) instead of being configurable (e.g., env‑driven).lastAlertTimeis per‑process and lost on restart.- In a horizontally scaled deployment, each instance enforces its own window, so multiple instances can all send alerts after the same DB event.
If this alerting is operationally important, consider backing the rate limit with a shared store (e.g., Redis with
SET NX PXor similar) and driving the window from configuration, keeping the current in‑memory fields as a fallback/cache.
🧹 Nitpick comments (3)
apps/utility/src/utilities.service.ts (1)
60-74: Consider surfacing richer error details and/or usingRpcExceptionforstoreObject.Right now a failed S3 upload logs the raw error and throws a generic
Error('An error occurred while uploading data to S3. Error::::::'). If this path is used over NATS like the URL methods, you may want to:
- Throw
RpcException(or a more specific error) for symmetry, and/or- Include at least a concise cause message to aid callers, while still avoiding sensitive data.
Not urgent, but worth aligning behavior across methods.
apps/api-gateway/src/utilities/utilities.service.ts (2)
10-25: Makepgreadonly and avoid constructing it when alerts are disabled.
pgis only assigned in the constructor and never reassigned, so it can be markedreadonly. Also, the currentif/elsealways instantiatesPgClientin theelsebranch, even whenDB_ALERT_ENABLEis not'true'(and possibly with an undefinedconnectionStringthat will never be used).You could simplify and avoid creating an unused client like this:
private readonly pg?: PgClient; constructor( @Inject('NATS_CLIENT') private readonly serviceProxy: ClientProxy, private readonly natsClient: NATSClient ) { super('UtilitiesService'); const alertsEnabled = 'true' === process.env.DB_ALERT_ENABLE?.trim()?.toLowerCase(); if (!alertsEnabled) { return; } if (!process.env.DATABASE_URL) { throw new Error('DATABASE_URL environment variable is required'); } this.pg = new PgClient({ connectionString: process.env.DATABASE_URL }); }Then guard
this.pginonModuleInit/onModuleDestroy(e.g.,if (!this.pg) return;). This keeps construction aligned with feature enablement and satisfies the Sonar hint aboutpgbeing effectively immutable.
78-105: Resolve or externalize the TODO aboutemailTosource.The TODO (
Check if the to email is actually this or we need to take it from DB) indicates uncertainty about whetherDB_ALERT_EMAILSis the correct recipient source versus a DB‑driven value. Given this is an operational alert path, it’s worth either:
- Implementing the intended behavior (e.g., fetching recipients from configuration/DB), or
- Capturing a clear decision (e.g., “environment‑only is fine”) and replacing the TODO with a comment or a tracked ticket ID.
As long as the TODO remains, it’s easy for this ambiguity to be forgotten.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api-gateway/src/utilities/utilities.service.ts(1 hunks)apps/utility/src/utilities.service.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/api-gateway/src/utilities/utilities.service.ts (2)
apps/api-gateway/src/platform/platform.service.ts (1)
Injectable(11-46)apps/api-gateway/src/user/user.service.ts (1)
Injectable(16-95)
🪛 GitHub Check: SonarCloud Code Analysis
apps/api-gateway/src/utilities/utilities.service.ts
[warning] 91-91: Complete the task associated to this "TODO" comment.
[warning] 10-10: Member 'pg' is never reassigned; mark it as readonly.
🔇 Additional comments (4)
apps/utility/src/utilities.service.ts (2)
1-22: Service wiring and dependency injection look consistent.Extending
BaseServiceand injectingUtilitiesRepository,AwsService, andEmailServicevia the constructor matches existing patterns in the codebase and gives you a consistent logger context viasuper('UtilitiesService').
24-58: Shortening URL methods are straightforward and consistent with existing patterns.
createAndStoreShorteningUrlandgetShorteningUrlcorrectly wrap repository calls, log with contextual prefixes, and surface errors viaRpcException; the structure looks sound.apps/api-gateway/src/utilities/utilities.service.ts (2)
27-40:onModuleIniterror handling and feature flag gating look good.The DB alert listener only connects when
DB_ALERT_ENABLEis'true', and failures duringconnect/LISTENare logged and rethrown so the module fails fast with a clear message; the structure is solid.
118-119: Graceful PostgreSQL shutdown is fine; just ensurepgexistence under the refactor.
await this.pg?.end();is a clean way to close the client. If you adopt the optionalpgconstruction pattern above, this remains safe; just make surepgis declared as possibly undefined (PgClient | undefined) to keep the typing consistent.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/api-gateway/src/utilities/utilities.service.ts (1)
41-115: Critical:isSendingNatsAlertflag is never reset, permanently blocking alerts.This issue was flagged in the previous review but remains unaddressed. On line 54,
isSendingNatsAlertis set totruebut is never reset tofalse. After the first notification that passes the guard, all subsequent alerts are permanently skipped (line 50 logs "Skipping duplicate NATS alert send...") even if:
- The previous send failed
- The percentage changed
- Early returns occurred (lines 63, 75, 88, 94)
Wrap the flag reset in a
finallyblock to ensure it's cleared on all code paths:if ('ledger_null' === msg.channel) { try { if (this.isSendingNatsAlert) { this.logger.warn('Skipping duplicate NATS alert send...'); return; } this.isSendingNatsAlert = true; // Step 1: Count total records const totalRes = await this.pg.query('SELECT COUNT(*) FROM org_agents'); // ... rest of the logic ... this.logger.debug('Received result', JSON.stringify(result, null, 2)); } catch (err) { this.logger.error(err?.message ?? 'Error in ledgerId alert handler'); + } finally { + this.isSendingNatsAlert = false; } }
🧹 Nitpick comments (2)
apps/api-gateway/src/utilities/utilities.service.ts (2)
42-45: Consider removing redundantDB_ALERT_ENABLEcheck.The guard on lines 42-45 appears redundant because
onModuleInitalready returns early ifDB_ALERT_ENABLEis not'true'(line 31), preventing the notification handler from being registered. If this guard is intended as defense-in-depth against runtime environment changes, consider adding a comment to clarify.
91-91: Resolve the TODO comment about email recipient source.The TODO on line 91 questions whether alert emails should come from
DB_ALERT_EMAILSenvironment variable or the database. Clarify the requirement and remove the TODO before merging.Do you want me to help implement database-driven email configuration, or should this TODO be removed if the environment variable approach is confirmed?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api-gateway/src/utilities/utilities.service.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/api-gateway/src/utilities/utilities.service.ts (2)
apps/api-gateway/src/platform/platform.service.ts (1)
Injectable(11-46)libs/service/base.service.ts (1)
BaseService(2-9)
🪛 GitHub Check: SonarCloud Code Analysis
apps/api-gateway/src/utilities/utilities.service.ts
[warning] 91-91: Complete the task associated to this "TODO" comment.
Signed-off-by: Krishna Waske <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/api-gateway/src/utilities/utilities.service.ts (2)
18-24: PgClient is still initialized when DB_ALERT_ENABLE is false.The current conditional logic creates a
PgClientinstance even whenDB_ALERT_ENABLEis not'true', potentially with an undefinedDATABASE_URL. This past review comment was marked as addressed but the fix was not fully applied.The condition should be restructured so the client is only created when alerting is enabled:
Apply this diff:
- if ('true' === process.env.DB_ALERT_ENABLE?.trim()?.toLowerCase() && !process.env.DATABASE_URL) { - throw new Error('DATABASE_URL environment variable is required'); - } else { + if ('true' === process.env.DB_ALERT_ENABLE?.trim()?.toLowerCase()) { + if (!process.env.DATABASE_URL) { + throw new Error('DATABASE_URL environment variable is required'); + } this.pg = new PgClient({ connectionString: process.env.DATABASE_URL }); }
42-42: UsetoLowerCase()for consistency.Line 42 uses
toLocaleLowerCase()while lines 18 and 29 usetoLowerCase(). For simple ASCII string comparison like'true', usetoLowerCase()consistently throughout the file.Apply this diff:
- if ('true' !== process.env.DB_ALERT_ENABLE?.trim()?.toLocaleLowerCase()) { + if ('true' !== process.env.DB_ALERT_ENABLE?.trim()?.toLowerCase()) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api-gateway/src/utilities/utilities.service.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/api-gateway/src/utilities/utilities.service.ts (1)
libs/service/base.service.ts (1)
BaseService(2-9)
🪛 GitHub Check: SonarCloud Code Analysis
apps/api-gateway/src/utilities/utilities.service.ts
[warning] 91-91: Complete the task associated to this "TODO" comment.
🔇 Additional comments (6)
apps/api-gateway/src/utilities/utilities.service.ts (6)
27-39: LGTM - Connection setup with proper error handling.The early return when
DB_ALERT_ENABLEis not enabled (lines 29-32) and the error handling with logging and rethrow (lines 36-39) correctly implement the requirements from past reviews.
49-76: LGTM - Alert logic with proper guards.The deduplication guard (lines 49-52), division-by-zero protection (lines 61-64), and percentage threshold check (line 74) correctly implement the alert logic with appropriate early returns.
78-95: LGTM - Email validation with clear error logging.The code properly validates that
DB_ALERT_EMAILScontains recipients (lines 83-89) and thatPUBLIC_PLATFORM_SUPPORT_EMAILis configured (lines 92-95), with helpful error messages that include the calculated percentage.
97-110: LGTM - Email DTO construction and NATS dispatch.The email payload structure is clear and includes both text and HTML versions with formatted percentage values. The NATS message send uses an appropriate topic
'alert-db-ledgerId-null'.
111-116: LGTM - Error handling with flag reset in finally block.The
finallyblock (lines 113-116) correctly ensures thatisSendingNatsAlertis reset even when early returns or errors occur within the try block, addressing the concern from previous reviews.
121-123: LGTM - Proper cleanup with optional chaining.The optional chaining
this.pg?.end()safely handles cases where the PostgreSQL client wasn't initialized (whenDB_ALERT_ENABLEis false), ensuring graceful module destruction.
Signed-off-by: Krishna Waske <[email protected]>
Signed-off-by: Krishna Waske <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/api-gateway/src/utilities/utilities.service.ts (2)
43-48: UsetoLowerCase()consistently forDB_ALERT_ENABLE.Line 44 uses
toLocaleLowerCase()while lines 18 and 31 usetoLowerCase(). For this simple'true'flag, usingtoLowerCase()everywhere is clearer and avoids locale-dependent behavior:- if ('true' !== process.env.DB_ALERT_ENABLE?.trim()?.toLocaleLowerCase()) { + if ('true' !== process.env.DB_ALERT_ENABLE?.trim()?.toLowerCase()) {
93-97: Resolve or remove the TODO about recipient source before merging.This TODO (“Check if the to email is actually this or we need to take it from DB”) is still present and Sonar is warning on it. Either:
- Confirm that env-driven recipients are the intended behavior, document that decision (and remove the TODO), or
- Implement the DB-driven recipient lookup (or open a tracked ticket) and wire it here.
Leaving this TODO in main will keep the static-analysis warning around and leaves business behavior ambiguous.
🧹 Nitpick comments (1)
apps/api-gateway/src/utilities/utilities.service.ts (1)
10-27: Optional: simplifyDATABASE_URLcheck to avoid negated condition/else.Logic is fine, but the nested
if (!process.env.DATABASE_URL) { ... } else { ... }hurts readability and is what Sonar calls an “unexpected negated condition.” You can invert it and drop theelse:- if ('true' === process.env.DB_ALERT_ENABLE?.trim()?.toLowerCase()) { - if (!process.env.DATABASE_URL) { - throw new Error('DATABASE_URL environment variable is required'); - } else { - this.pg = new PgClient({ - connectionString: process.env.DATABASE_URL - }); - } - } + if ('true' === process.env.DB_ALERT_ENABLE?.trim()?.toLowerCase()) { + const databaseUrl = process.env.DATABASE_URL; + if (!databaseUrl) { + throw new Error('DATABASE_URL environment variable is required'); + } + this.pg = new PgClient({ connectionString: databaseUrl }); + }This keeps behavior identical but satisfies the static-analysis concern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api-gateway/src/utilities/utilities.service.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: SonarCloud Code Analysis
apps/api-gateway/src/utilities/utilities.service.ts
[warning] 93-93: Complete the task associated to this "TODO" comment.
[warning] 19-19: Unexpected negated condition.
🔇 Additional comments (3)
apps/api-gateway/src/utilities/utilities.service.ts (3)
29-42: PG listener initialization and error handling look solid.Guarding on
DB_ALERT_ENABLE, wrappingconnect/LISTENin atry/catch, logging, and rethrowing on failure give a clear, fail-fast startup path for the alert listener. No issues here.
49-91: LedgerId NULL-percentage computation and alert gating look correct.The flow here—guard on
isSendingNatsAlertwith afinallyreset, count total vs NULLledgerId, skip whentotal === 0, computepercent, and only send whenpercent > 30—is coherent and fixes the earlier “flag stuck to true” issue. The additional checks for non-emptyDB_ALERT_EMAILSand logging when empty give good observability.
123-124: Graceful PG client shutdown matches conditional initialization.Using
await this.pg?.end();is a good fit with the conditionalPgClientcreation; it safely shuts down the client only when it was actually initialized.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/api-gateway/src/utilities/utilities.service.ts (1)
93-93: TODO comment requires resolution before merge.This TODO was flagged in a previous review and by static analysis. Please either confirm the current approach (using
PUBLIC_PLATFORM_SUPPORT_EMAIL) is correct and remove the TODO, or implement the database lookup if needed.
🧹 Nitpick comments (2)
apps/api-gateway/src/utilities/utilities.service.ts (2)
18-26: Remove unnecessaryelseafterthrow.Since line 20 throws, execution never reaches line 21's
else. Removing theelseimproves readability and addresses the static analysis hint about unexpected negated conditions.if ('true' === process.env.DB_ALERT_ENABLE?.trim()?.toLowerCase()) { if (!process.env.DATABASE_URL) { throw new Error('DATABASE_URL environment variable is required'); - } else { - this.pg = new PgClient({ - connectionString: process.env.DATABASE_URL - }); } + this.pg = new PgClient({ + connectionString: process.env.DATABASE_URL + }); }
43-47: Redundant DB_ALERT_ENABLE check inside notification handler.This check can never be false when the handler executes:
- If
DB_ALERT_ENABLEis false,onModuleInitreturns early (line 33) before registering this handler- The handler is only registered when alerting is enabled
Consider removing this dead code for clarity.
this.pg.on('notification', async (msg) => { - if ('true' !== process.env.DB_ALERT_ENABLE?.trim()?.toLowerCase()) { - // in case it is not enabled, return - return; - } - if ('ledger_null' === msg.channel) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api-gateway/src/utilities/utilities.service.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: SonarCloud Code Analysis
apps/api-gateway/src/utilities/utilities.service.ts
[warning] 93-93: Complete the task associated to this "TODO" comment.
[warning] 19-19: Unexpected negated condition.
🔇 Additional comments (3)
apps/api-gateway/src/utilities/utilities.service.ts (3)
29-41: LGTM!The initialization logic correctly handles the disabled case with an early return, and the try/catch provides proper error handling with logging before rethrowing.
113-118: LGTM on the finally block fix!The
finallyblock correctly ensuresisSendingNatsAlertis reset regardless of the exit path (success, early return, or exception), preventing the flag from being permanently stuck.
123-125: LGTM!The optional chaining on
this.pg?.end()correctly handles the case where alerting is disabled andthis.pgwas never initialized.
Signed-off-by: Krishna Waske <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
apps/api-gateway/src/utilities/utilities.service.ts (2)
18-26: Simplify the nested condition to address SonarCloud warning.The
elseblock is unnecessary since thethrowstatement would prevent further execution. This also addresses the static analysis warning about the negated condition.Apply this diff:
if ('true' === process.env.DB_ALERT_ENABLE?.trim()?.toLowerCase()) { if (!process.env.DATABASE_URL) { throw new Error('DATABASE_URL environment variable is required'); - } else { - this.pg = new PgClient({ - connectionString: process.env.DATABASE_URL - }); } + this.pg = new PgClient({ + connectionString: process.env.DATABASE_URL + }); }
76-78: Consider using standard comparison order for readability.Yoda conditions (
30 >= percent) are typically used for equality checks to prevent accidental assignment. For comparison operators, standard order is more intuitive.- if (30 >= percent) { + if (percent <= 30) { return; }apps/utility/src/utilities.repository.ts (3)
13-13: Add type annotation for thepayloadparameter.The
payloadparameter lacks type information, reducing type safety. Consider defining an interface for the expected structure.- async saveShorteningUrl(payload): Promise<object> { + async saveShorteningUrl(payload: { referenceId: string; invitationPayload: object }): Promise<object> {
31-31: Add type annotation for thereferenceIdparameter.- async getShorteningUrl(referenceId): Promise<shortening_url> { + async getShorteningUrl(referenceId: string): Promise<shortening_url> {
52-54: Return type should reflect nullable result.
findFirst()can returnnullwhen no records exist. The return type should accurately reflect this to ensure callers handle the null case.- async getPlatformConfigDetails(): Promise<platform_config> { + async getPlatformConfigDetails(): Promise<platform_config | null> { try { return this.prisma.platform_config.findFirst();Note: The caller in
UtilitiesService.handleLedgerAlertcorrectly checks for null, but accurate typing helps prevent future bugs.apps/utility/src/utilities.service.ts (1)
99-103: Add error handler to prevent unhandled promise rejections.While
sendWithRetryhas internal try/catch, adding a.catch()handler ensures robustness against any unexpected rejections and follows best practices for fire-and-forget promises.// 3. Start async retry flow — do not block the caller this.isSendingAlert = true; - this.sendWithRetry(emailDto).finally(() => { - this.isSendingAlert = false; - }); + this.sendWithRetry(emailDto) + .catch((err) => { + this.logger.error('Unexpected error in sendWithRetry:', err); + }) + .finally(() => { + this.isSendingAlert = false; + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/api-gateway/src/utilities/utilities.service.ts(1 hunks)apps/utility/src/utilities.repository.ts(1 hunks)apps/utility/src/utilities.service.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apps/utility/src/utilities.service.ts (4)
apps/utility/src/utilities.repository.ts (1)
getShorteningUrl(31-45)libs/service/base.service.ts (1)
BaseService(2-9)apps/utility/src/utilities.controller.ts (1)
getShorteningUrl(20-22)libs/common/src/dtos/email.dto.ts (1)
EmailDto(1-8)
apps/utility/src/utilities.repository.ts (1)
apps/connection/src/connection.repository.ts (1)
storeShorteningUrl(201-213)
apps/api-gateway/src/utilities/utilities.service.ts (1)
libs/service/base.service.ts (1)
BaseService(2-9)
🪛 GitHub Check: SonarCloud Code Analysis
apps/api-gateway/src/utilities/utilities.service.ts
[warning] 19-19: Unexpected negated condition.
🔇 Additional comments (2)
apps/api-gateway/src/utilities/utilities.service.ts (1)
49-112: LGTM! Well-structured notification handler with proper safeguards.The implementation correctly handles:
- Dedup guard to prevent concurrent alerts
- Division by zero protection when
org_agentsis empty- Threshold check before sending alerts
- Environment variable validation
finallyblock to reset the flag regardless of outcomeapps/utility/src/utilities.service.ts (1)
106-135: LGTM! Well-implemented retry logic.The implementation correctly:
- Uses a bounded retry loop with configurable attempts and delay
- Updates
lastAlertTimeonly after successful email send (addresses previous review feedback)- Logs appropriately at each attempt
- Uses exponential-backoff-friendly delay between retries
Signed-off-by: Krishna Waske <[email protected]>
Signed-off-by: Krishna Waske <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/utility/src/utilities.service.ts (1)
83-110: In-memory rate limiting and concurrency guards remain unreliable across instances/restarts.The
lastAlertTimeandisSendingAlertstate will reset on service restart and won't synchronize across horizontally-scaled instances, potentially leading to duplicate alerts or bypassed rate limits.Additionally, the fire-and-forget pattern (line 107) means the caller receives no feedback on email delivery success. If observability is required, consider emitting metrics or events on success/failure.
Confirm whether distributed rate limiting (e.g., Redis with atomic SET NX PX) is required for production, or if single-instance deployment makes in-memory state acceptable.
🧹 Nitpick comments (3)
apps/utility/src/utilities.service.ts (3)
25-25: Consider adding explicit type forpayloadparameter.The
payloadparameter is implicitly typed asany. Adding an explicit interface or type would improve type safety and documentation.- async createAndStoreShorteningUrl(payload): Promise<string> { + async createAndStoreShorteningUrl(payload: { + credentialId: string; + schemaId: string; + credDefId: string; + invitationUrl: string; + attributes: unknown; + }): Promise<string> {
65-81: LGTM with minor consistency note.The error message now properly includes the actual error details. However, other methods in this service throw
RpcExceptionwhile this method throws a genericError. Consider usingRpcExceptionfor consistency if this is invoked via microservice messaging.} catch (error) { this.logger.error(error); - throw new Error( + throw new RpcException( `An error occurred while uploading data to S3: ${error instanceof Error ? error?.message : error}` ); }
112-141: Rate-limit timing issue addressed; consider exponential backoff.The
lastAlertTimeis now correctly updated only after successful email delivery (line 122), addressing the previous concern.The fixed 3-second delay works, but exponential backoff (e.g.,
delayMs * attemptordelayMs * 2^(attempt-1)) would be more resilient against transient failures that need longer recovery windows.// Wait before retrying - await new Promise((resolve) => setTimeout(resolve, delayMs)); + const backoffDelay = delayMs * Math.pow(2, attempt - 1); // Exponential backoff + await new Promise((resolve) => setTimeout(resolve, backoffDelay)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/utility/src/utilities.service.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/utility/src/utilities.service.ts (4)
apps/utility/src/utilities.repository.ts (2)
Injectable(6-60)getShorteningUrl(31-45)libs/aws/src/aws.service.ts (1)
Injectable(6-115)libs/service/base.service.ts (1)
BaseService(2-9)libs/common/src/dtos/email.dto.ts (1)
EmailDto(1-8)
🔇 Additional comments (1)
apps/utility/src/utilities.service.ts (1)
45-63: LGTM! Null handling addressed.The null check at lines 49-51 correctly handles the case when the repository returns no result, throwing a clear
NotFoundExceptionwith a descriptive message.
GHkrishna
requested review from
tipusinghaw
and removed request for
RinkalBhojani
Signed-off-by: Krishna Waske <[email protected]>
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
libs/common/src/common.constant.ts (1)
388-388: Consider making the alert threshold configurable via environment variable.The 30% threshold is currently hardcoded. For operational flexibility, consider reading this from an environment variable with the current value as a fallback default (e.g.,
Number(process.env.DB_ALERT_THRESHOLD_PERCENTAGE || '30')). This allows teams to adjust alert sensitivity without code changes.apps/api-gateway/src/utilities/utilities.service.ts (2)
30-30: Resolve or remove the TODO comment.The TODO raises a valid architectural point about centralizing event listening and email logic. Before merging, either:
- Implement the suggested refactoring if it's critical for this PR, or
- Create a follow-up issue to track the work and reference it in the comment, or
- Remove the TODO if the current approach is intentional and sufficient
Static analysis has flagged this as incomplete.
46-49: Consider removing the redundantDB_ALERT_ENABLEcheck.This check inside the notification handler appears redundant. If
DB_ALERT_ENABLEis not'true',onModuleInitreturns early (lines 33-36) and never registers this handler. Unless you plan to support runtime toggling of the flag (which would require additional hot-reload logic), this check can be removed to simplify the code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api-gateway/src/utilities/utilities.service.ts(1 hunks)libs/common/src/common.constant.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: SonarCloud Code Analysis
apps/api-gateway/src/utilities/utilities.service.ts
[warning] 20-20: Unexpected negated condition.
[warning] 30-30: Complete the task associated to this "TODO" comment.
🔇 Additional comments (1)
apps/api-gateway/src/utilities/utilities.service.ts (1)
95-103: Verify that emptyemailFromis acceptable.The
emailFromfield is set to an empty string. Ensure that the downstream email service (utility microservice) either:
- Populates a default sender address when
emailFromis empty, or- Accepts empty sender and uses a configured default
If the email service requires a valid sender, populate
emailFromwithprocess.env.PUBLIC_PLATFORM_SUPPORT_EMAILor another appropriate address.
* feat: alert via email while ledgerId gets set to null Signed-off-by: Krishna Waske <[email protected]> * feat: alert via email while ledgerId gets set to null Signed-off-by: Krishna Waske <[email protected]> * fix: add pg query listen Signed-off-by: Krishna Waske <[email protected]> * fix: multiple event listner handle Signed-off-by: Krishna Waske <[email protected]> * fix: log enabling of alerts Signed-off-by: Krishna Waske <[email protected]> * fix: add env demo and sample events Signed-off-by: Krishna Waske <[email protected]> * fix: remove unwanted imports Signed-off-by: Krishna Waske <[email protected]> * fix: change toLocaleLowercase to toLowerCase Signed-off-by: Krishna Waske <[email protected]> * fix: gracefully handle pg connect Signed-off-by: Krishna Waske <[email protected]> * fix: increase try catch scope Signed-off-by: Krishna Waske <[email protected]> * fix: handle missing env variables gracefully Signed-off-by: Krishna Waske <[email protected]> * fix: handle positive scenario properly Signed-off-by: Krishna Waske <[email protected]> * fix: handle positive scenario properly Signed-off-by: Krishna Waske <[email protected]> * fix: handle empty org_agents table in alerts Signed-off-by: Krishna Waske <[email protected]> * fix: handle retry logic and multiple notifications for send email Signed-off-by: Krishna Waske <[email protected]> * fix: make pg obj readonly Signed-off-by: Krishna Waske <[email protected]> * finally reset flag Signed-off-by: Krishna Waske <[email protected]> * fix: Only initialize PgClient when DB_ALERT_ENABLE is true Signed-off-by: Krishna Waske <[email protected]> * fix: toLowerLocaleCase to toLowerCase Signed-off-by: Krishna Waske <[email protected]> * fix: TODOs regarding email from platformconfig Signed-off-by: Krishna Waske <[email protected]> * fix: cosmetic changes and missing env variable add in sample and demo Signed-off-by: Krishna Waske <[email protected]> * fix: minor code rabbit changes Signed-off-by: Krishna Waske <[email protected]> * fix: percentage threshold from common constants Signed-off-by: Krishna Waske <[email protected]> --------- Signed-off-by: Krishna Waske <[email protected]> Signed-off-by: pranalidhanavade <[email protected]>
What
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.