feat: add Laravel service provider for zero-config setup#9
feat: add Laravel service provider for zero-config setup#9jordanpartridge merged 2 commits intomasterfrom
Conversation
Fixes #3 - Add PrServiceProvider with auto-discovery support - Create publishable config/pr.php configuration file - Auto-configure PullRequests facade on Laravel boot - Read token from config('services.github.token') or GITHUB_TOKEN env - Exclude service provider from PHPStan analysis (Laravel not required)
|
Warning Rate limit exceeded@jordanpartridge has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 37 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 (2)
WalkthroughIntroduces Laravel service provider integration for automatic dependency injection setup. Registers Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 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 |
![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)
phpstan.neon (1)
5-6: Excludingsrc/PrServiceProvider.phpis pragmatic, but it also blinds static analysis for shipped code.
If Laravel integration is important, consider analyzing it with a separate PHPStan config that includes Laravel stubs/deps (while keeping the default config framework-agnostic), rather than excluding entirely.composer.json (1)
46-52: Laravel auto-discovery provider registration looks right; consider adding a Composersuggestfor Laravel/Illuminate.
Sincesrc/PrServiceProvider.phpdepends on Laravel, adding something like"illuminate/support": "Required for Laravel service provider integration"undersuggesthelps non-Laravel consumers understand the optional dependency.src/PrServiceProvider.php (1)
38-40: Consider removing (or rethinking) theGitHubPrService::classsingleton binding.
BindingGitHubPrService::classto whateverPrServiceInterfaceresolves to can become misleading if you ever swap implementations; consumers asking forGitHubPrServicegenerally expect the concrete type.- $this->app->singleton(GitHubPrService::class, function ($app) { - return $app->make(PrServiceInterface::class); - }); + // Optional: only bind the interface; consumers should depend on PrServiceInterface.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
composer.json(1 hunks)config/pr.php(1 hunks)phpstan.neon(1 hunks)src/PrServiceProvider.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.php: Usedeclare(strict_types=1)at the top of all PHP files
Apply Laravel Pint withlaravelpreset for code formatting
Files:
src/PrServiceProvider.phpconfig/pr.php
src/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
Use namespace
ConduitUI\Prfor all classes in the library
Files:
src/PrServiceProvider.php
🧠 Learnings (2)
📚 Learning: 2025-12-12T15:34:14.754Z
Learnt from: CR
Repo: conduit-ui/pr PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-12T15:34:14.754Z
Learning: Applies to src/**/*.php : Use namespace `ConduitUI\Pr` for all classes in the library
Applied to files:
composer.jsonsrc/PrServiceProvider.php
📚 Learning: 2025-12-12T15:34:14.754Z
Learnt from: CR
Repo: conduit-ui/pr PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-12T15:34:14.754Z
Learning: Require connector setup via `PullRequests::setConnector()` with a `ConduitUi\GitHubConnector\Connector` instance before using static methods
Applied to files:
src/PrServiceProvider.php
🔇 Additional comments (2)
config/pr.php (1)
1-20: LGTM: strict types + simple config surface.
The config file is minimal and matches the “zero-config viaGITHUB_TOKEN” story; fallback toservices.github.tokencan remain provider-driven.src/PrServiceProvider.php (1)
19-22: Config merge path looks correct for a package layout (src/../config).
No changes requested here.
| if ($this->app->runningInConsole()) { | ||
| $this->publishes([ | ||
| __DIR__.'/../config/pr.php' => config_path('pr.php'), | ||
| ], 'pr-config'); | ||
| } | ||
|
|
||
| // Auto-configure the PullRequests facade | ||
| $service = $this->app->make(PrServiceInterface::class); | ||
| PullRequests::setService($service); | ||
| } |
There was a problem hiding this comment.
Avoid eager resolution in boot()—missing token shouldn’t break the whole Laravel app / artisan.
Right now boot() always resolves PrServiceInterface, so a missing token throws during app boot, even if the package isn’t used. Prefer lazy configuration (or only set up the facade when token is present).
public function boot(): void
{
if ($this->app->runningInConsole()) {
$this->publishes([
__DIR__.'/../config/pr.php' => config_path('pr.php'),
], 'pr-config');
}
- // Auto-configure the PullRequests facade
- $service = $this->app->make(PrServiceInterface::class);
- PullRequests::setService($service);
+ // Auto-configure the PullRequests facade (lazy / non-fatal if unconfigured)
+ $token = $this->app['config']->get('pr.github.token')
+ ?: $this->app['config']->get('services.github.token')
+ ?: env('GITHUB_TOKEN');
+
+ if ($token) {
+ PullRequests::setService($this->app->make(PrServiceInterface::class));
+ }
}🤖 Prompt for AI Agents
In src/PrServiceProvider.php around lines 48 to 57, boot() eagerly resolves
PrServiceInterface which throws if the token is missing; instead avoid calling
$this->app->make() during boot — either guard resolution with a config/token
check (e.g. if (config('pr.token')) {
PullRequests::setService($this->app->make(PrServiceInterface::class)); }) or
defer wiring by using the container hooks (e.g.
$this->app->afterResolving(PrServiceInterface::class, fn($service) =>
PullRequests::setService($service)); or $this->app->resolving(...)) so the
service is only resolved when available and the app can boot without the token.
- Extract token resolution to helper method that properly handles empty strings (trim + explicit check instead of ?? operator) - Make boot() lazy - only configures facade if token is available, preventing app crashes when PR package is installed but not used - Add explanatory comment for PHPStan exclusion Addresses CodeRabbit review comments on empty string handling and eager resolution in boot().
1 participant
Fixes #3
Summary
PrServiceProviderwith Laravel package auto-discoveryconfig/pr.phpconfiguration filePullRequestsfacade on Laravel bootconfig('services.github.token')orGITHUB_TOKENenv variableImplementation Details
Service Provider (
src/PrServiceProvider.php)PrServiceInterfaceandGitHubPrServiceas singletonsPullRequestsfacade viasetService()on bootConfiguration (
config/pr.php)GITHUB_TOKENenvironment variableconfig('services.github.token')Auto-Discovery
extra.laravel.providerstocomposer.jsonfor automatic service provider registrationPHPStan
PrServiceProvider.phpfrom static analysis since Laravel is an optional dependencyUsage in Laravel
After installing, Laravel users get zero-config setup:
Users can optionally publish the config:
Test Plan
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.