Skip to content
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

fix: iOS notification counts and sums are wacky #1796

Merged

Conversation

Soxasora
Copy link
Member

@Soxasora Soxasora commented Jan 4, 2025

Description

Partial fix of #756, 2: (notification counts and sums)
This PR provides immediate relief for iOS users, specifically fixes iOS check to enable ek's fix (initialValue = 1) giving proper counting and sums on all notifications

os value is not updated between events, this fix reliably saves and gets the OS value to/from ServiceWorkerStorage

Screenshots

AMOUNT counts
image

SUM_SATS sums
IMG_1491

Additional Context

This is part of a series of fixes tracked on #1794

Checklist

Are your changes backwards compatible? Please answer below:
Yes

On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:
9, didn't test FOLLOW but it follows (heh) the same counting method as others, tested also with withdraw/deposit sums

For frontend changes: Tested on mobile, light and dark mode? Please answer below:
n/a

Did you introduce any new environment variables? If so, call them out explicitly here:
No

@Soxasora Soxasora changed the title fix: notification counts are wacky fix: iOS notification counts are wacky Jan 4, 2025
@Soxasora Soxasora added the bug label Jan 4, 2025
@ekzyis ekzyis self-requested a review January 4, 2025 21:47
@Soxasora Soxasora changed the title fix: iOS notification counts are wacky fix: iOS notification counts and sums are wacky Jan 4, 2025
Copy link
Member

@ekzyis ekzyis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, incredible, we were apparently so close but I didn't realize my naive code to detect iOS in the service worker didn't work.

Reproduced the bug on master (34175ba) with replies and then checked out this branch. Replies don't go 1->2->4 anymore but go 1->2->3. If a reply notification is closed, the next notification will show one less (as expected).

I think the UX is fine since the latest notification now always shows the correct amount of "unread notifications".

The correct sum for "your item stacked X sats" is also very useful. So I suspect we will keep the merging code on iOS but let's see in prod.

Thank you so much again for this simple fix!

Comment on lines -258 to +264
os = event.data.os
event.waitUntil(storage.setItem('os', event.data.os))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, did simply assigning it to os not work because the service worker does not run in a single scope?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You guessed it! But only in iOS (drove me nuts for a whole day)

@Soxasora
Copy link
Member Author

Soxasora commented Jan 4, 2025

Thanks a lot!! I'm trying new strategies for the other issues, I look forward to try and complete what remains ^^

@huumn huumn merged commit a04647d into stackernews:master Jan 5, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants