feat: Add option in room to always use sytem-wide default presentation#2746
Open
feat: Add option in room to always use sytem-wide default presentation#2746
Conversation
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:
WalkthroughAdds system-default presentation support: DB migration and Room flags, new FormRequest and controller method + route to manage system-default settings, Room and MeetingService changes to honor flags, frontend components/UI for configuring system-default, translations, and minor CSS/markup tweaks. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #2746 +/- ##
=============================================
- Coverage 96.77% 96.39% -0.38%
- Complexity 1839 1849 +10
=============================================
Files 444 446 +2
Lines 12570 12650 +80
Branches 2063 2076 +13
=============================================
+ Hits 12164 12194 +30
- Misses 406 456 +50 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
app/Models/Room.php (1)
196-214: System-default-as-default can be ignored due to early return.If a room file is already default and
use_in_meetingis true, the function returns before honoringuse_system_default_presentation_as_default. This prevents the system default from becoming default when the option is enabled.🔧 Proposed fix
- $currentDefault = $this->files()->firstWhere('default', true); + $systemDefault = app(BigBlueButtonSettings::class)->default_presentation; + $currentDefault = $this->files()->firstWhere('default', true); if ($currentDefault != null) { // If the default file is also used in the next meeting, it can stay the default, otherwise remove default // and look for alternative - if ($currentDefault->use_in_meeting == true) { + if ($currentDefault->use_in_meeting == true && ! ($systemDefault && $this->use_system_default_presentation_as_default)) { return; } $currentDefault->default = false; $currentDefault->save(); } // If system has a default presentation and the system presentation is set as default, no further action needed - if (app(BigBlueButtonSettings::class)->default_presentation && $this->use_system_default_presentation_as_default) { + if ($systemDefault && $this->use_system_default_presentation_as_default) { return; }
🤖 Fix all issues with AI agents
In `@app/Http/Requests/UpdateRoomSystemDefaultPresentation.php`:
- Around line 7-15: The FormRequest subclass UpdateRoomSystemDefaultPresentation
is missing an authorize() override (FormRequest defaults authorize() to false),
so the endpoint will always 403; add an authorize() method to this class that
either returns true or implements the proper permission check (e.g., inspect the
current user/room ownership/ability) and keep the existing rules() validation
for 'use_in_meeting' and 'default' booleans; ensure the method name is authorize
and it returns a boolean reflecting the intended access control.
In `@resources/css/override/_animations.css`:
- Around line 12-15: The leave selector currently uses a global class
`.p-message-leave-active` which affects all message components; narrow its scope
by changing the selector to target only inline notes (e.g., use
`.inline-note.p-message-leave-active` alongside
`.inline-note.p-message-enter-active`) so the zero-duration animation applies
only to the `.inline-note` component and not globally.
In `@resources/js/components/RoomTabFiles.vue`:
- Around line 370-374: The API response may omit system_default for users
without viewAllFiles, so ensure systemDefault (ref) always has a safe object
before the template reads its properties: when assigning the API result to
systemDefault.value (in the RoomTabFiles.vue component where systemDefault is
updated), check if response.system_default is present and if not set
systemDefault.value to the existing default object { file: null, use_in_meeting:
false, use_as_default: false }; replace any direct assignment like
systemDefault.value = response.system_default with a guarded assignment that
falls back to the default ref value.
In `@resources/js/components/RoomTabFilesSystemDefault.vue`:
- Around line 52-60: The Button rendered as an anchor with target="_blank" in
RoomTabFilesSystemDefault.vue (the Button element using as="a", :href="file",
and data-test="room-files-view-button") needs rel="noopener noreferrer" to
prevent reverse-tabnabbing; update the Button props to include rel="noopener
noreferrer" so the generated <a> tag includes that attribute whenever
target="_blank" is used.
Comment on lines
52
to
60
| <Button | ||
| v-tooltip="$t('rooms.files.view')" | ||
| :aria-label="$t('rooms.files.view')" | ||
| target="_blank" | ||
| icon="fa-solid fa-eye" | ||
| data-test="room-files-view-button" | ||
| as="a" | ||
| :href="file" | ||
| /> |
Contributor
There was a problem hiding this comment.
Add rel="noopener noreferrer" on the view link.
target="_blank" without rel enables reverse‑tabnabbing.
🔒 Proposed fix
v-tooltip="$t('rooms.files.view')"
:aria-label="$t('rooms.files.view')"
target="_blank"
+ rel="noopener noreferrer"
icon="fa-solid fa-eye"
data-test="room-files-view-button"
as="a"
:href="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
| <Button | |
| v-tooltip="$t('rooms.files.view')" | |
| :aria-label="$t('rooms.files.view')" | |
| target="_blank" | |
| icon="fa-solid fa-eye" | |
| data-test="room-files-view-button" | |
| as="a" | |
| :href="file" | |
| /> | |
| <Button | |
| v-tooltip="$t('rooms.files.view')" | |
| :aria-label="$t('rooms.files.view')" | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| icon="fa-solid fa-eye" | |
| data-test="room-files-view-button" | |
| as="a" | |
| :href="file" | |
| /> |
🤖 Prompt for AI Agents
In `@resources/js/components/RoomTabFilesSystemDefault.vue` around lines 52 - 60,
The Button rendered as an anchor with target="_blank" in
RoomTabFilesSystemDefault.vue (the Button element using as="a", :href="file",
and data-test="room-files-view-button") needs rel="noopener noreferrer" to
prevent reverse-tabnabbing; update the Button props to include rel="noopener
noreferrer" so the generated <a> tag includes that attribute whenever
target="_blank" is used.
PILOS
|
||||||||||||||||||||||||||||
| Project |
PILOS
|
| Branch Review |
2662-add-option-to-always-include-default-slide
|
| Run status |
|
| Run duration | 07m 23s |
| Commit |
|
| Committer | Samuel Weirich |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
613
|
| View all changes introduced in this branch ↗︎ | |
Collaborator
Author
|
Collaborator
Author
|
Feedback from @Sabr1n4W:
|
Contributor
Another thing we discussed adressed the default behaviour when uploading a new presentation and making it available in the next meeting for the first time (no other presentation enabled). Current default behaviour
Possible adjusted default behaviour
Reason for suggested changes of the default behaviour
Possible disadvantages
Additional possible changes
|
# Conflicts: # app/Http/Controllers/api/v1/RoomFileController.php # app/Models/Room.php
samuelwei
force-pushed
the
2662-add-option-to-always-include-default-slide
branch
from
f54187d to
802d541
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
app/Http/Controllers/api/v1/RoomFileController.php (2)
140-141: Missing PHPDoc comment onupdateSystemDefault().Every other public method in this controller has a docblock;
updateSystemDefaulthas none.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Controllers/api/v1/RoomFileController.php` around lines 140 - 141, Add a PHPDoc docblock above the public function updateSystemDefault(UpdateRoomSystemDefaultPresentation $request, Room $room) describing the method's purpose, the parameter types and names (`@param` UpdateRoomSystemDefaultPresentation $request, `@param` Room $room), the return type (`@return` \Illuminate\Http\JsonResponse or whatever other response is used by other methods), and any relevant authorization or behavior notes to match the style of other public methods in this controller; place the docblock directly above the updateSystemDefault declaration so it matches existing method docblocks.
120-128: No database transaction wrapping related multi-step writes.Both
update()(lines 120–128) andupdateSystemDefault()(lines 140–165) perform a direct bulk DB update ($room->files()->update(...)) followed by$room->save(). If$room->save()fails, filedefaultflags are already cleared in the database while the room's own flags remain unchanged, leaving data in an inconsistent state.♻️ Suggested refactor (example for `updateSystemDefault`)
+ \Illuminate\Support\Facades\DB::transaction(function () use ($request, $room) { if ($request->has('use_in_meeting')) { $room->use_system_default_presentation_in_meeting = $request->use_in_meeting; } if ($request->has('default')) { if ($request->default === true) { $room->files()->update(['default' => false]); $room->use_system_default_presentation_in_meeting = true; } $room->use_system_default_presentation_as_default = $request->default; } $room->save(); + });Also applies to: 140-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Controllers/api/v1/RoomFileController.php` around lines 120 - 128, The multi-step updates in RoomFileController (methods update and updateSystemDefault) must be wrapped in a DB transaction to avoid partial commits: move the bulk file update calls (e.g. $room->files()->whereNot('id', $file->id)->update(...) and $room->files()->update(...)) and the subsequent $room->save() into a single DB::transaction(...) block (or manual beginTransaction/commit/rollback) so that if $room->save() fails the prior updates are rolled back; ensure exceptions are caught and rethrown or handled so the transaction rolls back on error and preserve existing behavior (including throwing/returning validation errors).app/Services/MeetingService.php (1)
107-133: CacheBigBlueButtonSettingsinstance –app()is called 4 times.♻️ Suggested refactor
+ $bbbSettings = app(BigBlueButtonSettings::class); + // Use system default presentation as default, if explicitly set - $useSystemDefaultFileAsDefault = app(BigBlueButtonSettings::class)->default_presentation && $this->meeting->room->use_system_default_presentation_as_default; + $useSystemDefaultFileAsDefault = $bbbSettings->default_presentation && $this->meeting->room->use_system_default_presentation_as_default; if ($useSystemDefaultFileAsDefault) { - $meetingParams->addPresentation(app(BigBlueButtonSettings::class)->default_presentation); + $meetingParams->addPresentation($bbbSettings->default_presentation); } // ... - if (! $useSystemDefaultFileAsDefault && app(BigBlueButtonSettings::class)->default_presentation && ( - empty($files->toArray()) || $this->meeting->room->use_system_default_presentation_in_meeting + if (! $useSystemDefaultFileAsDefault && $bbbSettings->default_presentation && ( + empty($files->toArray()) || $this->meeting->room->use_system_default_presentation_in_meeting )) { - $meetingParams->addPresentation(app(BigBlueButtonSettings::class)->default_presentation); + $meetingParams->addPresentation($bbbSettings->default_presentation); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Services/MeetingService.php` around lines 107 - 133, The code calls app(BigBlueButtonSettings::class) multiple times; cache that instance into a local variable (e.g. $bbbSettings) at the start of the method in MeetingService and replace all occurrences (the calls within the $useSystemDefaultFileAsDefault assignment, the two meetingParams->addPresentation calls, and the conditional that checks default_presentation and use_system_default_presentation_in_meeting) with that cached $bbbSettings->default_presentation / $bbbSettings->... to avoid repeated container lookups and improve readability.app/Http/Requests/UpdateRoomSystemDefaultPresentation.php (1)
9-15:requiredvalidation makes the controller's$request->has()guards dead code.Both
use_in_meetinganddefaultarerequired, so validation rejects the request if either is absent — meaning$request->has(...)is alwaystrueinsideupdateSystemDefault(). The partial-update pattern in the controller impliessometimes, but the rules enforce presence.Either change the rules to
['sometimes', 'boolean']to allow genuine partial updates, or remove thehas()guards inRoomFileController::updateSystemDefault()to match the current always-present contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Requests/UpdateRoomSystemDefaultPresentation.php` around lines 9 - 15, The validation in UpdateRoomSystemDefaultPresentation::rules currently marks 'use_in_meeting' and 'default' as required which makes the $request->has(...) checks in RoomFileController::updateSystemDefault() redundant; to preserve the controller's partial-update behavior, change the rules in UpdateRoomSystemDefaultPresentation::rules to use ['sometimes','boolean'] for both 'use_in_meeting' and 'default' so missing fields are allowed and still validated when present.app/Models/Room.php (1)
208-214: Stale/incomplete comment on line 213.The comment
// If no default file is explicitly set or the system default should be used,ends with a dangling comma and doesn't match what follows (unconditionally picks the firstuse_in_meetingfile). The addition at lines 208–211 is logically correct.✏️ Suggested comment fix
- // If no default file is explicitly set or the system default should be used, + // No explicit default set; promote the first in-meeting file to default $newDefaultFile = $this->files()->firstWhere('use_in_meeting', true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/Room.php` around lines 208 - 214, The inline comment above the selection of a new default file is incomplete and misleading; update the comment to accurately describe the unconditional behavior of choosing the first file with use_in_meeting set to true when the system default presentation is not being used. Replace the dangling comment (the one before the $newDefaultFile assignment) with a clear line such as: "If we are not using the system default presentation, pick the first file marked use_in_meeting as the new default." Reference BigBlueButtonSettings, the property use_system_default_presentation_as_default, and the files()->firstWhere('use_in_meeting', true) call to make intent explicit.lang/en/rooms.php (1)
141-143: New keys placed out of alphabetical order.
always_available_in_meeting,available_in_next_meeting, andnot_available_in_next_meeting(all starting witha/n) are appended afteruploadedinstead of being inserted at their alphabetical positions within thefilesarray. This makes the file harder to scan for existing keys.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lang/en/rooms.php` around lines 141 - 143, The three translation keys 'always_available_in_meeting', 'available_in_next_meeting', and 'not_available_in_next_meeting' were appended after 'uploaded' instead of being placed alphabetically inside the files array; move those three entries into the files array at their correct alphabetical positions (so they appear where other keys starting with "a" and "n" belong), preserving their exact values and surrounding formatting/commas.
🤖 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/rooms.php`:
- Line 141: Rename the translation key files.always_available_in_meeting to
files.always_available_in_next_meeting and update its value text to remove the
ambiguity — either make it one-shot like siblings (e.g., "Available in next
video conference") or persistent (e.g., "Always available in video
conferences"); pick the correct intent, change the key to
always_available_in_next_meeting if it's next-meeting scoped, and update any
component references that use always_available_in_meeting to the new key name.
- Around line 112-115: The English translation keys for file actions were
changed but not propagated—update all locale files that still define the old
keys (files.download_hidden, files.download_visible, files.edit,
files.use_in_next_meeting, files.use_in_next_meeting_disabled) to instead define
the new keys (download_allowed, download_not_allowed,
always_available_in_meeting OR rename to always_available_in_next_meeting for
consistency, available_in_next_meeting, not_available_in_next_meeting,
configure) so all locales match English; update all E2E assertions in
RoomsViewFiles.cy.js and RoomsViewFilesFileActions.cy.js to expect the new keys
(download_allowed, download_not_allowed, always_available_in_next_meeting or
always_available_in_next_meeting if you rename, available_in_next_meeting,
not_available_in_next_meeting, configure); if you choose to rename
always_available_in_meeting to always_available_in_next_meeting, rename that key
across translations, code references and tests to keep naming consistent.
In `@resources/js/components/RoomTabFilesSystemDefault.vue`:
- Around line 92-99: Static analysis flags that the Button attribute order
should place the static attribute autofocus before the event binding `@click`;
update the Button element in RoomTabFilesSystemDefault.vue (the Button that uses
:label="$t('app.cancel')", severity="secondary", :disabled="isLoadingAction",
data-test="dialog-cancel-button", `@click`="modalVisible = false", autofocus) so
that the autofocus attribute appears before the `@click` attribute (i.e., move
autofocus to precede `@click`) while preserving all other props and bindings.
- Around line 155-182: Add a missing disabled prop to the component's
defineProps (e.g., add disabled: { type: Boolean, default: false }) so the
parent's :disabled="isBusy" is accepted, and update the View and Configure
buttons to use a combined disabled state (use props.disabled || isLoadingAction)
so they are inert while the parent is busy or an internal action is loading;
modify the button bindings in RoomTabFilesSystemDefault.vue where the "View" and
"Configure" buttons are rendered and reference props.disabled and
isLoadingAction accordingly.
---
Duplicate comments:
In `@resources/js/components/RoomTabFiles.vue`:
- Around line 429-436: The component crashes because systemDefault.value is set
directly from response.data.system_default which can be undefined; update the
fetch handler in the API promise (.then) so systemDefault.value is assigned a
safe default (e.g., null or an empty object) using a null-coalescing fallback
(assign systemDefault.value = response.data.system_default ?? null) and ensure
any downstream code or template uses safe access (e.g., guards like
systemDefault && systemDefault.file) — locate the assignment to
systemDefault.value in the block that also sets files.value, defaultFile.value
and calls paginator.updateMeta and apply this defensive assignment there.
In `@resources/js/components/RoomTabFilesSystemDefault.vue`:
- Around line 52-60: The anchor rendered by the Button with target="_blank" (the
<Button ... as="a" target="_blank" :href="file" ... /> instance) is missing a
rel attribute which allows reverse-tabnabbing; update that Button invocation to
include rel="noopener noreferrer" so the generated <a> tag uses rel="noopener
noreferrer" (ensure the change is applied to the Button component usage that has
as="a", target="_blank", and :href="file").
---
Nitpick comments:
In `@app/Http/Controllers/api/v1/RoomFileController.php`:
- Around line 140-141: Add a PHPDoc docblock above the public function
updateSystemDefault(UpdateRoomSystemDefaultPresentation $request, Room $room)
describing the method's purpose, the parameter types and names (`@param`
UpdateRoomSystemDefaultPresentation $request, `@param` Room $room), the return
type (`@return` \Illuminate\Http\JsonResponse or whatever other response is used
by other methods), and any relevant authorization or behavior notes to match the
style of other public methods in this controller; place the docblock directly
above the updateSystemDefault declaration so it matches existing method
docblocks.
- Around line 120-128: The multi-step updates in RoomFileController (methods
update and updateSystemDefault) must be wrapped in a DB transaction to avoid
partial commits: move the bulk file update calls (e.g.
$room->files()->whereNot('id', $file->id)->update(...) and
$room->files()->update(...)) and the subsequent $room->save() into a single
DB::transaction(...) block (or manual beginTransaction/commit/rollback) so that
if $room->save() fails the prior updates are rolled back; ensure exceptions are
caught and rethrown or handled so the transaction rolls back on error and
preserve existing behavior (including throwing/returning validation errors).
In `@app/Http/Requests/UpdateRoomSystemDefaultPresentation.php`:
- Around line 9-15: The validation in UpdateRoomSystemDefaultPresentation::rules
currently marks 'use_in_meeting' and 'default' as required which makes the
$request->has(...) checks in RoomFileController::updateSystemDefault()
redundant; to preserve the controller's partial-update behavior, change the
rules in UpdateRoomSystemDefaultPresentation::rules to use
['sometimes','boolean'] for both 'use_in_meeting' and 'default' so missing
fields are allowed and still validated when present.
In `@app/Models/Room.php`:
- Around line 208-214: The inline comment above the selection of a new default
file is incomplete and misleading; update the comment to accurately describe the
unconditional behavior of choosing the first file with use_in_meeting set to
true when the system default presentation is not being used. Replace the
dangling comment (the one before the $newDefaultFile assignment) with a clear
line such as: "If we are not using the system default presentation, pick the
first file marked use_in_meeting as the new default." Reference
BigBlueButtonSettings, the property use_system_default_presentation_as_default,
and the files()->firstWhere('use_in_meeting', true) call to make intent
explicit.
In `@app/Services/MeetingService.php`:
- Around line 107-133: The code calls app(BigBlueButtonSettings::class) multiple
times; cache that instance into a local variable (e.g. $bbbSettings) at the
start of the method in MeetingService and replace all occurrences (the calls
within the $useSystemDefaultFileAsDefault assignment, the two
meetingParams->addPresentation calls, and the conditional that checks
default_presentation and use_system_default_presentation_in_meeting) with that
cached $bbbSettings->default_presentation / $bbbSettings->... to avoid repeated
container lookups and improve readability.
In `@lang/en/rooms.php`:
- Around line 141-143: The three translation keys 'always_available_in_meeting',
'available_in_next_meeting', and 'not_available_in_next_meeting' were appended
after 'uploaded' instead of being placed alphabetically inside the files array;
move those three entries into the files array at their correct alphabetical
positions (so they appear where other keys starting with "a" and "n" belong),
preserving their exact values and surrounding formatting/commas.
Comment on lines
+112
to
+115
| 'download_allowed' => 'Download allowed', | ||
| 'download_not_allowed' => 'Download not allowed', | ||
| 'downloadable' => 'Downloadable', | ||
| 'edit' => 'Edit file', | ||
| 'configure' => 'Configure file', |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for dangling references to removed translation keys
rg -n --type=vue --type=js --type=php \
-e "files\.download_hidden" \
-e "files\.download_visible" \
-e "files\.edit['\"]" \
-e "use_in_next_meeting" \
-e "download_hidden" \
-e "download_visible" \
.Repository: THM-Health/PILOS
Length of output: 87
🏁 Script executed:
#!/bin/bash
# Also search in JSON files and any files that might reference these keys
rg -n "download_hidden|download_visible|use_in_next_meeting" \
--type=json --type=ts --type=jsx .Repository: THM-Health/PILOS
Length of output: 87
🏁 Script executed:
#!/bin/bash
# Check for other locale files that may need updates
fd -e php -path "*/lang/*" | head -20Repository: THM-Health/PILOS
Length of output: 228
🏁 Script executed:
#!/bin/bash
# Read the actual file to understand the context of changes
cat -n lang/en/rooms.php | sed -n '100,150p'Repository: THM-Health/PILOS
Length of output: 2927
🏁 Script executed:
#!/bin/bash
# Search for dangling references to removed keys with simpler patterns
echo "=== Checking for references to removed keys ==="
rg "download_hidden|download_visible" -n --no-heading
rg "files\.edit" -n --no-heading
rg "use_in_next_meeting" -n --no-headingRepository: THM-Health/PILOS
Length of output: 7710
🏁 Script executed:
#!/bin/bash
# Find all locale files
echo "=== Finding all locale translation files ==="
find . -path "*/lang/*/rooms.php" -type fRepository: THM-Health/PILOS
Length of output: 231
🏁 Script executed:
#!/bin/bash
# Check for files that reference the old keys
rg "download_hidden" . 2>/dev/null | head -20Repository: THM-Health/PILOS
Length of output: 1063
🏁 Script executed:
#!/bin/bash
# Look for Vue component files that might reference these translation keys
find . -name "*.vue" -o -name "*.js" | head -20 | xargs rg -l "download_hidden|download_visible|use_in_next_meeting" 2>/dev/null || echo "No matches found"Repository: THM-Health/PILOS
Length of output: 76
Critical: Translation keys were changed in English but not propagated to other locales or E2E tests.
The removed keys (files.download_hidden, files.download_visible, files.edit, files.use_in_next_meeting, files.use_in_next_meeting_disabled) are still referenced in:
- E2E tests: ~50+ assertions across RoomsViewFiles.cy.js and RoomsViewFilesFileActions.cy.js expect these old keys to appear in the UI
- Other language files: de, de-gender, fr, fa, ar all still define the old keys while the English file uses new ones
This creates a critical inconsistency where E2E tests will fail and non-English locales will have mismatched translation keys. All language files must be updated together with the same keys, and E2E tests must be updated to check for the new keys (download_allowed, download_not_allowed, always_available_in_meeting, available_in_next_meeting, not_available_in_next_meeting, configure).
Additionally, line 141 has a key naming inconsistency: always_available_in_meeting omits the _next_ infix present in available_in_next_meeting and not_available_in_next_meeting. Consider renaming to always_available_in_next_meeting for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lang/en/rooms.php` around lines 112 - 115, The English translation keys for
file actions were changed but not propagated—update all locale files that still
define the old keys (files.download_hidden, files.download_visible, files.edit,
files.use_in_next_meeting, files.use_in_next_meeting_disabled) to instead define
the new keys (download_allowed, download_not_allowed,
always_available_in_meeting OR rename to always_available_in_next_meeting for
consistency, available_in_next_meeting, not_available_in_next_meeting,
configure) so all locales match English; update all E2E assertions in
RoomsViewFiles.cy.js and RoomsViewFilesFileActions.cy.js to expect the new keys
(download_allowed, download_not_allowed, always_available_in_next_meeting or
always_available_in_next_meeting if you rename, available_in_next_meeting,
not_available_in_next_meeting, configure); if you choose to rename
always_available_in_meeting to always_available_in_next_meeting, rename that key
across translations, code references and tests to keep naming consistent.
| 'uploaded' => 'File \':name\' uploaded', | ||
| 'use_in_next_meeting' => 'Use in the next meeting', | ||
| 'use_in_next_meeting_disabled' => 'Not available in video conference', | ||
| 'always_available_in_meeting' => 'Always available in next video conference', |
Contributor
There was a problem hiding this comment.
Key name is inconsistent with its siblings and the value text is ambiguous.
- The key
always_available_in_meetingdrops the_next_infix, while both siblings use_in_next_meeting. For consistency it should bealways_available_in_next_meeting. - The value "Always available in next video conference" is internally contradictory: always implies all future meetings, while next implies only the immediately upcoming one. If the intent is a sticky/persistent setting, consider "Always available in video conferences"; if it is one-shot like the others, drop "always".
✏️ Proposed rename and wording fix
- 'always_available_in_meeting' => 'Always available in next video conference',
+ 'always_available_in_next_meeting' => 'Always available in video conferences',Any component referencing files.always_available_in_meeting would need to be updated to files.always_available_in_next_meeting accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lang/en/rooms.php` at line 141, Rename the translation key
files.always_available_in_meeting to files.always_available_in_next_meeting and
update its value text to remove the ambiguity — either make it one-shot like
siblings (e.g., "Available in next video conference") or persistent (e.g.,
"Always available in video conferences"); pick the correct intent, change the
key to always_available_in_next_meeting if it's next-meeting scoped, and update
any component references that use always_available_in_meeting to the new key
name.
Comment on lines
+92
to
+99
| <Button | ||
| :label="$t('app.cancel')" | ||
| severity="secondary" | ||
| :disabled="isLoadingAction" | ||
| data-test="dialog-cancel-button" | ||
| @click="modalVisible = false" | ||
| autofocus | ||
| /> |
Contributor
There was a problem hiding this comment.
autofocus must precede @click (static analysis warning on line 98).
✏️ Proposed fix
<Button
:label="$t('app.cancel')"
severity="secondary"
:disabled="isLoadingAction"
data-test="dialog-cancel-button"
+ autofocus
`@click`="modalVisible = false"
- autofocus
/>📝 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
| <Button | |
| :label="$t('app.cancel')" | |
| severity="secondary" | |
| :disabled="isLoadingAction" | |
| data-test="dialog-cancel-button" | |
| @click="modalVisible = false" | |
| autofocus | |
| /> | |
| <Button | |
| :label="$t('app.cancel')" | |
| severity="secondary" | |
| :disabled="isLoadingAction" | |
| data-test="dialog-cancel-button" | |
| autofocus | |
| `@click`="modalVisible = false" | |
| /> |
🧰 Tools
🪛 GitHub Check: Frontend Code Style Check
[warning] 98-98:
Attribute "autofocus" should go before "@click"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@resources/js/components/RoomTabFilesSystemDefault.vue` around lines 92 - 99,
Static analysis flags that the Button attribute order should place the static
attribute autofocus before the event binding `@click`; update the Button element
in RoomTabFilesSystemDefault.vue (the Button that uses
:label="$t('app.cancel')", severity="secondary", :disabled="isLoadingAction",
data-test="dialog-cancel-button", `@click`="modalVisible = false", autofocus) so
that the autofocus attribute appears before the `@click` attribute (i.e., move
autofocus to precede `@click`) while preserving all other props and bindings.
Comment on lines
+155
to
+182
| <script setup> | ||
| import env from "../env"; | ||
| import { useApi } from "../composables/useApi.js"; | ||
| import { ref, watch } from "vue"; | ||
| import { useFormErrors } from "../composables/useFormErrors.js"; | ||
|
|
||
| const props = defineProps({ | ||
| file: { | ||
| type: String, | ||
| default: null, | ||
| }, | ||
| roomId: { | ||
| type: String, | ||
| required: true, | ||
| }, | ||
| useInMeeting: { | ||
| type: Boolean, | ||
| default: false, | ||
| }, | ||
| useAsDefault: { | ||
| type: Boolean, | ||
| default: false, | ||
| }, | ||
| defaultFile: { | ||
| type: Object, | ||
| default: null, | ||
| }, | ||
| }); |
Contributor
There was a problem hiding this comment.
Missing disabled prop — parent's isBusy signal is silently dropped.
RoomTabFiles.vue passes :disabled="isBusy" to this component (line 56 of that file), but disabled is not declared in defineProps. In Vue 3 with <script setup>, it falls through as an HTML attribute on the root <div> and never reaches the View or Configure buttons. Both buttons remain interactive while the parent is reloading data, which can cause the modal to open with stale props.
✏️ Proposed fix
const props = defineProps({
file: { type: String, default: null },
roomId: { type: String, required: true },
useInMeeting: { type: Boolean, default: false },
useAsDefault: { type: Boolean, default: false },
defaultFile: { type: Object, default: null },
+ disabled: { type: Boolean, default: false },
});Then use :disabled="props.disabled || isLoadingAction" on the View and Configure buttons.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@resources/js/components/RoomTabFilesSystemDefault.vue` around lines 155 -
182, Add a missing disabled prop to the component's defineProps (e.g., add
disabled: { type: Boolean, default: false }) so the parent's :disabled="isBusy"
is accepted, and update the View and Configure buttons to use a combined
disabled state (use props.disabled || isLoadingAction) so they are inert while
the parent is busy or an internal action is loading; modify the button bindings
in RoomTabFilesSystemDefault.vue where the "View" and "Configure" buttons are
rendered and reference props.disabled and isLoadingAction accordingly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Models/Room.php (1)
207-223:⚠️ Potential issue | 🔴 Critical
updateDefaultFile()can silently revert an explicit user choice to not use the system default as the room default.Consider this scenario:
- A room has
use_system_default_presentation_in_meeting = trueand no files withuse_in_meeting = true.- The user explicitly sets
use_system_default_presentation_as_default = falseviaupdateSystemDefault()(withdefault=false).- The controller saves the room, then calls
updateDefaultFile().- Inside
updateDefaultFile(), no file hasdefault = true, so$currentDefaultisnulland execution falls through to line 210.- The condition
default_presentation && use_system_default_presentation_in_meetingistrue, so the method setsuse_system_default_presentation_as_default = true— overriding the user's explicit opt-out.The root cause is that
updateDefaultFile()doesn't distinguish between "no default was ever set" and "the user actively chose not to have the system default." You likely need to guard against overwriting an explicitfalseset in the same request, or restructure so thatupdateDefaultFile()only promotes the system default when no explicit user decision was made.One way to fix this: don't call
updateDefaultFile()at all when the user explicitly setsdefault=falsein theupdateSystemDefaultendpoint, or add a parameter/flag soupdateDefaultFileknows to respect the explicit choice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/Room.php` around lines 207 - 223, updateDefaultFile() currently promotes the system default when default_presentation && use_system_default_presentation_in_meeting without respecting an explicit user opt-out; change the flow so updateDefaultFile() does not override an explicit choice made by updateSystemDefault(): either (A) have the controller that handles updateSystemDefault() skip calling updateDefaultFile() when the payload contains default=false, or (B) add a parameter/flag to updateDefaultFile($respectExplicitChoice = true) and, when true, have updateDefaultFile() check for an explicit user decision (e.g. detect if use_system_default_presentation_as_default was deliberately set to false in the current request or if a dedicated flag was passed) before setting use_system_default_presentation_as_default = true; ensure references to updateDefaultFile(), updateSystemDefault(), use_system_default_presentation_as_default, use_system_default_presentation_in_meeting, and default_presentation are updated accordingly.
🧹 Nitpick comments (2)
database/migrations/2026_01_19_105554_add_use_system_default_presentation_in_next_meeting_to_rooms_table.php (1)
34-37: Minor: combinedropColumncalls into a single array.Proposed fix
Schema::table('rooms', function (Blueprint $table) { - $table->dropColumn('use_system_default_presentation_in_meeting'); - $table->dropColumn('use_system_default_presentation_as_default'); + $table->dropColumn([ + 'use_system_default_presentation_in_meeting', + 'use_system_default_presentation_as_default', + ]); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@database/migrations/2026_01_19_105554_add_use_system_default_presentation_in_next_meeting_to_rooms_table.php` around lines 34 - 37, In the migration's down/alter block where Schema::table(...) uses a Blueprint $table, replace the two separate $table->dropColumn('use_system_default_presentation_in_meeting') and $table->dropColumn('use_system_default_presentation_as_default') calls with a single $table->dropColumn([...]) call that passes both column names as an array; update the Schema::table callback (the function (Blueprint $table) block) to call dropColumn once with the array of the two column names.resources/js/components/RoomTabFilesSystemDefault.vue (1)
28-44:<p>used as a flex layout container is semantically incorrect.A
<p>element is phrasing content and shouldn't serve as a flex column wrapper. While PrimeVue'sTagrenders a<span>(so it's technically not invalid), the intent here is clearly layout, not a paragraph.♻️ Proposed fix
- <p class="m-0 flex flex-col gap-2 text-sm"> + <div class="m-0 flex flex-col gap-2 text-sm"> <Tag v-if="useInMeeting" severity="success" :value="$t('rooms.files.always_available_in_meeting')" /> <Tag v-else-if="defaultFile === null" severity="success" :value="$t('rooms.files.available_in_next_meeting')" /> <Tag v-else severity="secondary" :value="$t('rooms.files.not_available_in_next_meeting')" /> - </p> + </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/components/RoomTabFilesSystemDefault.vue` around lines 28 - 44, Replace the semantic misuse of the <p> element used as a flex container in RoomTabFilesSystemDefault.vue: change the paragraph wrapper that contains the Tag components (the element with classes "m-0 flex flex-col gap-2 text-sm") to a neutral block element such as a <div> (or <section>) so layout is preserved but semantics are correct; keep the existing classes and the Tag usage (useInMeeting, defaultFile checks) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Http/Controllers/api/v1/RoomFileController.php`:
- Around line 144-169: The updateSystemDefault method currently reads booleans
directly from $request and can leave an inconsistent state when use_in_meeting
is set to false without default being provided; change all raw accesses
($request->use_in_meeting and $request->default) to use
$request->boolean('use_in_meeting') and $request->boolean('default') for
consistent boolean coercion, and when you detect use_in_meeting is explicitly
false while the request did not include an explicit default=false, also clear
use_system_default_presentation_as_default (set it to false) so the room cannot
have a default presentation that is not included in the meeting; keep the
existing behavior in the block where default === true (files()->update(...) and
setting use_system_default_presentation_in_meeting = true) and then save and
call updateDefaultFile() as before.
In
`@database/migrations/2026_01_19_105554_add_use_system_default_presentation_in_next_meeting_to_rooms_table.php`:
- Around line 20-25: Replace the per-model loop that loads Room models via
RoomFile::each (the RoomFile model and its $file->room access) with a single raw
DB update to avoid N+1 and model observer side-effects: compute the set of room
IDs that have a default, use_in_meeting RoomFile and then run a
DB::table('rooms')->whereIn('id',
$ids)->update(['use_system_default_presentation_as_default' => false]); also add
use Illuminate\Support\Facades\DB; to the file header; do not call Room::save()
or rely on Room observers in the migration.
---
Outside diff comments:
In `@app/Models/Room.php`:
- Around line 207-223: updateDefaultFile() currently promotes the system default
when default_presentation && use_system_default_presentation_in_meeting without
respecting an explicit user opt-out; change the flow so updateDefaultFile() does
not override an explicit choice made by updateSystemDefault(): either (A) have
the controller that handles updateSystemDefault() skip calling
updateDefaultFile() when the payload contains default=false, or (B) add a
parameter/flag to updateDefaultFile($respectExplicitChoice = true) and, when
true, have updateDefaultFile() check for an explicit user decision (e.g. detect
if use_system_default_presentation_as_default was deliberately set to false in
the current request or if a dedicated flag was passed) before setting
use_system_default_presentation_as_default = true; ensure references to
updateDefaultFile(), updateSystemDefault(),
use_system_default_presentation_as_default,
use_system_default_presentation_in_meeting, and default_presentation are updated
accordingly.
---
Duplicate comments:
In `@resources/js/components/RoomTabFilesSystemDefault.vue`:
- Around line 90-97: In RoomTabFilesSystemDefault.vue update the Button element
so the static attribute autofocus appears before the event binding `@click`;
specifically, in the Button with :label="$t('app.cancel')" and
:disabled="isLoadingAction" move the autofocus token to precede
`@click`="modalVisible = false" (keeping all other props intact) to satisfy the
analyzer warning.
- Around line 159-180: The component is missing a disabled prop so parent
:disabled="isBusy" doesn't reach the buttons; add a disabled Boolean prop to the
defineProps (default false) and then apply it to the View and Configure buttons
by setting their disabled binding to combine the prop with the component's
internal loading state (e.g., :disabled="props.disabled || isLoadingAction") so
the buttons become non-interactive when the parent or the component is busy.
- Around line 51-59: The Button anchor in RoomTabFilesSystemDefault.vue
currently uses target="_blank" (the <Button ... as="a" target=\"_blank\"
:href=\"file\" ... /> instance) which is vulnerable to reverse-tabnabbing;
update that Button to include rel="noopener noreferrer" (i.e., add rel="noopener
noreferrer" to the Button props when as="a" and target="_blank") so the external
link opened in a new tab cannot access the opener.
---
Nitpick comments:
In
`@database/migrations/2026_01_19_105554_add_use_system_default_presentation_in_next_meeting_to_rooms_table.php`:
- Around line 34-37: In the migration's down/alter block where
Schema::table(...) uses a Blueprint $table, replace the two separate
$table->dropColumn('use_system_default_presentation_in_meeting') and
$table->dropColumn('use_system_default_presentation_as_default') calls with a
single $table->dropColumn([...]) call that passes both column names as an array;
update the Schema::table callback (the function (Blueprint $table) block) to
call dropColumn once with the array of the two column names.
In `@resources/js/components/RoomTabFilesSystemDefault.vue`:
- Around line 28-44: Replace the semantic misuse of the <p> element used as a
flex container in RoomTabFilesSystemDefault.vue: change the paragraph wrapper
that contains the Tag components (the element with classes "m-0 flex flex-col
gap-2 text-sm") to a neutral block element such as a <div> (or <section>) so
layout is preserved but semantics are correct; keep the existing classes and the
Tag usage (useInMeeting, defaultFile checks) unchanged.
Comment on lines
+144
to
+169
| public function updateSystemDefault(UpdateRoomSystemDefaultPresentation $request, Room $room) | ||
| { | ||
| if ($request->has('use_in_meeting')) { | ||
| $room->use_system_default_presentation_in_meeting = $request->use_in_meeting; | ||
| } | ||
|
|
||
| if ($request->has('default')) { | ||
| // Make other files not the default | ||
| if ($request->default === true) { | ||
| $room->files()->update(['default' => false]); | ||
|
|
||
| // If system default is set as default, it must also be used in the next meeting | ||
| $room->use_system_default_presentation_in_meeting = true; | ||
| } | ||
|
|
||
| $room->use_system_default_presentation_as_default = $request->default; | ||
| } | ||
|
|
||
| $room->save(); | ||
|
|
||
| Log::info('Changed system default presentation settings in room {room}', ['room' => $room->getLogLabel()]); | ||
|
|
||
| $room->updateDefaultFile(); | ||
|
|
||
| return response()->noContent(); | ||
| } |
Contributor
There was a problem hiding this comment.
Two issues in updateSystemDefault().
1. Inconsistent boolean access — Lines 147 and 159 use $request->use_in_meeting and $request->default (raw property access), while update() at lines 113/117 uses $request->boolean(...). Raw access can pass through strings like "1" or "0" rather than actual booleans. Use $request->boolean() for consistency.
2. Setting use_in_meeting = false doesn't clear as_default — If the user disables the system presentation for the meeting (use_in_meeting = false) without explicitly sending default = false, the room ends up with use_system_default_presentation_as_default = true while use_system_default_presentation_in_meeting = false. This is an inconsistent state — the presentation can't be the default if it's not included in the meeting.
Proposed fix
public function updateSystemDefault(UpdateRoomSystemDefaultPresentation $request, Room $room)
{
if ($request->has('use_in_meeting')) {
- $room->use_system_default_presentation_in_meeting = $request->use_in_meeting;
+ $room->use_system_default_presentation_in_meeting = $request->boolean('use_in_meeting');
+
+ // If system default is removed from the meeting, it cannot remain the default
+ if (!$request->boolean('use_in_meeting')) {
+ $room->use_system_default_presentation_as_default = false;
+ }
}
if ($request->has('default')) {
// Make other files not the default
- if ($request->default === true) {
+ if ($request->boolean('default')) {
$room->files()->update(['default' => false]);
// If system default is set as default, it must also be used in the next meeting
$room->use_system_default_presentation_in_meeting = true;
}
- $room->use_system_default_presentation_as_default = $request->default;
+ $room->use_system_default_presentation_as_default = $request->boolean('default');
}
$room->save();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Http/Controllers/api/v1/RoomFileController.php` around lines 144 - 169,
The updateSystemDefault method currently reads booleans directly from $request
and can leave an inconsistent state when use_in_meeting is set to false without
default being provided; change all raw accesses ($request->use_in_meeting and
$request->default) to use $request->boolean('use_in_meeting') and
$request->boolean('default') for consistent boolean coercion, and when you
detect use_in_meeting is explicitly false while the request did not include an
explicit default=false, also clear use_system_default_presentation_as_default
(set it to false) so the room cannot have a default presentation that is not
included in the meeting; keep the existing behavior in the block where default
=== true (files()->update(...) and setting
use_system_default_presentation_in_meeting = true) and then save and call
updateDefaultFile() as before.
Comment on lines
+20
to
+25
| RoomFile::where('default', true) | ||
| ->where('use_in_meeting', true) | ||
| ->each(function (RoomFile $file) { | ||
| $file->room->use_system_default_presentation_as_default = false; | ||
| $file->room->save(); | ||
| }); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Data migration triggers N+1 queries and risks model/observer side effects.
Two concerns with using Eloquent models here:
-
N+1 queries: Each iteration loads
$file->roomindividually. For a large number of rooms with default files, this could be slow. A single query would be more efficient. -
Model coupling: If
Room's observer (RoomObserver) performs side-effects on save (e.g., logging, cache invalidation), those will fire for every room during migration. Migrations should ideally be decoupled from application-layer concerns.
A raw DB update avoids both issues:
Proposed fix
- RoomFile::where('default', true)
- ->where('use_in_meeting', true)
- ->each(function (RoomFile $file) {
- $file->room->use_system_default_presentation_as_default = false;
- $file->room->save();
- });
+ DB::table('rooms')
+ ->whereIn('id', function ($query) {
+ $query->select('room_id')
+ ->from('room_files')
+ ->where('default', true)
+ ->where('use_in_meeting', true);
+ })
+ ->update(['use_system_default_presentation_as_default' => false]);You'll need to add use Illuminate\Support\Facades\DB; at the top.
📝 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
| RoomFile::where('default', true) | |
| ->where('use_in_meeting', true) | |
| ->each(function (RoomFile $file) { | |
| $file->room->use_system_default_presentation_as_default = false; | |
| $file->room->save(); | |
| }); | |
| DB::table('rooms') | |
| ->whereIn('id', function ($query) { | |
| $query->select('room_id') | |
| ->from('room_files') | |
| ->where('default', true) | |
| ->where('use_in_meeting', true); | |
| }) | |
| ->update(['use_system_default_presentation_as_default' => false]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@database/migrations/2026_01_19_105554_add_use_system_default_presentation_in_next_meeting_to_rooms_table.php`
around lines 20 - 25, Replace the per-model loop that loads Room models via
RoomFile::each (the RoomFile model and its $file->room access) with a single raw
DB update to avoid N+1 and model observer side-effects: compute the set of room
IDs that have a default, use_in_meeting RoomFile and then run a
DB::table('rooms')->whereIn('id',
$ids)->update(['use_system_default_presentation_as_default' => false]); also add
use Illuminate\Support\Facades\DB; to the file header; do not call Room::save()
or rely on Room observers in the migration.
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
2 participants
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.
Fixes #2662
Type
Checklist
Changes
Other information
Summary by CodeRabbit
New Features
UI/UX Improvements
Bug Fixes
Chores
Style