feat: standardize backend logging, remove direct console usage#75
feat: standardize backend logging, remove direct console usage#75Ebenezer199914 wants to merge 1 commit into
Conversation
…ble-Protocol#65) Replace all direct console.* calls across the codebase with the project's structured winston logger (src/utils/logger.ts). Changes: - logger.ts: add named helpers logInfo, logWarn, logError, logDebug with optional context metadata; keep default export for back-compat - distribution.service.ts: replace 4 console.error/warn calls with logError / logWarn - distribution.controller.ts: replace 3 console.error calls with logError - data-source.ts: replace console.error in resetDatabase with logger.error - src/utils/index.ts: replace console.error in executeTransaction with logError - src/utils/errorHandler.ts: replace console.info with logger.info - src/scripts/generate-migration.ts: replace console.error and console.warn with logger.error and logger.warn - eslint.config.mjs: add 'no-console': 'error' rule to enforce the pattern going forward Verification: - bun run type-check: passes (0 errors) - bun run test: 33/33 pass - bun run lint: 0 errors (all no-console violations resolved) Closes Fundable-Protocol#65
|
@Ebenezer199914 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! 🚀 |
📝 WalkthroughWalkthroughAdds four named logging helpers ( ChangesLogging standardization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/config/persistence/data-source.ts`:
- Line 78: The database reset error logging in data-source should preserve
structured error details instead of converting the exception to a string. Update
the reset-failure handling to use logError("Error resetting the database",
error) or an equivalent structured logger call so the existing name, message,
and stack fields are retained; adjust the error path near the logger.error call
in the reset logic accordingly.
In `@src/scripts/generate-migration.ts`:
- Line 11: The CLI messages in generate-migration.ts are now being sent through
the shared logger, so users no longer see them in terminal output. Update the
script’s migration-name and no-schema-changes paths to write directly to
stderr/stdout for CLI feedback, or add a Console transport in
src/utils/logger.ts if you want these messages to remain visible while still
using logger.error. Keep the script-specific UX on the terminal side, and leave
file logging behavior unchanged.
In `@src/utils/logger.ts`:
- Around line 76-80: The logDebug helper is currently unreachable because the
shared logger is initialized with a fixed info level, so debug calls are always
dropped. Update the logger setup in logger.ts so the base level is driven by
configuration or environment instead of being hard-coded, and keep logDebug
delegating through the existing logger.debug path so it becomes effective when
debug logging is enabled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b999c784-10ea-4c00-802b-8513ca6c7dab
📒 Files selected for processing (8)
eslint.config.mjssrc/components/v1/distribution/distribution.controller.tssrc/components/v1/distribution/distribution.service.tssrc/config/persistence/data-source.tssrc/scripts/generate-migration.tssrc/utils/errorHandler.tssrc/utils/index.tssrc/utils/logger.ts
| logger.info('All tables dropped. Database reset successfully!'); | ||
| } catch (error) { | ||
| console.error('Error resetting the database:', error); | ||
| logger.error('Error resetting the database:', { error: String(error) }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Don’t stringify the reset failure here.
String(error) drops the structured fields this PR just standardized (name, message, stack), which makes DB reset failures much harder to diagnose. Route this through logError("Error resetting the database", error) or pass equivalent structured metadata.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/config/persistence/data-source.ts` at line 78, The database reset error
logging in data-source should preserve structured error details instead of
converting the exception to a string. Update the reset-failure handling to use
logError("Error resetting the database", error) or an equivalent structured
logger call so the existing name, message, and stack fields are retained; adjust
the error path near the logger.error call in the reset logic accordingly.
|
|
||
| if (!migrationName) { | ||
| console.error('Please provide a migration name.'); | ||
| logger.error('Please provide a migration name.'); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
This breaks the script’s terminal feedback.
These messages are part of the CLI contract, but the shared logger shown in src/utils/logger.ts only writes to error.log/combined.log. After this change, callers no longer see “Please provide a migration name” or “No schema changes detected” in stderr/stdout. Keep direct console output for CLI-only scripts, or add a Console transport before routing script UX through the shared logger.
Also applies to: 23-23
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { exec } from 'child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/scripts/generate-migration.ts` at line 11, The CLI messages in
generate-migration.ts are now being sent through the shared logger, so users no
longer see them in terminal output. Update the script’s migration-name and
no-schema-changes paths to write directly to stderr/stdout for CLI feedback, or
add a Console transport in src/utils/logger.ts if you want these messages to
remain visible while still using logger.error. Keep the script-specific UX on
the terminal side, and leave file logging behavior unchanged.
| export const logDebug = ( | ||
| message: string, | ||
| context?: Record<string, unknown> | ||
| ): void => { | ||
| logger.debug(message, context); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
logDebug is unreachable with the current logger configuration.
Line 6 hard-codes the logger level to info, so every call through this helper is discarded in all environments. If this helper is meant to be usable, drive the base level from config/env instead of a fixed value.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/logger.ts` around lines 76 - 80, The logDebug helper is currently
unreachable because the shared logger is initialized with a fixed info level, so
debug calls are always dropped. Update the logger setup in logger.ts so the base
level is driven by configuration or environment instead of being hard-coded, and
keep logDebug delegating through the existing logger.debug path so it becomes
effective when debug logging is enabled.
|
Thank you for your awesome contribution, however after analyzing your implementation, there are some minor fixes to be done. Kindly fix them to merge your PR asap. Also do not forget to use fundable.finance to offramp. |
2 participants
Summary
Replace all direct
console.*calls across the backend with the project's structured Winston logger (src/utils/logger.ts). Adds named helper functions for better discoverability and adds an ESLint rule to prevent regressions.Area
src/)Scope
Changes
src/utils/logger.ts– Add named helperslogInfo,logWarn,logError,logDebugwith optional context metadata. Default export is preserved for backward compatibility.src/components/v1/distribution/distribution.service.ts– Replace 4console.error/console.warncalls withlogError/logWarn.src/components/v1/distribution/distribution.controller.ts– Replace 3console.errorcalls withlogError.src/config/persistence/data-source.ts– Replaceconsole.errorinresetDatabasewithlogger.error.src/utils/index.ts– Replaceconsole.errorinexecuteTransactionwithlogError.src/utils/errorHandler.ts– Replaceconsole.infoinhandleErrorResponsewithlogger.info.src/scripts/generate-migration.ts– Replaceconsole.errorandconsole.warnwithlogger.error/logger.warn.eslint.config.mjs– Add'no-console': 'error'rule to enforce the pattern going forward.Verification
bun run type-check— passes (0 errors)bun run test— 33/33 tests passbun run lint— 0 errors (allno-consoleviolations resolved)Notes
Closes #65
Summary by CodeRabbit