-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[MM-66711] Verify push notifications on magic link login #9322
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Adds push notification capability verification for magic link login flow to match the behavior of normal login. This ensures push notifications are properly enabled after authentication via magic link, similar to how they're verified during the standard add-a-server flow.
- Added push notification check after successful magic link authentication
- Calls
doPingto verify server push notification capabilities - Shows push notification prompts if needed using the user's locale
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
app/actions/remote/session.ts
Outdated
| client.setCSRFToken(csrfToken); | ||
|
|
||
| // Check push notification capability (similar to normal login flow) | ||
| const pingResult = await doPing(serverUrlToUse, true, undefined, undefined, client); |
Copilot
AI
Nov 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The multiple undefined parameters make the function call unclear. Consider using named parameters or an options object for doPing, or add comments explaining what each parameter represents.
| const pingResult = await doPing(serverUrlToUse, true, undefined, undefined, client); | |
| const pingResult = await doPing( | |
| serverUrlToUse, // serverUrl | |
| true, // isMobile | |
| undefined, // token | |
| undefined, // deviceId | |
| client, // client | |
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Coverage Comparison ReportGenerated on December 12, 2025 at 16:12:44 UTC |
|
@larkox I'm having trouble with this one, on both iOS and Android. Here's what I'm doing:
Observed: A brief Mattermost logo loading screen, but then it disappears and nothing else loads on the screen. It is completely blank. |
|
@lindalumitchell I added something that was missing to the PR to actually work, but it is completely unrelated to the issue you were facing :( |
Summary
When we login normally, we check the push notifications during the add a server flow.
On magic link, we are omitting that step, so we need to do the check afterwards.
Ticket Link
Fix https://mattermost.atlassian.net/browse/MM-66711
Release Note
No release note needed since it will be out in the same release as the feature