Conversation
WalkthroughThe SizeRotator rotation logic now computes an Changes
Sequence Diagram(s)sequenceDiagram
participant Timer as Size check / trigger
participant Rotator as SizeRotator._setFile
participant FS as File system
Note over Rotator: compute ext = gzip ? '.gz' : ''
Timer->>Rotator: check file size / decide rotation
Rotator->>FS: build maxFileName = `${logPath}.maxFiles${ext}`
Rotator->>FS: for i = max -> 1: build srcPath = `${logPath}.${i}${ext}`
Rotator->>FS: for i = max -> 1: build targetPath = `${logPath}.${i+1}${ext}`
FS-->>Rotator: rename / move (or ignore missing)
Rotator->>FS: write new/empty current file (with ext logic applied where needed)
Note right of FS: try/catch around operations preserved
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
Summary of ChangesHello @hunjixin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a minor but critical bug in the log rotation mechanism. It ensures that the file extension is properly included when determining the source file path for rotation, thereby preventing potential issues with file identification and movement during the log management process. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a bug in the size-based log rotation where the file extension (e.g., .gz) was missing from the source path of rotated log files, ensuring gzipped logs are rotated properly. While this change is correct, a related issue exists in src/lib/size_rotator.ts outside of this diff: at line 38, maxFileName is constructed without the file extension. This will prevent the oldest gzipped log file from being deleted when gzip is enabled, causing rotation to fail. Addressing this in a follow-up would make the size rotation feature fully robust for gzipped files.
2e0a588 to
07ecb9a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/lib/size_rotator.ts (1)
63-74: Approve the rotation logic; consider extracting the duplicated extension calculation.The rotation logic correctly applies the gzip extension to all intermediate and target paths, ensuring consistency throughout the rotation chain.
However, the
extcalculation on line 63 duplicates the logic from line 22. Consider extracting this into a private helper method or computing it once and passing it as a parameter to_setFile.Example refactor:
_setFile(logPath: string, files: Map<string, RotateFile>) { const maxFiles = this.app.config.logrotator.maxFiles; if (files.has(logPath)) { return; } - const ext = this.app.config.logrotator.gzip === true ? '.gz' : ''; + const ext = this._getExtension(); // foo.log.2 -> foo.log.3 // foo.log.1 -> foo.log.2 for (let i = maxFiles - 1; i >= 1; i--) {And add a helper method:
private _getExtension(): string { return this.app.config.logrotator.gzip === true ? '.gz' : ''; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/size_rotator.ts(3 hunks)
🔇 Additional comments (1)
src/lib/size_rotator.ts (1)
22-39: LGTM! Gzip extension handling is correct.The logic correctly computes the extension based on the gzip configuration and applies it to the max file name, ensuring that rotated files with gzip enabled are properly handled.
|
can you add unittest for this bug fix? |
Summary by CodeRabbit