Release v1.7.0 #50
Release v1.7.0 #50
Conversation
feat: Add workspaces for users
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
📝 WalkthroughWalkthroughAdds workspace multi-tenancy (models, migrations, controllers, policies, views) and workspace-scoped Pro checks; scheduled marketing-trigger commands with events/listeners and webhook deliveries; admin user management and impersonation; Grafana Faro RUM and OpenTelemetry bootstrap; k6 load tests; many view/test updates and related migrations. Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler
participant CheckCmd as CheckMarketingTriggers
participant DB as Database
participant Cache
participant Event as Event Bus
participant Listener as SendUserInactiveNotification
participant Webhook as External Webhook
Scheduler->>CheckCmd: run hourly
CheckCmd->>DB: query users matching condition (e.g., inactive 30d)
DB-->>CheckCmd: users
loop per user
CheckCmd->>Cache: check dedupe key
Cache-->>CheckCmd: miss
CheckCmd->>Event: dispatch UserInactive(user)
Event->>Listener: handle event
Listener->>DB: read settings.user_inactive_webhook_url
DB-->>Listener: webhook_url
alt webhook_url present
Listener->>Webhook: POST { event, user }
Webhook-->>Listener: 200/other
end
CheckCmd->>Cache: set dedupe key (7d)
end
sequenceDiagram
participant Client
participant API as DisplayController (API)
participant Service as DisplayService
participant DB as Database
participant User as User Model
participant Workspace as Workspace Model
participant Policy as DisplayPolicy
Client->>API: GET /api/displays/{id}/data (with device)
API->>Service: getDisplay(device, displayId)
Service->>DB: load device + user + workspaces
DB-->>Service: device + user + workspaces
Service->>User: get workspace IDs
User-->>Service: [workspace_ids]
Service->>DB: query display where workspace_id IN (...)
DB-->>Service: display
Service->>Workspace: workspace.hasPro() (if pro requested)
Workspace-->>Service: hasPro result
Service->>Policy: authorize(user, display)
Policy->>Workspace: hasMember(user)
Workspace-->>Policy: member?
Policy-->>Service: authorized
Service-->>API: display data
API-->>Client: 200 OK + payload
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
backend/app/Http/Requests/CreateDisplayRequest.php (1)
19-22: Change validation rule fromstringtoulidto match the Workspace model's ID format.Line 38 validates that
workspace_idexists but uses the loosestringtype when Workspace IDs are ULIDs (the model usesHasUlidtrait). While the controller properly validates workspace membership (lines 33–40), the validation rule should accurately reflect the expected data type:-'workspace_id' => 'nullable|string|exists:workspaces,id', +'workspace_id' => 'nullable|ulid|exists:workspaces,id',Note: Authorization is already handled in the controller—
$user->workspaces()->find($workspaceId)ensures the user owns/manages the workspace before creating the display.backend/resources/views/pages/displays/settings.blade.php (1)
6-16: JS will throw for non‑Pro branch (missingsettingsForm/check_in_enabled).
Because the form is only rendered in the@elsebranch, the always-included script should null-check (or be included only when the form exists).Proposed fix (null guards)
<script> // Add any JavaScript for form handling if needed - document.getElementById('settingsForm').addEventListener('submit', function(e) { - // Form will be submitted normally, but we could add validation here if needed - }); - // Show/hide grace period input based on check-in enabled - document.getElementById('check_in_enabled').addEventListener('change', function(e) { - document.getElementById('gracePeriodInput').style.display = this.checked ? 'block' : 'none'; - document.getElementById('checkInMinutesInput').style.display = this.checked ? 'block' : 'none'; - }); + const form = document.getElementById('settingsForm'); + if (form) { + form.addEventListener('submit', function(e) { + // Form will be submitted normally, but we could add validation here if needed + }); + } + + // Show/hide grace period input based on check-in enabled + const checkInEnabled = document.getElementById('check_in_enabled'); + if (checkInEnabled) { + checkInEnabled.addEventListener('change', function(e) { + document.getElementById('gracePeriodInput').style.display = this.checked ? 'block' : 'none'; + document.getElementById('checkInMinutesInput').style.display = this.checked ? 'block' : 'none'; + }); + } </script>Also applies to: 189-200
backend/tests/Feature/DisplaySettingsApiTest.php (1)
13-16: Tests underbackend/tests/**should use Pest for consistency.This file is still PHPUnit-style, while the suite (and this PR) uses Pest elsewhere.
backend/app/Http/Controllers/API/Auth/AuthController.php (1)
54-76: Logging looks reasonable, but double-check PII policy for IP/User-Agent.You’re logging IP + user agent on both success and failure; ensure this matches your retention/privacy requirements.
backend/tests/Feature/API/EventControllerTest.php (1)
17-44: Room fixtures are still inconsistent: some creations omitworkspace_id.Given
backend/database/factories/RoomFactory.phpdoesn’t setworkspace_idby default, the Room creations in “does not cache events…” and “handles errors gracefully” should also passworkspace_id(same as the other tests updated here).Proposed fix
$room = Room::factory()->create([ 'user_id' => $this->user->id, + 'workspace_id' => $this->workspace->id, 'calendar_id' => $this->calendar->id, 'email_address' => 'test@example.com' ]);$room = Room::factory()->create([ 'user_id' => $this->user->id, + 'workspace_id' => $this->workspace->id, 'calendar_id' => $this->calendar->id, 'email_address' => 'test@example.com' ]);Also applies to: 64-73, 135-143, 262-271, 207-215, 325-333
backend/app/Services/GoogleService.php (1)
384-435: HTTP status extraction is unreliable;getCode()returns 0 for Google API exceptions—the entire error classification fails.The code assumes
$e->getCode()contains the HTTP status code, but the Google PHP API client defaults exception$codeto 0. The HTTP status is typically embedded in the JSON error payload (accessible viagetErrors()or parsed from the message), not the exception code. This means$statusCodewill almost always be 0, breaking the 4xx/5xx logic entirely.Additionally, 429 responses should trigger retry logic with exponential backoff and Retry-After headers, not be thrown as user errors. The proper fix requires:
- Extract HTTP status from the exception payload (e.g., via
$e->getErrors()forGoogle\Service\Exception), notgetCode()- Treat 429 as retriable/transient with backoff, not as a user fix
- Properly distinguish actual 4xx user errors (e.g., 403, 404) from retriable and server errors
backend/resources/views/pages/displays/create.blade.php (1)
373-373: Remove debug console.log statement.This debug statement should be removed before merging to production.
🧹 Suggested fix
- console.log(selectedProvider);backend/app/Http/Controllers/DisplayController.php (1)
76-82: Verify account belongs to the target workspace.The provider account validation (Lines 77-82) checks that the account exists but doesn't verify it belongs to the workspace where the display is being created. A user could potentially select an account from workspace A while creating a display in workspace B, creating a cross-workspace security vulnerability.
Add workspace validation after the account is retrieved:
🔒 Suggested fix
+ $account = match ($provider) { + 'outlook' => OutlookAccount::findOrFail($accountId), + 'google' => GoogleAccount::findOrFail($accountId), + 'caldav' => CalDAVAccount::findOrFail($accountId), + default => throw new \InvalidArgumentException('Invalid provider') + }; + $user = auth()->user(); // Get workspace from request, session (selected workspace), or default to primary $workspaceId = $validatedData['workspace_id'] ?? session()->get('selected_workspace_id') ?? $user->primaryWorkspace()?->id; if (!$workspaceId) { return redirect()->back()->with('error', 'No workspace found. Please contact support.'); } // Verify user has access to this workspace $workspace = $user->workspaces()->find($workspaceId); if (!$workspace) { return redirect()->back()->with('error', 'You do not have access to this workspace.'); } + // Verify account belongs to this workspace + if ($account->workspace_id !== $workspaceId) { + return redirect()->back()->with('error', 'The selected account does not belong to this workspace.'); + }
🤖 Fix all issues with AI agents
In @backend/app/Http/Controllers/API/Auth/AuthController.php:
- Around line 29-52: The connect code retrieval and user lookup are unsafe:
replace cache()->get("connect-code:$code") with
cache()->pull("connect-code:$code") to atomically invalidate the code after
first use, and after $connectedUserId = cache()->pull(...) ensure the user
exists (check result of User::find($connectedUserId) and abort/return if null)
before calling $user->primaryWorkspace() or creating a Device (the
Device::firstOrCreate / update logic should only run when $user is non-null).
In @backend/app/Http/Controllers/API/DeviceController.php:
- Around line 29-56: The changeDisplay flow currently allows selecting any
Display in any workspace the user belongs to; restrict this by enforcing the
device's bound workspace when present: if $device->workspace_id is set, limit
the Display lookup to that workspace (e.g., include a where('workspace_id',
$device->workspace_id) condition or intersect $workspaceIds with
[$device->workspace_id]) before calling
Display::query()->whereIn(...)->find($data['display_id']), otherwise fallback to
the existing user-workspaces check; update the authorization/error paths in
DeviceController (changeDisplay) to return a 403/400 when the requested display
is outside the device's workspace.
In @backend/app/Http/Controllers/DashboardController.php:
- Around line 59-70: The log in DashboardController is currently recording PII
(user email); remove or replace the 'email' field in the logger()->info call to
avoid storing plaintext PII—either omit 'email' entirely or substitute a
pseudonymized identifier (e.g., a deterministic hash of $user->email such as
sha256) and ensure the key name reflects that (e.g., 'user_email_hash'), keeping
'user_id' intact; update the logger()->info payload where 'email' is referenced
and any related telemetry/retention docs as needed.
In @backend/app/Http/Controllers/DisplaySettingsController.php:
- Around line 24-27: The Pro-entitlement check uses hasProForCurrentWorkspace()
which validates the session-selected workspace rather than the display's
workspace; update the four controller methods index, update, customization, and
updateCustomization to check Pro against the display's workspace by using
hasProForWorkspace($display->workspace_id) (or equivalent) or move this check
into the DisplayPolicy so the entitlement is evaluated against
$display->workspace_id alongside the existing authorize($display) call; ensure
the check replaces calls to hasProForCurrentWorkspace() so Pro gating matches
the policy authorization context.
In @backend/app/Listeners/SendUserActivatedAfter24hNotification.php:
- Around line 14-25: The handle method in SendUserActivatedAfter24hNotification
synchronously posts to the webhook without timeout or error handling; make the
listener async and resilient by implementing ShouldQueue (queue the listener)
and, inside handle(UserActivatedAfter24h $event), perform the HTTP call with a
timeout (e.g., Http::timeout(...)->post(...)) wrapped in a try/catch that logs
failures and the payload (use process logger or Log::error) and optionally
retries via the queue; ensure you still use UserWebhookData::from($event->user)
for the payload and keep the
config('settings.user_activated_after_24h_webhook_url') guard.
In @backend/app/Listeners/SendUserInactiveNotification.php:
- Around line 14-25: The SendUserInactiveNotification listener currently
performs a blocking Http::post call in the handle(UserInactive $event) method
without error handling, timeout, retries or queueing; update the
SendUserInactiveNotification class to implement
Illuminate\Contracts\Queue\ShouldQueue (or dispatch the HTTP call to a dedicated
queued job) and move the Http::post into that queued job (or mark the listener
as Queueable) so it runs in the background; ensure the HTTP call
(UserWebhookData::from($event->user) payload) uses a timeout and retry/backoff
(e.g., Http::timeout(...)->retry(...)) and wrap the call in try/catch to log
failures instead of letting it block event processing.
In @backend/app/Services/OutlookService.php:
- Around line 453-491: The code treats all 4xx responses as user errors and
throws an exception, which incorrectly includes HTTP 429 (throttling); change
the $isUserError calculation to consider 4xx except 429 (e.g., $isUserError =
$statusCode >= 400 && $statusCode < 500 && $statusCode !== 429) so 429 is
treated as transient (no throw, return null) and ensure the existing thrown
Exception path still uses the "Failed to create Outlook subscription" message so
re-throw detection based on that string continues to work.
In
@backend/database/migrations/2025_12_30_000004_create_workspaces_for_existing_users.php:
- Around line 7-17: The migration currently uses Eloquent models (Workspace,
WorkspaceMember, User, etc.), which is brittle if model definitions change;
switch to the DB Query Builder/raw inserts instead: generate IDs with
Str::ulid(), insert into the workspaces table with
DB::table('workspaces')->insert([...]) and then insert the owner row into
workspace_members via DB::table('workspace_members')->insert([...]); replace all
Workspace::create and WorkspaceMember::create calls in this migration with
equivalent DB::table inserts (including created_at/updated_at and role using
WorkspaceRole::OWNER or its string), and remove model imports so the migration
only depends on schema and not Eloquent model code.
- Around line 27-73: The migration currently performs per-user creations and
updates (User::chunk callback creating Workspace::create,
WorkspaceMember::create, and updates via Display::where..., Device::where...,
Calendar::where..., Room::where..., OutlookAccount::where...,
GoogleAccount::where..., CalDAVAccount::where...) without transactional
boundaries; wrap the entire per-user migration logic inside a DB::transaction
(or call DB::transaction(fn() => { ... }) within the User::chunk loop) so that
creating the workspace, adding the owner via WorkspaceMember::create, and all
update(...) calls are executed atomically and will roll back on exception;
ensure any exceptions are rethrown so the transaction can rollback and the
migration fails visibly.
In @backend/resources/views/components/displays/table-row.blade.php:
- Around line 31-34: The template in table-row.blade.php uses dynamically
constructed Tailwind classes (e.g., bg-{{ $display->status->color() }}-50,
text-{{ $display->status->color() }}-700, ring-{{ $display->status->color()
}}-600) which Tailwind's scanner won't detect; either add a safelist in
backend/tailwind.config.js that covers those combinations (e.g., a regex for
bg|text|ring with the known colors and shades) or change the DisplayStatus enum
to return full class strings (add methods like bgClass(), textClass(),
ringClass() on DisplayStatus that return complete class names for each state)
and then update the blade to use those methods (e.g., use {{
$display->status->bgClass() }} etc.) so the outputted classes are static and
included in the compiled CSS.
In @backend/tests/Feature/DisplaySettingsApiTest.php:
- Around line 22-32: The test currently calls $user->primaryWorkspace() and
immediately uses $workspace->id which can null-deref; ensure primaryWorkspace()
exists by either asserting it (e.g., $this->assertNotNull($workspace =
$user->primaryWorkspace())) or explicitly creating/attaching a workspace for
$user before using it, then pass that $workspace->id into Display::factory() and
Device::factory() (apply same fix to the other occurrences around lines 65-75
and 92-102).
In @backend/tests/Unit/DisplaySettingsTest.php:
- Around line 13-19: The tests call $user->primaryWorkspace() which is nullable;
add a null-safety step before accessing $workspace->id by replacing the direct
use with a guarded assignment: call primaryWorkspace(), and if it returns null
create or fetch a fallback workspace (e.g., via Workspace factory or the user's
workspace-creation helper) and assign it to $workspace so $workspace->id is
always valid. Apply this change to each test that currently uses
primaryWorkspace() (the blocks creating Display via Display::factory() in the
three tests) so the tests no longer assume a non-null primary workspace.
🟡 Minor comments (8)
backend/app/Http/Controllers/API/ApiController.php-21-33 (1)
21-33: Inconsistency between comment and implementation for auth error logging.The comment on Line 21 states the code will "skip 404s and auth errors to avoid noise," but the condition
($code >= 400 && $code < 404)actually logs 401 (Unauthorized) and 403 (Forbidden), which are authentication/authorization errors. This creates unexpected logging behavior.Consider either:
- Excluding 401 and 403 from logging if auth errors should truly be skipped
- Updating the comment to accurately reflect what's being logged
Option 1: Skip auth errors as stated in the comment
- // Log API errors for observability (skip 404s and auth errors to avoid noise) - if ($code >= 500 || ($code >= 400 && $code < 404)) { + // Log API errors for observability (skip 404s and auth errors to avoid noise) + if ($code >= 500 || $code === 400 || $code === 402) { logger()->warning('API error response', [Option 2: Update comment to reflect actual behavior
- // Log API errors for observability (skip 404s and auth errors to avoid noise) + // Log API errors for observability (skip 404s to avoid noise) if ($code >= 500 || ($code >= 400 && $code < 404)) { logger()->warning('API error response', [backend/bootstrap/opentelemetry.php-1-12 (1)
1-12: Usegetenv()to robustly check for existing environment variables before overriding.In common PHP deployments, the
$_ENVsuperglobal may not be populated even when environment variables are set (depending on php.ini'svariables_ordersetting). The current code checks only$_ENV, which can cause it to override an already-configuredOTEL_PHP_DISABLED_INSTRUMENTATIONSvalue.Use
getenv()instead, which reliably accesses environment variables regardless of php.ini configuration:Proposed fix
-if (!extension_loaded('opentelemetry') && !isset($_ENV['OTEL_PHP_DISABLED_INSTRUMENTATIONS'])) { - $_ENV['OTEL_PHP_DISABLED_INSTRUMENTATIONS'] = 'laravel'; - putenv('OTEL_PHP_DISABLED_INSTRUMENTATIONS=laravel'); +if (!extension_loaded('opentelemetry')) { + $current = getenv('OTEL_PHP_DISABLED_INSTRUMENTATIONS'); + if ($current === false || $current === '') { + $_ENV['OTEL_PHP_DISABLED_INSTRUMENTATIONS'] = 'laravel'; + putenv('OTEL_PHP_DISABLED_INSTRUMENTATIONS=laravel'); + } }backend/config/faro.php-43-43 (1)
43-43: Default API key poses a security risk if not overridden in production.The default value
'faro-secret-key'could inadvertently be used in production ifFARO_API_KEYis not set. Consider either:
- Setting the default to
nulland validating that a key is set when Faro is enabled- Adding runtime validation in the Faro blade component to warn/fail if the default key is detected in non-local environments
backend/FARO_SETUP.md-160-162 (1)
160-162: Use proper markdown link syntax for URL.- - Visit http://localhost:9090/targets + - Visit <http://localhost:9090/targets>backend/FARO_SETUP.md-103-107 (1)
103-107: Use proper markdown link syntax for URLs.Per markdownlint, bare URLs should be wrapped in angle brackets or use markdown link syntax.
-1. Open Grafana at http://localhost:3000 +1. Open Grafana at <http://localhost:3000>backend/resources/views/components/displays/table-row.blade.php-10-28 (1)
10-28: Add null check for calendar relationship.The code accesses
$display->calendar->outlookAccount,$display->calendar->googleAccount, etc. without checking if$display->calendarexists. If a display somehow has no calendar, this will throw an error.🐛 Suggested fix
<td class="whitespace-nowrap px-3 py-4 text-sm text-gray-500"> <div class="flex flex-col gap-1"> + @if($display->calendar) @if($display->calendar->outlookAccount) <div class="flex items-center"> <x-icons.microsoft class="h-4 w-4 text-gray-900 mr-2" /> <span class="text-gray-900">{{ $display->calendar->outlookAccount->name }}</span> </div> @endif {{-- ... other account checks ... --}} <div class="text-gray-500">{{ $display->calendar->name }}</div> + @endif </div> </td>backend/app/Http/Controllers/AdminController.php-385-398 (1)
385-398:displays_countmay not be loaded.Line 389 uses
$user->displays_count, but the user model is loaded withload()(eager loading relationships) on line 370, notloadCount(). This will causedisplays_countto be null. You should useloadCount('displays')or access the count differently.Proposed fix
// Load user relationships for display $user->load([ 'outlookAccounts', 'googleAccounts', 'caldavAccounts', 'displays', 'devices', 'workspaces', 'subscriptions' => function($query) { $query->where(function($q) { $q->whereNull('ends_at') ->orWhere('ends_at', '>', now()); })->orderByDesc('created_at'); }, - ]); + ])->loadCount('displays');backend/app/Console/Commands/CheckMarketingTriggers.php-104-133 (1)
104-133: Users with nulllast_activity_atare excluded despite comment suggesting otherwise.The comment on line 111 states "User's last activity is within the window (or null)", but the query on line 113 uses
whereNotNull('last_activity_at'), which excludes users who have never had any activity recorded. If users with nulllast_activity_atshould be considered passive, this needs adjustment.Proposed fix to include users with null last_activity_at
$users = User::whereNull('deleted_at') ->where(function ($query) use ($cutoffDate, $previousCutoffDate) { - // User's last activity is within the window (or null) - $query->where(function ($q) use ($cutoffDate, $previousCutoffDate) { - $q->whereNotNull('last_activity_at') - ->where('last_activity_at', '<=', $cutoffDate) - ->where('last_activity_at', '>', $previousCutoffDate); - }); + // User's last activity is within the window + $query->where(function ($q) use ($cutoffDate, $previousCutoffDate) { + $q->whereNotNull('last_activity_at') + ->where('last_activity_at', '<=', $cutoffDate) + ->where('last_activity_at', '>', $previousCutoffDate); + }) + // Or user has no activity recorded and was created before the cutoff + ->orWhere(function ($q) use ($cutoffDate, $previousCutoffDate) { + $q->whereNull('last_activity_at') + ->where('created_at', '<=', $cutoffDate) + ->where('created_at', '>', $previousCutoffDate); + }); })
🧹 Nitpick comments (40)
backend/resources/views/vendor/pagination/tailwind.blade.php (1)
27-39: Consider using a single translation string with placeholders for better i18n support.The current implementation breaks the sentence into multiple translation calls, which makes it difficult to translate to languages with different word orders. Consider using a single translation string with placeholders instead.
♻️ Proposed refactor for better i18n support
- <p class="text-sm text-gray-700 leading-5"> - {!! __('Showing') !!} - @if ($paginator->firstItem()) - <span class="font-medium">{{ $paginator->firstItem() }}</span> - {!! __('to') !!} - <span class="font-medium">{{ $paginator->lastItem() }}</span> - @else - {{ $paginator->count() }} - @endif - {!! __('of') !!} - <span class="font-medium">{{ $paginator->total() }}</span> - {!! __('results') !!} - </p> + <p class="text-sm text-gray-700 leading-5"> + @if ($paginator->firstItem()) + {!! __('Showing :first to :last of :total results', [ + 'first' => '<span class="font-medium">' . $paginator->firstItem() . '</span>', + 'last' => '<span class="font-medium">' . $paginator->lastItem() . '</span>', + 'total' => '<span class="font-medium">' . $paginator->total() . '</span>', + ]) !!} + @else + {!! __('Showing :count of :total results', [ + 'count' => $paginator->count(), + 'total' => '<span class="font-medium">' . $paginator->total() . '</span>', + ]) !!} + @endif + </p>This approach provides better context for translators and allows them to adjust word order as needed for different languages.
k6/tags.js (1)
6-11: Consider documenting or removing the unuseddataparameter.The function accepts a
dataparameter but doesn't use it. If this parameter is required for k6's tag generation interface compatibility, consider adding a comment explaining why it's unused. Otherwise, remove it for clarity.♻️ Optional refactor to remove unused parameter
-export function tags(data) { +export function tags() { return { test_type: 'continuous_load', service: 'spacepad-app', }; }backend/artisan (1)
8-10: Preferrequire_oncefor the pre-autoload bootstrap.Proposed tweak
-require __DIR__.'/bootstrap/opentelemetry.php'; +require_once __DIR__.'/bootstrap/opentelemetry.php';backend/routes/console.php (1)
3-3: Verify the hourly schedule won’t miss users if the scheduler slips, and confirm the command runs all checks.
- The command logic (see snippet from
backend/app/Console/Commands/CheckMarketingTriggers.php) uses narrow “previous hour/day” windows; if the scheduler skips/delays an hour, some users may never match.- In that same snippet,
handle()appears to not callcheckTrialExpiredOrCancelled()(it’s defined, but not invoked). Please confirm that’s addressed in the actual file.Also applies to: 40-43
backend/resources/views/layouts/error.blade.php (1)
12-12: Consider conditional loading of the Faro script.The Faro script is loaded unconditionally, but other monitoring scripts in the codebase (e.g., Clarity, Google Tag Manager in
blank.blade.php) use@includeWhento respect configuration flags. Since the AI summary mentions aconfig('faro')with anenabledflag, consider making this conditional to avoid loading unnecessary scripts when Faro is disabled.♻️ Proposed refactor
- @include('components.scripts.faro') + @includeWhen(config('faro.enabled'), 'components.scripts.faro')backend/resources/views/layouts/blank.blade.php (1)
31-31: Consider conditional loading of the Faro script.Consistent with the pattern used for other monitoring services in this layout (Clarity at line 30, Google Tag Manager at line 22), the Faro script should be conditionally loaded based on configuration to avoid unnecessary overhead when disabled.
♻️ Proposed refactor
- @include('components.scripts.faro') + @includeWhen(config('faro.enabled'), 'components.scripts.faro')backend/app/Http/Controllers/DisplaySettingsController.php (1)
20-121: Consider centralizing Pro gating into middleware/policy to avoid 4x duplication.
This will also reduce the chance future endpoints miss the Pro check.backend/resources/views/pages/displays/settings.blade.php (1)
6-7: Optional hardening: avoid assumingauth()->user()is always present in the view.
If there’s any chance this renders for guests, preferauth()->check()/optional(auth()->user())?->...to avoid a fatal error.backend/app/Models/Calendar.php (1)
18-27: Enforceworkspace_idconsistently (model/validation/DB constraint).Line 18-27 and Line 59-62: Adding the field + relation is good, but ensure calendars can’t be created without
workspace_id(otherwise workspace-scoped queries/policies can leak/miss data). Prefer a DB constraint (non-null + FK) and/or explicit assignment in creation flows/factories.Also applies to: 59-62
backend/docs/WORKSPACE_SETUP.md (1)
58-63: Doc polish: compound adjective + stronger wording.
- Line 60-62: consider “Revoke member access” instead of repeating “Remove”.
- Line 109: “backward-compatible” (hyphen) when used as an adjective.
Also applies to: 107-110
backend/app/Http/Controllers/CalDAVAccountsController.php (1)
49-58: Consider removing unused variable assignment.The
$accountvariable is assigned but never used after the CalDAVAccount is created. The static analysis tool correctly identified this.♻️ Suggested refactor
- $account = CalDAVAccount::create([ + CalDAVAccount::create([ 'user_id' => auth()->id(), 'workspace_id' => $workspaceId, 'name' => parse_url($validated['url'], PHP_URL_HOST), 'email' => $validated['username'], 'url' => $validated['url'], 'username' => $validated['username'], 'password' => $validated['password'], 'permission_type' => PermissionType::WRITE, ]);k6/README.md (1)
105-111: Optional: Consider wrapping bare URLs in angle brackets or links.The static analysis tool flagged bare URLs at lines 105 and 111. While these are acceptable in documentation context, you could optionally wrap them in angle brackets (
<http://localhost:3000>) or use markdown link syntax for stricter markdown compliance.📝 Optional markdown formatting
-1. **Grafana** (http://localhost:3000): +1. **Grafana** (<http://localhost:3000>): - Traces in Tempo showing request flows - Metrics in Prometheus showing request rates, latencies - Service maps showing service dependencies - Logs in Loki showing application logs -2. **Prometheus** (http://localhost:9090): +2. **Prometheus** (<http://localhost:9090>): - Query: `rate(http_requests_total[1m])` - Query: `histogram_quantile(0.95, rate(http_request_duration_seconds_bucket[5m]))`backend/app/Enums/WorkspaceRole.php (1)
23-26: Optional: Consider using match expression for consistency.The
canManage()method usesin_array()which works correctly. For consistency with thelabel()method and to make the logic more explicit, you could optionally use a match expression.♻️ Alternative implementation using match
public function canManage(): bool { - return in_array($this, [self::OWNER, self::ADMIN]); + return match($this) { + self::OWNER, self::ADMIN => true, + self::MEMBER => false, + }; }backend/database/migrations/2025_12_06_000003_add_first_name_and_last_name_to_users_table.php (1)
12-21: Combine into a single Schema::table call for efficiency.The
up()method uses two separateSchema::table()calls to add the columns, which results in two separate ALTER TABLE operations. This can be combined into a single call for better performance and consistency with thedown()method.♻️ Suggested refactor
public function up(): void { Schema::table('users', function (Blueprint $table) { $table->string('first_name')->nullable()->after('name'); - }); - - Schema::table('users', function (Blueprint $table) { $table->string('last_name')->nullable()->after('first_name'); }); }backend/app/Http/Controllers/API/Auth/AuthController.php (1)
39-52:firstOrCreate()won’t update existing devicename(and only conditionally updatesworkspace_id).If you expect
nameto reflect the latest device-provided value, useupdateOrCreate()(or explicitly update when changed).backend/app/Services/OutlookService.php (1)
114-128: Workspace scoping looks consistent, but ensure workspace_id can’t be null in normal flows.
If a user can ever exist without a primary workspace,workspace_idbecomesnulland creates “unscoped” provider accounts that your new workspace-based policies will likely deny later.backend/app/Services/GoogleService.php (1)
62-76: Workspace scoping on GoogleAccount upsert looks right.
If “selected workspace” can ever be null, you’ll create unscoped accounts (workspace_id = null) that may be unusable with workspace-based authorization.backend/app/Data/UserWebhookData.php (1)
10-26: Give new nullable fields default values to make Data::from() more robust.
This avoids constructor-arg issues if the source object/array doesn’t havefirstName/lastName.Proposed change
- public ?string $firstName, - public ?string $lastName, + public ?string $firstName = null, + public ?string $lastName = null,backend/app/Listeners/SendUserNotActivatedAfter24hNotification.php (1)
14-25: Add timeout + exception handling so webhook delivery can’t crash the listener.
At minimum, guard against ConnectionException/timeouts and log failures (especially if this runs in a scheduled command/queue worker).Proposed change
- Http::post($webhookUrl, [ - 'event' => 'user_not_activated_after_24h', - 'user' => UserWebhookData::from($event->user), - ]); + try { + Http::timeout(5)->post($webhookUrl, [ + 'event' => 'user_not_activated_after_24h', + 'user' => UserWebhookData::from($event->user), + ]); + } catch (\Throwable $e) { + logger()->warning('Failed to send user_not_activated_after_24h webhook', [ + 'error' => $e->getMessage(), + ]); + }docker-compose.dev.yml (2)
90-103: Make k6 opt-in (profile) and pin the image version (avoid :latest).
Otherwisedocker compose upwill start continuous load generation by default and could pull breaking changes unexpectedly.Example tweak
k6-load: - image: grafana/k6:latest + image: grafana/k6:0.53.0 + profiles: + - load
17-27: Quote boolean-like env values to ensure consistency and avoid YAML type surprises in Compose.Unquoted
truein YAML may be parsed as a boolean; quoting forces string values, which are safer to work with across tooling. This file already quotes other boolean env vars (e.g.,AUTORUN_ENABLED: 'false'), so quotingOTEL_PHP_AUTOLOAD_ENABLEDandOTEL_EXPERIMENTAL_METRIC_ENABLEmaintains consistency.Example tweak
- OTEL_PHP_AUTOLOAD_ENABLED: true + OTEL_PHP_AUTOLOAD_ENABLED: "true" ... - OTEL_EXPERIMENTAL_METRIC_ENABLE: true + OTEL_EXPERIMENTAL_METRIC_ENABLE: "true"Also applies to: 43-55
backend/WORKSPACE_SETUP.md (1)
109-109: Minor style suggestion: use hyphenated compound adjective.Consider using "backward-compatible" (hyphenated) when it modifies a noun as a compound adjective.
📝 Suggested fix
-- All existing functionality remains backward compatible +- All existing functionality remains backward-compatiblebackend/app/Listeners/SendTrialExpiredOrCancelledNotification.php (1)
21-24: Add error handling and timeout for webhook reliability.The HTTP request lacks error handling and timeout configuration. If the webhook endpoint is slow or fails, this could cause issues for the event handling pipeline.
♻️ Suggested improvement
- Http::post($webhookUrl, [ - 'event' => 'trial_expired_or_cancelled', - 'user' => UserWebhookData::from($event->user), - ]); + try { + Http::timeout(10)->post($webhookUrl, [ + 'event' => 'trial_expired_or_cancelled', + 'user' => UserWebhookData::from($event->user), + ]); + } catch (\Exception $e) { + logger()->error('Failed to send trial_expired_or_cancelled webhook', [ + 'user_id' => $event->user->id, + 'error' => $e->getMessage(), + ]); + }Additionally, consider implementing
ShouldQueueto process the webhook asynchronously for better reliability:class SendTrialExpiredOrCancelledNotification implements ShouldQueue { use InteractsWithQueue; // ... }Based on coding guidelines, Laravel queues should be used for background job processing.
backend/resources/views/pages/admin.blade.php (1)
333-365: Consider computing counts in the controller for better performance.The view iterates over
$allUsersmultiple times for different counts (total, with displays, Pro users). For large user bases, this could impact performance.Consider computing these counts in the
AdminControllerand passing them as separate variables:// In AdminController $totalUsersCount = $allUsers->count(); $usersWithDisplaysCount = $allUsers->filter(fn($u) => $u->displays_count > 0)->count(); $proUsersCount = $allUsers->filter(fn($u) => $u->hasPro())->count();Then use
{{ $totalUsersCount }},{{ $usersWithDisplaysCount }}, and{{ $proUsersCount }}in the view. This separates concerns and avoids repeated iteration in the template.backend/app/Services/DisplayService.php (1)
49-51: Consider removing unnecessary eager loading.The
workspace.membersrelationship is eager-loaded but not used in this method. This could be removed to avoid an unnecessary database query.♻️ Suggested change
- $display = Display::with('workspace.members') + $display = Display::with('workspace') ->whereIn('workspace_id', $workspaceIds) ->find($displayId);backend/routes/web.php (1)
121-125: Consider using dedicated admin middleware instead of controller-level checks.The admin routes rely on
checkAdminAccess()being called in each controller method. A dedicated middleware (e.g.,adminorcan:admin) would provide defense-in-depth and make the authorization requirement explicit in the route definition. Based on learnings, middleware should be used for authentication and request processing.♻️ Suggested approach
// Create a middleware or use policy-based authorization Route::middleware(['auth', 'admin'])->prefix('admin')->group(function () { Route::get('/', [AdminController::class, 'index'])->name('admin.index'); Route::get('/users/{user}', [AdminController::class, 'showUser'])->name('admin.users.show'); Route::delete('/users/{user}', [AdminController::class, 'deleteUser'])->name('admin.users.delete'); Route::post('/users/{user}/impersonate', [AdminController::class, 'impersonate'])->name('admin.users.impersonate'); Route::post('/stop-impersonating', [AdminController::class, 'stopImpersonating'])->name('admin.stop-impersonating'); });backend/app/Console/Commands/TriggerRegistrationWebhookForMissingNames.php (1)
28-51: LGTM - Command logic is sound.The command correctly identifies users missing name data and triggers the webhook. A few observations:
- Processing one user per run is intentional and appropriate for gradual backfill.
- If the
Usermodel uses theSoftDeletestrait, consider using->withoutTrashed()instead of->whereNull('deleted_at')for consistency with Laravel conventions.💡 Optional: Use SoftDeletes trait method
- $user = User::whereNull('deleted_at') + $user = User::withoutTrashed() ->where(function ($query) { $query->whereNull('first_name') ->orWhereNull('last_name'); }) ->orderBy('created_at', 'asc') ->first();backend/resources/views/pages/dashboard.blade.php (1)
243-256: Simplify duplicate conditional branches.Lines 243-250 have nearly identical output for both
! $isSelfHosted && auth()->user()->shouldUpgradeForCurrentWorkspace()and$isSelfHosted && auth()->user()->shouldUpgradeForCurrentWorkspace(). These can be consolidated since the rendered HTML is the same.♻️ Suggested simplification
- @if(! $isSelfHosted && auth()->user()->shouldUpgradeForCurrentWorkspace()) - <span class="inline-flex items-center rounded-md bg-gray-100 px-3 py-2 text-center text-sm font-semibold text-gray-400 shadow-sm ring-1 ring-inset ring-gray-200 cursor-not-allowed" title="Upgrade to Pro to create more displays"> - <x-icons.plus class="h-5 w-5 mr-1" /> Create new display <span class="ml-2 inline-flex items-center rounded-md bg-blue-50 px-1.5 py-0.5 text-xs font-medium text-blue-700 ring-1 ring-inset ring-blue-600">Pro</span> - </span> - @elseif($isSelfHosted && auth()->user()->shouldUpgradeForCurrentWorkspace()) - <span class="inline-flex items-center rounded-md bg-gray-100 px-3 py-2 text-center text-sm font-semibold text-gray-400 shadow-sm ring-1 ring-inset ring-gray-200 cursor-not-allowed" title="Upgrade to Pro to create more displays"> - <x-icons.plus class="h-5 w-5 mr-1" /> Create new display <span class="ml-2 inline-flex items-center rounded-md bg-blue-50 px-1.5 py-0.5 text-xs font-medium text-blue-700 ring-1 ring-inset ring-blue-600">Pro</span> - </span> - @else + @if(auth()->user()->shouldUpgradeForCurrentWorkspace()) + <span class="inline-flex items-center rounded-md bg-gray-100 px-3 py-2 text-center text-sm font-semibold text-gray-400 shadow-sm ring-1 ring-inset ring-gray-200 cursor-not-allowed" title="Upgrade to Pro to create more displays"> + <x-icons.plus class="h-5 w-5 mr-1" /> Create new display <span class="ml-2 inline-flex items-center rounded-md bg-blue-50 px-1.5 py-0.5 text-xs font-medium text-blue-700 ring-1 ring-inset ring-blue-600">Pro</span> + </span> + @elsebackend/app/Models/Workspace.php (2)
9-9: Remove unused import.
BelongsTois imported but not used in this model.-use Illuminate\Database\Eloquent\Relations\BelongsTo;
121-130: Consider using Collection method for cleaner iteration.The
hasPro()logic is correct, but can be simplified using Laravel's collectioncontains()method with a callback.♻️ Suggested refactor
public function hasPro(): bool { - $owners = $this->owners()->with('subscriptions')->get(); - foreach ($owners as $owner) { - if ($owner->hasPro()) { - return true; - } - } - return false; + return $this->owners() + ->with('subscriptions') + ->get() + ->contains(fn ($owner) => $owner->hasPro()); }backend/app/Http/Controllers/API/DisplayController.php (1)
34-61: Workspace-scoped display retrieval is correctly implemented.The changes properly scope displays to all workspaces the user belongs to. One minor optimization:
Line 44 uses
$user->workspaces->pluck('id')which triggers a full model load. For better efficiency when you only need IDs:💡 Optional optimization
- $workspaceIds = $user->workspaces->pluck('id'); + $workspaceIds = $user->workspaces()->pluck('workspaces.id');The added logging provides good observability. Ensure log storage has appropriate access controls since it contains user IDs and IP addresses.
backend/FARO_SETUP.md (1)
202-212: Document feature toggle variables in main configuration section.The advanced feature toggle environment variables (
FARO_PERFORMANCE_ENABLED,FARO_ERRORS_ENABLED, etc.) are mentioned here but not in the main configuration section (Lines 24-31). Consider adding a note in the main config section pointing to these advanced options, or documenting their defaults.backend/app/Console/Commands/CheckMarketingTriggers.php (2)
60-76: Consider using soft delete scope and avoiding PII in logs.
If the
Usermodel usesSoftDeletestrait,whereNull('deleted_at')is redundant since the trait applies it automatically. If not using the trait, this is correct.Logging user emails (line 73) may create compliance concerns (GDPR/CCPA) as PII ends up in logs. Consider logging only the user ID instead.
Proposed change for log output
- $this->line("Fired UserNotActivatedAfter24h for user {$user->email}"); + $this->line("Fired UserNotActivatedAfter24h for user ID: {$user->id}");
139-168: Same nulllast_activity_atexclusion issue and code duplication.This method has the same issue as
checkPassiveUserswhere users with nulllast_activity_atare excluded despite the comment. Additionally, there's significant code duplication between these two methods.Consider extracting the common logic into a private helper method:
private function checkInactivityForDays(int $days, string $eventClass, string $cacheKeyPrefix, string $eventName): void { $cutoffDate = now()->subDays($days); $previousCutoffDate = now()->subDays($days + 1); // ... shared query logic }k6/load-test.js (3)
59-61: Module-level variables are not needed and may cause confusion.In k6,
setup()runs once globally (not per VU), and the returned data object is passed to each VU's default function. The module-levelauthTokenanddisplayIdvariables are redundant since you correctly usedata.tokenanddata.displayIdin the default function.Remove unused module-level variables
-// Shared state for VU - stores auth token and display ID -let authToken = null; -let displayId = null;
64-75:__VUis always 0 in setup() context.In k6,
setup()runs once in the init context where__VUis always 0, not per virtual user. The device UID and name on lines 70-71 will always use__VU= 0, so all VUs share the same setup data. This is likely intentional for load testing, but the comments suggest it's "once per VU" which is misleading.Update comment for clarity
-// Setup function - runs once per VU to authenticate +// Setup function - runs once globally before all VUs start export function setup() {
284-319: Custom duration metric duplicates built-in k6 metric.Line 289 adds a custom
requestDurationmetric, but k6 already provideshttp_req_durationas a built-in metric (used in thresholds on lines 39 and 53). Consider using only the built-in metric unless you need specific custom tagging beyond what's available.backend/app/Http/Controllers/AdminController.php (3)
143-148: LIKE wildcard characters in search input are not escaped.User-provided search input containing
%or_characters will be interpreted as LIKE wildcards, which could lead to unexpected search results or performance issues with crafted inputs like%%%%%.Escape LIKE special characters
if ($search) { + $escapedSearch = str_replace(['%', '_'], ['\\%', '\\_'], $search); $allUsersQuery->where(function($query) use ($search) { - $query->where('name', 'like', "%{$search}%") - ->orWhere('email', 'like', "%{$search}%"); + $query->where('name', 'like', "%{$escapedSearch}%") + ->orWhere('email', 'like', "%{$escapedSearch}%"); }); }
363-368: Remove unused$adminvariable.The
$adminvariable on line 367 is declared but never used in this method, as flagged by static analysis.Remove unused variable
public function showUser(User $user) { $this->checkAdminAccess(); - - $admin = Auth::user(); // Load user relationships for display
430-558: Deletion logic is thorough but logs PII.The cascading delete logic properly handles:
- Tokens, displays, devices, rooms
- Calendar accounts and their nested calendars/events
- Workspace ownership transfer with fallback to deletion
- Instance cleanup
However, lines 552-557 log the user's email address, which is PII and could create compliance concerns.
Avoid logging email addresses
logger()->info('User account deleted by admin', [ 'deleted_user_id' => $user->id, - 'deleted_user_email' => $user->email, 'deleted_by_admin_id' => $admin->id, - 'deleted_by_admin_email' => $admin->email, ]);
| // Log exceptions with context (skip 404s and validation errors to avoid noise) | ||
| if (!($e instanceof NotFoundHttpException) && !($e instanceof ValidationException)) { | ||
| $logLevel = $e instanceof AuthenticationException ? 'warning' : 'error'; | ||
|
|
||
| logger()->{$logLevel}('Unhandled exception', [ | ||
| 'exception' => get_class($e), | ||
| 'message' => $e->getMessage(), | ||
| 'code' => $e->getCode(), | ||
| 'file' => $e->getFile(), | ||
| 'line' => $e->getLine(), | ||
| 'route' => $request->route()?->getName(), | ||
| 'path' => $request->path(), | ||
| 'method' => $request->method(), | ||
| 'ip' => $request->ip(), | ||
| 'user_id' => auth()->id(), | ||
| 'user_agent' => substr($request->userAgent() ?? '', 0, 200), | ||
| 'trace' => config('app.debug') ? substr($e->getTraceAsString(), 0, 1000) : null, | ||
| ]); | ||
| } |
There was a problem hiding this comment.
Avoid leaking sensitive data + reduce noise/duplication in exception logging (consider shouldReport() + 5xx-only).
- Line 44-57 logs IP/user-agent + raw exception message in all environments; that can capture PII/secrets (and may violate retention/compliance expectations).
- This may also double-log exceptions if
report()(or a sink like Sentry) already logs them. - “Unhandled exception” can include non-404 4xx
HttpExceptions; consider skipping those unless you explicitly want them.
Proposed adjustment (minimize prod context; log only reportable/5xx)
use Illuminate\Auth\AuthenticationException;
use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
+use Symfony\Component\HttpKernel\Exception\HttpExceptionInterface;
use Throwable;
...
- // Log exceptions with context (skip 404s and validation errors to avoid noise)
- if (!($e instanceof NotFoundHttpException) && !($e instanceof ValidationException)) {
+ // Log exceptions with context (skip 4xx noise; avoid leaking details in prod)
+ if (
+ $this->shouldReport($e) &&
+ !($e instanceof NotFoundHttpException) &&
+ !($e instanceof ValidationException) &&
+ !($e instanceof HttpExceptionInterface && $e->getStatusCode() < 500)
+ ) {
$logLevel = $e instanceof AuthenticationException ? 'warning' : 'error';
-
- logger()->{$logLevel}('Unhandled exception', [
+
+ $context = [
'exception' => get_class($e),
- 'message' => $e->getMessage(),
- 'code' => $e->getCode(),
- 'file' => $e->getFile(),
- 'line' => $e->getLine(),
'route' => $request->route()?->getName(),
'path' => $request->path(),
'method' => $request->method(),
- 'ip' => $request->ip(),
'user_id' => auth()->id(),
- 'user_agent' => substr($request->userAgent() ?? '', 0, 200),
- 'trace' => config('app.debug') ? substr($e->getTraceAsString(), 0, 1000) : null,
- ]);
+ ];
+
+ if (config('app.debug')) {
+ $context += [
+ 'message' => $e->getMessage(),
+ 'code' => $e->getCode(),
+ 'file' => $e->getFile(),
+ 'line' => $e->getLine(),
+ 'ip' => $request->ip(),
+ 'user_agent' => substr($request->userAgent() ?? '', 0, 200),
+ 'trace' => substr($e->getTraceAsString(), 0, 1000),
+ ];
+ }
+
+ logger()->{$logLevel}('Unhandled exception', $context);
}| $code = $request->validated()['code']; | ||
| $uid = $request->validated()['uid']; | ||
| $name = $request->validated()['name'] ?? 'Unknown'; | ||
| $connectedUserId = cache()->get("connect-code:$code"); | ||
|
|
||
| // Check if the code is a valid connect code | ||
| if ($connectedUserId !== null) { | ||
| $user = User::find($connectedUserId); | ||
| $workspace = $user?->primaryWorkspace(); | ||
|
|
||
| $device = Device::firstOrCreate([ | ||
| 'user_id' => $connectedUserId, | ||
| 'uid' => $request->validated()['uid'], | ||
| 'uid' => $uid, | ||
| ],[ | ||
| 'user_id' => $connectedUserId, | ||
| 'uid' => $request->validated()['uid'], | ||
| 'name' => $request->validated()['name'], | ||
| 'workspace_id' => $workspace?->id, | ||
| 'uid' => $uid, | ||
| 'name' => $name, | ||
| ]); | ||
|
|
||
| // Update workspace_id if device already existed but didn't have one | ||
| if ($device->workspace_id === null && $workspace) { | ||
| $device->update(['workspace_id' => $workspace->id]); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verification script to check connect code handling
echo "=== Searching for connect-code cache invalidation ==="
rg -n "connect-code:\$code" -S
echo ""
echo "=== Searching for cache->forget calls ==="
rg -n "forget.*connect-code" -S
echo ""
echo "=== Searching for cache->pull calls ==="
rg -n "pull.*connect-code" -SRepository: magweter/spacepad
Length of output: 198
🏁 Script executed:
#!/bin/bash
# Locate the AuthController file and examine full context
find . -name "AuthController.php" -path "*/Controllers/API/Auth/*" | head -5Repository: magweter/spacepad
Length of output: 119
🏁 Script executed:
#!/bin/bash
# Get the full content of the AuthController to understand the complete flow
fd -H "AuthController.php" -path "*/Controllers/API/Auth/*" --exec wc -l {} \;Repository: magweter/spacepad
Length of output: 229
🏁 Script executed:
#!/bin/bash
# Search for where connect codes are created to understand the flow
rg -n "connect-code:" -S -B 2 -A 2Repository: magweter/spacepad
Length of output: 1152
🏁 Script executed:
cat -n backend/app/Http/Controllers/API/Auth/AuthController.php | head -100Repository: magweter/spacepad
Length of output: 3567
🏁 Script executed:
# Get a larger view of the login method
sed -n '29,80p' backend/app/Http/Controllers/API/Auth/AuthController.php | cat -nRepository: magweter/spacepad
Length of output: 2440
Make connect codes single-use and validate user existence before device creation.
The current implementation allows connect codes to be reused indefinitely (uses cache()->get() without invalidation), and devices can be created for non-existent or deleted users since there's no null check after User::find(). This permits unlimited token generation with a single connect code and allows authentication tokens to be issued for orphaned device records.
Use cache()->pull() to atomically retrieve and invalidate the code, and add an explicit user existence check before proceeding:
Fix
- $connectedUserId = cache()->get("connect-code:$code");
+ $connectedUserId = cache()->pull("connect-code:$code");
// Check if the code is a valid connect code
if ($connectedUserId !== null) {
$user = User::find($connectedUserId);
$workspace = $user?->primaryWorkspace();
+ if (! $user) {
+ return $this->error(
+ message: 'User not found.',
+ code: 404
+ );
+ }
$device = Device::firstOrCreate([📝 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.
| $code = $request->validated()['code']; | |
| $uid = $request->validated()['uid']; | |
| $name = $request->validated()['name'] ?? 'Unknown'; | |
| $connectedUserId = cache()->get("connect-code:$code"); | |
| // Check if the code is a valid connect code | |
| if ($connectedUserId !== null) { | |
| $user = User::find($connectedUserId); | |
| $workspace = $user?->primaryWorkspace(); | |
| $device = Device::firstOrCreate([ | |
| 'user_id' => $connectedUserId, | |
| 'uid' => $request->validated()['uid'], | |
| 'uid' => $uid, | |
| ],[ | |
| 'user_id' => $connectedUserId, | |
| 'uid' => $request->validated()['uid'], | |
| 'name' => $request->validated()['name'], | |
| 'workspace_id' => $workspace?->id, | |
| 'uid' => $uid, | |
| 'name' => $name, | |
| ]); | |
| // Update workspace_id if device already existed but didn't have one | |
| if ($device->workspace_id === null && $workspace) { | |
| $device->update(['workspace_id' => $workspace->id]); | |
| } | |
| $code = $request->validated()['code']; | |
| $uid = $request->validated()['uid']; | |
| $name = $request->validated()['name'] ?? 'Unknown'; | |
| $connectedUserId = cache()->pull("connect-code:$code"); | |
| // Check if the code is a valid connect code | |
| if ($connectedUserId !== null) { | |
| $user = User::find($connectedUserId); | |
| $workspace = $user?->primaryWorkspace(); | |
| if (! $user) { | |
| return $this->error( | |
| message: 'User not found.', | |
| code: 404 | |
| ); | |
| } | |
| $device = Device::firstOrCreate([ | |
| 'user_id' => $connectedUserId, | |
| 'uid' => $uid, | |
| ],[ | |
| 'user_id' => $connectedUserId, | |
| 'workspace_id' => $workspace?->id, | |
| 'uid' => $uid, | |
| 'name' => $name, | |
| ]); | |
| // Update workspace_id if device already existed but didn't have one | |
| if ($device->workspace_id === null && $workspace) { | |
| $device->update(['workspace_id' => $workspace->id]); | |
| } |
🤖 Prompt for AI Agents
In @backend/app/Http/Controllers/API/Auth/AuthController.php around lines 29 -
52, The connect code retrieval and user lookup are unsafe: replace
cache()->get("connect-code:$code") with cache()->pull("connect-code:$code") to
atomically invalidate the code after first use, and after $connectedUserId =
cache()->pull(...) ensure the user exists (check result of
User::find($connectedUserId) and abort/return if null) before calling
$user->primaryWorkspace() or creating a Device (the Device::firstOrCreate /
update logic should only run when $user is non-null).
| if (!$device->user_id) { | ||
| return $this->error( | ||
| message: 'Device is not associated with a user', | ||
| code: Response::HTTP_BAD_REQUEST | ||
| ); | ||
| } | ||
|
|
||
| $user = User::with('workspaces')->find($device->user_id); | ||
| if (!$user) { | ||
| return $this->error( | ||
| message: 'User not found', | ||
| code: Response::HTTP_NOT_FOUND | ||
| ); | ||
| } | ||
|
|
||
| // Get all workspace IDs the user is a member of | ||
| $workspaceIds = $user->workspaces->pluck('id'); | ||
| if ($workspaceIds->isEmpty()) { | ||
| return $this->error( | ||
| message: 'User is not a member of any workspace', | ||
| code: Response::HTTP_BAD_REQUEST | ||
| ); | ||
| } | ||
|
|
||
| // Find display in any of the user's workspaces | ||
| $display = Display::query() | ||
| ->where('user_id', $device->user_id) | ||
| ->whereIn('workspace_id', $workspaceIds) | ||
| ->find($data['display_id']); |
There was a problem hiding this comment.
Potential authz gap: device can switch to any display in any of the user’s workspaces.
If a device is meant to be “bound” to a workspace (via devices.workspace_id), restrict changeDisplay to that workspace (or explicitly justify allowing cross-workspace switching). Based on learnings, device-specific authentication/display assignment should be enforced in backend paths.
Proposed fix (restrict to device workspace when present)
// Get all workspace IDs the user is a member of
- $workspaceIds = $user->workspaces->pluck('id');
+ $workspaceIds = $user->workspaces->pluck('id');
+ if ($device->workspace_id) {
+ $workspaceIds = $workspaceIds->intersect([$device->workspace_id])->values();
+ }
if ($workspaceIds->isEmpty()) {
return $this->error(
message: 'User is not a member of any workspace',
code: Response::HTTP_BAD_REQUEST
);
}
// Find display in any of the user's workspaces
$display = Display::query()
->whereIn('workspace_id', $workspaceIds)
->find($data['display_id']);🤖 Prompt for AI Agents
In @backend/app/Http/Controllers/API/DeviceController.php around lines 29 - 56,
The changeDisplay flow currently allows selecting any Display in any workspace
the user belongs to; restrict this by enforcing the device's bound workspace
when present: if $device->workspace_id is set, limit the Display lookup to that
workspace (e.g., include a where('workspace_id', $device->workspace_id)
condition or intersect $workspaceIds with [$device->workspace_id]) before
calling Display::query()->whereIn(...)->find($data['display_id']), otherwise
fallback to the existing user-workspaces check; update the authorization/error
paths in DeviceController (changeDisplay) to return a 403/400 when the requested
display is outside the device's workspace.
| // Check if user has Pro access (workspace-aware) | ||
| if (!auth()->user()->hasProForCurrentWorkspace()) { | ||
| return redirect()->route('dashboard')->with('error', 'Display settings are only available for Pro users.'); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd backend && find . -name "DisplaySettingsController.php" -type fRepository: magweter/spacepad
Length of output: 113
🏁 Script executed:
cd backend && find . -path "*/Models/User.php" -type fRepository: magweter/spacepad
Length of output: 82
🏁 Script executed:
cd backend && find . -path "*/routes/*.php" -type f | head -20Repository: magweter/spacepad
Length of output: 137
🏁 Script executed:
cd backend && git ls-files | grep -E "(routes|Controllers|Models)" | head -30Repository: magweter/spacepad
Length of output: 1391
🏁 Script executed:
cd backend && cat -n app/Http/Controllers/DisplaySettingsController.phpRepository: magweter/spacepad
Length of output: 10523
🏁 Script executed:
cd backend && cat -n app/Models/User.php | head -200Repository: magweter/spacepad
Length of output: 6761
🏁 Script executed:
cd backend && find . -path "*/Models/Display.php" -type fRepository: magweter/spacepad
Length of output: 85
🏁 Script executed:
cd backend && cat -n routes/web.php | grep -A 5 -B 5 "DisplaySettings"Repository: magweter/spacepad
Length of output: 2475
🏁 Script executed:
cd backend && cat -n app/Models/User.php | tail -100Repository: magweter/spacepad
Length of output: 4254
🏁 Script executed:
cd backend && grep -n "hasProForCurrentWorkspace\|getSelectedWorkspace" app/Models/User.phpRepository: magweter/spacepad
Length of output: 295
🏁 Script executed:
cd backend && cat -n app/Models/Display.php | head -150Repository: magweter/spacepad
Length of output: 4708
🏁 Script executed:
cd backend && find . -path "*/Policies/DisplayPolicy.php" -type fRepository: magweter/spacepad
Length of output: 93
🏁 Script executed:
cd backend && find . -path "*/Models/Workspace.php" -type fRepository: magweter/spacepad
Length of output: 87
🏁 Script executed:
cd backend && sed -n '213,230p' app/Models/User.phpRepository: magweter/spacepad
Length of output: 572
🏁 Script executed:
cd backend && cat -n app/Policies/DisplayPolicy.phpRepository: magweter/spacepad
Length of output: 2270
🏁 Script executed:
cd backend && cat -n app/Models/Workspace.php | head -100Repository: magweter/spacepad
Length of output: 3204
🏁 Script executed:
cd backend && grep -n "Workspace::hasPro\|canBeManagedBy" app/Models/Workspace.phpRepository: magweter/spacepad
Length of output: 116
🏁 Script executed:
cd backend && grep -n "hasPro" app/Models/Workspace.phpRepository: magweter/spacepad
Length of output: 139
🏁 Script executed:
cd backend && cat -n app/Models/Workspace.php | tail -50Repository: magweter/spacepad
Length of output: 1779
Pro gating is checked against the selected workspace, not the display's workspace.
The four methods (index, update, customization, updateCustomization) authorize users against $display->workspace via the policy, but then check hasProForCurrentWorkspace(), which validates the session-selected workspace instead of the display's actual workspace. This allows users with Pro in one workspace to access Pro-only features on displays in a different workspace where they lack Pro entitlement.
Tie the Pro check to $display->workspace_id to match the authorization context, or refactor into a policy ability to enforce entitlement alongside authorization.
🤖 Prompt for AI Agents
In @backend/app/Http/Controllers/DisplaySettingsController.php around lines 24 -
27, The Pro-entitlement check uses hasProForCurrentWorkspace() which validates
the session-selected workspace rather than the display's workspace; update the
four controller methods index, update, customization, and updateCustomization to
check Pro against the display's workspace by using
hasProForWorkspace($display->workspace_id) (or equivalent) or move this check
into the DisplayPolicy so the entitlement is evaluated against
$display->workspace_id alongside the existing authorize($display) call; ensure
the check replaces calls to hasProForCurrentWorkspace() so Pro gating matches
the policy authorization context.
| public function handle(UserActivatedAfter24h $event): void | ||
| { | ||
| $webhookUrl = config('settings.user_activated_after_24h_webhook_url'); | ||
| if (!$webhookUrl) { | ||
| return; | ||
| } | ||
|
|
||
| Http::post($webhookUrl, [ | ||
| 'event' => 'user_activated_after_24h', | ||
| 'user' => UserWebhookData::from($event->user), | ||
| ]); | ||
| } |
There was a problem hiding this comment.
Add timeout and error handling for webhook reliability.
The synchronous HTTP call without a timeout or error handling poses reliability risks:
- If the webhook endpoint is slow or unresponsive, it blocks the application thread
- Silent failures provide no visibility into delivery issues
Consider one of these approaches:
🔧 Solution 1: Add timeout and error handling (fire-and-forget)
public function handle(UserActivatedAfter24h $event): void
{
$webhookUrl = config('settings.user_activated_after_24h_webhook_url');
if (!$webhookUrl) {
return;
}
- Http::post($webhookUrl, [
- 'event' => 'user_activated_after_24h',
- 'user' => UserWebhookData::from($event->user),
- ]);
+ try {
+ Http::timeout(5)->post($webhookUrl, [
+ 'event' => 'user_activated_after_24h',
+ 'user' => UserWebhookData::from($event->user),
+ ]);
+ } catch (\Exception $e) {
+ \Log::warning('Failed to send user_activated_after_24h webhook', [
+ 'user_id' => $event->user->id,
+ 'error' => $e->getMessage(),
+ ]);
+ }
}🔧 Solution 2: Make listener queued for async processing (recommended)
+use Illuminate\Contracts\Queue\ShouldQueue;
+
-class SendUserActivatedAfter24hNotification
+class SendUserActivatedAfter24hNotification implements ShouldQueue
{
/**
* Handle the event.
*/
public function handle(UserActivatedAfter24h $event): void
{
$webhookUrl = config('settings.user_activated_after_24h_webhook_url');
if (!$webhookUrl) {
return;
}
- Http::post($webhookUrl, [
+ Http::timeout(5)->retry(3, 100)->post($webhookUrl, [
'event' => 'user_activated_after_24h',
'user' => UserWebhookData::from($event->user),
]);
}
}🤖 Prompt for AI Agents
In @backend/app/Listeners/SendUserActivatedAfter24hNotification.php around lines
14 - 25, The handle method in SendUserActivatedAfter24hNotification
synchronously posts to the webhook without timeout or error handling; make the
listener async and resilient by implementing ShouldQueue (queue the listener)
and, inside handle(UserActivatedAfter24h $event), perform the HTTP call with a
timeout (e.g., Http::timeout(...)->post(...)) wrapped in a try/catch that logs
failures and the payload (use process logger or Log::error) and optionally
retries via the queue; ensure you still use UserWebhookData::from($event->user)
for the payload and keep the
config('settings.user_activated_after_24h_webhook_url') guard.
| User::chunk(100, function ($users) { | ||
| foreach ($users as $user) { | ||
| // Skip if user already has a workspace | ||
| if ($user->workspaces()->exists()) { | ||
| continue; | ||
| } | ||
|
|
||
| // Create workspace for user | ||
| $workspace = Workspace::create([ | ||
| 'name' => $user->name . "'s Workspace", | ||
| ]); | ||
|
|
||
| // Add user as owner member (use WorkspaceMember::create to generate ULID) | ||
| WorkspaceMember::create([ | ||
| 'workspace_id' => $workspace->id, | ||
| 'user_id' => $user->id, | ||
| 'role' => WorkspaceRole::OWNER, | ||
| ]); | ||
|
|
||
| // Migrate displays to workspace | ||
| Display::where('user_id', $user->id)->update(['workspace_id' => $workspace->id]); | ||
|
|
||
| // Migrate devices to workspace | ||
| Device::where('user_id', $user->id)->update(['workspace_id' => $workspace->id]); | ||
|
|
||
| // Migrate calendars to workspace | ||
| Calendar::where('user_id', $user->id)->update(['workspace_id' => $workspace->id]); | ||
|
|
||
| // Migrate rooms to workspace | ||
| Room::where('user_id', $user->id)->update(['workspace_id' => $workspace->id]); | ||
|
|
||
| // Migrate Outlook accounts to workspace | ||
| OutlookAccount::where('user_id', $user->id) | ||
| ->whereNull('workspace_id') | ||
| ->update(['workspace_id' => $workspace->id]); | ||
|
|
||
| // Migrate Google accounts to workspace | ||
| GoogleAccount::where('user_id', $user->id) | ||
| ->whereNull('workspace_id') | ||
| ->update(['workspace_id' => $workspace->id]); | ||
|
|
||
| // Migrate CalDAV accounts to workspace | ||
| CalDAVAccount::where('user_id', $user->id) | ||
| ->whereNull('workspace_id') | ||
| ->update(['workspace_id' => $workspace->id]); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Wrap data migration in a transaction to prevent partial failures.
If an error occurs mid-migration (e.g., when migrating calendars for a user), the database could be left in an inconsistent state with some users migrated and others not.
♻️ Proposed fix
User::chunk(100, function ($users) {
foreach ($users as $user) {
+ DB::transaction(function () use ($user) {
// Skip if user already has a workspace
- if ($user->workspaces()->exists()) {
+ if (DB::table('workspace_members')->where('user_id', $user->id)->exists()) {
- continue;
+ return;
}
// ... rest of migration logic ...
+ });
}
});📝 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.
| User::chunk(100, function ($users) { | |
| foreach ($users as $user) { | |
| // Skip if user already has a workspace | |
| if ($user->workspaces()->exists()) { | |
| continue; | |
| } | |
| // Create workspace for user | |
| $workspace = Workspace::create([ | |
| 'name' => $user->name . "'s Workspace", | |
| ]); | |
| // Add user as owner member (use WorkspaceMember::create to generate ULID) | |
| WorkspaceMember::create([ | |
| 'workspace_id' => $workspace->id, | |
| 'user_id' => $user->id, | |
| 'role' => WorkspaceRole::OWNER, | |
| ]); | |
| // Migrate displays to workspace | |
| Display::where('user_id', $user->id)->update(['workspace_id' => $workspace->id]); | |
| // Migrate devices to workspace | |
| Device::where('user_id', $user->id)->update(['workspace_id' => $workspace->id]); | |
| // Migrate calendars to workspace | |
| Calendar::where('user_id', $user->id)->update(['workspace_id' => $workspace->id]); | |
| // Migrate rooms to workspace | |
| Room::where('user_id', $user->id)->update(['workspace_id' => $workspace->id]); | |
| // Migrate Outlook accounts to workspace | |
| OutlookAccount::where('user_id', $user->id) | |
| ->whereNull('workspace_id') | |
| ->update(['workspace_id' => $workspace->id]); | |
| // Migrate Google accounts to workspace | |
| GoogleAccount::where('user_id', $user->id) | |
| ->whereNull('workspace_id') | |
| ->update(['workspace_id' => $workspace->id]); | |
| // Migrate CalDAV accounts to workspace | |
| CalDAVAccount::where('user_id', $user->id) | |
| ->whereNull('workspace_id') | |
| ->update(['workspace_id' => $workspace->id]); | |
| } | |
| }); | |
| User::chunk(100, function ($users) { | |
| foreach ($users as $user) { | |
| DB::transaction(function () use ($user) { | |
| // Skip if user already has a workspace | |
| if (DB::table('workspace_members')->where('user_id', $user->id)->exists()) { | |
| return; | |
| } | |
| // Create workspace for user | |
| $workspace = Workspace::create([ | |
| 'name' => $user->name . "'s Workspace", | |
| ]); | |
| // Add user as owner member (use WorkspaceMember::create to generate ULID) | |
| WorkspaceMember::create([ | |
| 'workspace_id' => $workspace->id, | |
| 'user_id' => $user->id, | |
| 'role' => WorkspaceRole::OWNER, | |
| ]); | |
| // Migrate displays to workspace | |
| Display::where('user_id', $user->id)->update(['workspace_id' => $workspace->id]); | |
| // Migrate devices to workspace | |
| Device::where('user_id', $user->id)->update(['workspace_id' => $workspace->id]); | |
| // Migrate calendars to workspace | |
| Calendar::where('user_id', $user->id)->update(['workspace_id' => $workspace->id]); | |
| // Migrate rooms to workspace | |
| Room::where('user_id', $user->id)->update(['workspace_id' => $workspace->id]); | |
| // Migrate Outlook accounts to workspace | |
| OutlookAccount::where('user_id', $user->id) | |
| ->whereNull('workspace_id') | |
| ->update(['workspace_id' => $workspace->id]); | |
| // Migrate Google accounts to workspace | |
| GoogleAccount::where('user_id', $user->id) | |
| ->whereNull('workspace_id') | |
| ->update(['workspace_id' => $workspace->id]); | |
| // Migrate CalDAV accounts to workspace | |
| CalDAVAccount::where('user_id', $user->id) | |
| ->whereNull('workspace_id') | |
| ->update(['workspace_id' => $workspace->id]); | |
| }); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In
@backend/database/migrations/2025_12_30_000004_create_workspaces_for_existing_users.php
around lines 27 - 73, The migration currently performs per-user creations and
updates (User::chunk callback creating Workspace::create,
WorkspaceMember::create, and updates via Display::where..., Device::where...,
Calendar::where..., Room::where..., OutlookAccount::where...,
GoogleAccount::where..., CalDAVAccount::where...) without transactional
boundaries; wrap the entire per-user migration logic inside a DB::transaction
(or call DB::transaction(fn() => { ... }) within the User::chunk loop) so that
creating the workspace, adding the owner via WorkspaceMember::create, and all
update(...) calls are executed atomically and will roll back on exception;
ensure any exceptions are rethrown so the transaction can rollback and the
migration fails visibly.
| <td class="whitespace-nowrap px-3 py-4 text-sm text-gray-500"> | ||
| <span class="inline-flex items-center rounded-md bg-{{ $display->status->color() }}-50 px-2 py-1 text-xs font-medium text-{{ $display->status->color() }}-700 ring-1 ring-inset ring-{{ $display->status->color() }}-600"> | ||
| {{ $display->status->label() }} | ||
| </span> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find Tailwind config files
fd -e "config" -path "*tailwind*" -o -name "tailwind.config.*" 2>/dev/nullRepository: magweter/spacepad
Length of output: 43
🏁 Script executed:
# Search for DisplayStatus enum
fd -type f -name "*.php" | xargs grep -l "DisplayStatus" | head -20Repository: magweter/spacepad
Length of output: 231
🏁 Script executed:
# Look at the specific blade template file
cat backend/resources/views/components/displays/table-row.blade.php 2>/dev/null || echo "File not found, searching..."Repository: magweter/spacepad
Length of output: 7139
🏁 Script executed:
# Search for tailwind config in common locations
find . -name "tailwind.config.*" -o -name "tailwind.js" 2>/dev/null | head -10Repository: magweter/spacepad
Length of output: 89
🏁 Script executed:
cat backend/tailwind.config.jsRepository: magweter/spacepad
Length of output: 611
🏁 Script executed:
# Find DisplayStatus enum
fd -name "DisplayStatus.php" -type fRepository: magweter/spacepad
Length of output: 291
🏁 Script executed:
# Also search for it in case it's in a different location
rg "enum DisplayStatus" --type phpRepository: magweter/spacepad
Length of output: 123
🏁 Script executed:
cat backend/app/Enums/DisplayStatus.phpRepository: magweter/spacepad
Length of output: 740
Dynamic Tailwind classes require safelist configuration to compile.
Tailwind's content scanner cannot detect dynamically constructed class names like bg-{{ $display->status->color() }}-50 at build time. These classes will not be included in the compiled CSS without explicit configuration.
Add these color classes to your Tailwind safelist in backend/tailwind.config.js:
safelist: [
{ pattern: /^(bg|text|ring)-(blue|green|gray|red)-(50|600|700)$/ },
]
Alternatively, modify the DisplayStatus enum to return full class strings:
public function bgClass(): string
{
return match($this) {
self::READY => 'bg-blue-50',
self::ACTIVE => 'bg-green-50',
// ...
};
}Then update the template to use bg-{{ $display->status->bgClass() }} instead of dynamically constructing classes.
🤖 Prompt for AI Agents
In @backend/resources/views/components/displays/table-row.blade.php around lines
31 - 34, The template in table-row.blade.php uses dynamically constructed
Tailwind classes (e.g., bg-{{ $display->status->color() }}-50, text-{{
$display->status->color() }}-700, ring-{{ $display->status->color() }}-600)
which Tailwind's scanner won't detect; either add a safelist in
backend/tailwind.config.js that covers those combinations (e.g., a regex for
bg|text|ring with the known colors and shades) or change the DisplayStatus enum
to return full class strings (add methods like bgClass(), textClass(),
ringClass() on DisplayStatus that return complete class names for each state)
and then update the blade to use those methods (e.g., use {{
$display->status->bgClass() }} etc.) so the outputted classes are static and
included in the compiled CSS.
| @if(config('faro.enabled')) | ||
| <script type="module"> | ||
| import { initializeFaro, getWebInstrumentations } from 'https://cdn.jsdelivr.net/npm/@grafana/faro-web-sdk@latest/+esm'; | ||
|
|
||
| try { | ||
| const faroInstance = initializeFaro({ | ||
| url: @json(config('faro.collector_url')), | ||
| apiKey: @json(config('faro.api_key')), | ||
| app: @json(config('faro.app')), | ||
| instrumentations: getWebInstrumentations(), | ||
| sessionTracking: { | ||
| enabled: @json(config('faro.session_tracking')), | ||
| }, | ||
| }); | ||
|
|
||
| // Store in window for debugging/access | ||
| if (window) { | ||
| Object.defineProperty(window, 'faroInstance', { | ||
| value: faroInstance, | ||
| writable: false, | ||
| configurable: true | ||
| }); | ||
| } | ||
|
|
||
| console.log('[FARO] Initialized - RUM telemetry enabled'); | ||
| } catch (error) { | ||
| console.error('[FARO] Failed to initialize:', error); | ||
| } | ||
| </script> | ||
| @endif |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for Faro configuration files in the project
find . -type f -name "*faro*" | head -20Repository: magweter/spacepad
Length of output: 146
🏁 Script executed:
# Check for composer.json, package.json, and lock files at project root
ls -la | grep -E "(composer|package|lock|mix|webpack|vite)"Repository: magweter/spacepad
Length of output: 43
🏁 Script executed:
# Search for any Faro-related configuration in config directory
find . -path "*/config/*" -type f | head -20Repository: magweter/spacepad
Length of output: 652
🏁 Script executed:
# Look for any references to Faro version constraints in the codebase
rg "faro.*version|faro.*@" --type php --type json -i | head -20Repository: magweter/spacepad
Length of output: 342
🏁 Script executed:
# Check if there are other Blade components that load external scripts/SDKs for reference
find . -path "*/resources/views/components/scripts/*" -type fRepository: magweter/spacepad
Length of output: 183
🏁 Script executed:
# Read the Faro configuration file to understand settings
cat -n ./backend/config/faro.phpRepository: magweter/spacepad
Length of output: 4990
🏁 Script executed:
# Read the Clarity script for comparison on how similar external SDKs are loaded
cat -n ./backend/resources/views/components/scripts/clarity.blade.phpRepository: magweter/spacepad
Length of output: 490
🏁 Script executed:
# Search for any version constraints or references to faro version in lock files
find . -type f \( -name "composer.lock" -o -name "package-lock.json" -o -name "yarn.lock" \) -exec grep -l "faro" {} \;Repository: magweter/spacepad
Length of output: 43
🌐 Web query:
Grafana Faro Web SDK available versions CDN jsdelivr latest
💡 Result:
Latest jsDelivr package info: @grafana/faro-web-sdk — version 2.1.0. CDN base and example URLs:
- jsDelivr package page (shows v2.1.0). [1]
- CDN URLs (use either pinned version or "latest"):
References:
- jsDelivr package page for @grafana/faro-web-sdk (shows v2.1.0).
- Grafana Faro docs — CDN usage / examples for the Web SDK.
🌐 Web query:
JavaScript static import vs dynamic import error handling try catch
💡 Result:
Short answer
- Static (declaration) imports run at module load/parse time and cannot be put inside a try/catch; you can’t conditionally wrap an import declaration to catch load-time errors. [1]
- Dynamic import() is asynchronous and returns a Promise, so you can catch failures with .catch() or await inside try/catch. [1][2]
Why
- import declarations are static: they are resolved and evaluated when the module is loaded (not at runtime inside the function where you wrote them), so a thrown error during load isn’t catchable by a surrounding try/catch at runtime. Use static imports for always-needed dependencies and for tooling (tree-shaking, static analysis). [1]
- import(moduleSpecifier) is an expression that returns a Promise that rejects if the module can’t be loaded or evaluating it throws; handle it with promise .catch() or with async/await + try/catch. [1][2]
Examples
-
Static (cannot catch):
import foo from './foo.js'; // cannot be wrapped in try/catch -
Dynamic with .catch():
import('./maybe-missing.js')
.then(m => m.default())
.catch(err => console.error('failed to load:', err)); // catches load/eval errors. [1] -
Dynamic with async/await + try/catch:
async function run() {
try {
const mod = await import('./maybe-missing.js');
mod.default();
} catch (err) {
console.error('failed to load:', err); // catches rejection/errors.
}
} // works because import() returns a Promise. [1][2]
Sources
- MDN: import() (dynamic import) and explanation of static vs dynamic imports. [1]
- MDN: try...catch statement (usage with async/await). [2]
Pin the Faro SDK version and use dynamic import to catch load failures.
@latest breaks reproducibility. More critically, the static import statement at the top level cannot be caught by your try/catch—static imports fail at module parse time, not runtime. Switch to dynamic import() with await inside the try block to properly handle CDN/network failures.
Pin to version 2.1.0 (current latest per jsDelivr). Ensure FARO_API_KEY is set to a non-default value in production environments.
Proposed fix
@if(config('faro.enabled'))
<script type="module">
- import { initializeFaro, getWebInstrumentations } from 'https://cdn.jsdelivr.net/npm/@grafana/faro-web-sdk@latest/+esm';
-
try {
+ const { initializeFaro, getWebInstrumentations } = await import(
+ 'https://cdn.jsdelivr.net/npm/@grafana/faro-web-sdk@2.1.0/+esm'
+ );
const faroInstance = initializeFaro({
url: @json(config('faro.collector_url')),
apiKey: @json(config('faro.api_key')),| $workspace = $user->primaryWorkspace(); | ||
|
|
||
| $display = Display::factory()->create(['user_id' => $user->id]); | ||
| $device = Device::factory()->create(['user_id' => $user->id, 'display_id' => $display->id]); | ||
| $display = Display::factory()->create([ | ||
| 'user_id' => $user->id, | ||
| 'workspace_id' => $workspace->id, | ||
| ]); | ||
| $device = Device::factory()->create([ | ||
| 'user_id' => $user->id, | ||
| 'workspace_id' => $workspace->id, | ||
| 'display_id' => $display->id, | ||
| ]); |
There was a problem hiding this comment.
Avoid null deref on $user->primaryWorkspace() in test setup.
If primaryWorkspace() returns null, $workspace->id will fatally error and the test failure will be confusing; add an explicit assertion (or explicitly create/attach a workspace in the test).
Proposed fix
$user = User::factory()->create([
'usage_type' => UsageType::PERSONAL,
]);
$workspace = $user->primaryWorkspace();
+ $this->assertNotNull($workspace);
$display = Display::factory()->create([
'user_id' => $user->id,
'workspace_id' => $workspace->id,
]);Also applies to: 65-75, 92-102
🤖 Prompt for AI Agents
In @backend/tests/Feature/DisplaySettingsApiTest.php around lines 22 - 32, The
test currently calls $user->primaryWorkspace() and immediately uses
$workspace->id which can null-deref; ensure primaryWorkspace() exists by either
asserting it (e.g., $this->assertNotNull($workspace =
$user->primaryWorkspace())) or explicitly creating/attaching a workspace for
$user before using it, then pass that $workspace->id into Display::factory() and
Device::factory() (apply same fix to the other occurrences around lines 65-75
and 92-102).
| test('display settings helper can get and set boolean values', function () { | ||
| $user = User::factory()->create(); | ||
| $display = Display::factory()->create(['user_id' => $user->id]); | ||
| $workspace = $user->primaryWorkspace(); | ||
| $display = Display::factory()->create([ | ||
| 'user_id' => $user->id, | ||
| 'workspace_id' => $workspace->id, | ||
| ]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and read the test file
find . -name "DisplaySettingsTest.php" -type fRepository: magweter/spacepad
Length of output: 105
🏁 Script executed:
# Check the User model to understand primaryWorkspace()
find . -path "*/Models/User.php" -type fRepository: magweter/spacepad
Length of output: 90
🏁 Script executed:
# Find User factory
find . -path "*/database/factories/*UserFactory*" -type fRepository: magweter/spacepad
Length of output: 105
🏁 Script executed:
cat -n ./backend/tests/Unit/DisplaySettingsTest.phpRepository: magweter/spacepad
Length of output: 3116
🏁 Script executed:
cat -n ./backend/app/Models/User.php | head -100Repository: magweter/spacepad
Length of output: 3394
🏁 Script executed:
cat -n ./backend/database/factories/UserFactory.phpRepository: magweter/spacepad
Length of output: 2258
🏁 Script executed:
# Search for primaryWorkspace method in User model
rg "primaryWorkspace" ./backend/app/Models/User.php -A 5Repository: magweter/spacepad
Length of output: 267
🏁 Script executed:
# Also search in the entire Models directory to be thorough
rg "primaryWorkspace" ./backend/app/ -A 5Repository: magweter/spacepad
Length of output: 2681
Add null-safety checks before accessing primaryWorkspace() in tests.
The primaryWorkspace() method returns ?Workspace (nullable), and directly accessing $workspace->id at lines 15, 40, and 61 will crash if null. While the User observer auto-creates workspaces on creation, the method signature is explicitly nullable and should be treated defensively in tests.
Option: null coalesce with fallback-create
test('display settings helper can get and set boolean values', function () {
$user = User::factory()->create();
- $workspace = $user->primaryWorkspace();
+ $workspace = $user->primaryWorkspace() ?? Workspace::factory()->create(['owner_id' => $user->id]);
$display = Display::factory()->create([
$user->id,Applies to all three tests: lines 15, 40, 61.
🤖 Prompt for AI Agents
In @backend/tests/Unit/DisplaySettingsTest.php around lines 13 - 19, The tests
call $user->primaryWorkspace() which is nullable; add a null-safety step before
accessing $workspace->id by replacing the direct use with a guarded assignment:
call primaryWorkspace(), and if it returns null create or fetch a fallback
workspace (e.g., via Workspace factory or the user's workspace-creation helper)
and assign it to $workspace so $workspace->id is always valid. Apply this change
to each test that currently uses primaryWorkspace() (the blocks creating Display
via Display::factory() in the three tests) so the tests no longer assume a
non-null primary workspace.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/Http/Controllers/DisplayController.php (1)
77-83: Missing authorization check for provider account ownership.The account validation only checks existence via
findOrFail()but doesn't verify the account belongs to the user's accessible workspaces. A malicious user could specify another user's account ID.🔒 Proposed fix to validate account belongs to workspace
+ // Validate the account belongs to the selected workspace + $workspace = $user->workspaces()->find($workspaceId); + // Validate the existence of the appropriate account based on provider - match ($provider) { - 'outlook' => OutlookAccount::findOrFail($accountId), - 'google' => GoogleAccount::findOrFail($accountId), - 'caldav' => CalDAVAccount::findOrFail($accountId), + $account = match ($provider) { + 'outlook' => OutlookAccount::where('id', $accountId) + ->where('workspace_id', $workspaceId) + ->firstOrFail(), + 'google' => GoogleAccount::where('id', $accountId) + ->where('workspace_id', $workspaceId) + ->firstOrFail(), + 'caldav' => CalDAVAccount::where('id', $accountId) + ->where('workspace_id', $workspaceId) + ->firstOrFail(), default => throw new \InvalidArgumentException('Invalid provider') };Note: This fix should be applied after the workspace resolution logic (lines 85-105) so
$workspaceIdis available.
🧹 Nitpick comments (3)
backend/resources/views/pages/dashboard.blade.php (1)
243-250: Remove duplicate conditional branches.Both the
! $isSelfHosted && shouldUpgradeForCurrentWorkspace()and$isSelfHosted && shouldUpgradeForCurrentWorkspace()branches render identical HTML. Consolidate into a single condition.♻️ Proposed fix
- @if(! $isSelfHosted && auth()->user()->shouldUpgradeForCurrentWorkspace()) - <span class="inline-flex items-center rounded-md bg-gray-100 px-3 py-2 text-center text-sm font-semibold text-gray-400 shadow-sm ring-1 ring-inset ring-gray-200 cursor-not-allowed" title="Upgrade to Pro to create more displays"> - <x-icons.plus class="h-5 w-5 mr-1" /> Create new display <span class="ml-2 inline-flex items-center rounded-md bg-blue-50 px-1.5 py-0.5 text-xs font-medium text-blue-700 ring-1 ring-inset ring-blue-600">Pro</span> - </span> - @elseif($isSelfHosted && auth()->user()->shouldUpgradeForCurrentWorkspace()) - <span class="inline-flex items-center rounded-md bg-gray-100 px-3 py-2 text-center text-sm font-semibold text-gray-400 shadow-sm ring-1 ring-inset ring-gray-200 cursor-not-allowed" title="Upgrade to Pro to create more displays"> - <x-icons.plus class="h-5 w-5 mr-1" /> Create new display <span class="ml-2 inline-flex items-center rounded-md bg-blue-50 px-1.5 py-0.5 text-xs font-medium text-blue-700 ring-1 ring-inset ring-blue-600">Pro</span> - </span> - @else + @if(auth()->user()->shouldUpgradeForCurrentWorkspace()) + <span class="inline-flex items-center rounded-md bg-gray-100 px-3 py-2 text-center text-sm font-semibold text-gray-400 shadow-sm ring-1 ring-inset ring-gray-200 cursor-not-allowed" title="Upgrade to Pro to create more displays"> + <x-icons.plus class="h-5 w-5 mr-1" /> Create new display <span class="ml-2 inline-flex items-center rounded-md bg-blue-50 px-1.5 py-0.5 text-xs font-medium text-blue-700 ring-1 ring-inset ring-blue-600">Pro</span> + </span> + @elsebackend/app/Models/User.php (2)
27-47: Potential race condition in boot method.The
workspaces()->exists()check followed by workspace creation is not atomic. Under concurrent user creation (e.g., duplicate form submissions), this could create duplicate workspaces.Consider using a database transaction or a unique constraint to prevent duplicates.
♻️ Proposed fix using transaction
static::created(function ($user) { - // Only create if user doesn't already have a workspace - if (!$user->workspaces()->exists()) { + DB::transaction(function () use ($user) { + // Re-check within transaction to prevent race condition + if ($user->workspaces()->lockForUpdate()->exists()) { + return; + } + $workspace = Workspace::create([ 'name' => $user->name . "'s Workspace", ]); // Add user as owner member (use WorkspaceMember::create to generate ULID) WorkspaceMember::create([ 'workspace_id' => $workspace->id, 'user_id' => $user->id, 'role' => WorkspaceRole::OWNER, ]); - } + }); });
193-208: Potential N+1 query performance issue.When checking workspace accounts, three separate
whereInqueries are executed. For users with many workspaces, consider optimizing with a single query or eager loading.♻️ Optimized single query approach
if (!$hasAccounts) { // Check if any workspace the user is a member of has accounts $workspaceIds = $this->workspaces()->pluck('workspaces.id')->toArray(); if (!empty($workspaceIds)) { - $workspaceAccountCount = OutlookAccount::whereIn('workspace_id', $workspaceIds)->count() - + GoogleAccount::whereIn('workspace_id', $workspaceIds)->count() - + CalDAVAccount::whereIn('workspace_id', $workspaceIds)->count(); - - if ($workspaceAccountCount > 0) { - $hasAccounts = true; - } + $hasAccounts = OutlookAccount::whereIn('workspace_id', $workspaceIds)->exists() + || GoogleAccount::whereIn('workspace_id', $workspaceIds)->exists() + || CalDAVAccount::whereIn('workspace_id', $workspaceIds)->exists(); } }Using
exists()instead ofcount()short-circuits on first match and avoids counting all records.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/app/Http/Controllers/DisplayController.phpbackend/app/Models/Display.phpbackend/app/Models/User.phpbackend/app/Policies/DisplayPolicy.phpbackend/resources/views/pages/dashboard.blade.php
🧰 Additional context used
📓 Path-based instructions (2)
backend/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
backend/**/*.php: Use Laravel Sanctum for API authentication in the backend.
Integrate with Google Calendar API, Microsoft Graph API, and CalDAV for calendar synchronization in the backend.
API Resources should be used to transform model data for API responses in the Laravel backend.
Webhook handlers in the Laravel backend should process real-time calendar updates from external providers.
The backend should use Laravel queues for background job processing.
Webhook handlers in the backend should validate signatures for external calendar providers.
API authentication should use Laravel Sanctum tokens.
Device-specific authentication and display assignment should be implemented in the backend.
User activity tracking and session management should be implemented in the backend.
Files:
backend/app/Policies/DisplayPolicy.phpbackend/app/Http/Controllers/DisplayController.phpbackend/app/Models/Display.phpbackend/resources/views/pages/dashboard.blade.phpbackend/app/Models/User.php
backend/app/Models/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
Eloquent models for the Laravel backend should be placed in /backend/app/Models/ as PHP files.
Files:
backend/app/Models/Display.phpbackend/app/Models/User.php
🧠 Learnings (2)
📚 Learning: 2025-07-23T20:29:35.881Z
Learnt from: CR
Repo: magweter/spacepad PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-23T20:29:35.881Z
Learning: Applies to backend/**/*.php : Device-specific authentication and display assignment should be implemented in the backend.
Applied to files:
backend/app/Policies/DisplayPolicy.php
📚 Learning: 2025-07-23T20:29:35.881Z
Learnt from: CR
Repo: magweter/spacepad PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-23T20:29:35.881Z
Learning: Applies to backend/**/*.php : Integrate with Google Calendar API, Microsoft Graph API, and CalDAV for calendar synchronization in the backend.
Applied to files:
backend/app/Http/Controllers/DisplayController.php
🧬 Code graph analysis (1)
backend/app/Models/User.php (3)
backend/app/Models/WorkspaceMember.php (3)
user(38-41)workspace(30-33)WorkspaceMember(11-42)backend/app/Models/Workspace.php (4)
Workspace(13-131)devices(42-45)rooms(58-61)hasPro(121-130)backend/app/Policies/DisplayPolicy.php (1)
create(17-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-push
🔇 Additional comments (11)
backend/app/Models/Display.php (2)
46-49: LGTM!The
workspace()relationship is correctly defined using the explicitworkspace_idforeign key, aligning with the migration addingworkspace_idto the displays table.
56-59: Theevents()relationship is correctly configured.The Event model has
display_idproperly defined in the migration with a foreign key constraint (foreignUlid('display_id')->constrained()->onDelete('cascade')), it's included in the$fillablearray, and the inverseBelongsTorelationship to Display exists in the Event model. The relationship setup follows Laravel conventions and requires no changes.backend/resources/views/pages/dashboard.blade.php (2)
5-42: LGTM!The workspace selector with form auto-submit on change provides a clean UX for switching workspaces. The connect code display is appropriately gated behind user having displays or multiple workspaces.
444-461: LGTM with a minor note on environment disclosure.The self-hosted server info block is helpful for debugging. Note that displaying
$appEnvexposes the environment name (e.g., "production", "staging") to users. Ensure this is acceptable for your use case.backend/app/Models/User.php (3)
126-158: LGTM!The workspace relationship methods are well-structured. The
primaryWorkspace()method correctly falls back from owned workspaces to any workspace membership.
226-244: LGTM!The
hasProForCurrentWorkspace()method correctly checks both user-level Pro status and workspace-level Pro status (via workspace owners), providing proper multi-tenant Pro access.
340-362: LGTM!The
getSelectedWorkspace()method properly validates workspace membership and clears invalid session selections, preventing unauthorized workspace access.backend/app/Policies/DisplayPolicy.php (2)
25-46: LGTM!The
update()anddelete()methods correctly enforce workspace-scoped authorization usingcanBeManagedBy(), requiring owner/admin role for modifications. Denying access whenworkspace_idis null is a safe fallback for legacy data.
51-69: LGTM!The
view()method correctly handles both User and Device authentication paths. UsinghasMember()for view access (vscanBeManagedBy()for updates) appropriately allows regular workspace members to view displays without modification rights. Based on learnings, the device-specific authentication path is properly implemented.backend/app/Http/Controllers/DisplayController.php (2)
85-105: LGTM!The workspace resolution chain (request → session → primary) with access validation via
workspaces()->find()and permission check viacanBeManagedBy()properly enforces authorization boundaries.
167-218: LGTM!The
createCalendar()method correctly associatesworkspace_idanduser_idon bothCalendarandRoomrecords. Usingworkspace_idin thefirstOrCreateunique key ensures proper multi-tenant data isolation.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @backend/app/Http/Controllers/DashboardController.php:
- Around line 69-80: The log in DashboardController is currently recording PII
by including user->email; change logger()->info so it omits email and instead
records only a non-PII identifier (user->id) or an anonymized value (e.g., a
hash of user->id or email) before logging; locate the logger()->info call in
DashboardController and replace the 'email' field with either no entry or a
sanitized field (e.g., 'user_hash' computed from user->id or a one-way hash of
user->email) to avoid storing raw emails in logs.
🧹 Nitpick comments (6)
backend/app/Http/Controllers/DashboardController.php (2)
9-14: Remove unused imports.
InstanceService(line 9) andCalendar(line 11) are imported but never used in this file.Suggested fix
-use App\Services\InstanceService; use App\Models\Display; -use App\Models\Calendar; use App\Models\OutlookAccount;
50-67: Workspace-scoped data loading is well-structured.The conditional loading based on
$selectedWorkspacecorrectly initializes empty collections when no workspace is selected, preventing null-related issues downstream.One minor optimization: the
->with(['workspace', ...])eager load on line 52 is redundant since$selectedWorkspaceis already available.Optional: Remove redundant eager load
$displays = Display::where('workspace_id', $selectedWorkspace->id) - ->with(['workspace', 'calendar.outlookAccount', 'calendar.googleAccount', 'calendar.caldavAccount']) + ->with(['calendar.outlookAccount', 'calendar.googleAccount', 'calendar.caldavAccount']) ->get();backend/resources/views/pages/dashboard.blade.php (4)
46-50: Unused variable$showLicenseModal.The variable
$showLicenseModalis defined on line 49 but is never referenced anywhere in the template.Suggested fix
@php $isSelfHosted = config('settings.is_self_hosted'); $checkout = auth()->user()->getCheckoutUrl(route('billing.thanks')); - $showLicenseModal = $isSelfHosted && !auth()->user()->hasPro(); @endphp
33-33: Avoid redundant database query; use passed$workspacesvariable.
auth()->user()->workspaces()->count()triggers an additional database query when$workspacesis already available from the controller.Suggested fix
- @if((auth()->user()->hasAnyDisplay() || auth()->user()->workspaces()->count() > 1) && $connectCode) + @if((auth()->user()->hasAnyDisplay() || $workspaces->count() > 1) && $connectCode)
157-157: Same redundant query issue as above.Replace
auth()->user()->workspaces()->count() > 1with$workspaces->count() > 1to avoid an extra database query.Suggested fix
- @if(auth()->user()->hasAnyDisplay() || auth()->user()->workspaces()->count() > 1) + @if(auth()->user()->hasAnyDisplay() || $workspaces->count() > 1)
243-256: Duplicate conditional branches produce identical output.Lines 243-246 and 247-250 render the exact same HTML. The
$isSelfHosteddistinction is unnecessary here.Suggested fix
- @if(! $isSelfHosted && auth()->user()->shouldUpgradeForCurrentWorkspace()) - <span class="inline-flex items-center rounded-md bg-gray-100 px-3 py-2 text-center text-sm font-semibold text-gray-400 shadow-sm ring-1 ring-inset ring-gray-200 cursor-not-allowed" title="Upgrade to Pro to create more displays"> - <x-icons.plus class="h-5 w-5 mr-1" /> Create new display <span class="ml-2 inline-flex items-center rounded-md bg-blue-50 px-1.5 py-0.5 text-xs font-medium text-blue-700 ring-1 ring-inset ring-blue-600">Pro</span> - </span> - @elseif($isSelfHosted && auth()->user()->shouldUpgradeForCurrentWorkspace()) + @if(auth()->user()->shouldUpgradeForCurrentWorkspace()) <span class="inline-flex items-center rounded-md bg-gray-100 px-3 py-2 text-center text-sm font-semibold text-gray-400 shadow-sm ring-1 ring-inset ring-gray-200 cursor-not-allowed" title="Upgrade to Pro to create more displays"> <x-icons.plus class="h-5 w-5 mr-1" /> Create new display <span class="ml-2 inline-flex items-center rounded-md bg-blue-50 px-1.5 py-0.5 text-xs font-medium text-blue-700 ring-1 ring-inset ring-blue-600">Pro</span> </span>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/Http/Controllers/DashboardController.phpbackend/resources/views/pages/dashboard.blade.php
🧰 Additional context used
📓 Path-based instructions (1)
backend/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
backend/**/*.php: Use Laravel Sanctum for API authentication in the backend.
Integrate with Google Calendar API, Microsoft Graph API, and CalDAV for calendar synchronization in the backend.
API Resources should be used to transform model data for API responses in the Laravel backend.
Webhook handlers in the Laravel backend should process real-time calendar updates from external providers.
The backend should use Laravel queues for background job processing.
Webhook handlers in the backend should validate signatures for external calendar providers.
API authentication should use Laravel Sanctum tokens.
Device-specific authentication and display assignment should be implemented in the backend.
User activity tracking and session management should be implemented in the backend.
Files:
backend/app/Http/Controllers/DashboardController.phpbackend/resources/views/pages/dashboard.blade.php
🧬 Code graph analysis (1)
backend/app/Http/Controllers/DashboardController.php (3)
backend/app/Models/Display.php (1)
Display(14-211)backend/app/Models/User.php (4)
workspaces(137-142)getSelectedWorkspace(346-362)getConnectCode(175-189)primaryWorkspace(147-150)backend/app/Models/Workspace.php (1)
owners(74-77)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-push
🔇 Additional comments (3)
backend/resources/views/pages/dashboard.blade.php (2)
5-42: Workspace selector and connect code UI looks good.The workspace switching form with CSRF protection and role-based labeling is well-implemented. The conditional rendering based on workspace count and connect code availability handles edge cases properly.
456-474: Self-hosted server info section is well-structured.Displaying version and environment information for self-hosted deployments aids troubleshooting and provides useful context to administrators.
backend/app/Http/Controllers/DashboardController.php (1)
38-42: Code is correct. Theowners()relationship properly returns User model instances through the BelongsToMany relationship filtered bywherePivot(). CallinggetConnectCode()on the User model works as expected.
| logger()->info('Dashboard page accessed', [ | ||
| 'user_id' => $user->id, | ||
| 'email' => $user->email, | ||
| 'outlook_accounts_count' => $outlookAccounts->count(), | ||
| 'google_accounts_count' => $googleAccounts->count(), | ||
| 'caldav_accounts_count' => $caldavAccounts->count(), | ||
| 'displays_count' => $displays->count(), | ||
| 'workspaces_count' => $workspaces->count(), | ||
| 'selected_workspace_id' => $selectedWorkspace?->id, | ||
| 'ip' => request()->ip(), | ||
| 'user_agent' => substr(request()->userAgent() ?? '', 0, 100), | ||
| ]); |
There was a problem hiding this comment.
Logging user email constitutes PII retention.
Logging email directly may violate GDPR/CCPA compliance requirements. Consider logging only the user_id or a hashed/anonymized identifier, unless there's an explicit business need for the email in logs.
Suggested fix
logger()->info('Dashboard page accessed', [
'user_id' => $user->id,
- 'email' => $user->email,
'outlook_accounts_count' => $outlookAccounts->count(),
'google_accounts_count' => $googleAccounts->count(),
'caldav_accounts_count' => $caldavAccounts->count(),📝 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.
| logger()->info('Dashboard page accessed', [ | |
| 'user_id' => $user->id, | |
| 'email' => $user->email, | |
| 'outlook_accounts_count' => $outlookAccounts->count(), | |
| 'google_accounts_count' => $googleAccounts->count(), | |
| 'caldav_accounts_count' => $caldavAccounts->count(), | |
| 'displays_count' => $displays->count(), | |
| 'workspaces_count' => $workspaces->count(), | |
| 'selected_workspace_id' => $selectedWorkspace?->id, | |
| 'ip' => request()->ip(), | |
| 'user_agent' => substr(request()->userAgent() ?? '', 0, 100), | |
| ]); | |
| logger()->info('Dashboard page accessed', [ | |
| 'user_id' => $user->id, | |
| 'outlook_accounts_count' => $outlookAccounts->count(), | |
| 'google_accounts_count' => $googleAccounts->count(), | |
| 'caldav_accounts_count' => $caldavAccounts->count(), | |
| 'displays_count' => $displays->count(), | |
| 'workspaces_count' => $workspaces->count(), | |
| 'selected_workspace_id' => $selectedWorkspace?->id, | |
| 'ip' => request()->ip(), | |
| 'user_agent' => substr(request()->userAgent() ?? '', 0, 100), | |
| ]); |
🤖 Prompt for AI Agents
In @backend/app/Http/Controllers/DashboardController.php around lines 69 - 80,
The log in DashboardController is currently recording PII by including
user->email; change logger()->info so it omits email and instead records only a
non-PII identifier (user->id) or an anonymized value (e.g., a hash of user->id
or email) before logging; locate the logger()->info call in DashboardController
and replace the 'email' field with either no entry or a sanitized field (e.g.,
'user_hash' computed from user->id or a one-way hash of user->email) to avoid
storing raw emails in logs.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/Http/Controllers/AdminController.php (1)
170-225: Add timeouts, retries, and error logging to Lemon Squeezy API calls.
All three HTTP calls in this controller (lines 187, 246, 278) lack->timeout(),->retry(), and logging. Failed requests silently returnnull. Cold-cache admin loads trigger sequential API calls; add defensive measures consistent withOutlookServiceandEventServicepatterns.Proposed fix
- $subscriptionResponse = Http::withToken($apiKey) + $subscriptionResponse = Http::withToken($apiKey) ->withHeaders([ 'Accept' => 'application/vnd.api+json', ]) + ->timeout(10) + ->retry(2, 200) ->get("https://api.lemonsqueezy.com/v1/subscriptions/{$subscriptionId}"); if ($subscriptionResponse->successful()) { return $subscriptionResponse->json(); } + logger()->warning('Lemon Squeezy subscription fetch failed', [ + 'subscription_id' => $subscriptionId, + 'status' => $subscriptionResponse->status(), + ]); return null; @@ - } catch (\Exception $e) { + } catch (\Exception $e) { + logger()->warning('Lemon Squeezy subscription fetch exception', [ + 'subscription_id' => $subscriptionId, + 'error' => $e->getMessage(), + ]); return null; }Apply the same pattern to
getSubscriptionPrice()(lines 246–250) and price fetch (lines 278–281).
🤖 Fix all issues with AI agents
In @backend/app/Http/Controllers/AdminController.php:
- Around line 552-557: The logger calls in AdminController (e.g., the
logger()->info invocation that currently includes 'deleted_user_email' and
'deleted_by_admin_email') expose PII; update these log entries to remove email
fields and log only IDs (e.g., 'deleted_user_id', 'deleted_by_admin_id') or, if
emails are required, store a one-way hash instead. Locate all similar
logger()->info calls in this file (including the other occurrences referenced
around the logger blocks at the commented ranges) and replace email keys with
IDs only or hashed_email keys using a secure hash function before logging.
- Around line 567-598: After switching identities in the impersonate flow,
regenerate the session and CSRF token to prevent session fixation: in the
impersonate(User $user) method call session()->regenerate() and
session()->regenerateToken() immediately after Auth::login($user) (and keep
logging/session flags as before); do the same in the complementary method that
stops/restores impersonation (e.g., stopImpersonation or restoreImpersonation)
right after re-authenticating the original admin so both start and stop flows
regenerate the session and token.
- Around line 20-36: The check in checkAdminAccess currently aborts when
config('settings.is_self_hosted') is true and also uses Auth::user() without an
explicit guard; invert or clarify the self-hosted condition and add an explicit
error message: replace Auth::user() with Auth::guard('web')->user() (or the
appropriate guard), and change the admin/self-hosted check to either use if
(!$user || !$user->isAdmin() || !config('settings.is_self_hosted')) with a clear
abort(403, 'Admin access is disabled on this instance.') if self-hosted should
be disallowed, or keep the current logic but make it explicit by aborting with a
descriptive message (e.g., 'Admin panel disabled on self-hosted instances.') so
the behavior is unambiguous.
- Around line 363-404: The code uses $user->displays_count without loading it
and recomputes MRR instead of using the value returned by getSubscriptionData;
fix by loading the count before using it (e.g., call
$user->loadCount('displays') or include 'displays' in a withCount) and then call
getSubscriptionData($subscription->lemon_squeezy_id, $user->displays_count);
when building $subscriptionInfo use the returned MRR if present (e.g., 'mrr' =>
$subscriptionData['mrr'] ?? (($subscriptionData['price'] ?? 0) *
$user->displays_count)) and remove the unconditional manual MRR calculation.
- Around line 409-563: In deleteUser, the WorkspaceMember role update uses the
enum object instead of its stored value; change
WorkspaceMember::where(...)->where(...)->update(['role' =>
\App\Enums\WorkspaceRole::OWNER]) to use the enum value (e.g.
\App\Enums\WorkspaceRole::OWNER->value) so the pivot column stores the correct
scalar; locate this in the transfer ownership block inside the deleteUser method
where WorkspaceMember is updated.
🧹 Nitpick comments (2)
backend/app/Http/Controllers/AdminController.php (2)
38-76: Bug risk:$activeDisplaysis reused for two different meanings (count vs user collection).
Line 42 assigns an integer count, then Line 58 overwrites it with aCollection<User>, and lateractiveDisplaysCountbecomes “active users” count, not “active displays” count. Rename to avoid silently wrong metrics in the admin dashboard.Proposed fix (rename to avoid clobbering)
- $activeDisplays = Display::where('status', DisplayStatus::ACTIVE)->count(); + $activeDisplaysCount = Display::where('status', DisplayStatus::ACTIVE)->count(); $totalDisplays = Display::count(); @@ - $activeDisplays = User::query() + $activeDisplayUsers = User::query() ->whereHas('displays', function($q) use ($sevenDaysAgo) { $q->where('last_sync_at', '>=', $sevenDaysAgo); }) @@ - return view('pages.admin', [ + return view('pages.admin', [ 'activeInstances' => $activeInstances, - 'activeDisplays' => $activeDisplays, + 'activeDisplays' => $activeDisplayUsers, 'payingUsers' => $payingUsers, 'allUsers' => $allUsers, - 'activeDisplaysCount' => $activeDisplays->count(), + 'activeDisplaysCount' => $activeDisplaysCount, 'totalDisplays' => $totalDisplays,Also applies to: 155-167
430-558: deleteUser(): relationship “existence” checks don’t do what they look like.
if ($user->outlookAccounts)/if ($account->calendars)will be truthy even when empty (collections are objects). This is harmless but misleading and encourages future bugs—prefer just iterating the collection.Proposed simplification
- if ($user->outlookAccounts) { - foreach ($user->outlookAccounts as $account) { + foreach ($user->outlookAccounts as $account) { if ($account->calendars) { foreach ($account->calendars as $calendar) { $calendar->events()->delete(); $calendar->delete(); } } $account->delete(); - } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/Http/Controllers/AdminController.php
🧰 Additional context used
📓 Path-based instructions (1)
backend/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
backend/**/*.php: Use Laravel Sanctum for API authentication in the backend.
Integrate with Google Calendar API, Microsoft Graph API, and CalDAV for calendar synchronization in the backend.
API Resources should be used to transform model data for API responses in the Laravel backend.
Webhook handlers in the Laravel backend should process real-time calendar updates from external providers.
The backend should use Laravel queues for background job processing.
Webhook handlers in the backend should validate signatures for external calendar providers.
API authentication should use Laravel Sanctum tokens.
Device-specific authentication and display assignment should be implemented in the backend.
User activity tracking and session management should be implemented in the backend.
Files:
backend/app/Http/Controllers/AdminController.php
🪛 PHPMD (2.15.0)
backend/app/Http/Controllers/AdminController.php
367-367: Avoid unused local variables such as '$admin'. (undefined)
(UnusedLocalVariable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-push
🔇 Additional comments (1)
backend/app/Http/Controllers/AdminController.php (1)
132-154: Tests: high-risk admin flows need coverage (Codecov patch coverage is very low).
Strongly consider feature tests for: admin gate (incl. impersonation lockout), impersonate/stopImpersonating, and deleteUser workspace-owner transfer + cascades.Also applies to: 409-563, 567-634
| /** | ||
| * Check if the current request is authorized for admin access | ||
| */ | ||
| private function checkAdminAccess(): void | ||
| { | ||
| $user = Auth::user(); | ||
|
|
||
| // Prevent access if impersonating | ||
| if (session()->get('impersonating')) { | ||
| abort(403, 'Cannot access admin panel while impersonating. Please stop impersonating first.'); | ||
| } | ||
|
|
||
| // Check if current user is admin | ||
| if (!$user || !$user->isAdmin() || config('settings.is_self_hosted')) { | ||
| abort(403); | ||
| } | ||
| } |
There was a problem hiding this comment.
Admin access check likely blocks self-hosted forever + missing explicit guard context.
If config('settings.is_self_hosted') is meant to disable admin panel entirely, consider an explicit message (like the impersonation case) so it’s debuggable; otherwise this condition looks inverted.
🤖 Prompt for AI Agents
In @backend/app/Http/Controllers/AdminController.php around lines 20 - 36, The
check in checkAdminAccess currently aborts when
config('settings.is_self_hosted') is true and also uses Auth::user() without an
explicit guard; invert or clarify the self-hosted condition and add an explicit
error message: replace Auth::user() with Auth::guard('web')->user() (or the
appropriate guard), and change the admin/self-hosted check to either use if
(!$user || !$user->isAdmin() || !config('settings.is_self_hosted')) with a clear
abort(403, 'Admin access is disabled on this instance.') if self-hosted should
be disallowed, or keep the current logic but make it explicit by aborting with a
descriptive message (e.g., 'Admin panel disabled on self-hosted instances.') so
the behavior is unambiguous.
| public function showUser(User $user) | ||
| { | ||
| $this->checkAdminAccess(); | ||
|
|
||
| $admin = Auth::user(); | ||
|
|
||
| // Load user relationships for display | ||
| $user->load([ | ||
| 'outlookAccounts', | ||
| 'googleAccounts', | ||
| 'caldavAccounts', | ||
| 'displays', | ||
| 'devices', | ||
| 'workspaces', | ||
| 'subscriptions' => function($query) { | ||
| $query->where(function($q) { | ||
| $q->whereNull('ends_at') | ||
| ->orWhere('ends_at', '>', now()); | ||
| })->orderByDesc('created_at'); | ||
| }, | ||
| ]); | ||
|
|
||
| // Get subscription info | ||
| $subscriptionInfo = null; | ||
| if (!$user->is_unlimited && $user->subscriptions->isNotEmpty()) { | ||
| $subscription = $user->subscriptions->first(); | ||
| $subscriptionData = $this->getSubscriptionData($subscription->lemon_squeezy_id, $user->displays_count); | ||
| if ($subscriptionData) { | ||
| $subscriptionInfo = [ | ||
| 'status' => $subscriptionData['status'] ?? null, | ||
| 'price' => $subscriptionData['price'] ?? 0, | ||
| 'mrr' => ($subscriptionData['price'] ?? 0) * $user->displays_count, | ||
| 'ends_at' => $subscription->ends_at, | ||
| ]; | ||
| } | ||
| } | ||
|
|
||
| return view('pages.admin.user', [ | ||
| 'user' => $user, | ||
| 'subscriptionInfo' => $subscriptionInfo, | ||
| ]); | ||
| } |
There was a problem hiding this comment.
showUser(): displays_count is never loaded; subscription MRR math likely wrong.
You call getSubscriptionData(..., $user->displays_count) but displays_count isn’t loaded here; also mrr is recomputed as price * displays_count even though getSubscriptionData() already claims to return mrr.
Proposed fix (loadCount + use returned MRR)
public function showUser(User $user)
{
$this->checkAdminAccess();
-
- $admin = Auth::user();
+ $user->loadCount('displays');
// Load user relationships for display
$user->load([
@@
if (!$user->is_unlimited && $user->subscriptions->isNotEmpty()) {
$subscription = $user->subscriptions->first();
$subscriptionData = $this->getSubscriptionData($subscription->lemon_squeezy_id, $user->displays_count);
if ($subscriptionData) {
$subscriptionInfo = [
'status' => $subscriptionData['status'] ?? null,
- 'price' => $subscriptionData['price'] ?? 0,
- 'mrr' => ($subscriptionData['price'] ?? 0) * $user->displays_count,
+ 'price' => $subscriptionData['price'] ?? 0,
+ 'mrr' => $subscriptionData['mrr'] ?? ($subscriptionData['price'] ?? 0),
'ends_at' => $subscription->ends_at,
];
}
}📝 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 showUser(User $user) | |
| { | |
| $this->checkAdminAccess(); | |
| $admin = Auth::user(); | |
| // Load user relationships for display | |
| $user->load([ | |
| 'outlookAccounts', | |
| 'googleAccounts', | |
| 'caldavAccounts', | |
| 'displays', | |
| 'devices', | |
| 'workspaces', | |
| 'subscriptions' => function($query) { | |
| $query->where(function($q) { | |
| $q->whereNull('ends_at') | |
| ->orWhere('ends_at', '>', now()); | |
| })->orderByDesc('created_at'); | |
| }, | |
| ]); | |
| // Get subscription info | |
| $subscriptionInfo = null; | |
| if (!$user->is_unlimited && $user->subscriptions->isNotEmpty()) { | |
| $subscription = $user->subscriptions->first(); | |
| $subscriptionData = $this->getSubscriptionData($subscription->lemon_squeezy_id, $user->displays_count); | |
| if ($subscriptionData) { | |
| $subscriptionInfo = [ | |
| 'status' => $subscriptionData['status'] ?? null, | |
| 'price' => $subscriptionData['price'] ?? 0, | |
| 'mrr' => ($subscriptionData['price'] ?? 0) * $user->displays_count, | |
| 'ends_at' => $subscription->ends_at, | |
| ]; | |
| } | |
| } | |
| return view('pages.admin.user', [ | |
| 'user' => $user, | |
| 'subscriptionInfo' => $subscriptionInfo, | |
| ]); | |
| } | |
| public function showUser(User $user) | |
| { | |
| $this->checkAdminAccess(); | |
| $user->loadCount('displays'); | |
| // Load user relationships for display | |
| $user->load([ | |
| 'outlookAccounts', | |
| 'googleAccounts', | |
| 'caldavAccounts', | |
| 'displays', | |
| 'devices', | |
| 'workspaces', | |
| 'subscriptions' => function($query) { | |
| $query->where(function($q) { | |
| $q->whereNull('ends_at') | |
| ->orWhere('ends_at', '>', now()); | |
| })->orderByDesc('created_at'); | |
| }, | |
| ]); | |
| // Get subscription info | |
| $subscriptionInfo = null; | |
| if (!$user->is_unlimited && $user->subscriptions->isNotEmpty()) { | |
| $subscription = $user->subscriptions->first(); | |
| $subscriptionData = $this->getSubscriptionData($subscription->lemon_squeezy_id, $user->displays_count); | |
| if ($subscriptionData) { | |
| $subscriptionInfo = [ | |
| 'status' => $subscriptionData['status'] ?? null, | |
| 'price' => $subscriptionData['price'] ?? 0, | |
| 'mrr' => $subscriptionData['mrr'] ?? ($subscriptionData['price'] ?? 0), | |
| 'ends_at' => $subscription->ends_at, | |
| ]; | |
| } | |
| } | |
| return view('pages.admin.user', [ | |
| 'user' => $user, | |
| 'subscriptionInfo' => $subscriptionInfo, | |
| ]); | |
| } |
🧰 Tools
🪛 PHPMD (2.15.0)
367-367: Avoid unused local variables such as '$admin'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
In @backend/app/Http/Controllers/AdminController.php around lines 363 - 404, The
code uses $user->displays_count without loading it and recomputes MRR instead of
using the value returned by getSubscriptionData; fix by loading the count before
using it (e.g., call $user->loadCount('displays') or include 'displays' in a
withCount) and then call getSubscriptionData($subscription->lemon_squeezy_id,
$user->displays_count); when building $subscriptionInfo use the returned MRR if
present (e.g., 'mrr' => $subscriptionData['mrr'] ?? (($subscriptionData['price']
?? 0) * $user->displays_count)) and remove the unconditional manual MRR
calculation.
| public function deleteUser(Request $request, User $user): RedirectResponse | ||
| { | ||
| $this->checkAdminAccess(); | ||
|
|
||
| $admin = Auth::user(); | ||
|
|
||
| // Prevent deleting yourself | ||
| if ($user->id === $admin->id) { | ||
| return redirect()->route('admin.index') | ||
| ->with('error', 'You cannot delete your own account.'); | ||
| } | ||
|
|
||
| // Confirm deletion | ||
| $request->validate([ | ||
| 'confirm_email' => ['required', 'email'], | ||
| ]); | ||
|
|
||
| if ($request->input('confirm_email') !== $user->email) { | ||
| return back()->withErrors(['confirm_email' => 'Email confirmation does not match.']); | ||
| } | ||
|
|
||
| DB::transaction(function () use ($user, $admin) { | ||
| // Delete all user's personal access tokens | ||
| $user->tokens()->delete(); | ||
|
|
||
| // Delete displays and their related data first (before calendars/accounts) | ||
| if ($user->displays) { | ||
| foreach ($user->displays as $display) { | ||
| // Delete event subscriptions | ||
| $display->eventSubscriptions()->delete(); | ||
| // Delete display settings | ||
| $display->settings()->delete(); | ||
| // Delete events associated with this display | ||
| $display->events()->delete(); | ||
| // Delete devices associated with this display | ||
| $display->devices()->delete(); | ||
| $display->delete(); | ||
| } | ||
| } | ||
|
|
||
| // Delete devices (standalone devices not linked to displays) | ||
| $user->devices()->delete(); | ||
|
|
||
| // Delete rooms | ||
| $user->rooms()->delete(); | ||
|
|
||
| // Delete Outlook accounts and their calendars/events | ||
| if ($user->outlookAccounts) { | ||
| foreach ($user->outlookAccounts as $account) { | ||
| if ($account->calendars) { | ||
| foreach ($account->calendars as $calendar) { | ||
| $calendar->events()->delete(); | ||
| $calendar->delete(); | ||
| } | ||
| } | ||
| $account->delete(); | ||
| } | ||
| } | ||
|
|
||
| // Delete Google accounts and their calendars/events | ||
| if ($user->googleAccounts) { | ||
| foreach ($user->googleAccounts as $account) { | ||
| if ($account->calendars) { | ||
| foreach ($account->calendars as $calendar) { | ||
| $calendar->events()->delete(); | ||
| $calendar->delete(); | ||
| } | ||
| } | ||
| $account->delete(); | ||
| } | ||
| } | ||
|
|
||
| // Delete CalDAV accounts and their calendars/events | ||
| if ($user->caldavAccounts) { | ||
| foreach ($user->caldavAccounts as $account) { | ||
| if ($account->calendars) { | ||
| foreach ($account->calendars as $calendar) { | ||
| $calendar->events()->delete(); | ||
| $calendar->delete(); | ||
| } | ||
| } | ||
| $account->delete(); | ||
| } | ||
| } | ||
|
|
||
| // Delete any remaining calendars directly linked to user (shouldn't happen, but safety check) | ||
| // Note: Calendars are linked through accounts, not directly to users, so this is unlikely | ||
| // Events are deleted through calendars above | ||
|
|
||
| // Handle workspaces | ||
| $ownedWorkspaces = $user->ownedWorkspaces()->get(); | ||
| foreach ($ownedWorkspaces as $workspace) { | ||
| // Get other members (excluding the user being deleted) | ||
| $otherMembers = $workspace->members()->where('user_id', '!=', $user->id)->get(); | ||
|
|
||
| if ($otherMembers->isNotEmpty()) { | ||
| // Find first admin or first member to transfer ownership | ||
| $newOwner = $otherMembers->first(function ($member) { | ||
| return $member->pivot->role === \App\Enums\WorkspaceRole::ADMIN->value; | ||
| }) ?? $otherMembers->first(); | ||
|
|
||
| if ($newOwner) { | ||
| // Transfer ownership | ||
| WorkspaceMember::where('workspace_id', $workspace->id) | ||
| ->where('user_id', $newOwner->id) | ||
| ->update(['role' => \App\Enums\WorkspaceRole::OWNER]); | ||
| } | ||
| } else { | ||
| // No other members, delete the workspace and all its data | ||
| foreach ($workspace->displays as $display) { | ||
| $display->eventSubscriptions()->delete(); | ||
| $display->settings()->delete(); | ||
| $display->events()->delete(); | ||
| $display->devices()->delete(); | ||
| $display->delete(); | ||
| } | ||
| $workspace->devices()->delete(); | ||
| foreach ($workspace->calendars as $calendar) { | ||
| $calendar->events()->delete(); | ||
| $calendar->delete(); | ||
| } | ||
| $workspace->rooms()->delete(); | ||
| WorkspaceMember::where('workspace_id', $workspace->id)->delete(); | ||
| $workspace->delete(); | ||
| } | ||
| } | ||
|
|
||
| // Delete workspace memberships (user's membership in workspaces they don't own) | ||
| WorkspaceMember::where('user_id', $user->id)->delete(); | ||
|
|
||
| // Note: Instances are system-wide (for self-hosted tracking), not user-specific | ||
| // No need to delete instances when deleting a user | ||
|
|
||
| // Cancel LemonSqueezy subscriptions (if any) | ||
| // Note: This doesn't actually cancel them in LemonSqueezy, just removes the local reference | ||
| // You might want to add API call to cancel subscriptions | ||
| if (method_exists($user, 'subscriptions')) { | ||
| $user->subscriptions()->delete(); | ||
| } | ||
|
|
||
| // Finally, delete the user | ||
| $user->delete(); | ||
|
|
||
| logger()->info('User account deleted by admin', [ | ||
| 'deleted_user_id' => $user->id, | ||
| 'deleted_user_email' => $user->email, | ||
| 'deleted_by_admin_id' => $admin->id, | ||
| 'deleted_by_admin_email' => $admin->email, | ||
| ]); | ||
| }); | ||
|
|
||
| return redirect()->route('admin.index') | ||
| ->with('success', "User account {$user->email} and all associated data have been permanently deleted."); | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1. Find and examine the User model to see ownedWorkspaces() definition
echo "=== Searching for ownedWorkspaces() definition ==="
find backend -name "User.php" -path "*/Models/*" | head -5
rg -n "ownedWorkspaces|function ownedWorkspaces" backend/app/Models/User.php -A 5 2>/dev/null || echo "Not found in User.php, searching elsewhere..."
# 2. Search more broadly for ownedWorkspaces
echo "=== Broader search for ownedWorkspaces ==="
rg -n "ownedWorkspaces" backend --type=php -B 2 -A 3
# 3. Examine Workspace model for owner_id
echo "=== Checking Workspace model structure ==="
find backend -name "Workspace.php" -path "*/Models/*" | head -3
cat backend/app/Models/Workspace.php 2>/dev/null | head -100
# 4. Check migrations for workspace table
echo "=== Checking Workspace migrations ==="
find backend/database/migrations -name "*workspace*" 2>/dev/null | head -3Repository: magweter/spacepad
Length of output: 4790
🏁 Script executed:
#!/bin/bash
# Check workspace migration files
echo "=== Checking workspace migrations for owner_id ==="
cat backend/database/migrations/2025_12_30_000000_create_workspaces_table.php
echo ""
echo "=== Checking workspace_members migration ==="
cat backend/database/migrations/2025_12_30_000001_create_workspace_members_table.php
echo ""
# Verify enum usage pattern in codebase
echo "=== Checking how enums are used elsewhere ==="
rg "WorkspaceRole::" backend/app --type=php | head -20Repository: magweter/spacepad
Length of output: 2791
Fix enum storage in workspace role update
Line 516: ->update(['role' => \App\Enums\WorkspaceRole::OWNER]) should be ->update(['role' => \App\Enums\WorkspaceRole::OWNER->value]).
Ownership is managed through the workspace_members.role pivot column, not through a workspaces.owner_id FK. The current code will fail when attempting to store the enum object as a string value.
🤖 Prompt for AI Agents
In @backend/app/Http/Controllers/AdminController.php around lines 409 - 563, In
deleteUser, the WorkspaceMember role update uses the enum object instead of its
stored value; change WorkspaceMember::where(...)->where(...)->update(['role' =>
\App\Enums\WorkspaceRole::OWNER]) to use the enum value (e.g.
\App\Enums\WorkspaceRole::OWNER->value) so the pivot column stores the correct
scalar; locate this in the transfer ownership block inside the deleteUser method
where WorkspaceMember is updated.
| logger()->info('User account deleted by admin', [ | ||
| 'deleted_user_id' => $user->id, | ||
| 'deleted_user_email' => $user->email, | ||
| 'deleted_by_admin_id' => $admin->id, | ||
| 'deleted_by_admin_email' => $admin->email, | ||
| ]); |
There was a problem hiding this comment.
Privacy/PII: avoid logging emails in admin actions.
The logs include user/admin emails; consider logging only IDs (or hashing emails) to reduce PII exposure and compliance burden.
Proposed fix (IDs only)
logger()->info('User account deleted by admin', [
'deleted_user_id' => $user->id,
- 'deleted_user_email' => $user->email,
'deleted_by_admin_id' => $admin->id,
- 'deleted_by_admin_email' => $admin->email,
]);Also applies to: 589-594, 625-630
🤖 Prompt for AI Agents
In @backend/app/Http/Controllers/AdminController.php around lines 552 - 557, The
logger calls in AdminController (e.g., the logger()->info invocation that
currently includes 'deleted_user_email' and 'deleted_by_admin_email') expose
PII; update these log entries to remove email fields and log only IDs (e.g.,
'deleted_user_id', 'deleted_by_admin_id') or, if emails are required, store a
one-way hash instead. Locate all similar logger()->info calls in this file
(including the other occurrences referenced around the logger blocks at the
commented ranges) and replace email keys with IDs only or hashed_email keys
using a secure hash function before logging.
| public function impersonate(User $user): RedirectResponse | ||
| { | ||
| $this->checkAdminAccess(); | ||
|
|
||
| $admin = Auth::user(); | ||
|
|
||
| // Prevent impersonating yourself | ||
| if ($admin->id === $user->id) { | ||
| return redirect()->route('admin.index') | ||
| ->with('error', 'You cannot impersonate yourself.'); | ||
| } | ||
|
|
||
| // Store original admin ID in session | ||
| session()->put('impersonating', true); | ||
| session()->put('impersonator_id', $admin->id); | ||
|
|
||
| // Clear any workspace selection from admin session - let impersonated user's workspace be selected | ||
| session()->forget('selected_workspace_id'); | ||
|
|
||
| // Log in as the target user | ||
| Auth::login($user); | ||
|
|
||
| logger()->info('Admin started impersonating user', [ | ||
| 'admin_id' => $admin->id, | ||
| 'admin_email' => $admin->email, | ||
| 'impersonated_user_id' => $user->id, | ||
| 'impersonated_user_email' => $user->email, | ||
| ]); | ||
|
|
||
| return redirect()->route('dashboard') | ||
| ->with('success', "You are now impersonating {$user->email}"); | ||
| } |
There was a problem hiding this comment.
Security: impersonation should regenerate session to prevent session fixation / cross-user leakage.
After swapping identities, call session()->regenerate() (and usually regenerateToken()), both when starting and stopping impersonation.
Proposed fix (regenerate session on identity switch)
// Log in as the target user
Auth::login($user);
+ request()->session()->regenerate();
+ request()->session()->regenerateToken();
@@
// Log back in as admin
Auth::login($impersonator);
+ request()->session()->regenerate();
+ request()->session()->regenerateToken();Also applies to: 603-634
🤖 Prompt for AI Agents
In @backend/app/Http/Controllers/AdminController.php around lines 567 - 598,
After switching identities in the impersonate flow, regenerate the session and
CSRF token to prevent session fixation: in the impersonate(User $user) method
call session()->regenerate() and session()->regenerateToken() immediately after
Auth::login($user) (and keep logging/session flags as before); do the same in
the complementary method that stops/restores impersonation (e.g.,
stopImpersonation or restoreImpersonation) right after re-authenticating the
original admin so both start and stop flows regenerate the session and token.
2 participants
Summary by CodeRabbit
New Features
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.