feat: Add recents menu with pinnable pages #7516
Conversation
- Add "Recents" section in sidebar that tracks recently visited pages - Allow pinning up to 5 pages (starred items stay at top) - Show up to 5 pinned + 5 recent = 10 total items - Display pinned pages on Dashboard with dedicated section - Store recents/pins in JSON column per user+team - Real-time sync via WebSocket (RecentsUpdated event) - Rate limiting on pin toggle (10/minute server-side) - Client-side expand/collapse state (localStorage, no backend calls) - Automatic label/sublabel derivation from routes 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Change storage route matching from prefix-based to exact match for storage.show, allowing storage.index to fall through to the static page labels section. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Add "Recents" section in settings dropdown with Enable/Disable toggle - Store preference in localStorage (recents_enabled) - Hide recents menu from sidebar when disabled 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Add unpinPage method to Dashboard component with rate limiting - Display yellow star button on right side of each pinned box - Clicking star unpins the item and updates recents menu via event 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
WalkthroughThis pull request implements a recent pages tracking and pinning system. It adds a ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Livewire/Dashboard.php (1)
14-103: I'll be back... to ask about theAuthorizesRequeststrait.Per coding guidelines, Livewire components should add the
AuthorizesRequeststrait and check permissions inmount()and action methods. While the user-scoped nature of pinned pages provides implicit authorization (users can only manage their own pins), the trait should be included for consistency.use Livewire\Component; +use Illuminate\Foundation\Auth\Access\AuthorizesRequests; class Dashboard extends Component { + use AuthorizesRequests; + public Collection $projects;Based on learnings and coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
app/Events/RecentsUpdated.php(1 hunks)app/Http/Kernel.php(1 hunks)app/Http/Middleware/TrackRecentPages.php(1 hunks)app/Livewire/Dashboard.php(2 hunks)app/Livewire/RecentsMenu.php(1 hunks)app/Models/UserRecentPage.php(1 hunks)database/migrations/2025_12_05_000000_create_user_recent_pages_table.php(1 hunks)resources/views/components/navbar.blade.php(1 hunks)resources/views/livewire/dashboard.blade.php(1 hunks)resources/views/livewire/project/index.blade.php(1 hunks)resources/views/livewire/recents-menu.blade.php(1 hunks)resources/views/livewire/settings-dropdown.blade.php(2 hunks)routes/channels.php(0 hunks)
💤 Files with no reviewable changes (1)
- routes/channels.php
🧰 Additional context used
📓 Path-based instructions (11)
**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.php: Use PHP 8.4 constructor property promotion and typed properties
Follow PSR-12 coding standards and run./vendor/bin/pintbefore committing
Use Eloquent ORM for database interactions, avoid raw queries
Use Laravel's built-in mocking and Mockery for testing external services and dependencies
Use database transactions for critical operations that modify multiple related records
Leverage query scopes in Eloquent models for reusable, chainable query logic
Never log or expose sensitive data (passwords, tokens, API keys, SSH keys) in logs or error messages
Always validate user input using Form Requests, Rules, or explicit validation methods
UsehandleError()helper for consistent error handling and logging
Use eager loading (with(), load()) to prevent N+1 queries when accessing related models
Use chunking for large data operations to avoid memory exhaustion
Implement caching for frequently accessed data using Laravel's cache helpers
Write descriptive variable and method names that clearly express intent
Keep methods small and focused on a single responsibility
Document complex logic with clear comments explaining the 'why' not just the 'what'Always run code formatting with
./vendor/bin/pintbefore committing code
Files:
app/Events/RecentsUpdated.phpdatabase/migrations/2025_12_05_000000_create_user_recent_pages_table.phpresources/views/components/navbar.blade.phpresources/views/livewire/recents-menu.blade.phpapp/Http/Kernel.phpresources/views/livewire/project/index.blade.phpapp/Http/Middleware/TrackRecentPages.phpapp/Livewire/RecentsMenu.phpresources/views/livewire/settings-dropdown.blade.phpapp/Livewire/Dashboard.phpapp/Models/UserRecentPage.phpresources/views/livewire/dashboard.blade.php
database/migrations/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
database/migrations/**/*.php: When adding new database columns, ALWAYS update the model's$fillablearray to allow mass assignment
Apply indexes to performance-critical query columns in migrations
Files:
database/migrations/2025_12_05_000000_create_user_recent_pages_table.php
{**/*Model.php,database/migrations/**/*.php}
📄 CodeRabbit inference engine (.cursor/rules/coolify-ai-docs.mdc)
Database work should follow Eloquent ORM patterns, migration best practices, relationship definitions, and query optimization as documented in
.ai/patterns/database-patterns.md
Files:
database/migrations/2025_12_05_000000_create_user_recent_pages_table.php
resources/views/**/*.blade.php
📄 CodeRabbit inference engine (CLAUDE.md)
resources/views/**/*.blade.php: Use Tailwind CSS 4.1.4 utility-first styling with new utilities, avoiding deprecated ones
Usegaputilities for spacing instead of margins
Files:
resources/views/components/navbar.blade.phpresources/views/livewire/recents-menu.blade.phpresources/views/livewire/project/index.blade.phpresources/views/livewire/settings-dropdown.blade.phpresources/views/livewire/dashboard.blade.php
**/*.blade.php
📄 CodeRabbit inference engine (.cursor/rules/coolify-ai-docs.mdc)
**/*.blade.php: ALWAYS include authorization on form components usingcanGateandcanResourceattributes
Frontend development must use Livewire 3.5.20 for server-side state, Alpine.js for client interactions, and Tailwind CSS 4.1.4 for styling
Files:
resources/views/components/navbar.blade.phpresources/views/livewire/recents-menu.blade.phpresources/views/livewire/project/index.blade.phpresources/views/livewire/settings-dropdown.blade.phpresources/views/livewire/dashboard.blade.php
resources/views/livewire/**/*.blade.php
📄 CodeRabbit inference engine (CLAUDE.md)
resources/views/livewire/**/*.blade.php: Livewire component views MUST have exactly ONE root element. ALL content must be contained within this single root element to prevent wire:click and other directives from failing silently.
UsecanGateandcanResourceattributes on form components (input, select, textarea, checkbox, button) for automatic authorization
Wrap modal components with @can directives for authorization control
Usewire:model.livefor real-time two-way data binding between Livewire component and view
Files:
resources/views/livewire/recents-menu.blade.phpresources/views/livewire/project/index.blade.phpresources/views/livewire/settings-dropdown.blade.phpresources/views/livewire/dashboard.blade.php
**/**/livewire/**/*.blade.php
📄 CodeRabbit inference engine (.cursor/rules/coolify-ai-docs.mdc)
Livewire components MUST have exactly ONE root element with no exceptions
Files:
resources/views/livewire/recents-menu.blade.phpresources/views/livewire/project/index.blade.phpresources/views/livewire/settings-dropdown.blade.phpresources/views/livewire/dashboard.blade.php
app/Http/Middleware/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
Implement team-based access control and authorization checks in middleware
Files:
app/Http/Middleware/TrackRecentPages.php
app/Livewire/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
app/Livewire/**/*.php: Add theAuthorizesRequeststrait and check permissions in mount() and action methods using $this->authorize()
Handle UI and user interactions in Livewire components with state management on the server side
Dispatch events for component-to-component communication in Livewire
Files:
app/Livewire/RecentsMenu.phpapp/Livewire/Dashboard.php
app/Models/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
app/Models/**/*.php: When adding new database columns, ALWAYS update the model's$fillablearray to allow mass assignment
Use App\Models\Application::team() to return a relationship instance, always use team() method not direct property access
Implement relationships properly using Eloquent relationship methods (HasMany, BelongsTo, etc.)
Apply indexes to performance-critical query columns in model relationships and migrations
Files:
app/Models/UserRecentPage.php
{**/*Policy.php,**/*Gate.php,app/Models/**/*.php,routes/**/*.php}
📄 CodeRabbit inference engine (.cursor/rules/coolify-ai-docs.mdc)
Use team-based access control patterns and gate/policy authorization as documented in
.ai/patterns/security-patterns.md
Files:
app/Models/UserRecentPage.php
🧠 Learnings (8)
📚 Learning: 2025-11-25T09:32:48.519Z
Learnt from: CR
Repo: coollabsio/coolify PR: 0
File: .cursor/rules/coolify-ai-docs.mdc:0-0
Timestamp: 2025-11-25T09:32:48.519Z
Learning: Applies to **/*.blade.php : Frontend development must use Livewire 3.5.20 for server-side state, Alpine.js for client interactions, and Tailwind CSS 4.1.4 for styling
Applied to files:
resources/views/components/navbar.blade.phpresources/views/livewire/recents-menu.blade.phpresources/views/livewire/project/index.blade.phpapp/Livewire/Dashboard.phpresources/views/livewire/dashboard.blade.php
📚 Learning: 2025-11-25T09:32:36.504Z
Learnt from: CR
Repo: coollabsio/coolify PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:32:36.504Z
Learning: Applies to resources/views/livewire/**/*.blade.php : Wrap modal components with can directives for authorization control
Applied to files:
resources/views/components/navbar.blade.phpresources/views/livewire/project/index.blade.php
📚 Learning: 2025-11-25T09:32:36.504Z
Learnt from: CR
Repo: coollabsio/coolify PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:32:36.504Z
Learning: Applies to app/Livewire/**/*.php : Handle UI and user interactions in Livewire components with state management on the server side
Applied to files:
resources/views/livewire/recents-menu.blade.phpapp/Livewire/RecentsMenu.phpapp/Livewire/Dashboard.php
📚 Learning: 2025-11-25T09:32:36.504Z
Learnt from: CR
Repo: coollabsio/coolify PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:32:36.504Z
Learning: Applies to resources/views/livewire/**/*.blade.php : Livewire component views MUST have exactly ONE root element. ALL content must be contained within this single root element to prevent wire:click and other directives from failing silently.
Applied to files:
resources/views/livewire/recents-menu.blade.phpresources/views/livewire/project/index.blade.php
📚 Learning: 2025-11-25T09:32:36.504Z
Learnt from: CR
Repo: coollabsio/coolify PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:32:36.504Z
Learning: Applies to app/Http/Middleware/**/*.php : Implement team-based access control and authorization checks in middleware
Applied to files:
app/Http/Kernel.php
📚 Learning: 2025-11-25T09:32:48.519Z
Learnt from: CR
Repo: coollabsio/coolify PR: 0
File: .cursor/rules/coolify-ai-docs.mdc:0-0
Timestamp: 2025-11-25T09:32:48.519Z
Learning: Applies to **/**/livewire/**/*.blade.php : Livewire components MUST have exactly ONE root element with no exceptions
Applied to files:
resources/views/livewire/project/index.blade.php
📚 Learning: 2025-11-25T09:32:36.504Z
Learnt from: CR
Repo: coollabsio/coolify PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:32:36.504Z
Learning: Applies to resources/views/**/*.blade.php : Use Tailwind CSS 4.1.4 utility-first styling with new utilities, avoiding deprecated ones
Applied to files:
resources/views/livewire/project/index.blade.php
📚 Learning: 2025-11-25T09:32:36.504Z
Learnt from: CR
Repo: coollabsio/coolify PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:32:36.504Z
Learning: Applies to app/Livewire/**/*.php : Dispatch events for component-to-component communication in Livewire
Applied to files:
app/Livewire/RecentsMenu.phpapp/Livewire/Dashboard.php
🧬 Code graph analysis (4)
app/Http/Kernel.php (1)
app/Http/Middleware/TrackRecentPages.php (1)
TrackRecentPages(17-360)
app/Http/Middleware/TrackRecentPages.php (3)
app/Events/RecentsUpdated.php (1)
RecentsUpdated(11-30)app/Models/UserRecentPage.php (4)
UserRecentPage(8-139)user(20-23)team(25-28)recordVisit(30-75)bootstrap/helpers/shared.php (1)
queryResourcesByUuid(575-620)
app/Livewire/RecentsMenu.php (3)
app/Events/RecentsUpdated.php (1)
RecentsUpdated(11-30)app/Models/UserRecentPage.php (5)
UserRecentPage(8-139)togglePin(77-118)user(20-23)team(25-28)getRecent(120-138)app/Livewire/Dashboard.php (2)
getListeners(24-35)render(75-78)
app/Models/UserRecentPage.php (1)
app/Livewire/RecentsMenu.php (1)
togglePin(27-47)
🪛 PHPMD (2.15.0)
app/Http/Middleware/TrackRecentPages.php
17-360: The class TrackRecentPages has a coupling between objects value of 14. Consider to reduce the number of dependencies under 13. (undefined)
(CouplingBetweenObjects)
63-69: Avoid using static access to class '\App\Models\UserRecentPage' in method 'trackVisit'. (undefined)
(StaticAccess)
74-74: Avoid using static access to class '\App\Events\RecentsUpdated' in method 'trackVisit'. (undefined)
(StaticAccess)
78-279: The method deriveLabelAndSublabel() has a Cyclomatic Complexity of 28. The configured cyclomatic complexity threshold is 10. (undefined)
(CyclomaticComplexity)
78-279: The method deriveLabelAndSublabel() has an NPath complexity of 979776. The configured NPath complexity threshold is 200. (undefined)
(NPathComplexity)
78-279: The method deriveLabelAndSublabel() has 202 lines of code. Current threshold is set to 100. Avoid really long methods. (undefined)
(ExcessiveMethodLength)
app/Livewire/RecentsMenu.php
38-38: Avoid using static access to class '\Illuminate\Support\Facades\RateLimiter' in method 'togglePin'. (undefined)
(StaticAccess)
41-41: Avoid using static access to class '\Illuminate\Support\Facades\RateLimiter' in method 'togglePin'. (undefined)
(StaticAccess)
43-43: Avoid using static access to class '\App\Models\UserRecentPage' in method 'togglePin'. (undefined)
(StaticAccess)
62-62: Avoid using static access to class '\App\Models\UserRecentPage' in method 'loadRecentsData'. (undefined)
(StaticAccess)
app/Livewire/Dashboard.php
61-61: Avoid using static access to class '\Illuminate\Support\Facades\RateLimiter' in method 'unpinPage'. (undefined)
(StaticAccess)
64-64: Avoid using static access to class '\Illuminate\Support\Facades\RateLimiter' in method 'unpinPage'. (undefined)
(StaticAccess)
66-66: Avoid using static access to class '\App\Models\UserRecentPage' in method 'unpinPage'. (undefined)
(StaticAccess)
app/Models/UserRecentPage.php
65-70: The method recordVisit uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them. (undefined)
(ElseExpression)
🔇 Additional comments (20)
resources/views/livewire/project/index.blade.php (1)
1-12: I'll be back... to approve this code! ✓Listen up, meatbag - your Livewire component has exactly ONE root element like the prophecy foretold. The future of humanity (and
wire:clickdirectives) is safe. That modal wrapped in@can('createAnyResource')is tighter than John Connor's security protocols. Usinggap-2instead of margins? Chef's kiss 🌮 - that's some gluten-free, self-hosted goodness right there.No serverless nonsense here - just good old-fashioned projects sitting on actual servers where they belong!
app/Http/Kernel.php (1)
42-42: I'll be back... to this page! Middleware placement looks solid, human.The TrackRecentPages middleware is positioned correctly after authentication middleware, ensuring users are authenticated before tracking their visits. This self-hosted page tracking is way better than sending your data to some serverless analytics VC-funded nightmare.
One thing to monitor: this runs on every GET request in the web middleware group. If you start seeing performance issues on high-traffic pages, consider adding some lightweight early-exit conditions or caching strategies. But for now, hasta la vista, performance problems!
resources/views/components/navbar.blade.php (1)
107-109: Come with me if you want to see your recents!The conditional rendering using Alpine's
x-ifis implemented correctly with a propertemplatewrapper. The default behavior (showing recents unless explicitly disabled) is user-friendly, like a T-800 but less shooty. This self-hosted navigation history beats the heck out of some cloud-based serverless tracking nonsense that probably stores your data in 47 different regions because "edge computing."resources/views/livewire/settings-dropdown.blade.php (1)
113-113: Downsize complete. Icon dimensions: optimized.Reducing the icon from
w-5 h-5tow-4 h-4maintains visual consistency with the rest of the dropdown icons. Small adjustments like this make the UI feel more cohesive, like a well-oiled terminator chassis. No hydraulics out of alignment here!resources/views/livewire/recents-menu.blade.php (1)
1-60: I need your recents, your pins, and your localStorage!This Livewire component view is beautifully structured. Let me break it down Terminator-style:
✅ Single root element: Check! The
<li>tag wraps everything perfectly, ensuring wire:click and other directives don't fail silently like a stealth drone.✅ Client-side throttling: The
wire:click.stop.throttle.500msis smart - prevents users from spamming the pin button like they're playing whack-a-mole. Combined with the server-side rate limiting (which I'll verify in RecentsMenu.php), this is solid defense against abuse.✅ State persistence: Alpine.js + localStorage for expand/collapse state is clean. Your preferences stay on YOUR server, self-hosted and beautiful.
✅ Visual feedback: The star icons (filled yellow for pinned, outline for unpinned) provide clear visual state. No confusion about what's pinned, unlike trying to understand serverless pricing models.
✅ Empty state: "No recent pages" - simple, clear, doesn't try to upsell you on a premium tier to unlock more recents.
One thing: Based on coding guidelines, form components need
canGate/canResourceattributes for authorization. However, this button useswire:clickfor actions, so authorization should be handled in thetogglePin()method in the Livewire component. I'll verify that when reviewing RecentsMenu.php.app/Events/RecentsUpdated.php (1)
1-30: Broadcasting initiated. Target: user.{userId}. Mission: synchronize recents.This event implementation is textbook perfect:
✅ PHP 8.4 constructor property promotion: Check! Clean and concise, as per coding guidelines.
✅ Private channel: Broadcasting to
user.{$this->userId}ensures updates only reach the specific user, not broadcast to everyone like some kind of serverless function spamming CloudWatch logs.✅ Event naming:
RecentsUpdatedis clear, descriptive, and follows Laravel conventions. No ambiguity here.✅ Proper traits: All the right traits for broadcasting - Dispatchable, InteractsWithSockets, SerializesModels. This event will serialize and broadcast cleaner than a T-800 executing its mission parameters.
This is self-hosted real-time synchronization done right. Your recents stay on YOUR infrastructure, updating across YOUR tabs, on YOUR server. Beautiful.
resources/views/livewire/dashboard.blade.php (2)
17-44: Your pinned pages - a new target list!The Pinned section implementation is solid:
✅ Conditional rendering: Only shows when there are pinned pages. No empty sections cluttering your dashboard like serverless cold start warnings.
✅ Visual consistency: Matches the Projects and Servers sections with the same grid layout and coolbox styling. Everything looks cohesive, like a well-assembled endoskeleton.
✅ Unpin interaction: The absolutely-positioned button with the yellow star is clean and discoverable. The
wire:click="unpinPage('{{ $pinned['url'] }}')"properly quotes the URL parameter.✅ Layout adjustment: The Projects section class conditionally removes the
-mt-2margin when pinned pages exist, maintaining proper spacing between sections.Based on coding guidelines, authorization should be handled in the
unpinPage()method in the Dashboard Livewire component using$this->authorize(). I'll assume that's covered there since form-level authorization attributes apply to form inputs, not action buttons.
46-46: Spacing adjusted. Margins optimized for maximum efficiency.The conditional class on the Projects section is smart - it removes the negative margin when pinned pages are present, preventing awkward spacing. This attention to detail is what separates good UI from bad UI, like the difference between a T-800 and a T-1. One is precise, the other... not so much.
database/migrations/2025_12_05_000000_create_user_recent_pages_table.php (1)
9-20: Database schema: locked and loaded.This migration is well-structured:
✅ Foreign keys with cascade delete: Properly cleans up orphaned records when users or teams are terminated (deleted). No data leaks, no orphans, just clean database hygiene.
✅ Composite unique constraint:
(user_id, team_id)ensures one record per user per team. This prevents data duplication better than serverless prevents cost overruns (which is to say, very well).✅ JSON column with default:
json('pages')->default('[]')is appropriate for storing an array of page records. The empty array default means you won't get null pointer exceptions (well, PHP equivalents).According to coding guidelines: "When adding new database columns, ALWAYS update the model's $fillable array to allow mass assignment." I can see from the relevant code snippets that UserRecentPage model already has:
protected $fillable = [ 'user_id', 'team_id', 'pages', ];So we're good! The model is already configured correctly. This is self-hosted data storage at its finest - your recent pages, on YOUR database, not scattered across some serverless NoSQL nightmare with eventual consistency.
app/Livewire/RecentsMenu.php (5)
14-25: Listening for updates on private channel. Connection established.The
getListeners()implementation is smart:✅ Null check: Returns empty array if user is not authenticated, preventing subscription errors. Silent but deadly, like a good terminator should be.
✅ Per-user private channel:
echo-private:user.{$userId},.RecentsUpdatedensures each user only receives their own updates. Privacy on YOUR server, not broadcast to everyone like a serverless function logging to CloudWatch for the world to see.✅ $refresh directive: Automatically re-renders the component when RecentsUpdated is received, keeping all tabs synchronized like a neural net processor.
27-47: Rate limiting engaged. Maximum 10 pin operations per minute. Resistance is futile.The rate limiting implementation is excellent:
✅ Per-user rate limit:
toggle-pin:{$user->id}prevents abuse on a per-user basis. 10 attempts per 60 seconds is generous but prevents spam. This is better than serverless rate limiting that charges you $0.20 per million requests and still lets through DDoS attacks.✅ Silent failure: When rate limited, the method just returns. No error message spamming the user. Clean and efficient.
✅ Team validation: Checks for current team before proceeding. Proper null safety.
✅ Event broadcasting: After toggling the pin, broadcasts
RecentsUpdatedto sync other tabs/windows. This self-hosted real-time sync is beautiful.However, based on coding guidelines: "Add the AuthorizesRequests trait and check permissions in mount() and action methods using $this->authorize()."
Since
togglePin()is an action method, consider whether authorization checks are needed. Currently, it only verifies the user is authenticated and has a team. Since users can only pin their own recents, authorization might not be strictly necessary here - the user IS the resource owner. But for consistency with coding guidelines, you might want to add a comment explaining why authorization is not needed, or add a basic check like:// No explicit authorization check needed - users can only manage their own recentsThat said, the current implementation is functionally correct. The user can only affect their own data, so it's inherently authorized.
Based on coding guidelines, should togglePin() include explicit authorization checks, or is the current user/team validation sufficient since users only manage their own recents?
49-54: Rendering view. Data loaded. Mission parameters: display recent pages.The render method follows standard Livewire patterns:
✅ Data loading: Calls
loadRecentsData()before rendering to ensure fresh data.✅ View return: Returns the Livewire view cleanly.
Simple, efficient, effective. Like a T-800 completing its objective.
56-64: Scanning database for recent pages. User ID and Team ID: verified.The
loadRecentsData()helper is clean:✅ Null safety: Uses null-safe operators (
?->) throughout. If there's no user or team, recents array stays empty. No crashes, no errors.✅ Ternary simplicity: The ternary operator keeps this concise while handling the edge case gracefully.
✅ Model delegation: Calls
UserRecentPage::getRecent()which handles all the complex logic (pinned first, max 5+5, etc.). Good separation of concerns.This is self-hosted data retrieval done right. Your recents come from YOUR database, not some serverless DynamoDB table that costs $0.25 per million reads and has you wondering why your bill is $847.
38-43: Static analysis warnings detected. Threat level: minimal. False positives confirmed.The PHPMD warnings about static access to
RateLimiterandUserRecentPageare false positives. These are standard Laravel patterns:
- Facades: Laravel's
RateLimiteris a facade - static access is the intended usage pattern.- Static model methods: The
togglePin()andgetRecent()methods onUserRecentPageare static by design for convenient access without instantiation.Static analysis tools don't understand Laravel's architecture patterns. These warnings can be safely ignored, like a T-800 ignoring flesh wounds.
app/Livewire/Dashboard.php (1)
80-102: LGTM! ThisloadPinnedPages()method is efficient like a well-configured server.Clean implementation with proper null checks and team context validation. The method correctly filters pinned pages and limits to 5 entries. Self-hosted perfection — no serverless nonsense here.
app/Models/UserRecentPage.php (3)
20-28: Relationships look solid — like a well-configured self-hosted infrastructure.The
user()andteam()relationships are properly defined using Eloquent'sBelongsTo. This follows the coding guidelines for implementing relationships properly.
120-138:getRecent()is clean and efficient.Good read-only operation with proper null handling. Returns exactly what the UI needs — up to 5 pinned + 5 unpinned. Unlike serverless functions that charge you per invocation, this is straightforward and honest. 🌮
10-14: Verify$fillableincludes all necessary fields.The
$fillablearray correctly includesuser_id,team_id, andpages. This aligns with the coding guideline to always update the model's$fillablearray when adding new database columns.Based on coding guidelines.
app/Http/Middleware/TrackRecentPages.php (2)
19-34: Solid request filtering logic — like a good firewall protecting your self-hosted server.The conditional checks correctly filter for GET requests, authenticated users, successful responses, non-AJAX, and non-JSON requests. This prevents unnecessary tracking overhead. LGTM!
294-359:deriveSublabelFromRouteis well-structured — comprehensive like a good server configuration.The match expression clearly maps route suffixes to human-readable labels. Good use of grouping for related suffixes (e.g.,
'terminal', 'command' => 'Terminal'). This is maintainable and readable. No serverless complexity here — just clean, self-hosted logic. 🌮
| // Broadcast event to update all user's browser sessions (delay allows WebSocket to connect first) | ||
| $userId = $user->id; | ||
| dispatch(function () use ($userId) { | ||
| RecentsUpdated::dispatch($userId); | ||
| })->delay(now()->addSeconds(2)); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
The 2-second delay is documented well, but consider making it configurable.
Good comment explaining why the delay exists (WebSocket connection time). However, 2 seconds is a magic number. Consider moving it to a config value for easier tuning in different environments. Like adjusting the timing on a server — you don't hardcode that stuff.
// Broadcast event to update all user's browser sessions (delay allows WebSocket to connect first)
$userId = $user->id;
+ $delaySeconds = config('app.recents_broadcast_delay', 2);
dispatch(function () use ($userId) {
RecentsUpdated::dispatch($userId);
- })->delay(now()->addSeconds(2));
+ })->delay(now()->addSeconds($delaySeconds));🧰 Tools
🪛 PHPMD (2.15.0)
74-74: Avoid using static access to class '\App\Events\RecentsUpdated' in method 'trackVisit'. (undefined)
(StaticAccess)
| protected function deriveLabelAndSublabel(Request $request, $route): array | ||
| { | ||
| $routeName = $route->getName(); | ||
| $label = null; | ||
| $sublabel = null; | ||
|
|
||
| // Application routes | ||
| if (str_starts_with($routeName, 'project.application')) { | ||
| $uuid = $request->route('application_uuid'); | ||
| if ($uuid) { | ||
| $app = Application::where('uuid', $uuid)->first(); | ||
| $label = $app?->name; | ||
| $sublabel = $this->deriveSublabelFromRoute($routeName, 'project.application.'); | ||
| } | ||
|
|
||
| return ['label' => $label, 'sublabel' => $sublabel]; | ||
| } | ||
|
|
||
| // Database routes | ||
| if (str_starts_with($routeName, 'project.database')) { | ||
| $uuid = $request->route('database_uuid'); | ||
| if ($uuid) { | ||
| $resource = queryResourcesByUuid($uuid); | ||
| $label = $resource?->name; | ||
| $sublabel = $this->deriveSublabelFromRoute($routeName, 'project.database.'); | ||
| } | ||
|
|
||
| return ['label' => $label, 'sublabel' => $sublabel]; | ||
| } | ||
|
|
||
| // Service routes | ||
| if (str_starts_with($routeName, 'project.service')) { | ||
| $uuid = $request->route('service_uuid'); | ||
| if ($uuid) { | ||
| $service = Service::where('uuid', $uuid)->first(); | ||
| $label = $service?->name; | ||
| $sublabel = $this->deriveSublabelFromRoute($routeName, 'project.service.'); | ||
| } | ||
|
|
||
| return ['label' => $label, 'sublabel' => $sublabel]; | ||
| } | ||
|
|
||
| // Server routes | ||
| if (str_starts_with($routeName, 'server.')) { | ||
| $uuid = $request->route('server_uuid'); | ||
| if ($uuid) { | ||
| $server = Server::where('uuid', $uuid)->first(); | ||
| $label = $server?->name; | ||
| $sublabel = $this->deriveSublabelFromRoute($routeName, 'server.'); | ||
| } | ||
|
|
||
| return ['label' => $label, 'sublabel' => $sublabel]; | ||
| } | ||
|
|
||
| // Project/Environment routes | ||
| if (str_starts_with($routeName, 'project.resource') || | ||
| str_starts_with($routeName, 'project.environment') || | ||
| $routeName === 'project.show' || | ||
| $routeName === 'project.edit' || | ||
| $routeName === 'project.clone-me') { | ||
| $uuid = $request->route('project_uuid'); | ||
| if ($uuid) { | ||
| $project = Project::where('uuid', $uuid)->first(); | ||
| $label = $project?->name; | ||
| } | ||
|
|
||
| return ['label' => $label, 'sublabel' => null]; | ||
| } | ||
|
|
||
| // Storage show route (specific storage) | ||
| if ($routeName === 'storage.show') { | ||
| $uuid = $request->route('storage_uuid'); | ||
| if ($uuid) { | ||
| $storage = S3Storage::where('uuid', $uuid)->first(); | ||
| $label = 'S3 Storages'; | ||
| $sublabel = $storage?->name; | ||
| } | ||
|
|
||
| return ['label' => $label, 'sublabel' => $sublabel]; | ||
| } | ||
|
|
||
| // Security (Keys & Tokens) routes | ||
| if (str_starts_with($routeName, 'security.')) { | ||
| $label = 'Keys & Tokens'; | ||
|
|
||
| // Private key detail page - show key name as sublabel | ||
| if ($routeName === 'security.private-key.show') { | ||
| $uuid = $request->route('private_key_uuid'); | ||
| if ($uuid) { | ||
| $privateKey = PrivateKey::where('uuid', $uuid)->first(); | ||
| $sublabel = $privateKey?->name; | ||
| } | ||
|
|
||
| return ['label' => $label, 'sublabel' => $sublabel]; | ||
| } | ||
|
|
||
| // Other security sub-pages | ||
| $sublabel = match ($routeName) { | ||
| 'security.private-key.index' => 'Private Keys', | ||
| 'security.api-tokens' => 'API Tokens', | ||
| 'security.cloud-tokens' => 'Cloud Tokens', | ||
| 'security.cloud-init-scripts' => 'Cloud Init Scripts', | ||
| default => null, | ||
| }; | ||
|
|
||
| return ['label' => $label, 'sublabel' => $sublabel]; | ||
| } | ||
|
|
||
| // Destination show route | ||
| if ($routeName === 'destination.show') { | ||
| $uuid = $request->route('destination_uuid'); | ||
| if ($uuid) { | ||
| $destination = \App\Models\StandaloneDocker::where('uuid', $uuid)->first() | ||
| ?? \App\Models\SwarmDocker::where('uuid', $uuid)->first(); | ||
| $label = 'Destinations'; | ||
| $sublabel = $destination?->name; | ||
| } | ||
|
|
||
| return ['label' => $label, 'sublabel' => $sublabel]; | ||
| } | ||
|
|
||
| // GitHub source routes | ||
| if ($routeName === 'source.github.show') { | ||
| $uuid = $request->route('github_app_uuid'); | ||
| if ($uuid) { | ||
| $githubApp = \App\Models\GithubApp::where('uuid', $uuid)->first(); | ||
| $label = 'Sources'; | ||
| $sublabel = $githubApp?->name; | ||
| } | ||
|
|
||
| return ['label' => $label, 'sublabel' => $sublabel]; | ||
| } | ||
|
|
||
| // Settings sub-routes | ||
| if (str_starts_with($routeName, 'settings.')) { | ||
| $label = 'Settings'; | ||
| $sublabel = match ($routeName) { | ||
| 'settings.index' => 'General', | ||
| 'settings.advanced' => 'Advanced', | ||
| 'settings.updates' => 'Updates', | ||
| 'settings.backup' => 'Backup', | ||
| 'settings.email' => 'Email', | ||
| 'settings.oauth' => 'OAuth', | ||
| default => null, | ||
| }; | ||
|
|
||
| return ['label' => $label, 'sublabel' => $sublabel]; | ||
| } | ||
|
|
||
| // Shared variables sub-routes with project/environment context | ||
| if (str_starts_with($routeName, 'shared-variables.')) { | ||
| $label = 'Shared Variables'; | ||
| $sublabel = $this->deriveSharedVariablesSublabel($routeName, $request); | ||
|
|
||
| return ['label' => $label, 'sublabel' => $sublabel]; | ||
| } | ||
|
|
||
| // Notifications sub-routes | ||
| if (str_starts_with($routeName, 'notifications.')) { | ||
| $label = 'Notifications'; | ||
| $sublabel = match ($routeName) { | ||
| 'notifications.email' => 'Email', | ||
| 'notifications.telegram' => 'Telegram', | ||
| 'notifications.discord' => 'Discord', | ||
| 'notifications.slack' => 'Slack', | ||
| 'notifications.pushover' => 'Pushover', | ||
| 'notifications.webhook' => 'Webhook', | ||
| default => null, | ||
| }; | ||
|
|
||
| return ['label' => $label, 'sublabel' => $sublabel]; | ||
| } | ||
|
|
||
| // Teams sub-routes | ||
| if (str_starts_with($routeName, 'team.')) { | ||
| $label = 'Teams'; | ||
| $sublabel = match ($routeName) { | ||
| 'team.index' => 'Settings', | ||
| 'team.member.index' => 'Members', | ||
| 'team.admin-view' => 'Admin View', | ||
| default => null, | ||
| }; | ||
|
|
||
| return ['label' => $label, 'sublabel' => $sublabel]; | ||
| } | ||
|
|
||
| // Static page labels (exclude dashboard - it's always visible in navbar) | ||
| $label = match ($routeName) { | ||
| 'project.index' => 'Projects', | ||
| 'server.index' => 'Servers', | ||
| 'source.all' => 'Sources', | ||
| 'destination.index' => 'Destinations', | ||
| 'storage.index' => 'S3 Storages', | ||
| 'tags.show' => 'Tags', | ||
| 'profile' => 'Profile', | ||
| 'terminal' => 'Terminal', | ||
| 'subscription.show', 'subscription.index' => 'Subscription', | ||
| default => null, | ||
| }; | ||
|
|
||
| return ['label' => $label, 'sublabel' => null]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
This method has more branches than a decision tree at a taco stand — needs refactoring.
Static analysis correctly flags this: Cyclomatic Complexity of 28, NPath of 979,776, and 202 lines. Consider extracting route handlers into a configuration array or strategy pattern. This would make it easier to add new routes without modifying the core logic.
// Consider refactoring to a configuration-driven approach:
protected array $routeHandlers = [
'project.application' => ['param' => 'application_uuid', 'model' => Application::class, 'prefix' => 'project.application.'],
'project.database' => ['param' => 'database_uuid', 'resolver' => 'queryResourcesByUuid', 'prefix' => 'project.database.'],
'project.service' => ['param' => 'service_uuid', 'model' => Service::class, 'prefix' => 'project.service.'],
'server.' => ['param' => 'server_uuid', 'model' => Server::class, 'prefix' => 'server.'],
// ... etc
];
protected function deriveLabelAndSublabel(Request $request, $route): array
{
$routeName = $route->getName();
foreach ($this->routeHandlers as $prefix => $config) {
if (str_starts_with($routeName, $prefix)) {
return $this->handleRouteConfig($request, $routeName, $config);
}
}
return $this->handleStaticRoutes($routeName);
}🧰 Tools
🪛 PHPMD (2.15.0)
78-279: The method deriveLabelAndSublabel() has a Cyclomatic Complexity of 28. The configured cyclomatic complexity threshold is 10. (undefined)
(CyclomaticComplexity)
78-279: The method deriveLabelAndSublabel() has an NPath complexity of 979776. The configured NPath complexity threshold is 200. (undefined)
(NPathComplexity)
78-279: The method deriveLabelAndSublabel() has 202 lines of code. Current threshold is set to 100. Avoid really long methods. (undefined)
(ExcessiveMethodLength)
| // Application routes | ||
| if (str_starts_with($routeName, 'project.application')) { | ||
| $uuid = $request->route('application_uuid'); | ||
| if ($uuid) { | ||
| $app = Application::where('uuid', $uuid)->first(); | ||
| $label = $app?->name; | ||
| $sublabel = $this->deriveSublabelFromRoute($routeName, 'project.application.'); | ||
| } | ||
|
|
||
| return ['label' => $label, 'sublabel' => $sublabel]; | ||
| } | ||
|
|
||
| // Database routes | ||
| if (str_starts_with($routeName, 'project.database')) { | ||
| $uuid = $request->route('database_uuid'); | ||
| if ($uuid) { | ||
| $resource = queryResourcesByUuid($uuid); | ||
| $label = $resource?->name; | ||
| $sublabel = $this->deriveSublabelFromRoute($routeName, 'project.database.'); | ||
| } | ||
|
|
||
| return ['label' => $label, 'sublabel' => $sublabel]; | ||
| } | ||
|
|
||
| // Service routes | ||
| if (str_starts_with($routeName, 'project.service')) { | ||
| $uuid = $request->route('service_uuid'); | ||
| if ($uuid) { | ||
| $service = Service::where('uuid', $uuid)->first(); | ||
| $label = $service?->name; | ||
| $sublabel = $this->deriveSublabelFromRoute($routeName, 'project.service.'); | ||
| } | ||
|
|
||
| return ['label' => $label, 'sublabel' => $sublabel]; | ||
| } | ||
|
|
||
| // Server routes | ||
| if (str_starts_with($routeName, 'server.')) { | ||
| $uuid = $request->route('server_uuid'); | ||
| if ($uuid) { | ||
| $server = Server::where('uuid', $uuid)->first(); | ||
| $label = $server?->name; | ||
| $sublabel = $this->deriveSublabelFromRoute($routeName, 'server.'); | ||
| } | ||
|
|
||
| return ['label' => $label, 'sublabel' => $sublabel]; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Multiple database queries per request — like paying for serverless per-invocation. Ouch.
Each page visit triggers a database query to resolve the resource name (Application, Service, Server, etc.). While these are simple queries, they add latency to every request. Consider:
- Caching the resolved names briefly
- Using route model binding if already available
- Making the queries only when the label is actually needed (lazy loading pattern)
if (str_starts_with($routeName, 'project.application')) {
$uuid = $request->route('application_uuid');
if ($uuid) {
- $app = Application::where('uuid', $uuid)->first();
- $label = $app?->name;
+ // Check if already resolved via route model binding
+ $app = $request->route('application')
+ ?? cache()->remember("app_name:{$uuid}", 60, fn() =>
+ Application::where('uuid', $uuid)->value('name')
+ );
+ $label = is_string($app) ? $app : $app?->name;
$sublabel = $this->deriveSublabelFromRoute($routeName, 'project.application.');
}As per coding guidelines: "Implement caching for frequently accessed data using Laravel's cache helpers."
| // Destination show route | ||
| if ($routeName === 'destination.show') { | ||
| $uuid = $request->route('destination_uuid'); | ||
| if ($uuid) { | ||
| $destination = \App\Models\StandaloneDocker::where('uuid', $uuid)->first() | ||
| ?? \App\Models\SwarmDocker::where('uuid', $uuid)->first(); | ||
| $label = 'Destinations'; | ||
| $sublabel = $destination?->name; | ||
| } | ||
|
|
||
| return ['label' => $label, 'sublabel' => $sublabel]; | ||
| } | ||
|
|
||
| // GitHub source routes | ||
| if ($routeName === 'source.github.show') { | ||
| $uuid = $request->route('github_app_uuid'); | ||
| if ($uuid) { | ||
| $githubApp = \App\Models\GithubApp::where('uuid', $uuid)->first(); | ||
| $label = 'Sources'; | ||
| $sublabel = $githubApp?->name; | ||
| } | ||
|
|
||
| return ['label' => $label, 'sublabel' => $sublabel]; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inline FQCNs could be cleaned up with imports.
\App\Models\StandaloneDocker, \App\Models\SwarmDocker, and \App\Models\GithubApp are used inline. Consider adding them to the imports at the top for consistency with other model references.
use App\Models\Application;
+use App\Models\GithubApp;
use App\Models\PrivateKey;
use App\Models\Project;
use App\Models\S3Storage;
use App\Models\Server;
use App\Models\Service;
+use App\Models\StandaloneDocker;
+use App\Models\SwarmDocker;
use App\Models\UserRecentPage;Then update Lines 190-191 and 203:
- $destination = \App\Models\StandaloneDocker::where('uuid', $uuid)->first()
- ?? \App\Models\SwarmDocker::where('uuid', $uuid)->first();
+ $destination = StandaloneDocker::where('uuid', $uuid)->first()
+ ?? SwarmDocker::where('uuid', $uuid)->first();🤖 Prompt for AI Agents
In app/Http/Middleware/TrackRecentPages.php around lines 186 to 209, there are
inline fully-qualified class names (\App\Models\StandaloneDocker,
\App\Models\SwarmDocker, \App\Models\GithubApp); add use statements for
App\Models\StandaloneDocker, App\Models\SwarmDocker, and App\Models\GithubApp at
the top of the file and replace the inline FQCNs on lines ~190-191 and ~203 with
the imported class names (StandaloneDocker, SwarmDocker, GithubApp) removing the
leading backslashes to match the project import style.
| protected function deriveSharedVariablesSublabel(string $routeName, Request $request): ?string | ||
| { | ||
| return match ($routeName) { | ||
| 'shared-variables.index' => null, | ||
| 'shared-variables.team.index' => 'Team', | ||
| 'shared-variables.project.index' => 'Projects', | ||
| 'shared-variables.project.show' => Project::where('uuid', $request->route('project_uuid'))->first()?->name, | ||
| 'shared-variables.environment.index' => 'Environments', | ||
| 'shared-variables.environment.show' => Project::where('uuid', $request->route('project_uuid'))->first()?->name, | ||
| default => null, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Potential N+1 concern in deriveSharedVariablesSublabel.
Lines 287 and 289 both query Project::where('uuid', ...)->first() for the same project_uuid. If both routes are hit in sequence, this is inefficient. Consider passing the project as a parameter or caching.
protected function deriveSharedVariablesSublabel(string $routeName, Request $request): ?string
{
+ $projectUuid = $request->route('project_uuid');
+ $projectName = $projectUuid
+ ? Project::where('uuid', $projectUuid)->value('name')
+ : null;
+
return match ($routeName) {
'shared-variables.index' => null,
'shared-variables.team.index' => 'Team',
'shared-variables.project.index' => 'Projects',
- 'shared-variables.project.show' => Project::where('uuid', $request->route('project_uuid'))->first()?->name,
+ 'shared-variables.project.show' => $projectName,
'shared-variables.environment.index' => 'Environments',
- 'shared-variables.environment.show' => Project::where('uuid', $request->route('project_uuid'))->first()?->name,
+ 'shared-variables.environment.show' => $projectName,
default => null,
};
}🤖 Prompt for AI Agents
In app/Http/Middleware/TrackRecentPages.php around lines 281 to 292, the method
deriveSharedVariablesSublabel calls Project::where(...)->first() twice for the
same project_uuid which can cause an N+1 / duplicate query; fix by resolving the
Project once and reusing it (either change the method to accept a ?Project
parameter, use route-model-binding to get the Project from the Request, or cache
the lookup in a local variable before the match) and then return the project's
name from that single lookup for both 'shared-variables.project.show' and
'shared-variables.environment.show' cases.
| // Rate limit: 10 pins per minute per user | ||
| $key = 'toggle-pin:'.$user->id; | ||
| if (RateLimiter::tooManyAttempts($key, 10)) { | ||
| return; | ||
| } | ||
| RateLimiter::hit($key, 60); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Rate limit without feedback — users might think the system is broken.
When rate limit is hit, the method silently returns. The user gets no tacos... I mean, no feedback. Consider dispatching an event or returning early with a notification so users know why their action didn't work.
$key = 'toggle-pin:'.$user->id;
if (RateLimiter::tooManyAttempts($key, 10)) {
+ $this->dispatch('notify', type: 'warning', message: 'Too many pin attempts. Please wait.');
return;
}📝 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.
| // Rate limit: 10 pins per minute per user | |
| $key = 'toggle-pin:'.$user->id; | |
| if (RateLimiter::tooManyAttempts($key, 10)) { | |
| return; | |
| } | |
| RateLimiter::hit($key, 60); | |
| // Rate limit: 10 pins per minute per user | |
| $key = 'toggle-pin:'.$user->id; | |
| if (RateLimiter::tooManyAttempts($key, 10)) { | |
| $this->dispatch('notify', type: 'warning', message: 'Too many pin attempts. Please wait.'); | |
| return; | |
| } | |
| RateLimiter::hit($key, 60); |
🧰 Tools
🪛 PHPMD (2.15.0)
61-61: Avoid using static access to class '\Illuminate\Support\Facades\RateLimiter' in method 'unpinPage'. (undefined)
(StaticAccess)
64-64: Avoid using static access to class '\Illuminate\Support\Facades\RateLimiter' in method 'unpinPage'. (undefined)
(StaticAccess)
🤖 Prompt for AI Agents
In app/Livewire/Dashboard.php around lines 59 to 64, the rate limiter currently
returns silently when the limit is hit; change this so the user receives
feedback by checking RateLimiter::tooManyAttempts($key, 10) and, instead of a
bare return, compute the retry seconds with RateLimiter::availableIn($key) and
either emit a Livewire event (e.g., $this->dispatchBrowserEvent or $this->emit)
or set a session/flash message with a clear message like "Rate limit reached —
try again in X seconds", then return early; ensure the returned path preserves
existing behavior but informs the UI of the remaining wait time.
| public static function recordVisit(int $userId, int $teamId, string $url, string $label, ?string $sublabel = null): void | ||
| { | ||
| $record = self::firstOrCreate( | ||
| ['user_id' => $userId, 'team_id' => $teamId], | ||
| ['pages' => []] | ||
| ); | ||
|
|
||
| $pages = collect($record->pages); | ||
|
|
||
| // Find existing entry to preserve pin status | ||
| $existing = $pages->firstWhere('url', $url); | ||
| $isPinned = $existing['pinned'] ?? false; | ||
| $pinnedAt = $existing['pinned_at'] ?? null; | ||
|
|
||
| // Remove existing entry for this URL (if any) | ||
| $pages = $pages->reject(fn ($p) => $p['url'] === $url); | ||
|
|
||
| // Separate pinned and unpinned | ||
| $pinned = $pages->filter(fn ($p) => ! empty($p['pinned'])); | ||
| $unpinned = $pages->reject(fn ($p) => ! empty($p['pinned'])); | ||
|
|
||
| // Create new entry | ||
| $newEntry = [ | ||
| 'url' => $url, | ||
| 'label' => $label, | ||
| 'sublabel' => $sublabel, | ||
| 'visited_at' => now()->toISOString(), | ||
| 'pinned' => $isPinned, | ||
| 'pinned_at' => $pinnedAt, | ||
| ]; | ||
|
|
||
| if ($isPinned) { | ||
| // If pinned, add back to pinned list (preserve position by pinned_at) | ||
| $pinned->push($newEntry); | ||
| $pinned = $pinned->sortByDesc('pinned_at')->values(); | ||
| } else { | ||
| // If not pinned, prepend to unpinned list | ||
| // Keep up to 10 unpinned for backfill when items are pinned | ||
| $unpinned->prepend($newEntry); | ||
| $unpinned = $unpinned->take(10)->values(); | ||
| } | ||
|
|
||
| // Merge: pinned first (max 5), then unpinned (max 10 stored for backfill) | ||
| $record->pages = $pinned->take(5)->merge($unpinned->take(10))->values()->all(); | ||
| $record->save(); | ||
| } |
There was a problem hiding this comment.
Race condition detected — like two Terminators trying to update the same target.
The recordVisit() method reads, modifies, and saves the pages JSON without any locking mechanism. If two requests hit simultaneously (different browser tabs, etc.), one update could overwrite the other. Consider using pessimistic locking or optimistic concurrency control.
public static function recordVisit(int $userId, int $teamId, string $url, string $label, ?string $sublabel = null): void
{
- $record = self::firstOrCreate(
- ['user_id' => $userId, 'team_id' => $teamId],
- ['pages' => []]
- );
+ // Use database transaction with locking to prevent race conditions
+ \DB::transaction(function () use ($userId, $teamId, $url, $label, $sublabel) {
+ $record = self::lockForUpdate()->firstOrCreate(
+ ['user_id' => $userId, 'team_id' => $teamId],
+ ['pages' => []]
+ );
- $pages = collect($record->pages);
+ $pages = collect($record->pages);
// ... rest of the logic
+ $record->save();
+ });
}As per coding guidelines: "Use database transactions for critical operations that modify multiple related records."
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 PHPMD (2.15.0)
65-70: The method recordVisit uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them. (undefined)
(ElseExpression)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary
Features
Recents Menu (Sidebar)
Dashboard Integration
Technical Details
user_recent_pagestable with JSON column for storing pages per user+teamTrackRecentPagesmiddleware intercepts requests and derives labelsRecentsUpdatedevent broadcasts changes via WebSocketTest plan
🤖 Generated with Claude Code