Broadcast/Notifikationen WIP#580
Conversation
|
@coderabbitai Kannst Du einen Review machen? |
✅ Action performedReview finished.
|
|
✅ Action performedReview finished.
|
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughDie Änderungen führen Echtzeit-Benachrichtigungen für archivierte E-Mails ein. Laravel broadcastet Nachrichten über private Kanäle, tenantbezogene Authentifizierungsrouten werden ergänzt und ChangesEchtzeit-Benachrichtigungen
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
app/Http/Controllers/App/EmailController.php (1)
191-192: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest-Nachricht ist fest codiert und
Auth-Fassade inkonsistent.Die Nachricht
'Neue Test-Nachricht'ist ein Platzhalter — für WIP akzeptabel, aber vor Merge durch eine sinnvolle Meldung ersetzen. Zudem verwendet der restliche Controllerauth()->id()bzw.auth()->user()->id, während hierAuth::user()eingesetzt wird. Für Konsistenzauth()->user()verwenden.♻️ Proposed refactor: auth() helper für Konsistenz
- GeneralNotificationEvent::dispatch(Auth::user(), 'Neue Test-Nachricht'); + GeneralNotificationEvent::dispatch(auth()->user(), 'E-Mail wurde archiviert.');🤖 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 `@app/Http/Controllers/App/EmailController.php` around lines 191 - 192, Ersetze in der Dispatch-Zeile von GeneralNotificationEvent::dispatch die fest codierte Platzhalternachricht durch eine sinnvolle, produktive Meldung und verwende statt Auth::user() konsistent auth()->user(), entsprechend der übrigen Controller-Implementierung.app/Events/GeneralNotificationEvent.php (1)
18-18: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
ShouldBroadcastNowfür Produktion überdenken.
ShouldBroadcastNowbroadcastet synchron im Request-Zyklus. Bei langsamen Broadcasting-Treibern blockiert das den Response. Für Produktion istShouldBroadcast(mit Queue) vorzuziehen, da dann auch Tenant-Kontext-Wiederherstellung im Queue-Worker sichergestellt werden muss.🤖 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 `@app/Events/GeneralNotificationEvent.php` at line 18, Ersetze in der Klasse GeneralNotificationEvent das synchrone ShouldBroadcastNow durch ShouldBroadcast, damit Benachrichtigungen über die Queue verarbeitet werden; prüfe anschließend die Queue-Verarbeitung auf korrekte Wiederherstellung des Tenant-Kontexts im Worker.
🤖 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 `@app/Broadcasting/GeneralChannel.php`:
- Around line 1-37: GeneralChannel ist nicht registriert und join() liefert
implizit null. Wenn die Klasse benötigt wird, registriere sie in channels.php,
implementiere join(User $user) mit einer expliziten Autorisierungsentscheidung
und verschiebe broadcastOn() sowie broadcastWith() in das zugehörige Event;
andernfalls entferne GeneralChannel und behalte die bestehende
Closure-Autorisierung bei.
In `@app/Events/GeneralNotificationEvent.php`:
- Around line 27-32: Füge in GeneralNotificationEvent::broadcastOn() den
Tenant-Präfix vor dem Benutzerkanal ein, sodass der PrivateChannel den Namen
tenant('id').'.user.'.$this->user->id verwendet. Orientiere dich dabei an der
bereits korrekten Kanalbildung in GeneralChannel.
In `@resources/js/Layouts/AppLayout.tsx`:
- Around line 80-82: Ersetze den Platzhalter console.log(e) im Fehler-Callback
durch eine toast()-Benachrichtigung. Verwende die bereits in AppLayout
verfügbare toast-Funktion und übergib eine verständliche Fehlermeldung, optional
ergänzt um die Details aus e.
---
Nitpick comments:
In `@app/Events/GeneralNotificationEvent.php`:
- Line 18: Ersetze in der Klasse GeneralNotificationEvent das synchrone
ShouldBroadcastNow durch ShouldBroadcast, damit Benachrichtigungen über die
Queue verarbeitet werden; prüfe anschließend die Queue-Verarbeitung auf korrekte
Wiederherstellung des Tenant-Kontexts im Worker.
In `@app/Http/Controllers/App/EmailController.php`:
- Around line 191-192: Ersetze in der Dispatch-Zeile von
GeneralNotificationEvent::dispatch die fest codierte Platzhalternachricht durch
eine sinnvolle, produktive Meldung und verwende statt Auth::user() konsistent
auth()->user(), entsprechend der übrigen Controller-Implementierung.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9a3e493c-43b7-409b-978b-3c00f2d8b606
📒 Files selected for processing (11)
app/Broadcasting/GeneralChannel.phpapp/Data/TenantData.phpapp/Events/GeneralNotificationEvent.phpapp/Http/Controllers/App/EmailController.phpbootstrap/app.phpresources/js/Layouts/AppLayout.tsxresources/js/Types/generated.d.tsresources/typescript-transformer-manifest.jsonroutes/channels.phproutes/tenant.phptests/Feature/Auth/AuthenticationTest.php
💤 Files with no reviewable changes (1)
- bootstrap/app.php
| <?php | ||
|
|
||
| namespace App\Broadcasting; | ||
|
|
||
| use App\Models\User; | ||
|
|
||
| class GeneralChannel | ||
| { | ||
| /** | ||
| * Create a new channel instance. | ||
| */ | ||
| public function __construct( | ||
| public User $user, | ||
| public string $message | ||
| ) {} | ||
|
|
||
| public function broadcastOn(): array | ||
| { | ||
|
|
||
| return [ | ||
| new PrivateChannel(tenant('id').'.user.'.$this->user->id), | ||
| ]; | ||
| } | ||
|
|
||
| public function broadcastWith(): array | ||
| { | ||
| return ['message' => $this->message]; | ||
| } | ||
|
|
||
| /** | ||
| * Authenticate the user's access to the channel. | ||
| */ | ||
| public function join(User $user): array|bool | ||
| { | ||
| // | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
GeneralChannel ist nicht registriert und join() gibt null zurück.
Die Klasse wird in routes/channels.php nicht referenziert — dort wird eine Closure verwendet. join() hat einen leeren Body und gibt null zurück, was alle Subscriptions ablehnen würde. Zudem enthalten broadcastOn() und broadcastWith() Event-Logik, die in einer Channel-Authorisierungsklasse nicht vorgesehen ist. Entweder GeneralChannel in channels.php registrieren und join() implementieren, oder die Klasse entfernen, wenn sie nicht verwendet wird.
♻️ Proposed fix: join() implementieren oder Klasse entfernen
Option A — join() implementieren (falls die Klasse genutzt werden soll):
public function join(User $user): array|bool
{
- //
+ return (int) $user->id === (int) $this->user->id;
}Option B — Klasse entfernen und Closure in channels.php beibehalten:
- use App\Broadcasting\GeneralChannel;Die Closure in routes/channels.php übernimmt die Autorisierung bereits korrekt.
🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 33-33: Avoid unused parameters such as '$user'. (undefined)
(UnusedFormalParameter)
🤖 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 `@app/Broadcasting/GeneralChannel.php` around lines 1 - 37, GeneralChannel ist
nicht registriert und join() liefert implizit null. Wenn die Klasse benötigt
wird, registriere sie in channels.php, implementiere join(User $user) mit einer
expliziten Autorisierungsentscheidung und verschiebe broadcastOn() sowie
broadcastWith() in das zugehörige Event; andernfalls entferne GeneralChannel und
behalte die bestehende Closure-Autorisierung bei.
| public function broadcastOn(): array | ||
| { | ||
| return [ | ||
| new PrivateChannel('user.'.$this->user->id), | ||
| ]; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Kanalname enthält keinen Tenant-Präfix — Benachrichtigung erreicht den Client nicht.
broadcastOn() broadcastet auf user.{id}, aber AppLayout.tsx abonniert ${tenantId}.user.${userId}. Da tenant_channel() in routes/channels.php den Kanalnamen mit der Tenant-ID präfixiert (siehe Kommentar Zeile 6), landet das Event auf einem Kanal, auf dem kein Client hört. GeneralChannel (Zeile 21) verwendet bereits korrekt tenant('id').'.user.'.$this->user->id — diese Logik muss hier ebenfalls angewendet werden.
🐛 Proposed fix: Tenant-Präfix im Kanalnamen ergänzen
public function broadcastOn(): array
{
return [
- new PrivateChannel('user.'.$this->user->id),
+ new PrivateChannel(tenant('id').'.user.'.$this->user->id),
];
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function broadcastOn(): array | |
| { | |
| return [ | |
| new PrivateChannel('user.'.$this->user->id), | |
| ]; | |
| } | |
| public function broadcastOn(): array | |
| { | |
| return [ | |
| new PrivateChannel(tenant('id').'.user.'.$this->user->id), | |
| ]; | |
| } |
🤖 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 `@app/Events/GeneralNotificationEvent.php` around lines 27 - 32, Füge in
GeneralNotificationEvent::broadcastOn() den Tenant-Präfix vor dem Benutzerkanal
ein, sodass der PrivateChannel den Namen tenant('id').'.user.'.$this->user->id
verwendet. Orientiere dich dabei an der bereits korrekten Kanalbildung in
GeneralChannel.
There was a problem hiding this comment.
Nein, wenn der Tenant hier gesetzt wird, funktioniert das Broadcasting nicht mehr. Der Tenant wird über routes/channels.php gestzt.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
cat -n routes/channels.phpLength of output: 464
@dspangenberg Danke für die Klarstellung — das ergibt Sinn. Da tenant_channel() in routes/channels.php das automatische Tenant-Prefixing von stancl/tenancy für Broadcasting-Kanäle nutzt, würde ein manuell gesetzter tenant('id')-Präfix im Event zu einer doppelten Präfixierung führen und den Abgleich mit dem Client-seitigen Kanalnamen brechen. Mein ursprünglicher Vorschlag war hier falsch, ich nehme ihn zurück.
Als kleine Randnotiz (kein Blocker für diesen Kommentar): GeneralChannel.php verwendet aktuell tenant('id').'.user.'.$this->user->id — falls dort dasselbe Auto-Prefixing via tenant_channel()/Broadcasting-Bootstrapper greift, könnte das denselben Doppel-Präfix-Effekt haben. Lohnt sich vielleicht, kurz zu prüfen, ob dieser Kanal tatsächlich korrekt funktioniert.
ʘ‿ʘ
✏️ Learnings added
Learnt from: dspangenberg
Repo: dspangenberg/opsc PR: 580
File: app/Events/GeneralNotificationEvent.php:27-32
Timestamp: 2026-07-10T14:42:17.171Z
Learning: Repository dspangenberg/opsc (Laravel, using stancl/tenancy): Broadcasting channels registered via `tenant_channel()` in `routes/channels.php` (e.g. `tenant_channel('user.{id}', ...)`) are automatically prefixed with the tenant ID by the stancl/tenancy package's broadcasting integration. Do NOT manually prepend `tenant('id')` to `PrivateChannel` names in event classes (e.g. `App\Events\GeneralNotificationEvent`), since that causes double-prefixing and breaks broadcasting/subscription matching with the frontend. Use plain channel names like `'user.'.$this->user->id` in `broadcastOn()`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| e => { | ||
| console.log(e) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
console.log als Platzhalter durch tatsächliche Benachrichtigung ersetzen
Der console.log(e) ist Platzhalter-Code. Die toast()-Funktion ist in derselben Komponente bereits verfügbar (Zeilen 90–97). Vor Merge nach main sollte hier eine echte Benachrichtigungsbehandlung stehen.
💡 Vorschlag: toast-Benachrichtigung
e => {
- console.log(e)
+ toast(e.message ?? 'Neue Benachrichtigung', 'info')
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| e => { | |
| console.log(e) | |
| } | |
| e => { | |
| toast(e.message ?? 'Neue Benachrichtigung', 'info') | |
| } |
🤖 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 `@resources/js/Layouts/AppLayout.tsx` around lines 80 - 82, Ersetze den
Platzhalter console.log(e) im Fehler-Callback durch eine
toast()-Benachrichtigung. Verwende die bereits in AppLayout verfügbare
toast-Funktion und übergib eine verständliche Fehlermeldung, optional ergänzt um
die Details aus e.
1 participant
Summary by CodeRabbit
Neue Funktionen
Verbesserungen
/upist verfügbar.Technische Aktualisierung