Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 40 commits into from
Mar 27, 2025
Merged

feat(coderd): add webpush package #17091

merged 40 commits into from
Mar 27, 2025

Conversation

johnstcn
Copy link
Member

@johnstcn johnstcn commented Mar 25, 2025 •
edited
Loading

Extracted from https://github.com/coder/coder/tree/kyle/tasks

This PR adds a webpush package under coderd to allow sending push notifications to browsers from coderd using Web Push and VAPID.

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 experiment web-push. This simply adds the package and a minimal service worker + button in the UI. Later PRs will build further upon this.

  • Adds codersdk.ExperimentWebPush (web-push)

  • Adds a coderd/webpush package that allows sending native push notifications via github.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)

@johnstcn johnstcn self-assigned this Mar 25, 2025
@johnstcn johnstcn changed the title Cj/push notifications 2 rebase feat: add push notification funcationality Mar 25, 2025
@johnstcn johnstcn changed the title feat: add push notification funcationality feat(coderd0: add push notification funcationality Mar 25, 2025
@johnstcn johnstcn changed the title feat(coderd0: add push notification funcationality feat(coderd): add push notification funcationality Mar 25, 2025
johnstcn
johnstcn commented Mar 25, 2025
View reviewed changes
@johnstcn johnstcn changed the title feat(coderd): add push notification funcationality feat(coderd): add push notification functionality Mar 25, 2025
@johnstcn johnstcn mentioned this pull request Mar 25, 2025
Open
@johnstcn johnstcn force-pushed the cj/push-notifications-2-rebase branch from 1fdf10c to 86c481e Compare March 25, 2025 21:59
johnstcn
johnstcn commented Mar 26, 2025
View reviewed changes
johnstcn
johnstcn commented Mar 26, 2025
View reviewed changes
johnstcn
johnstcn commented Mar 26, 2025
View reviewed changes
johnstcn
johnstcn commented Mar 26, 2025
View reviewed changes
johnstcn
johnstcn commented Mar 26, 2025
View reviewed changes
johnstcn
johnstcn commented Mar 26, 2025
View reviewed changes
Copy link
Member Author

@johnstcn johnstcn 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

johnstcn
johnstcn commented Mar 26, 2025
View reviewed changes
@johnstcn johnstcn force-pushed the cj/push-notifications-2-rebase branch 2 times, most recently from c44bdca to 1274623 Compare March 26, 2025 14:18
@johnstcn johnstcn force-pushed the cj/push-notifications-2-rebase branch from 8dde95a to 449f882 Compare March 26, 2025 16:37
@johnstcn johnstcn marked this pull request as ready for review March 26, 2025 16:40
@johnstcn johnstcn requested a review from mtojek March 26, 2025 16:41
@johnstcn johnstcn changed the title feat(coderd): add push notification functionality feat(coderd): add webpush package Mar 26, 2025
mtojek
mtojek reviewed Mar 26, 2025
View reviewed changes
Copy link
Member

@mtojek mtojek 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.

cli/server_regenerate_vapid_keypair.go

defer cancel()

if regenVapidKeypairDBURL == "" {
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
Member Author

@johnstcn johnstcn Mar 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)

cli/server_regenerate_vapid_keypair_test.go

// Ensure the push subscriptions were deleted.
var count int64
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
Member Author

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.

cli/testdata/coder_server_--help.golden
postgres-builtin-serve Run the built-in PostgreSQL deployment.
postgres-builtin-url Output the connection URL for the built-in
PostgreSQL deployment.
create-admin-user Create 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
Member Author

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.

coderd/webpush/webpush_test.go
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
Member Author

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.

codersdk/notifications.go
}

type WebpushMessage struct {
Icon string `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
Member Author

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.

@johnstcn johnstcn mentioned this pull request Mar 26, 2025
Open
mtojek
mtojek approved these changes Mar 27, 2025
View reviewed changes
Copy link
Member

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

Cosmetics

@johnstcn johnstcn merged commit 06e5d9e into main Mar 27, 2025
34 of 35 checks passed
@johnstcn johnstcn deleted the cj/push-notifications-2-rebase branch March 27, 2025 10:03
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2025
@stirby
Copy link
Collaborator

stirby commented Apr 1, 2025

/cherry-pick release/2.21

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

@mtojek mtojek mtojek approved these changes

Assignees

@johnstcn johnstcn

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