Issue/504 load modules boot stage#505
Conversation
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe PR moves module loading from per-request execution in ChangesBoot & Request Lifecycle Refactoring
Sequence DiagramsequenceDiagram
participant Boot as Boot Pipeline
participant LMS as LoadModulesStage
participant ML as ModuleLoader
participant RB as RouteBuilder
participant DI as DI Container
participant Start as start() Handler
participant Req as Request
Note over Boot,Start: Before: Module Load Per-Request
Boot->>Start: invoke start()
activate Start
Start->>ML: loadModules()
activate ML
ML->>DI: register routes
deactivate ML
Start->>Req: handle request
deactivate Start
Note over Boot,Start: After: Module Load At Boot
Boot->>LMS: boot phase
activate LMS
LMS->>ML: load modules
activate ML
ML->>RB: pass module routes
deactivate ML
RB->>DI: build & register RouteCollection
deactivate LMS
Boot->>Start: start (per-request)
activate Start
Start->>Req: handle request (routes cached)
deactivate Start
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 866b4d490a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/App/Stages/LoadModulesStage.php`:
- Line 52: The unconditional registration Di::set(RouteCollection::class,
$collection) in LoadModulesStage risks dependencyAlreadyRegistered when a
RouteCollection is already bound; guard this by checking the container before
setting (e.g., use Di::has or equivalent to detect an existing RouteCollection)
and only call Di::set(RouteCollection::class, $collection) when none exists, or
catch and ignore dependencyAlreadyRegistered around the Di::set call so multiple
app instances can boot safely.
In `@tests/Unit/Http/Helpers/HttpHelperTest.php`:
- Around line 61-65: The test is mutating the DI-shared RouteCollection by
calling Di::get(RouteCollection::class) and then $routeCollection->add(...);
replace that with an isolated fresh instance: always instantiate a new
RouteCollection() for the route-finder setup instead of using Di::get or
Di::has, and call add() on that new instance so the test does not modify shared
DI state (look for usage of Di::get(RouteCollection::class),
Di::has(RouteCollection::class), the $routeCollection variable and the add()
call).
🪄 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: f4286641-69ef-4713-961d-487d65e20060
📒 Files selected for processing (15)
src/App/Adapters/WebAppAdapter.phpsrc/App/Stages/LoadModulesStage.phpsrc/App/Traits/WebAppTrait.phpsrc/Console/Commands/OpenApiCommand.phpsrc/Console/Commands/RouteListCommand.phptests/Unit/App/Adapters/WebAppAdapterTest.phptests/Unit/Http/Helpers/HttpHelperTest.phptests/Unit/Http/RequestTest.phptests/Unit/Http/Traits/Request/HttpRequestBodyTest.phptests/Unit/Http/Traits/Request/HttpRequestFileTest.phptests/Unit/Http/Traits/Request/HttpRequestHeaderTest.phptests/Unit/Http/Traits/Request/HttpRequestQueryTest.phptests/Unit/Http/Traits/Request/HttpRequestRouteTest.phptests/Unit/Http/Traits/Request/HttpRequestUrlTest.phptests/Unit/Router/Helpers/RouteHelpersTest.php
💤 Files with no reviewable changes (6)
- tests/Unit/Http/Traits/Request/HttpRequestBodyTest.php
- tests/Unit/Http/Traits/Request/HttpRequestRouteTest.php
- tests/Unit/Http/Traits/Request/HttpRequestFileTest.php
- tests/Unit/Http/Traits/Request/HttpRequestUrlTest.php
- tests/Unit/Http/Traits/Request/HttpRequestQueryTest.php
- tests/Unit/Http/RequestTest.php
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #505 +/- ##
============================================
- Coverage 90.87% 90.70% -0.18%
- Complexity 2927 2930 +3
============================================
Files 255 256 +1
Lines 7708 7711 +3
============================================
- Hits 7005 6994 -11
- Misses 703 717 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ions in contributing guide
Closes #504
Summary by CodeRabbit
Refactor
Bug Fixes