Open
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughEggLoader and egg-bundler persist and restore loader metadata via a new ChangesLoader Manifest Persistence
Tegg Manifest Path Restoration
Bundled Runtime Framework Path Virtualization
Controller Loader & Test Timeouts / Minor Fixes
Sequence DiagramsequenceDiagram
participant Bundler as Bundler (build)
participant ManifestStore as ManifestStore
participant Runtime as Generated Worker
participant EggLoader as EggLoader
participant Framework as egg Framework
Bundler->>ManifestStore: collect loader metadata -> extensions.eggLoader
Bundler->>ManifestStore: normalize eggLoader paths/plugins
Note over Runtime: Startup
Runtime->>ManifestStore: retrieve manifest.extensions.eggLoader
ManifestStore-->>Runtime: eggPaths + plugin metadata
Runtime->>Runtime: __patchFrameworkPaths() → virtualize framework roots
Runtime->>EggLoader: getEggPaths()
EggLoader->>ManifestStore: read eggLoader.eggPaths
EggLoader-->>Runtime: return merged absolute eggPaths (exclude output-root)
Runtime->>EggLoader: loadPlugin()
EggLoader->>ManifestStore: read plugin metadata
EggLoader->>EggLoader: apply cached plugin info → merged allPlugins
EggLoader->>ManifestStore: persist selected plugin metadata
EggLoader-->>Runtime: loadPlugin() complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: |
52da926
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5cb92046.egg-cci.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-01ea13af.egg-cci.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5934 +/- ##
==========================================
+ Coverage 85.18% 85.27% +0.08%
==========================================
Files 668 668
Lines 19288 19358 +70
Branches 3784 3816 +32
==========================================
+ Hits 16431 16507 +76
+ Misses 2465 2459 -6
Partials 392 392 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Deploying egg-v3 with
|
| Latest commit: |
52da926
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1c2e8ae3.egg-v3.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-01ea13af.egg-v3.pages.dev |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to bundle and restore loader state, including framework paths and plugin information, through a new manifest extension. Key changes include the ability for the core loader to merge paths from the manifest, utilities for restoring relative paths at runtime, and enhancements to the bundler to normalize manifest data and patch framework paths in generated entries. Feedback highlights potential issues with path comparison logic in both the core loader and the bundler's generated code, specifically regarding the handling of symbolic links when identifying root paths.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes bundled runtime startup for tegg + egg loader by replaying manifest-derived paths against the runtime bundle baseDir early enough to avoid “collapsed to outputDir” resolution, and adds targeted regression tests across bundler/core/tegg.
Changes:
- Normalize and replay
eggLoadermanifest extension (eggPaths + plugin paths) during bundler manifest load and core loader startup. - Restore tegg manifest module reference/descriptor paths against runtime
baseDirbefore module config + load-unit creation. - Add focused tests for framework path virtualization in the generated worker entry, tegg manifest restoration, and controller load-unit
baseDirfallback.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tools/egg-bundler/src/lib/ManifestLoader.ts | Normalize extensions.eggLoader paths during manifest normalization. |
| tools/egg-bundler/src/lib/EntryGenerator.ts | Patch framework customEggPaths() in generated worker to virtualize eggPaths under output node_modules/ before startEgg(). |
| tools/egg-bundler/test/ManifestLoader.test.ts | Extend coverage for normalized eggLoader extension payload. |
| tools/egg-bundler/test/EntryGenerator.test.ts | Add runtime test ensuring framework eggPaths are virtualized before startEgg() executes. |
| tools/egg-bundler/test/snapshots/EntryGenerator.worker.canonical.snap | Update canonical worker snapshot to include framework-path patching logic. |
| packages/core/src/loader/egg_loader.ts | Apply bundled loader manifest eggPaths/plugin metadata and collect eggLoader extension for manifest generation. |
| packages/core/test/loader/mixin/load_plugin.test.ts | Add coverage that bundled loader manifest eggPaths + plugin paths are applied. |
| tegg/core/loader/src/LoaderFactory.ts | Add helpers to restore manifest module paths against runtime baseDir. |
| tegg/core/loader/test/LoaderFactoryManifest.test.ts | Add test verifying restored manifest paths match on-disk loader results. |
| tegg/plugin/tegg/src/lib/EggModuleLoader.ts | Restore tegg manifest extension paths before calling LoaderFactory.loadApp(). |
| tegg/plugin/config/src/app.ts | Restore manifest module reference paths before config/load-unit creation. |
| tegg/plugin/config/test/ReadModule.test.ts | Add coverage for restoring manifest module references against runtime baseDir. |
| tegg/plugin/controller/src/lib/ControllerLoadUnitHandler.ts | Fallback to app.baseDir when config.baseDir isn’t available yet. |
| tegg/plugin/controller/test/lib/ControllerLoadUnitHandler.test.ts | Add regression test for controller load-unit baseDir fallback. |
| tegg/plugin/aop/test/aop.test.ts | Increase test timeout for readiness. |
| tegg/plugin/orm/test/index.test.ts | Increase test timeout for readiness. |
| tegg/plugin/tegg/test/ModuleConfig.test.ts | Increase test timeout for readiness. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tegg/core/loader/src/LoaderFactory.ts (1)
46-57: ⚡ Quick winConsider defensive fallbacks for potentially missing arrays.
manifest.moduleReferencesandmanifest.moduleDescriptorscome from raw deserialized JSON, so either field can be absent despite the type annotation. Calling.map()onundefinedwould throw and crash app startup.This inconsistency is also visible in the caller (
EggModuleLoader.tsline 44), whererestoredManifestTegg?.moduleDescriptors?.lengthuses optional chaining to guard — yet that check happens afterrestoreTeggManifestExtensionwould have already thrown ifmoduleDescriptorswas missing.🛡️ Proposed defensive fix
export function restoreTeggManifestExtension(manifest: TeggManifestExtension, baseDir: string): TeggManifestExtension { return { - moduleReferences: manifest.moduleReferences.map((ref) => ({ + moduleReferences: (manifest.moduleReferences ?? []).map((ref) => ({ ...ref, path: restoreManifestModulePath(ref.path, baseDir), })), - moduleDescriptors: manifest.moduleDescriptors.map((desc) => ({ + moduleDescriptors: (manifest.moduleDescriptors ?? []).map((desc) => ({ ...desc, unitPath: restoreManifestModulePath(desc.unitPath, baseDir), })), }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tegg/core/loader/src/LoaderFactory.ts` around lines 46 - 57, The function restoreTeggManifestExtension will throw if manifest.moduleReferences or manifest.moduleDescriptors are undefined; update restoreTeggManifestExtension to defensively handle missing arrays by using safe fallbacks (e.g., manifest.moduleReferences ?? [] and manifest.moduleDescriptors ?? []) before calling .map(); keep using restoreManifestModulePath for each element and return the same TeggManifestExtension shape with empty arrays when fields are absent.tegg/plugin/config/test/ReadModule.test.ts (1)
59-80: ⚡ Quick winConsider key-aware
getExtensionmock for better isolation.
getExtension()currently ignores its argument and always returns the tegg manifest. IfloadMetadata()also callsgetExtension()for other keys (e.g.,eggLoader), those calls silently return the tegg payload, which could mask unexpected interactions.🔧 Proposed tighter mock
- getExtension() { + getExtension(key: string) { + if (key !== 'tegg') return undefined; return { moduleReferences: [🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tegg/plugin/config/test/ReadModule.test.ts` around lines 59 - 80, The current mock of getExtension in the test always returns the tegg manifest regardless of the requested key, which can mask unintended interactions; update the mock used in ReadModule.test.ts so that getExtension(key) returns different payloads based on the key (e.g., when key === 'tegg' return the moduleReferences/moduleDescriptors object, when key === 'eggLoader' or others return undefined or a minimal stub). Locate the getExtension mock in the test (the anonymous object under manifest) and change it to a key-aware function that switches on the incoming key to return only the intended manifest for the tegg key and safe defaults for any other keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tegg/plugin/controller/test/lib/ControllerLoadUnitHandler.test.ts`:
- Around line 5-6: Update the ESM imports in ControllerLoadUnitHandler.test:
replace the TypeScript file extensions in the import statements that reference
CONTROLLER_LOAD_UNIT and ControllerLoadUnitHandler so they import the compiled
ESM module names (use .js instead of .ts); locate the two imports that currently
import '../../src/lib/ControllerLoadUnit.ts' and
'../../src/lib/ControllerLoadUnitHandler.ts' and change them to their .js
counterparts to comply with the repo ESM import rule.
---
Nitpick comments:
In `@tegg/core/loader/src/LoaderFactory.ts`:
- Around line 46-57: The function restoreTeggManifestExtension will throw if
manifest.moduleReferences or manifest.moduleDescriptors are undefined; update
restoreTeggManifestExtension to defensively handle missing arrays by using safe
fallbacks (e.g., manifest.moduleReferences ?? [] and manifest.moduleDescriptors
?? []) before calling .map(); keep using restoreManifestModulePath for each
element and return the same TeggManifestExtension shape with empty arrays when
fields are absent.
In `@tegg/plugin/config/test/ReadModule.test.ts`:
- Around line 59-80: The current mock of getExtension in the test always returns
the tegg manifest regardless of the requested key, which can mask unintended
interactions; update the mock used in ReadModule.test.ts so that
getExtension(key) returns different payloads based on the key (e.g., when key
=== 'tegg' return the moduleReferences/moduleDescriptors object, when key ===
'eggLoader' or others return undefined or a minimal stub). Locate the
getExtension mock in the test (the anonymous object under manifest) and change
it to a key-aware function that switches on the incoming key to return only the
intended manifest for the tegg key and safe defaults for any other keys.
🪄 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: bf35f149-cf3e-45bb-8ad3-fbd22d091e4f
⛔ Files ignored due to path filters (1)
tools/egg-bundler/test/__snapshots__/EntryGenerator.worker.canonical.snapis excluded by!**/*.snap
📒 Files selected for processing (16)
packages/core/src/loader/egg_loader.tspackages/core/test/loader/mixin/load_plugin.test.tstegg/core/loader/src/LoaderFactory.tstegg/core/loader/test/LoaderFactoryManifest.test.tstegg/plugin/aop/test/aop.test.tstegg/plugin/config/src/app.tstegg/plugin/config/test/ReadModule.test.tstegg/plugin/controller/src/lib/ControllerLoadUnitHandler.tstegg/plugin/controller/test/lib/ControllerLoadUnitHandler.test.tstegg/plugin/orm/test/index.test.tstegg/plugin/tegg/src/lib/EggModuleLoader.tstegg/plugin/tegg/test/ModuleConfig.test.tstools/egg-bundler/src/lib/EntryGenerator.tstools/egg-bundler/src/lib/ManifestLoader.tstools/egg-bundler/test/EntryGenerator.test.tstools/egg-bundler/test/ManifestLoader.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tegg/plugin/config/src/app.ts (1)
85-100:⚠️ Potential issue | 🔴 CriticalThis replacement loses critical path resolution logic and will break module loading
ModuleConfigUtil.resolveModuleDirappends'config'to relative paths:return path.join(baseDir, 'config', moduleDir);
restoreManifestModulePathdoes not:return path.join(baseDir, modulePath);When
reference.pathis relative, the new code will look for module config at${baseDir}/${path}instead of${baseDir}/config/${path}, causingreadModuleNameSyncandloadModuleConfigSyncto fail on the wrong directory. This differs from the pattern still used integg/standalone/standalone/src/Runner.ts, which continues usingresolveModuleDir. Either the manifest paths are guaranteed absolute (in which case the semantic change should be documented), or this change introduces a critical bug in module resolution.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tegg/plugin/config/src/app.ts` around lines 85 - 100, The replacement in _loadModuleConfigs() breaks relative-path handling because restoreManifestModulePath does not include the 'config' segment like ModuleConfigUtil.resolveModuleDir does; update _loadModuleConfigs() to resolve module directories using the same logic as ModuleConfigUtil.resolveModuleDir (or call that method) when constructing absoluteRef.path for relative reference.path values so readModuleNameSync and loadModuleConfigSync receive the expected ${baseDir}/config/${moduleDir}; preserve ModuleReference.optional/name handling and only alter how absoluteRef.path is computed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tegg/plugin/config/src/app.ts`:
- Around line 19-30: restoreTeggManifestExtension currently calls
manifest.moduleDescriptors.map(...) without guarding for missing
moduleDescriptors which will throw a TypeError; update
restoreTeggManifestExtension to treat moduleDescriptors like moduleReferences
(use manifest.moduleDescriptors ?? [] or skip mapping when undefined) and only
map/restore unitPath when entries exist, leaving moduleReferences mapping as-is
(still using restoreManifestModulePath for ref.path) and avoid unnecessary work
since the call-site only consumes moduleReferences.
---
Outside diff comments:
In `@tegg/plugin/config/src/app.ts`:
- Around line 85-100: The replacement in _loadModuleConfigs() breaks
relative-path handling because restoreManifestModulePath does not include the
'config' segment like ModuleConfigUtil.resolveModuleDir does; update
_loadModuleConfigs() to resolve module directories using the same logic as
ModuleConfigUtil.resolveModuleDir (or call that method) when constructing
absoluteRef.path for relative reference.path values so readModuleNameSync and
loadModuleConfigSync receive the expected ${baseDir}/config/${moduleDir};
preserve ModuleReference.optional/name handling and only alter how
absoluteRef.path is computed.
🪄 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: f2a97f00-6fdb-47db-b307-0919df12c7ba
📒 Files selected for processing (3)
tegg/core/loader/test/LoaderFactoryManifest.test.tstegg/plugin/config/src/app.tstegg/plugin/tegg/src/lib/EggModuleLoader.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tegg/plugin/tegg/src/lib/EggModuleLoader.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Dependency
Tests
Summary by CodeRabbit
New Features
Bug Fixes
Tests