refactor(logging): migrate to singleton logger service #207
Conversation
- Created LoggerService singleton with WeakMap-based operation tracking - Prevents duplicate logs and memory leaks - CloudWatch-optimized structured JSON logging - Updated 10 controllers, 2 middleware, 7 services, 2 utils - Improved error handling with centralized error middleware logging - Removed redundant error logs before next(error) calls - Net reduction of 706 lines LFXV2-903 Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Asitha de Silva <[email protected]>
github-actions
bot
requested review from
andrest50,
dealako and
mauriciozanettisalomao
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughCentralizes logging around a new LoggerService and serverLogger across backend code and docs, removes the legacy Logger helper, adds operation lifecycle APIs, introduces AiService-backed meeting agenda generation, and a frontend change to use meetingInput().uid for meeting attachments/recordings/summaries. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR refactors the logging infrastructure from a static helper class to a singleton service pattern with enhanced capabilities for operation tracking, duplicate log prevention, and CloudWatch-optimized structured logging.
Key Changes:
- Introduced a new
LoggerServicesingleton with WeakMap-based operation tracking to prevent memory leaks - Migrated 20+ files from the static
Loggerhelper to the newloggersingleton instance - Removed redundant error logging in controllers (error middleware now handles centrally)
- Added CloudWatch-friendly structured logging with AWS trace ID support
Reviewed changes
Copilot reviewed 28 out of 29 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
apps/lfx-one/src/server/services/logger.service.ts |
New singleton logger service with operation state tracking and duplicate prevention |
apps/lfx-one/src/server/helpers/logger.ts |
Removed static Logger helper class (replaced by service) |
apps/lfx-one/src/server/middleware/error-handler.middleware.ts |
Updated to derive operation names from paths and use new logger |
apps/lfx-one/src/server/middleware/auth.middleware.ts |
Migrated all logging calls to new logger service |
apps/lfx-one/src/server/server.ts |
Added CloudWatch optimizations (base fields, mixin for trace ID, disabled auto-logging) |
apps/lfx-one/src/server/controllers/*.controller.ts |
Removed redundant error logs before next(error) calls (10 controllers) |
apps/lfx-one/src/server/services/*.service.ts |
Migrated to new logger service (7 services) |
apps/lfx-one/src/server/utils/*.util.ts |
Updated logging imports and calls (2 utils) |
apps/lfx-one/src/server/helpers/validation.helper.ts |
Updated logger import and method calls |
apps/lfx-one/src/server/routes/meetings.route.ts |
Updated to use new logger service for agenda generation |
apps/lfx-one/src/app/modules/meetings/components/meeting-card/meeting-card.component.ts |
Fixed signal reference bug (meeting() → meetingInput()) |
package.json, yarn.lock |
Updated turbo from 2.5.5 to 2.6.3 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
apps/lfx-one/src/app/modules/meetings/components/meeting-card/meeting-card.component.ts
Show resolved
Hide resolved
apps/lfx-one/src/app/modules/meetings/components/meeting-card/meeting-card.component.ts
Show resolved
Hide resolved
apps/lfx-one/src/app/modules/meetings/components/meeting-card/meeting-card.component.ts
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
apps/lfx-one/src/server/services/committee.service.ts (3)
1-379: Close allstartOperation()calls with correspondingsuccess()orerror()methods.The following operations start with
logger.startOperation()but are never closed withlogger.success()orlogger.error(), which violates the pattern used consistently across other services (user.service.ts, project.service.ts, meeting.service.ts):
- Line 205:
getCommitteeMembersCount()– start operation, return count, but no success- Line 256:
createCommitteeMember()– start operation after member creation, but no success- Line 294:
updateCommitteeMember()– start operation after member update, but no success- Line 320:
deleteCommitteeMember()– start operation after member deletion, but no success- Line 351:
getCommitteeMembersByCategory()– start operation, return members, but no success- Line 375:
updateCommitteeSettings()– start operation after settings update, but no successAdd
logger.success(req, operationName, startTime, {...metadata})before each return statement. For error paths, uselogger.error(req, operationName, startTime, error, {...metadata})in catch blocks (existinglogger.warning()calls for non-fatal issues are correct).
363-379:update_committee_settings: MovestartOperation()before the PUT request and addsuccess()after. The operation start should bracket the actual work to capture duration correctly. Current placement after the request completes reads like a completion log and discards the returned start time.private async updateCommitteeSettings(req: Request, committeeId: string, settings: CommitteeSettingsData): Promise<void> { const settingsData = { ...(settings.business_email_required !== undefined && { business_email_required: settings.business_email_required, }), ...(settings.is_audit_enabled !== undefined && { is_audit_enabled: settings.is_audit_enabled, }), }; const startTime = logger.startOperation(req, 'update_committee_settings', { committee_uid: committeeId, settings_data: settingsData, }); await this.microserviceProxy.proxyRequest(req, 'LFX_V2_SERVICE', `/committees/${committeeId}/settings`, 'PUT', {}, settingsData); logger.success(req, 'update_committee_settings', startTime, { committee_uid: committeeId, }); }
334-358: Log operation lifecycle correctly with proper PII handlingLogging should start before the proxy request and use
success()after completion to capture operation duration. Additionally,usernameis PII and must be sanitized usinglogger.sanitize()before logging.Replace lines 334-358 with:
public async getCommitteeMembersByCategory(req: Request, username: string, userEmail: string, category: string): Promise<CommitteeMember[]> { const startTime = logger.startOperation(req, 'get_committee_members_by_category', { category, }); const params = { v: '1', type: 'committee_member', tags_all: [`username:${username}`, `committee_category:${category}`], }; const { resources } = await this.microserviceProxy.proxyRequest<QueryServiceResponse<CommitteeMember>>( req, 'LFX_V2_SERVICE', '/query/resources', 'GET', params ); const userMemberships = resources.map((resource) => resource.data); logger.success(req, 'get_committee_members_by_category', startTime, { memberships_count: userMemberships.length, }); return userMemberships; }apps/lfx-one/src/server/utils/persona-helper.ts (1)
65-90: Misuse oflogger.startOperationfor intermediate logging steps.
logger.startOperationis designed for tracking the start of operations with duration measurement. Using it for intermediate logging steps likecheck_committee_category,found_committee_memberships,matched_persona, andno_memberships_foundis semantically incorrect and will pollute the operation stack without correspondingsuccessorerrorcalls to clean them up.Per coding guidelines, these internal operation details should use DEBUG level logging instead.
Consider using
logger.debug(if available) orreq.log.debugfor these intermediate steps:for (const [category, persona] of Object.entries(COMMITTEE_CATEGORY_TO_PERSONA)) { - logger.startOperation(req, 'check_committee_category', { category }); + req.log.debug({ category }, 'Checking committee category'); const memberships = await committeeService.getCommitteeMembersByCategory(req, username, userEmail || '', category); - logger.startOperation(req, 'found_committee_memberships', { - category, - memberships_count: memberships.length, - }); + req.log.debug({ category, memberships_count: memberships.length }, 'Found committee memberships'); if (memberships.length > 0) { - logger.startOperation(req, 'matched_persona', { - username, - category, - persona, - memberships_count: memberships.length, - }); + req.log.debug({ username, category, persona, memberships_count: memberships.length }, 'Matched persona'); matchedPersonas.push(persona); // ... } else { - logger.startOperation(req, 'no_memberships_found', { category }); + req.log.debug({ category }, 'No memberships found'); } }apps/lfx-one/src/server/services/project.service.ts (2)
332-407: Ensure started operations are closed on error paths (to avoid stale op state + missing duration).
resolveEmailToSub,resolveEmailToUsername, andgetUserInfostart an operation but, on thrown errors/timeouts, they rethrow without callinglogger.error(...)(or another “end” method). That can leave operation entries around for the rest of the request and loses failure duration tracking.const startTime = logger.startOperation(req, 'resolve_email_to_sub', { email: normalizedEmail }); try { ... return username; } catch (error) { + // Close the operation for unexpected failures (avoid stale op state) + logger.error(req, 'resolve_email_to_sub', startTime, error, { email: normalizedEmail }, { skipIfLogged: true }); // Re-throw ResourceNotFoundError as-is if (error instanceof ResourceNotFoundError) { throw error; }(Apply the same pattern to
resolve_email_to_usernameandget_user_info.)Also applies to: 423-498, 534-601
332-350: Re-check whether logging raw email/sub is allowed (PII).
These logs includesub/usernamein structured metadata. If your logging policy treats these as sensitive, consider hashing/redacting (or usinglogger’s sanitize utility if available) to avoid leaking identifiers into CloudWatch.Also applies to: 423-441, 523-535
apps/lfx-one/src/server/controllers/organization.controller.ts (1)
19-37: Don’t start the operation before validation (or explicitly end it on validation failure).
As written, invalid requests callnext(validationError)without closing thesearch_organizationsoperation. Prefer movingstartOperationafter validation passes (or calllogger.validation(...)withstartTime).public async searchOrganizations(req: Request, res: Response, next: NextFunction): Promise<void> { const { query } = req.query; - const startTime = logger.startOperation(req, 'search_organizations', { - has_query: !!query, - }); try { // Check if the search query is provided and is a string if (!query || typeof query !== 'string') { // Create a validation error - error handler will log const validationError = ServiceValidationError.forField('query', 'Search query is required and must be a string', { operation: 'search_organizations', service: 'organization_controller', path: req.path, }); next(validationError); return; } + + const startTime = logger.startOperation(req, 'search_organizations', { + has_query: true, + });apps/lfx-one/src/server/controllers/analytics.controller.ts (1)
32-56: Update coding guideline and remove controller-level error logging once middleware is updated.The guideline in CLAUDE.md currently documents the old
Logger.start()/Logger.error()class-based pattern, but the code has migrated to theloggersingleton service with methods likelogger.startOperation()andlogger.error(). Update CLAUDE.md to reflect the newlogger.servicepattern.Additionally, controller-level
logger.error(...); next(error)calls (present across all handlers in this file) create redundant error handling when the error middleware should be the single source. OnceapiErrorHandleris updated to pass{ skipIfLogged: true }tologger.error(), remove these controller calls:} catch (error) { - logger.error(req, 'get_active_weeks_streak', startTime, error); next(error); }This applies to all 25+ endpoints in this controller (lines 32-56, 62-86, 92-116, etc.).
apps/lfx-one/src/server/controllers/meeting.controller.ts (1)
137-155: Avoid loggingsuccessandfailedfor the same operation (sub-step failures should be separate ops or WARN).
getMeetingById: you loglogger.success(req, 'get_meeting_by_id', ...)before the registrants enrichment; then on registrants failure you calllogger.error(req, 'get_meeting_by_id', ...)(same operation). That can yield contradictory “success” + “failed” entries for the same op and also clears op-state early (LoggerService.error deletes the operation).createBatchResponse: per-item failure useslogger.error(req, operationName, startTime, ...)(same op as the controller). If there are any failures, this can similarly emit “Failed to add_meeting_registrants” followed by “Successfully completed add_meeting_registrants”.Suggested change: use a distinct operation name for sub-step failures (or WARN if you’re intentionally continuing), and don’t “fail” the parent op when returning 207/partial success.
@@ - // TODO: Remove this once we have a way to get the registrants count + // TODO: Remove this once we have a way to get the registrants count try { const registrants = await this.meetingService.getMeetingRegistrants(req, meeting.uid); @@ } catch (error) { - // Log the error for registrants fetch failure - logger.error(req, 'get_meeting_by_id', startTime, error, { - meeting_uid: uid, - }); + // Registrants enrichment failed; don't fail the parent operation + logger.warning(req, 'get_meeting_by_id_registrants', 'Failed to fetch registrants; continuing without counts', { + meeting_uid: uid, + err: error, + }); } @@ - // Log individual failure - logger.error(req, operationName, startTime, error, { + // Log individual failure without failing the parent operation state + const itemStartTime = Date.now(); + logger.error(req, `${operationName}_item_failed`, itemStartTime, error, { meeting_uid: meetingUid, identifier: getIdentifier(inputData[index], index), });Also applies to: 1246-1287
apps/lfx-one/src/server/controllers/public-meeting.controller.ts (1)
466-470: PII leak:restrictedMeetingChecklogs raw
SENSITIVE_FIELDSincludeslogger.successdoes not auto-sanitize; this will emit the email address into logs.- logger.success(req, 'restricted_meeting_check', helperStartTime, { - meeting_id: id, - email, - registrant_count: registrants.resources.length, - }); + logger.success(req, 'restricted_meeting_check', helperStartTime, { + meeting_id: id, + has_email: !!email, + registrant_count: registrants.resources.length, + });#!/bin/bash # Quick scan for email being logged via logger.* metadata. rg -n --type=ts -C2 "logger\\.(debug|info|success|warning|error)\\([^\\)]*\\bemail\\b"
🧹 Nitpick comments (13)
apps/lfx-one/src/app/modules/meetings/components/meeting-card/meeting-card.component.ts (1)
482-492: Consider retaining thetoSignal(...)return value ininitRecording()/initSummary()to make lifetime explicit. Right now it’s created only for side-effects viatap(). Storing it (even as a private field) or switching totoObservable(...).pipe(...).subscribe(...)withtakeUntilDestroyed()makes ownership clearer.Also applies to: 494-504
apps/lfx-one/src/server/helpers/validation.helper.ts (1)
6-6: Preferlogger.validation(...)(or warn) here to avoid noisy ERRORs + potential duplicate loggingThese are client/input validation failures, but they’re currently logged via
logger.error(...)and then forwarded tonext(...), which can duplicate logs if the error middleware also logs the resultingServiceValidationError. Also, ERROR level will skew alerting/metrics.Consider switching to
logger.validation(...)(per PR objectives) and/or pass a “skip duplicate” option if supported.Also applies to: 35-36, 71-72, 107-108, 136-137
apps/lfx-one/src/server/utils/m2m-token.util.ts (1)
21-25: Add request timeout + avoid logging/propagating raw error bodies/objects
fetch(tokenEndpoint, ...)has no timeout; considerAbortControllerwith a small upper bound.error_bodymay contain unexpected sensitive details; consider whitelisting fields.original_error: error(unknown) insideMicroserviceErrorcan be circular or overly large—prefer{ name, message }(and maybe a truncated stack) instead.Also applies to: 50-55, 68-71, 79-84, 95-106
apps/lfx-one/src/server/services/access-check.service.ts (1)
39-44: Verifywarningerror serialization + guard againstresponse.resultslength mismatch
- Please verify
logger.warning(...)emits errors as top-levelerr(not nested), otherwise Pino error tooling won’t work well.- Consider adding a sanity check that
response.results.length === resources.lengthand warn when mismatched (it currently defaults missing entries tofalseimplicitly).Also applies to: 87-97
apps/lfx-one/src/server/utils/persona-helper.ts (1)
70-73: Useerrfield for consistency with Pino's error serializer.The coding guidelines specify using the
errfield for errors to leverage Pino's error serializer. Line 72 useserror:while line 78 correctly useserr:.logger.warning(req, 'get_current_user_profile', 'Failed to fetch user metadata from NATS', { username, - error: natsResponse.error, + err: natsResponse.error, });apps/lfx-one/src/server/controllers/user.controller.ts (1)
21-24: Consider sanitizing query parameters in operation start.Query parameters like
persona,projectUid, andprojectSlugare logged directly without sanitization. While these specific parameters are unlikely to contain sensitive data, usinglogger.sanitizewould provide consistent protection and align with the pattern used in other controllers (e.g.,committee.controller.tslines 22-23, 43).const startTime = logger.startOperation(req, 'get_pending_actions', { - persona: req.query['persona'], - project_uid: req.query['projectUid'], + query_params: logger.sanitize(req.query as Record<string, any>), });apps/lfx-one/src/server/controllers/profile.controller.ts (1)
70-79: Inconsistent error field naming in warning logs.Line 72 uses
error:while line 78 useserr:. Per coding guidelines, useerrfield consistently to leverage Pino's error serializer.logger.warning(req, 'get_current_user_profile', 'Failed to fetch user metadata from NATS', { username, - error: natsResponse.error, + err: natsResponse.error, });apps/lfx-one/src/server/services/project.service.ts (1)
232-240: Avoid “orphaned”/duplicate operations inupdateProjectPermissions(silent start + second start).
Right now themanualUserInfopath callslogger.startOperation(..., { silent: true })and then later starts the same operation again (and only the second one is paired withsuccess). Prefer a single start, or emit a debug log forinfo_sourceinstead of a silent operation start.- if (manualUserInfo) { - logger.startOperation( - req, - `${operation}_user_project_permissions`, - { - username: backendIdentifier, - info_source: 'manual', - }, - { silent: true } - ); + if (manualUserInfo) { + logger.debug(req, `${operation}_user_project_permissions`, 'Using manual user info', { + username: backendIdentifier, + info_source: 'manual', + }); userInfo = { name: manualUserInfo.name, email: manualUserInfo.email, username: backendIdentifier, // Use the sub for backend consistency };Also applies to: 295-315
apps/lfx-one/src/server/controllers/meeting.controller.ts (2)
1180-1188:_startTimeis unused; remove it or use it.
Right now_startTimeis just noise inprocessRegistrantOperationsand makes call sites harder to read.- private async processRegistrantOperations<T, R>( - req: Request, - next: NextFunction, - _startTime: number, + private async processRegistrantOperations<T, R>( + req: Request, + next: NextFunction, operationName: string, meetingUid: string, inputData: T[], operation: (input: T) => Promise<R> ): Promise<{ results: PromiseSettledResult<R>[]; shouldReturn: boolean }> {
174-181:body_size: JSON.stringify(req.body).lengthcan throw / be expensive; preferBuffer.byteLengthwith a safe fallback.
This is instrumentation-only, but you don’t want logging metadata to be the reason an endpoint fails or slows.- body_size: JSON.stringify(req.body).length, + body_size: (() => { + try { + return Buffer.byteLength(JSON.stringify(req.body)); + } catch { + return undefined; + } + })(),Also applies to: 209-216, 391-396, 483-487, 571-575
apps/lfx-one/src/server/services/user.service.ts (1)
478-481: Fetch meetings in parallel (small latency win).- const meetings = await this.meetingService.getMeetings(req, query, 'meeting', false); - const v1Meetings = await this.meetingService.getMeetings(req, query, 'v1_meeting', false); + const [meetings, v1Meetings] = await Promise.all([ + this.meetingService.getMeetings(req, query, 'meeting', false), + this.meetingService.getMeetings(req, query, 'v1_meeting', false), + ]);apps/lfx-one/src/server/services/logger.service.ts (2)
256-266:sanitize()is shallow; nested sensitive fields will leak unless callers pre-flatten.
If payloads include nested{ user: { email } }, this won’t redact it. Consider deep-walking objects/arrays (or clearly document “top-level only” and enforce usage patterns).
189-203: Validation logs don’t close operation state; consider marking/deleting if started.
IfstartOperation()happened and validation short-circuits, the op remains in the per-request Map until GC; also it may increase duplicate-start noise. Consider: if the operation exists in the stack, mark it logged and delete it insidevalidation().Also applies to: 284-292
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (28)
apps/lfx-one/src/app/modules/meetings/components/meeting-card/meeting-card.component.ts(3 hunks)apps/lfx-one/src/server/controllers/analytics.controller.ts(64 hunks)apps/lfx-one/src/server/controllers/committee.controller.ts(21 hunks)apps/lfx-one/src/server/controllers/meeting.controller.ts(49 hunks)apps/lfx-one/src/server/controllers/organization.controller.ts(3 hunks)apps/lfx-one/src/server/controllers/past-meeting.controller.ts(22 hunks)apps/lfx-one/src/server/controllers/profile.controller.ts(20 hunks)apps/lfx-one/src/server/controllers/project.controller.ts(17 hunks)apps/lfx-one/src/server/controllers/public-meeting.controller.ts(18 hunks)apps/lfx-one/src/server/controllers/search.controller.ts(3 hunks)apps/lfx-one/src/server/controllers/user.controller.ts(5 hunks)apps/lfx-one/src/server/helpers/logger.ts(0 hunks)apps/lfx-one/src/server/helpers/validation.helper.ts(5 hunks)apps/lfx-one/src/server/middleware/auth.middleware.ts(16 hunks)apps/lfx-one/src/server/middleware/error-handler.middleware.ts(4 hunks)apps/lfx-one/src/server/routes/meetings.route.ts(2 hunks)apps/lfx-one/src/server/server.ts(7 hunks)apps/lfx-one/src/server/services/access-check.service.ts(3 hunks)apps/lfx-one/src/server/services/committee.service.ts(9 hunks)apps/lfx-one/src/server/services/etag.service.ts(3 hunks)apps/lfx-one/src/server/services/logger.service.ts(1 hunks)apps/lfx-one/src/server/services/meeting.service.ts(27 hunks)apps/lfx-one/src/server/services/microservice-proxy.service.ts(0 hunks)apps/lfx-one/src/server/services/project.service.ts(16 hunks)apps/lfx-one/src/server/services/user.service.ts(8 hunks)apps/lfx-one/src/server/utils/m2m-token.util.ts(6 hunks)apps/lfx-one/src/server/utils/persona-helper.ts(6 hunks)package.json(1 hunks)
💤 Files with no reviewable changes (2)
- apps/lfx-one/src/server/services/microservice-proxy.service.ts
- apps/lfx-one/src/server/helpers/logger.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Always use direct imports for standalone components - no barrel exports
Use TypeScript interfaces instead of union types for better maintainability
Do not nest ternary expressions
Prefer interface for defining object shapes in TypeScript
Use path mappings and import aliases as configured in tsconfig.json for imports
Files:
apps/lfx-one/src/server/routes/meetings.route.tsapps/lfx-one/src/server/controllers/search.controller.tsapps/lfx-one/src/server/controllers/project.controller.tsapps/lfx-one/src/server/helpers/validation.helper.tsapps/lfx-one/src/server/utils/m2m-token.util.tsapps/lfx-one/src/server/services/access-check.service.tsapps/lfx-one/src/server/controllers/committee.controller.tsapps/lfx-one/src/server/middleware/auth.middleware.tsapps/lfx-one/src/server/server.tsapps/lfx-one/src/server/controllers/past-meeting.controller.tsapps/lfx-one/src/app/modules/meetings/components/meeting-card/meeting-card.component.tsapps/lfx-one/src/server/services/committee.service.tsapps/lfx-one/src/server/services/project.service.tsapps/lfx-one/src/server/controllers/public-meeting.controller.tsapps/lfx-one/src/server/controllers/profile.controller.tsapps/lfx-one/src/server/controllers/organization.controller.tsapps/lfx-one/src/server/utils/persona-helper.tsapps/lfx-one/src/server/controllers/analytics.controller.tsapps/lfx-one/src/server/services/meeting.service.tsapps/lfx-one/src/server/services/logger.service.tsapps/lfx-one/src/server/services/user.service.tsapps/lfx-one/src/server/middleware/error-handler.middleware.tsapps/lfx-one/src/server/services/etag.service.tsapps/lfx-one/src/server/controllers/meeting.controller.tsapps/lfx-one/src/server/controllers/user.controller.ts
**/*.{html,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always add data-testid attributes when creating new components for reliable test targeting
Files:
apps/lfx-one/src/server/routes/meetings.route.tsapps/lfx-one/src/server/controllers/search.controller.tsapps/lfx-one/src/server/controllers/project.controller.tsapps/lfx-one/src/server/helpers/validation.helper.tsapps/lfx-one/src/server/utils/m2m-token.util.tsapps/lfx-one/src/server/services/access-check.service.tsapps/lfx-one/src/server/controllers/committee.controller.tsapps/lfx-one/src/server/middleware/auth.middleware.tsapps/lfx-one/src/server/server.tsapps/lfx-one/src/server/controllers/past-meeting.controller.tsapps/lfx-one/src/app/modules/meetings/components/meeting-card/meeting-card.component.tsapps/lfx-one/src/server/services/committee.service.tsapps/lfx-one/src/server/services/project.service.tsapps/lfx-one/src/server/controllers/public-meeting.controller.tsapps/lfx-one/src/server/controllers/profile.controller.tsapps/lfx-one/src/server/controllers/organization.controller.tsapps/lfx-one/src/server/utils/persona-helper.tsapps/lfx-one/src/server/controllers/analytics.controller.tsapps/lfx-one/src/server/services/meeting.service.tsapps/lfx-one/src/server/services/logger.service.tsapps/lfx-one/src/server/services/user.service.tsapps/lfx-one/src/server/middleware/error-handler.middleware.tsapps/lfx-one/src/server/services/etag.service.tsapps/lfx-one/src/server/controllers/meeting.controller.tsapps/lfx-one/src/server/controllers/user.controller.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: License headers are required on all source files
Prepend 'Generated with Claude Code (https://claude.ai/code)' if assisted with the code
Files:
apps/lfx-one/src/server/routes/meetings.route.tsapps/lfx-one/src/server/controllers/search.controller.tsapps/lfx-one/src/server/controllers/project.controller.tsapps/lfx-one/src/server/helpers/validation.helper.tsapps/lfx-one/src/server/utils/m2m-token.util.tsapps/lfx-one/src/server/services/access-check.service.tsapps/lfx-one/src/server/controllers/committee.controller.tsapps/lfx-one/src/server/middleware/auth.middleware.tsapps/lfx-one/src/server/server.tsapps/lfx-one/src/server/controllers/past-meeting.controller.tsapps/lfx-one/src/app/modules/meetings/components/meeting-card/meeting-card.component.tsapps/lfx-one/src/server/services/committee.service.tsapps/lfx-one/src/server/services/project.service.tsapps/lfx-one/src/server/controllers/public-meeting.controller.tsapps/lfx-one/src/server/controllers/profile.controller.tsapps/lfx-one/src/server/controllers/organization.controller.tsapps/lfx-one/src/server/utils/persona-helper.tsapps/lfx-one/src/server/controllers/analytics.controller.tsapps/lfx-one/src/server/services/meeting.service.tsapps/lfx-one/src/server/services/logger.service.tsapps/lfx-one/src/server/services/user.service.tsapps/lfx-one/src/server/middleware/error-handler.middleware.tsapps/lfx-one/src/server/services/etag.service.tsapps/lfx-one/src/server/controllers/meeting.controller.tsapps/lfx-one/src/server/controllers/user.controller.ts
apps/**/**/server/**/*.{ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/**/**/server/**/*.{ts,js}: Use Pino for structured JSON logs with sensitive data redaction in all backend services
Always use err field for errors to leverage Pino's error serializer - correct: req.log.error({ err: error, ...metadata }, 'message')
Log INFO level for business operation completions (created, updated, deleted) and successful data retrieval
Log WARN level for error conditions leading to exceptions and data quality issues
Log DEBUG level for internal operations, preparation steps, and intent statements
Log ERROR level for system failures, unhandled exceptions, and critical errors
Files:
apps/lfx-one/src/server/routes/meetings.route.tsapps/lfx-one/src/server/controllers/search.controller.tsapps/lfx-one/src/server/controllers/project.controller.tsapps/lfx-one/src/server/helpers/validation.helper.tsapps/lfx-one/src/server/utils/m2m-token.util.tsapps/lfx-one/src/server/services/access-check.service.tsapps/lfx-one/src/server/controllers/committee.controller.tsapps/lfx-one/src/server/middleware/auth.middleware.tsapps/lfx-one/src/server/server.tsapps/lfx-one/src/server/controllers/past-meeting.controller.tsapps/lfx-one/src/server/services/committee.service.tsapps/lfx-one/src/server/services/project.service.tsapps/lfx-one/src/server/controllers/public-meeting.controller.tsapps/lfx-one/src/server/controllers/profile.controller.tsapps/lfx-one/src/server/controllers/organization.controller.tsapps/lfx-one/src/server/utils/persona-helper.tsapps/lfx-one/src/server/controllers/analytics.controller.tsapps/lfx-one/src/server/services/meeting.service.tsapps/lfx-one/src/server/services/logger.service.tsapps/lfx-one/src/server/services/user.service.tsapps/lfx-one/src/server/middleware/error-handler.middleware.tsapps/lfx-one/src/server/services/etag.service.tsapps/lfx-one/src/server/controllers/meeting.controller.tsapps/lfx-one/src/server/controllers/user.controller.ts
apps/**/**/server/**/*controller*.{ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
All controller functions must use Logger helper methods: Logger.start(), Logger.success(), Logger.error(), Logger.warning(), Logger.validation()
Files:
apps/lfx-one/src/server/controllers/search.controller.tsapps/lfx-one/src/server/controllers/project.controller.tsapps/lfx-one/src/server/controllers/committee.controller.tsapps/lfx-one/src/server/controllers/past-meeting.controller.tsapps/lfx-one/src/server/controllers/public-meeting.controller.tsapps/lfx-one/src/server/controllers/profile.controller.tsapps/lfx-one/src/server/controllers/organization.controller.tsapps/lfx-one/src/server/controllers/analytics.controller.tsapps/lfx-one/src/server/controllers/meeting.controller.tsapps/lfx-one/src/server/controllers/user.controller.ts
**/*.component.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use Angular 19 zoneless change detection with signals for component state management
Files:
apps/lfx-one/src/app/modules/meetings/components/meeting-card/meeting-card.component.ts
🧠 Learnings (1)
📚 Learning: 2025-12-10T23:20:29.281Z
Learnt from: asithade
Repo: linuxfoundation/lfx-v2-ui PR: 206
File: apps/lfx-one/src/app/modules/meetings/components/agenda-template-selector/agenda-template-selector.component.ts:11-15
Timestamp: 2025-12-10T23:20:29.281Z
Learning: For Angular 19+ projects, standalone defaults to true for components, directives, and pipes when not explicitly declared. Therefore, components with an imports array in the Component decorator do not require an explicit standalone: true declaration to be standalone. In reviews, verify that a component without an explicit standalone flag and with an imports array will behave as a standalone component according to Angular 19+ defaults. If you need to enforce explicit standalone behavior for clarity or consistency, you can still add standalone: true, but it is not required by default in Angular 19+. Ensure this guidance is considered when reviewing any component.ts files across the codebase.
Applied to files:
apps/lfx-one/src/app/modules/meetings/components/meeting-card/meeting-card.component.ts
🧬 Code graph analysis (22)
apps/lfx-one/src/server/routes/meetings.route.ts (1)
apps/lfx-one/src/server/services/logger.service.ts (1)
logger(303-303)
apps/lfx-one/src/server/controllers/search.controller.ts (1)
apps/lfx-one/src/server/services/logger.service.ts (1)
logger(303-303)
apps/lfx-one/src/server/controllers/project.controller.ts (1)
apps/lfx-one/src/server/services/logger.service.ts (1)
logger(303-303)
apps/lfx-one/src/server/helpers/validation.helper.ts (1)
apps/lfx-one/src/server/services/logger.service.ts (2)
logger(303-303)error(146-184)
apps/lfx-one/src/server/utils/m2m-token.util.ts (1)
apps/lfx-one/src/server/services/logger.service.ts (2)
logger(303-303)error(146-184)
apps/lfx-one/src/server/services/access-check.service.ts (1)
apps/lfx-one/src/server/services/logger.service.ts (2)
logger(303-303)error(146-184)
apps/lfx-one/src/server/controllers/committee.controller.ts (1)
apps/lfx-one/src/server/services/logger.service.ts (1)
logger(303-303)
apps/lfx-one/src/server/middleware/auth.middleware.ts (1)
apps/lfx-one/src/server/services/logger.service.ts (2)
logger(303-303)error(146-184)
apps/lfx-one/src/server/server.ts (1)
apps/lfx-one/src/server/services/logger.service.ts (2)
logger(303-303)error(146-184)
apps/lfx-one/src/server/controllers/past-meeting.controller.ts (1)
apps/lfx-one/src/server/services/logger.service.ts (2)
logger(303-303)error(146-184)
apps/lfx-one/src/server/services/committee.service.ts (1)
apps/lfx-one/src/server/services/logger.service.ts (1)
logger(303-303)
apps/lfx-one/src/server/services/project.service.ts (1)
apps/lfx-one/src/server/services/logger.service.ts (1)
logger(303-303)
apps/lfx-one/src/server/controllers/public-meeting.controller.ts (2)
apps/lfx-one/src/server/services/logger.service.ts (2)
logger(303-303)error(146-184)apps/lfx-one/src/server/utils/m2m-token.util.ts (1)
generateM2MToken(16-108)
apps/lfx-one/src/server/controllers/profile.controller.ts (1)
apps/lfx-one/src/server/services/logger.service.ts (2)
logger(303-303)error(146-184)
apps/lfx-one/src/server/controllers/organization.controller.ts (1)
apps/lfx-one/src/server/services/logger.service.ts (2)
logger(303-303)error(146-184)
apps/lfx-one/src/server/utils/persona-helper.ts (1)
apps/lfx-one/src/server/services/logger.service.ts (2)
logger(303-303)error(146-184)
apps/lfx-one/src/server/controllers/analytics.controller.ts (1)
apps/lfx-one/src/server/services/logger.service.ts (2)
logger(303-303)error(146-184)
apps/lfx-one/src/server/services/logger.service.ts (1)
packages/shared/src/constants/server.constants.ts (1)
SENSITIVE_FIELDS(8-26)
apps/lfx-one/src/server/services/user.service.ts (2)
apps/lfx-one/src/server/services/logger.service.ts (2)
logger(303-303)error(146-184)apps/lfx-one/src/server/utils/m2m-token.util.ts (1)
generateM2MToken(16-108)
apps/lfx-one/src/server/middleware/error-handler.middleware.ts (2)
apps/lfx-one/src/server/errors/index.ts (1)
isBaseApiError(27-29)apps/lfx-one/src/server/services/logger.service.ts (2)
error(146-184)logger(303-303)
apps/lfx-one/src/server/controllers/meeting.controller.ts (1)
apps/lfx-one/src/server/services/logger.service.ts (2)
logger(303-303)error(146-184)
apps/lfx-one/src/server/controllers/user.controller.ts (1)
apps/lfx-one/src/server/services/logger.service.ts (1)
logger(303-303)
🔇 Additional comments (22)
package.json (1)
45-45: Confirm turbo version bump was intentional.The turbo devDependency was updated from ^2.5.5 to ^2.6.3 (a minor version bump). This change appears unrelated to the logging refactoring objectives described in the PR. Please confirm whether this update was intentional or incidentally included.
Minor version bumps are generally backward-compatible and safe. However, if this change was unintended, consider reverting it to keep the PR scope focused on the logging migration.
apps/lfx-one/src/app/modules/meetings/components/meeting-card/meeting-card.component.ts (1)
474-479: Good fix: usingmeetingInput().uidprevents startup races withmeeting()not yet initialized.apps/lfx-one/src/server/services/committee.service.ts (2)
17-17: Import migration looks consistent.
205-208: This concern is already addressed—LoggerService sanitizes query parameters. ThestartOperationmethod in logger.ts callssanitizeQueryParams()before logging, which redacts sensitive keys (password, token, api_key, secret, email, phone, ssn, credit_card, etc.) from the query object. No changes needed.apps/lfx-one/src/server/utils/persona-helper.ts (1)
43-43: LGTM on main operation tracking pattern.The primary operation tracking with
logger.startOperationat the beginning andlogger.successat the end correctly implements duration-aware logging for thefetch_user_persona_and_organizationsoperation.Also applies to: 108-113
apps/lfx-one/src/server/controllers/committee.controller.ts (3)
8-8: LGTM on logger service import.Import correctly updated to use the centralized logger service singleton.
20-36: Consistent logging pattern across controller methods.The migration correctly applies the new logger service pattern:
logger.startOperationcaptures operation start with relevant metadatalogger.sanitizeproperly redacts sensitive data from query params and request bodieslogger.successlogs completion with duration and result metadata- Error handling delegates to centralized middleware via
next(error)This pattern is consistently applied across all 11 controller methods.
70-80: Validation errors delegated to error middleware.Validation errors are created via
ServiceValidationError.forField()and forwarded to the error middleware vianext(validationError). This aligns with the PR's goal of centralizing error logging in the error-handler middleware rather than logging at the controller level.apps/lfx-one/src/server/controllers/user.controller.ts (2)
82-93: LGTM on success logging and error handling.The
getPendingActionsmethod correctly logs success with relevant metadata (persona, project identifiers, action count) and delegates errors to the middleware vianext(error).
101-146: LGTM ongetUserMeetingsmethod.The logging pattern is correctly implemented with operation start, success logging with duration, and centralized error handling.
apps/lfx-one/src/server/controllers/profile.controller.ts (2)
649-652: Good practice: logging token length instead of actual token.Correctly logs only the token length rather than the sensitive bearer token itself.
32-109: LGTM ongetCurrentUserProfilelogging pattern.Appropriate use of:
logger.startOperationat entrylogger.warningfor non-critical NATS failures (allowing graceful degradation)logger.successon completion with relevant metadataapps/lfx-one/src/server/controllers/search.controller.ts (2)
23-27: Good practice: logging presence flags instead of actual search values.Using boolean flags (
has_name,has_type,has_tags) instead of actual search values avoids logging potentially sensitive user search queries while still providing useful debugging information.
77-85: LGTM on success logging and error handling.The search operation correctly logs completion with result metadata and delegates errors to the centralized error middleware.
apps/lfx-one/src/server/controllers/organization.controller.ts (1)
49-52: Error handling pattern is correct and follows best practices.The controller correctly delegates errors via
next(error)without local logging, allowing the centralizedapiErrorHandlermiddleware to handle all error logging. TheLoggerServicetracks operation state per-request to prevent duplicate logs through its operation stack mechanism. Withpino-http'sautoLogging: false, only explicit logging calls (viareq.logor the logger service) generate logs, eliminating the risk of unintended duplicate logging. No changes needed.apps/lfx-one/src/server/server.ts (1)
156-166: Verify baseline telemetry coverage for successful requests withautoLogging: false.
autoLogging: falsecorrectly disables pino-http's automatic request/response logs in v10.5.0. Error scenarios are covered—the error handler logs method, path, and status_code. However, successful requests have no automatic baseline telemetry (method/path/status/duration) unless the application explicitly callslogger.*()methods. Consider adding a global response middleware to log successful requests, or ensure all critical paths call LoggerService methods. Also note that pino-http v10.5.0 has open GitHub issues reportingautoLogging: falsemay not suppress logs in some scenarios—verify this works in your environment.apps/lfx-one/src/server/services/meeting.service.ts (2)
31-31: Import path looks correct for services-layer usage.
import { logger } from './logger.service';is consistent withinapps/lfx-one/src/server/services/*.
276-280: WARN logs currently won’t use Pino’s error serializer if{ err }is nested underdata.
This call passes{ err: error }, but LoggerService.warning currently wraps metadata as{ data: metadata }, so the serializer won’t treat it as an error. Fixing LoggerService (see logger.service.ts review) will improve this across the codebase without touching every call site.apps/lfx-one/src/server/services/user.service.ts (1)
69-70: Operation timing + structured metadata usage looks consistent.
ThestartOperation/success/errorflow ingetUserInfoandupdateUserMetadatais coherent and should improve traceability.Also applies to: 87-88, 114-152
apps/lfx-one/src/server/controllers/public-meeting.controller.ts (1)
41-45: Public controller logging migration is internally consistent.
Operation names and metadata are clear, and “error handler will log” comments align with central error logging.Also applies to: 95-96, 161-164, 216-221, 237-240, 311-315, 327-337, 375-393, 399-418, 437-471
apps/lfx-one/src/server/services/logger.service.ts (1)
64-107: Good foundation: operation tracking + structured logs +errfield on ERROR.
The singleton + WeakMap-per-request design andreq.log.error({ err: error, ... })pattern align well with Pino and should reduce duplicate logging.Also applies to: 112-140, 146-184
apps/lfx-one/src/server/controllers/meeting.controller.ts (1)
16-16: Logger helper migration is correctly implemented — guideline alignment confirmed.The controller properly uses the
LoggerServicesingleton (logger) with all required methods:startOperation(),success(),error(),warning(), andvalidation(). The guideline requirement for "Logger helper methods" is satisfied; the lowercaseloggerinstance name andstartOperation()method name (vs.start()) are intentional design choices in the new service and do not represent a mismatch. No legacy Logger helper imports or calls remain. The implementation is consistent and ready.
apps/lfx-one/src/app/modules/meetings/components/meeting-card/meeting-card.component.ts
Show resolved
Hide resolved
Extended LoggerService to support both request-scoped and infrastructure operations by making req parameter optional. Migrated all 41 direct serverLogger calls across services, utilities, and routes to use the unified logger service pattern. Changes: - Extended LoggerService methods to accept Request | undefined - Migrated 11 request-scoped operations to pass req parameter - Migrated 30 infrastructure operations to use logger with undefined - Updated ai.service, user.service, project.service method signatures - Replaced serverLogger calls in nats, snowflake, lock-manager services - Updated CLAUDE.md with comprehensive logging documentation - Removed serverLogger imports from all service/utility files Benefits: - Unified logging interface for all operations - Better request correlation when context available - Consistent error handling with err field - Infrastructure operations now use same pattern LFXV2-903 Signed-off-by: Asitha de Silva <[email protected]>
Updated logging-monitoring.md to reflect the new dual-mode logger service that supports both request-scoped and infrastructure operations. Added documentation for the new server-logger.ts file that breaks the circular dependency between server.ts and logger.service.ts. Changes: - Added logging architecture layers diagram showing server-logger.ts - Updated all method signatures to show req | undefined pattern - Added infrastructure logging examples for all methods - Documented circular dependency resolution - Clarified when to use request-scoped vs infrastructure logging LFXV2-903 Signed-off-by: Asitha de Silva <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/lfx-one/src/server/services/project.service.ts (1)
326-382: Avoid logging email addresses as they constitute PII.Per coding guidelines, logging sensitive data like emails poses compliance/privacy risks (GDPR/CCPA). The
normalizedEmailis logged instartOperation,warning, andsuccesscalls throughout this method.Consider redacting or hashing email addresses before logging, or removing them from log metadata entirely. You could log a sanitized identifier instead.
- const startTime = logger.startOperation(req, 'resolve_email_to_sub', { email: normalizedEmail }); + const startTime = logger.startOperation(req, 'resolve_email_to_sub', { email_domain: normalizedEmail.split('@')[1] || 'unknown' });Similar changes should be applied to the warning and success log calls in this method (lines 340-343, 368-370, 379-382).
apps/lfx-one/src/server/utils/persona-helper.ts (1)
97-99: Missing operation closure on early return.When
matchedPersonas.length === 0, the function returns without callinglogger.success()or logging the operation completion. The operation started at line 43 remains unclosed in the operation tracking stack.// No committee memberships found if (matchedPersonas.length === 0) { + logger.success(req, 'fetch_user_persona_and_organizations', startTime, { + matched_personas: [], + selected_persona: null, + organization_count: 0, + }); return result; }apps/lfx-one/src/server/utils/lock-manager.ts (1)
100-114: Duration tracking is broken -Date.now()captured at completion time.
logger.success(undefined, 'lock_manager_shutdown', Date.now())passes the current timestamp asstartTime, resulting in ~0ms duration since both start and end times are effectively the same moment.public shutdown(): void { + const startTime = Date.now(); if (this.cleanupInterval) { clearInterval(this.cleanupInterval); this.cleanupInterval = null; } this.memoryLocks.clear(); // Future Redis cleanup // if (this.redisClient) { // this.redisClient.quit(); // } - logger.success(undefined, 'lock_manager_shutdown', Date.now()); + logger.success(undefined, 'lock_manager_shutdown', startTime); }
♻️ Duplicate comments (6)
apps/lfx-one/src/server/services/project.service.ts (1)
417-473: Same PII logging concern applies here.The
normalizedEmailis logged throughout this method as well. Apply the same redaction approach recommended forresolveEmailToSub.apps/lfx-one/src/server/middleware/error-handler.middleware.ts (2)
49-55: Duration tracking and duplicate suppression now implemented.The previous review concerns about
startTime=0and missingskipIfLoggedhave been addressed:
Date.now()is now used instead of0{ skipIfLogged: true }option is passed to prevent duplicate error logs
65-71: Unhandled error logging now uses real timestamp.The
Date.now()fix from the previous review has been applied.apps/lfx-one/src/server/server.ts (1)
192-252: SSR duration tracking now correctly implemented.The previous review concern about
startTime=0has been addressed:
ssrStartTimeis captured at handler entry (line 193)- Passed to
logger.error()(line 246) for accurate duration calculationThe structured error metadata includes all relevant context for debugging SSR failures.
apps/lfx-one/src/server/services/committee.service.ts (2)
117-121: Swallowed error - capture and includeerrfor Pino serialization.The catch block discards the error entirely. Per coding guidelines, use the
errfield to leverage Pino's error serializer.- } catch { - logger.warning(req, 'create_committee_settings', 'Failed to update committee settings, but committee was created successfully', { - committee_uid: newCommittee.uid, - }); + } catch (err) { + logger.warning(req, 'create_committee_settings', 'Failed to update committee settings, but committee was created successfully', { + err, + committee_uid: newCommittee.uid, + }); }
155-159: Same issue: capture error for proper logging.- } catch { - logger.warning(req, 'update_committee_settings', 'Failed to update committee settings, but committee was updated successfully', { - committee_uid: committeeId, - }); + } catch (err) { + logger.warning(req, 'update_committee_settings', 'Failed to update committee settings, but committee was updated successfully', { + err, + committee_uid: committeeId, + }); }
🧹 Nitpick comments (3)
apps/lfx-one/src/server/middleware/error-handler.middleware.ts (1)
31-50: Potential operation name mismatch between logContext and logger call.The
operationpassed tologger.error()on line 50 is always derived from the request path. However,logContextspreadserror.getLogContext()after settingoperation, meaning if the error contains its ownoperationfield,logContext.operationwill differ from theoperationargument passed to the logger.Consider extracting the final operation from logContext:
const logContext = { operation, status: 'failed', error_type: error.code, status_code: error.statusCode, ...error.getLogContext(), request_id: req.id, path: req.path, method: req.method, user_agent: req.get('User-Agent'), }; + const finalOperation = (logContext.operation as string) || operation; if (logLevel === 'error') { - logger.error(req, operation, Date.now(), error, logContext, { skipIfLogged: true }); + logger.error(req, finalOperation, Date.now(), error, logContext, { skipIfLogged: true }); } else if (logLevel === 'warn') { - logger.warning(req, operation, `API error: ${error.message}`, { ...logContext, err: error }); + logger.warning(req, finalOperation, `API error: ${error.message}`, { ...logContext, err: error }); } else { - logger.debug(req, operation, `API error: ${error.message}`, { ...logContext, err: error }); + logger.debug(req, finalOperation, `API error: ${error.message}`, { ...logContext, err: error }); }apps/lfx-one/src/server/services/logger.service.ts (2)
402-412: Sanitize only redacts top-level keys; nested sensitive data will leak.The
sanitizemethod checks only immediate object keys. If metadata contains nested objects (e.g.,{ user: { email: '[email protected]' } }), the nestedConsider recursively sanitizing nested objects:
public sanitize(metadata: Record<string, unknown>): Record<string, unknown> { const sanitized = { ...metadata }; Object.keys(sanitized).forEach((key) => { if (SENSITIVE_FIELDS.some((field) => key.toLowerCase().includes(field))) { sanitized[key] = '[REDACTED]'; + } else if (sanitized[key] !== null && typeof sanitized[key] === 'object' && !Array.isArray(sanitized[key])) { + sanitized[key] = this.sanitize(sanitized[key] as Record<string, unknown>); } }); return sanitized; }
253-283: Consider WARN level for validation errors instead of ERROR.Per the coding guidelines, ERROR level should be used for "system failures, unhandled exceptions, and critical errors." Validation failures are expected application behavior (client errors returning 400), not system failures.
- serverLogger.error( + serverLogger.warn( { operation, status: 'failed', error_type: 'validation', ... - req.log.error( + req.log.warn( { operation, status: 'failed', error_type: 'validation',Based on coding guidelines: "Log WARN level for error conditions leading to exceptions."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
CLAUDE.md(1 hunks)apps/lfx-one/src/server/middleware/auth.middleware.ts(16 hunks)apps/lfx-one/src/server/middleware/error-handler.middleware.ts(4 hunks)apps/lfx-one/src/server/routes/meetings.route.ts(1 hunks)apps/lfx-one/src/server/server-logger.ts(1 hunks)apps/lfx-one/src/server/server.ts(7 hunks)apps/lfx-one/src/server/services/ai.service.ts(5 hunks)apps/lfx-one/src/server/services/committee.service.ts(9 hunks)apps/lfx-one/src/server/services/logger.service.ts(1 hunks)apps/lfx-one/src/server/services/nats.service.ts(4 hunks)apps/lfx-one/src/server/services/project.service.ts(19 hunks)apps/lfx-one/src/server/services/snowflake.service.ts(13 hunks)apps/lfx-one/src/server/services/user.service.ts(11 hunks)apps/lfx-one/src/server/utils/lock-manager.ts(5 hunks)apps/lfx-one/src/server/utils/persona-helper.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/lfx-one/src/server/services/user.service.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Always use direct imports for standalone components - no barrel exports
Use TypeScript interfaces instead of union types for better maintainability
Do not nest ternary expressions
Prefer interface for defining object shapes in TypeScript
Use path mappings and import aliases as configured in tsconfig.json for imports
Files:
apps/lfx-one/src/server/routes/meetings.route.tsapps/lfx-one/src/server/utils/persona-helper.tsapps/lfx-one/src/server/server-logger.tsapps/lfx-one/src/server/utils/lock-manager.tsapps/lfx-one/src/server/services/ai.service.tsapps/lfx-one/src/server/services/snowflake.service.tsapps/lfx-one/src/server/server.tsapps/lfx-one/src/server/services/committee.service.tsapps/lfx-one/src/server/services/project.service.tsapps/lfx-one/src/server/services/nats.service.tsapps/lfx-one/src/server/middleware/error-handler.middleware.tsapps/lfx-one/src/server/services/logger.service.tsapps/lfx-one/src/server/middleware/auth.middleware.ts
**/*.{html,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always add data-testid attributes when creating new components for reliable test targeting
Files:
apps/lfx-one/src/server/routes/meetings.route.tsapps/lfx-one/src/server/utils/persona-helper.tsapps/lfx-one/src/server/server-logger.tsapps/lfx-one/src/server/utils/lock-manager.tsapps/lfx-one/src/server/services/ai.service.tsapps/lfx-one/src/server/services/snowflake.service.tsapps/lfx-one/src/server/server.tsapps/lfx-one/src/server/services/committee.service.tsapps/lfx-one/src/server/services/project.service.tsapps/lfx-one/src/server/services/nats.service.tsapps/lfx-one/src/server/middleware/error-handler.middleware.tsapps/lfx-one/src/server/services/logger.service.tsapps/lfx-one/src/server/middleware/auth.middleware.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: License headers are required on all source files
Prepend 'Generated with Claude Code (https://claude.ai/code)' if assisted with the code
Files:
apps/lfx-one/src/server/routes/meetings.route.tsapps/lfx-one/src/server/utils/persona-helper.tsapps/lfx-one/src/server/server-logger.tsapps/lfx-one/src/server/utils/lock-manager.tsapps/lfx-one/src/server/services/ai.service.tsapps/lfx-one/src/server/services/snowflake.service.tsapps/lfx-one/src/server/server.tsapps/lfx-one/src/server/services/committee.service.tsapps/lfx-one/src/server/services/project.service.tsapps/lfx-one/src/server/services/nats.service.tsapps/lfx-one/src/server/middleware/error-handler.middleware.tsapps/lfx-one/src/server/services/logger.service.tsapps/lfx-one/src/server/middleware/auth.middleware.ts
apps/**/**/server/**/*.{ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/**/**/server/**/*.{ts,js}: Use Pino for structured JSON logs with sensitive data redaction in all backend services
Always use err field for errors to leverage Pino's error serializer - correct: req.log.error({ err: error, ...metadata }, 'message')
Log INFO level for business operation completions (created, updated, deleted) and successful data retrieval
Log WARN level for error conditions leading to exceptions and data quality issues
Log DEBUG level for internal operations, preparation steps, and intent statements
Log ERROR level for system failures, unhandled exceptions, and critical errors
Files:
apps/lfx-one/src/server/routes/meetings.route.tsapps/lfx-one/src/server/utils/persona-helper.tsapps/lfx-one/src/server/server-logger.tsapps/lfx-one/src/server/utils/lock-manager.tsapps/lfx-one/src/server/services/ai.service.tsapps/lfx-one/src/server/services/snowflake.service.tsapps/lfx-one/src/server/server.tsapps/lfx-one/src/server/services/committee.service.tsapps/lfx-one/src/server/services/project.service.tsapps/lfx-one/src/server/services/nats.service.tsapps/lfx-one/src/server/middleware/error-handler.middleware.tsapps/lfx-one/src/server/services/logger.service.tsapps/lfx-one/src/server/middleware/auth.middleware.ts
🧬 Code graph analysis (12)
apps/lfx-one/src/server/routes/meetings.route.ts (1)
apps/lfx-one/src/server/services/logger.service.ts (1)
logger(449-449)
apps/lfx-one/src/server/utils/persona-helper.ts (1)
apps/lfx-one/src/server/services/logger.service.ts (2)
logger(449-449)error(186-247)
apps/lfx-one/src/server/server-logger.ts (1)
apps/lfx-one/src/server/helpers/error-serializer.ts (1)
customErrorSerializer(11-37)
apps/lfx-one/src/server/utils/lock-manager.ts (1)
apps/lfx-one/src/server/services/logger.service.ts (1)
logger(449-449)
apps/lfx-one/src/server/services/ai.service.ts (2)
packages/shared/src/interfaces/ai.interface.ts (3)
GenerateAgendaRequest(9-20)GenerateAgendaResponse(25-30)OpenAIChatResponse(75-84)apps/lfx-one/src/server/services/logger.service.ts (2)
logger(449-449)error(186-247)
apps/lfx-one/src/server/services/snowflake.service.ts (3)
apps/lfx-one/src/server/services/logger.service.ts (2)
logger(449-449)error(186-247)packages/shared/src/interfaces/snowflake.interface.ts (1)
SnowflakeQueryResult(10-14)packages/shared/src/constants/snowflake.constant.ts (1)
SNOWFLAKE_CONFIG(7-62)
apps/lfx-one/src/server/server.ts (1)
apps/lfx-one/src/server/services/logger.service.ts (2)
logger(449-449)error(186-247)
apps/lfx-one/src/server/services/project.service.ts (2)
apps/lfx-one/src/server/services/logger.service.ts (2)
logger(449-449)error(186-247)packages/shared/src/interfaces/project.interface.ts (1)
ProjectSlugToIdResponse(157-161)
apps/lfx-one/src/server/services/nats.service.ts (2)
apps/lfx-one/src/server/services/logger.service.ts (2)
logger(449-449)error(186-247)packages/shared/src/constants/api.constants.ts (1)
NATS_CONFIG(28-43)
apps/lfx-one/src/server/middleware/error-handler.middleware.ts (2)
apps/lfx-one/src/server/errors/index.ts (1)
isBaseApiError(27-29)apps/lfx-one/src/server/services/logger.service.ts (2)
error(186-247)logger(449-449)
apps/lfx-one/src/server/services/logger.service.ts (2)
apps/lfx-one/src/server/server-logger.ts (1)
serverLogger(29-70)packages/shared/src/constants/server.constants.ts (1)
SENSITIVE_FIELDS(8-26)
apps/lfx-one/src/server/middleware/auth.middleware.ts (1)
apps/lfx-one/src/server/services/logger.service.ts (2)
logger(449-449)error(186-247)
🪛 GitHub Actions: Markdown Lint
CLAUDE.md
[error] 204-204: MD040: Fenced code blocks should have a language specified. Context: '```' without a language in CLAUDE.md:204.
🪛 GitHub Check: markdown-lint
CLAUDE.md
[failure] 204-204: Fenced code blocks should have a language specified [Context: ""] CLAUDE.md:204 MD040/fenced-code-language Fenced code blocks should have a language specified [Context: ""]
🔇 Additional comments (25)
apps/lfx-one/src/server/services/project.service.ts (7)
52-52: LGTM!Import correctly references the singleton logger instance from the new logger service, aligning with the migration objective.
129-132: LGTM!Correct use of
logger.warningfor a data quality issue per coding guidelines. Metadata is appropriately non-sensitive.
231-234: LGTM!Appropriate use of
logger.debugfor internal preparation steps per coding guidelines.
289-308: LGTM!Correct
startOperation/successpattern wrapping the ETag update operation with appropriate metadata.
523-572: LGTM!The remaining logging in
getUserInfocorrectly uses the non-PIIusernameidentifier. Log levels are appropriate per coding guidelines.
1323-1355: LGTM!Method signature correctly updated to accept
reqfor request-scoped logging. Logging levels are appropriate:
DEBUGfor internal operations (slug lookup outcomes)WARNINGfor error conditions (NATS failures)- Correctly uses
errfield for error serialization per coding guidelines
164-164: LGTM!Caller correctly updated to pass
reqto match the new method signature.apps/lfx-one/src/server/server-logger.ts (1)
1-70: Well-structured base logger configuration.The server-wide logger is well-configured with:
- Environment-aware pretty printing
- AWS trace ID integration via mixin
- Proper use of
customErrorSerializerfor theerrfield per coding guidelines- Sensitive data redaction
- ISO timestamps for CloudWatch compatibility
One observation on the redaction strategy:
- In non-production (lines 54),
req.headers.*redacts all request headers, which may hinder local debugging. Consider redacting only sensitive headers (like authorization, cookie) in development as well.apps/lfx-one/src/server/services/snowflake.service.ts (6)
12-13: Logger service migration looks correct.The import and usage of the centralized logger service follows the established patterns.
41-46: Good defensive logging for SSR build safety.The try-catch wrapper prevents logging errors during SSR build when the logger may not be fully initialized. This is a pragmatic approach for singleton initialization.
95-150: Query execution logging follows the correct pattern.
startOperationcaptures timing at the beginningsuccesslogs with duration on completionerrorlogs with duration and wraps inMicroserviceErrorThe structured metadata (query_hash, row_count, pool stats) provides good observability.
198-223: Shutdown logging pattern is correct.The
startTimeis properly captured fromlogger.startOperation()and passed to bothsuccessanderrorpaths, ensuring accurate duration tracking.
260-356: Pool creation logging is comprehensive.Good coverage of:
- Operation start with timing
- Missing environment variable validation with detailed metadata
- Debug logs for key configuration steps
- Success with pool configuration details
- Error with account context
387-401: Validation warnings provide good security audit trail.Logging blocked queries with SQL preview and matched pattern helps with security monitoring and debugging without exposing full query content.
apps/lfx-one/src/server/middleware/error-handler.middleware.ts (1)
9-22: Helper function correctly derives operation from path.The
getOperationFromPathfunction properly normalizes request paths by:
- Removing leading slashes
- Stripping UUIDs and numeric IDs
- Converting to snake_case
This provides consistent operation naming for error logging context.
apps/lfx-one/src/server/server.ts (3)
20-21: Correct dual import of serverLogger and logger service.
serverLoggeris appropriately used as the base logger forpinoHttp, whileloggerservice is used for application-level logging. This maintains the architecture separation.
102-103: Disabling autoLogging prevents duplicate request logs.With
autoLogging: false, the LoggerService now controls operation logging, avoiding redundant request/response logs from pinoHttp while still providingreq.logfor request-scoped logging.
283-288: Server startup logging uses appropriate DEBUG level.Per coding guidelines, DEBUG level is correct for "internal operations" and "infrastructure operations." The metadata includes useful deployment context (port, node_env, PM2 status).
CLAUDE.md (3)
102-165: Comprehensive logging documentation aligns with coding guidelines.The documentation accurately reflects the logging architecture and provides clear guidance on:
- When to use request-scoped vs infrastructure logging
- Proper use of
errfield for error serialization- Log level selection per the coding guidelines
The code examples demonstrate correct patterns that match the implementation in the reviewed files.
166-198: Log level guidelines match coding standards.The documented levels align with the coding guidelines:
- INFO for business operation completions
- WARN for recoverable issues
- DEBUG for internal/infrastructure operations
- ERROR for critical failures
225-298: Controller and service logging patterns are well-documented.The example patterns correctly demonstrate:
- Capturing
startTimefromlogger.startOperation()- Passing
reqfor request context orundefinedfor infrastructure- Using
logger.success()andlogger.error()with timing- Propagating
reqto internal methods for correlationapps/lfx-one/src/server/routes/meetings.route.ts (2)
89-94: LGTM: Service call updated correctly.The service call properly passes
reqas the first parameter, aligning with the updatedgenerateMeetingAgendasignature for request-scoped logging.
96-98: LGTM: Success logging properly closes the operation.The
logger.success()call correctly passesstartTimefor duration tracking and includes relevant metadata.apps/lfx-one/src/server/services/ai.service.ts (2)
27-33: LGTM: Method signature updated correctly for request-scoped logging.The updated signature accepting
req: Requestas the first parameter enables proper request-scoped operation tracking and aligns with the centralized logger service pattern.
180-183: LGTM: Proper error field usage in warning log.The warning log correctly uses the
errfield for the parse error, leveraging Pino's error serializer as per coding guidelines.As per coding guidelines: "Always use err field for errors to leverage Pino's error serializer."
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/architecture/backend/logging-monitoring.md (2)
628-648: Add request correlation and tracing guidance.The environment configuration section (lines 650–661) mentions
_X_AMZN_TRACE_IDfor AWS X-Ray tracing, but the main documentation does not explain how request correlation IDs or trace IDs integrate with LoggerService logging. Best practices do not address correlation ID propagation or how to use trace IDs in structured logging metadata.Add guidance on:
- Automatically including trace IDs in log output
- Propagating correlation IDs across service calls
- Querying related logs by trace ID in CloudWatch
This will improve observability for distributed tracing scenarios.
I can add a new section documenting request correlation patterns and trace ID integration if you'd like.
85-90: Minor: Simplify wording in the returns description."startTime for use with success/error calls" is slightly verbose. Consider shorter phrasing.
-**Returns:** `number` - startTime for use with success/error calls +**Returns:** `number` - startTime for success/error calls
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
docs/architecture/backend/logging-monitoring.md(1 hunks)
🧰 Additional context used
🪛 GitHub Check: markdown-lint
docs/architecture/backend/logging-monitoring.md
[failure] 35-35: Fenced code blocks should have a language specified [Context: ""] docs/architecture/backend/logging-monitoring.md:35 MD040/fenced-code-language Fenced code blocks should have a language specified [Context: ""]
🪛 LanguageTool
docs/architecture/backend/logging-monitoring.md
[style] ~88-~88: ‘with success’ might be wordy. Consider a shorter alternative.
Context: ...Returns:** number - startTime for use with success/error calls Log Level: INFO (unles...
(EN_WORDINESS_PREMIUM_WITH_SUCCESS)
🔇 Additional comments (2)
docs/architecture/backend/logging-monitoring.md (2)
388-425: Remove this comment —getOperationFromPath()is already documented.The utility function at
apps/lfx-one/src/server/middleware/error-handler.middleware.ts:12has clear JSDoc documentation stating "Derives operation name from request path for logging context." The implementation demonstrates the mapping logic: removing leading slashes, filtering out UUIDs, and converting slashes to underscores (e.g.,/api/v1/meetings/123→api_v1_meetings). This function is a private utility within the error-handler middleware and is not intended for external export. The documentation provided in the code is sufficient for the pattern shown in the logging-monitoring.md architecture document.
59-250: Clarify method visibility in LoggerService before documenting.The review references
etag(),sanitize(),hasOperation(), andgetOperationStartTime()based on PR objectives and enriched summary, but I cannot verify their actual existence or visibility in the codebase. Please confirm:
- Whether these methods are part of the public API surface or internal utilities
- The actual method signatures and their role in the logging system
- Whether they should be exposed in the public documentation
If these are public API methods, comprehensive documentation with usage examples is needed.
Moved AI agenda generation logic from inline route handler to MeetingController.generateAgenda method for better separation of concerns and consistency with other route patterns. Also improved logger usage in middleware and services: - auth.middleware: better error tracking with startTime - error-handler.middleware: use operation startTime when available - access-check.service: move startOperation before try block - etag.service: use debug instead of silent startOperation - persona-helper: use success/error instead of warning LFXV2-903 Signed-off-by: Asitha de Silva <[email protected]>
Updated CLAUDE.md logging architecture diagram to reflect the new server-logger.ts file that breaks the circular dependency between server.ts and logger.service.ts. LFXV2-903 Signed-off-by: Asitha de Silva <[email protected]>
Signed-off-by: Asitha de Silva <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/lfx-one/src/server/middleware/auth.middleware.ts (1)
112-118: Fix operation name mismatch in token refresh success log.Line 117 logs success for operation
'token_refresh', but the operation started at line 88 is'token_extraction'. When the operation names don't match, the LoggerService cannot properly clean up the operation stack (the'token_extraction'entry remains), and you're logging success for an operation that was never started. This creates inconsistency in operation tracking and a potential memory issue with the WeakMap-based operation stack.Apply this diff to use the correct operation name:
const refreshedToken = await req.oidc.accessToken.refresh(); if (refreshedToken?.access_token) { req.bearerToken = refreshedToken.access_token; - logger.success(req, 'token_refresh', startTime, { path: req.path }); + logger.success(req, 'token_extraction', startTime, { + path: req.path, + token_refreshed: true + }); return { success: true, needsLogout: false }; }apps/lfx-one/src/server/utils/persona-helper.ts (1)
65-92: Avoid logging rawusernamein debug logs (likely PII); rely on request correlation or redact
logger.debug(... { username, ... })(Line 76-81) risks emitting identifiers broadly at DEBUG. Consider removingusernamefrom debug, or logging a redacted form if LoggerService supports it.- logger.debug(req, 'matched_persona', 'Persona matched for user', { - username, + logger.debug(req, 'matched_persona', 'Persona matched for user', { category, persona, memberships_count: memberships.length, });apps/lfx-one/src/server/middleware/error-handler.middleware.ts (1)
40-48: Preventerror.getLogContext()from overridingerror_type/status_code.
Because...error.getLogContext()comes aftererror_type/status_code, it can overwrite those fields.- const logContext = { - error_type: error.code, - status_code: error.statusCode, - ...error.getLogContext(), + const logContext = { + ...error.getLogContext(), + error_type: error.code, + status_code: error.statusCode, request_id: req.id, path: req.path, method: req.method, user_agent: req.get('User-Agent'), };
♻️ Duplicate comments (2)
apps/lfx-one/src/server/services/etag.service.ts (1)
21-57: Consider usingstartOperation()/success()/error()here to get duration + dedupe semanticsRight now this service logs debug/warning messages but doesn’t use the LoggerService operation lifecycle APIs, so you won’t get duration_ms tracking for these operations (and any per-operation state management the service provides).
Also applies to: 64-67, 73-76
apps/lfx-one/src/server/middleware/error-handler.middleware.ts (1)
9-22: Operation derivation likely won’t matchstartOperation()names (breaks duration + dedupe correlation).
Right nowoperationis path-derived, but most call sites typically pass explicit operation keys tologger.startOperation(); if they differ,getOperationStartTime()won’t find the tracked start time andskipIfLoggedwon’t suppress duplicates for the “real” operation.Suggested adjustment: prefer an operation coming from structured error context (when available), otherwise fall back to the path-derived operation.
- const operation = getOperationFromPath(req.path); - - // Try to get the operation start time if it was tracked, otherwise use current time - const startTime = logger.getOperationStartTime(req, operation) || Date.now(); + const fallbackOperation = getOperationFromPath(req.path); + const operation = + (isBaseApiError(error) ? (error.getLogContext()?.['operation'] as string | undefined) : undefined) ?? fallbackOperation; + + // Try to get the operation start time if it was tracked, otherwise use current time + const startTime = logger.getOperationStartTime(req, operation) || Date.now();Also applies to: 31-35
🧹 Nitpick comments (3)
apps/lfx-one/src/server/utils/persona-helper.ts (1)
5-9: Use type-only import for ExpressRequestto avoid runtime dependency
Requestis used only as a type here; preferimport type(keeps emitted JS cleaner).-import { Request } from 'express'; +import type { Request } from 'express';apps/lfx-one/src/server/services/snowflake.service.ts (2)
90-96: Use thestartTimereturned bylogger.startOperation()for consistency.
You’re manually doingDate.now()+startOperation()inexecute(), while other methods use the return value ofstartOperation().- const startTime = Date.now(); - logger.startOperation(undefined, 'snowflake_query', { + const startTime = logger.startOperation(undefined, 'snowflake_query', { query_hash: queryHash, - sql_preview: sqlText.substring(0, 100), bind_count: binds?.length || 0, });Also applies to: 124-129
40-45: Don’t mention secret env var names in logs; avoid swallowing logger failures broadly.
The message referencesSNOWFLAKE_API_KEYand thetry/catch {}will hide real logger faults. Consider narrowing the try/catch (or ensuring logger never throws) and use a generic message.- try { - logger.debug(undefined, 'snowflake_singleton', 'SnowflakeService singleton instance created', {}); - } catch { - // Silently ignore logging errors during SSR build - } + // If SSR build can execute this module, ensure logger is safe/no-op in SSR instead of swallowing errors here. + logger.debug(undefined, 'snowflake_singleton', 'SnowflakeService singleton instance created', {}); ... - logger.debug(undefined, 'snowflake_pool_creation', 'Using SNOWFLAKE_API_KEY from environment variable', {}); + logger.debug(undefined, 'snowflake_pool_creation', 'Using Snowflake private key from environment', {});Also applies to: 293-294
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
apps/lfx-one/src/server/controllers/meeting.controller.ts(48 hunks)apps/lfx-one/src/server/middleware/auth.middleware.ts(16 hunks)apps/lfx-one/src/server/middleware/error-handler.middleware.ts(4 hunks)apps/lfx-one/src/server/routes/meetings.route.ts(2 hunks)apps/lfx-one/src/server/services/access-check.service.ts(3 hunks)apps/lfx-one/src/server/services/etag.service.ts(3 hunks)apps/lfx-one/src/server/services/nats.service.ts(4 hunks)apps/lfx-one/src/server/services/snowflake.service.ts(13 hunks)apps/lfx-one/src/server/utils/persona-helper.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/lfx-one/src/server/services/nats.service.ts
- apps/lfx-one/src/server/routes/meetings.route.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Always use direct imports for standalone components - no barrel exports
Use TypeScript interfaces instead of union types for better maintainability
Do not nest ternary expressions
Prefer interface for defining object shapes in TypeScript
Use path mappings and import aliases as configured in tsconfig.json for imports
Files:
apps/lfx-one/src/server/utils/persona-helper.tsapps/lfx-one/src/server/services/etag.service.tsapps/lfx-one/src/server/services/access-check.service.tsapps/lfx-one/src/server/services/snowflake.service.tsapps/lfx-one/src/server/controllers/meeting.controller.tsapps/lfx-one/src/server/middleware/auth.middleware.tsapps/lfx-one/src/server/middleware/error-handler.middleware.ts
**/*.{html,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always add data-testid attributes when creating new components for reliable test targeting
Files:
apps/lfx-one/src/server/utils/persona-helper.tsapps/lfx-one/src/server/services/etag.service.tsapps/lfx-one/src/server/services/access-check.service.tsapps/lfx-one/src/server/services/snowflake.service.tsapps/lfx-one/src/server/controllers/meeting.controller.tsapps/lfx-one/src/server/middleware/auth.middleware.tsapps/lfx-one/src/server/middleware/error-handler.middleware.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: License headers are required on all source files
Prepend 'Generated with Claude Code (https://claude.ai/code)' if assisted with the code
Files:
apps/lfx-one/src/server/utils/persona-helper.tsapps/lfx-one/src/server/services/etag.service.tsapps/lfx-one/src/server/services/access-check.service.tsapps/lfx-one/src/server/services/snowflake.service.tsapps/lfx-one/src/server/controllers/meeting.controller.tsapps/lfx-one/src/server/middleware/auth.middleware.tsapps/lfx-one/src/server/middleware/error-handler.middleware.ts
apps/**/**/server/**/*.{ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/**/**/server/**/*.{ts,js}: Use Pino for structured JSON logs with sensitive data redaction in all backend services
Always use err field for errors to leverage Pino's error serializer - correct: req.log.error({ err: error, ...metadata }, 'message')
Log INFO level for business operation completions (created, updated, deleted) and successful data retrieval
Log WARN level for error conditions leading to exceptions and data quality issues
Log DEBUG level for internal operations, preparation steps, and intent statements
Log ERROR level for system failures, unhandled exceptions, and critical errors
Files:
apps/lfx-one/src/server/utils/persona-helper.tsapps/lfx-one/src/server/services/etag.service.tsapps/lfx-one/src/server/services/access-check.service.tsapps/lfx-one/src/server/services/snowflake.service.tsapps/lfx-one/src/server/controllers/meeting.controller.tsapps/lfx-one/src/server/middleware/auth.middleware.tsapps/lfx-one/src/server/middleware/error-handler.middleware.ts
apps/**/**/server/**/*controller*.{ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
All controller functions must use Logger helper methods: Logger.start(), Logger.success(), Logger.error(), Logger.warning(), Logger.validation()
Files:
apps/lfx-one/src/server/controllers/meeting.controller.ts
🧬 Code graph analysis (4)
apps/lfx-one/src/server/services/etag.service.ts (3)
apps/lfx-one/src/server/services/logger.service.ts (3)
logger(449-449)error(186-247)etag(289-324)packages/shared/src/interfaces/api.interface.ts (1)
ETagError(94-103)packages/shared/src/constants/server.constants.ts (1)
HTTP_HEADERS(31-39)
apps/lfx-one/src/server/services/access-check.service.ts (1)
apps/lfx-one/src/server/services/logger.service.ts (2)
logger(449-449)error(186-247)
apps/lfx-one/src/server/middleware/auth.middleware.ts (1)
apps/lfx-one/src/server/services/logger.service.ts (2)
logger(449-449)error(186-247)
apps/lfx-one/src/server/middleware/error-handler.middleware.ts (2)
apps/lfx-one/src/server/services/logger.service.ts (2)
logger(449-449)error(186-247)apps/lfx-one/src/server/errors/index.ts (1)
isBaseApiError(27-29)
🔇 Additional comments (14)
apps/lfx-one/src/server/services/access-check.service.ts (4)
7-7: LGTM! Clean migration to centralized logger service.The import correctly references the singleton LoggerService, aligning with the PR's objective to replace the static Logger helper.
31-35: Excellent structured operation tracking with meaningful metrics.The operation start correctly captures request count, unique resource types, and unique access types. The Set-based deduplication for metrics provides valuable observability with minimal overhead for typical batch sizes.
87-90: Well-structured success logging with complementary metrics.The success call correctly uses the same operation name, automatic duration tracking, and adds the granted_count metric to complement the initial request_count. This provides complete visibility into access check outcomes.
94-97: Correct error logging for handled exception.The error logging is appropriate here since the exception is handled with a safe fallback (all access denied) rather than propagated. The metadata correctly documents the fallback_behavior, and the operation tracking will be cleaned up automatically by the logger service.
apps/lfx-one/src/server/middleware/auth.middleware.ts (1)
68-407: Migration to LoggerService looks good overall.The migration from the old Logger helper to the centralized LoggerService is well-executed. Operation tracking is properly implemented with captured
startTimevariables and matching operation names (except for the issue at line 117). The use oflogger.debug()for decision breadcrumbs,logger.warning()for auth failures, andlogger.error()for exceptions aligns with the coding guidelines.The concerns raised in the previous review appear to have been addressed—all the problematic uncaptured
startOperation()calls have been replaced withlogger.debug()calls, andlogger.warning()calls no longer includeerrfields in metadata.apps/lfx-one/src/server/utils/persona-helper.ts (2)
43-58: Operation is properly closed on the “no username” path
startOperation()+success()before returning keeps the operation tracking consistent.
43-128: Error serialization is correct; username redaction is not requiredThe LoggerService properly emits errors under the
errkey (line 247:req.log.error({ ... err: error, ... })), which complies with the Pino guideline. However,usernameis not classified as a sensitive field inSENSITIVE_FIELDS(which includes password, token, secret, key, authorization, cookie, jwt, bearer, auth, credentials, apikey, api_key, access_token, refresh_token, email, passcode, organizers). Logging username without redaction is acceptable per the codebase design.Likely an incorrect or invalid review comment.
apps/lfx-one/src/server/controllers/meeting.controller.ts (7)
4-20: LGTM! Imports properly updated for logger service migration.The imports correctly reflect the migration from the old Logger helper to the new singleton logger service, and appropriately add support for AI-powered agenda generation.
27-27: LGTM! AI service properly instantiated.The AI service instance follows the same pattern as the existing meetingService, maintaining consistency in the controller's architecture.
33-35: LGTM! Logger service migration executed consistently.All controller methods have been correctly migrated to use the new logger service with:
- Request-scoped operation tracking via
logger.startOperation(req, ...)- Proper operation lifecycle logging with
logger.success(req, ...)- Consistent metadata capture for request correlation
- Appropriate use of
logger.sanitize()for query parametersThe migration maintains proper context propagation and improves structured logging across all operations.
Also applies to: 78-82, 95-97, 104-106, 120-122, 140-144, 177-184, 191-195, 212-219, 237-242, 257-259, 277-280, 295-298, 326-330, 348-351, 368-372, 394-398, 446-451, 486-490, 541-546, 574-578, 644-649, 674-677, 708-711, 730-735, 766-768, 785-788, 804-808, 823-825, 841-843, 859-863, 918-921, 938-942, 987-991, 1017-1020, 1048-1052, 1063-1066, 1094-1098, 1112-1114, 1131-1135, 1149-1151, 1168-1172
154-158: LGTM! Error and warning logging properly contextualizes failures.The error and warning logging patterns correctly distinguish between:
- Non-fatal errors that allow operations to continue (registrants fetch failure)
- Degraded functionality warnings with graceful fallbacks (metadata fetch)
- Individual batch operation failures with proper context
Each log maintains operation correlation with appropriate metadata.
Also applies to: 980-984, 1320-1323
1206-1217: LGTM! AI agenda generation properly integrated.The AI service integration follows the established controller pattern with proper operation lifecycle logging and response handling. The success path correctly captures operation completion with relevant metadata.
1226-1280: LGTM! Batch operation helper correctly refactored.The
processRegistrantOperationshelper properly:
- Marks the unused parent
_startTimeparameter with an underscore prefix- Creates its own operation tracking via
logger.startOperationfor batch processing- Maintains fail-fast behavior for 403 errors
- Logs batch processing completion with appropriate metadata
The nested operation tracking aligns with the new logger service's request-scoped operation management.
1283-1344: LGTM! Batch response creation properly instrumented.The
createBatchResponsehelper correctly:
- Tracks response construction as a nested operation
- Logs individual failures with full context and error details
- Maintains request correlation throughout batch processing
- Provides comprehensive summary logging
The nested operation tracking provides detailed observability for batch operations.
LFXV2-903 Use ServiceValidationError.fromFieldErrors() and next() for validation errors instead of manual 400 response, consistent with other controller methods. Provides field-level error details and centralized error handling through middleware. Signed-off-by: Asitha de Silva <[email protected]>
Signed-off-by: Asitha de Silva <[email protected]>
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 (3)
apps/lfx-one/src/server/controllers/meeting.controller.ts (3)
119-158:getMeetingById: avoid loggingsuccessthenerrorfor the same operation/startTime
Right now the request logssuccess(Line 140-145) and then may logerror(Line 155-158) for a non-fatal registrants enrichment failure, using the sameoperation+startTime. That can (a) misclassify healthy requests as errors, and (b) interfere with operation tracking/cleanup/dedup inLoggerService.Prefer either:
- Move the main
logger.success('get_meeting_by_id', ...)to the end (after enrichment), and treat registrants enrichment failure aswarning, or- Use a separate operation name/startTime for the enrichment step.
Example (option 2):
- // Log the success - logger.success(req, 'get_meeting_by_id', startTime, { - meeting_uid: uid, - project_uid: meeting.project_uid, - title: meeting.title, - }); - // TODO: Remove this once we have a way to get the registrants count try { + const registrantsStartTime = logger.startOperation(req, 'get_meeting_registrants_count', { + meeting_uid: uid, + }); const registrants = await this.meetingService.getMeetingRegistrants(req, meeting.uid); const committeeMembers = registrants.filter((r) => r.type === 'committee').length ?? 0; meeting.individual_registrants_count = registrants.length - committeeMembers; meeting.committee_members_count = committeeMembers; + logger.success(req, 'get_meeting_registrants_count', registrantsStartTime, { + meeting_uid: uid, + registrant_count: registrants.length, + }); } catch (error) { - logger.error(req, 'get_meeting_by_id', startTime, error, { + logger.warning(req, 'get_meeting_registrants_count', 'Failed to fetch registrants count', { meeting_uid: uid, + err: error, }); }
975-985: Use{ err }instead of{ error }to leverage Pino error serialization
logger.warning(..., { error: ... })likely won’t serialize stack/details consistently. Prefer passing the actual error undererr(and avoid manually reducing it to message unless needed). As per coding guidelines, useerrfor errors with Pino.- logger.warning(req, 'get_meeting_attachment_metadata', 'Failed to fetch metadata, using defaults', { + logger.warning(req, 'get_meeting_attachment_metadata', 'Failed to fetch metadata, using defaults', { meeting_uid: uid, attachment_id: attachmentId, - error: metadataError instanceof Error ? metadataError.message : metadataError, + err: metadataError, });
1298-1330: Per-itemlogger.error()calls with identicaloperationNamemay skip subsequent failures if deduplication is enabledIn lines 1320-1329, each failure is logged with
logger.error(req, operationName, startTime, error, {...})whereoperationNameis constant across the loop. Iflogger.error()deduplicates by operation name, only the first failure will be logged, losing per-item diagnostics.Verify the
loggerimplementation's deduplication behavior and either:
- Use a distinct operation name for per-item failures (e.g.,
${operationName}_item_failure), or- Disable deduplication explicitly if supported by the logger
🧹 Nitpick comments (3)
apps/lfx-one/src/server/controllers/meeting.controller.ts (3)
26-28: Consider injectingAiService(testability) rather thannewin-field
If you unit test this controller,private aiService: AiService = new AiService()makes mocking harder; constructor injection (defaulting tonew AiService()) keeps runtime behavior but improves tests.
175-184: AvoidJSON.stringify(req.body).lengthforbody_size(perf + memory)
Stringifying large request bodies (or arrays) just to compute size is avoidable overhead. Preferreq.headers['content-length']when present, or omitbody_size.- body_size: JSON.stringify(req.body).length, + body_size: req.headers['content-length'] ? Number(req.headers['content-length']) : undefined,Also applies to: 212-219, 394-398, 486-490, 574-578
1232-1257: Guard against emptyinputDatainprocessRegistrantOperations(defensive)
Callers currently validate non-empty arrays, but this helper will throw oninputData[0]if reused elsewhere. A small guard makes it safer.): Promise<{ results: PromiseSettledResult<R>[]; shouldReturn: boolean }> { + if (inputData.length === 0) { + return { results: [], shouldReturn: false }; + } const helperStartTime = logger.startOperation(req, `${operationName}_batch_processing`, { meeting_uid: meetingUid, batch_size: inputData.length, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/lfx-one/src/server/controllers/meeting.controller.ts(48 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Always use direct imports for standalone components - no barrel exports
Use TypeScript interfaces instead of union types for better maintainability
Do not nest ternary expressions
Prefer interface for defining object shapes in TypeScript
Use path mappings and import aliases as configured in tsconfig.json for imports
Files:
apps/lfx-one/src/server/controllers/meeting.controller.ts
**/*.{html,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always add data-testid attributes when creating new components for reliable test targeting
Files:
apps/lfx-one/src/server/controllers/meeting.controller.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: License headers are required on all source files
Prepend 'Generated with Claude Code (https://claude.ai/code)' if assisted with the code
Files:
apps/lfx-one/src/server/controllers/meeting.controller.ts
apps/**/**/server/**/*.{ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/**/**/server/**/*.{ts,js}: Use Pino for structured JSON logs with sensitive data redaction in all backend services
Always use err field for errors to leverage Pino's error serializer - correct: req.log.error({ err: error, ...metadata }, 'message')
Log INFO level for business operation completions (created, updated, deleted) and successful data retrieval
Log WARN level for error conditions leading to exceptions and data quality issues
Log DEBUG level for internal operations, preparation steps, and intent statements
Log ERROR level for system failures, unhandled exceptions, and critical errors
Files:
apps/lfx-one/src/server/controllers/meeting.controller.ts
apps/**/**/server/**/*controller*.{ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
All controller functions must use Logger helper methods: Logger.start(), Logger.success(), Logger.error(), Logger.warning(), Logger.validation()
Files:
apps/lfx-one/src/server/controllers/meeting.controller.ts
🧬 Code graph analysis (1)
apps/lfx-one/src/server/controllers/meeting.controller.ts (4)
apps/lfx-one/src/server/services/ai.service.ts (1)
AiService(11-199)apps/lfx-one/src/server/services/logger.service.ts (2)
logger(449-449)error(186-247)apps/lfx-one/src/server/errors/index.ts (1)
ServiceValidationError(7-7)packages/shared/src/interfaces/ai.interface.ts (1)
GenerateAgendaResponse(25-30)
🔇 Additional comments (3)
apps/lfx-one/src/server/controllers/meeting.controller.ts (3)
15-20: Controller logging guideline mismatch (Logger. vslogger.*) — please confirm expected standard*
Coding guidelines say controllers must useLogger.start()/success()/error()/..., but this file now uses theloggersingleton service. If the migration is intentional (per PR objective), please update/align the guideline to avoid future regressions. As per coding guidelines, controllers should follow the prescribed logging API.
32-35: Structured/sanitized query logging looks good
logger.sanitize(req.query)instartOperationand a compactsuccesspayload is a solid pattern for CloudWatch-friendly logs.Also applies to: 78-82
1184-1226:generateAgenda: validation + error propagation is now consistent
UsingServiceValidationError.fromFieldErrors(...); return next(validationError);aligns with centralized error handling and avoids controller-owned 400 responses. This also matches the previously raised concern in past review comments.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/architecture/backend/logging-monitoring.md (1)
88-88: Minor wordiness: Simplify the return documentation.The phrase "for use with success/error calls" is slightly redundant. Consider tightening to improve readability.
-**Returns:** `number` - startTime for use with success/error calls +**Returns:** `number` - startTime for duration tracking in success/error calls
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
docs/architecture/backend/logging-monitoring.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/architecture/backend/logging-monitoring.md
[style] ~88-~88: ‘with success’ might be wordy. Consider a shorter alternative.
Context: ...Returns:** number - startTime for use with success/error calls Log Level: INFO (unles...
(EN_WORDINESS_PREMIUM_WITH_SUCCESS)
🔇 Additional comments (2)
docs/architecture/backend/logging-monitoring.md (2)
35-52: ✅ Previous markdown-lint issue resolved.The code block now correctly includes the
textlanguage identifier, addressing the earlier concern.
1-661: Comprehensive and well-structured documentation.The architectural guide clearly explains:
- The singleton LoggerService design and operation lifecycle
- Layered architecture (server-logger.ts → server.ts → logger.service.ts)
- All public API methods with parameter documentation and examples
- Duplicate prevention mechanism and error handling patterns
- Duration tracking gotchas with SSR examples
- CloudWatch optimization guidance and operation naming conventions
- Security redaction and safe metadata practices
- Best practices and a code review checklist
The documentation accurately reflects the refactoring objectives and provides actionable patterns for controllers, services, and infrastructure operations. Examples are clear and cover both request-scoped and infrastructure contexts (
reqvsundefined).
Summary
Refactored logging infrastructure from static Logger helper to singleton LoggerService with advanced operation tracking, duplicate log prevention, and CloudWatch-optimized structured logging.
Key Changes
New Logger Service
LoggerServicesingleton class with operation state trackingUpdated Components
Improvements
Technical Details
Logger.start()tologger.startOperation()../helpers/loggerto../services/logger.servicenext(error)in controllersJIRA Ticket
https://linuxfoundation.atlassian.net/browse/LFXV2-903
🤖 Generated with Claude Code