Fix 653 #738
Fix 653 #738
Conversation
According to my observations: - when the timetables are shown, the request supplies the from and until values and they contain a timezone. - data coming from the database is in Europe/Budapest timezone (timezone information is not stored in the database). From this mismatch, spectacular errors can happen (like #722). If a user opens the page, their upcoming requests will contain a set timezone that is offset from UTC, therefore causing problems due to DST, so innocent users will also send "malformed requests". This change should fix this by setting the requests' timezone to Europe/Budapest.
`$splittingPointAfter->minute = 0;` did not do anything, as `$splittingPointAfter = Carbon::make($block->getFrom());` does not make a normal mutable instance from an immutable instance. There are probably better ways of doing this.
I checked the issue. The main problem is that using livewire with dates seems to be quite impractical.
Like: putting a simple text field `<input type="text" value="{{$firstDayYYYYMMDD}}">` works fine, but updating does not work (at all) if we navigate back to the page with dates on Chrome: `<input type="date" value="{{$firstDayYYYYMMDD}}">`
With `<input type="date" wire:model="firstDayYYYYMMDD">` and `<input type="text" wire:model="firstDayYYYYMMDD">` updating works even if we navigate back to the page but it shows the old date for the first time in Chrome.
I'm not an expert in Livewire and from all of my tried solutions, I could only get it to work by waiting for a bit and update the value of the input field.
I could not get it to work with materialize's datepicker on chrome, so the original HTML element is used.
I am quite unhappy with the quality of this change but at least it makes it functional.
📝 WalkthroughWalkthroughThe changes introduce a new public property to the Timetable Livewire component for holding the date in ISO format, synchronising it with a native HTML5 date input in the Blade view. The Materialize datepicker is replaced with a native date input, and associated JavaScript is updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant LivewireComponent as Timetable (Livewire)
participant Server
User->>Browser: Clicks date input or navigation button
Browser->>LivewireComponent: Updates firstDayYYYYMMDD via wire:model or calls firstDayUpdated
LivewireComponent->>LivewireComponent: Updates $firstDay, $lastDay, $firstDayYYYYMMDD
LivewireComponent->>Browser: Renders updated timetable
✨ 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/Livewire/Timetable.php(6 hunks)resources/views/livewire/timetable.blade.php(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/Livewire/Timetable.php (3)
app/Models/User.php (1)
reservations(469-472)app/Models/Reservations/ReservableItem.php (1)
reservations(34-37)app/Models/Reservations/ReservationGroup.php (1)
reservations(46-49)
🔇 Additional comments (5)
resources/views/livewire/timetable.blade.php (1)
54-59: Well-implemented native date input with proper Livewire binding.The transition from Materialize datepicker to native HTML5 date input is executed properly. The styling maintains visual consistency whilst the
wire:modelbinding ensures proper data synchronisation.The approach addresses the Chrome compatibility issues mentioned in the PR objectives effectively.
app/Livewire/Timetable.php (4)
150-150: New property properly declared for frontend binding.The addition of
$firstDayYYYYMMDDas a public string property is correctly implemented to support the native date input binding. The naming convention clearly indicates the expected format.
277-277: Excellent fix for Carbon mutability requirements.The modification properly ensures a mutable Carbon instance is created from the immutable one. This prevents potential issues where the splitting point calculations require mutable date operations.
The explicit conversion from immutable to mutable Carbon instances demonstrates proper understanding of Carbon's API.
329-329: Property synchronisation correctly implemented across lifecycle methods.The consistent updating of
$firstDayYYYYMMDDinmount,step, andfirstDayUpdatedmethods ensures proper synchronisation between the backend Carbon instance and the frontend date input value.The use of
isoFormat('YYYY-MM-DD')appropriately matches the HTML5 date input's expected format.Also applies to: 356-356, 371-371
354-358:❓ Verification inconclusive
Verify timezone consistency across all date operations.
The explicit timezone setting before date manipulations is prudent defensive programming. However, ensure this approach is consistently applied throughout the application to prevent timezone-related inconsistencies.
🏁 Script executed:
#!/bin/bash # Description: Search for other Carbon date operations that might need timezone setting # Expected: Find potential inconsistencies in timezone handling echo "Searching for Carbon date manipulations without explicit timezone setting..." rg -A 3 -B 1 "Carbon::(today|now|make|parse)" --type php echo -e "\nSearching for existing timezone setting patterns..." rg "setTimezone|timezone" --type phpLength of output: 32497
Verify consistent timezone handling in all Carbon operations
The Timetable Livewire component now explicitly applies
setTimezone(config('app.timezone'))before manipulating dates, which is prudent. However, our search revealed many other Carbon usages that default to the global timezone without an explicit override. Please review these instances to ensure they honour the same timezone context:• Models (e.g. app/Models/PeriodicEvent.php, Reservations/ReservableItem.php, Semester.php, User.php)
• Controllers and Livewire components usingCarbon::now(),Carbon::parse(),Carbon::make()
• Blade views comparing or displaying\Carbon\Carbon::now()
• Seeders and factories invokingCarbon::today()->add...or$this->faker->dateTimeBetween(..., $timezone = null)Decide whether Laravel’s default timezone (config/app.php) is sufficient or if you should explicitly pass or set the timezone in these calls (for example,
Carbon::now(config('app.timezone'))or chaining->setTimezone(...)).
|
|
||
| document.addEventListener('DOMContentLoaded', function() { | ||
| var elem = document.getElementById('firstDay'); | ||
| M.Datepicker.init(elem, { | ||
| format: 'yyyy-mm-dd', | ||
| firstDay: 1, | ||
| showClearBtn: false, | ||
| onClose: () => @this.firstDayUpdated(elem.value) | ||
| const firstDay = document.getElementById("firstDay"); | ||
| firstDay.addEventListener('click', function(event) { | ||
| event.target.showPicker(); | ||
| }); | ||
| firstDay.addEventListener('change', function(event) { | ||
| @this.firstDayUpdated(event.target.value); | ||
| }); | ||
| setTimeout(function(){ | ||
| firstDay.value = "{{$firstDayYYYYMMDD}}"; | ||
| }, 50); //FIXME, it is a terrible hack |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address the acknowledged setTimeout hack for proper implementation.
I must say, whilst the implementation achieves the desired functionality, the setTimeout hack on lines 29-31 is rather concerning. The explicit "FIXME" comment indicates this is a temporary solution that warrants proper resolution.
The 50ms delay suggests timing issues with Livewire's reactive updates. Consider implementing a more robust solution using Livewire's lifecycle hooks or dispatching custom events.
- setTimeout(function(){
- firstDay.value = "{{$firstDayYYYYMMDD}}";
- }, 50); //FIXME, it is a terrible hack
+ // Wait for Livewire to fully initialize before setting value
+ document.addEventListener('livewire:initialized', function() {
+ firstDay.value = "{{$firstDayYYYYMMDD}}";
+ });Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In resources/views/livewire/timetable.blade.php around lines 20 to 31, replace
the setTimeout hack used to set the firstDay input value with a more reliable
approach by leveraging Livewire's lifecycle hooks or event listeners. Instead of
delaying with setTimeout, use Livewire's hook such as the 'livewire:load' event
or dispatch a custom event from Livewire to set the input value once the
component is fully initialized, ensuring synchronization without arbitrary
delays.
The main problem is that using livewire with dates seems to be quite impractical.
Like: putting a simple text field
<input type="text" value="{{$firstDayYYYYMMDD}}">works fine, but updating does not work (at all) if we navigate back to the page with dates on Chrome:<input type="date" value="{{$firstDayYYYYMMDD}}">With
<input type="date" wire:model="firstDayYYYYMMDD">and<input type="text" wire:model="firstDayYYYYMMDD">updating works even if we navigate back to the page but it shows the old date for the first time in Chrome.I'm not an expert in Livewire and from all of my tried solutions, I could only get it to work by waiting for a bit and update the value of the input field.
I could not get it to work with materialize's datepicker on chrome, so the original HTML element is used.
I am quite unhappy with the quality of this change but at least it makes it functional.