feat(logrotator): merge logrotator plugin into monorepo (#logrotator) #5559
Conversation
- Merge @eggjs/logrotator plugin into monorepo at plugins/logrotator - Update version to 5.0.0-beta.13 to align with framework version - Convert tests from Mocha to Vitest framework - Update dependencies to use workspace:* and catalog: references - Add moment and glob to pnpm catalog dependencies - Configure tsdown for unbundled ESM builds - Extend root tsconfig.json following monorepo standards - Update CLAUDE.md documentation to include logrotator plugin - Fix test imports to use beforeEach/afterEach from vitest The logrotator plugin provides automatic log file rotation based on time or size, supports daily rotation with configurable patterns, and includes log cleanup to remove old files based on maxDays configuration. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Rate limit exceeded@fengmk2 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 0 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 ignored due to path filters (1)
📒 Files selected for processing (3)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new @eggjs/logrotator plugin (code, config, types, schedules, boot, tests, docs, license, changelog) that rotates and cleans logs (daily/hourly/size), integrates with egg-schedule, emits a "log-reload" event, and introduces an all-in-one-process messenger mode used by mock apps and tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Scheduler as egg-schedule
participant App as Egg Application
participant Rotator as LogRotator (Day/Hour/Size)
participant FS as File System
participant Messenger as app.messenger
participant Loggers as app.loggers
Scheduler->>App: schedule triggers
App->>Rotator: rotator.rotate()
Rotator->>Rotator: getRotateFiles()
Rotator->>FS: rename/delete (optionally gzip)
FS-->>Rotator: success / error
alt any rotation succeeded
Rotator->>Messenger: emit 'log-reload'
Messenger->>Loggers: reload() (Boot/handlers respond)
end
Rotator-->>App: rotate() completes (summary logged)
sequenceDiagram
autonumber
participant Sender as App
participant IPC as Messenger IPC
participant Self as SameProcess
participant Other as OtherProcess
Sender->>IPC: send('log-reload' message)
alt mode == 'all-in-one-process'
IPC->>Self: emit 'sendmessage-to-self' (in-process dispatch)
else
IPC->>Other: process.send / message (IPC)
end
Note over Self,Other: Receivers invoke loggers.reload()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
There was a problem hiding this comment.
Pull Request Overview
This PR merges the standalone @eggjs/logrotator plugin into the Eggjs monorepo at plugins/logrotator to centralize plugin management.
- Converts the plugin from CommonJS to ESM with TypeScript support and Vitest testing
- Updates build configuration to use tsdown for unbundled ESM builds and align with monorepo standards
- Adds moment and glob dependencies to the pnpm catalog and updates plugin version to 5.0.0-beta.13
Reviewed Changes
Copilot reviewed 82 out of 83 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-workspace.yaml | Adds glob and moment dependencies to catalog for logrotator plugin usage |
| plugins/logrotator/vitest.config.ts | Configures Vitest test runner to include test files in test directory |
| plugins/logrotator/tsdown.config.ts | Configures tsdown for unbundled ESM builds with TypeScript declarations |
| plugins/logrotator/tsconfig.json | Extends root TypeScript config with baseUrl configuration |
| plugins/logrotator/test/* | Test files converted from Mocha to Vitest with comprehensive coverage |
| plugins/logrotator/src/* | Complete plugin source code with TypeScript types and ESM modules |
| plugins/logrotator/package.json | Package configuration with monorepo workspace dependencies and ESM exports |
| packages/egg/src/lib/types.plugin.ts | Updates plugin type references from import to triple-slash directive |
| CLAUDE.md | Documents logrotator plugin features in framework overview |
Deploying egg with
|
| Latest commit: |
00b4f44
|
| Status: | 🚫 Build failed. |
Deploying egg-v3 with
|
| Latest commit: |
00b4f44
|
| Status: | 🚫 Build failed. |
Summary of ChangesHello @fengmk2, 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 integrates the 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully merges the @eggjs/logrotator plugin into the monorepo, converting its codebase to TypeScript and its tests to Vitest. The changes are extensive and well-structured. However, I've identified a few critical issues. The log cleaning logic fails to handle gzipped files, and the utility function for finding log files is broken, which will prevent both cleaning and rotation from working correctly. There's also an issue in size-based rotation when the gzip setting is changed. Additionally, I've noted some minor issues in the documentation and a couple of skipped tests that should be addressed. Overall, a great effort in modernizing and integrating the plugin, but the critical bugs need to be fixed before merging.
| const name = path.extname(file).slice(1); | ||
| if (!/^\d{4}-\d{2}-\d{2}/.test(name)) { | ||
| return false; | ||
| } | ||
| const date = moment(name, 'YYYY-MM-DD').startOf('date'); | ||
| if (!date.isValid()) { | ||
| return false; | ||
| } | ||
| return date.isBefore(expiredDate); |
There was a problem hiding this comment.
The current logic for parsing the date from the filename doesn't account for gzipped log files (ending in .gz). If gzip: true is used for rotation, the rotated files will be named like ...YYYY-MM-DD.gz. path.extname() will return .gz, and the date parsing will fail, meaning gzipped logs will never be cleaned up. This is a critical issue. The logic should be updated to handle the optional .gz extension.
let basename = file;
if (basename.endsWith('.gz')) {
basename = basename.slice(0, -3);
}
const name = path.extname(basename).slice(1);
if (!/^\d{4}-\d{2}-\d{2}/.test(name)) {
return false;
}
const date = moment(name, 'YYYY-MM-DD').startOf('date');
if (!date.isValid()) {
return false;
}
return date.isBefore(expiredDate);| */ | ||
| export function walkLoggerFile(loggers: Record<string, Map<string, LoggerTransport>>) { | ||
| const files: string[] = []; | ||
| for (const registeredLogger of Object.values(loggers)) { |
There was a problem hiding this comment.
There's a critical issue here. app.loggers is an EggLoggers instance, which extends Map. Using Object.values(loggers) on a Map instance will return an empty array, causing walkLoggerFile to always return []. This means no log files will be found for rotation or cleaning. You should use loggers.values() to iterate over the map's values. Also, the type hint for loggers should probably be Map<string, Map<string, LoggerTransport>> instead of a Record.
| for (const registeredLogger of Object.values(loggers)) { | |
| for (const registeredLogger of loggers.values()) { |
| // foo.log.2 -> foo.log.3 | ||
| // foo.log.1 -> foo.log.2 | ||
| for (let i = maxFiles - 1; i >= 1; i--) { | ||
| const srcPath = `${logPath}.${i}`; |
There was a problem hiding this comment.
This loop for rotating size-based logs doesn't account for previously gzipped files. srcPath is constructed as ${logPath}.${i}, but if gzip was previously enabled, the file might exist as ${logPath}.${i}.gz. This will cause rotation to fail if the gzip config is toggled. The logic should check for the existence of both the plain and gzipped versions of the source file. This might require making _setFile async to use await exists().
| - Built-in memory store for development | ||
| - Configurable session options (maxAge, renew, etc.) | ||
| - Session encryption and signing | ||
| - Compatible with Redis and other external stores |
There was a problem hiding this comment.
This change seems to have accidentally removed the last feature point for the session plugin (- Compatible with Redis and other external stores). The new logrotator section should be added after the complete session section to keep the documentation accurate.
| function getRotator(app) { | ||
| class CustomRotator extends app.LogRotator { | ||
| // return map that contains a pair of srcPath and targetPath | ||
| // LogRotator will rename ksrcPath to targetPath |
There was a problem hiding this comment.
There's a small typo in the comment. ksrcPath should be srcPath.
| // LogRotator will rename ksrcPath to targetPath | |
| // LogRotator will rename srcPath to targetPath |
|
|
||
| ### 按小时切割 | ||
|
|
||
| 可以配置 `filesRotateBySize` 文件列表按小时切割，每小时0分开始切割，格式为 `.log.YYYY-MM-DD-HH`。 |
There was a problem hiding this comment.
There seems to be a copy-paste error here. The text describes rotating by hour, but it references filesRotateBySize. This should probably be filesRotateByHour to match the description.
| 可以配置 `filesRotateBySize` 文件列表按小时切割，每小时0分开始切割，格式为 `.log.YYYY-MM-DD-HH`。 | |
| 可以配置 `filesRotateByHour` 文件列表按小时切割，每小时0分开始切割，格式为 `.log.YYYY-MM-DD-HH`。 |
| *task() { | ||
| yield rotator.rotate(); | ||
| }, | ||
| }; | ||
| }; | ||
|
|
||
| function getRotator(app) { | ||
| class CustomRotator extends app.LogRotator { | ||
| // return map that contains a pair of srcPath and targetPath | ||
| // LogRotator will rename srcPath to targetPath | ||
| // 返回一个 map，其中包含 srcPath 和 targetPath， | ||
| // LogRotator 会将 srcPath 重命名成 targetPath | ||
| *getRotateFiles() { | ||
| const files = new Map(); | ||
| const srcPath = '/home/admin/foo.log'; | ||
| const targetPath = '/home/admin/foo.log.2016.09.30'; | ||
| files.set(srcPath, { srcPath, targetPath }); | ||
| return files; | ||
| } | ||
| } | ||
| return new CustomRotator({ app }); | ||
| } |
There was a problem hiding this comment.
The code examples in this README are using generator functions (*task(), *getRotateFiles()). For consistency with the English README.md and modern JavaScript practices within the project, these should be updated to use async/await.
async task() {
await rotator.rotate();
},
};
};
function getRotator(app) {
class CustomRotator extends app.LogRotator {
// return map that contains a pair of srcPath and targetPath
// LogRotator will rename srcPath to targetPath
// 返回一个 map，其中包含 srcPath 和 targetPath，
// LogRotator 会将 srcPath 重命名成 targetPath
async getRotateFiles() {
const files = new Map();
const srcPath = '/home/admin/foo.log';
const targetPath = '/home/admin/foo.log.2016.09.30';
files.set(srcPath, { srcPath, targetPath });
return files;
}
}
return new CustomRotator({ app });
}| await app.runSchedule(schedule); | ||
| }); | ||
|
|
||
| it.skip('should error when rename to existed file', async () => { |
There was a problem hiding this comment.
This file contains skipped tests (e.g., it.skip). Skipped tests can hide regressions and should be avoided in the final codebase. Please either fix and enable these tests or remove them if they are no longer relevant.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pnpm-workspace.yaml (1)
13-13: Remove internal plugin from catalog; use workspace for internal deps
@eggjs/logrotatoris now in this monorepo. Keeping it in catalog risks pulling the external ^4.x instead of your internal 5.0.0-beta.13 when someone uses catalog:. Remove this entry. As per coding guidelines.'@eggjs/ip': ^2.1.0 - '@eggjs/logrotator': ^4.0.0 '@eggjs/multipart': ^4.0.0plugins/logrotator/src/typings/index.d.ts (1)
1-5: Include ambient typings in published package
package.json’sfilesonly listsdist, sosrc/typings/index.d.tswon’t be shipped—add it (e.g. in plugins/logrotator/package.json):"files": [ - "dist" + "dist", + "src/typings/index.d.ts" ]
🧹 Nitpick comments (43)
plugins/logrotator/test/fixtures/logrotator-json-format/config/config.default.js (3)
11-18: Ensure JSON-suffixed files are actually written or remove them from rotation listsYou rotate hour.json.log and size.json.log but no logger writes to them. Either add matching custom loggers or drop these entries to avoid dead config and test flakiness.
Option A — add matching loggers:
customLogger: { dayLogger: { file: path.join(__dirname, '../logs', 'logrotator', 'day.log'), }, sizeLogger: { file: path.join(__dirname, '../logs', 'logrotator', 'size.log'), }, + jsonSizeLogger: { + file: path.join(__dirname, '../logs', 'logrotator', 'size.json.log'), + }, hourLogger: { file: path.join(__dirname, '../logs', 'logrotator', 'hour.log'), }, + jsonHourLogger: { + file: path.join(__dirname, '../logs', 'logrotator', 'hour.json.log'), + }, },Option B — remove the unused JSON-suffixed paths from the rotation arrays.
Also applies to: 22-32
19-20: Confirm maxFileSize units; 1 may cause excessive rotationsIf maxFileSize is in bytes (as in other fixtures set to 1024), 1 will rotate almost every write and can introduce flakiness. If intentional, ignore; otherwise align to 1024.
- maxFileSize: 1, + maxFileSize: 1024,
6-6: Rename localexportstoconfigfor clarityAvoid shadowing Node’s
exportsconcept; improves readability.-module.exports = () => { - const exports = { +module.exports = () => { + const config = { ... - }; - return exports; + }; + return config; };Also applies to: 34-34
plugins/logrotator/test/fixtures/logrotator-app-size/package.json (1)
1-3: Mark fixture package manifests as privatePlease add
"private": trueso this fixture can't be accidentally published to npm. Apply the same change to the other newly added fixture package.json files in this PR.{ "name": "logrotator-app-size" + , "private": true }plugins/logrotator/test/fixtures/logrotator-default/package.json (1)
1-3: Mark fixture package manifests as privateAdd
"private": truehere (and in the rest of the new fixture package.json files) to avoid accidental publication.{ "name": "logrotator-default" + , "private": true }plugins/logrotator/test/fixtures/clean-log/package.json (1)
1-3: Mark fixture package manifests as privatePlease mark this fixture package as private (and do the same for the other new fixtures) so it cannot be published.
{ "name": "clean-log" + , "private": true }plugins/logrotator/test/fixtures/logrotator-app-hour-gzip/package.json (1)
1-3: Mark fixture package manifests as privateAdd
"private": trueto ensure this fixture package isn’t publishable; mirror the change across the other new fixture manifests.{ "name": "logrotator-app-hour-gzip" + , "private": true }plugins/logrotator/test/fixtures/logrotator-app-size-gzip/package.json (1)
1-3: Mark fixture package.json as private to avoid accidental publishRecommended for all test fixtures.
{ "name": "logrotator-app-size-gzip" + , "private": true }plugins/logrotator/test/fixtures/logger-reload/package.json (1)
1-3: Mark fixture package.json as private to avoid accidental publishRecommended for all test fixtures.
{ "name": "logger-reload" + , "private": true }plugins/logrotator/test/fixtures/logrotator-app-hour-gzip/app/router.js (1)
1-5: Prefer ctx over this in route handlersUse ctx param for clarity/future‑proofing with Koa/Egg handlers.
module.exports = app => { - app.get('/', async function () { - this.body = 123; - }); + app.get('/', async ctx => { + ctx.body = 123; + }); };plugins/logrotator/test/fixtures/logrotator-app-size/app/router.js (1)
1-5: Prefer ctx over this in route handlersAlign with Koa v2 conventions for readability and to avoid implicit this binding.
module.exports = app => { - app.get('/', async function () { - this.body = 123; - }); + app.get('/', async ctx => { + ctx.body = 123; + }); };plugins/logrotator/test/fixtures/logrotator-app-size/app.js (1)
3-5: Use app.coreLogger instead of console for test observabilityLeverage framework logger so tests can assert via log files/levels consistently.
If tests intentionally assert console output, keep as-is.
module.exports = function (app) { - app.messenger.on('log-reload', () => console.log('app got log-reload')); + app.messenger.on('log-reload', () => app.coreLogger.info('app got log-reload')); };plugins/logrotator/test/index.test.ts (1)
1-7: Align assertions with Vitest conventionsPer our test guidelines we should import
describe/itfrom Vitest but rely on Node’s built-inassertmodule for expectations, rather thanexpect.As per coding guidelines
-import { describe, it, expect } from 'vitest'; -import { LogRotator } from '../src/index.js'; +import { describe, it } from 'vitest'; +import assert from 'node:assert/strict'; +import { LogRotator } from '../src/index.js'; describe('test/index.test.ts', () => { it('should export LogRotator', () => { - expect(LogRotator).toBeDefined(); + assert.ok(LogRotator); }); });plugins/logrotator/test/fixtures/logrotator-app-size-gzip/app/router.js (1)
2-4: Prefer ctx param over relying on this bindingAvoid implicit this to make the handler resilient across Koa/Egg versions.
- app.get('/', async function () { - this.body = 123; - }); + app.get('/', async (ctx) => { + ctx.body = 123; + });plugins/logrotator/.gitignore (1)
1-15: Ignore fixture logs and pnpm debug logsTests for log rotation will produce logs under fixtures; also ignore pnpm debug logs.
logs/ -npm-debug.log +npm-debug.log +pnpm-debug.log* node_modules/ coverage/ .idea/ run/ .DS_Store *.swp -test/fixtures/**/run +test/fixtures/**/run +test/fixtures/**/logs .tshy* .eslintcache dist package-lock.json .package-lock.jsonplugins/logrotator/src/typings/index.d.ts (1)
1-5: Ensure the .d.ts stays a module even if the import is elidedAdding an empty export keeps this file module-scoped in all toolchains.
import 'egg'; +export {};plugins/logrotator/test/fixtures/logrotator-app-hour-gzip/agent.js (1)
4-4: Use coreLogger instead of console.log in fixturesRoutes logs through Egg’s logger and keeps test output cleaner.
- agent.messenger.on('log-reload', () => console.log('agent got log-reload')); + agent.messenger.on('log-reload', () => agent.coreLogger.info('agent got log-reload'));plugins/logrotator/README.md (2)
14-14: Fix markdown heading style consistency.The static analysis tool flagged inconsistent heading styles. According to the markdown linter, this should use ATX-style headings (
#) instead of setext-style.-LogRotator for egg. Rotate all file of `app.loggers` by default +# LogRotator for egg + +Rotate all file of `app.loggers` by default
1-122: Consider addressing markdown linting issues for consistency.The static analysis tool identified several markdown formatting issues including inconsistent heading styles, bare URLs, and list indentation. While not critical, addressing these would improve document consistency and maintainability.
For better markdown consistency, consider:
- Using ATX-style headings (
#) consistently- Proper list indentation (0 spaces for top-level items)
- Converting bare URLs to proper markdown links where appropriate
These are style improvements that would align with common markdown best practices.
plugins/logrotator/src/index.ts (1)
3-3: Prefer explicit, stable re-exports overexport *Avoid broad surface changes and make the public API intentional. Also re-export concrete rotators and types if they’re part of the intended API.
As per coding guidelines. Example:
-export * from './lib/rotator.js'; +export { LogRotator } from './lib/rotator.js'; +export type { RotatorOptions, RotateFile } from './lib/rotator.js'; +export { DayRotator } from './lib/day_rotator.js'; +export { HourRotator } from './lib/hour_rotator.js'; +export { SizeRotator } from './lib/size_rotator.js';plugins/logrotator/src/app/schedule/rotate_by_file.ts (1)
1-18: Optional: Rename rotate_by_file.ts to rotate_by_day.ts
This file uses DayRotator for daily rotation; renaming it will improve clarity.plugins/logrotator/test/fixtures/logger-reload/app/router.js (2)
1-1: Consider using ESM imports for consistency.The file mixes Node.js built-in imports with CommonJS module pattern. For consistency with the TypeScript sources that use ESM, consider using
importstatements.Apply this diff to use ESM imports:
-const path = require('node:path'); +import path from 'node:path';
13-17: Consider using dynamic import for schedule path resolution.The hardcoded relative path to the schedule file could break if the directory structure changes. Since this is test code and the path is relative to the fixture structure, it should be acceptable, but using a more dynamic approach would be more robust.
Based on relevant code snippets from
plugins/logrotator/src/app/schedule/rotate_by_file.ts, the schedule file returns a function that accepts anappparameter. The current implementation should work correctly withapp.runSchedule().plugins/logrotator/src/lib/utils.ts (2)
1-5: Improve interface to match actual egg-logger Transport structure.The
LoggerTransportinterface appears to be simplified compared to the actual egg-logger Transport structure. Based on egg-logger documentation, transports like FileTransport have more complex option structures.Consider expanding the interface to better match the actual Transport structure:
interface LoggerTransport { options: { - file: string; + file?: string; + level?: string; + [key: string]: any; }; }
14-17: Add explicit type annotation for better clarity.The
filevariable could benefit from explicit typing to make the code more self-documenting.Apply this diff for better type clarity:
- const file = transport.options.file; + const file: string | undefined = transport.options.file;plugins/logrotator/src/boot.ts (1)
6-12: Add explicit return type to public lifecycle method.Keep public API signatures explicit.
As per coding guidelines
- async didLoad() { + async didLoad(): Promise<void> {plugins/logrotator/src/app/schedule/rotate_by_size.ts (2)
8-12: Guard missing rotateDuration and provide a sane default.If rotateDuration is undefined while filesRotateBySize is set, scheduler config may be invalid. Coalesce to a default and/or disable when missing.
return { schedule: { type: 'worker', - interval: app.config.logrotator.rotateDuration, - disable: (app.config.logrotator.filesRotateBySize || []).length === 0, + interval: app.config.logrotator.rotateDuration ?? 60_000, + disable: + (app.config.logrotator.filesRotateBySize || []).length === 0 || + app.config.logrotator.rotateDuration == null, },
4-18: Consider explicit return type for the exported schedule factory.Optional, but aligns with strict typing for public APIs.
As per coding guidelines
plugins/logrotator/src/app/schedule/clean_log.ts (1)
35-36: Log only when cleanup is enabledAvoid logging when
maxDaysis falsy/0 to reduce noise.Apply this diff:
- logger.info('[@eggjs/logrotator] clean all log before %s days', maxDays); + if (maxDays && maxDays > 0) { + logger.info('[@eggjs/logrotator] clean all log before %s days', maxDays); + }plugins/logrotator/test/fixtures/logrotator-app-size/config/config.default.js (1)
9-14: Use path.join segments for cross‑platform correctnessAvoid embedding path separators in a single segment.
Apply this diff:
- path.join(appInfo.baseDir, `logs/${appInfo.name}/egg-web.log`), - path.join(appInfo.baseDir, `logs/${appInfo.name}/egg-web.log`), + path.join(appInfo.baseDir, 'logs', appInfo.name, 'egg-web.log'), + path.join(appInfo.baseDir, 'logs', appInfo.name, 'egg-web.log'), 'egg-web.log', // ignore unexist file - path.join(appInfo.baseDir, `logs/${appInfo.name}/no-exist.log`), + path.join(appInfo.baseDir, 'logs', appInfo.name, 'no-exist.log'),plugins/logrotator/src/config/config.default.ts (1)
5-60: Consider readonly fields in config interfaceConfig is effectively immutable at runtime; marking arrays and fields as readonly improves type safety.
Example:
export interface LogrotatorConfig { readonly disableRotateByDay: boolean; readonly filesRotateByHour: readonly string[] | null; readonly hourDelimiter: string; readonly filesRotateBySize: readonly string[] | null; readonly maxFileSize: number; readonly maxFiles: number; readonly rotateDuration: number; readonly maxDays: number; readonly gzip: boolean; }As per coding guidelines.
plugins/logrotator/src/lib/size_rotator.ts (2)
15-16: Add explicit return type to public methodApply this diff:
- async getRotateFiles() { + async getRotateFiles(): Promise<Map<string, RotateFile>> {As per coding guidelines.
54-54: Add explicit return type to helperApply this diff:
- _setFile(logPath: string, files: Map<string, RotateFile>) { + _setFile(logPath: string, files: Map<string, RotateFile>): void {plugins/logrotator/src/lib/hour_rotator.ts (3)
14-16: Add explicit return type to public methodApply this diff:
- async getRotateFiles() { + async getRotateFiles(): Promise<Map<string, RotateFile>> {As per coding guidelines.
34-36: Type getter explicitlyApply this diff:
- get hourDelimiter() { + get hourDelimiter(): string {
38-45: Type private helper explicitlyApply this diff:
- _setFile(srcPath: string, files: Map<string, RotateFile>) { + _setFile(srcPath: string, files: Map<string, RotateFile>): void {plugins/logrotator/test/clean_log.test.ts (1)
11-11: Prefer Vitest native snapshots over snap-shot-itUsing snap-shot-it with Vitest works but is non-idiomatic. Vitest has built-in snapshots.
Apply this diff:
-import snapshot from 'snap-shot-it'; +// Prefer Vitest snapshots-import { describe, it, beforeEach, afterEach } from 'vitest'; +import { describe, it, beforeEach, afterEach, expect } from 'vitest';- it('should keep config stable', () => { - snapshot(app.config.logrotator); - }); + it('should keep config stable', () => { + expect(app.config.logrotator).toMatchSnapshot(); + });As per coding guidelines.
Also applies to: 19-19, 45-46
plugins/logrotator/src/lib/day_rotator.ts (3)
16-23: Harden types: mark fields readonly and add explicit typesAlign with strict TS and explicit public API types.
Apply this diff:
-export class DayRotator extends LogRotator { - private filesRotateBySize: string[]; - private filesRotateByHour: string[]; +export class DayRotator extends LogRotator { + private readonly filesRotateBySize: readonly string[]; + private readonly filesRotateByHour: readonly string[];- this.filesRotateBySize = this.app.config.logrotator.filesRotateBySize || []; - this.filesRotateByHour = this.app.config.logrotator.filesRotateByHour || []; + this.filesRotateBySize = this.app.config.logrotator.filesRotateBySize || []; + this.filesRotateByHour = this.app.config.logrotator.filesRotateByHour || [];As per coding guidelines.
25-36: Add explicit return type on public methodMake the API surface clear and stable.
Apply this diff:
- async getRotateFiles() { + async getRotateFiles(): Promise<Map<string, RotateFile>> {As per coding guidelines.
73-91: Limit visibility and annotate return type for helperThis is an internal helper; mark it private and add return type.
Apply this diff:
- _setFile(srcPath: string, files: Map<string, RotateFile>) { + private _setFile(srcPath: string, files: Map<string, RotateFile>): void {As per coding guidelines.
plugins/logrotator/test/logrotator.test.ts (3)
94-98: Avoid brittle array index in log assertionsUse contains check instead of msg[1] to prevent order sensitivity.
Apply this diff:
- assert.match(msg[1], /rotate files success by DayRotator/); + assert(msg.some(m => /rotate files success by DayRotator/.test(m)));
486-492: Wait for gzip stream completion instead of sleepingRely on stream events to make tests deterministic.
Example refactor:
await new Promise<void>((resolve, reject) => { const gzip = createUnzip(); fs.createReadStream(file).pipe(gzip); gzip.on('data', data => { assert(data.toString().includes('logrotator-app-hour-gzip')); // adjust per test }); gzip.once('end', resolve); gzip.once('error', reject); });Also applies to: 514-521, 545-551
34-35: Standardize schedule module path usageMixed usage with and without “.ts” can confuse ESM resolution. Prefer a consistent style across tests.
No behavior change; choose either always with or always without “.ts” in these constants.
Also applies to: 186-187, 314-315, 338-339, 362-372, 391-392, 414-415, 436-447, 469-470, 497-498, 527-528
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (83)
CLAUDE.md(1 hunks)packages/egg/src/lib/types.plugin.ts(1 hunks)plugins/logrotator/.gitignore(1 hunks)plugins/logrotator/CHANGELOG.md(1 hunks)plugins/logrotator/LICENSE(1 hunks)plugins/logrotator/README.md(1 hunks)plugins/logrotator/README.zh-CN.md(1 hunks)plugins/logrotator/__snapshots__/clean_log.test.ts.js(1 hunks)plugins/logrotator/package.json(1 hunks)plugins/logrotator/src/agent.ts(1 hunks)plugins/logrotator/src/app.ts(1 hunks)plugins/logrotator/src/app/extend/agent.ts(1 hunks)plugins/logrotator/src/app/extend/application.ts(1 hunks)plugins/logrotator/src/app/schedule/clean_log.ts(1 hunks)plugins/logrotator/src/app/schedule/rotate_by_file.ts(1 hunks)plugins/logrotator/src/app/schedule/rotate_by_hour.ts(1 hunks)plugins/logrotator/src/app/schedule/rotate_by_size.ts(1 hunks)plugins/logrotator/src/boot.ts(1 hunks)plugins/logrotator/src/config/config.default.ts(1 hunks)plugins/logrotator/src/index.ts(1 hunks)plugins/logrotator/src/lib/day_rotator.ts(1 hunks)plugins/logrotator/src/lib/hour_rotator.ts(1 hunks)plugins/logrotator/src/lib/rotator.ts(1 hunks)plugins/logrotator/src/lib/size_rotator.ts(1 hunks)plugins/logrotator/src/lib/utils.ts(1 hunks)plugins/logrotator/src/types.ts(1 hunks)plugins/logrotator/src/typings/index.d.ts(1 hunks)plugins/logrotator/test/clean_log.test.ts(1 hunks)plugins/logrotator/test/fixtures/clean-log/config/config.default.js(1 hunks)plugins/logrotator/test/fixtures/clean-log/package.json(1 hunks)plugins/logrotator/test/fixtures/logger-reload/agent.js(1 hunks)plugins/logrotator/test/fixtures/logger-reload/app/router.js(1 hunks)plugins/logrotator/test/fixtures/logger-reload/config/config.js(1 hunks)plugins/logrotator/test/fixtures/logger-reload/package.json(1 hunks)plugins/logrotator/test/fixtures/logrotator-agent/app.js(1 hunks)plugins/logrotator/test/fixtures/logrotator-agent/config/config.default.js(1 hunks)plugins/logrotator/test/fixtures/logrotator-agent/package.json(1 hunks)plugins/logrotator/test/fixtures/logrotator-app-day-gzip/agent.js(1 hunks)plugins/logrotator/test/fixtures/logrotator-app-day-gzip/app.js(1 hunks)plugins/logrotator/test/fixtures/logrotator-app-day-gzip/app/router.js(1 hunks)plugins/logrotator/test/fixtures/logrotator-app-day-gzip/config/config.default.js(1 hunks)plugins/logrotator/test/fixtures/logrotator-app-day-gzip/package.json(1 hunks)plugins/logrotator/test/fixtures/logrotator-app-hour-custom_hourdelimiter/agent.js(1 hunks)plugins/logrotator/test/fixtures/logrotator-app-hour-custom_hourdelimiter/app.js(1 hunks)plugins/logrotator/test/fixtures/logrotator-app-hour-custom_hourdelimiter/app/router.js(1 hunks)plugins/logrotator/test/fixtures/logrotator-app-hour-custom_hourdelimiter/config/config.default.js(1 hunks)plugins/logrotator/test/fixtures/logrotator-app-hour-custom_hourdelimiter/package.json(1 hunks)plugins/logrotator/test/fixtures/logrotator-app-hour-gzip/agent.js(1 hunks)plugins/logrotator/test/fixtures/logrotator-app-hour-gzip/app.js(1 hunks)plugins/logrotator/test/fixtures/logrotator-app-hour-gzip/app/router.js(1 hunks)plugins/logrotator/test/fixtures/logrotator-app-hour-gzip/config/config.default.js(1 hunks)plugins/logrotator/test/fixtures/logrotator-app-hour-gzip/package.json(1 hunks)plugins/logrotator/test/fixtures/logrotator-app-hour/agent.js(1 hunks)plugins/logrotator/test/fixtures/logrotator-app-hour/app.js(1 hunks)plugins/logrotator/test/fixtures/logrotator-app-hour/app/router.js(1 hunks)plugins/logrotator/test/fixtures/logrotator-app-hour/config/config.default.js(1 hunks)plugins/logrotator/test/fixtures/logrotator-app-hour/package.json(1 hunks)plugins/logrotator/test/fixtures/logrotator-app-size-gzip/agent.js(1 hunks)plugins/logrotator/test/fixtures/logrotator-app-size-gzip/app.js(1 hunks)plugins/logrotator/test/fixtures/logrotator-app-size-gzip/app/router.js(1 hunks)plugins/logrotator/test/fixtures/logrotator-app-size-gzip/config/config.default.js(1 hunks)plugins/logrotator/test/fixtures/logrotator-app-size-gzip/package.json(1 hunks)plugins/logrotator/test/fixtures/logrotator-app-size/agent.js(1 hunks)plugins/logrotator/test/fixtures/logrotator-app-size/app.js(1 hunks)plugins/logrotator/test/fixtures/logrotator-app-size/app/router.js(1 hunks)plugins/logrotator/test/fixtures/logrotator-app-size/config/config.default.js(1 hunks)plugins/logrotator/test/fixtures/logrotator-app-size/package.json(1 hunks)plugins/logrotator/test/fixtures/logrotator-app/agent.js(1 hunks)plugins/logrotator/test/fixtures/logrotator-app/app.js(1 hunks)plugins/logrotator/test/fixtures/logrotator-app/app/router.js(1 hunks)plugins/logrotator/test/fixtures/logrotator-app/config/config.default.js(1 hunks)plugins/logrotator/test/fixtures/logrotator-app/package.json(1 hunks)plugins/logrotator/test/fixtures/logrotator-default/package.json(1 hunks)plugins/logrotator/test/fixtures/logrotator-json-format/config/config.default.js(1 hunks)plugins/logrotator/test/fixtures/logrotator-json-format/package.json(1 hunks)plugins/logrotator/test/fixtures/noexist-rotator-dir/config/config.default.js(1 hunks)plugins/logrotator/test/fixtures/noexist-rotator-dir/package.json(1 hunks)plugins/logrotator/test/index.test.ts(1 hunks)plugins/logrotator/test/logrotator.test.ts(1 hunks)plugins/logrotator/tsconfig.json(1 hunks)plugins/logrotator/tsdown.config.ts(1 hunks)plugins/logrotator/vitest.config.ts(1 hunks)pnpm-workspace.yaml(2 hunks)
🧰 Additional context used
📓 Path-based instructions (11)
**/test/fixtures/**
📄 CodeRabbit inference engine (AGENTS.md)
Put reusable test data under test/fixtures/
Files:
plugins/logrotator/test/fixtures/logger-reload/package.jsonplugins/logrotator/test/fixtures/logrotator-app-size/package.jsonplugins/logrotator/test/fixtures/logger-reload/app/router.jsplugins/logrotator/test/fixtures/logger-reload/config/config.jsplugins/logrotator/test/fixtures/logrotator-app-hour-gzip/app/router.jsplugins/logrotator/test/fixtures/logrotator-app-size-gzip/package.jsonplugins/logrotator/test/fixtures/logrotator-app-hour/package.jsonplugins/logrotator/test/fixtures/logrotator-app-hour-gzip/package.jsonplugins/logrotator/test/fixtures/logrotator-agent/package.jsonplugins/logrotator/test/fixtures/logrotator-agent/config/config.default.jsplugins/logrotator/test/fixtures/logrotator-app-size-gzip/app.jsplugins/logrotator/test/fixtures/logrotator-app/app/router.jsplugins/logrotator/test/fixtures/logrotator-app-hour-custom_hourdelimiter/config/config.default.jsplugins/logrotator/test/fixtures/logrotator-app-day-gzip/package.jsonplugins/logrotator/test/fixtures/logrotator-app-size/agent.jsplugins/logrotator/test/fixtures/logrotator-app-hour-gzip/app.jsplugins/logrotator/test/fixtures/logrotator-app-day-gzip/agent.jsplugins/logrotator/test/fixtures/logrotator-app-day-gzip/config/config.default.jsplugins/logrotator/test/fixtures/logrotator-app-size/config/config.default.jsplugins/logrotator/test/fixtures/logrotator-json-format/config/config.default.jsplugins/logrotator/test/fixtures/logrotator-app/app.jsplugins/logrotator/test/fixtures/noexist-rotator-dir/config/config.default.jsplugins/logrotator/test/fixtures/logrotator-app-size/app.jsplugins/logrotator/test/fixtures/logrotator-app-size-gzip/agent.jsplugins/logrotator/test/fixtures/logrotator-app-day-gzip/app.jsplugins/logrotator/test/fixtures/logrotator-default/package.jsonplugins/logrotator/test/fixtures/logrotator-app-day-gzip/app/router.jsplugins/logrotator/test/fixtures/logrotator-app-hour-custom_hourdelimiter/agent.jsplugins/logrotator/test/fixtures/logrotator-app-hour-gzip/agent.jsplugins/logrotator/test/fixtures/logrotator-app-hour-custom_hourdelimiter/app.jsplugins/logrotator/test/fixtures/clean-log/config/config.default.jsplugins/logrotator/test/fixtures/logrotator-agent/app.jsplugins/logrotator/test/fixtures/logrotator-app/agent.jsplugins/logrotator/test/fixtures/logrotator-app-hour-custom_hourdelimiter/package.jsonplugins/logrotator/test/fixtures/noexist-rotator-dir/package.jsonplugins/logrotator/test/fixtures/logrotator-json-format/package.jsonplugins/logrotator/test/fixtures/logrotator-app-hour/app/router.jsplugins/logrotator/test/fixtures/logrotator-app-hour/agent.jsplugins/logrotator/test/fixtures/logrotator-app-hour-gzip/config/config.default.jsplugins/logrotator/test/fixtures/clean-log/package.jsonplugins/logrotator/test/fixtures/logrotator-app-size/app/router.jsplugins/logrotator/test/fixtures/logrotator-app-hour/app.jsplugins/logrotator/test/fixtures/logrotator-app-hour-custom_hourdelimiter/app/router.jsplugins/logrotator/test/fixtures/logrotator-app/package.jsonplugins/logrotator/test/fixtures/logger-reload/agent.jsplugins/logrotator/test/fixtures/logrotator-app-size-gzip/app/router.jsplugins/logrotator/test/fixtures/logrotator-app-hour/config/config.default.jsplugins/logrotator/test/fixtures/logrotator-app-size-gzip/config/config.default.jsplugins/logrotator/test/fixtures/logrotator-app/config/config.default.js
**/package.json
📄 CodeRabbit inference engine (CLAUDE.md)
**/package.json: All packages must declare Node.js engines >= 22.18.0
All packages must include a typecheck script running 'tsc --noEmit'
All packages must use oxlint for linting with 'oxlint --type-aware' and provide a lint:fix script that runs '--fix'
Reference external dependencies using "catalog:" and internal workspace dependencies using "workspace:"
Use workspace: for internal dependencies across the monorepo
Files:
plugins/logrotator/test/fixtures/logger-reload/package.jsonplugins/logrotator/test/fixtures/logrotator-app-size/package.jsonplugins/logrotator/test/fixtures/logrotator-app-size-gzip/package.jsonplugins/logrotator/test/fixtures/logrotator-app-hour/package.jsonplugins/logrotator/test/fixtures/logrotator-app-hour-gzip/package.jsonplugins/logrotator/test/fixtures/logrotator-agent/package.jsonplugins/logrotator/test/fixtures/logrotator-app-day-gzip/package.jsonplugins/logrotator/package.jsonplugins/logrotator/test/fixtures/logrotator-default/package.jsonplugins/logrotator/test/fixtures/logrotator-app-hour-custom_hourdelimiter/package.jsonplugins/logrotator/test/fixtures/noexist-rotator-dir/package.jsonplugins/logrotator/test/fixtures/logrotator-json-format/package.jsonplugins/logrotator/test/fixtures/clean-log/package.jsonplugins/logrotator/test/fixtures/logrotator-app/package.json
plugins/**/package.json
📄 CodeRabbit inference engine (CLAUDE.md)
plugins/**/package.json: All plugins must include egg in peerDependencies with version 'workspace:*'
Plugin package.json exports must map TypeScript sources for development (e.g., '.': './src/index.ts', './agent': './src/agent.ts', './app': './src/app.ts')
Plugin publishConfig.exports must mirror exports but point to compiled JS in dist (e.g., '.': './dist/index.js')
Plugins must include scripts: build (tsdown), clean (rimraf dist), typecheck (tsc --noEmit), lint (oxlint --type-aware), lint:fix, test (lint:fix && vitest), prepublishOnly (build)
Plugins should publish only 'dist' via the files field
Plugin package.json must include an eggPlugin configuration field
Files:
plugins/logrotator/test/fixtures/logger-reload/package.jsonplugins/logrotator/test/fixtures/logrotator-app-size/package.jsonplugins/logrotator/test/fixtures/logrotator-app-size-gzip/package.jsonplugins/logrotator/test/fixtures/logrotator-app-hour/package.jsonplugins/logrotator/test/fixtures/logrotator-app-hour-gzip/package.jsonplugins/logrotator/test/fixtures/logrotator-agent/package.jsonplugins/logrotator/test/fixtures/logrotator-app-day-gzip/package.jsonplugins/logrotator/package.jsonplugins/logrotator/test/fixtures/logrotator-default/package.jsonplugins/logrotator/test/fixtures/logrotator-app-hour-custom_hourdelimiter/package.jsonplugins/logrotator/test/fixtures/noexist-rotator-dir/package.jsonplugins/logrotator/test/fixtures/logrotator-json-format/package.jsonplugins/logrotator/test/fixtures/clean-log/package.jsonplugins/logrotator/test/fixtures/logrotator-app/package.json
{packages,plugins,tools}/**/tsconfig.json
📄 CodeRabbit inference engine (CLAUDE.md)
All sub-project tsconfig.json files must extend the root tsconfig.json using "extends": "../../tsconfig.json" and set compilerOptions.baseUrl to "./"
Files:
plugins/logrotator/tsconfig.json
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: Prefer TypeScript and ESM: write sources and exports in .ts (ESM-first) rather than CommonJS
Use two-space indentation, trailing commas, and semicolons (Prettier/oxlint defaults)
Name files in lowercase with hyphens (e.g., loader-context.ts)
Name classes in PascalCase
Name functions and variables in camelCase
Re-export types thoughtfully to keep the public API stable
Files:
plugins/logrotator/src/app.tsplugins/logrotator/src/types.tsplugins/logrotator/src/lib/utils.tsplugins/logrotator/tsdown.config.tsplugins/logrotator/src/app/extend/application.tsplugins/logrotator/src/config/config.default.tsplugins/logrotator/src/index.tsplugins/logrotator/src/app/schedule/rotate_by_file.tsplugins/logrotator/src/boot.tsplugins/logrotator/test/index.test.tsplugins/logrotator/src/app/schedule/clean_log.tspackages/egg/src/lib/types.plugin.tsplugins/logrotator/src/lib/size_rotator.tsplugins/logrotator/src/lib/hour_rotator.tsplugins/logrotator/src/app/extend/agent.tsplugins/logrotator/vitest.config.tsplugins/logrotator/src/agent.tsplugins/logrotator/src/app/schedule/rotate_by_size.tsplugins/logrotator/src/app/schedule/rotate_by_hour.tsplugins/logrotator/src/lib/rotator.tsplugins/logrotator/src/typings/index.d.tsplugins/logrotator/src/lib/day_rotator.tsplugins/logrotator/test/logrotator.test.tsplugins/logrotator/test/clean_log.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Enable strict TypeScript mode and use explicit return types for public APIs
Prefer interfaces over type aliases for object shapes
Use readonly modifiers where appropriate
Avoid the 'any' type; use 'unknown' when the type is truly unknown
Files:
plugins/logrotator/src/app.tsplugins/logrotator/src/types.tsplugins/logrotator/src/lib/utils.tsplugins/logrotator/tsdown.config.tsplugins/logrotator/src/app/extend/application.tsplugins/logrotator/src/config/config.default.tsplugins/logrotator/src/index.tsplugins/logrotator/src/app/schedule/rotate_by_file.tsplugins/logrotator/src/boot.tsplugins/logrotator/test/index.test.tsplugins/logrotator/src/app/schedule/clean_log.tspackages/egg/src/lib/types.plugin.tsplugins/logrotator/src/lib/size_rotator.tsplugins/logrotator/src/lib/hour_rotator.tsplugins/logrotator/src/app/extend/agent.tsplugins/logrotator/vitest.config.tsplugins/logrotator/src/agent.tsplugins/logrotator/src/app/schedule/rotate_by_size.tsplugins/logrotator/src/app/schedule/rotate_by_hour.tsplugins/logrotator/src/lib/rotator.tsplugins/logrotator/src/typings/index.d.tsplugins/logrotator/src/lib/day_rotator.tsplugins/logrotator/test/logrotator.test.tsplugins/logrotator/test/clean_log.test.ts
plugins/**/tsdown.config.ts
📄 CodeRabbit inference engine (CLAUDE.md)
All plugins must use the standard tsdown configuration (entry: 'src/**/*.ts', unbundle: true, dts: true, exports.devExports: true)
Files:
plugins/logrotator/tsdown.config.ts
**/test/**/*.test.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/test/**/*.test.ts: Place test suites following Vitest discovery: /test//*.test.ts
Mirror the repository test pattern when adding new suites
**/test/**/*.test.ts: Test files should follow the naming pattern test/**/*.test.ts
Prefer importing test APIs from 'vitest' and use Node's 'assert' for assertions in Vitest-based packages
Files:
plugins/logrotator/test/index.test.tsplugins/logrotator/test/logrotator.test.tsplugins/logrotator/test/clean_log.test.ts
packages/**/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use TypeScript for all source files in packages (no JavaScript source files)
Files:
packages/egg/src/lib/types.plugin.ts
{packages,plugins}/**/vitest.config.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Each package (except specified Mocha-only ones) should include a vitest.config.ts
Files:
plugins/logrotator/vitest.config.ts
pnpm-workspace.yaml
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Define dependency versions via the catalog section in pnpm-workspace.yaml and reference them with "catalog:"
Define external dependencies centrally using the pnpm catalog in pnpm-workspace.yaml
Files:
pnpm-workspace.yaml
🧠 Learnings (23)
📚 Learning: 2025-09-27T12:34:30.406Z
Learnt from: CR
PR: eggjs/egg#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-27T12:34:30.406Z
Learning: Applies to {packages,plugins,tools}/**/tsconfig.json : All sub-project tsconfig.json files must extend the root tsconfig.json using "extends": "../../tsconfig.json" and set compilerOptions.baseUrl to "./"
Applied to files:
plugins/logrotator/tsconfig.json
📚 Learning: 2025-09-14T08:41:30.618Z
Learnt from: CR
PR: eggjs/egg#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-14T08:41:30.618Z
Learning: Applies to packages/*/tsconfig.json : All sub-project tsconfig.json files must extend from the root via "extends": "../../tsconfig.json"
Applied to files:
plugins/logrotator/tsconfig.json
📚 Learning: 2025-09-27T12:34:30.406Z
Learnt from: CR
PR: eggjs/egg#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-27T12:34:30.406Z
Learning: Applies to plugins/**/tsdown.config.ts : All plugins must use the standard tsdown configuration (entry: 'src/**/*.ts', unbundle: true, dts: true, exports.devExports: true)
Applied to files:
plugins/logrotator/tsconfig.jsonplugins/logrotator/tsdown.config.ts
📚 Learning: 2025-09-18T07:56:17.417Z
Learnt from: CR
PR: eggjs/egg#0
File: AGENTS.md:0-0
Timestamp: 2025-09-18T07:56:17.417Z
Learning: Applies to **/*.ts : Re-export types thoughtfully to keep the public API stable
Applied to files:
plugins/logrotator/src/app.tsplugins/logrotator/src/index.tspackages/egg/src/lib/types.plugin.tsplugins/logrotator/src/typings/index.d.ts
📚 Learning: 2025-09-27T12:34:30.406Z
Learnt from: CR
PR: eggjs/egg#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-27T12:34:30.406Z
Learning: Applies to plugins/**/package.json : Plugin publishConfig.exports must mirror exports but point to compiled JS in dist (e.g., '.': './dist/index.js')
Applied to files:
plugins/logrotator/package.json
📚 Learning: 2025-09-27T12:34:30.406Z
Learnt from: CR
PR: eggjs/egg#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-27T12:34:30.406Z
Learning: Applies to plugins/**/package.json : Plugins must include scripts: build (tsdown), clean (rimraf dist), typecheck (tsc --noEmit), lint (oxlint --type-aware), lint:fix, test (lint:fix && vitest), prepublishOnly (build)
Applied to files:
plugins/logrotator/package.json
📚 Learning: 2025-09-27T12:34:30.406Z
Learnt from: CR
PR: eggjs/egg#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-27T12:34:30.406Z
Learning: Applies to plugins/**/package.json : Plugin package.json must include an eggPlugin configuration field
Applied to files:
plugins/logrotator/package.json
📚 Learning: 2025-09-27T12:34:30.406Z
Learnt from: CR
PR: eggjs/egg#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-27T12:34:30.406Z
Learning: Applies to plugins/**/package.json : Plugins should publish only 'dist' via the files field
Applied to files:
plugins/logrotator/package.json
📚 Learning: 2025-09-27T12:34:30.406Z
Learnt from: CR
PR: eggjs/egg#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-27T12:34:30.406Z
Learning: Applies to plugins/**/package.json : All plugins must include egg in peerDependencies with version 'workspace:*'
Applied to files:
plugins/logrotator/package.json
📚 Learning: 2025-09-14T08:41:30.618Z
Learnt from: CR
PR: eggjs/egg#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-14T08:41:30.618Z
Learning: Applies to packages/*/tsdown.config.ts : Each package must provide a tsdown.config.ts for unbundled ESM builds
Applied to files:
plugins/logrotator/tsdown.config.ts
📚 Learning: 2025-09-27T12:34:30.406Z
Learnt from: CR
PR: eggjs/egg#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-27T12:34:30.406Z
Learning: Applies to {packages,plugins}/**/vitest.config.ts : Each package (except specified Mocha-only ones) should include a vitest.config.ts
Applied to files:
plugins/logrotator/tsdown.config.tsplugins/logrotator/test/index.test.tsplugins/logrotator/vitest.config.ts
📚 Learning: 2025-09-27T12:34:30.406Z
Learnt from: CR
PR: eggjs/egg#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-27T12:34:30.406Z
Learning: Applies to **/test/**/*.test.ts : Prefer importing test APIs from 'vitest' and use Node's 'assert' for assertions in Vitest-based packages
Applied to files:
plugins/logrotator/test/index.test.tsplugins/logrotator/vitest.config.ts
📚 Learning: 2025-09-14T08:41:30.618Z
Learnt from: CR
PR: eggjs/egg#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-14T08:41:30.618Z
Learning: Applies to packages/**/test/**/*.test.ts : Name test files as test/**/*.test.ts and run them with Vitest
Applied to files:
plugins/logrotator/test/index.test.tsplugins/logrotator/vitest.config.ts
📚 Learning: 2025-09-14T08:41:30.618Z
Learnt from: CR
PR: eggjs/egg#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-14T08:41:30.618Z
Learning: Applies to packages/**/test/**/*.test.ts : Use import { describe, it } from 'vitest' in tests
Applied to files:
plugins/logrotator/test/index.test.tsplugins/logrotator/vitest.config.ts
📚 Learning: 2025-09-18T07:56:17.417Z
Learnt from: CR
PR: eggjs/egg#0
File: AGENTS.md:0-0
Timestamp: 2025-09-18T07:56:17.417Z
Learning: Applies to **/test/**/*.test.ts : Mirror the repository test pattern when adding new suites
Applied to files:
plugins/logrotator/test/index.test.ts
📚 Learning: 2025-09-18T07:56:17.417Z
Learnt from: CR
PR: eggjs/egg#0
File: AGENTS.md:0-0
Timestamp: 2025-09-18T07:56:17.417Z
Learning: Applies to **/test/**/*.test.ts : Place test suites following Vitest discovery: **/test/**/*.test.ts
Applied to files:
plugins/logrotator/test/index.test.tsplugins/logrotator/vitest.config.ts
📚 Learning: 2025-09-14T08:41:30.618Z
Learnt from: CR
PR: eggjs/egg#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-14T08:41:30.618Z
Learning: Applies to packages/**/test/**/*.test.ts : Use Node.js built-in assert module for test assertions
Applied to files:
plugins/logrotator/test/index.test.ts
📚 Learning: 2025-09-27T12:34:30.406Z
Learnt from: CR
PR: eggjs/egg#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-27T12:34:30.406Z
Learning: Applies to **/test/**/*.test.ts : Test files should follow the naming pattern test/**/*.test.ts
Applied to files:
plugins/logrotator/vitest.config.ts
📚 Learning: 2025-09-14T08:41:30.618Z
Learnt from: CR
PR: eggjs/egg#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-14T08:41:30.618Z
Learning: Applies to pnpm-workspace.yaml : Define dependency versions via the catalog section in pnpm-workspace.yaml and reference them with "catalog:"
Applied to files:
pnpm-workspace.yaml
📚 Learning: 2025-09-27T12:34:30.406Z
Learnt from: CR
PR: eggjs/egg#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-27T12:34:30.406Z
Learning: Applies to pnpm-workspace.yaml : Define external dependencies centrally using the pnpm catalog in pnpm-workspace.yaml
Applied to files:
pnpm-workspace.yaml
📚 Learning: 2025-09-27T12:34:30.406Z
Learnt from: CR
PR: eggjs/egg#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-27T12:34:30.406Z
Learning: Applies to **/package.json : Reference external dependencies using "catalog:" and internal workspace dependencies using "workspace:*"
Applied to files:
pnpm-workspace.yaml
📚 Learning: 2025-09-27T12:34:30.406Z
Learnt from: CR
PR: eggjs/egg#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-27T12:34:30.406Z
Learning: Applies to **/package.json : Use workspace:* for internal dependencies across the monorepo
Applied to files:
pnpm-workspace.yaml
📚 Learning: 2025-09-18T07:56:17.417Z
Learnt from: CR
PR: eggjs/egg#0
File: AGENTS.md:0-0
Timestamp: 2025-09-18T07:56:17.417Z
Learning: For features affecting HTTP or process orchestration, add regression tests that exercise both the CommonJS and TypeScript example apps
Applied to files:
plugins/logrotator/test/logrotator.test.ts
🧬 Code graph analysis (29)
plugins/logrotator/test/fixtures/logger-reload/app/router.js (1)
plugins/logrotator/src/app/schedule/rotate_by_file.ts (1)
app(4-18)
plugins/logrotator/src/app.ts (1)
plugins/logrotator/src/boot.ts (1)
Boot(3-13)
plugins/logrotator/src/types.ts (2)
packages/egg/src/lib/types.ts (1)
EggAppConfig(123-297)plugins/logrotator/src/config/config.default.ts (1)
LogrotatorConfig(5-60)
plugins/logrotator/src/lib/utils.ts (1)
packages/egg/src/lib/egg.ts (1)
loggers(405-410)
plugins/logrotator/test/fixtures/logrotator-app-hour-custom_hourdelimiter/config/config.default.js (3)
plugins/logrotator/test/fixtures/logrotator-app-hour-gzip/config/config.default.js (2)
path(3-3)exports(6-18)plugins/logrotator/test/fixtures/logrotator-app-hour/config/config.default.js (2)
path(3-3)exports(6-17)plugins/logrotator/test/fixtures/logrotator-app/config/config.default.js (2)
path(3-3)exports(6-37)
plugins/logrotator/test/fixtures/logrotator-app-day-gzip/config/config.default.js (2)
plugins/logrotator/test/fixtures/logrotator-app-hour-gzip/config/config.default.js (1)
exports(6-18)plugins/logrotator/test/fixtures/logrotator-app-size-gzip/config/config.default.js (1)
exports(6-20)
plugins/logrotator/test/fixtures/logrotator-app-size/config/config.default.js (3)
plugins/logrotator/test/fixtures/logrotator-app-size-gzip/config/config.default.js (2)
path(3-3)exports(6-20)plugins/logrotator/test/fixtures/logrotator-app/config/config.default.js (2)
path(3-3)exports(6-37)plugins/logrotator/test/fixtures/logrotator-json-format/config/config.default.js (2)
path(3-3)exports(6-33)
plugins/logrotator/test/fixtures/logrotator-json-format/config/config.default.js (1)
plugins/logrotator/test/fixtures/logrotator-app/config/config.default.js (2)
path(3-3)exports(6-37)
plugins/logrotator/__snapshots__/clean_log.test.ts.js (3)
plugins/logrotator/test/fixtures/logrotator-app-day-gzip/config/config.default.js (1)
exports(4-8)plugins/logrotator/test/fixtures/logrotator-app-hour-gzip/config/config.default.js (1)
exports(6-18)plugins/logrotator/test/fixtures/logrotator-app-size-gzip/config/config.default.js (1)
exports(6-20)
plugins/logrotator/tsdown.config.ts (1)
packages/egg/src/lib/types.ts (1)
defineConfig(105-107)
plugins/logrotator/test/fixtures/noexist-rotator-dir/config/config.default.js (1)
plugins/logrotator/test/fixtures/logrotator-json-format/config/config.default.js (1)
exports(6-33)
plugins/logrotator/src/config/config.default.ts (1)
plugins/logrotator/src/types.ts (1)
LogrotatorConfig(4-4)
plugins/logrotator/test/fixtures/clean-log/config/config.default.js (2)
plugins/logrotator/test/fixtures/logger-reload/app/router.js (1)
path(1-1)scripts/version.js (1)
__dirname(13-13)
plugins/logrotator/src/app/schedule/rotate_by_file.ts (4)
plugins/logrotator/src/app/schedule/clean_log.ts (1)
app(11-37)plugins/logrotator/src/app/schedule/rotate_by_hour.ts (1)
app(4-18)plugins/logrotator/src/app/schedule/rotate_by_size.ts (1)
app(4-18)plugins/logrotator/src/lib/day_rotator.ts (1)
DayRotator(15-92)
plugins/logrotator/src/boot.ts (4)
plugins/logrotator/src/app/schedule/clean_log.ts (1)
app(11-37)plugins/logrotator/src/app/schedule/rotate_by_file.ts (1)
app(4-18)plugins/logrotator/src/app/schedule/rotate_by_hour.ts (1)
app(4-18)plugins/logrotator/src/app/schedule/rotate_by_size.ts (1)
app(4-18)
plugins/logrotator/src/app/schedule/clean_log.ts (2)
packages/core/src/egg.ts (1)
EggCore(95-543)plugins/logrotator/src/lib/utils.ts (1)
walkLoggerFile(10-21)
plugins/logrotator/src/lib/size_rotator.ts (1)
plugins/logrotator/src/lib/rotator.ts (1)
RotateFile(17-20)
plugins/logrotator/src/lib/hour_rotator.ts (1)
plugins/logrotator/src/lib/rotator.ts (1)
RotateFile(17-20)
plugins/logrotator/src/agent.ts (1)
plugins/logrotator/src/boot.ts (1)
Boot(3-13)
plugins/logrotator/src/app/schedule/rotate_by_size.ts (3)
plugins/logrotator/src/app/schedule/rotate_by_file.ts (1)
app(4-18)plugins/logrotator/src/app/schedule/rotate_by_hour.ts (1)
app(4-18)plugins/logrotator/src/lib/size_rotator.ts (1)
SizeRotator(14-72)
plugins/logrotator/src/app/schedule/rotate_by_hour.ts (5)
plugins/logrotator/src/app/schedule/clean_log.ts (1)
app(11-37)plugins/logrotator/src/app/schedule/rotate_by_file.ts (1)
app(4-18)plugins/logrotator/src/app/schedule/rotate_by_size.ts (1)
app(4-18)packages/core/src/egg.ts (1)
EggCore(95-543)plugins/logrotator/src/lib/hour_rotator.ts (1)
HourRotator(13-46)
plugins/logrotator/src/lib/rotator.ts (1)
packages/core/src/egg.ts (1)
EggCore(95-543)
plugins/logrotator/test/fixtures/logrotator-app-hour-gzip/config/config.default.js (3)
plugins/logrotator/test/fixtures/logrotator-app-hour-custom_hourdelimiter/config/config.default.js (2)
path(3-3)exports(6-18)plugins/logrotator/test/fixtures/logrotator-app-hour/config/config.default.js (2)
path(3-3)exports(6-17)plugins/logrotator/test/fixtures/logrotator-app-size-gzip/config/config.default.js (2)
path(3-3)exports(6-20)
plugins/logrotator/src/lib/day_rotator.ts (3)
plugins/logrotator/src/lib/rotator.ts (2)
RotatorOptions(13-15)RotateFile(17-20)packages/egg/src/lib/egg.ts (1)
loggers(405-410)plugins/logrotator/src/lib/utils.ts (1)
walkLoggerFile(10-21)
plugins/logrotator/test/fixtures/logrotator-app-hour/config/config.default.js (3)
plugins/logrotator/test/fixtures/logrotator-app-hour-custom_hourdelimiter/config/config.default.js (2)
path(3-3)exports(6-18)plugins/logrotator/test/fixtures/logrotator-app-hour-gzip/config/config.default.js (2)
path(3-3)exports(6-18)plugins/logrotator/test/fixtures/logrotator-app/config/config.default.js (2)
path(3-3)exports(6-37)
plugins/logrotator/test/logrotator.test.ts (3)
plugins/logrotator/src/app/schedule/rotate_by_file.ts (1)
app(4-18)plugins/logrotator/src/app/schedule/rotate_by_hour.ts (1)
app(4-18)plugins/logrotator/src/app/schedule/rotate_by_size.ts (1)
app(4-18)
plugins/logrotator/test/fixtures/logrotator-app-size-gzip/config/config.default.js (2)
plugins/logrotator/test/fixtures/logrotator-app-hour-gzip/config/config.default.js (2)
path(3-3)exports(6-18)plugins/logrotator/test/fixtures/logrotator-app-size/config/config.default.js (2)
path(3-3)exports(6-19)
plugins/logrotator/test/fixtures/logrotator-app/config/config.default.js (1)
plugins/logrotator/test/fixtures/logrotator-json-format/config/config.default.js (2)
path(3-3)exports(6-33)
plugins/logrotator/test/clean_log.test.ts (1)
plugins/logrotator/src/app/schedule/clean_log.ts (1)
app(11-37)
🪛 markdownlint-cli2 (0.18.1)
plugins/logrotator/README.md
15-15: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
15-15: Bare URL used
(MD034, no-bare-urls)
32-32: Bare URL used
(MD034, no-bare-urls)
34-34: Bare URL used
(MD034, no-bare-urls)
85-85: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
88-88: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
89-89: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
91-91: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
94-94: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
95-95: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
97-97: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
100-100: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
101-101: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
103-103: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
106-106: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
107-107: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
109-109: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
112-112: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
113-113: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
114-114: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
116-116: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
119-119: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
121-121: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
plugins/logrotator/README.zh-CN.md
103-103: Link text should be descriptive
(MD059, descriptive-link-text)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test (macos-latest, 24, packages)
- GitHub Check: Cloudflare Pages: egg-v3
| "scripts": { | ||
| "build": "tsdown", | ||
| "typecheck": "tsc --noEmit", | ||
| "lint": "oxlint --type-aware", | ||
| "test": "vitest run", | ||
| "prepublishOnly": "npm run build" | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add missing scripts and align test with repo standards.
Required plugin scripts are incomplete; add clean and lint:fix, and run lint:fix before tests.
As per coding guidelines
"scripts": {
"build": "tsdown",
+ "clean": "rimraf dist",
"typecheck": "tsc --noEmit",
"lint": "oxlint --type-aware",
- "test": "vitest run",
- "prepublishOnly": "npm run build"
+ "lint:fix": "oxlint --type-aware --fix",
+ "test": "npm run lint:fix && vitest",
+ "prepublishOnly": "npm run build"
},📝 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.
| "scripts": { | |
| "build": "tsdown", | |
| "typecheck": "tsc --noEmit", | |
| "lint": "oxlint --type-aware", | |
| "test": "vitest run", | |
| "prepublishOnly": "npm run build" | |
| }, | |
| "scripts": { | |
| "build": "tsdown", | |
| "clean": "rimraf dist", | |
| "typecheck": "tsc --noEmit", | |
| "lint": "oxlint --type-aware", | |
| "lint:fix": "oxlint --type-aware --fix", | |
| "test": "npm run lint:fix && vitest", | |
| "prepublishOnly": "npm run build" | |
| }, |
🤖 Prompt for AI Agents
In plugins/logrotator/package.json around lines 67 to 73, the scripts section is
missing required entries and the test script doesn't run lint fixes first; add a
"clean" script to remove build artifacts (e.g., dist or lib), add a "lint:fix"
script to run the linter in fix mode, and modify the "test" script to run
"lint:fix" before executing the test runner so lint auto-fixes run as part of
the test pipeline.
| function getRotator(app) { | ||
| class CustomRotator extends app.LogRotator { | ||
| // return map that contains a pair of srcPath and targetPath | ||
| // LogRotator will rename ksrcPath to targetPath |
There was a problem hiding this comment.
Fix typo in comment.
There's a typo in the comment: "LogRotator will rename ksrcPath to targetPath" should be "LogRotator will rename srcPath to targetPath".
- // LogRotator will rename ksrcPath to targetPath
+ // LogRotator will rename srcPath to targetPath📝 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.
| // LogRotator will rename ksrcPath to targetPath | |
| // LogRotator will rename srcPath to targetPath |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
94-94: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🤖 Prompt for AI Agents
In plugins/logrotator/README.md around line 94, there's a typo in the inline
comment: it says "LogRotator will rename ksrcPath to targetPath" but should read
"LogRotator will rename srcPath to targetPath"; update the comment text to
remove the stray 'k' so it correctly references srcPath.
| 可以配置 `filesRotateBySize` 文件列表按小时切割，每小时0分开始切割，格式为 `.log.YYYY-MM-DD-HH`。 | ||
|
|
There was a problem hiding this comment.
Correct the hourly rotation config key
The hourly rotation section should reference filesRotateByHour, not filesRotateBySize, otherwise readers will configure the wrong field.
-可以配置 `filesRotateBySize` 文件列表按小时切割，每小时0分开始切割，格式为 `.log.YYYY-MM-DD-HH`。
+可以配置 `filesRotateByHour` 文件列表按小时切割，每小时0分开始切割，格式为 `.log.YYYY-MM-DD-HH`。📝 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.
| 可以配置 `filesRotateBySize` 文件列表按小时切割，每小时0分开始切割，格式为 `.log.YYYY-MM-DD-HH`。 | |
| 可以配置 `filesRotateByHour` 文件列表按小时切割，每小时0分开始切割，格式为 `.log.YYYY-MM-DD-HH`。 |
🤖 Prompt for AI Agents
In plugins/logrotator/README.zh-CN.md around lines 55 to 56, the hourly rotation
section mistakenly references the config key `filesRotateBySize`; update the
text to reference `filesRotateByHour` so the documentation matches the actual
configuration key and prevents users from configuring the wrong field.
| const names = files.filter(file => { | ||
| const name = path.extname(file).slice(1); | ||
| if (!/^\d{4}-\d{2}-\d{2}/.test(name)) { | ||
| return false; | ||
| } | ||
| const date = moment(name, 'YYYY-MM-DD').startOf('date'); | ||
| if (!date.isValid()) { | ||
| return false; | ||
| } | ||
| return date.isBefore(expiredDate); | ||
| }); |
There was a problem hiding this comment.
Fix cleanup to include .gz files and strictly parse the date token
Rotated files with gzip (e.g., foo.log.2025-09-20.gz) are never deleted because path.extname(file) returns .gz. Also add strict parsing and support optional hourly suffix (-HH).
Apply this diff:
- const names = files.filter(file => {
- const name = path.extname(file).slice(1);
- if (!/^\d{4}-\d{2}-\d{2}/.test(name)) {
- return false;
- }
- const date = moment(name, 'YYYY-MM-DD').startOf('date');
- if (!date.isValid()) {
- return false;
- }
- return date.isBefore(expiredDate);
- });
+ const names = files.filter(file => {
+ // Match suffix ".YYYY-MM-DD" or ".YYYY-MM-DD-HH", optionally followed by ".gz"
+ const m = file.match(/\.(\d{4}-\d{2}-\d{2})(?:-\d{2})?(?:\.gz)?$/);
+ if (!m) return false;
+ const date = moment(m[1], 'YYYY-MM-DD', true).startOf('date');
+ return date.isValid() && date.isBefore(expiredDate);
+ });📝 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.
| const names = files.filter(file => { | |
| const name = path.extname(file).slice(1); | |
| if (!/^\d{4}-\d{2}-\d{2}/.test(name)) { | |
| return false; | |
| } | |
| const date = moment(name, 'YYYY-MM-DD').startOf('date'); | |
| if (!date.isValid()) { | |
| return false; | |
| } | |
| return date.isBefore(expiredDate); | |
| }); | |
| const names = files.filter(file => { | |
| // Match suffix ".YYYY-MM-DD" or ".YYYY-MM-DD-HH", optionally followed by ".gz" | |
| const m = file.match(/\.(\d{4}-\d{2}-\d{2})(?:-\d{2})?(?:\.gz)?$/); | |
| if (!m) return false; | |
| const date = moment(m[1], 'YYYY-MM-DD', true).startOf('date'); | |
| return date.isValid() && date.isBefore(expiredDate); | |
| }); |
🤖 Prompt for AI Agents
In plugins/logrotator/src/app/schedule/clean_log.ts around lines 50 to 60, the
current code uses path.extname(file) and loose parsing so gzip rotated files
(e.g., foo.log.2025-09-20.gz) are skipped and dates are parsed non-strictly;
update the filter to extract the date token from the filename regardless of a
.gz extension (e.g., strip trailing .gz then take the last dot-separated token
or match with a regex) and test it against a strict regex for YYYY-MM-DD with an
optional -HH suffix (e.g., /^\d{4}-\d{2}-\d{2}(?:-\d{2})?$/), then parse with
moment using strict mode and the appropriate format(s) (e.g., 'YYYY-MM-DD' and
'YYYY-MM-DD-HH') and only return true when the parsed moment is valid and before
expiredDate.
| return { | ||
| schedule: { | ||
| type: 'worker', // only one worker run this task | ||
| cron: '1 0 0 * * *', // run every day at 00:00 |
There was a problem hiding this comment.
Fix invalid 6-field cron; use 5-field to match other schedules
Egg schedules here use 5-field cron (see clean_log.ts). This 6-field value is likely ignored/misparsed. Run at 00:01 with a 5-field cron for consistency with hourly job.
Apply this diff:
- cron: '1 0 0 * * *', // run every day at 00:00
+ cron: '1 0 * * *', // run every day at 00:01🤖 Prompt for AI Agents
In plugins/logrotator/src/app/schedule/rotate_by_file.ts around line 10, the
schedule uses a 6-field cron string which is invalid for the project's Egg
schedules; replace the 6-field cron "1 0 0 * * *" with a 5-field cron "1 0 * *
*" so the job runs daily at 00:01 and matches the format used by other schedules
like clean_log.ts.
| }) | ||
| ); | ||
| }); | ||
| afterEach(() => app.close()); |
There was a problem hiding this comment.
Await app.close() in teardown to avoid resource leaks/flaky tests
app.close() returns a Promise; not awaiting it can leave servers/file handles open between tests.
Apply this diff:
-afterEach(() => app.close());
+afterEach(async () => {
+ await app.close();
+});📝 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.
| afterEach(() => app.close()); | |
| afterEach(async () => { | |
| await app.close(); | |
| }); |
🤖 Prompt for AI Agents
In plugins/logrotator/test/clean_log.test.ts around line 42, the test teardown
calls app.close() without awaiting its Promise which can leak resources and
cause flaky tests; change the afterEach to await app.close() by making the
callback async and calling await app.close(), or return the Promise from
afterEach so the test runner waits for shutdown.
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## next #5559 +/- ##
==========================================
+ Coverage 78.78% 79.24% +0.46%
==========================================
Files 173 183 +10
Lines 5165 5325 +160
Branches 789 801 +12
==========================================
+ Hits 4069 4220 +151
- Misses 1025 1033 +8
- Partials 71 72 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/egg/src/lib/core/messenger/ipc.ts (1)
164-167: Prevent listener leaks in all-in-one-process and worker_threads.close() only removes 'message'. Also remove the custom channel and workerThreads listener.
close() { - process.removeListener('message', this.onMessage); + if (this.egg.options.mode === 'all-in-one-process') { + (process as unknown as NodeJS.EventEmitter).removeListener('sendmessage-to-self', this.onMessage); + } else { + process.removeListener('message', this.onMessage); + } + if (!workerThreads.isMainThread) { + workerThreads.parentPort?.off('message', this.onMessage); + } this.removeAllListeners(); }
🧹 Nitpick comments (20)
plugins/mock/src/lib/app.ts (1)
185-188: Make the override explicit in logs (keep behavior).Silently overriding createOptions.mode can surprise callers. Log the override and mark the literal as const to satisfy TS inference.
export function createApp(createOptions?: MockOptions): ApplicationUnittest { - const options = formatOptions({ - ...createOptions, - mode: 'all-in-one-process', - }); + if (createOptions?.mode && createOptions.mode !== 'all-in-one-process') { + debug('[createApp] overriding options.mode from %s to all-in-one-process', createOptions.mode); + } + const options = formatOptions({ + ...createOptions, + mode: 'all-in-one-process' as const, + });plugins/mock/src/lib/types.ts (1)
2-6: Types LGTM; consider centralizing the mode union.To avoid drift with packages/egg, define a shared AppMode type (or reuse EggApplicationCoreOptions['mode']) and reference it here.
+export type AppMode = 'cluster' | 'single' | 'all-in-one-process'; export interface MockOptions { /** * The mode of the application */ - mode?: 'all-in-one-process' | 'cluster' | 'single'; + mode?: AppMode; }As per coding guidelines.
packages/egg/src/lib/egg.ts (1)
144-151: Update constructor JSDoc to include the new mode.Docs still say “cluster / single”. Add “all-in-one-process” for accuracy.
- * - {String} [mode] - process mode, can be cluster / single, default is `cluster` + * - {String} [mode] - process mode: 'cluster' | 'single' | 'all-in-one-process' (test/mock); default `cluster`packages/egg/src/lib/core/messenger/ipc.ts (1)
124-131: Minor: tighten 'to' typing for clarity.Not required, but restricting to 'app' | 'agent' improves DX and catch typos.
- send(action: string, data: unknown | undefined, to?: string): Messenger { + send(action: string, data: unknown | undefined, to?: 'app' | 'agent'): Messenger {plugins/logrotator/test/index.test.ts (1)
1-9: Prefer Node's assert with Vitest APIsPer repo guidance, keep describe/it from Vitest but use Node's assert for assertions.
As per coding guidelines
-import { describe, it, expect } from 'vitest'; +import { describe, it } from 'vitest'; +import assert from 'node:assert/strict'; import { LogRotator } from '../src/index.ts'; describe('test/index.test.ts', () => { it('should export LogRotator', () => { - expect(LogRotator).toBeDefined(); + assert.ok(LogRotator); }); });plugins/logrotator/src/app/schedule/clean_log.ts (2)
21-24: Normalize logger file paths to absolute before deriving directoriesIf any logger uses a relative file path, the cleanup may look in the wrong directory.
- for (const file of loggerFiles) { - const logDir = path.dirname(file); - logDirs.add(logDir); - } + for (const file of loggerFiles) { + let resolved = file; + if (!path.isAbsolute(resolved)) { + resolved = path.join(app.config.logger.dir, resolved); + } + const logDir = path.dirname(resolved); + logDirs.add(logDir); + }
65-76: Avoid mutating Error.message; log context insteadMutation can obscure stack traces. Prefer structured logging with context.
- } catch (e) { - const err = e as Error; - err.message = `[@eggjs/logrotator] remove logFile ${logFile} error, ${err.message}`; - logger.error(err); - } + } catch (e) { + logger.error(e, '[@eggjs/logrotator] remove logFile %s error', logFile); + }plugins/logrotator/test/rotate_by_size.test.ts (4)
27-33: Avoid flaky waits; rely on runSchedule’s awaitability
app.runSchedule()awaits the task; extrascheduler.wait(...)is unnecessary and can introduce flakiness. Assert immediately afterrunSchedule.it('should rotate by size', async () => { fs.writeFileSync(mockfile, 'mock log text'); await app.runSchedule(schedule); - await scheduler.wait(100); expect(fs.existsSync(`${mockfile}.1`)).toBe(true); }); @@ // rotate first await app.runSchedule(schedule); - await scheduler.wait(100); @@ await app.runSchedule(schedule); - - await scheduler.wait(100); + // no extra wait needed @@ await app.runSchedule(schedule); - await scheduler.wait(100); + // no extra wait neededAlso applies to: 37-39, 42-45, 48-50
15-23: Combine hooks to ensure ordering clarityVitest awaits hooks in order, but combining app creation/ready and path setup reduces cognitive load and avoids accidental reordering.
-beforeEach(() => { - app = mm.app({ - baseDir: getFixtures('logrotator-app-size'), - }); - return app.ready(); -}); -beforeEach(() => { - mockfile = path.join(app.config.logger.dir, 'egg-web.log'); -}); +beforeEach(async () => { + app = mm.app({ baseDir: getFixtures('logrotator-app-size') }); + await app.ready(); + mockfile = path.join(app.config.logger.dir, 'egg-web.log'); +});
34-56: Isolate filesystem state between testsLeftover rotated files from one test can affect the next (especially with maxFiles assertions). Clean the log dir (or specific rotated files) before each test to keep tests deterministic.
+beforeEach(() => { + const dir = path.dirname(path.join(app.config.logger.dir, 'placeholder')); + try { + for (const suffix of ['.1', '.2', '.3', '.4']) { + fs.rmSync(path.join(dir, `egg-web.log${suffix}`), { force: true }); + } + } catch {} +});Please run the suite twice locally to confirm it stays green across re-runs.
Also applies to: 71-79
58-69: Either enable or remove the skipped error-path testIf kept, assert the exact error message format to guard logs; if not critical, remove to reduce maintenance.
plugins/logrotator/test/rotate_by_day.test.ts (3)
48-77: Remove fixed sleeps; assert after the awaited schedule
runScheduleawaits the task; the extrascheduler.wait(1000)is not needed and slows CI.await app.runSchedule(schedule); - await scheduler.wait(1000); const content = fs.readFileSync(path.join(logDir, `egg-web.log`), 'utf8');Also applies to: 71-74
150-156: Prototype pollution in tests: restore after useExtending
Map.prototypeis fine for the test, but please delete the property after the run to avoid leaking to other suites.(Map.prototype as any).test = () => { console.log('test Map extend'); }; await app.runSchedule(schedule); +// cleanup +// oxlint-disable-next-line typescript/no-explicit-any +delete (Map.prototype as any).test;
26-27: Assert with Node’s assert for Vitest-based packages (repo convention)Coding guidelines prefer Node’s assert for assertions in Vitest tests. Consider switching from
expecttoassertfor consistency.As per coding guidelines
Also applies to: 51-57, 60-69
plugins/logrotator/src/lib/rotator.ts (2)
52-58: Consider single broadcast or dedupe in all-in-one-process modeOn some setups, sending both
sendToAppandsendToAgentmay deliver two reloads to the same target. If messenger now supports all‑in‑one mode, guard or dedupe.
8-10: Verifyutility.existssemantics and perfEnsure
existsis non-throwing and efficient; if it swallows EACCES as false, that’s acceptable here. Otherwise, preferfs.stat/fs.accesswith explicit error handling.plugins/logrotator/src/lib/day_rotator.ts (4)
25-31: Add explicit return type for public API
getRotateFilesis part of the public contract; declare its return type for stricter TS and stable API surface.As per coding guidelines
- async getRotateFiles() { + async getRotateFiles(): Promise<Map<string, RotateFile>> {Also applies to: 70-71
73-91: Limit surface: make_setFileprivateThis helper isn’t intended for external use; mark it
privateto keep the public API minimal.- _setFile(srcPath: string, files: Map<string, RotateFile>) { + private _setFile(srcPath: string, files: Map<string, RotateFile>) {
84-90: Date shift comment vs implementationComment says “allow 2 minutes deviation” while code subtracts 23h58m (which is correct). Adjust comment to avoid confusion.
- // allow 2 minutes deviation + // Shift by 23h58m to ensure date suffix points to previous day around midnight rollovers
8-9: ESM import extensions: prefer.js(or extensionless) in TS sourceImporting
./rotator.tscan break in ESM after emit. Use./rotator.jsin TS (TypeScript resolves it) or rely on your build tool to rewrite. Align with monorepo standard.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
plugins/logrotator/test/__snapshots__/clean_log.test.ts.snapis excluded by!**/*.snappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (27)
packages/egg/src/lib/core/messenger/ipc.ts(3 hunks)packages/egg/src/lib/egg.ts(1 hunks)packages/egg/test/lib/plugins/logrotator.test.ts(0 hunks)plugins/logrotator/package.json(1 hunks)plugins/logrotator/src/agent.ts(1 hunks)plugins/logrotator/src/app.ts(1 hunks)plugins/logrotator/src/app/extend/agent.ts(1 hunks)plugins/logrotator/src/app/extend/application.ts(1 hunks)plugins/logrotator/src/app/schedule/clean_log.ts(1 hunks)plugins/logrotator/src/app/schedule/rotate_by_file.ts(1 hunks)plugins/logrotator/src/app/schedule/rotate_by_hour.ts(1 hunks)plugins/logrotator/src/app/schedule/rotate_by_size.ts(1 hunks)plugins/logrotator/src/boot.ts(1 hunks)plugins/logrotator/src/config/config.default.ts(1 hunks)plugins/logrotator/src/index.ts(1 hunks)plugins/logrotator/src/lib/day_rotator.ts(1 hunks)plugins/logrotator/src/lib/hour_rotator.ts(1 hunks)plugins/logrotator/src/lib/rotator.ts(1 hunks)plugins/logrotator/src/lib/size_rotator.ts(1 hunks)plugins/logrotator/test/clean_log.test.ts(1 hunks)plugins/logrotator/test/index.test.ts(1 hunks)plugins/logrotator/test/logrotator.test.ts(1 hunks)plugins/logrotator/test/rotate_by_day.test.ts(1 hunks)plugins/logrotator/test/rotate_by_size.test.ts(1 hunks)plugins/logrotator/test/utils.ts(1 hunks)plugins/mock/src/lib/app.ts(1 hunks)plugins/mock/src/lib/types.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/egg/test/lib/plugins/logrotator.test.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- plugins/logrotator/src/index.ts
- plugins/logrotator/src/app/extend/agent.ts
- plugins/logrotator/test/clean_log.test.ts
- plugins/logrotator/src/boot.ts
- plugins/logrotator/src/lib/size_rotator.ts
- plugins/logrotator/src/app/schedule/rotate_by_hour.ts
- plugins/logrotator/package.json
- plugins/logrotator/src/app/schedule/rotate_by_size.ts
- plugins/logrotator/src/lib/hour_rotator.ts
- plugins/logrotator/src/app.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: Prefer TypeScript and ESM: write sources and exports in .ts (ESM-first) rather than CommonJS
Use two-space indentation, trailing commas, and semicolons (Prettier/oxlint defaults)
Name files in lowercase with hyphens (e.g., loader-context.ts)
Name classes in PascalCase
Name functions and variables in camelCase
Re-export types thoughtfully to keep the public API stable
Files:
plugins/mock/src/lib/types.tspackages/egg/src/lib/egg.tsplugins/logrotator/test/index.test.tsplugins/logrotator/test/utils.tsplugins/logrotator/src/agent.tspackages/egg/src/lib/core/messenger/ipc.tsplugins/logrotator/src/app/extend/application.tsplugins/mock/src/lib/app.tsplugins/logrotator/src/app/schedule/rotate_by_file.tsplugins/logrotator/test/rotate_by_day.test.tsplugins/logrotator/test/logrotator.test.tsplugins/logrotator/src/config/config.default.tsplugins/logrotator/src/lib/day_rotator.tsplugins/logrotator/src/app/schedule/clean_log.tsplugins/logrotator/src/lib/rotator.tsplugins/logrotator/test/rotate_by_size.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Enable strict TypeScript mode and use explicit return types for public APIs
Prefer interfaces over type aliases for object shapes
Use readonly modifiers where appropriate
Avoid the 'any' type; use 'unknown' when the type is truly unknown
Files:
plugins/mock/src/lib/types.tspackages/egg/src/lib/egg.tsplugins/logrotator/test/index.test.tsplugins/logrotator/test/utils.tsplugins/logrotator/src/agent.tspackages/egg/src/lib/core/messenger/ipc.tsplugins/logrotator/src/app/extend/application.tsplugins/mock/src/lib/app.tsplugins/logrotator/src/app/schedule/rotate_by_file.tsplugins/logrotator/test/rotate_by_day.test.tsplugins/logrotator/test/logrotator.test.tsplugins/logrotator/src/config/config.default.tsplugins/logrotator/src/lib/day_rotator.tsplugins/logrotator/src/app/schedule/clean_log.tsplugins/logrotator/src/lib/rotator.tsplugins/logrotator/test/rotate_by_size.test.ts
packages/**/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use TypeScript for all source files in packages (no JavaScript source files)
Files:
packages/egg/src/lib/egg.tspackages/egg/src/lib/core/messenger/ipc.ts
**/test/**/*.test.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/test/**/*.test.ts: Place test suites following Vitest discovery: /test//*.test.ts
Mirror the repository test pattern when adding new suites
**/test/**/*.test.ts: Test files should follow the naming pattern test/**/*.test.ts
Prefer importing test APIs from 'vitest' and use Node's 'assert' for assertions in Vitest-based packages
Files:
plugins/logrotator/test/index.test.tsplugins/logrotator/test/rotate_by_day.test.tsplugins/logrotator/test/logrotator.test.tsplugins/logrotator/test/rotate_by_size.test.ts
packages/egg/src/lib/core/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Implement core components (httpclient, logger, messenger) under packages/egg/src/lib/core/
Files:
packages/egg/src/lib/core/messenger/ipc.ts
🧠 Learnings (9)
📚 Learning: 2025-09-27T12:34:30.406Z
Learnt from: CR
PR: eggjs/egg#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-27T12:34:30.406Z
Learning: Applies to **/test/**/*.test.ts : Prefer importing test APIs from 'vitest' and use Node's 'assert' for assertions in Vitest-based packages
Applied to files:
plugins/logrotator/test/index.test.ts
📚 Learning: 2025-09-14T08:41:30.618Z
Learnt from: CR
PR: eggjs/egg#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-14T08:41:30.618Z
Learning: Applies to packages/**/test/**/*.test.ts : Name test files as test/**/*.test.ts and run them with Vitest
Applied to files:
plugins/logrotator/test/index.test.ts
📚 Learning: 2025-09-27T12:34:30.406Z
Learnt from: CR
PR: eggjs/egg#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-27T12:34:30.406Z
Learning: Applies to {packages,plugins}/**/vitest.config.ts : Each package (except specified Mocha-only ones) should include a vitest.config.ts
Applied to files:
plugins/logrotator/test/index.test.ts
📚 Learning: 2025-09-14T08:41:30.618Z
Learnt from: CR
PR: eggjs/egg#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-14T08:41:30.618Z
Learning: Applies to packages/**/test/**/*.test.ts : Use import { describe, it } from 'vitest' in tests
Applied to files:
plugins/logrotator/test/index.test.ts
📚 Learning: 2025-09-14T08:41:30.618Z
Learnt from: CR
PR: eggjs/egg#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-14T08:41:30.618Z
Learning: Applies to packages/**/test/**/*.test.ts : Use Node.js built-in assert module for test assertions
Applied to files:
plugins/logrotator/test/index.test.ts
📚 Learning: 2025-09-18T07:56:17.417Z
Learnt from: CR
PR: eggjs/egg#0
File: AGENTS.md:0-0
Timestamp: 2025-09-18T07:56:17.417Z
Learning: Applies to **/test/**/*.test.ts : Place test suites following Vitest discovery: **/test/**/*.test.ts
Applied to files:
plugins/logrotator/test/index.test.ts
📚 Learning: 2025-09-18T07:56:17.417Z
Learnt from: CR
PR: eggjs/egg#0
File: AGENTS.md:0-0
Timestamp: 2025-09-18T07:56:17.417Z
Learning: Applies to **/*.ts : Re-export types thoughtfully to keep the public API stable
Applied to files:
plugins/logrotator/src/app/extend/application.ts
📚 Learning: 2025-09-18T07:56:17.417Z
Learnt from: CR
PR: eggjs/egg#0
File: AGENTS.md:0-0
Timestamp: 2025-09-18T07:56:17.417Z
Learning: For features affecting HTTP or process orchestration, add regression tests that exercise both the CommonJS and TypeScript example apps
Applied to files:
plugins/logrotator/test/logrotator.test.ts
📚 Learning: 2025-09-27T12:34:30.406Z
Learnt from: CR
PR: eggjs/egg#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-27T12:34:30.406Z
Learning: Applies to packages/egg/src/config/{plugin.ts,config.default.ts} : Built-in plugin and default framework configurations must reside at packages/egg/src/config/plugin.ts and config.default.ts
Applied to files:
plugins/logrotator/src/config/config.default.ts
🧬 Code graph analysis (8)
plugins/logrotator/src/agent.ts (1)
plugins/logrotator/src/boot.ts (1)
Boot(3-16)
plugins/logrotator/src/app/schedule/rotate_by_file.ts (3)
plugins/logrotator/src/app/schedule/clean_log.ts (1)
app(11-37)plugins/logrotator/src/app/schedule/rotate_by_hour.ts (1)
app(5-19)plugins/logrotator/src/app/schedule/rotate_by_size.ts (1)
app(5-19)
plugins/logrotator/test/rotate_by_day.test.ts (4)
plugins/logrotator/src/app/schedule/rotate_by_file.ts (1)
app(5-19)plugins/logrotator/src/app/schedule/rotate_by_hour.ts (1)
app(5-19)plugins/logrotator/src/app/schedule/rotate_by_size.ts (1)
app(5-19)plugins/logrotator/test/utils.ts (1)
getFixtures(3-5)
plugins/logrotator/test/logrotator.test.ts (4)
plugins/logrotator/src/app/schedule/rotate_by_file.ts (1)
app(5-19)plugins/logrotator/src/app/schedule/rotate_by_hour.ts (1)
app(5-19)plugins/logrotator/src/app/schedule/rotate_by_size.ts (1)
app(5-19)plugins/logrotator/test/utils.ts (1)
getFixtures(3-5)
plugins/logrotator/src/config/config.default.ts (1)
packages/egg/src/lib/types.ts (1)
EggAppConfig(123-297)
plugins/logrotator/src/lib/day_rotator.ts (3)
plugins/logrotator/src/lib/rotator.ts (2)
RotatorOptions(13-15)RotateFile(17-20)packages/egg/src/lib/egg.ts (1)
loggers(405-410)plugins/logrotator/src/lib/utils.ts (1)
walkLoggerFile(10-21)
plugins/logrotator/src/app/schedule/clean_log.ts (1)
plugins/logrotator/src/lib/utils.ts (1)
walkLoggerFile(10-21)
plugins/logrotator/test/rotate_by_size.test.ts (2)
plugins/logrotator/src/app/schedule/rotate_by_size.ts (1)
app(5-19)plugins/logrotator/test/utils.ts (1)
getFixtures(3-5)
🪛 GitHub Check: Test egg-bin (windows-latest, 22)
packages/egg/src/lib/core/messenger/ipc.ts
[failure] 137-137:
No overload matches this call.
🪛 GitHub Check: Test egg-bin (macos-latest, 22)
packages/egg/src/lib/core/messenger/ipc.ts
[failure] 137-137:
No overload matches this call.
🪛 GitHub Check: Test egg-bin (ubuntu-latest, 22)
packages/egg/src/lib/core/messenger/ipc.ts
[failure] 137-137:
No overload matches this call.
🪛 GitHub Check: Test egg-bin (macos-latest, 24)
packages/egg/src/lib/core/messenger/ipc.ts
[failure] 137-137:
No overload matches this call.
🪛 GitHub Check: Test egg-bin (ubuntu-latest, 24)
packages/egg/src/lib/core/messenger/ipc.ts
[failure] 137-137:
No overload matches this call.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Test (macos-latest, 22, plugins)
- GitHub Check: Test (macos-latest, 24, packages)
- GitHub Check: Test (windows-latest, 24, packages)
- GitHub Check: Test (ubuntu-latest, 24, packages)
- GitHub Check: Test (ubuntu-latest, 22, packages)
- GitHub Check: Test (windows-latest, 24, plugins)
- GitHub Check: Test (windows-latest, 24, tools)
- GitHub Check: Test (windows-latest, 22, packages)
- GitHub Check: Test (windows-latest, 22, plugins)
- GitHub Check: Test (macos-latest, 24, tools)
- GitHub Check: Test (macos-latest, 22, packages)
- GitHub Check: Test egg-bin (windows-latest, 24)
- GitHub Check: Cloudflare Pages: egg-v3
🔇 Additional comments (13)
packages/egg/src/lib/egg.ts (1)
45-45: API surface update approved.Allowing 'all-in-one-process' in EggApplicationCoreOptions.mode aligns with mock/all-in-one IPC changes.
packages/egg/src/lib/core/messenger/ipc.ts (1)
60-72: Preserve receiver fields; approved.Adding receiverPid/receiverWorkerId locally and delegating to #sendMessage is a clean internalization.
plugins/logrotator/test/index.test.ts (1)
5-9: Sanity check looks goodTest verifies the public export surface without side effects.
plugins/logrotator/src/agent.ts (1)
1-3: Passthrough default export is correctRe-exporting Boot as default aligns with Egg plugin entry expectations.
plugins/logrotator/src/app/schedule/rotate_by_file.ts (1)
9-13: Fix invalid 6-field cron; use 5-field (run at 00:01 daily)Egg schedules here use 5 fields. Current value likely won’t run as intended.
return { schedule: { type: 'worker', // only one worker run this task - cron: '1 0 0 * * *', // run every day at 00:00 + cron: '1 0 * * *', // run every day at 00:01 disable: app.config.logrotator.disableRotateByDay, },plugins/logrotator/src/app/schedule/clean_log.ts (1)
49-60: Expired-file filter skips .gz files and parses dates looselyGzipped rotations (…YYYY-MM-DD.gz) aren’t matched; parsing isn’t strict and misses optional hourly suffixes.
Use a regex to capture the date token regardless of “.gz” and parse strictly:
- const names = files.filter(file => { - const name = path.extname(file).slice(1); - if (!/^\d{4}-\d{2}-\d{2}/.test(name)) { - return false; - } - const date = moment(name, 'YYYY-MM-DD').startOf('date'); - if (!date.isValid()) { - return false; - } - return date.isBefore(expiredDate); - }); + const names = files.filter(file => { + // Match suffix ".YYYY-MM-DD" or ".YYYY-MM-DD-HH", optionally followed by ".gz" + const m = file.match(/\.(\d{4}-\d{2}-\d{2})(?:-\d{2})?(?:\.gz)?$/); + if (!m) return false; + const date = moment(m[1], 'YYYY-MM-DD', true).startOf('date'); + return date.isValid() && date.isBefore(expiredDate); + });plugins/logrotator/test/logrotator.test.ts (1)
37-38: Await app.close() in afterAll
app.close()returns a promise; letting Vitest treat it as a sync callback leaves sockets/files open and causes cross-suite flakiness (echoing prior feedback). Please await it here and in the otherafterAll(() => app.close())blocks below.-afterAll(() => app.close()); +afterAll(async () => { + await app.close(); +});plugins/logrotator/test/rotate_by_day.test.ts (3)
15-23: Good: ensure clean log dir before starting appProactive cleanup of fixtures avoids cross-test interference. LGTM.
79-103: Nice coverage of rename failure pathsThe negative tests cover “target exists” and “rename error” scenarios and validate logger error behavior. LGTM.
136-148: Correct ignore lists are enforcedAssertions that day rotation skips size/hour paths are valuable to prevent double-rotation. LGTM.
plugins/logrotator/src/lib/rotator.ts (2)
36-51: Good: per-file error isolation with summary and reload signallingRotation isolates failures per file and still reloads when any succeed. Solid resilience design.
81-90: Gzip failure can orphan tmp and break logger reload; add rollbackIf
pipelinethrows after renaming totmp, the source path is gone and.tmpremains. Add try/catch with rollback.if (gzip === true) { const tmpPath = `${targetPath}.tmp`; - await fs.rename(srcPath, tmpPath); - await pipeline(createReadStream(tmpPath), createGzip(), createWriteStream(targetPath)); - await fs.unlink(tmpPath); + await fs.rename(srcPath, tmpPath); + try { + await pipeline(createReadStream(tmpPath), createGzip(), createWriteStream(targetPath)); + await fs.unlink(tmpPath); + } catch (err) { + // Best-effort rollback so logger keeps a valid path + await fs.rename(tmpPath, srcPath).catch(() => {}); + throw err; + } } else { await fs.rename(srcPath, targetPath); }plugins/logrotator/src/lib/day_rotator.ts (1)
45-68: Deprecation path looks good; guard IO with exists + try/catchSolid handling of deprecated
rotateLogDirswith best-effort scanning and error logging. LGTM.
| if (this.egg.options.mode === 'all-in-one-process') { | ||
| process.on('sendmessage-to-self', this.onMessage); | ||
| } else { | ||
| process.on('message', this.onMessage); | ||
| } |
There was a problem hiding this comment.
TS type mismatch on process.on for custom event.
CI shows “No overload matches this call.” Use EventEmitter typing to register the custom channel.
- if (this.egg.options.mode === 'all-in-one-process') {
- process.on('sendmessage-to-self', this.onMessage);
- } else {
+ if (this.egg.options.mode === 'all-in-one-process') {
+ (process as unknown as NodeJS.EventEmitter).on('sendmessage-to-self', this.onMessage);
+ } else {
process.on('message', this.onMessage);
- }
+ }📝 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.
| if (this.egg.options.mode === 'all-in-one-process') { | |
| process.on('sendmessage-to-self', this.onMessage); | |
| } else { | |
| process.on('message', this.onMessage); | |
| } | |
| if (this.egg.options.mode === 'all-in-one-process') { | |
| (process as unknown as NodeJS.EventEmitter).on('sendmessage-to-self', this.onMessage); | |
| } else { | |
| process.on('message', this.onMessage); | |
| } |
🤖 Prompt for AI Agents
In packages/egg/src/lib/core/messenger/ipc.ts around lines 30-34, TypeScript
complains "No overload matches this call" when registering the custom
'sendmessage-to-self' channel on process; fix it by using EventEmitter typing
for process (import EventEmitter from 'events' or use the NodeJS.EventEmitter
type) and cast process to that type when calling .on (e.g. (process as unknown
as EventEmitter).on('sendmessage-to-self', this.onMessage)) or use .addListener
on the cast emitter; also, if necessary, cast this.onMessage to a generic
listener type ((...args: any[]) => void) to satisfy the signature.
| #sendMessage(message: any) { | ||
| debug('[%s:%s] send message %j, mode: %s', this.egg.type, this.pid, message, this.egg.options.mode); | ||
| if (this.egg.options.mode === 'all-in-one-process') { | ||
| process.emit('sendmessage-to-self', message); | ||
| } else { | ||
| sendmessage(process, message); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix compile error: process.emit overload.
Same issue for emit; cast to EventEmitter to satisfy Node 22+ typings.
#sendMessage(message: any) {
debug('[%s:%s] send message %j, mode: %s', this.egg.type, this.pid, message, this.egg.options.mode);
if (this.egg.options.mode === 'all-in-one-process') {
- process.emit('sendmessage-to-self', message);
+ (process as unknown as NodeJS.EventEmitter).emit('sendmessage-to-self', message);
} else {
sendmessage(process, 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.
| #sendMessage(message: any) { | |
| debug('[%s:%s] send message %j, mode: %s', this.egg.type, this.pid, message, this.egg.options.mode); | |
| if (this.egg.options.mode === 'all-in-one-process') { | |
| process.emit('sendmessage-to-self', message); | |
| } else { | |
| sendmessage(process, message); | |
| } | |
| } | |
| #sendMessage(message: any) { | |
| debug('[%s:%s] send message %j, mode: %s', this.egg.type, this.pid, message, this.egg.options.mode); | |
| if (this.egg.options.mode === 'all-in-one-process') { | |
| (process as unknown as NodeJS.EventEmitter).emit('sendmessage-to-self', message); | |
| } else { | |
| sendmessage(process, message); | |
| } | |
| } |
🧰 Tools
🪛 GitHub Check: Test egg-bin (windows-latest, 22)
[failure] 137-137:
No overload matches this call.
🪛 GitHub Check: Test egg-bin (macos-latest, 22)
[failure] 137-137:
No overload matches this call.
🪛 GitHub Check: Test egg-bin (ubuntu-latest, 22)
[failure] 137-137:
No overload matches this call.
🪛 GitHub Check: Test egg-bin (macos-latest, 24)
[failure] 137-137:
No overload matches this call.
🪛 GitHub Check: Test egg-bin (ubuntu-latest, 24)
[failure] 137-137:
No overload matches this call.
🤖 Prompt for AI Agents
In packages/egg/src/lib/core/messenger/ipc.ts around lines 134 to 142,
TypeScript/Node 22+ typings cause a compile error for process.emit (and for
passing process into sendmessage); cast process to EventEmitter to satisfy the
overloads: import EventEmitter from 'events' (or use the EventEmitter type) and
replace process with (process as unknown as EventEmitter) when calling emit and
when passing to sendmessage so the calls compile against the EventEmitter-based
signatures.
| import { Boot } from './boot.ts'; | ||
|
|
||
| export default Boot; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify ESM import specifiers for build output
Source imports use “.ts” extensions. Ensure tsdown (or TS config) rewrites to “.js” in emitted ESM, or consider switching source specifiers to “.js”.
Run to audit import specifiers:
🏁 Script executed:
#!/bin/bash
# List TS import specifiers ending with .ts under this plugin
rg -nP --type=ts "from\s+['\"][^'\"]+\.ts['\"]" plugins/logrotator/srcLength of output: 1264
🏁 Script executed:
#!/bin/bash
find plugins/logrotator -type f -name 'tsdown.config.ts'Length of output: 35
🏁 Script executed:
#!/bin/bash
cat plugins/logrotator/tsdown.config.tsLength of output: 168
Align import extensions with ESM output
Change all “.ts” import specifiers under plugins/logrotator/src to “.js” (e.g. import { Boot } from './boot.js') or configure tsdown/rollup to rewrite them—unbundle mode emits .js files and doesn’t rewrite “.ts” imports by default. (tsdown.dev)
🤖 Prompt for AI Agents
In plugins/logrotator/src/agent.ts lines 1-3, the import uses a ".ts" specifier
but the build emits ESM ".js" files; update the import specifier to "./boot.js"
(or alternatively adjust tsdown/rollup config to rewrite .ts imports to .js
during unbundle builds) so runtime module resolution matches the emitted files.
| it('should not great than maxFileSize', async () => { | ||
| fs.rmSync(`${mockfile}.1`, { force: true }); | ||
| fs.writeFileSync(mockfile, ''); | ||
| await app.runSchedule(schedule); | ||
| await app.runSchedule(schedule); | ||
| // console.log(fs.readdirSync(path.dirname(mockfile))); | ||
| expect(fs.existsSync(`${mockfile}.1`)).toBe(true); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Fix test name grammar
Rename to “should not exceed maxFileSize”.
-it('should not great than maxFileSize', async () => {
+it('should not exceed maxFileSize', async () => {📝 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.
| it('should not great than maxFileSize', async () => { | |
| fs.rmSync(`${mockfile}.1`, { force: true }); | |
| fs.writeFileSync(mockfile, ''); | |
| await app.runSchedule(schedule); | |
| await app.runSchedule(schedule); | |
| // console.log(fs.readdirSync(path.dirname(mockfile))); | |
| expect(fs.existsSync(`${mockfile}.1`)).toBe(true); | |
| }); | |
| }); | |
| it('should not exceed maxFileSize', async () => { | |
| fs.rmSync(`${mockfile}.1`, { force: true }); | |
| fs.writeFileSync(mockfile, ''); | |
| await app.runSchedule(schedule); | |
| await app.runSchedule(schedule); | |
| // console.log(fs.readdirSync(path.dirname(mockfile))); | |
| expect(fs.existsSync(`${mockfile}.1`)).toBe(true); | |
| }); |
🤖 Prompt for AI Agents
In plugins/logrotator/test/rotate_by_size.test.ts around lines 71 to 79, the
test name grammar is incorrect ("should not great than maxFileSize"); update the
test description to "should not exceed maxFileSize" by renaming the it()
description string accordingly so the test reads clearly and grammatically
correct.
| export function getFixtures(name: string) { | ||
| return path.join(import.meta.dirname, 'fixtures', name); | ||
| } |
There was a problem hiding this comment.
import.meta.dirname is non-standard and will be undefined under Node/Vitest
Use import.meta.url + fileURLToPath to derive dirname; also add an explicit return type.
Apply:
import path from 'node:path';
+import { fileURLToPath } from 'node:url';
-export function getFixtures(name: string) {
- return path.join(import.meta.dirname, 'fixtures', name);
+const __dirname = path.dirname(fileURLToPath(import.meta.url));
+
+export function getFixtures(name: string): string {
+ return path.join(__dirname, 'fixtures', name);
}📝 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.
| export function getFixtures(name: string) { | |
| return path.join(import.meta.dirname, 'fixtures', name); | |
| } | |
| import path from 'node:path'; | |
| import { fileURLToPath } from 'node:url'; | |
| const __dirname = path.dirname(fileURLToPath(import.meta.url)); | |
| export function getFixtures(name: string): string { | |
| return path.join(__dirname, 'fixtures', name); | |
| } |
🤖 Prompt for AI Agents
In plugins/logrotator/test/utils.ts around lines 3 to 5, replace the
non-standard use of import.meta.dirname with a Node-compatible approach: import
fileURLToPath from 'url' (or destructure fileURLToPath from 'url') and derive
__dirname via path.dirname(fileURLToPath(import.meta.url)), then use that
dirname when joining 'fixtures' and name; also add an explicit return type of
string to the getFixtures function signature. Ensure required imports (path and
fileURLToPath) are present at the top of the file.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/egg/src/lib/core/messenger/ipc.ts (3)
2-2: Fix invalid default import from node:worker_threads (ESM runtime error).Built-in Node ESM modules don’t have default exports. This will throw at runtime under unbundled ESM builds.
Apply:
-import workerThreads from 'node:worker_threads'; +import * as workerThreads from 'node:worker_threads';
165-169: Remove the listener you actually added; also detach workerThreads listener (memory leak).In all-in-one mode you never remove 'sendmessage-to-self', so listeners leak across test runs and hot restarts. Also detach parentPort.
Proposed implementation (outside changed hunk):
close() { const procEmitter = process as unknown as NodeJS.EventEmitter; if (this.egg.options.mode === 'all-in-one-process') { procEmitter.removeListener('sendmessage-to-self', this.onMessage as (...args: any[]) => void); } else { procEmitter.removeListener('message', this.onMessage as (...args: any[]) => void); } workerThreads.parentPort?.off?.('message', this.onMessage); this.removeAllListeners(); }
6-8: Enable.tsextension imports in TS config: add"allowImportingTsExtensions": true(or set"moduleResolution": "nodenext") to your roottsconfig.jsonand ensurepackages/egg/tsconfig.jsoninherits it—without this, the.tsimports inipc.tswill break under ESM.
🧹 Nitpick comments (2)
packages/egg/src/lib/core/messenger/ipc.ts (2)
62-72: Type the IPC envelope; avoid implicit any on message payload.Improves API clarity and prevents shape drift.
- const message = { + const message: IpcEnvelope = { action, data, /** * @deprecated Keep compatible, please use receiverWorkerId instead */ receiverPid: String(workerId), receiverWorkerId: String(workerId), }; this.#sendMessage(message);Add near the top (outside changed hunk):
interface IpcEnvelope { readonly action: string; readonly data?: unknown; readonly to?: 'app' | 'agent'; readonly receiverPid?: string; readonly receiverWorkerId?: string; }
124-131: Restore debug context and enforce envelope typing on send().Keeps logs consistent with the rest of the file and ensures the object matches the envelope type.
- debug('send message %s with %j to %s', action, data, to); - this.#sendMessage({ + debug('[%s:%s] send %s with %j to %s', this.egg.type, this.pid, action, data, to); + this.#sendMessage({ action, data, to, - }); + } as IpcEnvelope);Optionally tighten the signature (outside changed hunk):
send(action: string, data: unknown | undefined, to?: 'app' | 'agent'): Messenger {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/egg/src/lib/core/messenger/ipc.ts(3 hunks)plugins/logrotator/src/lib/day_rotator.ts(1 hunks)plugins/logrotator/test/clean_log.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
packages/**/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use TypeScript for all source files in packages (no JavaScript source files)
Files:
packages/egg/src/lib/core/messenger/ipc.ts
packages/egg/src/lib/core/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Implement core components (httpclient, logger, messenger) under packages/egg/src/lib/core/
Files:
packages/egg/src/lib/core/messenger/ipc.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: Prefer TypeScript and ESM: write sources and exports in .ts (ESM-first) rather than CommonJS
Use two-space indentation, trailing commas, and semicolons (Prettier/oxlint defaults)
Name files in lowercase with hyphens (e.g., loader-context.ts)
Name classes in PascalCase
Name functions and variables in camelCase
Re-export types thoughtfully to keep the public API stable
Files:
packages/egg/src/lib/core/messenger/ipc.tsplugins/logrotator/test/clean_log.test.tsplugins/logrotator/src/lib/day_rotator.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Enable strict TypeScript mode and use explicit return types for public APIs
Prefer interfaces over type aliases for object shapes
Use readonly modifiers where appropriate
Avoid the 'any' type; use 'unknown' when the type is truly unknown
Files:
packages/egg/src/lib/core/messenger/ipc.tsplugins/logrotator/test/clean_log.test.tsplugins/logrotator/src/lib/day_rotator.ts
**/test/**/*.test.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/test/**/*.test.ts: Place test suites following Vitest discovery: /test//*.test.ts
Mirror the repository test pattern when adding new suites
**/test/**/*.test.ts: Test files should follow the naming pattern test/**/*.test.ts
Prefer importing test APIs from 'vitest' and use Node's 'assert' for assertions in Vitest-based packages
Files:
plugins/logrotator/test/clean_log.test.ts
🧬 Code graph analysis (2)
plugins/logrotator/test/clean_log.test.ts (2)
plugins/logrotator/src/app/schedule/clean_log.ts (1)
app(11-37)plugins/logrotator/test/utils.ts (1)
getFixtures(3-5)
plugins/logrotator/src/lib/day_rotator.ts (3)
plugins/logrotator/src/lib/rotator.ts (2)
RotatorOptions(13-15)RotateFile(17-20)packages/egg/src/lib/egg.ts (1)
loggers(405-410)plugins/logrotator/src/lib/utils.ts (1)
walkLoggerFile(10-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: Test (macos-latest, 24, packages)
- GitHub Check: Test (windows-latest, 22, plugins)
- GitHub Check: Test (windows-latest, 24, plugins)
- GitHub Check: Test (windows-latest, 22, packages)
- GitHub Check: Test (macos-latest, 22, packages)
- GitHub Check: Test (macos-latest, 22, tools)
- GitHub Check: Test (ubuntu-latest, 24, packages)
- GitHub Check: Test (ubuntu-latest, 22, packages)
- GitHub Check: Test (windows-latest, 24, packages)
- GitHub Check: Test egg-bin (windows-latest, 24)
- GitHub Check: Test egg-bin (windows-latest, 22)
- GitHub Check: Test egg-bin (macos-latest, 22)
- GitHub Check: Test egg-bin (ubuntu-latest, 22)
- GitHub Check: Test egg-bin (ubuntu-latest, 24)
- GitHub Check: Test egg-bin (macos-latest, 24)
- GitHub Check: Cloudflare Pages: egg-v3
🔇 Additional comments (3)
plugins/logrotator/test/clean_log.test.ts (1)
37-37: Await app.close() to finish teardownLine 37 still calls
app.close()without awaiting the promise, so the app may keep sockets and file handles open between tests. Please make the callback async (or return the promise) so Vitest waits for shutdown.- afterEach(() => app.close()); + afterEach(async () => { + await app.close(); + });packages/egg/src/lib/core/messenger/ipc.ts (2)
30-34: Cast process to EventEmitter for custom event binding to satisfy Node 22+ typings.Avoids “No overload matches this call” for non-typed events and keeps ‘message’ path unchanged.
- if (this.egg.options.mode === 'all-in-one-process') { - process.on('sendmessage-to-self', this.onMessage); - } else { + if (this.egg.options.mode === 'all-in-one-process') { + (process as unknown as NodeJS.EventEmitter).on( + 'sendmessage-to-self', + this.onMessage as (...args: any[]) => void, + ); + } else { process.on('message', this.onMessage); - } + }
134-142: Cast process for emit/sendmessage; drop @ts-expect-error.Fixes compile errors on Node 22+ typings and resolves CI failures.
- if (this.egg.options.mode === 'all-in-one-process') { - // @ts-expect-error event `sendmessage-to-self` is not typed - process.emit('sendmessage-to-self', message); - } else { - sendmessage(process, message); - } + if (this.egg.options.mode === 'all-in-one-process') { + (process as unknown as NodeJS.EventEmitter).emit('sendmessage-to-self', message); + } else { + // sendmessage types accept EventEmitter in some versions; cast for compatibility + sendmessage(process as unknown as NodeJS.EventEmitter, message); + }
| this.filesRotateBySize = this.app.config.logrotator.filesRotateBySize ?? []; | ||
| this.filesRotateByHour = this.app.config.logrotator.filesRotateByHour ?? []; | ||
| } |
There was a problem hiding this comment.
Normalize skip lists to absolute paths
walkLoggerFile yields absolute file paths, yet we store filesRotateBySize / filesRotateByHour exactly as provided. If a project config uses the documented relative-path form, includes(srcPath) will never hit, so those logs keep getting rotated by day instead of size/hour—a regression from the existing plugin behaviour. Please normalize the configured paths against logger.dir during construction.
Apply this diff to restore the expected behaviour:
constructor(options: RotatorOptions) {
super(options);
- this.filesRotateBySize = this.app.config.logrotator.filesRotateBySize ?? [];
- this.filesRotateByHour = this.app.config.logrotator.filesRotateByHour ?? [];
+ const resolvePath = (file: string) =>
+ path.isAbsolute(file) ? file : path.join(this.app.config.logger.dir, file);
+ this.filesRotateBySize =
+ (this.app.config.logrotator.filesRotateBySize ?? []).map(resolvePath);
+ this.filesRotateByHour =
+ (this.app.config.logrotator.filesRotateByHour ?? []).map(resolvePath);
}📝 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.
| this.filesRotateBySize = this.app.config.logrotator.filesRotateBySize ?? []; | |
| this.filesRotateByHour = this.app.config.logrotator.filesRotateByHour ?? []; | |
| } | |
| constructor(options: RotatorOptions) { | |
| super(options); | |
| const resolvePath = (file: string) => | |
| path.isAbsolute(file) ? file : path.join(this.app.config.logger.dir, file); | |
| this.filesRotateBySize = | |
| (this.app.config.logrotator.filesRotateBySize ?? []).map(resolvePath); | |
| this.filesRotateByHour = | |
| (this.app.config.logrotator.filesRotateByHour ?? []).map(resolvePath); | |
| } |
🤖 Prompt for AI Agents
In plugins/logrotator/src/lib/day_rotator.ts around lines 21 to 23, the
configured filesRotateBySize/filesRotateByHour may be relative while
walkLoggerFile yields absolute paths, so normalize each configured entry to
absolute paths against this.app.logger.dir during construction: for both arrays,
map each entry to its absolute path (if path.isAbsolute(entry) keep it,
otherwise path.join(this.app.logger.dir, entry or entry.trim())), preserve
empty/default arrays, and assign the normalized arrays back to
this.filesRotateBySize and this.filesRotateByHour so includes(srcPath) will
match correctly.
| expect( | ||
| files.some(name => name.includes('foo.log.')), | ||
| `files: ${JSON.stringify(files)}` | ||
| ); |
There was a problem hiding this comment.
Call a matcher on expect(files.some(...))
Line 91 creates an expectation but never invokes a matcher, so the assertion becomes a no-op and the test can silently pass even if no rotated logs remain. Chain a matcher (e.g., .toBe(true)) to actually verify the condition.
- expect(
- files.some(name => name.includes('foo.log.')),
- `files: ${JSON.stringify(files)}`
- );
+ expect(
+ files.some(name => name.includes('foo.log.')),
+ `files: ${JSON.stringify(files)}`
+ ).toBe(true);📝 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.
| expect( | |
| files.some(name => name.includes('foo.log.')), | |
| `files: ${JSON.stringify(files)}` | |
| ); | |
| expect( | |
| files.some(name => name.includes('foo.log.')), | |
| `files: ${JSON.stringify(files)}` | |
| ).toBe(true); |
🤖 Prompt for AI Agents
In plugins/logrotator/test/clean_log.test.ts around lines 91-94, the test calls
expect(files.some(name => name.includes('foo.log.'))) but doesn't chain a
matcher so the assertion is a no-op; change it to actually assert the boolean
(for example: expect(files.some(name => name.includes('foo.log.'))).toBe(true))
and remove the unused second argument to expect (or move any debug JSON into a
console.debug/log if you need it).
1 participant
The logrotator plugin provides automatic log file rotation based on time or size, supports daily rotation with configurable patterns, and includes log cleanup to remove old files based on maxDays configuration.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
Chores