refactor: fetch api-key from .env and call agent token API #1473
Conversation
WalkthroughRemoved AFJ token capture, extraction, file creation, printing, and cleanup across provisioning scripts and services; agent endpoint (CONTROLLER_ENDPOINT) is now the single persisted artifact. Added DB-alerting feature (Postgres trigger, listener, NATS email alert flow), updated Prisma module wiring, tweaked DTOs/constants, and bumped Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Postgres
participant PrismaTrigger as DB Trigger
participant ApiGateway as Api-Gateway UtilitiesService
participant Repo as UtilitiesRepository
participant NATS
participant UtilitySvc as Utility Service (handleLedgerAlert)
participant EmailSvc as EmailService
Postgres->>PrismaTrigger: org_agents UPDATE (ledgerId becomes NULL)
PrismaTrigger->>Postgres: pg_notify('ledger_null', payload)
Postgres->>ApiGateway: notification on channel 'ledger_null'
ApiGateway->>Repo: count total org_agents / count ledgerId IS NULL
Repo-->>ApiGateway: counts
ApiGateway->>NATS: publish alert payload (subject: utilities.alert-db-ledgerId-null)
NATS->>UtilitySvc: message delivered (alert-db-ledgerId-null)
UtilitySvc->>Repo: getPlatformConfigDetails()
Repo-->>UtilitySvc: platform config (emailFrom etc.)
UtilitySvc->>EmailSvc: sendWithRetry(emailDto)
EmailSvc-->>UtilitySvc: send result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/common/src/common.service.ts (1)
311-325: Validate AGENT_API_KEY before use.The
AGENT_API_KEYis read from the environment without validation. If it's undefined or empty, it will be passed to the Authorization header, potentially causing unclear failures downstream.Apply this diff to add validation:
async getBaseAgentToken(agentEndPoint: string): Promise<string> { const normalizedBaseUrl = await this.normalizeUrlWithProtocol(agentEndPoint); this.logger.log(`Fetching base agent token from ${normalizedBaseUrl}`); const apikey = process.env.AGENT_API_KEY; + if (!apikey) { + throw new BadRequestException('AGENT_API_KEY environment variable is not configured'); + } const agentBaseWalletDetils = await this.httpPost(`${normalizedBaseUrl}${CommonConstants.URL_AGENT_TOKEN}`, '', { headers: { Accept: 'application/json', Authorization: apikey } }); if (!agentBaseWalletDetils) { throw new NotFoundException(ResponseMessages.common.error.fetchBaseWalletToken); } return agentBaseWalletDetils.token; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/agent-provisioning/AFJ/scripts/start_agent.sh(0 hunks)apps/agent-provisioning/src/agent-provisioning.service.ts(2 hunks)apps/agent-service/src/agent-service.service.ts(1 hunks)libs/common/src/common.service.ts(2 hunks)
💤 Files with no reviewable changes (1)
- apps/agent-provisioning/AFJ/scripts/start_agent.sh
🔇 Additional comments (1)
apps/agent-service/src/agent-service.service.ts (1)
529-529: LGTM! Token retrieval now environment-based.The updated call to
getBaseAgentTokenwith a single argument correctly aligns with the refactored signature that retrieves the API key from environment variables.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/agent-provisioning/src/agent-provisioning.service.ts (1)
45-45: Validate AGENT_API_KEY before use
In apps/agent-provisioning/src/agent-provisioning.service.ts (line 45), add a guard to throw a clear error ifprocess.env.AGENT_API_KEYis undefined instead of passing"undefined"to the shell script.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/agent-provisioning/src/agent-provisioning.service.ts(2 hunks)
🔇 Additional comments (1)
apps/agent-provisioning/src/agent-provisioning.service.ts (1)
1-11: LGTM!The import reorganization and dotenv configuration are clean and follow standard practices.
pranalidhanavade
force-pushed
the
fetch-agent-token-via-api-in-credo-controller
branch
from
117068f to
3ef3ca2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libs/common/src/common.service.ts (1)
8-18: Prefer NestJS ConfigModule over dotenv for better DI and testing.While
dotenv.config()works, NestJS applications typically use@nestjs/config'sConfigModulefor environment variable management. This provides better dependency injection, type safety, and testability.Consider refactoring to use ConfigModule:
- Install
@nestjs/configif not already present:npm install @nestjs/config
- Import ConfigModule in your root module (e.g.,
app.module.ts):import { ConfigModule } from '@nestjs/config'; @Module({ imports: [ ConfigModule.forRoot({ isGlobal: true, cache: true, }), // other imports ], }) export class AppModule {}
- Inject ConfigService in CommonService:
import { ConfigService } from '@nestjs/config'; @Injectable() export class CommonService { constructor( private readonly httpService: HttpService, private readonly configService: ConfigService ) {} // Use this.configService.get<string>('AGENT_API_KEY') instead of process.env.AGENT_API_KEY }This approach centralizes configuration, improves testing (easier to mock), and provides better type safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.env.demo(0 hunks).env.sample(0 hunks)apps/agent-provisioning/AFJ/scripts/docker_start_agent.sh(1 hunks)apps/agent-provisioning/AFJ/scripts/fargate.sh(0 hunks)apps/agent-provisioning/AFJ/scripts/start_agent.sh(1 hunks)apps/agent-provisioning/AFJ/scripts/start_agent_ecs.sh(0 hunks)apps/agent-provisioning/src/agent-provisioning.service.ts(2 hunks)apps/agent-service/src/agent-service.service.ts(1 hunks)libs/common/src/common.service.ts(2 hunks)
💤 Files with no reviewable changes (4)
- apps/agent-provisioning/AFJ/scripts/start_agent_ecs.sh
- apps/agent-provisioning/AFJ/scripts/fargate.sh
- .env.sample
- .env.demo
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/agent-service/src/agent-service.service.ts
🔇 Additional comments (3)
apps/agent-provisioning/AFJ/scripts/docker_start_agent.sh (1)
228-240: Clean refactoring: token extraction removed successfully.The endpoint file now contains only
CONTROLLER_ENDPOINT, and all token-related logic has been removed. The refactoring aligns well with the PR objectives.apps/agent-provisioning/src/agent-provisioning.service.ts (1)
59-79: Well done: addresses all previous review comments.The implementation now includes:
- Clear error messages with proper grammar
- JSON parsing wrapped in try-catch with specific error handling
- Validation for the presence of
CONTROLLER_ENDPOINT- Appropriate logging at each step
The refactoring successfully removes token handling while maintaining robust endpoint parsing.
apps/agent-provisioning/AFJ/scripts/start_agent.sh (1)
231-243: Retain endpoint file creation—it's consumed by the service
The TypeScript service (apps/agent-provisioning/src/agent-provisioning.service.ts) reads and parses this JSON to extract CONTROLLER_ENDPOINT and will error if it’s missing. Removing this logic will break endpoint discovery.
|
Signed-off-by: pranalidhanavade <[email protected]>
Signed-off-by: pranalidhanavade <[email protected]>
Signed-off-by: pranalidhanavade <[email protected]>
Signed-off-by: pranalidhanavade <[email protected]>
pranalidhanavade
force-pushed
the
fetch-agent-token-via-api-in-credo-controller
branch
from
a2d3986 to
c50995b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/agent-provisioning/AFJ/scripts/docker_start_agent.sh (1)
28-37: Avoid logging wallet and storage passwords to stdout.
WALLET_PASSWORDandWALLET_STORAGE_PASSWORDare echoed, which will end up in logs and expose secrets. This is a security risk even if it predates this refactor.Drop these two
echostatements (or mask the values) while keeping the rest of the debug logging.apps/agent-provisioning/src/agent-provisioning.service.ts (1)
45-66: Fix async error handling inspinUpResponsePromise.The
spinUpResponsePromise is initialized with onlyresolveand norejectparameter, while errors are thrown inside theexeccallback. These thrown errors won't reject the outer Promise and will result in uncaught exceptions or hung callers when the endpoint file is missing, unparseable, or lacks the requiredCONTROLLER_ENDPOINTkey. Additionally, theerrparameter fromexecis silently ignored.Refactor to use both
resolveandreject, route all failure paths throughreject, and handle exec errors:- const spinUpResponse: object = new Promise(async (resolve) => { - await exec(walletProvision, async (err, stdout, stderr) => { + const spinUpResponse: object = new Promise((resolve, reject) => { + exec(walletProvision, (err, stdout, stderr) => { this.logger.log(`shell script output: ${stdout}`); if (stderr) { this.logger.log(`shell script error: ${stderr}`); } + + if (err) { + this.logger.error(`[walletProvision] - shell script failed: ${err.message}`); + return reject(err); + } const agentEndpointPath = `${process.cwd()}${process.env.AFJ_AGENT_ENDPOINT_PATH}${orgId}_${containerName}.json`; const agentEndPointExists = await this.checkFileExistence(agentEndpointPath); let agentEndPoint; if (agentEndPointExists) { this.logger.log('Agent endpoint file exists'); agentEndPoint = await fs.readFileSync(agentEndpointPath, 'utf8'); } else { this.logger.log('Agent endpoint file does not exist'); - throw new NotFoundException(`Agent endpoint file does not exist: ${agentEndpointPath}`); + return reject( + new NotFoundException(`Agent endpoint file does not exist: ${agentEndpointPath}`) + ); } let parsedEndpoint; try { parsedEndpoint = JSON.parse(agentEndPoint); } catch (parseError) { this.logger.error(`Failed to parse agent endpoint file: ${parseError.message}`); - throw new Error(`Invalid JSON in agent endpoint file: ${agentEndpointPath}`); + return reject( + new Error(`Invalid JSON in agent endpoint file: ${agentEndpointPath}`) + ); } if (!parsedEndpoint.CONTROLLER_ENDPOINT) { this.logger.error('CONTROLLER_ENDPOINT key missing in agent endpoint file'); - throw new Error(`Missing CONTROLLER_ENDPOINT in: ${agentEndpointPath}`); + return reject( + new Error(`Missing CONTROLLER_ENDPOINT in: ${agentEndpointPath}`) + ); } - resolve({ - agentEndPoint: parsedEndpoint.CONTROLLER_ENDPOINT - }); + return resolve({ + agentEndPoint: parsedEndpoint.CONTROLLER_ENDPOINT + }); }); });This ensures callers receive a rejected Promise on any failure rather than an uncaught exception.
🧹 Nitpick comments (3)
apps/agent-provisioning/AFJ/scripts/docker_start_agent.sh (1)
92-104: Align endpoint directory with file path used by the TS service.You create the
endpointsdirectory under${PWD}/agent-provisioning/AFJ/endpoints, but the JSON file is written to${PWD}/endpoints/${AGENCY}_${CONTAINER_NAME}.json. The Node service reads from${process.cwd()}${process.env.AFJ_AGENT_ENDPOINT_PATH}${orgId}_${containerName}.json, so ifAFJ_AGENT_ENDPOINT_PATHpoints atagent-provisioning/AFJ/endpoints, the current script will never produce the expected file.Consider deriving the directory and file path from a single base (e.g.,
${PWD}/agent-provisioning/AFJ/endpoints) to avoid drift and ensure the path matchesAFJ_AGENT_ENDPOINT_PATH.Also applies to: 229-240
apps/agent-service/src/agent-service.service.ts (1)
531-540: ValidateAGENT_API_KEYbefore callinggetBaseAgentToken.You now rely on
process.env.AGENT_API_KEYfor the base wallet token, but if this env var is unset/empty you’ll end up making a failing HTTP call and only see a genericbaseWalletTokenerror.Add an explicit check (and log) for a missing/empty
AGENT_API_KEYhere and fail fast with a clear configuration error before invokinggetBaseAgentToken.apps/agent-provisioning/src/agent-provisioning.service.ts (1)
45-46: Consider validatingAGENT_API_KEYbefore composing the shell command.
walletProvisionnow passes${process.env.AGENT_API_KEY}into the spin‑up script, but if this env var is missing, the agent config will either have an emptyapiKeyor the shell command may not be what you expect.Adding a simple check (and clear log) for a missing/empty
AGENT_API_KEYbefore buildingwalletProvisionwould make configuration errors much easier to diagnose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.env.demo(0 hunks).env.sample(0 hunks)apps/agent-provisioning/AFJ/scripts/docker_start_agent.sh(1 hunks)apps/agent-provisioning/AFJ/scripts/fargate.sh(0 hunks)apps/agent-provisioning/AFJ/scripts/start_agent.sh(1 hunks)apps/agent-provisioning/AFJ/scripts/start_agent_ecs.sh(0 hunks)apps/agent-provisioning/src/agent-provisioning.service.ts(2 hunks)apps/agent-service/src/agent-service.service.ts(1 hunks)libs/common/src/common.service.ts(2 hunks)
💤 Files with no reviewable changes (4)
- apps/agent-provisioning/AFJ/scripts/fargate.sh
- apps/agent-provisioning/AFJ/scripts/start_agent_ecs.sh
- .env.sample
- .env.demo
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/agent-provisioning/AFJ/scripts/start_agent.sh
- libs/common/src/common.service.ts
KambleSahil3
force-pushed
the
fetch-agent-token-via-api-in-credo-controller
branch
from
e7e2f31 to
c50995b
Compare
Signed-off-by: [email protected] <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/common/src/resend-helper-file.ts (1)
7-12: Consider lazy initialization instead of module-level validation.The module-level error throw will crash any service that imports this file if
RESEND_API_KEYis missing, even if Resend functionality is never used. Since this is in a shared library (libs/common), it affects all consumers.Consider deferring initialization until
sendWithResendis actually called:-dotenv.config(); - -const apiKey = process.env.RESEND_API_KEY; -if (!apiKey) { - throw new Error('Missing RESEND_API_KEY in environment variables.'); -} -const resend = new Resend(apiKey); +dotenv.config(); + +let resend: Resend | null = null; + +const getResendClient = (): Resend => { + if (!resend) { + const apiKey = process.env.RESEND_API_KEY; + if (!apiKey) { + throw new Error('Missing RESEND_API_KEY in environment variables.'); + } + resend = new Resend(apiKey); + } + return resend; +}; export const sendWithResend = async (emailDto: EmailDto): Promise<boolean> => { try { - const response = await resend.emails.send({ + const response = await getResendClient().emails.send({This allows services to import the module without requiring the API key unless they actually call the function.
🧹 Nitpick comments (7)
libs/prisma-service/prisma/migrations/20251125062851_add_ledger_null_trigger/migration.sql (1)
2-14: Remove redundant NULL check in trigger function.The function's
IF NEW."ledgerId" IS NULL THENcheck (line 5) is redundant because the trigger'sWHENclause (line 20) already guarantees this condition. The trigger only executes whenNEW."ledgerId" IS NULL AND OLD."ledgerId" IS NOT NULL, so the inner IF is unnecessary.Apply this diff to simplify:
CREATE OR REPLACE FUNCTION alert_ledger_null() RETURNS trigger AS $$ 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;apps/utility/src/utilities.repository.ts (2)
13-13: Add type annotation forpayloadparameter.The
payloadparameter lacks a type annotation, reducing type safety. Consider defining an interface or inline type.- async saveShorteningUrl(payload): Promise<object> { + async saveShorteningUrl(payload: { referenceId: string; invitationPayload: string }): Promise<object> {
31-31: Add type annotation forreferenceIdparameter.- async getShorteningUrl(referenceId): Promise<shortening_url> { + async getShorteningUrl(referenceId: string): Promise<shortening_url> {apps/api-gateway/src/utilities/utilities.service.ts (2)
9-10: Consider implementing lifecycle hook interfaces explicitly.The class defines
onModuleInitandonModuleDestroymethods but doesn't implement theOnModuleInitandOnModuleDestroyinterfaces. While NestJS will still call these methods, implementing the interfaces provides type safety and makes the intent explicit.-import { Inject, Injectable } from '@nestjs/common'; +import { Inject, Injectable, OnModuleInit, OnModuleDestroy } from '@nestjs/common'; ... @Injectable() -export class UtilitiesService extends BaseService { +export class UtilitiesService extends BaseService implements OnModuleInit, OnModuleDestroy {
30-30: Good observation in the TODO comment.The TODO comment raises a valid architectural concern. Consider creating an issue to track consolidating the DB alerting logic into a shared library.
Would you like me to open an issue to track this refactoring task?
apps/utility/src/utilities.service.ts (2)
25-25: Add type annotation forpayloadparameter.The
payloadparameter lacks a type annotation, reducing type safety. Define an interface or DTO for the expected shape.- async createAndStoreShorteningUrl(payload): Promise<string> { + async createAndStoreShorteningUrl(payload: { + credentialId: string; + schemaId: string; + credDefId: string; + invitationUrl: string; + attributes: unknown; + }): Promise<string> {
117-119: Simplify the boolean check.The
if (true !== result)comparison is unconventional. Consider using the more idiomatic form.- if (true !== result) { + if (result !== true) {
📜 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 (16)
.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.repository.ts(1 hunks)apps/utility/src/utilities.service.ts(1 hunks)libs/common/src/common.constant.ts(1 hunks)libs/common/src/dtos/email.dto.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/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(0 hunks)package.json(1 hunks)
💤 Files with no reviewable changes (1)
- libs/prisma-service/src/prisma-service.service.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .env.sample
- .env.demo
🧰 Additional context used
🧬 Code graph analysis (5)
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)
libs/prisma-service/src/prisma-service.module.ts (2)
apps/ledger/src/ledger.module.ts (1)
Module(16-36)libs/org-roles/src/org-roles.module.ts (1)
Module(7-12)
apps/utility/src/utilities.repository.ts (2)
libs/prisma-service/src/prisma-service.service.ts (1)
Injectable(5-54)libs/logger/src/logger.interface.ts (1)
Logger(6-20)
apps/api-gateway/src/utilities/utilities.service.ts (1)
libs/service/base.service.ts (1)
BaseService(2-9)
🔇 Additional comments (14)
libs/common/src/resend-helper-file.ts (1)
13-13: Good: Using validated constant for Resend initialization.The change to use a validated
apiKeyconstant instead of directly accessingprocess.env.RESEND_API_KEYimproves code clarity and ensures the key is validated before use.libs/prisma-service/src/prisma-service.module.ts (1)
3-6: PrismaServiceModule wiring with CommonModule looks consistentImporting
CommonModuleand adding it toimportscleanly exposes shared providers/utilities to anything usingPrismaServiceModule. The DI surface remains simple and consistent with the rest of the app.libs/org-roles/src/org-roles.module.ts (1)
1-9: Good migration to usingPrismaServiceModulefor Prisma DISwitching from a direct PrismaService provider to importing
PrismaServiceModuleis the right NestJS pattern and matches other modules in the repo. Providers list is now focused on OrgRoles concerns while still getting Prisma via the imported module.apps/ledger/src/ledger.module.ts (1)
5-35: LedgerModule’s Prisma wiring is now aligned with the shared module patternImporting
PrismaServiceModuleand droppingPrismaServicefromprovidersis the correct NestJS approach and keeps this module’s providers focused on ledger concerns while still enabling Prisma injection in services/repositories.apps/api-gateway/src/main.ts (1)
122-130: LGTM!The logging changes are appropriate. The conditional check for
DB_ALERT_ENABLEis defensive with optional chaining and string normalization.apps/utility/src/utilities.controller.ts (1)
35-46: LGTM!The new message handler follows the established pattern in this controller. The error handling is appropriate.
libs/common/src/common.constant.ts (1)
386-388: LGTM!The new threshold constant is well-named and centralized for reuse. The 30% default is a reasonable starting point for alerting.
libs/common/src/dtos/email.dto.ts (1)
1-8: LGTM!The
emailTotype expansion tostring | string[]is backwards compatible and enables multi-recipient support needed for the DB alerting feature.apps/api-gateway/src/utilities/utilities.service.ts (2)
61-72: LGTM on the query logic.The COUNT queries are safe (no user input), and the division-by-zero case is properly handled by checking
total === 0before computing the percentage.
77-80: Threshold comparison mismatch with comment.The comment says "> 30% for now" but the condition
>=means alerts trigger only when percent is strictly greater than 30%. If exactly 30%, no alert is sent. Consider clarifying the intent or updating the comment.If the intent is to alert at exactly 30% or above:
- // Condition: > 30% for now - if (CommonConstants.AFFECTED_RECORDS_THRESHOLD_PERCENTAGE_FOR_DB_ALERT >= percent) { + // Condition: >= 30% + if (CommonConstants.AFFECTED_RECORDS_THRESHOLD_PERCENTAGE_FOR_DB_ALERT > percent) {Likely an incorrect or invalid review comment.
package.json (1)
109-109: Thepgpackage version8.16.3is valid and compatible.Version 8.16.3 exists on npm and introduces only minor features and improvements over 8.11.2 (e.g., queryMode, per-query timeouts, ESM support). No breaking changes exist between these versions, so the bump is safe.
apps/utility/src/utilities.service.ts (3)
45-63: LGTM!Proper null check with
NotFoundExceptionfor missing data, and consistent error propagation viaRpcException.
65-81: LGTM!Clean S3 upload implementation. Note: This method throws a plain
Errorrather thanRpcExceptionused elsewhere—verify this is intentional for the call sites consuming this method.
83-110: LGTM!The fire-and-forget pattern with rate limiting and concurrency guard is appropriate for non-blocking alert dispatch. The
finally()cleanup ensuresisSendingAlertresets regardless of outcome.
| const emailDto = { | ||
| emailFrom: '', | ||
| 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>` | ||
| }; |
There was a problem hiding this comment.
Empty emailFrom may cause email delivery failures.
The emailFrom is set to an empty string. Many email services require a valid sender address. Consider using a configured default sender from environment variables.
const emailDto = {
- emailFrom: '',
+ emailFrom: process.env.DB_ALERT_EMAIL_FROM || process.env.DEFAULT_EMAIL_FROM || '',
emailTo: alertEmails,📝 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 emailDto = { | |
| emailFrom: '', | |
| 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>` | |
| }; | |
| const emailDto = { | |
| emailFrom: process.env.DB_ALERT_EMAIL_FROM || process.env.DEFAULT_EMAIL_FROM || '', | |
| 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>` | |
| }; |
🤖 Prompt for AI Agents
In apps/api-gateway/src/utilities/utilities.service.ts around lines 95 to 103,
the emailDto.emailFrom is set to an empty string which can cause delivery
failures; replace the empty string with a valid default sender pulled from
configuration/environment (e.g., process.env.DEFAULT_EMAIL_SENDER or your app
config) and fall back to a safe no-reply address if the env var is missing;
ensure the value is a valid email (or formatted display name <email>) and update
any related tests or callers expecting the previous empty value.
| async getPlatformConfigDetails(): Promise<platform_config> { | ||
| try { | ||
| return this.prisma.platform_config.findFirst(); | ||
| } catch (error) { | ||
| this.logger.error(`[getPlatformConfigDetails] - error: ${JSON.stringify(error)}`); | ||
| throw error; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing await causes error handling to be bypassed.
The findFirst() call is not awaited, so any database errors will not be caught by the catch block. The promise rejection will propagate uncaught.
Apply this diff to fix the issue:
// eslint-disable-next-line camelcase
async getPlatformConfigDetails(): Promise<platform_config> {
try {
- return this.prisma.platform_config.findFirst();
+ return await this.prisma.platform_config.findFirst();
} catch (error) {
this.logger.error(`[getPlatformConfigDetails] - error: ${JSON.stringify(error)}`);
throw error;
}
}📝 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.
| async getPlatformConfigDetails(): Promise<platform_config> { | |
| try { | |
| return this.prisma.platform_config.findFirst(); | |
| } catch (error) { | |
| this.logger.error(`[getPlatformConfigDetails] - error: ${JSON.stringify(error)}`); | |
| throw error; | |
| } | |
| } | |
| \ No newline at end of file | |
| } | |
| async getPlatformConfigDetails(): Promise<platform_config> { | |
| try { | |
| return await this.prisma.platform_config.findFirst(); | |
| } catch (error) { | |
| this.logger.error(`[getPlatformConfigDetails] - error: ${JSON.stringify(error)}`); | |
| throw error; | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/utility/src/utilities.repository.ts around lines 52 to 59, the call to
this.prisma.platform_config.findFirst() is missing an await so thrown DB errors
bypass the try/catch; update the method to await the promise (e.g., assign or
return await this.prisma.platform_config.findFirst()) so exceptions are caught
by the catch block and logged, preserving the existing error logging and rethrow
behavior.
| private lastAlertTime: number | null = null; | ||
| private isSendingAlert = false; // Prevent concurrent retries |
There was a problem hiding this comment.
Instance-level rate limiting won't work across multiple service instances.
When this service scales horizontally, each instance maintains its own lastAlertTime and isSendingAlert state, allowing duplicate alerts to be sent from different instances. For production reliability, consider using a distributed store (e.g., Redis) to coordinate alert throttling across instances.
🤖 Prompt for AI Agents
In apps/utility/src/utilities.service.ts around lines 14-15, the current
instance-level variables lastAlertTime and isSendingAlert cannot prevent
duplicate alerts when the service is scaled horizontally; replace them with a
distributed coordination mechanism (e.g., Redis) by storing a single alert key
with an expiry and using an atomic check-and-set (SETNX or a Redis lock/LUA
script) to determine whether an alert may be sent, update the timestamp TTL
atomically when sending succeeds, and fall back to renewing or releasing the
lock on failure; ensure all code paths consult the distributed key instead of
the in-memory fields and handle Redis errors gracefully (e.g., conservative
allow/deny policy) so throttling is enforced across all instances.
| // If last attempt → throw | ||
| if (attempt === retries) { | ||
| this.logger.error('All email retry attempts failed.'); | ||
| return; |
There was a problem hiding this comment.
Misleading comment: code returns, not throws.
The comment says "throw" but the implementation returns on final failure. Update the comment to match the behavior.
- // If last attempt → throw
+ // If last attempt → stop retrying
if (attempt === retries) {
this.logger.error('All email retry attempts failed.');
return;
}📝 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.
| // If last attempt → throw | |
| if (attempt === retries) { | |
| this.logger.error('All email retry attempts failed.'); | |
| return; | |
| // If last attempt → stop retrying | |
| if (attempt === retries) { | |
| this.logger.error('All email retry attempts failed.'); | |
| return; |
🤖 Prompt for AI Agents
In apps/utility/src/utilities.service.ts around lines 131 to 134, the inline
comment says "If last attempt → throw" but the code returns instead of throwing;
update the comment to accurately reflect behavior (e.g., "If last attempt →
return" or "If last attempt → stop retrying and return") so the comment matches
the implementation and avoid confusion.
e27a4d0 to
9f745f4
Compare
|
What
Summary by CodeRabbit
Refactor
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.