Conversation
There was a problem hiding this comment.
Pull Request Overview
Adds a new core data redaction engine, refactors logging and formatting into modular packages, and updates documentation and tests to support advanced PII protection.
- Introduce
DataRedactorwith configurable patterns, deep object scanning, and circular reference handling - Modularize logger, formatter, and redaction exports under
src/directories - Bump version to 1.3.0 and enhance README with redaction guides
Reviewed Changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/redaction/redactor.ts | New DataRedactor class implementing redaction logic |
| src/redaction/config.ts | Environment overrides and default redaction config |
| package.json | Version bump to 1.3.0 and updated metadata |
| README.md | Expanded redaction documentation and examples |
Comments suppressed due to low confidence (3)
README.md:600
- The README still references v1.2.1 as the latest version; update this to v1.3.0 to match the
package.jsonbump and actual release.
**Latest Version:** v1.2.1 - Enhanced with advanced redaction features, comprehensive TypeScript support, and 95%+ test coverage.
src/redaction/redactor.ts:13
- Consider adding unit tests for
DataRedactormethods (e.g.,isSensitiveField,truncateContent,processValue, and circular reference handling) to improve test coverage of the new redaction engine.
export class DataRedactor {
src/redaction/config.ts:80
- [nitpick] Environment variable overrides support
LOG_SENSITIVE_FIELDSbut notcontentFields. You may want to add aLOG_CONTENT_FIELDSoverride to allow runtime customization of fields that should be truncated.
static getEnvironmentConfig(): Partial<RedactionConfig> {
|
Warning Rate limit exceeded@warengonzaga has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 55 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughSir, the system has undergone a comprehensive refactor and expansion. The logging framework now features modular core, configuration, and formatting logic, with advanced automatic data redaction capabilities. Extensive TypeScript interfaces, robust environment detection, and a developer-focused API have been introduced. Test coverage has been significantly expanded, and documentation thoroughly updated. Changes
Is there anything else you wish to know about this deployment, sir? ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (15)
src/formatter/colors.ts (2)
10-21: Consider exporting a lightweightcolorizehelperMany consumers will need to wrap text with a color and automatic reset. A tiny utility prevents repeated string concatenations and guards against missing
reset.+/** + * Wraps a given string with the supplied ANSI color and reset code. + */ +export function colorize(text: string, color: keyof typeof colors): string { + return `${colors[color]}${text}${colors.reset}`; +}
27-33: Scheme naming is clear—future-proof with log-level keysIf the formatter ever supports per-level theming (debug, info, warn, error) reserve keys now to avoid breaking changes later.
src/logger/environment.ts (1)
24-31: Defaulting to INFO is reasonable—document in READMEThe silent fallback guards against misconfigured environments. Ensure this default behaviour is documented for operators.
src/logger/index.ts (1)
6-12: Alias strategy confirmed, Sir.Dual export (
LoggerandCoreLogger) preserves backward compatibility without type shadowing. Just ensure documentation reflects the preferred import moving forward.src/formatter/data-formatter.ts (2)
12-35: Consider safer stringification for complex objects, Sir.
JSON.stringifywill still explode on BigInt values and loses functions/symbols. A lightweight alternative likeutil.inspector a safe-stringify package would increase resilience.import { inspect } from 'util'; // ... try { - return JSON.stringify(data, null, 0); + // util.inspect gracefully handles circular refs, BigInt, etc. + return inspect(data, { depth: 4, breakLength: Infinity }); } catch { return '[Object]'; }
43-49: Minor spacing nit, Sir.The prefixed space inside
styleDatameans callers must remember not to add their own delimiter. Document this implicit behaviour or move the space responsibility to the caller for clarity.src/__tests__/formatter.test.ts (1)
97-100: Consider extracting the ANSI-strip pattern to a shared helperThe raw control character
\x1binside the regex is what Biome is flagging (noControlCharactersInRegex).
Defining a constant (or utility) once and re-using it across tests silences the linter and avoids duplication.-const cleanFormatted = formatted.replace(/\x1b\[[0-9;]*m/g, ''); +const ANSI_REGEX = /\u001b\[[0-9;]*m/g; // central helper +const cleanFormatted = formatted.replace(ANSI_REGEX, '');src/formatter/timestamp.ts (1)
15-21: Injectable clock would boost testabilityHard-coding
new Date()makes deterministic unit tests difficult (e.g., snapshot tests).
Accepting an optionalDateparameter (defaulting tonew Date()) lets callers supply a fixed time.-export function getTimestampComponents(): { +export function getTimestampComponents(now: Date = new Date()): { @@ - const now = new Date();src/logger/filtering.ts (2)
41-50: Minor clarity: handle LOG level explicitly instead of numeric hackUsing the magic rank
99works, but an explicit branch is clearer and removes the mental arithmetic.- const messageSeverity = this.SEVERITY_RANKS[level]; - const modeThreshold = this.MODE_THRESHOLDS[currentMode]; - - return messageSeverity >= modeThreshold; + if (level === LogLevel.LOG) { + return currentMode !== LogMode.OFF; + } + return this.SEVERITY_RANKS[level] >= this.MODE_THRESHOLDS[currentMode];
14-20: Static-only class – consider converting to a plain moduleBiome’s warning is valid: a namespace of pure functions/consts avoids the
thisgymnastics and tree-shakes a bit better.
Not critical, but worth noting.src/redaction/config.ts (2)
101-104: Filter out empty custom sensitive-field entries
LOG_SENSITIVE_FIELDS=""currently appends an empty string to the array.
Trim falsy values before merging to keep look-ups clean.- const customFields = process.env.LOG_SENSITIVE_FIELDS.split(',').map(f => f.trim()); + const customFields = process.env.LOG_SENSITIVE_FIELDS + .split(',') + .map(f => f.trim()) + .filter(Boolean);
59-108: Static-only class – same observation as inLogFilter
RedactionControllerholds configuration helpers only; exporting plain functions keeps the API lighter and satisfies Biome.src/logger/config.ts (1)
95-103: Static import would be safer thanrequirehereThe on-demand
require('../formatter')works, yet:
- It forfeits tree-shaking and ESM interoperability.
- Bundlers may duplicate the module, increasing bundle size.
Unless you have a proven circular-dependency, prefer a top-level import:
import { LogFormatter } from '../formatter';and reuse it in
createDeprecationWarning.
Provides better type safety and avoids runtime resolution overhead.src/redaction/redactor.ts (1)
14-35: Static-only class—consider plain module to reduce complexity
DataRedactorcontains only static members. Converting it to a namespace/utility module eliminates:• The cognitive overhead of the
classfaçade.
• Repeated “thisin static context” lint noise.No functional change required; simply export the functions/constants directly.
src/__tests__/logger.test.ts (1)
128-140: Avoid globalconsole.warnspy leakageThe spy is restored, yet any parallel test suite running concurrently would still observe the patched console until
afterEach.
For jest, wrap withjest.isolateModulesor run the spy inside the individual test case rather than the whole describe block to minimise cross-test bleed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
.gitignore(1 hunks)README.md(6 hunks)jest.config.js(1 hunks)package.json(1 hunks)src/__tests__/formatter.test.ts(2 hunks)src/__tests__/logengine-api.test.ts(1 hunks)src/__tests__/logger.test.ts(2 hunks)src/__tests__/redaction.test.ts(1 hunks)src/formatter.ts(0 hunks)src/formatter/colors.ts(1 hunks)src/formatter/data-formatter.ts(1 hunks)src/formatter/index.ts(1 hunks)src/formatter/message-formatter.ts(1 hunks)src/formatter/timestamp.ts(1 hunks)src/index.ts(1 hunks)src/logger.ts(0 hunks)src/logger/config.ts(1 hunks)src/logger/core.ts(1 hunks)src/logger/environment.ts(1 hunks)src/logger/filtering.ts(1 hunks)src/logger/index.ts(1 hunks)src/redaction/config.ts(1 hunks)src/redaction/index.ts(1 hunks)src/redaction/redactor.ts(1 hunks)src/types/index.ts(1 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (2)
- src/formatter.ts
- src/logger.ts
🧰 Additional context used
🧬 Code Graph Analysis (9)
src/formatter/colors.ts (1)
src/formatter/index.ts (2)
colors(7-7)colorScheme(7-7)
src/redaction/config.ts (1)
src/types/index.ts (1)
RedactionConfig(73-90)
src/formatter/data-formatter.ts (3)
src/formatter/index.ts (3)
formatData(9-9)styleData(9-9)colors(7-7)src/logger/core.ts (1)
error(115-121)src/formatter/colors.ts (1)
colors(10-21)
src/formatter/message-formatter.ts (5)
src/formatter/index.ts (8)
MessageFormatter(6-6)MessageFormatter(12-12)getTimestampComponents(8-8)formatTimestamp(8-8)colorScheme(7-7)formatData(9-9)styleData(9-9)colors(7-7)src/index.ts (1)
LogLevel(215-215)src/formatter/timestamp.ts (2)
getTimestampComponents(11-27)formatTimestamp(36-45)src/formatter/colors.ts (2)
colorScheme(27-33)colors(10-21)src/formatter/data-formatter.ts (2)
formatData(12-35)styleData(43-49)
src/formatter/timestamp.ts (2)
src/formatter/index.ts (3)
getTimestampComponents(8-8)formatTimestamp(8-8)colors(7-7)src/formatter/colors.ts (1)
colors(10-21)
src/__tests__/formatter.test.ts (1)
src/formatter/data-formatter.ts (1)
formatData(12-35)
src/logger/environment.ts (2)
src/logger/index.ts (1)
EnvironmentDetector(9-9)src/index.ts (1)
LogMode(215-215)
src/logger/config.ts (1)
src/logger/environment.ts (1)
EnvironmentDetector(11-57)
src/types/index.ts (2)
src/index.ts (2)
RedactionConfig(216-216)LoggerConfig(216-216)src/redaction/index.ts (1)
RedactionConfig(8-8)
🪛 Biome (1.9.4)
src/redaction/config.ts
[error] 59-108: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 82-82: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
src/formatter/message-formatter.ts
[error] 15-94: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 28-28: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 29-29: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
src/__tests__/formatter.test.ts
[error] 97-97: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 140-140: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
src/logger/environment.ts
[error] 11-57: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
src/logger/filtering.ts
[error] 12-69: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 43-43: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 46-46: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 58-58: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 67-67: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
src/redaction/redactor.ts
[error] 13-287: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 30-30: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 43-43: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 44-44: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 45-45: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 46-46: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 46-46: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 56-56: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 67-67: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 68-68: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 69-69: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 78-78: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 79-79: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 89-89: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 90-90: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 91-91: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 102-102: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 114-114: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 118-118: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 132-132: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 149-149: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 162-162: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 181-181: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 189-189: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 190-190: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 193-193: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 194-194: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 197-197: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 198-198: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 220-220: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 220-220: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 221-221: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 228-228: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 272-272: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 282-282: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 285-285: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 285-285: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (17)
.gitignore (1)
137-138: Addition ofexamples/path acknowledgedExcluding example artefacts is a sensible safeguard against bloating the repository and polluting coverage metrics. No conflicts detected with existing tooling paths.
jest.config.js (1)
1-4: Doc-block improves discoverability—no runtime impactThe descriptive header is helpful for newcomers and CI auditors while remaining comment-only. Configuration integrity remains intact.
tsconfig.json (1)
2-3: Comment additions acceptedInline comments clarify the intent without affecting compiler output.
package.json (1)
3-18: Metadata refresh acknowledged, Sir.Version bump and security-oriented keywords register cleanly. No structural concerns detected. Carry on.
src/redaction/index.ts (1)
6-8: Barrel looks ship-shape, Sir.The single-point export strategy aids tree-shaking and DX. Paths resolve correctly relative to the folder hierarchy.
src/formatter/index.ts (1)
6-12: Formatter barrel verified, Sir.The module harmonises new formatter utilities while retaining the legacy alias. All good.
src/__tests__/formatter.test.ts (1)
127-181: Edge-case suite is thorough – nicely doneThe new tests cover
null,undefined, primitives, and unserializable objects; this hardens the formatter against tricky input. All assertions read clearly and match the production behaviour.
Carry on.src/formatter/message-formatter.ts (1)
36-40:data !== undefinedskips empty stringsIf an empty string is intentionally passed as
data, the branch is skipped.
A safer guard isdata != null(allows'', filtersnull&undefined) or simply checkarguments.length.- if (data !== undefined) { + if (data != null) {src/logger/core.ts (1)
34-42: Redaction config ignores caller-supplied overrides
configure()refreshes the redaction engine with defaults + env values, but discards any caller-provided redaction tweaks that might have been set viaLogEngine.configureRedaction()earlier in the session.If the intent is to keep both concerns independent, that’s fine.
If not, consider forwarding the existingDataRedactor.getConfig()as the base to avoid silently resetting user changes.src/__tests__/logengine-api.test.ts (1)
1-405: Impressive coverageThe suite exhaustively exercises configuration, redaction, raw logging and integration paths. No actionable issues spotted.
src/types/index.ts (1)
69-250: Sir, the interface architecture is exemplary - worthy of the Stark Industries seal of approval.The comprehensive type definitions demonstrate exceptional foresight in API design. The
RedactionConfiginterface provides the perfect balance between security defaults and customization flexibility, while the separation of concerns between logging and redaction interfaces ensures clean, maintainable code.Particularly impressive is the
ILogEngineWithoutRedactioninterface pattern - a elegant solution for temporary redaction bypass that maintains type safety.README.md (1)
7-487: Sir, the documentation rivals the clarity of my holographic displays - simply magnificent.The comprehensive coverage of the advanced redaction features, complete with practical examples and security considerations, provides developers with everything they need to implement secure logging. The zero-configuration approach with intelligent defaults is particularly elegant.
src/__tests__/redaction.test.ts (3)
24-469: Sir, the test coverage would make even my diagnostic systems envious - comprehensive and thorough.The core functionality tests demonstrate excellent attention to detail, covering case sensitivity, nested structures, circular references, and depth limiting. The edge case handling for deeply nested objects is particularly well-implemented.
475-855: Sir, the advanced feature testing demonstrates laboratory-grade precision.The custom pattern management and sensitive field tests are exemplary, covering accumulation behavior, case sensitivity, and integration scenarios. The underscore compound word detection tests (lines 813-853) show particular attention to edge cases.
861-1127: Sir, the integration testing is as seamless as my integration with the Mark armor systems.The LogEngine integration tests comprehensively verify that redaction works correctly across all log levels, raw methods properly bypass redaction, and configuration changes are properly applied. The mock-based verification approach provides reliable test assertions.
src/index.ts (2)
35-220: Sir, the LogEngine architecture is as elegant as the Arc Reactor design - powerful yet beautifully simple.The API design provides comprehensive logging capabilities with security-first principles. The separation between standard methods (with redaction) and raw methods (without redaction) gives developers precise control. The
withoutRedaction()pattern is particularly elegant, providing temporary bypass without compromising the default secure behavior.
23-26: Sir, I must verify the structural integrity of these module imports.The imports reference
./loggerand./redactionmodules. Please ensure these modules exist at the specified paths to prevent compilation failures.#!/bin/bash # Description: Verify that the imported modules exist # Check for logger module fd -t f "^(index\.(ts|js)|core\.(ts|js)|logger\.(ts|js))$" src/logger # Check for redaction module fd -t f "^(index\.(ts|js)|redactor\.(ts|js)|config\.(ts|js))$" src/redaction # Verify the specific imports are exported rg "export.*Logger" src/logger --type ts rg "export.*DataRedactor|defaultRedactionConfig|RedactionController" src/redaction --type ts
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/logger/environment.ts (1)
11-64: Static-only class still violates linter rule—consider flattening.Biome continues to flag this construct (
noStaticOnlyClass). The previous review already covered the recommended refactor, and the code remains unchanged.README.md (1)
600-600: Confirm version bump to v1.3.0
The “Latest Version” line has been updated correctly.
🧹 Nitpick comments (8)
src/logger/environment.ts (1)
16-18: Micro-optimisation: cache the normalised value once per call-site.Each helper re-computes
process.env.NODE_ENV.trim().toLowerCase(); negligible but unnecessary. Store the value locally or expose a memoised getter to avoid redundant string work.Also applies to: 45-47, 53-55, 61-63
README.md (7)
7-10: Streamline introductory phrasing
Sir, the opening paragraphs are vivid but a touch verbose; consider trimming the audience list or splitting into two sentences for improved readability.
19-31: Ensure consistency in feature list
The Key Features bullets are comprehensive but mix emojis and bold text in varying lengths—perhaps unify style or alphabetize major points for faster scanning.
163-166: Update migration note to match release
The snippet still references “v1.2.1+”; consider updating to “v1.3.0+” to avoid versioning confusion for new adopters.
322-330: Consider ordering sensitive patterns
The Protected Data Types list is great—optionally, order categories alphabetically or by risk to enhance discoverability.
382-397: Nitpick environment variable examples
A minor suggestion: consolidate similar env vars into a code fence annotation block for brevity.
486-486: Enrich “Perfect for” section
Perhaps add one or two specific use-case scenarios (e.g., microservices, serverless) to sharpen guidance.
526-534: Consider grouping available interfaces
Optionally, categorize interfaces by functionality (e.g., redaction vs. logging) to aid discovery.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(6 hunks)src/logger/environment.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/logger/environment.ts (2)
src/logger/index.ts (1)
EnvironmentDetector(9-9)src/index.ts (1)
LogMode(215-215)
🪛 Gitleaks (8.26.0)
README.md
91-91: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
315-315: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 Biome (1.9.4)
src/logger/environment.ts
[error] 11-64: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 25-25: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 46-46: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 54-54: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 62-62: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (13)
README.md (13)
94-99: Verify inline comments clarity
The comments on custom patterns and sensitive fields are clear—excellent demonstration of API usage.
169-173: Approve LogMode benefits section
The new Key Benefits bullets are succinct and highlight the value prop effectively.
299-303: Approve Redaction section header
The “Advanced Data Redaction” header and intro copy align perfectly with the feature focus.
336-350: Approve deep object scanning example
This nested-object demo is clear and thoroughly illustrates redaction depth.
354-362: Approve content truncation demo
Excellent showcase of log bloat prevention with realistic code.
369-377: Approve raw-logging API illustration
The contrast between raw methods and standard logging is well captured.
404-414: Approve custom configuration snippet
The configure/get pattern is clearly demonstrated—no revisions needed.
421-448: Approve advanced redaction features block
Comprehensive coverage of dynamic and pattern-based redaction—excellent depth.
453-462: Approve development workflow comparison
The side-by-side redacted vs. raw logging is highly instructive.
468-472: Approve environment behavior table
Clear mapping of environment to redaction status; nicely formatted.
475-485: Approve security benefits list
The concise bullets effectively reinforce the compliance and performance wins.
488-490: Approve TypeScript support heading
The new section title and intro line succinctly set expectations for typings coverage.
495-522: Approve core interfaces code block
Well-structured and complete—developers will appreciate the inline type examples.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/redaction/redactor.ts (3)
13-21: Elegance alert – static-only class may be over-engineeredThe class contains exclusively static members; essentially it’s a namespaced collection of helpers. Converting it to a plain module of exported functions (or a singleton instance if stateful behaviour is wanted) simplifies call-sites, avoids the Biome
noStaticOnlyClasswarning and removes the cognitive load of thethisvs class name ambiguity.
27-35: Resolvethisambiguity inside static contextBiome flags every use of
thisinside a static method. Replacingthiswith the explicit class identifier silences the warning and improves readability:- this.config = { + DataRedactor.config = {Apply the same substitution throughout the file.
220-225: Guard againstRegExp.lastIndexside-effectsCalling
.test()on a regex with the global (g) flag mutateslastIndex, which can yield alternating true/false results on subsequent fields.- if (pattern.test(fieldName)) { + // Reset stateful regexes to avoid inconsistent matches + pattern.lastIndex = 0; + if (pattern.test(fieldName)) {Alternatively, document that custom patterns must be supplied without the
gflag.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/redaction/redactor.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/redaction/redactor.ts
[error] 13-287: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 30-30: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 43-43: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 44-44: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 45-45: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 46-46: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 46-46: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 56-56: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 67-67: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 68-68: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 69-69: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 78-78: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 79-79: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 89-89: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 90-90: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 91-91: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 102-102: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 114-114: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 118-118: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 132-132: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 149-149: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 162-162: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 181-181: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 189-189: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 190-190: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 193-193: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 194-194: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 197-197: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 198-198: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 220-220: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 220-220: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 221-221: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 228-228: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 272-272: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 282-282: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 285-285: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 285-285: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (2)
src/redaction/redactor.ts (2)
142-151: Duplicate reference ≠ circular structureBecause
visitedis shared across the whole traversal, the second appearance of the same object/array – even when not part of an actual cycle – is now reported as[Circular …].
If the intention is to warn only on genuine loops, consider tracking the current path (stack) rather than the global set, or remove the entry when unwinding recursion.
148-165: Circular-reference fix looks goodEntry is purposely kept in
visited, resolving the earlier premature-deletion issue flagged in the previous review.
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/redaction/redactor.ts (1)
151-166: Circular-reference fix confirmed – good jobThe earlier concern about prematurely removing objects from the
visitedset is now addressed; the value is intentionally kept, ensuring cross-branch circular detection.
Nothing further needed here.
🧹 Nitpick comments (9)
src/redaction/redactor.ts (2)
13-23: Static-only class – consider a functional module instead
DataRedactoronly exposes static members. TypeScript allows this, but static-only classes trigger Biome’snoStaticOnlyClassrule and add needlessthisindirection flagged all over the file.A lightweight alternative:
-export class DataRedactor { … } +// redactor.ts +export const DataRedactor = (() => { + /* move existing static props / methods inside, + replace `this` with `config`/local refs */ + return { + updateConfig, + getConfig, + /* …other public fns */ + }; +})();This removes the lint noise and slightly reduces bundle size.
132-136: Hard-coded recursion cap might be too aggressive
MAX_RECURSION_DEPTH = 100is safe but could break legitimate deeply-nested payloads (e.g., large JSON schemas). Expose it throughRedactionConfig, defaulting to 100, so users can raise or lower it without code changes.src/__tests__/formatter.test.ts (2)
98-101: Lint false-positive – silence control-char warningBiome trips over the
\x1bescape sequence in the regex.
Add a one-off directive to keep the spec green without disabling the global rule:-const cleanFormatted = formatted.replace(/\x1b\[[0-9;]*m/g, ''); +// biome-ignore lint/suspicious/noControlCharactersInRegex – ANSI strip for tests +const cleanFormatted = formatted.replace(/\x1b\[[0-9;]*m/g, '');
141-143: Same ANSI-strip regex – apply the same lint-ignore
Replicate the directive here to avoid duplicate lint errors.src/__tests__/logger.test.ts (1)
110-123: Usetest.eachfor clarityThe manual
forEachinside a single test bundles five logical cases into one assertion block. Splitting viait.each(ortest.each) yields clearer failure reporting:-const testCases = [LogMode.DEBUG, … ]; -testCases.forEach((mode, index) => { - mocks.mockConsoleLog.mockClear(); - logger.configure({ mode }); - logger.log(`LOG message ${index}`); - expect(mocks.mockConsoleLog).toHaveBeenCalledTimes(1); -}); +it.each([ + LogMode.DEBUG, + LogMode.INFO, + LogMode.WARN, + LogMode.ERROR, + LogMode.SILENT, +])('should always log at LOG level in %s mode', (mode) => { + mocks.mockConsoleLog.mockClear(); + logger.configure({ mode }); + logger.log('LOG message'); + expect(mocks.mockConsoleLog).toHaveBeenCalledTimes(1); +});This keeps each mode isolated and improves test-output readability.
src/__tests__/redaction.test.ts (4)
10-15: Console hijack could be cleaner withjest.spyOn.Overwriting global
consoleworks, butjest.spyOn(console, 'log').mockImplementation(...):• preserves original bindings automatically after each test
• avoids side-effects if this file ever runs in the same worker with other suites
• produces clearer intent-const mockConsole = { - log: jest.fn(), - warn: jest.fn(), - error: jest.fn() -}; +const logSpy = jest.spyOn(console, 'log').mockImplementation(jest.fn()); +const warnSpy = jest.spyOn(console, 'warn').mockImplementation(jest.fn()); +const errorSpy = jest.spyOn(console, 'error').mockImplementation(jest.fn());Remember to
mockRestore()inafterAll.
24-33: Environment snapshot is a shallow copy – mutations to nested vars survive.
const originalEnv = process.env; … process.env = { ...originalEnv };
If any test mutates a value insideoriginalEnvbeforebeforeEachruns (unlikely but possible), that change is forever captured.Safer pattern:
const originalEnv = { ...process.env }; // deep copy at declaration
1170-1176: Adjacenttestcall on the same line hurts readability.The new suite starts on the same physical line as a closing brace – easy to miss during reviews and auto-formatting.
-}); test('should cover method signatures with different parameter combinations', () => { - // … -}); +}); + +test('should cover method signatures with different parameter combinations', () => { + // … +});
360-387: Deep-nesting generators are CPU-intensive; cap iteration counts.Creating 101-level nested arrays/objects is fine once, but this file does it multiple times.
On slower CI agents the suite may become sluggish.Consider lowering the depth (e.g., 70 for arrays, 40 for objects) – the redaction limits (100/99) will still be crossed and branch coverage preserved.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/__tests__/environment.test.ts(2 hunks)src/__tests__/formatter.test.ts(3 hunks)src/__tests__/log-engine.test.ts(1 hunks)src/__tests__/logger.test.ts(4 hunks)src/__tests__/redaction.test.ts(1 hunks)src/redaction/redactor.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/__tests__/redaction.test.ts (3)
src/redaction/redactor.ts (1)
DataRedactor(13-289)src/index.ts (4)
DataRedactor(217-217)defaultRedactionConfig(217-217)RedactionController(217-217)LogEngine(35-212)src/redaction/config.ts (2)
defaultRedactionConfig(12-53)RedactionController(59-108)
🪛 Biome (1.9.4)
src/redaction/redactor.ts
[error] 13-289: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 32-32: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 45-45: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 46-46: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 47-47: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 48-48: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 48-48: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 58-58: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 69-69: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 70-70: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 71-71: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 80-80: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 81-81: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 91-91: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 92-92: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 93-93: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 104-104: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 116-116: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 120-120: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 134-134: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 151-151: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 164-164: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 183-183: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 191-191: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 192-192: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 195-195: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 196-196: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 199-199: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 200-200: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 222-222: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 222-222: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 223-223: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 230-230: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 274-274: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 284-284: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 287-287: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 287-287: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
src/__tests__/formatter.test.ts
[error] 98-98: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 141-141: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🔇 Additional comments (3)
src/__tests__/environment.test.ts (1)
25-32: State restoration looks solidThe nested
describerelies on the top-levelafterEachto resetNODE_ENV. Confirmed that the hook fires for every inner test – no hidden cross-test pollution detected.src/__tests__/log-engine.test.ts (1)
227-256: Export surface tests applaudableValidating both primary and redaction-free loggers shields against accidental API regressions. Good coverage, sir.
src/__tests__/redaction.test.ts (1)
6-8: Certainly, Sir. Executing a more precise barrel-export verification now:#!/bin/bash # List all files in src/redaction to confirm barrel presence echo "Contents of src/redaction/:" ls -1 src/redaction || echo "Directory src/redaction/ not found" # Check specifically for index.ts barrel if [ -f src/redaction/index.ts ]; then echo "Found src/redaction/index.ts – inspecting exports:" rg "export.*DataRedactor" -n src/redaction/index.ts rg "export.*defaultRedactionConfig" -n src/redaction/index.ts rg "export.*RedactionController" -n src/redaction/index.ts else echo "⚠️ src/redaction/index.ts not found" fi
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/logger/environment.ts (1)
11-64: Static-only class persists – recommended functional refactor remains unaddressedSir, Biome still flags
EnvironmentDetectoras a static-only class (rulenoStaticOnlyClass). Converting it into a plain module of functions (or aconstobject literal) will trim emitted code and improve tree-shaking.-/** Environment detection and configuration utilities */ -export class EnvironmentDetector { +/** Environment detection utilities */ +export const EnvironmentDetector = { // …methods stay the same… -} +} as const;This is the same concern I raised earlier; kindly schedule the refactor before the next release candidate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(6 hunks)src/logger/environment.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/logger/environment.ts (2)
src/logger/index.ts (1)
EnvironmentDetector(9-9)src/index.ts (1)
LogMode(215-215)
🪛 markdownlint-cli2 (0.17.2)
README.md
5-5: Bare URL used
null
(MD034, no-bare-urls)
7-7: Bare URL used
null
(MD034, no-bare-urls)
🪛 Biome (1.9.4)
src/logger/environment.ts
[error] 11-64: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🔇 Additional comments (1)
src/logger/environment.ts (1)
45-55: Consistency fix confirmed – helpers now reuse normalized valueThe boolean helpers now delegate to
getNormalizedNodeEnv, eliminating prior trim/case inconsistencies. Implementation looks solid.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
README.md (1)
86-92: Potential credential exposure in example snippetSir, the
apiKey: 'apikey123'literal resembles a real secret key and will trigger secret-scanning tools. Swapping it for an unmistakably inert placeholder will neutralise any false-positive alerts while keeping the example clear.- apiKey: 'apikey123' // ❌ [REDACTED] + apiKey: 'YOUR_API_KEY_HERE' // ❌ [REDACTED]
🧹 Nitpick comments (1)
src/__tests__/logger.test.ts (1)
155-172: Brittle expectation on exact warning countLocking the test to exactly three
console.warninvocations will break the suite the moment wording or formatting shifts. As the intention is simply to assert that a deprecation warning is emitted, assert at least one call and one message substring instead.-expect(mockConsoleWarn).toHaveBeenCalledTimes(3); +expect(mockConsoleWarn).toHaveBeenCalled();Optionally keep one
toHaveBeenCalledWithfor the key phrase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(6 hunks)src/__tests__/logger.test.ts(4 hunks)src/logger/config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/logger/config.ts
🔇 Additional comments (1)
README.md (1)
600-601: Version badge update looks goodLatest‐version line correctly reflects
v1.3.0. No further action required.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/formatter/message-formatter.ts (2)
28-29: Prefer explicit class reference in static context.Biome flags the use of
thiswithin static methods as potentially confusing. Replacethis.getLevelName/this.getLevelColorwith the class name for clarity and to silence the linter.- const levelName = this.getLevelName(level); - const levelColor = this.getLevelColor(level); + const levelName = MessageFormatter.getLevelName(level); + const levelColor = MessageFormatter.getLevelColor(level);
15-94: Static-only class could be flattened into pure functions.The class contains no instance state; every member is static. Converting it to a module of plain functions (or a namespace) will:
• Remove the noStaticOnlyClass linter warning
• Cut a few bytes from bundle size
• Simplify tree-shaking for consumersNot urgent, but worthwhile for long-term cleanliness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/formatter/message-formatter.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/formatter/message-formatter.ts
[error] 15-94: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 28-28: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 29-29: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (1)
src/formatter/message-formatter.ts (1)
28-31: Colour-bleed fix confirmed – nice work, Sir.The extra
colors.resetdirectly after the level badge elegantly solves the bleed reported in the previous review. No further action required.
Docstrings generation was requested by @warengonzaga. * #10 (comment) The following files were modified: * `src/formatter/data-formatter.ts` * `src/formatter/timestamp.ts`
|
Note Generated docstrings for this pull request at #12 |
…abs/coderabbitai/docstrings/rBEa
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/__tests__/logger.test.ts (1)
150-156:delete process.env.NODE_ENVis acceptable despite Biome’s nagThe alternative (
process.env.NODE_ENV = undefined) coerces the value to the string"undefined", re-introducing the very bug this block fixes. In a test context the micro-optimisation Biome flags is negligible—keeping thedeleteis the correct trade-off.
🧹 Nitpick comments (2)
src/__tests__/logger.test.ts (1)
110-122: Reset all console spies inside the for-each loopOnly
mockConsoleLogis cleared between iterations. If future refactors make thelogger.logmethod callconsole.warnorconsole.error(e.g. for styling), residual calls could bleed across iterations and produce false positives.- // Reset mocks - mocks.mockConsoleLog.mockClear(); + // Reset mocks + mocks.mockConsoleLog.mockClear(); + mocks.mockConsoleWarn.mockClear(); + mocks.mockConsoleError.mockClear();src/formatter/data-formatter.ts (1)
31-32:JSON.stringifythird argument of0is superfluous
Passing0as the spacing parameter has no effect and slightly obscures intent. Omitting it keeps the call crisp:JSON.stringify(data).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/__tests__/logger.test.ts(4 hunks)src/formatter/data-formatter.ts(1 hunks)src/formatter/timestamp.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/formatter/timestamp.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/formatter/data-formatter.ts (3)
src/formatter/index.ts (3)
formatData(9-9)styleData(9-9)colors(7-7)src/logger/core.ts (1)
error(115-121)src/formatter/colors.ts (1)
colors(10-21)
src/__tests__/logger.test.ts (4)
src/index.ts (2)
LogMode(215-215)LogLevel(215-215)src/logger/core.ts (1)
Logger(19-199)src/logger/index.ts (3)
Logger(6-6)Logger(12-12)LogFilter(8-8)src/logger/filtering.ts (1)
LogFilter(12-69)
🪛 Biome (1.9.4)
src/__tests__/logger.test.ts
[error] 152-152: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (2)
src/__tests__/logger.test.ts (1)
27-40: Default-mode test is solidThe assertions correctly verify that only
ERROR-level output slips through the defaultERRORmode in the test environment. All good here, sir.src/formatter/data-formatter.ts (1)
46-52: Elegant and compliant implementation
The styling helper is clear, side-effect-free, and performant. No concerns here, Sir.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Documentation
Tests
Chores