Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

feat(coderd): add webpush package#17091

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
johnstcn merged 40 commits intomainfromcj/push-notifications-2-rebase
Mar 27, 2025
Merged

Conversation

johnstcn
Copy link
Member

@johnstcnjohnstcn commentedMar 25, 2025•
edited
Loading

Extracted fromhttps://github.com/coder/coder/tree/kyle/tasks

This PR adds awebpush package undercoderd to allow sending push notifications to browsers from coderd using Web Push andVAPID.

Why VAPID? Why not just dial the agent directly?

  • Most browsers support it out-of-the-box
  • It's a standard and can be self-hosted if need be (e.g. in air-gapped environments)
  • It provides identification information for notifications so you can contact the operator sending you ceaseless cat facts.

It is not yet hooked up to the wider notifications ecosystem inside Coder, and is therefore hidden behind an experimentweb-push. This simply adds the package and a minimal service worker + button in the UI. Later PRs will build further upon this.

  • Addscodersdk.ExperimentWebPush (web-push)

  • Adds acoderd/webpush package that allows sending native push notifications viagithub.com/SherClockHolmes/webpush-go

    • VAPID keypairs are automatically generated as required.

  • Adds database tables to store push notification subscriptions.

  • Adds an API endpoint that allows users to subscribe/unsubscribe, and send a test notification

  • Adds server CLI command to regenerate VAPID keys (note: regenerating the VAPID keypair requires deleting all existing subscriptions)

@johnstcnjohnstcn self-assigned thisMar 25, 2025
@johnstcnjohnstcn changed the titleCj/push notifications 2 rebasefeat: add push notification funcationalityMar 25, 2025
@johnstcnjohnstcn changed the titlefeat: add push notification funcationalityfeat(coderd0: add push notification funcationalityMar 25, 2025
@johnstcnjohnstcn changed the titlefeat(coderd0: add push notification funcationalityfeat(coderd): add push notification funcationalityMar 25, 2025
@johnstcnjohnstcn changed the titlefeat(coderd): add push notification funcationalityfeat(coderd): add push notification functionalityMar 25, 2025
@johnstcnjohnstcnforce-pushed thecj/push-notifications-2-rebase branch from1fdf10c to86c481eCompareMarch 25, 2025 21:59
Copy link
MemberAuthor

@johnstcnjohnstcn left a comment

Choose a reason for hiding this comment

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

We don't want to call this a notification of any kind because this will confuse with our existing notification targets. Instead we should call this something like PushMessage

@johnstcnjohnstcnforce-pushed thecj/push-notifications-2-rebase branch 2 times, most recently fromc44bdca to1274623CompareMarch 26, 2025 14:18
@johnstcnjohnstcnforce-pushed thecj/push-notifications-2-rebase branch from8dde95a to449f882CompareMarch 26, 2025 16:37
@johnstcnjohnstcn marked this pull request as ready for reviewMarch 26, 2025 16:40
@johnstcnjohnstcn requested a review frommtojekMarch 26, 2025 16:41
@johnstcnjohnstcn changed the titlefeat(coderd): add push notification functionalityfeat(coderd): add webpush packageMar 26, 2025
Copy link
Member

@mtojekmtojek left a comment

Choose a reason for hiding this comment

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

First round done. I skipped "site" changes for now.


defercancel()

ifregenVapidKeypairDBURL=="" {
Copy link
Member

Choose a reason for hiding this comment

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

I'm quite sure I have seen this pattern in other CLI commands:
startBuiltinPostgres - codersdk.PostgresAuth - ConnectToPostgres

we may consider some base pattern for all commands using the database

Copy link
MemberAuthor

@johnstcnjohnstcnMar 26, 2025•
edited
Loading

Choose a reason for hiding this comment

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

Yep there are a few commands that do this. I'll create a follow-up. (EDIT:coder/internal#541)

mtojek reacted with thumbs up emoji

// Ensure the push subscriptions were deleted.
varcountint64
rows,err:=sqlDB.QueryContext(ctx,"SELECT COUNT(*) FROM webpush_subscriptions")
Copy link
Member

Choose a reason for hiding this comment

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

just confirming - is there a chance that there will be another testrun operating on this table (flakiness risk?)

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

There shouldn't be. It operates entirely in its own transaction, and each test should be operating on its own database.

mtojek reacted with thumbs up emoji
postgres-builtin-serve Run the built-in PostgreSQL deployment.
postgres-builtin-url Output the connection URL for the built-in
PostgreSQL deployment.
create-admin-userCreate a new admin user with the given username,
Copy link
Member

Choose a reason for hiding this comment

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

nit: unrelated change?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I'm actually not sure what's causing these changes, but the command to update these golden files is convinced that this is how it should be now.

mtojek reacted with thumbs up emoji
assert.Len(t,subscriptions,0,"No subscriptions should be returned")
})

t.Run("FailedDelivery",func(t*testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

do we want to implement retry mechanism at some point?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Yes, we will need to do this later if we make it a first-class notifciation target.

mtojek reacted with thumbs up emoji
}

typeWebpushMessagestruct {
Iconstring`json:"icon"`
Copy link
Member

Choose a reason for hiding this comment

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

In the notification payload we use version for potential schema changes. Should we adopt this pattern too?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Not a bad idea! It might be slightly premature right now, but I could see this being useful down the line.

mtojek reacted with thumbs up emoji
Copy link
Member

@mtojekmtojek left a comment

Choose a reason for hiding this comment

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

Cosmetics

@johnstcnjohnstcn merged commit06e5d9e intomainMar 27, 2025
34 of 35 checks passed
@johnstcnjohnstcn deleted the cj/push-notifications-2-rebase branchMarch 27, 2025 10:03
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsMar 27, 2025
@stirby
Copy link
Collaborator

/cherry-pick release/2.21

Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@mtojekmtojekmtojek approved these changes

Assignees

@johnstcnjohnstcn

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@johnstcn@stirby@mtojek@kylecarbs
[8]ページ先頭
©2009-2025 Movatter.jp