Dev #33
Dev #33
Conversation
Feature/customization
WalkthroughThis update introduces customizable display texts and a privacy toggle for meeting titles in both backend and frontend. It adds new fields and methods for display settings, updates API resources, and provides UI for Pro users to edit display messages. Pro-related UI logic is streamlined, and several localization, documentation, and test adjustments are included. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WebApp
participant DisplaySettingsController
participant DisplaySettings
participant DB
User->>WebApp: Access customization page (GET /displays/{id}/customization)
WebApp->>DisplaySettingsController: customization(display)
DisplaySettingsController->>DisplaySettings: Fetch settings
DisplaySettings->>DB: Query settings
DB-->>DisplaySettings: Settings data
DisplaySettings-->>DisplaySettingsController: Settings object
DisplaySettingsController-->>WebApp: Render customization form
User->>WebApp: Submit customization form (PUT /displays/{id}/customization)
WebApp->>DisplaySettingsController: updateCustomization(request, display)
DisplaySettingsController->>DisplaySettings: Update or delete settings
DisplaySettings->>DB: Write settings
DB-->>DisplaySettings: Success/failure
DisplaySettings-->>DisplaySettingsController: Result
DisplaySettingsController-->>WebApp: Redirect with status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Codecov ReportAttention: 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: 3
🔭 Outside diff range comments (1)
app/lib/controllers/dashboard_controller.dart (1)
30-46: Avoid making onInit async - potential timing issuesMaking
onInit()async can cause issues because GetX doesn't await controller initialization. This could lead to race conditions where the controller is used before initialization completes.- void onInit() async { + void onInit() { super.onInit(); updateTime(); // Check if display ID is set, redirect to display page if not final displayIdResult = AuthService.instance.getCurrentDisplayId(); if (displayIdResult == null) { Get.offAll(() => const DisplayPage()); return; } else { displayId.value = displayIdResult; } initializeTimers(); - await fetchDisplayData(); + fetchDisplayData(); loading.value = false; }
🧹 Nitpick comments (7)
docs/UPGRADE_GUIDE.md (1)
11-13: Confirm mount-precedence does not unintentionally shadow files insidestorage/.Mounting the whole directory (
storage_data:/var/www/html/storage) and then a bind-mount for a single file inside the same target path relies on Docker’s “more specific mount wins” rule.
While this usually works, it can surprise operators if additional files are later added tostorage/on the host and silently hidden by the named volume, or vice-versa if the compose order is changed.Consider either
- Dropping the directory-level volume if all you need is the SQLite file, or
- Moving the file outside
storage/(e.g../database.sqlite:/var/www/html/database.sqlite) to avoid overlapping mounts.At minimum, add a short note explaining why two overlapping mounts are intentional and safe.
app/lib/translations/translations.dart (1)
50-51: Minor inconsistency in check-in text formatting.The English locale uses hyphens in 'check_in_now' and 'check_in', but other locales don't follow this pattern consistently. Consider standardizing the formatting across all locales for consistency.
backend/app/Http/Requests/UpdateDisplayCustomizationRequest.php (1)
17-23: Consider making the boolean field nullable for consistency.The validation rules look good overall. However, consider whether
show_meeting_titleshould benullable|booleanfor consistency with the text fields and to handle cases where the field might not be provided.The 64-character limit for text fields is reasonable for display purposes.
- 'show_meeting_title' => 'boolean', + 'show_meeting_title' => 'nullable|boolean',app/lib/models/display_settings_model.dart (1)
41-41: Questionable return type change for toJson method.The return type changed from
Map<String, dynamic>toMap<String, dynamic>?but the method implementation still returns a non-null map. This change seems unnecessary and potentially misleading.Consider reverting the return type to
Map<String, dynamic>since the method never returns null:- Map<String, dynamic>? toJson() { + Map<String, dynamic> toJson() {backend/app/Http/Controllers/DisplaySettingsController.php (1)
139-139: Consider documenting the boolean field behavior.The
show_meeting_titlefield is always set regardless of checkbox state, defaulting tofalsewhen unchecked. This is correct behavior for checkboxes but could benefit from a brief comment explaining the always-set approach.Consider adding a comment to clarify the boolean handling:
+ // Always set show_meeting_title (defaults to false if checkbox unchecked) $updated = $updated && DisplaySettings::setShowMeetingTitle($display, $request->boolean('show_meeting_title'));backend/resources/views/pages/dashboard.blade.php (1)
265-277: Consider consolidating the duplicate upgrade logic.The empty state contains duplicate conditional logic for both self-hosted and non-self-hosted scenarios that render identical disabled buttons. This could be simplified for better maintainability.
Consider consolidating the duplicate upgrade logic:
- @if(! $isSelfHosted && auth()->user()->shouldUpgrade()) - <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()->shouldUpgrade()) - <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> + @if(auth()->user()->shouldUpgrade()) + <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>backend/app/Helpers/DisplaySettings.php (1)
113-116: Consider providing default values for text gettersThe text getter methods return
nullwhen settings don't exist, which could lead to null reference issues in consuming code. Consider either:
- Providing sensible default values (e.g., 'Available', 'Transitioning', etc.)
- Clearly documenting that these methods return nullable strings so callers handle null cases appropriately
public static function getAvailableText(Display $display): ?string { - return self::getSetting($display, 'text_available'); + return self::getSetting($display, 'text_available', 'Available'); }Also applies to: 122-125, 131-134, 140-143
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
app/lib/components/action_panel.dart(1 hunks)app/lib/components/calendar_modal.dart(1 hunks)app/lib/controllers/dashboard_controller.dart(8 hunks)app/lib/models/display_settings_model.dart(2 hunks)app/lib/translations/translations.dart(5 hunks)app/pubspec.yaml(1 hunks)backend/app/Helpers/DisplaySettings.php(2 hunks)backend/app/Http/Controllers/DisplayController.php(0 hunks)backend/app/Http/Controllers/DisplaySettingsController.php(2 hunks)backend/app/Http/Requests/UpdateDisplayCustomizationRequest.php(1 hunks)backend/app/Http/Resources/API/DisplayResource.php(0 hunks)backend/app/Http/Resources/API/DisplaySettingsResource.php(1 hunks)backend/app/Models/Display.php(1 hunks)backend/resources/views/components/icons/brush.blade.php(1 hunks)backend/resources/views/pages/dashboard.blade.php(5 hunks)backend/resources/views/pages/displays/create.blade.php(1 hunks)backend/resources/views/pages/displays/customization.blade.php(1 hunks)backend/routes/web.php(1 hunks)backend/tests/Feature/DisplaySettingsApiTest.php(0 hunks)docs/PRICING.md(1 hunks)docs/UPGRADE_GUIDE.md(1 hunks)
💤 Files with no reviewable changes (3)
- backend/tests/Feature/DisplaySettingsApiTest.php
- backend/app/Http/Controllers/DisplayController.php
- backend/app/Http/Resources/API/DisplayResource.php
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/Http/Resources/API/DisplaySettingsResource.php (3)
backend/app/Models/Display.php (5)
getAvailableText(146-149)getTransitioningText(151-154)getReservedText(156-159)getCheckInText(161-164)getShowMeetingTitle(166-169)backend/app/Helpers/DisplaySettings.php (5)
getAvailableText(113-116)getTransitioningText(122-125)getReservedText(131-134)getCheckInText(140-143)getShowMeetingTitle(150-153)backend/app/Http/Resources/API/DisplayResource.php (2)
DisplayResource(8-24)toArray(15-23)
🔇 Additional comments (25)
docs/PRICING.md (1)
10-10: Documentation clarity improvement looks good.The updated trial description is more specific and accurate, clarifying that the free trial applies to the Pro plan and requires registration.
backend/resources/views/pages/displays/create.blade.php (1)
112-117: UI simplification looks good and aligns with backend changes.The removal of Pro-related conditional logic for the "Connect by Room" feature creates a cleaner, more straightforward user experience. This change is consistent with the backend controller updates that removed Pro enforcement for room functionality.
app/pubspec.yaml (1)
19-19: Appropriate version bump for new features.The minor version increment from 1.1.2 to 1.2.0 correctly reflects the new display customization features and refactorings mentioned in the PR.
app/lib/components/action_panel.dart (1)
49-49: Method call update aligns with controller refactoring.The simplified
bookRoom(min)call is consistent with the controller method signature changes that moved the localized "reserved" text handling into the controller itself. This reduces coupling and simplifies the component interface.backend/app/Http/Resources/API/DisplaySettingsResource.php (1)
23-27: LGTM! Well-structured addition of display customization fields.The new fields are properly integrated into the API resource and align with the backend implementation. The method calls delegate appropriately to the Display model's convenience methods.
app/lib/translations/translations.dart (1)
60-60: LGTM! Consistent addition of 'meeting' translations.The new 'meeting' translation key is properly added to all supported locales with appropriate translations.
Also applies to: 116-116, 172-172, 228-228, 284-284
backend/resources/views/components/icons/brush.blade.php (1)
1-18: LGTM! Well-implemented SVG icon component.The brush icon follows Laravel Blade component conventions correctly:
- Uses
{{ $attributes }}for dynamic attribute binding- Uses
currentColorfor theme-aware coloring- Proper SVG structure with appropriate viewBox
The SVG path appears to be a valid vector definition for a brush icon.
backend/routes/web.php (1)
80-84: LGTM! Well-structured routes for display customization.The new routes follow Laravel conventions and are properly placed within the authenticated middleware group. The RESTful naming pattern is consistent with existing routes.
Ensure that the
DisplaySettingsControllermethods handle proper authorization, especially since this appears to be a Pro feature.backend/app/Http/Requests/UpdateDisplayCustomizationRequest.php (1)
9-13: LGTM! Proper authorization delegation to controller.The authorization logic correctly delegates to the controller, which is appropriate for complex authorization scenarios like Pro feature access.
backend/app/Models/Display.php (1)
146-169: Well-structured convenience methods following established patterns.The new display customization accessor methods are implemented consistently with the existing pattern in this class. They provide clean encapsulation by delegating to the
DisplaySettingshelper class while maintaining the model's interface.backend/resources/views/pages/displays/customization.blade.php (4)
6-17: Proper Pro feature gating with clear user guidance.The conditional rendering effectively restricts access to Pro users while providing clear messaging and navigation for non-Pro users. The upgrade prompt is informative and actionable.
35-37: Secure form implementation with proper CSRF protection.The form correctly implements CSRF protection and method spoofing for the PUT request, following Laravel security best practices.
43-43: Robust input handling with proper fallbacks.The input field correctly uses
old()for form persistence on validation errors and falls back to the DisplaySettings helper method for initial values. Themaxlength="64"attribute provides client-side validation.
61-61: Proper boolean checkbox handling.The checkbox implementation correctly handles the boolean value using Laravel's ternary operator for the
checkedattribute, with proper fallback to the DisplaySettings helper method.app/lib/models/display_settings_model.dart (2)
7-11: Well-designed field additions with appropriate nullability.The new customization fields are properly typed with nullable strings for optional text values and a required boolean for the meeting title visibility. This design allows flexibility while maintaining clear semantics.
37-37: Sensible default for privacy setting.The default value of
trueforshowMeetingTitleis appropriate as it maintains existing behavior when the setting is not explicitly configured, ensuring backwards compatibility.backend/app/Http/Controllers/DisplaySettingsController.php (3)
88-99: Proper authorization and Pro access control.The method correctly implements authorization checks and Pro user validation with appropriate error messaging and redirection. This follows the established security pattern in the controller.
111-136: Robust text field update logic with proper cleanup.The implementation correctly handles both setting and deleting text values based on input presence using Laravel's
filled()helper. The logic appropriately deletes settings when fields are empty, preventing storage of empty values.
141-145: Appropriate error handling and user feedback.The error handling correctly uses
back()for form errors and provides proper success redirection with flash messages. The redirect to the customization route maintains user context.backend/resources/views/pages/dashboard.blade.php (3)
32-60: Improved Pro upgrade banner with better visual hierarchy.The new indigo-themed banner is more visually prominent and provides comprehensive information about Pro features. The structured layout with icon, content, and CTA button creates better user engagement than the previous implementation.
77-82: Consistent Pro feature gating with clear user feedback.The conditional rendering effectively prevents non-Pro users from accessing premium features while providing clear tooltips explaining the upgrade requirement. The disabled styling with appropriate cursor indicates the unavailable state.
232-245: Unified approach to Pro feature access controls.The display action buttons (customization and settings) consistently implement the Pro gating pattern with disabled spans for non-Pro users. The tooltips provide clear messaging about feature availability.
backend/app/Helpers/DisplaySettings.php (1)
149-157: LGTM!The show meeting title toggle methods are well-implemented with appropriate default value for backward compatibility.
app/lib/controllers/dashboard_controller.dart (2)
66-66: Good null-safe implementationThe updated getters properly handle potential null values with appropriate fallbacks, maintaining backward compatibility.
Also applies to: 74-79, 256-256, 260-260, 277-277, 281-281, 285-285
312-322: Well-structured helper methodsThe new helper methods properly implement the meeting title privacy feature with good separation of concerns and null safety.
| import 'package:spacepad/controllers/dashboard_controller.dart'; | ||
| import 'package:spacepad/services/auth_service.dart'; |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Verify that these imports are actually used.
The new imports for DashboardController and AuthService don't appear to be referenced anywhere in the current code. Ensure these imports are necessary and being used, or remove them if they're not needed to avoid dead imports.
🏁 Script executed:
#!/bin/bash
# Description: Check if the imported classes are actually used in this file
# Expected: Should find usage of DashboardController or AuthService if imports are needed
# Search for usage of DashboardController in the calendar modal file
rg -A 3 -B 3 "DashboardController|AuthService" app/lib/components/calendar_modal.dartLength of output: 87
Remove unused imports in calendar_modal.dart
It looks like the imports for DashboardController and AuthService aren’t referenced anywhere in app/lib/components/calendar_modal.dart. To keep the code clean, please either:
- Remove these two import lines if they’re not needed
- Or add the intended usage for
DashboardControllerandAuthServiceinto this file
Locations to update:
- app/lib/components/calendar_modal.dart, lines 6–7
🤖 Prompt for AI Agents
In app/lib/components/calendar_modal.dart at lines 6 to 7, the imports for
DashboardController and AuthService are not used anywhere in the file. To fix
this, remove these two import lines to clean up the code and avoid unnecessary
dependencies.
| // Global variables for device, display, and settings | ||
| DeviceModel? globalCurrentDevice; | ||
| DisplayModel? globalDisplay; | ||
| DisplaySettingsModel? globalSettings; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider the global variable design pattern
The introduction of instance variables prefixed with "global" raises several concerns:
- The "global" prefix is misleading since these are instance variables, not truly global
- This creates a potential synchronization issue with the reactive
AuthService.currentDevice - Risk of stale data if these aren't updated in sync with the reactive state
Consider using the existing reactive pattern consistently or rename these variables to better reflect their scope.
- // Global variables for device, display, and settings
- DeviceModel? globalCurrentDevice;
- DisplayModel? globalDisplay;
- DisplaySettingsModel? globalSettings;
+ // Cached references for quick access
+ DeviceModel? _cachedDevice;
+ DisplayModel? _cachedDisplay;
+ DisplaySettingsModel? _cachedSettings;📝 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.
| // Global variables for device, display, and settings | |
| DeviceModel? globalCurrentDevice; | |
| DisplayModel? globalDisplay; | |
| DisplaySettingsModel? globalSettings; | |
| // Cached references for quick access | |
| DeviceModel? _cachedDevice; | |
| DisplayModel? _cachedDisplay; | |
| DisplaySettingsModel? _cachedSettings; |
🤖 Prompt for AI Agents
In app/lib/controllers/dashboard_controller.dart around lines 21 to 24, the
variables named with a "global" prefix are instance variables, not truly global,
which is misleading and risks stale data due to lack of synchronization with
AuthService.currentDevice. To fix this, either remove these variables and rely
directly on the reactive AuthService.currentDevice for consistent state
management, or rename these variables to reflect their actual scope and ensure
they are updated reactively in sync with AuthService.currentDevice to avoid
stale data issues.
| // Update global device, display, and settings | ||
| if (AuthService.instance.currentDevice.value != null) { | ||
| AuthService.instance.currentDevice.value!.display = displayData.display; | ||
| globalCurrentDevice = AuthService.instance.currentDevice.value; | ||
| globalCurrentDevice!.display = displayData.display; | ||
| globalDisplay = globalCurrentDevice!.display; | ||
| globalSettings = globalDisplay?.settings; | ||
|
|
||
| AuthService.instance.currentDevice.refresh(); | ||
| } | ||
|
|
||
| // Update events | ||
| events.value = displayData.events | ||
| .where((e) => e.status != EventStatus.cancelled) | ||
| .map((e) { | ||
| e.summary = getDisplayableSummary(e); | ||
| return e; | ||
| }) | ||
| .toList(); | ||
| } catch (e) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid direct mutation of AuthService state
This code directly mutates the AuthService.currentDevice display property, which could cause unexpected side effects and breaks encapsulation.
Consider updating through a proper service method rather than direct mutation:
// Update global device, display, and settings
if (AuthService.instance.currentDevice.value != null) {
globalCurrentDevice = AuthService.instance.currentDevice.value;
- globalCurrentDevice!.display = displayData.display;
globalDisplay = displayData.display;
globalSettings = globalDisplay?.settings;
-
- AuthService.instance.currentDevice.refresh();
+ // Update through service if needed
+ AuthService.instance.updateDeviceDisplay(displayData.display);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/lib/controllers/dashboard_controller.dart around lines 201 to 219, the
code directly mutates the display property of
AuthService.instance.currentDevice.value, which breaks encapsulation and may
cause side effects. To fix this, avoid direct mutation by creating a method in
AuthService to update the currentDevice's display and related properties safely.
Replace the direct assignments with calls to this method, ensuring the state is
updated through controlled service logic.
2 participants
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Chores