Repository navigation
[flutter_local_notifications_web] focus the tab on notification click + wait for SW activation + cache burst for SW file changes - #2820
Conversation
|
Hi @MaikuB, it would be nice to get this PR reviewed. It has minimal changes but I think these high impact for users. I am ready to do the changes requested to get this merged. Thank you in advance :) |
There was a problem hiding this comment.
🟡 Changes recommended
Address the two moderate service-worker reliability issues before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates web notification handling to focus existing tabs, wait for service-worker activation, and bust cached worker scripts.
Changes:
- Focuses an open tab when a notification is clicked.
- Waits up to 10 seconds for service-worker activation.
- Adds a version query parameter to the worker URL.
File summaries
| File | Summary | Findings |
|---|---|---|
flutter_local_notifications_web/web/notifications_service_worker.js |
Focuses the notification client before sending the message. | Await client.focus() and include the full operation in event.waitUntil() (moderate, 3 votes). |
flutter_local_notifications_web/lib/src/plugin.dart |
Adds activation waiting and cache busting. | Register the message listener before waiting for activation (moderate, 2 votes). Correct “burst cache” to “cache bust” (nit, 1 vote). |
Review details
Suppressed comments (1)
flutter_local_notifications_web/lib/src/plugin.dart:157
- The standard term is “cache bust”; “burst cache” is a typo in this newly added comment.
// Add version query parameter to the service worker file to burst cache
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| completeIfActivated(); | ||
| }).toJS; | ||
| completeIfActivated(); | ||
| return completer.future.timeout(const Duration(seconds: 10)); |
There was a problem hiding this comment.
What's the reason for 10 seconds? Looks like a magic number
There was a problem hiding this comment.
sorry for the super late reply, yes it's arbitrary but I set it so even devices with poor internet connection can run without throwing.
For reference, flutter service worker sets 4 seconds for a similar case:
https://github.com/flutter/flutter/blob/9911f49fc707c782835c41bd7945cdf23ede2598/engine/src/flutter/lib/web_ui/flutter_js/src/service_worker_loader.js#L85
I'm open for suggestions as well.
ec608c8 to
00165f0
Compare
Uh oh!
There was an error while loading. Please reload this page.