Skip to content

Conversation

axelvyrn
Copy link
Contributor

Description

Closes #2438
Introduces logic to suppress push notifications for users based on their satsFilter setting, preventing notifications for items below 0. The satsFilter is checked centrally under sendUserNotification since all notifications pass through it.

Screenshots

N/A

Additional Context

The working should be checked on phone, since I couldn't check it.
I could only provide a logical fix and tested through PC (windows).

Checklist

Are your changes backward compatible? Please answer below:
yup

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:
8 (because did test only on PC and not phone)

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:
none

Did you use AI for this? If so, how much did it assist you?

10% - Copilot helped me identify the possible files where the bug for this error might be since I am not still accustomed to the codebase. I filtered the files after that. No AI was used for coding purposes.

@axelvyrn axelvyrn marked this pull request as ready for review September 14, 2025 07:08
@axelvyrn
Copy link
Contributor Author

@ekzyis hope there are no conflicts now.

@axelvyrn
Copy link
Contributor Author

I have a bad feeling about the notifyThreadSubscribers and notifyItemParents functioning though

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.

#2442 (review)

requesting changes for the 7th time

I have a bad feeling about the notifyThreadSubscribers and notifyItemParents functioning though

Can you be more precise? If you tested it, why do you have a bad feeling that they aren't functioning?

@axelvyrn
Copy link
Contributor Author

Can you be more precise? If you tested it, why do you have a bad feeling that they aren't functioning?

Because testing on development mode often gives false negatives/positives. On dev, process.env.NODE_ENV === 'development', and unless I've set the VAPID keys, push notifications might not actually be sent (which actually is complicated for me since I've never worked with keys or CORS stuff)

Even if they are sent (like via localhost), the “/notifications” page may behave differently because it might be populated via API without filtering, or the dev DB might have different msats/cost values. So only you can test it properly on a production build.

@Soxasora
Copy link
Member

Soxasora commented Sep 14, 2025

and unless I've set the VAPID keys, push notifications might not actually be sent (which actually is complicated for me since I've never worked with keys or CORS stuff)

You can obtain VAPID keys and enable true push notifications by following our README section on it. Even without this, you can place a console.log in the appropriate section in webPush.js and watch the worker container logs.

the “/notifications” page may behave differently because it might be populated via API without filtering

This issue is all about the fact that the /notifications resolver has the investmentClause that filters out posts that are worth less than a user-set amount of sats (among other things), while webPush.js doesn't.

or the dev DB might have different msats/cost values.

The dev DB seed is only a starter point, you can still create posts with 2 accounts and test this out.


I'm sure you understand the implications of pushing untested code into production, I'm sure you wouldn't do that to yourself either. Fortunately we can spot stuff, but you just lose time, rep, and, honestly, money. You reached a 70% bounty reduction this way, other than giving us the feeling that you're not doing anything of value.

Look at rule 3 of pull request awards

If you can't test something, don't rush into review, search or think about how you can test stuff and if you can't, as a last resort, ping us, we can reply just like we are doing now.

@axelvyrn
Copy link
Contributor Author

axelvyrn commented Sep 14, 2025

The dev DB seed is only a starter point, you can still create posts with 2 accounts and test this out.

That is what I have been doing.

I'm sure you understand the implications of pushing untested code into production, I'm sure you wouldn't do that to yourself either. Fortunately we can spot stuff, but you just lose time, rep, and, honestly, money. You reached a 70% bounty reduction this way, other than giving us the feeling that you're not doing anything of value.

I understand and am sorry, and I'm not worried about the bounty though. I thought I knew the solution so I tried it out. Seems like I was wrong because the way I wanted to solve it is not required. I'll try one last time.

@axelvyrn axelvyrn marked this pull request as draft September 20, 2025 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

push notifications don't respect sats filter
3 participants