Stop auto-reload if room guest access is denied #2588
Conversation
WalkthroughPrevent duplicate auto-refresh timers in RoomsView, stop auto-reload when guests lack access, resume auto-reload after successful reloads, add changelog entry documenting disabled auto-reload for guests, and add two e2e tests covering auto-reload behavior and error handling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #2588 +/- ##
=============================================
- Coverage 96.75% 96.24% -0.51%
Complexity 1816 1816
=============================================
Files 434 257 -177
Lines 12483 6213 -6270
Branches 2078 0 -2078
=============================================
- Hits 12078 5980 -6098
+ Misses 405 233 -172 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
PILOS
|
||||||||||||||||||||||||||||
| Project |
PILOS
|
| Branch Review |
disable-room-reload-on-guest-access-forbidden
|
| Run status |
|
| Run duration | 07m 40s |
| Commit |
|
| Committer | Samuel Weirich |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
610
|
| View all changes introduced in this branch ↗︎ | |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(2 hunks)resources/js/views/RoomsView.vue(3 hunks)
🔇 Additional comments (3)
CHANGELOG.md (1)
10-13: LGTM!The changelog entry accurately documents the change and follows the established format conventions.
Also applies to: 592-592
resources/js/views/RoomsView.vue (2)
311-318: Good guard to prevent duplicate intervals, but needs companion fix.The null check prevents multiple intervals from being created simultaneously. However, this guard relies on
reloadInterval.valuebeing explicitly set tonullafterclearInterval()is called elsewhere in the code, sinceclearInterval()only stops the interval but doesn't reset the variable.See the critical issue flagged below in
handleGuestsNotAllowed()andhandleInvalidToken().
475-475: Correct logic to resume auto-refresh after successful reload.Calling
startAutoRefresh()here ensures that auto-refresh resumes when a previously denied guest gains access (e.g., after room settings change or user logs in). This completes the intended flow of disabling auto-refresh on denial and re-enabling on recovery.Note: This logic depends on the critical fix above—
reloadInterval.valuemust be set tonullafterclearInterval()for this to work correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/Frontend/e2e/RoomsViewGeneral.cy.js (1)
1826-1852: LGTM! Test correctly validates disabled auto-reload on error.The test properly verifies that auto-reload does not occur after a
guests_not_allowederror by asserting only one room request is made despite ticking past the refresh interval.Consider adding a test case that verifies auto-reload resumes after a successful manual reload following a
guests_not_allowederror. This would validate the complete lifecycle:it("auto-reload resumes after error recovery", function () { cy.intercept("GET", "api/v1/currentUser", {}); cy.fixture("config.json").then((config) => { config.data.room.refresh_rate = 60; cy.intercept("GET", "api/v1/config", { statusCode: 200, body: config, }); }); // Initial request returns 403 cy.intercept("GET", "api/v1/rooms/abc-def-123", { statusCode: 403, body: { message: "guests_not_allowed" }, }).as("roomRequest"); cy.clock(); cy.visit("/rooms/abc-def-123"); cy.wait("@roomRequest"); // Tick forward - no auto-reload should occur cy.tick(100000); cy.get("@roomRequest.all").should("have.length", 1); // Now return success for subsequent requests cy.fixture("room.json").then((room) => { cy.intercept("GET", "api/v1/rooms/abc-def-123", { statusCode: 200, body: room, }).as("roomRequest"); }); // Manual reload cy.get('[data-test="reload-room-button"]').click(); cy.wait("@roomRequest"); // Verify auto-reload resumes cy.tick(100000); cy.get("@roomRequest.all").should("have.length", 3); // initial 403 + manual + auto });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
resources/js/views/RoomsView.vue(4 hunks)tests/Frontend/e2e/RoomsViewGeneral.cy.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Docker Build
- GitHub Check: Backend
🔇 Additional comments (5)
tests/Frontend/e2e/RoomsViewGeneral.cy.js (1)
1797-1824: LGTM! Auto-reload test validates expected behavior.The test correctly configures a 60-second refresh rate and uses
cy.clock()to verify that auto-reload triggers after the interval. The 100-second tick provides sufficient margin to account for the ±15% randomness in the refresh interval.resources/js/views/RoomsView.vue (4)
311-318: LGTM! Guard prevents duplicate auto-refresh intervals.The guard
if (reloadInterval.value === null)ensures that multiple calls tostartAutoRefresh()won't create duplicate intervals. This works correctly with the cleanup in error handlers and allows auto-refresh to resume after the interval is cleared and nulled.
345-350: Excellent! Past review comment addressed.The addition of
reloadInterval.value = nullafterclearInterval()correctly resets the interval state. This ensures:
- The guard in
startAutoRefresh()(line 312) will allow a new interval when access is restored- Auto-refresh remains disabled until an explicit successful reload occurs
368-375: Excellent! Past review comment addressed for token errors.The addition of
reloadInterval.value = nullafterclearInterval()inhandleInvalidToken()ensures consistent cleanup behavior withhandleGuestsNotAllowed(). Since invalid token errors are permanent (the token cannot be restored), disabling auto-refresh is the correct behavior.
451-484: LGTM! Auto-refresh correctly resumes after successful reload.Calling
startAutoRefresh()on line 477 after a successful reload ensures that:
- Auto-refresh resumes after room access is restored (e.g., settings changed, user logged in)
- The guard on line 312 prevents duplicate intervals if auto-refresh is already running
- The reload cycle continues for normal operation
This completes the intended behavior: auto-refresh is disabled when guests are denied access and resumes when access is restored.
Checklist
Changes
Other information
The old behaviour didn't offer much of an advantage, as reloading probably won't make a difference. It would only have an effect if the room settings were changed. The major disadvantage of the old approach was the high number of failed requests, which spammed the logs and increased the error count metrics.
Summary by CodeRabbit
Changed
Bug Fixes
Tests