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

feat: add push notifications #163

Open
wants to merge 42 commits into
base: master
Choose a base branch
from
Open

Conversation

reneaaron
Copy link
Contributor

@reneaaron reneaaron commented Oct 22, 2024

Add a todo about migrating wallet connections using 0.0 version to 1.0

NIP-44 decryption

Other TODOs

Description

  • Adds notification service extension for iOS
  • Overrides onMessageReceived function in Android

Testing

  • Replace NOSTR_API_URL to local http-nostr url

  • Run feat: add endpoint for registering alby go notifications http-nostr#128 and use ngrok http 8888

  • Copy the ngrok link to NOSTR_API_URL in constants

  • If running on android, download google-services.json from firebase console and add it to the root of your directory and set this in your .env.local: GOOGLE_SERVICES_JSON=./google-services.json

  • Connect your device via cable and run yarn install and then npx expo run:ios --device / npx expo run:android --device

  • Enable notifications in settings and receive a transaction to get notified (tapping on the notification also switches to the right wallet and takes you to the transaction screen)

@reneaaron reneaaron force-pushed the feat/push-notifications branch from 0289ded to 1c6e544 Compare October 31, 2024 09:38
@reneaaron reneaaron added this to the 1.8 milestone Nov 4, 2024
@im-adithya im-adithya marked this pull request as ready for review January 17, 2025 05:30
lib/walletInfo.ts Outdated Show resolved Hide resolved
@@ -67,6 +69,15 @@ export default function RootLayout() {
}
}

async function checkAndPromptForNotifications() {
const isEnabled = useAppStore.getState().isNotificationsEnabled;
// prompt the user to enable notifications on first open
Copy link
Contributor

Choose a reason for hiding this comment

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

is this good UX? CC @reneaaron

assets/MessagingService.kt Outdated Show resolved Hide resolved
lib/link.ts Outdated Show resolved Hide resolved
@rolznz
Copy link
Contributor

rolznz commented Jan 17, 2025

Could you update the README to instruct devs how to run with expo go (notifications disabled) or natively (with notifications enabled)

services/Notifications.ts Outdated Show resolved Hide resolved
@im-adithya
Copy link
Member

im-adithya commented Jan 21, 2025

Could you update the README to instruct devs how to run with expo go (notifications disabled) or natively (with notifications enabled)

Done! Let me know if there are any changes.

Luckily we have ExecutionEnvironment constant, so we can automatically detect if the user is running it in Expo Go and disable notifications.

if (!response.ok) {
errorToast(
new Error("Failed to deregister push notification subscription"),
);
Copy link
Member

Choose a reason for hiding this comment

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

Currently if this fails for some reason, then we might have a weird case where some wallets have notifications and some don't, and if they toggle it again (because we do toggle it off even if this fails), it might make it even messier because they will receive 2 notifications since the last one is also active

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@im-adithya im-adithya Jan 21, 2025

Choose a reason for hiding this comment

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

I'm thinking of a UI where there can be all wallets with sub-toggles under the main allow notifications toggle, and if for some reason a wallet fails to deregister, it's toggle will still be on for the user to try again, wdyt?

We can do this as a follow up since http-nostr can be considered quite reliable since here it's only http and not relays

Currently it looks like this:

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.

3 participants