fix: reset stale serialized state when JSON.stringify throws - #437
Open
hiro-nikaitou wants to merge 1 commit into
Open
fix: reset stale serialized state when JSON.stringify throws#437hiro-nikaitou wants to merge 1 commit into
hiro-nikaitou wants to merge 1 commit into
Conversation
When logging objects with circular references, JSON.stringify inside the throttle block throws. The catch block silently absorbed the error without updating _lastLog.serialized, leaving it stale. A subsequent log could then be incorrectly compared against the old serialized value. Reset _lastLog.serialized to undefined in the catch block so the next log always starts with a fresh comparison.
📝 WalkthroughWalkthroughThe throttle serializer now clears ChangesThrottle serialization handling
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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.
Actionable comments posted: 1
🤖 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 `@test/consola.test.ts`:
- Around line 78-94: Make the regression test around consola.log explicitly fail
on the old implementation by verifying that the circular-reference log clears
consola’s _lastLog.serialized state, or change the final message to repeat
"Control message" with the required throttling setup so stale serialization
would suppress it. Keep the existing assertions for distinct output where they
remain applicable.
🪄 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: 33340cef-083e-4782-b2d5-148830927c74
📒 Files selected for processing (2)
src/consola.tstest/consola.test.ts
Comment on lines
+78
to
+94
| // 1. Log a normal message - sets _lastLog.serialized | ||
| consola.log("Control message"); | ||
|
|
||
| // 2. Log an object with circular reference - triggers catch block | ||
| const circular: { value: string; self?: any } = { value: "circular data" }; | ||
| circular.self = circular; | ||
| consola.log(circular); | ||
|
|
||
| // 3. Log another normal message - should NOT be treated as repeat | ||
| consola.log("Different message"); | ||
|
|
||
| await wait(300); | ||
|
|
||
| // All three messages should appear distinctly | ||
| expect(logs.length).toBe(3); | ||
| expect(logs[0].args[0]).toBe("Control message"); | ||
| expect(logs[2].args[0]).toBe("Different message"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the regression test fail without the fix.
"Control message" and "Different message" serialize differently, so the stale "Control message" serialization cannot classify the final log as a repeat. This test therefore passes against the old implementation too. Assert _lastLog.serialized is cleared after the circular log, or repeat the original message with enough throttling state to trigger the old suppression behavior.
Suggested assertion
consola.log(circular);
+ expect(consola._lastLog.serialized).toBeUndefined();
consola.log("Different message");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // 1. Log a normal message - sets _lastLog.serialized | |
| consola.log("Control message"); | |
| // 2. Log an object with circular reference - triggers catch block | |
| const circular: { value: string; self?: any } = { value: "circular data" }; | |
| circular.self = circular; | |
| consola.log(circular); | |
| // 3. Log another normal message - should NOT be treated as repeat | |
| consola.log("Different message"); | |
| await wait(300); | |
| // All three messages should appear distinctly | |
| expect(logs.length).toBe(3); | |
| expect(logs[0].args[0]).toBe("Control message"); | |
| expect(logs[2].args[0]).toBe("Different message"); | |
| // 1. Log a normal message - sets _lastLog.serialized | |
| consola.log("Control message"); | |
| // 2. Log an object with circular reference - triggers catch block | |
| const circular: { value: string; self?: any } = { value: "circular data" }; | |
| circular.self = circular; | |
| consola.log(circular); | |
| expect(consola._lastLog.serialized).toBeUndefined(); | |
| // 3. Log another normal message - should NOT be treated as repeat | |
| consola.log("Different message"); | |
| await wait(300); | |
| // All three messages should appear distinctly | |
| expect(logs.length).toBe(3); | |
| expect(logs[0].args[0]).toBe("Control message"); | |
| expect(logs[2].args[0]).toBe("Different message"); |
🤖 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 `@test/consola.test.ts` around lines 78 - 94, Make the regression test around
consola.log explicitly fail on the old implementation by verifying that the
circular-reference log clears consola’s _lastLog.serialized state, or change the
final message to repeat "Control message" with the required throttling setup so
stale serialization would suppress it. Keep the existing assertions for distinct
output where they remain applicable.
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.
When logging objects with circular references,
JSON.stringifyinside the throttle block throws. The existing catch block silently absorbed the error without updating_lastLog.serialized, leaving it stale. A subsequent log could then be incorrectly compared against the old serialized value within the throttle window.This fix resets
_lastLog.serializedtoundefinedin the catch block, so the next comparison always starts fresh.A test is added that verifies:
_lastLog.serializedSummary by CodeRabbit
Bug Fixes
Tests