Open
Conversation
# Conflicts: # lang/en/app.php
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 19 minutes and 0 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds structured 404 handling: backend returns Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
PILOS
|
||||||||||||||||||||||||||||
| Project |
PILOS
|
| Branch Review |
86-improve-notfoundexceptions
|
| Run status |
|
| Run duration | 07m 27s |
| Commit |
|
| Committer | Sabrina Wüst |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
623
|
| View all changes introduced in this branch ↗︎ | |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3036 +/- ##
=============================================
+ Coverage 96.70% 96.77% +0.06%
- Complexity 1913 1923 +10
=============================================
Files 445 458 +13
Lines 12915 13070 +155
Branches 2078 2123 +45
=============================================
+ Hits 12490 12648 +158
+ Misses 425 422 -3 ☔ 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: 8
🧹 Nitpick comments (4)
resources/js/views/RoomsView.vue (1)
442-448: Userouter.replace()for terminal not-found redirects.Keeping
push()here leaves the broken room URL in browser history, so Back can reopen the same missing-room redirect loop.replace()avoids that while preserving the new auth-vs-guest behavior.♻️ Proposed change
- router.push({ name: "rooms.index" }); + router.replace({ name: "rooms.index" }); ... - router.push({ name: "404" }); + router.replace({ name: "404" });Also applies to: 524-530
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/views/RoomsView.vue` around lines 442 - 448, The 404 handling in RoomsView.vue currently uses router.push which leaves the broken room URL in history; replace router.push calls with router.replace in the error.response.status === env.HTTP_NOT_FOUND block (the branch that routes to "rooms.index" or "404") so the missing-room redirect is terminal, and make the same change for the second identical block around the other not-found handler (the other router.push at lines ~524-530) to avoid back-button loops.tests/Backend/Feature/api/v1/Room/MembershipTest.php (1)
785-791: Consider a shared assertion helper formodel_not_foundpayloads.These repeated JSON checks are consistent, but a tiny helper (e.g.,
assertModelNotFound($response, $model, $ids)) would reduce duplication and keep future contract changes centralized.Also applies to: 801-807, 848-854, 888-894
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Backend/Feature/api/v1/Room/MembershipTest.php` around lines 785 - 791, Extract the repeated JSON assertions for the "model_not_found" payload into a single test helper (e.g., assertModelNotFound($response, string $model, array $ids)) on the base TestCase (or a shared trait) and replace the inline ->assertNotFound()->assertJson([...]) chains in MembershipTest with the new helper; the helper should accept the response (or perform the status check itself), assert the 404 via assertNotFound() and assert the JSON body contains 'message' => 'model_not_found', 'model' => $model, and 'ids' => $ids so all occurrences (the deleteJson route('api.v1.rooms.member.destroy', ...) calls and similar assertions) use this centralized assertion.tests/Frontend/e2e/RoomsViewMembers.cy.js (1)
488-505: Register the 404 intercept before reload to avoid a flaky branch.
cy.reload()currently runs before the newroomMembersRequest404 intercept is installed. If the members request is triggered during reload, this scenario may miss the intended stub and become nondeterministic.🔧 Suggested stabilization
- cy.interceptRoomViewRequests(); - cy.reload(); - - // Test 404 error (room not found) - cy.interceptRoomIndexRequests(); - cy.intercept("GET", "api/v1/rooms/abc-def-123/member*", { + cy.interceptRoomViewRequests(); + cy.interceptRoomIndexRequests(); + // Test 404 error (room not found) + cy.intercept("GET", "api/v1/rooms/abc-def-123/member*", { statusCode: 404, body: { message: "model_not_found", model: "room", ids: ["abc-def-123"], }, }).as("roomMembersRequest"); + cy.reload(); + cy.wait("@roomRequest"); cy.get("#tab-members").click();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Frontend/e2e/RoomsViewMembers.cy.js` around lines 488 - 505, The 404 stub for the members request is installed after cy.reload(), making the test flaky if the members request fires during reload; move the cy.intercept("GET", "api/v1/rooms/abc-def-123/member*", { ... }).as("roomMembersRequest") call to run before cy.reload() (while keeping cy.interceptRoomViewRequests() / cy.interceptRoomIndexRequests() as needed) so the "@roomMembersRequest" stub is active during the page reload and the subsequent cy.get("#tab-members").click() step.resources/js/services/Api.js (1)
128-135: Preferreplace()for these 404 redirects.Using
push()keeps the stale room URL in history, so Back lands on the broken page and immediately redirects again.replace()avoids that loop and matches the existing unauthenticated/guests-only handlers.Suggested change
if (model === ROOM && options.redirectOnRoomModelNotFound !== false) { // Redirect to room index page if user is authenticated, otherwise show 404 page, because // unauthenticated user is not able to visit the room index page if (this.auth.isAuthenticated) { - this.router.push({ name: "rooms.index" }); + this.router.replace({ name: "rooms.index" }); } else { - this.router.push({ name: "404" }); + this.router.replace({ name: "404" }); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/services/Api.js` around lines 128 - 135, The redirect logic that handles model === ROOM when options.redirectOnRoomModelNotFound !== false should use this.router.replace instead of this.router.push so the stale room URL is not kept in history; update the branches that check this.auth.isAuthenticated and call this.router.push({ name: "rooms.index" }) or this.router.push({ name: "404" }) to call this.router.replace(...) with the same route objects to avoid back-button redirect loops and match guest/unauthenticated handlers.
🤖 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 69-72: The translation string for flash.model_not_found.details
contains a leading space; open the array entry for
'flash.model_not_found.details' (the 'details' key under 'model_not_found') and
remove the leading whitespace so the value reads 'Model id: :ids' instead of '
Model id: :ids' to ensure consistent UI text formatting.
In `@resources/js/components/SettingsUsersDeleteButton.vue`:
- Around line 109-114: The 404 case is being double-handled: after emitting
notFound in the catch block (when error.response.status === env.HTTP_NOT_FOUND)
the code still calls api.error(error). Modify the catch logic in the delete
flow/handler (the block using modalVisible.value, emit("notFound") and
api.error) so that once you emit("notFound") and set modalVisible.value = false
you skip calling api.error—either by returning early immediately after
emit("notFound") or by wrapping api.error(error) in an else branch that only
runs when the status is not env.HTTP_NOT_FOUND.
In `@tests/Frontend/e2e/RoomsViewDescription.cy.js`:
- Around line 466-483: The test clicks the edit button immediately after
cy.reload(), which can race with the room view request; update the test to wait
for the relevant intercepted room request to finish before reopening the editor
by awaiting the alias created by cy.interceptRoomViewRequests() or
cy.interceptRoomIndexRequests() (e.g., cy.wait('@roomRequest') or the actual
alias returned by those helpers) before calling
cy.get('[data-test="room-description-edit-button"]').click() so the reload
completes and the 404 save path is exercised reliably.
In `@tests/Frontend/e2e/RoomsViewGeneral.cy.js`:
- Line 727: Fix the typo in the inline comment that currently reads "// Check
wit 404 error (room not found) as guest" by changing "wit" to "with" so it reads
"// Check with 404 error (room not found) as guest"; this update should be made
in the RoomsViewGeneral.cy.js test comment near the guest 404 check.
In `@tests/Frontend/e2e/RoomsViewMembersBulkActions.cy.js`:
- Around line 412-422: The test races selecting members with the members grid
reload; after calling cy.interceptRoomViewRequests() and after clicking
'#tab-members' (or immediately after cy.reload()/reopening the tab), wait for
the intercepted members request alias '@roomMembersRequest' before interacting
with '[data-test="room-members-select-all-checkbox"]'; update the three
occurrences (around cy.interceptRoomViewRequests(), cy.reload(),
cy.get("#tab-members").click(), and the select-all click) to include a
cy.wait("@roomMembersRequest") so the members grid is fully loaded before
selection.
In `@tests/Frontend/e2e/RoomsViewRecordingsRecordingActions.cy.js`:
- Around line 970-1001: After reloading and clicking the recordings tab you must
wait for the refreshed recordings request before opening the delete dialog to
avoid the race; after invoking cy.reload() and/or after
cy.get("#tab-recordings").click(), add a cy.wait for the recordings intercept
alias (the `@roomRecordingsRequest` created by cy.interceptRoomRecordingsRequests)
and only then proceed to find and click the delete button and confirm the dialog
so the subsequent 404 assertion targets the refreshed list.
In `@tests/Frontend/e2e/RoomsViewSettings.cy.js`:
- Around line 2372-2378: The current cy.url().should("include", "/rooms") can
match both /rooms and /rooms/abc-def-123; replace it with a stricter assertion
that the pathname is exactly the rooms index (e.g. use
cy.location('pathname').should('match', /^\/rooms\/?$/) or assert equality to
'/rooms') so the test fails if the browser is still on the room detail URL;
update the assertion that currently calls cy.url() to use
cy.location('pathname') and the regexp or exact equals check, leaving the
cy.checkToastMessage call unchanged.
- Around line 1691-1702: The PUT intercept created with cy.intercept("PUT",
"api/v1/rooms/abc-def-123", ...) is missing the alias expected by the later
cy.wait("@roomSettingsSaveRequest"); update the intercept call to attach the
alias by calling .as("roomSettingsSaveRequest") so the cy.wait can find the
request; you should modify the cy.intercept invocation in
RoomsViewSettings.cy.js to include .as("roomSettingsSaveRequest") to match the
existing cy.wait.
---
Nitpick comments:
In `@resources/js/services/Api.js`:
- Around line 128-135: The redirect logic that handles model === ROOM when
options.redirectOnRoomModelNotFound !== false should use this.router.replace
instead of this.router.push so the stale room URL is not kept in history; update
the branches that check this.auth.isAuthenticated and call this.router.push({
name: "rooms.index" }) or this.router.push({ name: "404" }) to call
this.router.replace(...) with the same route objects to avoid back-button
redirect loops and match guest/unauthenticated handlers.
In `@resources/js/views/RoomsView.vue`:
- Around line 442-448: The 404 handling in RoomsView.vue currently uses
router.push which leaves the broken room URL in history; replace router.push
calls with router.replace in the error.response.status === env.HTTP_NOT_FOUND
block (the branch that routes to "rooms.index" or "404") so the missing-room
redirect is terminal, and make the same change for the second identical block
around the other not-found handler (the other router.push at lines ~524-530) to
avoid back-button loops.
In `@tests/Backend/Feature/api/v1/Room/MembershipTest.php`:
- Around line 785-791: Extract the repeated JSON assertions for the
"model_not_found" payload into a single test helper (e.g.,
assertModelNotFound($response, string $model, array $ids)) on the base TestCase
(or a shared trait) and replace the inline ->assertNotFound()->assertJson([...])
chains in MembershipTest with the new helper; the helper should accept the
response (or perform the status check itself), assert the 404 via
assertNotFound() and assert the JSON body contains 'message' =>
'model_not_found', 'model' => $model, and 'ids' => $ids so all occurrences (the
deleteJson route('api.v1.rooms.member.destroy', ...) calls and similar
assertions) use this centralized assertion.
In `@tests/Frontend/e2e/RoomsViewMembers.cy.js`:
- Around line 488-505: The 404 stub for the members request is installed after
cy.reload(), making the test flaky if the members request fires during reload;
move the cy.intercept("GET", "api/v1/rooms/abc-def-123/member*", { ...
}).as("roomMembersRequest") call to run before cy.reload() (while keeping
cy.interceptRoomViewRequests() / cy.interceptRoomIndexRequests() as needed) so
the "@roomMembersRequest" stub is active during the page reload and the
subsequent cy.get("#tab-members").click() step.
🪄 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: 571f1968-c615-451e-b8a8-5e3dc5fa57af
📒 Files selected for processing (85)
app/Exceptions/Handler.phpapp/Http/Controllers/api/v1/RoomMemberController.phpapp/Http/Controllers/api/v1/RoomPersonalizedLinkController.phplang/en/app.phpresources/js/components/RoomCard.vueresources/js/components/RoomFavoriteButton.vueresources/js/components/RoomTabFilesDeleteButton.vueresources/js/components/RoomTabFilesEditButton.vueresources/js/components/RoomTabMembersDeleteButton.vueresources/js/components/RoomTabMembersEditButton.vueresources/js/components/RoomTabPersonalizedLinksDeleteButton.vueresources/js/components/RoomTabPersonalizedLinksEditButton.vueresources/js/components/RoomTabRecordingsDeleteButton.vueresources/js/components/RoomTabRecordingsEditButton.vueresources/js/components/SettingsRolesDeleteButton.vueresources/js/components/SettingsServerPoolsDeleteButton.vueresources/js/components/SettingsServersDeleteButton.vueresources/js/components/SettingsUsersDeleteButton.vueresources/js/components/SettingsUsersResetPasswordButton.vueresources/js/constants/modelNames.jsresources/js/services/Api.jsresources/js/views/AdminRolesIndex.vueresources/js/views/AdminRolesView.vueresources/js/views/AdminRoomTypesView.vueresources/js/views/AdminServerPoolsIndex.vueresources/js/views/AdminServerPoolsView.vueresources/js/views/AdminServersIndex.vueresources/js/views/AdminServersView.vueresources/js/views/AdminUsersIndex.vueresources/js/views/AdminUsersView.vueresources/js/views/RoomsView.vueroutes/api.phptests/Backend/Feature/api/v1/RecordingTest.phptests/Backend/Feature/api/v1/RoleTest.phptests/Backend/Feature/api/v1/Room/FileTest.phptests/Backend/Feature/api/v1/Room/MembershipTest.phptests/Backend/Feature/api/v1/Room/RoomDescriptionTest.phptests/Backend/Feature/api/v1/Room/RoomPersonalizedLinkTest.phptests/Backend/Feature/api/v1/Room/RoomTest.phptests/Backend/Feature/api/v1/RoomTypeTest.phptests/Backend/Feature/api/v1/ServerPoolTest.phptests/Backend/Feature/api/v1/ServerTest.phptests/Backend/Feature/api/v1/UserTest.phptests/Frontend/e2e/AdminRolesEdit.cy.jstests/Frontend/e2e/AdminRolesIndexRoleActions.cy.jstests/Frontend/e2e/AdminRolesView.cy.jstests/Frontend/e2e/AdminRolesViewRoleActions.cy.jstests/Frontend/e2e/AdminRoomTypesEdit.cy.jstests/Frontend/e2e/AdminRoomTypesIndexRoomTypeActions.cy.jstests/Frontend/e2e/AdminRoomTypesView.cy.jstests/Frontend/e2e/AdminRoomTypesViewRoomTypeActions.cy.jstests/Frontend/e2e/AdminServerPoolsEdit.cy.jstests/Frontend/e2e/AdminServerPoolsIndexServerPoolActions.cy.jstests/Frontend/e2e/AdminServerPoolsView.cy.jstests/Frontend/e2e/AdminServerPoolsViewServerPoolActions.cy.jstests/Frontend/e2e/AdminServersEdit.cy.jstests/Frontend/e2e/AdminServersIndexServerActions.cy.jstests/Frontend/e2e/AdminServersView.cy.jstests/Frontend/e2e/AdminServersViewServerActions.cy.jstests/Frontend/e2e/AdminUsersEdit.cy.jstests/Frontend/e2e/AdminUsersEditBase.cy.jstests/Frontend/e2e/AdminUsersEditEmail.cy.jstests/Frontend/e2e/AdminUsersEditOthers.cy.jstests/Frontend/e2e/AdminUsersEditSecurity.cy.jstests/Frontend/e2e/AdminUsersIndexUserActions.cy.jstests/Frontend/e2e/AdminUsersView.cy.jstests/Frontend/e2e/AdminUsersViewUserActions.cy.jstests/Frontend/e2e/RoomsIndex.cy.jstests/Frontend/e2e/RoomsViewDescription.cy.jstests/Frontend/e2e/RoomsViewFiles.cy.jstests/Frontend/e2e/RoomsViewFilesFileActions.cy.jstests/Frontend/e2e/RoomsViewGeneral.cy.jstests/Frontend/e2e/RoomsViewHistory.cy.jstests/Frontend/e2e/RoomsViewHistoryMeetingActions.cy.jstests/Frontend/e2e/RoomsViewMeetings.cy.jstests/Frontend/e2e/RoomsViewMembers.cy.jstests/Frontend/e2e/RoomsViewMembersBulkActions.cy.jstests/Frontend/e2e/RoomsViewMembersMemberActions.cy.jstests/Frontend/e2e/RoomsViewPersonalizedLinks.cy.jstests/Frontend/e2e/RoomsViewPersonalizedLinksTokenActions.cy.jstests/Frontend/e2e/RoomsViewRecordings.cy.jstests/Frontend/e2e/RoomsViewRecordingsRecordingActions.cy.jstests/Frontend/e2e/RoomsViewSettings.cy.jstests/Frontend/e2e/RoomsViewStreaming.cy.jstests/Frontend/e2e/RoomsViewStreamingConfigActions.cy.js
Comment on lines
+109
to
114
| if (error.response && error.response.status === env.HTTP_NOT_FOUND) { | ||
| modalVisible.value = false; | ||
| emit("notFound"); | ||
| } | ||
| api.error(error); | ||
| }) |
Contributor
There was a problem hiding this comment.
Stop double-handling 404 after emitting notFound.
On Line 109 onward, the code emits notFound but still calls api.error(error), which can cause duplicate/conflicting handling for the same response.
🔧 Proposed fix
import { ref } from "vue";
import { useApi } from "../composables/useApi.js";
import env from "../env.js";
+import { USER } from "../constants/modelNames.js";
@@
- if (error.response && error.response.status === env.HTTP_NOT_FOUND) {
+ if (
+ error.response?.status === env.HTTP_NOT_FOUND &&
+ error.response.data?.model === USER
+ ) {
modalVisible.value = false;
emit("notFound");
+ return;
}
api.error(error);📝 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
| if (error.response && error.response.status === env.HTTP_NOT_FOUND) { | |
| modalVisible.value = false; | |
| emit("notFound"); | |
| } | |
| api.error(error); | |
| }) | |
| if ( | |
| error.response?.status === env.HTTP_NOT_FOUND && | |
| error.response.data?.model === USER | |
| ) { | |
| modalVisible.value = false; | |
| emit("notFound"); | |
| return; | |
| } | |
| api.error(error); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@resources/js/components/SettingsUsersDeleteButton.vue` around lines 109 -
114, The 404 case is being double-handled: after emitting notFound in the catch
block (when error.response.status === env.HTTP_NOT_FOUND) the code still calls
api.error(error). Modify the catch logic in the delete flow/handler (the block
using modalVisible.value, emit("notFound") and api.error) so that once you
emit("notFound") and set modalVisible.value = false you skip calling
api.error—either by returning early immediately after emit("notFound") or by
wrapping api.error(error) in an else branch that only runs when the status is
not env.HTTP_NOT_FOUND.
Contributor
Author
There was a problem hiding this comment.
The notFound emit is used to inform the parent component that either the user list needs to be reloaded or a redirect to the user index page is required.
The api.error(error) call is responsible for displaying a toast message to inform the user that an error occurred.
Contributor
There was a problem hiding this comment.
@Sabr1n4W, thanks for the clarification — that makes the separation of concerns clear. The notFound emit handles the structural/navigation response in the parent, while api.error takes care of surfacing the toast notification independently. That's a reasonable design.
One minor thing still worth considering: the current condition fires emit("notFound") for any HTTP 404, not just when data.model === "user". If a 404 with a different model (e.g., from a race condition or an unrelated resource) were ever returned by this endpoint, it would still trigger the parent's reload/redirect. Adding the model discriminator (as suggested in the original fix) would make the intent more explicit and guard against that edge case — but I understand if you consider that overly defensive for this endpoint.
(o´ω`o)
✏️ Learnings added
Learnt from: Sabr1n4W
Repo: THM-Health/PILOS PR: 3036
File: resources/js/components/SettingsUsersDeleteButton.vue:109-114
Timestamp: 2026-04-10T14:09:05.618Z
Learning: In `resources/js/components/SettingsUsersDeleteButton.vue` (and similar Settings*DeleteButton components in the PILOS project), `emit("notFound")` and `api.error(error)` are intentionally called together in the 404 catch block. They serve different purposes: `emit("notFound")` informs the parent component to reload the list or redirect to the index page, while `api.error(error)` is responsible for displaying the toast message to the user. Both should run for a 404 — no early return is needed.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
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.
Fixes #86
Fixes #1676
Type
Checklist
Changes
Other information
Summary by CodeRabbit
New Features
Bug Fixes
Improvements