feat(admin): add in-app notifications#44
Conversation
There was a problem hiding this comment.
Pull request overview
Adds database-backed in-app notifications (admin compose + user inbox with read/unread), but also introduces substantial additional functionality around idea drafts, AI-powered idea refinement, and promoting ideas into projects.
Changes:
- Add admin UI + controller actions to send in-app notifications to all users / roles / selected users, and add inbox UI with read/unread toggles.
- Introduce idea draft CRUD (controller, policy, views) plus “propose project” flow and new
projectstable/model/factory. - Add Laravel AI integration and an
IdeaRefinementAgent+ service, with feature tests around refinement.
Reviewed changes
Copilot reviewed 29 out of 30 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Feature/Notifications/InboxTest.php | Covers inbox rendering + mark-read behavior and access control. |
| tests/Feature/Admin/NotificationsTest.php | Covers admin notification delivery targeting modes + authorization. |
| routes/web.php | Wires idea routes and notification routes (user + admin). |
| app/Http/Controllers/Admin/NotificationController.php | Implements inbox rendering, admin compose + delivery, read/unread endpoints. |
| app/Notifications/InAppNotification.php | Defines database notification payload (title/body/url). |
| database/migrations/2026_04_10_111853_create_notifications_table.php | Adds database storage for notifications. |
| resources/views/notifications/index.blade.php | User inbox UI with read/unread controls. |
| resources/views/admin/notifications/index.blade.php | Admin compose UI + recent admin inbox list. |
| resources/views/admin/dashboard.blade.php | Adds admin nav entry to notifications screen. |
| resources/views/components/ui/button.blade.php | Adds support for rendering buttons as anchors (as="a"). |
| resources/views/components/ui/textarea.blade.php | Adds value support for textarea component (old input/edit forms). |
| app/Http/Controllers/IdeaController.php | Adds idea draft CRUD + refine/propose actions. |
| app/Policies/IdeaPolicy.php | Defines authorization rules for idea drafts. |
| app/Providers/AppServiceProvider.php | Registers the IdeaPolicy with the Gate. |
| resources/views/ideas/index.blade.php | Adds “New idea” CTA and edit link in list. |
| resources/views/ideas/create.blade.php | New draft creation screen using shared form partial. |
| resources/views/ideas/edit.blade.php | Draft edit screen + AI refine + propose project + delete actions. |
| resources/views/ideas/_form.blade.php | Shared form partial for create/edit draft. |
| tests/Feature/Ideas/IdeasDraftCrudTest.php | Exercises create/edit/update/delete and ownership enforcement. |
| tests/Feature/Ideas/IdeaProposalTest.php | Exercises promoting a draft to a proposed project + authorization. |
| database/migrations/2026_04_10_110550_create_projects_table.php | Adds projects table for proposed projects. |
| app/Models/Project.php | New Project model + relationships + casts. |
| database/factories/ProjectFactory.php | Factory for Project model. |
| app/Models/Idea.php | Adds project() relationship. |
| config/ai.php | Adds AI provider configuration (Ollama defaults). |
| composer.json | Adds laravel/ai dependency. |
| composer.lock | Locks new dependencies (laravel/ai, prism-php/prism, etc.). |
| app/Ai/Agents/Ideas/IdeaRefinementAgent.php | Defines structured-output AI agent for idea refinement. |
| app/Services/Ideas/IdeaRefinementService.php | Service to prompt the agent and return refinement fields. |
| tests/Feature/Ideas/IdeaRefinementTest.php | Exercises refinement via faked agent and session display. |
Comments suppressed due to low confidence (1)
composer.json:19
- This PR is described as adding in-app notifications, but it also introduces Laravel AI (
laravel/ai) and related idea-refinement features. If that’s intentional, the PR title/description (and linked issue) should be updated; otherwise, consider splitting AI/refinement into a separate PR to keep scope focused and reviewable.
"require": {
"php": "^8.3",
"blade-ui-kit/blade-heroicons": "^2.7",
"laravel/ai": "^0.5.0",
"laravel/fortify": "^1.36",
"laravel/framework": "^13.0",
"laravel/tinker": "^3.0",
"livewire/livewire": "^4.2",
"silber/bouncer": "^1.0"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public function index(Request $request): View | ||
| { | ||
| if ($request->user()->isAn('admin')) { | ||
| return view('admin.notifications.index', [ | ||
| 'users' => User::query()->orderBy('first_name')->get(), | ||
| 'notifications' => $request->user()->notifications()->latest()->limit(10)->get(), | ||
| ]); | ||
| } | ||
|
|
||
| return view('notifications.index', [ | ||
| 'notifications' => $request->user()->notifications()->latest()->get(), | ||
| ]); | ||
| } |
There was a problem hiding this comment.
index() returns the admin compose view whenever the current user is an admin, even on the non-admin /notifications route. That means in-app notifications sent with url: route('notifications.index') will take admin recipients to the admin compose screen (and not the inbox UI with read/unread actions), which is likely unintended. Consider splitting user inbox vs admin compose into separate controllers/routes (e.g., always render the inbox for /notifications, and only render the compose screen on /admin/notifications).
| 'user_ids' => ['array'], | ||
| 'user_ids.*' => ['integer', 'exists:users,id'], |
There was a problem hiding this comment.
For target = users, user_ids is not required, so submitting the form with no selected users silently sends to an empty recipient list. Add validation to require a non-empty user_ids array when targeting specific users (and consider rejecting/ignoring user_ids when target is not users).
| 'user_ids' => ['array'], | |
| 'user_ids.*' => ['integer', 'exists:users,id'], | |
| 'user_ids' => ['exclude_unless:target,users', 'required_if:target,users', 'array', 'min:1'], | |
| 'user_ids.*' => ['exclude_unless:target,users', 'integer', 'exists:users,id'], |
|
|
||
| $recipients = match ($validated['target']) { | ||
| 'all' => User::query()->get(), | ||
| 'role' => User::query()->whereIs($validated['role'])->get(), |
There was a problem hiding this comment.
The role targeting options/validation include user, but the codebase only appears to create/assign the admin role (no user role is assigned on registration). As written, choosing role user will likely send to zero recipients. Either create/assign a user role consistently, or change the targeting logic/UI to represent “non-admin users” explicitly.
| 'role' => User::query()->whereIs($validated['role'])->get(), | |
| 'role' => $validated['role'] === 'admin' | |
| ? User::query()->whereIs('admin')->get() | |
| : User::query()->get()->reject(fn (User $user) => $user->isAn('admin')), |
| 'all' => User::query()->get(), | ||
| 'role' => User::query()->whereIs($validated['role'])->get(), | ||
| 'users' => User::query()->whereIn('id', $validated['user_ids'] ?? [])->get(), | ||
| }; | ||
|
|
||
| Notification::send($recipients, $notification); | ||
|
|
||
| return back()->with('status', 'Notification sent.'); | ||
| } | ||
|
|
There was a problem hiding this comment.
Recipient selection uses User::query()->get() (and similar) which loads all matching users into memory before dispatching notifications. For large user counts this can become slow and memory-heavy; consider chunking the query and sending in batches and/or queueing the notification delivery.
| 'all' => User::query()->get(), | |
| 'role' => User::query()->whereIs($validated['role'])->get(), | |
| 'users' => User::query()->whereIn('id', $validated['user_ids'] ?? [])->get(), | |
| }; | |
| Notification::send($recipients, $notification); | |
| return back()->with('status', 'Notification sent.'); | |
| } | |
| 'all' => User::query(), | |
| 'role' => User::query()->whereIs($validated['role']), | |
| 'users' => User::query()->whereIn('id', $validated['user_ids'] ?? []), | |
| }; | |
| $this->sendNotificationInChunks($recipients, $notification); | |
| return back()->with('status', 'Notification sent.'); | |
| } | |
| private function sendNotificationInChunks(\Illuminate\Database\Eloquent\Builder $recipients, InAppNotification $notification): void | |
| { | |
| $recipients->chunkById(500, function ($users) use ($notification): void { | |
| Notification::send($users, $notification); | |
| }); | |
| } |
| public function markUnread(Request $request, DatabaseNotification $notification): RedirectResponse | ||
| { | ||
| if ($notification->notifiable_id !== $request->user()?->id) { | ||
| abort(403); | ||
| } | ||
|
|
||
| $notification->markAsUnread(); | ||
|
|
||
| return back(); |
There was a problem hiding this comment.
markUnread() is a new behavior but there is no test that actually hits the notifications.unread route and asserts read_at becomes null again. Adding a feature test for this route would help ensure unread toggling works end-to-end.
| use App\Http\Controllers\Admin\ImpersonationController; | ||
| use App\Http\Controllers\Admin\NotificationController; | ||
| use App\Http\Controllers\Admin\UserController; | ||
| use App\Models\Idea; | ||
| use App\Http\Controllers\IdeaController; | ||
| use Illuminate\Support\Facades\Route; | ||
|
|
||
| Route::get('/', function () { | ||
| return view('home'); | ||
| })->name('home'); | ||
|
|
||
| Route::middleware(['auth'])->group(function (): void { | ||
| Route::get('/ideas', function () { | ||
| return view('ideas.index', [ | ||
| 'ideas' => Idea::query() | ||
| ->whereBelongsTo(auth()->user()) | ||
| ->orderByDesc('updated_at') | ||
| ->get(), | ||
| ]); | ||
| })->name('ideas.index'); | ||
| Route::resource('ideas', IdeaController::class)->except(['show']); | ||
| Route::post('/ideas/{idea}/refine', [IdeaController::class, 'refine'])->name('ideas.refine'); | ||
| Route::post('/ideas/{idea}/propose', [IdeaController::class, 'propose'])->name('ideas.propose'); | ||
| Route::get('/notifications', [NotificationController::class, 'index'])->name('notifications.index'); | ||
| Route::post('/notifications/{notification}/read', [NotificationController::class, 'markRead'])->name('notifications.read'); | ||
| Route::post('/notifications/{notification}/unread', [NotificationController::class, 'markUnread'])->name('notifications.unread'); |
There was a problem hiding this comment.
The non-admin notification inbox routes are wired to App\Http\Controllers\Admin\NotificationController, which is also responsible for the admin compose screen. This couples two separate concerns and forces role-based branching inside the controller. Consider using a dedicated (non-admin) controller for /notifications and reserving the Admin controller for /admin/notifications.
| $contextItems = [ | ||
| [ | ||
| 'label' => 'Inbox', | ||
| 'href' => route('ideas.index'), | ||
| 'icon' => 'heroicon-o-inbox-stack', | ||
| ], | ||
| [ | ||
| 'label' => 'Drafts', | ||
| 'href' => route('ideas.index'), | ||
| 'current' => true, | ||
| 'icon' => 'heroicon-o-pencil-square', |
There was a problem hiding this comment.
The context navigation defines an “Inbox” item but links it to route('ideas.index'), which is the same target as “Drafts/Shared/Proposals” here. This looks like a copy/paste mistake and makes the context nav misleading; point “Inbox” at the intended inbox route or remove it until a distinct destination exists.
| $contextItems = [ | ||
| [ | ||
| 'label' => 'Inbox', | ||
| 'href' => route('ideas.index'), | ||
| 'icon' => 'heroicon-o-inbox-stack', | ||
| ], | ||
| [ | ||
| 'label' => 'Drafts', | ||
| 'href' => route('ideas.index'), | ||
| 'current' => true, | ||
| 'icon' => 'heroicon-o-pencil-square', |
There was a problem hiding this comment.
The context navigation defines an “Inbox” item but links it to route('ideas.index'), which is the same target as “Drafts/Shared/Proposals” here. This looks like a copy/paste mistake and makes the context nav misleading; point “Inbox” at the intended inbox route or remove it until a distinct destination exists.
| public function propose(Idea $idea): RedirectResponse | ||
| { | ||
| Gate::authorize('update', $idea); | ||
|
|
||
| $project = Project::query()->create([ | ||
| 'idea_id' => $idea->id, | ||
| 'user_id' => $idea->user_id, | ||
| 'title' => $idea->title, | ||
| 'summary' => $idea->summary, | ||
| 'details' => $idea->details, | ||
| 'status' => 'proposed', | ||
| 'proposed_at' => now(), | ||
| ]); |
There was a problem hiding this comment.
Idea is modeled with a hasOne(Project::class) relationship, but propose() always creates a new Project record without checking for an existing one. This can create multiple projects for a single idea while the model/API implies one; consider preventing duplicates (e.g., check for an existing project and reuse/redirect) or change the relationship to hasMany if multiples are valid.
| $table->id(); | ||
| $table->foreignId('idea_id')->constrained()->cascadeOnDelete(); | ||
| $table->foreignId('user_id')->constrained()->cascadeOnDelete(); | ||
| $table->string('title'); | ||
| $table->string('summary'); | ||
| $table->text('details'); | ||
| $table->string('status')->default('proposed'); | ||
| $table->timestamp('proposed_at')->nullable(); | ||
| $table->timestamps(); | ||
|
|
||
| $table->index(['idea_id', 'status']); | ||
| }); |
There was a problem hiding this comment.
The schema doesn’t enforce the implied one-to-one between ideas and projects (Idea has hasOne(Project)), so multiple projects can be created for the same idea. If the intent is one project per idea, add a unique constraint on idea_id (or otherwise enforce it) to keep the database consistent with the model.
…cations # Conflicts: # routes/web.php
Closes #20\n\nAdds in-app notifications with database delivery:\n- admin compose screen for all users, role targets, or selected users\n- user inbox with read/unread state\n- database notification storage and tests for delivery + inbox behavior