fix: restrict service worker scope to Mainsail routes - #2524
Conversation
cf3302d to
ded7780
Compare
jothan
changed the title
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe Vite PWA Workbox configuration in ChangesPWA Service Worker Configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
vite.config.ts (2)
53-55: 💤 Low valueConsider adding a comment to document the allowlisted routes.
The regex pattern correctly restricts the service worker's navigation fallback to specific Mainsail routes, but a brief comment explaining that these are the app's client-side routes would improve maintainability.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vite.config.ts` around lines 53 - 55, The navigateFallbackAllowlist configuration lacks documentation explaining its purpose and the routes it covers. Add a brief comment above the navigateFallbackAllowlist property (or on the same line) to document that the regex pattern restricts the service worker's navigation fallback to specific Mainsail client-side routes such as allPrinters, cam, console, heightmap, files, viewer, history, timelapse, config, and settings, along with their sub-routes. This will improve maintainability and help future developers understand why these specific routes are allowlisted.
66-73: Replacenew Function()with a standard arrow function for better type safety and adherence to documented patterns.The
new Function()withas anytype assertion is not the documented pattern for vite-plugin-pwa. Official Workbox and vite-plugin-pwa documentation uses a standard arrow function:plugins: [ { cacheWillUpdate: async ({ response }) => { return response && !response.redirected ? response : null }, }, ],This approach provides proper TypeScript typing, improves readability, and follows the pattern used in all official examples.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vite.config.ts` around lines 66 - 73, The cacheWillUpdate property in the plugins configuration is using the new Function() constructor with an as any type assertion instead of the documented pattern, which reduces type safety and readability. Replace this with a standard async arrow function that directly takes the destructured response parameter and returns the conditional result. This will provide proper TypeScript typing without type assertions and follow the official vite-plugin-pwa and Workbox documentation patterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@vite.config.ts`:
- Around line 53-55: The navigateFallbackAllowlist configuration lacks
documentation explaining its purpose and the routes it covers. Add a brief
comment above the navigateFallbackAllowlist property (or on the same line) to
document that the regex pattern restricts the service worker's navigation
fallback to specific Mainsail client-side routes such as allPrinters, cam,
console, heightmap, files, viewer, history, timelapse, config, and settings,
along with their sub-routes. This will improve maintainability and help future
developers understand why these specific routes are allowlisted.
- Around line 66-73: The cacheWillUpdate property in the plugins configuration
is using the new Function() constructor with an as any type assertion instead of
the documented pattern, which reduces type safety and readability. Replace this
with a standard async arrow function that directly takes the destructured
response parameter and returns the conditional result. This will provide proper
TypeScript typing without type assertions and follow the official
vite-plugin-pwa and Workbox documentation patterns.
1 participant
Description
This patch makes sure the service worker only handles requests directed towards Mainsail.
Core issue:
Without being logged in, my authelia setup redirects the Mainsail URL to the login page:
https://kepler.x2a.org/ 302 found -> https://kepler.x2a.org/authelia?rd=https%3A%2F%2Fkepler.x2a.org%2F
The service worker would cache the login page with the root URL as its path. After login, being redirected back to the root url I would get a grey Authelia page coming out of the Mainsail service worker instead of getting the Mainsail UI.
The approach taken is two-pronged:
Starting from an empty / clean state, I have manually confirmed that redirected URLs are not cached by the WebWorker. It might be required to manually clear caches after applying this patch to get the full benefits. My phone browser ended up in this weird state and through remote chrome debugging, I was able to finally confirm that I was hitting an old cache (this is why this PR stayed a draft for a couple of days).
Related Tickets & Documents
None found
Mobile & Desktop Screenshots/Recordings
Not a visual change.
[optional] Are there any post-deployment tasks we need to perform?
🤖 This Pull Request was created with the help of brain.