Open
Conversation
Show profile image rounded during cropping and inside profile page
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaced the Vue wrapper with upstream CropperJS (dependency + scoped CSS), refactored profile image selector to instantiate/destroy Cropper programmatically, removed Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
resources/js/components/UserProfileImageSelector.vue (1)
173-177:⚠️ Potential issue | 🟡 MinorVerify
fillColorvalue.The
fillColor: "#ffff"appears to be a typo. Valid CSS color values are"#fff"(3-digit) or"#ffffff"(6-digit). While some parsers may accept 4-digit hex as RGBA shorthand, this could cause unexpected transparency.🔧 Proposed fix
const oc = cropper.value.getCroppedCanvas({ width: 100, height: 100, - fillColor: "#ffff", + fillColor: "#fff", });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/components/UserProfileImageSelector.vue` around lines 173 - 177, The fillColor passed to cropper.value.getCroppedCanvas uses an invalid/ambiguous hex "#ffff"; update the call in the code that constructs oc (cropper.value.getCroppedCanvas) to use a valid CSS hex like "#fff" or "#ffffff" (choose one consistent with your color scheme) so the generated canvas background is deterministic and not accidentally transparent.
🧹 Nitpick comments (1)
resources/js/components/UserProfileImageSelector.vue (1)
112-119: Use template ref instead ofdocument.getElementById.Using
document.getElementByIdis not idiomatic in Vue and can cause issues if multiple instances of this component exist simultaneously. Consider using a template ref instead.♻️ Proposed fix using template ref
Update the template:
- <img - :src="selectedFile" - width="100%" - id="cropper" - :alt="$t('admin.users.image.title')" - /> + <img + ref="cropperImageRef" + :src="selectedFile" + width="100%" + :alt="$t('admin.users.image.title')" + />Update the script:
const cropper = ref(); +const cropperImageRef = ref(null);- cropper.value = new Cropper(document.getElementById("cropper"), { + cropper.value = new Cropper(cropperImageRef.value, {Additionally, the static analysis tool indicates that
idshould go beforewidthif you keep using the id attribute.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/components/UserProfileImageSelector.vue` around lines 112 - 119, The template currently relies on an element id ("cropper") and likely uses document.getElementById to access the image, which is not idiomatic and breaks with multiple component instances; change the <img> to use a Vue template ref (e.g., ref="cropperImg") and update any code that calls document.getElementById("cropper") to use this.$refs.cropperImg (or the Composition API ref) instead; also remove or keep the id attribute (preferably remove it) or if kept follow static analysis ordering by placing id before width in the <img> attributes. Ensure references to "selectedFile" remain unchanged and update any methods that manipulate the cropper DOM to use the ref.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@resources/js/components/UserProfileImageSelector.vue`:
- Around line 173-177: The fillColor passed to cropper.value.getCroppedCanvas
uses an invalid/ambiguous hex "#ffff"; update the call in the code that
constructs oc (cropper.value.getCroppedCanvas) to use a valid CSS hex like
"#fff" or "#ffffff" (choose one consistent with your color scheme) so the
generated canvas background is deterministic and not accidentally transparent.
---
Nitpick comments:
In `@resources/js/components/UserProfileImageSelector.vue`:
- Around line 112-119: The template currently relies on an element id
("cropper") and likely uses document.getElementById to access the image, which
is not idiomatic and breaks with multiple component instances; change the <img>
to use a Vue template ref (e.g., ref="cropperImg") and update any code that
calls document.getElementById("cropper") to use this.$refs.cropperImg (or the
Composition API ref) instead; also remove or keep the id attribute (preferably
remove it) or if kept follow static analysis ordering by placing id before width
in the <img> attributes. Ensure references to "selectedFile" remain unchanged
and update any methods that manipulate the cropper DOM to use the ref.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7ed00fd2-f410-40b5-bbc4-122b8cefb1ce
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
package.jsonresources/css/app/_cropperjs.cssresources/css/app/app.cssresources/js/components/UserAvatar.vueresources/js/components/UserProfileImageSelector.vueresources/js/views/AdminUsersNew.vue
💤 Files with no reviewable changes (1)
- resources/js/views/AdminUsersNew.vue
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3060 +/- ##
=============================================
- Coverage 96.73% 96.34% -0.40%
Complexity 1924 1924
=============================================
Files 457 278 -179
Lines 12988 6674 -6314
Branches 2079 0 -2079
=============================================
- Hits 12564 6430 -6134
+ Misses 424 244 -180 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
PILOS
|
||||||||||||||||||||||||||||||||||||||||||||||
| Project |
PILOS
|
| Branch Review |
refactor-image-cropper
|
| Run status |
|
| Run duration | 11m 06s |
| Commit |
|
| Committer | Samuel Weirich |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
3
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
604
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
AdminUsersEditBase.cy.js • 1 failed test • Frontend tests
| Test | Artifacts | |
|---|---|---|
| Admin users edit base > check view and save changes |
Test Replay
Screenshots
|
|
UserProfileBase.cy.js • 1 failed test • Frontend tests
| Test | Artifacts | |
|---|---|---|
| User Profile Base > check view and save changes |
Test Replay
Screenshots
|
|
AdminUsersViewUserActions.cy.js • 1 failed test • Frontend tests
| Test | Artifacts | |
|---|---|---|
| Admin users view user actions > switch between edit and view |
Test Replay
Screenshots
|
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
resources/js/components/UserProfileImageSelector.vue (1)
173-177:⚠️ Potential issue | 🟡 MinorLikely typo in
fillColorvalue.
"#ffff"is 4 hex digits. This is likely meant to be"#fff"(3 digits) or"#ffffff"(6 digits) for white. While 4-digit hex is valid CSS (RGBA shorthand), it's unusual here and may not be intentional.🔧 Proposed fix
const oc = cropper.value.getCroppedCanvas({ width: 100, height: 100, - fillColor: "#ffff", + fillColor: "#fff", });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/components/UserProfileImageSelector.vue` around lines 173 - 177, In the getCroppedCanvas call where oc is created, fix the fillColor value which is currently "#ffff" (likely a typo); update the fillColor option passed to cropper.value.getCroppedCanvas to a proper white hex such as "#fff" or "#ffffff" (refer to the oc variable and the getCroppedCanvas invocation) so the canvas is filled with the intended white color.
🧹 Nitpick comments (2)
resources/js/components/UserProfileImageSelector.vue (2)
205-237: Consider adding error handling for robustness.If file reading fails or the Cropper fails to initialize,
isLoadingActionwill remaintrue, leaving the UI in a stuck state. Consider adding error handling.♻️ Suggested error handling pattern
async function onFileSelect(event) { modalVisible.value = true; isLoadingAction.value = true; const file = event.files[0]; const reader = new FileReader(); + reader.onerror = () => { + isLoadingAction.value = false; + closeModal(); + // Optionally show error toast/notification + }; reader.onload = async (event) => { selectedFile.value = event.target.result; await nextTick(); - cropper.value = new Cropper(document.getElementById("cropper"), { + try { + cropper.value = new Cropper(cropperImg.value, { // ... options - }); + }); + } catch (e) { + isLoadingAction.value = false; + closeModal(); + } }; reader.readAsDataURL(file); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/components/UserProfileImageSelector.vue` around lines 205 - 237, The onFileSelect handler can leave isLoadingAction true if file reading or Cropper initialization fails; add try/catch around the async work (FileReader read and Cropper creation) so any thrown error sets isLoadingAction.value = false and modalVisible.value = false (or closes the modal) and logs or surfaces the error; specifically wrap the FileReader onload handler and the new Cropper(...) creation/ready logic to catch initialization errors, ensure selectedFile.value is only set on success, and in the catch block clean up cropper.value if partially created and call document.querySelector('[data-test="dialog-cancel-button"]').focus() (or appropriate UI reset) to avoid a stuck loading state.
227-233: Consider using a template ref for focus management.
document.querySelector('[data-test="dialog-cancel-button"]')queries the entire document, which could select the wrong element if multiple instances exist. Consider adding a template ref to the cancel button for scoped access.♻️ Proposed approach
Add a ref to the cancel button:
<Button + ref="cancelButton" :label="$t('app.cancel')" severity="secondary"In script:
+const cancelButton = ref(null);Then focus using the ref:
- document.querySelector('[data-test="dialog-cancel-button"]').focus(); + cancelButton.value?.$el?.focus();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/components/UserProfileImageSelector.vue` around lines 227 - 233, The ready method currently uses document.querySelector('[data-test="dialog-cancel-button"]') which can collide across instances; add a template ref (e.g., cancelButtonRef) to the cancel button in the component template and replace the global query in the ready() function with a scoped ref access: after await nextTick(), check cancelButtonRef.value and call .focus() on it (with a null guard). Update any related imports/`ref` declarations in the script setup so cancelButtonRef is reactive and accessible in ready().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@resources/js/components/UserProfileImageSelector.vue`:
- Around line 112-119: Replace the hardcoded id usage for the image element and
any document.getElementById("cropper") calls with a Vue template ref (e.g.,
ref="cropperRef") so DOM access is scoped per-component; update the template
image tag that uses selectedFile to remove the id and add the ref, and update
the Cropper initialization code (wherever Cropper is constructed) to use the
template ref (this.$refs.cropperRef or the setup/ref equivalent) instead of
getElementById; apply the same change to the other occurrence noted (line ~216),
and ensure attribute ordering no longer relies on an id being present.
---
Outside diff comments:
In `@resources/js/components/UserProfileImageSelector.vue`:
- Around line 173-177: In the getCroppedCanvas call where oc is created, fix the
fillColor value which is currently "#ffff" (likely a typo); update the fillColor
option passed to cropper.value.getCroppedCanvas to a proper white hex such as
"#fff" or "#ffffff" (refer to the oc variable and the getCroppedCanvas
invocation) so the canvas is filled with the intended white color.
---
Nitpick comments:
In `@resources/js/components/UserProfileImageSelector.vue`:
- Around line 205-237: The onFileSelect handler can leave isLoadingAction true
if file reading or Cropper initialization fails; add try/catch around the async
work (FileReader read and Cropper creation) so any thrown error sets
isLoadingAction.value = false and modalVisible.value = false (or closes the
modal) and logs or surfaces the error; specifically wrap the FileReader onload
handler and the new Cropper(...) creation/ready logic to catch initialization
errors, ensure selectedFile.value is only set on success, and in the catch block
clean up cropper.value if partially created and call
document.querySelector('[data-test="dialog-cancel-button"]').focus() (or
appropriate UI reset) to avoid a stuck loading state.
- Around line 227-233: The ready method currently uses
document.querySelector('[data-test="dialog-cancel-button"]') which can collide
across instances; add a template ref (e.g., cancelButtonRef) to the cancel
button in the component template and replace the global query in the ready()
function with a scoped ref access: after await nextTick(), check
cancelButtonRef.value and call .focus() on it (with a null guard). Update any
related imports/`ref` declarations in the script setup so cancelButtonRef is
reactive and accessible in ready().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ae44d204-0039-410e-812d-e0a1512e6be0
📒 Files selected for processing (2)
resources/css/app/_cropperjs.cssresources/js/components/UserProfileImageSelector.vue
✅ Files skipped from review due to trivial changes (1)
- resources/css/app/_cropperjs.css
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/Frontend/e2e/RoomsViewFilesFileActions.cy.js (1)
20-27:⚠️ Potential issue | 🟠 MajorFix the expected max-size interpolation string.
Line 26 won’t match the usual i18n fallback format because the
sizevalue should be quoted.🧪 Proposed test fix
- .should("include.text", 'app.file.max_size_{"size":30 MB') + .should("include.text", 'app.file.max_size_{"size":"30 MB"}')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Frontend/e2e/RoomsViewFilesFileActions.cy.js` around lines 20 - 27, The assertion expecting the i18n fallback for max size is missing quotes around the interpolated size; update the check on the cy.get('[data-test="room-files-upload-dialog"]') chain so the expected string is 'app.file.max_size_{"size":"30 MB"}' (i.e., add surrounding quotes for the size value and include the closing brace) so the .should("include.text", ...) matches the usual i18n format.resources/js/views/AdminStreamingSettings.vue (1)
52-64:⚠️ Potential issue | 🟡 MinorAlign CSS file upload limit with backend validation.
The backend enforces a 500 KB maximum for CSS files (
app/Http/Requests/UpdateStreamingSettingsRequest.phpline:max('500kb')), but the frontend allows 5 MB. This mismatch will cause file upload failures for users who select valid files under the frontend limit but over the backend limit. The default pause image (correctly set to 5 MB on both frontend and backend) shows this was likely unintentional.Proposed fix
- :max-file-size="5_000_000" + :max-file-size="500_000"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/views/AdminStreamingSettings.vue` around lines 52 - 64, The CSS file upload limit in the SettingsFileSelector for streaming CSS is out of sync with backend validation: change the max-file-size passed to the SettingsFileSelector (the prop currently set via v-model/file-url="settings.css_file" and related bindings) from 5_000_000 to 500_000 so the frontend enforces the same 500 KB limit as UpdateStreamingSettingsRequest; leave the pause image/upload limit unchanged if it already uses 5_000_000.
🧹 Nitpick comments (3)
resources/js/components/UserProfileImageSelector.vue (1)
214-243: HandleFileReadererror / cropper init failure.If
readerfails (ornew Cropper(...)throws synchronously beforereadyfires),isLoadingActionstaystrueand the modal remains open with a spinner and no way to recover other than the cancel button. Add anonerrorhandler that closes the modal, and consider wrapping the Cropper construction in try/catch to reset loading state.♻️ Proposed fix
const reader = new FileReader(); reader.onload = async (event) => { selectedFile.value = event.target.result; await nextTick(); - cropper.value = new Cropper(cropperImgRef.value, { + try { + cropper.value = new Cropper(cropperImgRef.value, { aspectRatio: 1, autoCropArea: 0.9, background: false, guides: false, center: false, rotatable: false, zoomable: false, movable: false, viewMode: 1, dragMode: "none", ready: async function () { isLoadingAction.value = false; }, - }); + }); + } catch (e) { + isLoadingAction.value = false; + closeModal(); + throw e; + } }; + reader.onerror = () => { + isLoadingAction.value = false; + closeModal(); + }; reader.readAsDataURL(file);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/components/UserProfileImageSelector.vue` around lines 214 - 243, The onFileSelect handler currently sets modalVisible and isLoadingAction but lacks error handling; add a reader.onerror callback that sets isLoadingAction.value = false and modalVisible.value = false (or shows an error state) and logs the error, and wrap the new Cropper(...) instantiation inside a try/catch in onFileSelect (or inside reader.onload) so that if Cropper throws synchronously you also reset isLoadingAction.value and modalVisible.value and clean up any partially created cropper.value; reference reader, reader.onerror, onFileSelect, cropper.value, Cropper, isLoadingAction.value and modalVisible.value when implementing.config/bigbluebutton.php (1)
13-13: Handle empty/whitespace env values when parsingBBB_ALLOWED_FILE_MIMES.
explode(',', '')yields['']and tokens aren’t trimmed, so an empty or whitespace-padded override silently produces invalid entries that downstreamFile::types(...)/extensions(...)will then include in the allowed list.♻️ Proposed fix
- 'allowed_file_mimes' => explode(',', env('BBB_ALLOWED_FILE_MIMES', 'pdf,doc,docx,xls,xlsx,ppt,pptx,txt,rtf,odt,ods,odp,odg,odc,odi,jpg,jpeg,png')), + 'allowed_file_mimes' => array_values(array_filter(array_map('trim', explode(',', env('BBB_ALLOWED_FILE_MIMES', 'pdf,doc,docx,xls,xlsx,ppt,pptx,txt,rtf,odt,ods,odp,odg,odc,odi,jpg,jpeg,png'))), 'strlen')),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/bigbluebutton.php` at line 13, The config currently uses explode(',', env('BBB_ALLOWED_FILE_MIMES', '...')) which yields [''] for empty env and doesn't trim tokens; update the allowed_file_mimes parsing to split on commas, trim each token, and filter out empty/whitespace-only entries (e.g., via array_map('trim') + array_filter or preg_split with PREG_SPLIT_NO_EMPTY and trim) so that allowed_file_mimes contains only valid non-empty extensions before passing to File::types(...) / extensions(...).resources/js/views/AdminSettings.vue (1)
1471-1477: Add a computed normalizer forbbb.file_mimesbefore passing toSettingsFileSelector.While the backend always sends this setting as an array via
explode()inconfig/bigbluebutton.php, the Visual test fixture contains a comma-separated string. SinceSettingsFileSelectorrequires an array (uses.join()on the prop), add defensive normalization here—the only place in the codebase where this setting is dynamically bound to the component:♻️ Proposed normalization
+ :allowed-extensions="bbbFileMimes" - :allowed-extensions=" - settingsStore.getSetting('bbb.file_mimes') - "Add near the other computed values:
const bbbFileMimes = computed(() => { const value = settingsStore.getSetting("bbb.file_mimes"); if (Array.isArray(value)) { return value; } return String(value ?? "") .split(",") .map((extension) => extension.trim()) .filter(Boolean); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/views/AdminSettings.vue` around lines 1471 - 1477, Add a defensive computed normalizer named bbbFileMimes that reads settingsStore.getSetting('bbb.file_mimes') and returns an array (if already an array return it; otherwise split the string on commas, trim each entry and filter out empties), then replace the direct binding to settingsStore.getSetting('bbb.file_mimes') passed into SettingsFileSelector with the computed bbbFileMimes so the prop always receives an array compatible with SettingsFileSelector (which uses .join()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lang/en/app.php`:
- Around line 144-149: The localization moved file-related messages under
app.file which breaks existing callers expecting app.validation.too_large and
app.validation.invalid_type; either restore alias keys under app.validation
(e.g., add validation.too_large and validation.invalid_type that reference the
same strings as file.too_large and file.invalid_type) or update the callers
(SettingsFileSelector.vue, SettingsImageSelector.vue and any E2E tests
referencing app.validation.too_large) to use app.file.too_large and
app.file.invalid_type so no missing-key errors occur; choose the alias approach
if you want backward compatibility with minimal code changes.
In `@resources/js/components/UserProfileImageSelector.vue`:
- Around line 15-16: Change the attribute hyphenation and order to satisfy the
frontend style checks: rename the prop binding :invalidFileSizeMessage to
:invalid-file-size-message to match :invalid-file-type-message; in the element
that currently has `@click` and autofocus (around where autofocus is on line 104)
move the autofocus attribute to appear before the `@click` handler; and in the
element with ref and width (around line 119) move the ref attribute so it
appears before the width attribute. Ensure you update the exact attributes in
UserProfileImageSelector.vue (the elements using :invalid-file-size-message,
autofocus/@click, and ref/width) without altering other logic.
- Line 127: Remove the unused import useTemplateRef from the import list in
UserProfileImageSelector.vue; keep the existing refs (e.g., cropperImgRef
defined via const cropperImgRef = ref(null) and used as ref="cropperImgRef") and
only import nextTick, ref, and watch so the unused-symbol lint error is
resolved.
In `@resources/js/composables/useFileHelpers.js`:
- Around line 10-28: The fileSize function should validate and normalize inputs
and promote values that round up across unit boundaries: coerce sizeInBytes to a
Number and if it's NaN or <= 0 return "0 B"; keep the units array and compute
exponent as before, compute value = sizeInBytes / Math.pow(1000, exponent), then
if after rounding (e.g., value.toFixed(0)) the value would be >= 1000 and
exponent < units.length - 1, increment exponent and recompute value = value /
1000 so boundaries like 999_999 become "1 MB"; finally return the rounded value
with the units[exponent]. Ensure negative/invalid inputs never produce undefined
units by returning "0 B".
---
Outside diff comments:
In `@resources/js/views/AdminStreamingSettings.vue`:
- Around line 52-64: The CSS file upload limit in the SettingsFileSelector for
streaming CSS is out of sync with backend validation: change the max-file-size
passed to the SettingsFileSelector (the prop currently set via
v-model/file-url="settings.css_file" and related bindings) from 5_000_000 to
500_000 so the frontend enforces the same 500 KB limit as
UpdateStreamingSettingsRequest; leave the pause image/upload limit unchanged if
it already uses 5_000_000.
In `@tests/Frontend/e2e/RoomsViewFilesFileActions.cy.js`:
- Around line 20-27: The assertion expecting the i18n fallback for max size is
missing quotes around the interpolated size; update the check on the
cy.get('[data-test="room-files-upload-dialog"]') chain so the expected string is
'app.file.max_size_{"size":"30 MB"}' (i.e., add surrounding quotes for the size
value and include the closing brace) so the .should("include.text", ...) matches
the usual i18n format.
---
Nitpick comments:
In `@config/bigbluebutton.php`:
- Line 13: The config currently uses explode(',', env('BBB_ALLOWED_FILE_MIMES',
'...')) which yields [''] for empty env and doesn't trim tokens; update the
allowed_file_mimes parsing to split on commas, trim each token, and filter out
empty/whitespace-only entries (e.g., via array_map('trim') + array_filter or
preg_split with PREG_SPLIT_NO_EMPTY and trim) so that allowed_file_mimes
contains only valid non-empty extensions before passing to File::types(...) /
extensions(...).
In `@resources/js/components/UserProfileImageSelector.vue`:
- Around line 214-243: The onFileSelect handler currently sets modalVisible and
isLoadingAction but lacks error handling; add a reader.onerror callback that
sets isLoadingAction.value = false and modalVisible.value = false (or shows an
error state) and logs the error, and wrap the new Cropper(...) instantiation
inside a try/catch in onFileSelect (or inside reader.onload) so that if Cropper
throws synchronously you also reset isLoadingAction.value and modalVisible.value
and clean up any partially created cropper.value; reference reader,
reader.onerror, onFileSelect, cropper.value, Cropper, isLoadingAction.value and
modalVisible.value when implementing.
In `@resources/js/views/AdminSettings.vue`:
- Around line 1471-1477: Add a defensive computed normalizer named bbbFileMimes
that reads settingsStore.getSetting('bbb.file_mimes') and returns an array (if
already an array return it; otherwise split the string on commas, trim each
entry and filter out empties), then replace the direct binding to
settingsStore.getSetting('bbb.file_mimes') passed into SettingsFileSelector with
the computed bbbFileMimes so the prop always receives an array compatible with
SettingsFileSelector (which uses .join()).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1a593afc-bcc8-481f-b052-495ec7270062
📒 Files selected for processing (22)
app/Http/Requests/StoreRoomFileRequest.phpapp/Http/Requests/UpdateRoomStreamingConfigRequest.phpapp/Http/Requests/UpdateRoomTypeStreamingSettingsRequest.phpapp/Http/Requests/UpdateSettingsRequest.phpapp/Http/Requests/UpdateStreamingSettingsRequest.phpapp/Http/Requests/UserRequest.phpconfig/bigbluebutton.phplang/en/app.phplang/en/rooms.phpresources/js/components/AdminStreamingRoomTypeEditButton.vueresources/js/components/RoomTabFilesUploadButton.vueresources/js/components/RoomTabStreamingConfigButton.vueresources/js/components/SettingsFileSelector.vueresources/js/components/SettingsImageSelector.vueresources/js/components/UserProfileImageSelector.vueresources/js/composables/useFileHelpers.jsresources/js/views/AdminSettings.vueresources/js/views/AdminStreamingSettings.vuetests/Backend/Feature/api/v1/ApplicationTest.phptests/Backend/Feature/api/v1/SettingsTest.phptests/Frontend/e2e/RoomsViewFilesFileActions.cy.jstests/Frontend/fixtures/config.json
✅ Files skipped from review due to trivial changes (2)
- resources/js/components/AdminStreamingRoomTypeEditButton.vue
- tests/Frontend/fixtures/config.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
resources/js/components/UserProfileImageSelector.vue (1)
15-15:⚠️ Potential issue | 🔴 CriticalResolve the still-present frontend lint/style failures.
Line 127 is a CI failure because
useTemplateRefis unused; Lines 15, 104, and 119 still have the style warnings already reported.🛠️ Proposed fix
- :invalidFileSizeMessage="$t('app.validation.too_large')" + :invalid-file-size-message="$t('app.validation.too_large')":disabled="isSavingAction" data-test="dialog-cancel-button" - `@click`="closeModal" autofocus + `@click`="closeModal"<img :src="selectedFile" - width="100%" ref="cropperImgRef" + width="100%" :alt="$t('admin.users.image.title')"-import { nextTick, ref, useTemplateRef, watch } from "vue"; +import { nextTick, ref, watch } from "vue";Also applies to: 103-104, 117-119, 127-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/components/UserProfileImageSelector.vue` at line 15, Remove or use the unused template helper and any other unused bindings reported by the linter: either delete the unused import/variable useTemplateRef from the component setup or actually use it where intended, and similarly remove or use any other unused variables/computeds flagged at the same locations; also fix the style warnings around the prop binding :invalidFileSizeMessage="$t('app.validation.too_large')" by aligning it with the project formatting (consistent spacing/line breaks) so the component passes frontend linting.
🧹 Nitpick comments (1)
resources/js/components/UserProfileImageSelector.vue (1)
285-338: Scope handle lookup to this cropper instance.
document.getElementsByClassName(...)[0]can select another cropper’s handles or returnundefined; the followingsetAttribute/addEventListenercalls will then target the wrong DOM or throw.♿ Safer scoped lookup pattern
function initKeyboardShortcuts() { const dimensions = cropper.value.getData(); scaleMove.value = Math.min(dimensions.width, dimensions.height) / 100; + const cropperRoot = cropper.value.container; // Get all handles - moveHandle.value = document.getElementsByClassName("cropper-move")[0]; - topLeftHandle.value = document.getElementsByClassName( - "cropper-point point-nw", - )[0]; - topRightHandle.value = document.getElementsByClassName( - "cropper-point point-ne", - )[0]; - bottomLeftHandle.value = document.getElementsByClassName( - "cropper-point point-sw", - )[0]; - bottomRightHandle.value = document.getElementsByClassName( - "cropper-point point-se", - )[0]; + moveHandle.value = cropperRoot?.querySelector(".cropper-move"); + topLeftHandle.value = cropperRoot?.querySelector(".cropper-point.point-nw"); + topRightHandle.value = cropperRoot?.querySelector(".cropper-point.point-ne"); + bottomLeftHandle.value = cropperRoot?.querySelector(".cropper-point.point-sw"); + bottomRightHandle.value = cropperRoot?.querySelector(".cropper-point.point-se"); + + if ( + !moveHandle.value || + !topLeftHandle.value || + !topRightHandle.value || + !bottomLeftHandle.value || + !bottomRightHandle.value + ) { + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/components/UserProfileImageSelector.vue` around lines 285 - 338, The element lookups in initKeyboardShortcuts currently use document.getElementsByClassName(...) which can find handles from other cropper instances or return undefined; change these to query the current cropper's container (e.g., obtain the cropper DOM container from cropper.value via its API or stored element, assign it to a local container variable) and call container.getElementsByClassName(...) for moveHandle, topLeftHandle, topRightHandle, bottomLeftHandle, bottomRightHandle; also guard each resulting element (check for null/undefined) before calling setAttribute or addEventListener so you never operate on the wrong or missing node.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@resources/js/components/UserProfileImageSelector.vue`:
- Around line 341-443: Each keyboard handler (moveEventListener,
topLeftEventListener, topRightEventListener, bottomLeftEventListener,
bottomRightEventListener) should early-return for non-arrow keys and call
e.preventDefault() when an Arrow key is handled so the page/dialog doesn't
scroll; update each function to first check if e.key startsWith "Arrow" (or
matches "ArrowUp"/"ArrowDown"/"ArrowLeft"/"ArrowRight"), call
e.preventDefault(), then run the existing switch logic and
cropper.value.setCropBoxData(cropDimensions).
- Around line 215-255: The onFileSelect/FileReader/Cropper path can finish after
the user cancels, so make the async cropper initialization cancel-safe: when
creating the FileReader and in reader.onload (inside onFileSelect), check that
modalVisible.value is still true, selectedFile.value still matches the file, and
cropperImgRef.value is non-null before calling new Cropper; also store the
FileReader instance so closeModal can call reader.abort() (or set a cancellation
flag) and ensure closeModal clears isLoadingAction.value and prevents any late
ready callback from initializing cropper.value or calling initKeyboardShortcuts;
update the ready handler to re-check modalVisible and cropperImgRef before
setting isLoadingAction/value or calling initKeyboardShortcuts.
---
Duplicate comments:
In `@resources/js/components/UserProfileImageSelector.vue`:
- Line 15: Remove or use the unused template helper and any other unused
bindings reported by the linter: either delete the unused import/variable
useTemplateRef from the component setup or actually use it where intended, and
similarly remove or use any other unused variables/computeds flagged at the same
locations; also fix the style warnings around the prop binding
:invalidFileSizeMessage="$t('app.validation.too_large')" by aligning it with the
project formatting (consistent spacing/line breaks) so the component passes
frontend linting.
---
Nitpick comments:
In `@resources/js/components/UserProfileImageSelector.vue`:
- Around line 285-338: The element lookups in initKeyboardShortcuts currently
use document.getElementsByClassName(...) which can find handles from other
cropper instances or return undefined; change these to query the current
cropper's container (e.g., obtain the cropper DOM container from cropper.value
via its API or stored element, assign it to a local container variable) and call
container.getElementsByClassName(...) for moveHandle, topLeftHandle,
topRightHandle, bottomLeftHandle, bottomRightHandle; also guard each resulting
element (check for null/undefined) before calling setAttribute or
addEventListener so you never operate on the wrong or missing node.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3d0c47ee-a96b-4fde-abe9-b8867ff4a3c7
📒 Files selected for processing (3)
lang/en/admin.phpresources/js/components/UserProfileImageSelector.vueresources/js/components/UserTabProfile.vue
✅ Files skipped from review due to trivial changes (1)
- lang/en/admin.php
Comment on lines
+215
to
255
| function closeModal() { | ||
| if (cropper.value) { | ||
| clearKeyboardShortcuts(); | ||
| cropper.value.destroy(); | ||
| cropper.value = null; | ||
| } | ||
| modalVisible.value = false; | ||
| selectedFile.value = null; | ||
| } | ||
|
|
||
| async function onFileSelect(event) { | ||
| modalVisible.value = true; | ||
| isLoadingAction.value = true; | ||
|
|
||
| const file = event.files[0]; | ||
|
|
||
| const reader = new FileReader(); | ||
| reader.onload = (event) => { | ||
| reader.onload = async (event) => { | ||
| selectedFile.value = event.target.result; | ||
| cropperRef.value.replace(selectedFile.value); | ||
| isLoadingAction.value = false; | ||
|
|
||
| await nextTick(); | ||
|
|
||
| cropper.value = new Cropper(cropperImgRef.value, { | ||
| aspectRatio: 1, | ||
| autoCropArea: 0.9, | ||
| background: false, | ||
| guides: false, | ||
| center: false, | ||
| rotatable: false, | ||
| zoomable: false, | ||
| movable: false, | ||
| viewMode: 1, | ||
| dragMode: "none", | ||
| ready: async function () { | ||
| isLoadingAction.value = false; | ||
|
|
||
| initKeyboardShortcuts(); | ||
| }, | ||
| }); | ||
| }; | ||
| reader.readAsDataURL(file); |
Contributor
There was a problem hiding this comment.
Make async cropper initialization cancel-safe.
If the user cancels while FileReader/nextTick/Cropper ready is still pending, the stale callback can initialize Cropper with a null cropperImgRef and leave isLoadingAction stuck.
🐛 Proposed guard for stale initialization
const cropper = ref();
const cropperImgRef = ref(null);
+const cropperInitToken = ref(0); function closeModal() {
+ cropperInitToken.value += 1;
+ isLoadingAction.value = false;
if (cropper.value) {
clearKeyboardShortcuts();
cropper.value.destroy();
cropper.value = null;
}
@@
async function onFileSelect(event) {
+ const initToken = ++cropperInitToken.value;
modalVisible.value = true;
isLoadingAction.value = true;
const file = event.files[0];
const reader = new FileReader();
reader.onload = async (event) => {
+ if (initToken !== cropperInitToken.value || !modalVisible.value) {
+ return;
+ }
+
selectedFile.value = event.target.result;
await nextTick();
+ if (
+ initToken !== cropperInitToken.value ||
+ !modalVisible.value ||
+ !cropperImgRef.value
+ ) {
+ selectedFile.value = null;
+ isLoadingAction.value = false;
+ return;
+ }
+
cropper.value = new Cropper(cropperImgRef.value, {
@@
dragMode: "none",
ready: async function () {
+ if (initToken !== cropperInitToken.value) {
+ return;
+ }
isLoadingAction.value = false;
initKeyboardShortcuts();
},
});
};
+ reader.onerror = () => {
+ if (initToken === cropperInitToken.value) {
+ isLoadingAction.value = false;
+ }
+ };
reader.readAsDataURL(file);
}📝 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.
Suggested change
| function closeModal() { | |
| if (cropper.value) { | |
| clearKeyboardShortcuts(); | |
| cropper.value.destroy(); | |
| cropper.value = null; | |
| } | |
| modalVisible.value = false; | |
| selectedFile.value = null; | |
| } | |
| async function onFileSelect(event) { | |
| modalVisible.value = true; | |
| isLoadingAction.value = true; | |
| const file = event.files[0]; | |
| const reader = new FileReader(); | |
| reader.onload = (event) => { | |
| reader.onload = async (event) => { | |
| selectedFile.value = event.target.result; | |
| cropperRef.value.replace(selectedFile.value); | |
| isLoadingAction.value = false; | |
| await nextTick(); | |
| cropper.value = new Cropper(cropperImgRef.value, { | |
| aspectRatio: 1, | |
| autoCropArea: 0.9, | |
| background: false, | |
| guides: false, | |
| center: false, | |
| rotatable: false, | |
| zoomable: false, | |
| movable: false, | |
| viewMode: 1, | |
| dragMode: "none", | |
| ready: async function () { | |
| isLoadingAction.value = false; | |
| initKeyboardShortcuts(); | |
| }, | |
| }); | |
| }; | |
| reader.readAsDataURL(file); | |
| function closeModal() { | |
| cropperInitToken.value += 1; | |
| isLoadingAction.value = false; | |
| if (cropper.value) { | |
| clearKeyboardShortcuts(); | |
| cropper.value.destroy(); | |
| cropper.value = null; | |
| } | |
| modalVisible.value = false; | |
| selectedFile.value = null; | |
| } | |
| async function onFileSelect(event) { | |
| const initToken = ++cropperInitToken.value; | |
| modalVisible.value = true; | |
| isLoadingAction.value = true; | |
| const file = event.files[0]; | |
| const reader = new FileReader(); | |
| reader.onload = async (event) => { | |
| if (initToken !== cropperInitToken.value || !modalVisible.value) { | |
| return; | |
| } | |
| selectedFile.value = event.target.result; | |
| await nextTick(); | |
| if ( | |
| initToken !== cropperInitToken.value || | |
| !modalVisible.value || | |
| !cropperImgRef.value | |
| ) { | |
| selectedFile.value = null; | |
| isLoadingAction.value = false; | |
| return; | |
| } | |
| cropper.value = new Cropper(cropperImgRef.value, { | |
| aspectRatio: 1, | |
| autoCropArea: 0.9, | |
| background: false, | |
| guides: false, | |
| center: false, | |
| rotatable: false, | |
| zoomable: false, | |
| movable: false, | |
| viewMode: 1, | |
| dragMode: "none", | |
| ready: async function () { | |
| if (initToken !== cropperInitToken.value) { | |
| return; | |
| } | |
| isLoadingAction.value = false; | |
| initKeyboardShortcuts(); | |
| }, | |
| }); | |
| }; | |
| reader.onerror = () => { | |
| if (initToken === cropperInitToken.value) { | |
| isLoadingAction.value = false; | |
| } | |
| }; | |
| reader.readAsDataURL(file); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@resources/js/components/UserProfileImageSelector.vue` around lines 215 - 255,
The onFileSelect/FileReader/Cropper path can finish after the user cancels, so
make the async cropper initialization cancel-safe: when creating the FileReader
and in reader.onload (inside onFileSelect), check that modalVisible.value is
still true, selectedFile.value still matches the file, and cropperImgRef.value
is non-null before calling new Cropper; also store the FileReader instance so
closeModal can call reader.abort() (or set a cancellation flag) and ensure
closeModal clears isLoadingAction.value and prevents any late ready callback
from initializing cropper.value or calling initKeyboardShortcuts; update the
ready handler to re-check modalVisible and cropperImgRef before setting
isLoadingAction/value or calling initKeyboardShortcuts.
Show up to two decimal places
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
resources/js/components/UserProfileImageSelector.vue (1)
164-165:⚠️ Potential issue | 🟠 MajorMake cropper initialization cancel-safe.
If the dialog is cancelled while
FileReader,nextTick, or Cropper’sreadycallback is still pending, stale callbacks can create a Cropper against a missing image ref or leave loading state stuck.Guard stale async initialization
const cropper = ref(); const cropperImgRef = ref(null); +const cropperInitToken = ref(0); +const activeReader = ref(null);function closeModal() { + cropperInitToken.value += 1; + if (activeReader.value?.readyState === FileReader.LOADING) { + activeReader.value.abort(); + } + activeReader.value = null; + isLoadingAction.value = false; + if (cropper.value) { clearKeyboardShortcuts(); cropper.value.destroy(); cropper.value = null; } @@ async function onFileSelect(event) { + const initToken = ++cropperInitToken.value; modalVisible.value = true; isLoadingAction.value = true; const file = event.files[0]; const reader = new FileReader(); - reader.onload = async (event) => { - selectedFile.value = event.target.result; + activeReader.value = reader; + reader.onload = async (loadEvent) => { + if (initToken !== cropperInitToken.value || !modalVisible.value) { + return; + } + + selectedFile.value = loadEvent.target.result; await nextTick(); + if ( + initToken !== cropperInitToken.value || + !modalVisible.value || + !cropperImgRef.value + ) { + isLoadingAction.value = false; + return; + } + cropper.value = new Cropper(cropperImgRef.value, { @@ dragMode: "none", ready: async function () { + if (initToken !== cropperInitToken.value || !modalVisible.value) { + return; + } + isLoadingAction.value = false; initKeyboardShortcuts(); }, }); }; + reader.onerror = reader.onabort = () => { + if (initToken === cropperInitToken.value) { + isLoadingAction.value = false; + } + }; reader.readAsDataURL(file); }Also applies to: 215-255
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/components/UserProfileImageSelector.vue` around lines 164 - 165, The async Cropper initialization must be made cancel-safe: introduce a local cancellation flag/token used in the image load flow that is set when the dialog is closed/cancelled, abort any in-progress FileReader, and return early from pending callbacks; before calling new Cropper(...) or accessing cropperImgRef.value validate the ref and the token, and in Cropper's ready callback check the token before touching state; also ensure cropper (ref) is destroyed and loading state cleared on cancel/cleanup (affects the functions using cropper, cropperImgRef, FileReader, nextTick, and Cropper.ready in the component).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@resources/js/components/UserProfileImageSelector.vue`:
- Around line 164-165: The async Cropper initialization must be made
cancel-safe: introduce a local cancellation flag/token used in the image load
flow that is set when the dialog is closed/cancelled, abort any in-progress
FileReader, and return early from pending callbacks; before calling new
Cropper(...) or accessing cropperImgRef.value validate the ref and the token,
and in Cropper's ready callback check the token before touching state; also
ensure cropper (ref) is destroyed and loading state cleared on cancel/cleanup
(affects the functions using cropper, cropperImgRef, FileReader, nextTick, and
Cropper.ready in the component).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: dff2f633-ecf0-43ba-9189-56d6ee10126e
📒 Files selected for processing (6)
resources/js/components/RoomTabFilesUploadButton.vueresources/js/components/SettingsFileSelector.vueresources/js/components/SettingsImageSelector.vueresources/js/components/UserProfileImageSelector.vueresources/js/composables/useFileHelpers.jstests/Frontend/e2e/RoomsViewFilesFileActions.cy.js
🚧 Files skipped from review as they are similar to previous changes (3)
- resources/js/components/SettingsFileSelector.vue
- tests/Frontend/e2e/RoomsViewFilesFileActions.cy.js
- resources/js/composables/useFileHelpers.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Type
Checklist
Changes
Other information
Summary by CodeRabbit