Implemented design patterns consistently#443
Conversation
|
@Juwonlo Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR addresses Issue #371 by refactoring conditional logic in the explainability layer and introducing standardized logging documentation plus a NestJS logger implementation.
Changes:
- Refactored dominant-signal explanation construction into a dedicated helper method and replaced some if/else chains with
switchstatements. - Added a NestJS
LoggerService+LoggerModulewith configurable log levels and structured JSON output in production. - Added documentation for logging strategy and design pattern usage across the ecosystem.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| recommendation-system/src/explainability/explainability.ts | Extracts dominant-signal explanation logic into a helper and converts some branches to switch. |
| docs/logger.service.ts | Introduces a configurable NestJS logger with JSON formatting in prod and colorized output in dev. |
| docs/logger.module.ts | Exposes the logger as a global NestJS module. |
| docs/LOGGING_STRATEGY.md | Defines required log levels, JSON schema, and best practices. |
| docs/DESIGN_PATTERNS.md | Documents intended design patterns used across the codebase. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private shouldLog(level: string): boolean { | ||
| const levels = ['debug', 'info', 'warn', 'error']; | ||
| const configuredIndex = levels.indexOf(this.logLevel.toLowerCase()); | ||
| const targetIndex = levels.indexOf(level); | ||
| return targetIndex >= configuredIndex; | ||
| } |
There was a problem hiding this comment.
If this.logLevel is misconfigured (not in levels), configuredIndex becomes -1, causing shouldLog to return true for all valid level values (unexpectedly enabling all logs). Add a fallback when configuredIndex === -1 (e.g., default to info), or validate/normalize the configured level at construction time.
| const logEntry = { | ||
| timestamp, | ||
| level, | ||
| context: currentContext, | ||
| message: typeof message === 'object' ? JSON.stringify(message) : message, | ||
| ...(Object.keys(metadata).length > 0 ? { data: metadata } : {}), | ||
| }; |
There was a problem hiding this comment.
JSON.stringify(message) can throw on circular structures (and can also drop important info from Error objects). This can cause the logger itself to throw and potentially mask the original error. Consider a safe serialization approach (try/catch with a fallback string, or a safe-stringify/inspector-based formatter), and special-case Error to include name/message/stack.
| *Note: The environment variable `LOG_LEVEL` dictates the minimum severity level output by the application.* | ||
|
|
||
| ## 2. Structured Logging Format | ||
|
|
||
| All off-chain services MUST output logs in **Structured JSON format** when running in production (`NODE_ENV=production`). This ensures seamless ingestion by modern log aggregators and monitoring tools (e.g., Datadog, ELK, CloudWatch). |
There was a problem hiding this comment.
This conflicts with the added LoggerService, which reads app.logLevel (and app.nodeEnv) from ConfigService rather than LOG_LEVEL / NODE_ENV directly. Align the documentation with the actual configuration keys used by the logger, or update the logger to consume the documented env vars (potentially via Nest config mapping).
| *Note: The environment variable `LOG_LEVEL` dictates the minimum severity level output by the application.* | |
| ## 2. Structured Logging Format | |
| All off-chain services MUST output logs in **Structured JSON format** when running in production (`NODE_ENV=production`). This ensures seamless ingestion by modern log aggregators and monitoring tools (e.g., Datadog, ELK, CloudWatch). | |
| *Note: The logger uses the application configuration key `app.logLevel` to determine the minimum severity level output by the application. If environment variables are used in a deployment, they should be mapped into `app.logLevel` via the Nest configuration layer rather than assumed to be read directly by the logger.* | |
| ## 2. Structured Logging Format | |
| All off-chain services MUST output logs in **Structured JSON format** when running in production (`app.nodeEnv = production`). This ensures seamless ingestion by modern log aggregators and monitoring tools (e.g., Datadog, ELK, CloudWatch). |
| **Context**: Conditional branches that change behavior based on a type or flag (e.g., generating explanations for AI recommendations) often result in large, unmaintainable `switch` blocks. | ||
| **Usage**: Used in `ExplanationGenerator` (`explainability.ts`) to dynamically select the correct algorithm for extracting dominant signal explanations. |
There was a problem hiding this comment.
The current ExplanationGenerator implementation still uses a switch inside extractDominantSignalExplanation, so this doc describes a Strategy Pattern that isn't implemented by the code shown in this PR. Either (a) update this document to reflect the current refactor (helper method + switch), or (b) implement the documented strategy approach (e.g., a list/map of strategy objects selected by matches(...)).
| private extractDominantSignalExplanation( | ||
| dominantSignal: string, | ||
| rankingSignal: any, | ||
| userProfile: Types.UserProfile, | ||
| similarUsers?: string[] | ||
| ): { |
There was a problem hiding this comment.
The helper introduces/propagates any for rankingSignal and a broad string for dominantSignal, which weakens type safety inside the method (e.g., property access on rankingSignal). Prefer using the same concrete type as the caller (e.g., a Types.RankingSignal-like interface) and narrow dominantSignal to a union/keyof type so invalid signal names are caught at compile time.
| if (!userProfile) return ''; // Guard clause | ||
|
|
There was a problem hiding this comment.
userProfile is typed as a required parameter (userProfile: Types.UserProfile), so this guard is inconsistent with the method signature and can hide upstream contract violations by silently returning an empty explanation. Either remove the guard (preferred if the type contract is correct) or make the parameter optional/nullable in the signature and document the behavior.
| if (!userProfile) return ''; // Guard clause |
Closes #371
Fixed Issue #371