fix: add isDirectory property #18
Conversation
|
Warning Rate limit exceeded@fengmk2 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 19 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces enhancements to the file system event handling mechanism in the development event source. The changes include adding an Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/lib/event-sources/development.ts (1)
92-97: Consider caching stat result to avoid duplicate fs callsThe implementation correctly adds the isDirectory property, but there's a potential performance optimization.
The
statis already computed in thewatchmethod for directories. Consider passing it to#onFsWatchChangeto avoid an extra fs call:- const stat = fs.statSync(file, { throwIfNoEntry: false }); + // Reuse stat from watch method for directories + const stat = existingStat ?? fs.statSync(file, { throwIfNoEntry: false });test/fixtures/apps/watcher-development-app/app/router.js (2)
8-8: Consider adding reset capability for hasDir flagThe
hasDirflag is set to true when a directory change is detected but there's no way to reset it to false. This could lead to stale state in long-running tests.Consider adding a reset endpoint or mechanism:
let hasDir = false; function callback(info) { console.log('got change %j', info); fileChangeCount++; if (info.isDirectory) { hasDir = true; } } +app.get('/app-reset', async ctx => { + hasDir = false; + fileChangeCount = 0; + ctx.body = 'reset success'; +});Also applies to: 13-15
32-36: Add error handling to /app-hasDir endpointThe endpoint implementation looks good but could benefit from error handling.
Consider wrapping the response in a try-catch:
app.get('/app-hasDir', async ctx => { + try { ctx.body = { hasDir, }; + } catch (error) { + ctx.status = 500; + ctx.body = { error: error.message }; + } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/lib/event-sources/development.ts(1 hunks)src/lib/types.ts(1 hunks)test/development.test.ts(1 hunks)test/fixtures/apps/watcher-development-app/app/router.js(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Publish Any Commit
src/lib/types.ts
[error] 29-29: Property 'watcher' has conflicting type declarations. It must be of type 'Watcher', but has a different 'Watcher' type definition. This conflicts with the declaration in node_modules/@eggjs/watcher/dist/esm/lib/types.d.ts:25
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Node.js / Test (windows-latest, 23)
- GitHub Check: Node.js / Test (windows-latest, 22)
- GitHub Check: Node.js / Test (windows-latest, 20)
- GitHub Check: Node.js / Test (windows-latest, 18)
| @@ -21,6 +21,7 @@ export interface ChangeInfo extends Record<string, any> { | |||
| */ | |||
| stat?: Stats; | |||
| path: string; | |||
| isDirectory?: boolean; | |||
There was a problem hiding this comment.
Fix type conflict with @eggjs/watcher package
The pipeline is failing due to conflicting Watcher type declarations between this package and @eggjs/watcher. This needs to be resolved to ensure type compatibility.
Consider one of these approaches:
- If this package is meant to extend @eggjs/watcher, use interface merging:
import type { ChangeInfo as BaseChangeInfo } from '@eggjs/watcher';
export interface ChangeInfo extends BaseChangeInfo {
isDirectory?: boolean;
}- If this is a standalone package, remove the dependency on @eggjs/watcher types.
test/development.test.ts
Outdated
| await app.httpRequest() | ||
| .get('/app-hasDir') | ||
| .expect(200) | ||
| .expect({ | ||
| hasDir: false, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for directory changes
The test only verifies the initial state where hasDir is false. Consider adding test cases for directory changes.
Add test cases to verify hasDir becomes true after directory operations:
// Test directory creation
fs.mkdirSync(file_path4, { recursive: true });
await scheduler.wait(100);
await app.httpRequest()
.get('/app-hasDir')
.expect(200)
.expect({
hasDir: true,
});
// Optional: Test directory deletion if it should trigger the flag
fs.rmdirSync(file_path4);
await scheduler.wait(100);
await app.httpRequest()
.get('/app-hasDir')
.expect(200)
.expect({
hasDir: true, // Verify if deletion should also set hasDir
});
commit: |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #18 +/- ##
==========================================
+ Coverage 93.37% 93.46% +0.09%
==========================================
Files 13 13
Lines 362 367 +5
Branches 45 46 +1
==========================================
+ Hits 338 343 +5
Misses 24 24 ☔ View full report in Codecov by Sentry. |
[skip ci] ## [4.0.4](v4.0.3...v4.0.4) (2025-01-11) ### Bug Fixes * add isDirectory property ([#18](#18)) ([1aaa983](1aaa983))
Summary by CodeRabbit
New Features
/app-hasDirto check directory existence.Tests
These updates improve the application's ability to handle file system events and provide users with information about directory modifications.