Add UI to set content access while creating movie in flimix - #890
Add UI to set content access while creating movie in flimix#890Karthikeyantestpress wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the movie creation page by modularizing it into sub-templates and introducing Alpine.js interactive logic for file uploads, genre selection, and content access configuration. The feedback highlights several critical improvements: resolving a memory leak and state race condition from an uncleared interval in the upload simulator, adding missing name attributes or bindings to the rental duration and media file inputs to prevent data loss, removing the unused pricing modal template, correcting a typo in the genre options, and ensuring robust state initialization and unique key generation for dynamic pricing rows.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| <div class="w-full max-w-3xl mx-auto pb-10" x-data="{ | ||
| status: 'idle', | ||
| fileName: '', | ||
| fileSize: '', | ||
| progress: 0, | ||
| movieTitle: '', | ||
| movieDescription: '', | ||
| movieReleaseYear: '', | ||
| coverImage: '', | ||
| handleFileSelect(e) { | ||
| const file = e.target.files[0]; | ||
| if (!file) return; | ||
| this.fileName = file.name; | ||
| this.fileSize = (file.size / (1024 * 1024)).toFixed(1) + ' MB'; | ||
| this.status = 'uploading'; | ||
| this.progress = 0; | ||
|
|
||
| // Auto extract movie title from file name (without extension) | ||
| const nameWithoutExt = file.name.substring(0, file.name.lastIndexOf('.')) || file.name; | ||
| // Clean it up (replace dashes/underscores with spaces, capitalize) | ||
| const cleanedName = nameWithoutExt.replace(/[_\-\.]+/g, ' ').replace(/\b\w/g, c => c.toUpperCase()); | ||
| if (!this.movieTitle) { | ||
| this.movieTitle = cleanedName; | ||
| } | ||
|
|
||
| let interval = setInterval(() => { | ||
| this.progress += 10; | ||
| if (this.progress >= 100) { | ||
| clearInterval(interval); | ||
| this.status = 'ready'; | ||
| } | ||
| }, 150); | ||
| }, | ||
| reset() { | ||
| this.status = 'idle'; | ||
| this.fileName = ''; | ||
| this.fileSize = ''; | ||
| this.progress = 0; | ||
| } | ||
| }"> |
There was a problem hiding this comment.
The setInterval used to simulate the upload progress is stored in a local variable and is never cleared if the user cancels the upload (by calling reset()) or selects a new file. This causes a memory leak and a state race condition: even after clicking cancel, the interval will continue running in the background and eventually set this.status = 'ready' and this.progress = 100, messing up the UI state.
To fix this, store the interval ID in the Alpine component's state (e.g., uploadInterval) and clear it both in reset() and before starting a new upload in handleFileSelect().
<div class="w-full max-w-3xl mx-auto pb-10" x-data="{
status: 'idle',
fileName: '',
fileSize: '',
progress: 0,
movieTitle: '',
movieDescription: '',
movieReleaseYear: '',
coverImage: '',
uploadInterval: null,
handleFileSelect(e) {
const file = e.target.files[0];
if (!file) return;
this.fileName = file.name;
this.fileSize = (file.size / (1024 * 1024)).toFixed(1) + ' MB';
this.status = 'uploading';
this.progress = 0;
// Auto extract movie title from file name (without extension)
const nameWithoutExt = file.name.substring(0, file.name.lastIndexOf('.')) || file.name;
// Clean it up (replace dashes/underscores with spaces, capitalize)
const cleanedName = nameWithoutExt.replace(/[_-\.]+/g, ' ').replace(/\b\w/g, c => c.toUpperCase());
if (!this.movieTitle) {
this.movieTitle = cleanedName;
}
if (this.uploadInterval) {
clearInterval(this.uploadInterval);
}
this.uploadInterval = setInterval(() => {
this.progress += 10;
if (this.progress >= 100) {
clearInterval(this.uploadInterval);
this.uploadInterval = null;
this.status = 'ready';
}
}, 150);
},
reset() {
if (this.uploadInterval) {
clearInterval(this.uploadInterval);
this.uploadInterval = null;
}
this.status = 'idle';
this.fileName = '';
this.fileSize = '';
this.progress = 0;
}
}">
| Rental duration | ||
| </label> | ||
| <div class="relative w-full" x-ignore> | ||
| <select data-hs-select='{ |
There was a problem hiding this comment.
The <select> element for Rental duration does not have a name attribute or an x-model binding. Consequently, the selected rental duration will not be submitted with the form or captured by Alpine.js, leading to data loss. Please add a name attribute or bind it to Alpine's state.
<select name="rental_duration" data-hs-select='{
| </span> | ||
| <label for="movie-thumbnail-upload" class="relative cursor-pointer bg-white font-semibold text-blue-600 hover:text-blue-700 rounded-lg decoration-2 hover:underline focus-within:outline-none focus-within:ring-2 focus-within:ring-blue-600 focus-within:ring-offset-2 dark:bg-neutral-800 dark:text-blue-500 dark:hover:text-blue-600"> | ||
| <span>browse</span> | ||
| <input id="movie-thumbnail-upload" type="file" class="sr-only"> |
There was a problem hiding this comment.
The file inputs for Thumbnail (line 34), Cover Art (line 66), and Poster (line 98) lack any name attributes, x-model bindings, or @change event handlers. As a result, any files selected by the user in these fields will not be captured, bound to the Alpine.js state, or submitted with the form.
Consider adding @change handlers or name attributes to these inputs so their values can be processed or uploaded.
| <!-- Add Event Modal --> | ||
| <div id="add_new_pricing" | ||
| class="hs-overlay hidden size-full fixed top-0 start-0 z-[80] overflow-x-hidden overflow-y-auto [--close-when-click-inside:true] pointer-events-none" | ||
| role="dialog" tabindex="-1" aria-labelledby="add_new_pricing-label"> |
There was a problem hiding this comment.
The file movie_pricing_modal.html is added in this pull request but is never included or referenced in create.html or any other template. If this modal is no longer needed (since pricing is now handled inline in movie_access.html), please delete this file to keep the codebase clean and maintainable.
|
|
||
| <option value="4">Drama</option> | ||
|
|
||
| <option value="5">Science Friction</option> |
| <div x-data="{ | ||
| accessType: 'Subscription', | ||
| pricings: [ | ||
| { id: 1, region: 'IN', currency: 'INR', price: '' } | ||
| ] | ||
| }" |
There was a problem hiding this comment.
The initial state of the first pricing item has region: 'IN'. Since the first row (index === 0) is treated as the Default pricing (where the country dropdown is hidden and replaced with the text 'Default pricing'), having region: 'IN' might cause the backend to incorrectly save or interpret the default pricing as specifically applying to India (IN). Consider setting region to an empty string '' or a specific default identifier.
<div x-data="{
accessType: 'Subscription',
pricings: [
{ id: 1, region: '', currency: 'USD', price: '' }
]
}"| <!-- Add Pricing Button --> | ||
| <div> | ||
| <button type="button" | ||
| @click="pricings.push({ id: Date.now(), region: '', currency: '', price: '' }); $nextTick(() => { if (window.HSSelect) HSSelect.autoInit(); })" |
There was a problem hiding this comment.
Using Date.now() as a unique key for dynamically added elements can lead to duplicate keys if the user clicks the button rapidly (within the same millisecond). Duplicate keys in x-for can cause rendering bugs in Alpine.js. Consider appending Math.random() to guarantee uniqueness.
@click="pricings.push({ id: Date.now() + Math.random(), region: '', currency: '', price: '' }); $nextTick(() => { if (window.HSSelect) HSSelect.autoInit(); })"
|
No description provided.