Open
Conversation
…and name handling
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a masking utility to redact sensitive fields in logs and integrates it into the logger; updates the test script to run Jest and adds Jest as a devDependency; and ignores Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Logger
participant Mask
participant FileSystem
Client->>Logger: send log info (level, message, meta)
Logger->>Mask: maskData(info.message)
Mask-->>Logger: maskedMessage
Logger->>FileSystem: write formatted log (uses maskedMessage) to combined.log / stdout
FileSystem-->>Logger: write ACK
Logger-->>Client: (completed)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/logger/mask.js (2)
104-114: Region masking logic is correct but slightly confusing.The recursive
maskObject(value, seen)call processes all nested fields, but thenaddress,city,state, andzipcodeare immediately overwritten withMASK(if truthy) or their original falsy value. This means:
- Any regex masking applied to these four fields by
maskObjectis discarded- The spread
...maskedRegionis only useful for preserving other nested fields that aren't explicitly listedConsider documenting this intent or simplifying by excluding these fields from the recursive call if they'll always be fully masked anyway.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/logger/mask.js` around lines 104 - 114, The region masking block calls maskObject(value, seen) then immediately overwrites address/city/state/zipcode which discards any regex-based masks; update the logic so either (a) exclude these four fields from the recursive mask call and then spread the remaining maskedRegion fields into cloned[key] before setting address/city/state/zipcode to MASK (if truthy), or (b) keep the recursive call but remove those keys from its result before overwriting—make the change in the if (key === "region" ...) branch referencing maskObject, MASK, cloned, and the address/city/state/zipcode fields and add a short comment explaining the chosen approach.
10-13: Phone regex may produce false positives.The pattern
/\+?\d[\d\s\-]{8,}\d/gwill match any sequence of 10+ digits with optional spaces/hyphens. This could inadvertently mask numeric identifiers like order IDs, tracking numbers, or timestamps that happen to be long numeric strings.Consider narrowing the pattern or adding additional heuristics if false positives become problematic in practice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/logger/mask.js` around lines 10 - 13, The phone-matching rule using the regex /\+?\d[\d\s\-]{8,}\d/g is too broad and may mask long numeric IDs; narrow it by tightening the pattern and/or adding heuristics in the mask pipeline: update the regex (or add a new predicate around it) to require phone-like cues (e.g., leading +country code, area-code parentheses, common separator patterns, or word boundaries) and check the candidate string with the existing maskPhone function to confirm it looks like a real phone before masking; locate the rule object that contains the regex and replacer referencing maskPhone and modify it to include these additional checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/logger/mask.js`:
- Around line 104-114: The region masking block calls maskObject(value, seen)
then immediately overwrites address/city/state/zipcode which discards any
regex-based masks; update the logic so either (a) exclude these four fields from
the recursive mask call and then spread the remaining maskedRegion fields into
cloned[key] before setting address/city/state/zipcode to MASK (if truthy), or
(b) keep the recursive call but remove those keys from its result before
overwriting—make the change in the if (key === "region" ...) branch referencing
maskObject, MASK, cloned, and the address/city/state/zipcode fields and add a
short comment explaining the chosen approach.
- Around line 10-13: The phone-matching rule using the regex
/\+?\d[\d\s\-]{8,}\d/g is too broad and may mask long numeric IDs; narrow it by
tightening the pattern and/or adding heuristics in the mask pipeline: update the
regex (or add a new predicate around it) to require phone-like cues (e.g.,
leading +country code, area-code parentheses, common separator patterns, or word
boundaries) and check the candidate string with the existing maskPhone function
to confirm it looks like a real phone before masking; locate the rule object
that contains the regex and replacer referencing maskPhone and modify it to
include these additional checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef91936a-c3bc-46bf-b744-bba30ffabc45
📒 Files selected for processing (5)
.gitignoreREADME.mdpackage.jsonsrc/logger/logger.jssrc/logger/mask.js
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/logger/mask.js (1)
109-118: Region field handling has redundant processing and inconsistent behavior.The code first recursively masks
value(which would apply regex masking to any string fields includingaddress,city, etc.), then spreads the result, and finally overwrites address-related fields withMASKif the original values were truthy. This causes:
- Redundant work: String fields are regex-masked, then immediately overwritten with
MASK.- Inconsistent masking: Address fields inside
regionget hard-masked with********, but address fields in other nested objects only get regex-masked.Consider simplifying by either skipping the recursive call for region and only applying the explicit field masking, or documenting why region needs this special treatment.
Suggested simplification
if (key === "region" && value !== null && typeof value === "object") { - const maskedRegion = maskObject(value, seen); - cloned[key] = { - ...maskedRegion, - address: value.address ? MASK : value.address, - city: value.city ? MASK : value.city, - state: value.state ? MASK : value.state, - zipcode: value.zipcode ? MASK : value.zipcode - }; + // Hard-mask known PII fields, recursively process others + const maskedRegion = {}; + for (const regionKey of Object.keys(value)) { + if (['address', 'city', 'state', 'zipcode'].includes(regionKey)) { + maskedRegion[regionKey] = value[regionKey] ? MASK : value[regionKey]; + } else { + maskedRegion[regionKey] = maskObject(value[regionKey], seen); + } + } + cloned[key] = maskedRegion; return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/logger/mask.js` around lines 109 - 118, For the special-case handling of the "region" object in src/logger/mask.js, remove the initial recursive call maskObject(value, seen) (which causes redundant regex-masking) and instead build cloned[key] by: copying non-address fields through maskObject individually (to preserve regular recursive masking for those fields) and explicitly setting address, city, state, and zipcode to MASK when their original values are truthy (keeping original falsy values). Update code around the key === "region" branch (referencing key, value, cloned, maskObject, and MASK) so region-specific address fields are deterministically hard-masked while other region properties still get proper recursive masking without double-processing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/logger/mask.js`:
- Around line 104-107: The condition in the if-statement that checks key and
value has an operator precedence bug: (key === "name") currently bypasses the
typeof check and calls maskName(value) even when value is not a string. Update
the condition in the block that uses key, value and cloned so it only calls
maskName when the key is "name" or "user" AND value is a string (e.g., group the
key checks together before &&), preserving the existing behavior of maskName and
its guard clause but ensuring the type check applies to both keys.
---
Nitpick comments:
In `@src/logger/mask.js`:
- Around line 109-118: For the special-case handling of the "region" object in
src/logger/mask.js, remove the initial recursive call maskObject(value, seen)
(which causes redundant regex-masking) and instead build cloned[key] by: copying
non-address fields through maskObject individually (to preserve regular
recursive masking for those fields) and explicitly setting address, city, state,
and zipcode to MASK when their original values are truthy (keeping original
falsy values). Update code around the key === "region" branch (referencing key,
value, cloned, maskObject, and MASK) so region-specific address fields are
deterministically hard-masked while other region properties still get proper
recursive masking without double-processing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests
Chores