fix(core): skip beforeClose hooks in metadataOnly mode#5904
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements conditional ChangesMetadata-Only Close Behavior
Documentation and Dependencies
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying egg with
|
| Latest commit: |
ca46307
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7fe3fcdb.egg-cci.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-rebuild-manife.egg-cci.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5904 +/- ##
==========================================
- Coverage 85.03% 85.03% -0.01%
==========================================
Files 665 665
Lines 19104 19108 +4
Branches 3717 3719 +2
==========================================
+ Hits 16246 16249 +3
- Misses 2465 2466 +1
Partials 393 393 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Deploying egg-v3 with
|
| Latest commit: |
ca46307
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c159b383.egg-v3.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-rebuild-manife.egg-v3.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/lifecycle.ts`:
- Around line 257-265: The metadata-only early return in close() (when
this.#metadataOnly is true) skips running hooks but fails to clear the internal
close hook registry; update the metadata-only branch to also clear
this.#closeFunctionSet (and any related state if needed) before returning so
stale callback references are removed; locate the block checking
this.#metadataOnly in lifecycle.ts and add logic to clear this.#closeFunctionSet
(and/or reset its backing structure) alongside
removeAllListeners()/this.app.removeAllListeners() and setting this.#isClosed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 08382e39-a703-4f35-975b-f7747387a01c
📒 Files selected for processing (3)
packages/core/src/lifecycle.tspackages/egg/test/fixtures/apps/metadata-only-app/app.jspackages/egg/test/start.test.ts
There was a problem hiding this comment.
Pull request overview
This PR prevents beforeClose hooks from running when closing an app started in metadataOnly mode (used for manifest generation), and adds a regression test/fixture coverage to ensure metadata-only close remains side-effect free.
Changes:
- Short-circuit
Lifecycle.close()inmetadataOnlymode to skipbeforeClosecallbacks. - Extend the
metadata-only-appfixture to register anapp.beforeClose()callback for regression coverage. - Add a test that asserts closing a metadata-only app does not trigger
beforeClosecallbacks.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/core/src/lifecycle.ts | Adds an early-return close path for metadataOnly to skip beforeClose functions. |
| packages/egg/test/start.test.ts | Adds a regression test for metadata-only close behavior (and adjusts surrounding tests). |
| packages/egg/test/fixtures/apps/metadata-only-app/app.js | Registers an app.beforeClose() callback to validate the new close behavior. |
![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 modifies the Lifecycle class to skip beforeClose functions when the application is in metadataOnly mode. It also includes a new test case to verify that these callbacks are correctly bypassed. A review comment identifies code duplication in the close method and suggests refactoring the cleanup logic into a common section to improve maintainability.
killagu
force-pushed
the
agent/egg-dev/rebuild-manifest-close
branch
from
1ea1b61 to
6a9b5c9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Around line 87-89: The patchedDependencies entry currently pins "utoo@1.0.28"
which can be skipped if pnpm resolves a different 1.x version; update
consistency by either changing the catalog entry for utoo in pnpm-workspace.yaml
from "utoo: ^1" to an exact version "1.0.28" or by updating the
patchedDependencies key in package.json to match the actual resolved version
(e.g., "utoo@<resolved-version>"); ensure the symbol names to modify are
pnpm-workspace.yaml's utoo entry and package.json's patchedDependencies
"utoo@1.0.28" key so the patch is applied reliably.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 20466413-873f-44a6-a9af-73b307aba475
📒 Files selected for processing (2)
package.jsonpatches/utoo@1.0.28.patch
✅ Files skipped from review due to trivial changes (1)
- patches/utoo@1.0.28.patch
There was a problem hiding this comment.
🧹 Nitpick comments (1)
package.json (1)
85-87: KeepoptionalDependenciesversion synchronized with the workspace catalog.The aliased target
@utoo/utoo-win32-x64@1.0.28is published on npm and verified to exist. However, the hard-pinned version1.0.28inoptionalDependenciesis decoupled from theutoo: "catalog:"entry indevDependencies. When the catalog version ofutoois bumped, this entry must be updated manually or the mingw64 variant will ship with a mismatched binary version.Add a comment to flag the sync requirement:
Suggested sync reminder
+ // NOTE: keep this version in sync with the `utoo` entry in the workspace catalog. "optionalDependencies": { "@utoo/utoo-mingw64_nt-10.0-26100-x64": "npm:`@utoo/utoo-win32-x64`@1.0.28" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 85 - 87, Add an inline comment next to the optionalDependencies entry for "@utoo/utoo-mingw64_nt-10.0-26100-x64" that reminds maintainers to keep the aliased target version "@utoo/utoo-win32-x64@1.0.28" in sync with the workspace catalog entry for "utoo" in devDependencies (the catalog: version); mention that when the catalog version is bumped the optionalDependencies alias must be updated to avoid shipping a mismatched binary version.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@package.json`:
- Around line 85-87: Add an inline comment next to the optionalDependencies
entry for "@utoo/utoo-mingw64_nt-10.0-26100-x64" that reminds maintainers to
keep the aliased target version "@utoo/utoo-win32-x64@1.0.28" in sync with the
workspace catalog entry for "utoo" in devDependencies (the catalog: version);
mention that when the catalog version is bumped the optionalDependencies alias
must be updated to avoid shipping a mismatched binary version.
Summary
Why
Tests
Refs EGG-4.
Summary by CodeRabbit
Bug Fixes
Documentation
Chores