From e9b9f04ff975d9434421b31190cab1bc89aee2ea Mon Sep 17 00:00:00 2001
From: Grace Kind <gracekind400@gmail.com>
Date: Sat, 15 Aug 2026 23:46:23 -0500
Subject: [PATCH] Remove notification service default

---
 src/_includes/head.html                       |   1 -
 src/js/config.js                              |   8 --
 src/js/push/courierPushService.js             |  37 +++--
 src/js/views/settings/notifications.view.js   |  47 +++---
 src/src.11tydata.js                           |   7 -
 tests/e2e/helpers.js                          |  11 +-
 tests/e2e/mockServer.js                       |  72 +++++++++-
 .../views/settingsNotifications.view.test.js  | 136 +++++++++++++++++-
 tests/e2e/testData.js                         |  12 ++
 tests/unit/specs/courierPushService.test.js   |  46 ++++--
 10 files changed, 317 insertions(+), 60 deletions(-)

diff --git a/src/_includes/head.html b/src/_includes/head.html
index 0148ba06..19889459 100644
--- a/src/_includes/head.html
+++ b/src/_includes/head.html
@@ -44,7 +44,6 @@
     oauthOptionalScopes: "{{ oauthOptionalScopes }}",
     playwright: "{{ playwright }}" === "true",
     useConfidentialOauth: "{{ useConfidentialOauth }}" === "true",
-    notificationServiceDid: "{{ notificationServiceDid }}",
   };
 </script>
 <script>
diff --git a/src/js/config.js b/src/js/config.js
index 0a1c933f..b9912c25 100644
--- a/src/js/config.js
+++ b/src/js/config.js
@@ -35,14 +35,6 @@ export const VIDEO_SERVICE_DID = "did:web:video.bsky.app";
 
 export const BSKY_LABELER_DID = "did:plc:ar7c4by46qjdydhdevvrndac";
 
-// Default push notification service. Per
-// specs/bsky-push-notification-service.md any conforming DID works; this is
-// only what a deployment suggests, and the user can point at a different
-// service in settings. Set at build time with NOTIFICATION_SERVICE_DID so a
-// deployment is never implicitly sending its users to somebody else's box.
-export const DEFAULT_NOTIFICATION_SERVICE_DID =
-  window.env?.notificationServiceDid || "did:web:courier.7778777.online";
-
 export const IN_APP_LINK_DOMAINS = [
   "bsky.app",
   "impro.social",
diff --git a/src/js/push/courierPushService.js b/src/js/push/courierPushService.js
index 864ec30d..0489da72 100644
--- a/src/js/push/courierPushService.js
+++ b/src/js/push/courierPushService.js
@@ -1,4 +1,3 @@
-import { DEFAULT_NOTIFICATION_SERVICE_DID } from "/js/config.js";
 import { resolveDid, getServiceEndpointFromDidDoc } from "/js/atproto.js";
 
 const STORAGE_KEY = "courier-push-enabled";
@@ -65,19 +64,24 @@ export class CourierPushService {
   }
 
   get isEnabled() {
-    return this.isSupported && localStorage.getItem(STORAGE_KEY) === "true";
+    return (
+      this.isSupported &&
+      this.hasService &&
+      localStorage.getItem(STORAGE_KEY) === "true"
+    );
   }
 
-  // The service this device is pointed at. Per the spec any conforming DID
-  // works; the deployment's default is only a suggestion.
+  // The service this device is pointed at, or null if the user has not named
+  // one. Impro suggests none: a service holds a read-only grant over the
+  // account and polls on the user's behalf, so the choice is always theirs.
   get serviceDid() {
-    return (
-      localStorage.getItem(SERVICE_KEY) || DEFAULT_NOTIFICATION_SERVICE_DID
-    );
+    return localStorage.getItem(SERVICE_KEY) || null;
   }
 
-  get isUsingDefaultService() {
-    return this.serviceDid === DEFAULT_NOTIFICATION_SERVICE_DID;
+  // Nothing can be enabled, resolved or registered until the user has named a
+  // service, so every entry point gates on this.
+  get hasService() {
+    return this.serviceDid !== null;
   }
 
   // The tier the service last confirmed, per the spec's callback echo — not
@@ -107,14 +111,13 @@ export class CourierPushService {
       await this.disable();
     }
     this._forgetConfig();
-    if (did === DEFAULT_NOTIFICATION_SERVICE_DID) {
-      localStorage.removeItem(SERVICE_KEY);
-    } else {
-      localStorage.setItem(SERVICE_KEY, did);
-    }
+    localStorage.setItem(SERVICE_KEY, did);
   }
 
   async fetchServiceConfig() {
+    if (!this.hasService) {
+      throw new Error("No notification service selected");
+    }
     return this._loadServiceConfig(this.serviceDid);
   }
 
@@ -248,7 +251,11 @@ export class CourierPushService {
   // idempotent upsert and there is no API to query registration state, so
   // this is the self-healing path for a rotated or lost subscription.
   async reassertIfEnabled() {
-    if (localStorage.getItem(STORAGE_KEY) !== "true" || !this.isSupported)
+    if (
+      localStorage.getItem(STORAGE_KEY) !== "true" ||
+      !this.isSupported ||
+      !this.hasService
+    )
       return;
     if (Notification.permission !== "granted") {
       // Permission was revoked out-of-band (browser site settings).
diff --git a/src/js/views/settings/notifications.view.js b/src/js/views/settings/notifications.view.js
index 8313b22f..f352b0e3 100644
--- a/src/js/views/settings/notifications.view.js
+++ b/src/js/views/settings/notifications.view.js
@@ -45,15 +45,15 @@ export default async function settingsNotificationsView({
   // The service is user-selectable per the spec, so its name is data, not a
   // constant — every mention of it in this view comes from the service's own
   // config document. Falls back to the DID until that resolves.
-  const $serviceDid = new Signal.State(courierPushService?.serviceDid ?? "");
-  const $serviceName = new Signal.State(courierPushService?.serviceDid ?? "");
+  const $serviceDid = new Signal.State(courierPushService?.serviceDid ?? null);
+  const $serviceName = new Signal.State(courierPushService?.serviceDid ?? null);
   const $pickerOpen = new Signal.State(false);
   const $pickerValue = new Signal.State("");
   const $pickerBusy = new Signal.State(false);
   const $pickerError = new Signal.State("");
 
   async function loadServiceName() {
-    if (!courierPushService) return;
+    if (!courierPushService?.hasService) return;
     const did = courierPushService.serviceDid;
     $serviceDid.set(did);
     try {
@@ -243,9 +243,13 @@ export default async function settingsNotificationsView({
     const pickerOpen = $pickerOpen.get();
     const pickerBusy = $pickerBusy.get();
     const pickerError = $pickerError.get();
-    const pushDescription = pushSupported
-      ? `Get notified even when Impro is closed, via ${serviceName}.`
-      : "Your browser doesn't support push notifications.";
+    const hasService = serviceDid !== null;
+    let pushDescription = "Your browser doesn't support push notifications.";
+    if (pushSupported) {
+      pushDescription = hasService
+        ? `Get notified even when Impro is closed, via ${serviceName}.`
+        : "Get notified even when Impro is closed. Choose a notification service below to turn this on.";
+    }
 
     render(
       html`<div id="settings-notifications-view">
@@ -287,7 +291,7 @@ export default async function settingsNotificationsView({
                 data-testid="push-notifications-toggle"
                 label="Enable push notifications"
                 ?checked=${pushEnabled}
-                ?disabled=${!pushSupported || pushBusy}
+                ?disabled=${!pushSupported || !hasService || pushBusy}
                 @change=${(event) => handlePushToggle(event.detail.checked)}
               ></toggle-switch>
             </div>
@@ -324,13 +328,23 @@ export default async function settingsNotificationsView({
               >
                 <div class="setting-item-info">
                   <h2 class="setting-item-name">Notification service</h2>
-                  <p class="setting-item-desc">
-                    Push notifications are delivered by
-                    <strong>${serviceName}</strong>, which holds a read-only
-                    grant to watch this account's notifications on your behalf.
-                    You can point Impro at a different service, or run your own.
-                    <br /><code>${serviceDid}</code>
-                  </p>
+                  ${hasService
+                    ? html`<p class="setting-item-desc">
+                        Push notifications are delivered by
+                        <strong>${serviceName}</strong>, which holds a read-only
+                        grant to watch this account's notifications on your
+                        behalf. You can point Impro at a different service, or
+                        run your own. <br /><code>${serviceDid}</code>
+                      </p>`
+                    : html`<p
+                        class="setting-item-desc"
+                        data-testid="notification-service-unset"
+                      >
+                        Impro doesn't pick a notification service for you. Enter
+                        the DID of one you trust — or run your own — and it will
+                        hold a read-only grant to watch this account's
+                        notifications on your behalf.
+                      </p>`}
                   ${pickerOpen
                     ? html`<div class="notification-service-picker">
                         <input
@@ -384,13 +398,14 @@ export default async function settingsNotificationsView({
                     : html`<button
                         class="button secondary"
                         data-testid="notification-service-change"
+                        data-teststate=${hasService ? "set" : "unset"}
                         @click=${() => {
-                          $pickerValue.set(serviceDid);
+                          $pickerValue.set(serviceDid ?? "");
                           $pickerError.set("");
                           $pickerOpen.set(true);
                         }}
                       >
-                        Change service
+                        ${hasService ? "Change service" : "Choose a service"}
                       </button>`}
                 </div>
               </section>`
diff --git a/src/src.11tydata.js b/src/src.11tydata.js
index 9f8012d8..b7b5ed23 100644
--- a/src/src.11tydata.js
+++ b/src/src.11tydata.js
@@ -12,13 +12,6 @@ export default {
   playwright: process.env.PLAYWRIGHT ? "true" : "",
   oauthScopes: OAUTH_SCOPES,
   oauthOptionalScopes: OPTIONAL_OAUTH_SCOPES,
-  // Which push notification service this deployment suggests by default.
-  // Deployment config rather than a constant: a service holds a read-only
-  // OAuth grant for every subscriber and polls on their behalf, so which one
-  // a deployment points at is that deployment's decision, not this repo's.
-  // Users can pick a different one in settings.
-  notificationServiceDid:
-    process.env.NOTIFICATION_SERVICE_DID ?? "did:web:courier.7778777.online",
   oauthPublicJwk: process.env.OAUTH_PUBLIC_JWK ?? "",
   useConfidentialOauth: process.env.OAUTH_PUBLIC_JWK ? "true" : "",
 };
diff --git a/tests/e2e/helpers.js b/tests/e2e/helpers.js
index 469f1fbb..72cba4e0 100644
--- a/tests/e2e/helpers.js
+++ b/tests/e2e/helpers.js
@@ -1,5 +1,5 @@
 import { OAUTH_SCOPES } from "../../src/oauthScopes.js";
-import { userProfile } from "./testData.js";
+import { notificationService, userProfile } from "./testData.js";
 
 // Hold the pointer down on an element long enough to trigger a long-press
 // (the app uses a 500ms threshold).
@@ -11,6 +11,15 @@ export async function longPress(page, locator) {
   await page.mouse.up();
 }
 
+// Points the app at MockServer's dummy notification service, as if the user
+// had entered its DID in settings. No service is configured by default, so any
+// test exercising push beyond the unset state needs this.
+export async function selectNotificationService(page, { did } = {}) {
+  await page.addInitScript((serviceDid) => {
+    localStorage.setItem("courier-push-service-did", serviceDid);
+  }, did ?? notificationService.did);
+}
+
 export async function login(page, { scope = OAUTH_SCOPES } = {}) {
   const oauthSession = {
     did: userProfile.did,
diff --git a/tests/e2e/mockServer.js b/tests/e2e/mockServer.js
index 7532dea5..f1726cb5 100644
--- a/tests/e2e/mockServer.js
+++ b/tests/e2e/mockServer.js
@@ -1,5 +1,5 @@
 import { createPost } from "../shared/factories.js";
-import { bskyLabeler, userProfile } from "./testData.js";
+import { bskyLabeler, notificationService, userProfile } from "./testData.js";
 import {
   TEST_PLUGIN_ID,
   TEST_PLUGIN_MANIFEST,
@@ -101,6 +101,9 @@ export class MockServer {
     // plugin with no README (404).
     this.pluginReadme = "# Remote Themes\n\nA test readme for the plugin.";
     this.tokenRefreshShouldFail = false;
+    this.notificationServiceUnreachable = false;
+    this.registerPushCalls = [];
+    this.unregisterPushCalls = [];
   }
 
   // Make subsequent OAuth token refreshes fail with a non-retryable error,
@@ -109,6 +112,12 @@ export class MockServer {
     this.tokenRefreshShouldFail = true;
   }
 
+  // Make the dummy notification service fail to resolve, simulating a DID
+  // that points at nothing conforming.
+  failNotificationServiceLookup() {
+    this.notificationServiceUnreachable = true;
+  }
+
   setSearchHistory({ searches = [], profiles = [] } = {}) {
     this.searchHistory = { searches, profiles };
   }
@@ -682,6 +691,46 @@ export class MockServer {
       route.fulfill({ status: 404, body: "Not Found" }),
     );
 
+    // The dummy notification service (see `notificationService` in
+    // testData.js). Resolving one costs a DID document then a config
+    // document, so both are served here.
+    await page.route(
+      `${notificationService.endpoint}/.well-known/did.json`,
+      (route) => {
+        if (this.notificationServiceUnreachable) {
+          route.fulfill({ status: 500, body: "Server Error" });
+          return;
+        }
+        route.fulfill({
+          status: 200,
+          contentType: "application/json",
+          body: JSON.stringify({
+            id: notificationService.did,
+            service: [
+              {
+                id: "#bsky_notif",
+                type: "BskyNotificationService",
+                serviceEndpoint: notificationService.endpoint,
+              },
+            ],
+          }),
+        });
+      },
+    );
+    await page.route(
+      `${notificationService.endpoint}/.well-known/notif-service.json`,
+      (route) =>
+        route.fulfill({
+          status: 200,
+          contentType: "application/json",
+          body: JSON.stringify({
+            name: notificationService.name,
+            authUrl: notificationService.authUrl,
+            vapidPublicKey: notificationService.vapidPublicKey,
+          }),
+        }),
+    );
+
     // OAuth token endpoint used by seeded test sessions. `invalid_request`
     // (not `invalid_grant`) fails immediately without the cross-tab
     // rotated-session recovery delay.
@@ -895,6 +944,27 @@ export class MockServer {
       });
     });
 
+    await page.route("**/xrpc/app.bsky.notification.registerPush*", (route) => {
+      this.registerPushCalls.push(route.request().postDataJSON());
+      return route.fulfill({
+        status: 200,
+        contentType: "text/plain",
+        body: "",
+      });
+    });
+
+    await page.route(
+      "**/xrpc/app.bsky.notification.unregisterPush*",
+      (route) => {
+        this.unregisterPushCalls.push(route.request().postDataJSON());
+        return route.fulfill({
+          status: 200,
+          contentType: "text/plain",
+          body: "",
+        });
+      },
+    );
+
     await page.route("**/xrpc/app.bsky.feed.getPosts*", async (route) => {
       const url = new URL(route.request().url());
       const uris = url.searchParams.getAll("uris");
diff --git a/tests/e2e/specs/views/settingsNotifications.view.test.js b/tests/e2e/specs/views/settingsNotifications.view.test.js
index c95da75d..033c9b17 100644
--- a/tests/e2e/specs/views/settingsNotifications.view.test.js
+++ b/tests/e2e/specs/views/settingsNotifications.view.test.js
@@ -1,6 +1,7 @@
 import { test, expect } from "../../base.js";
-import { login } from "../../helpers.js";
+import { login, selectNotificationService } from "../../helpers.js";
 import { MockServer } from "../../mockServer.js";
+import { notificationService } from "../../testData.js";
 
 // Stubs the Notification API so permission prompts are deterministic in CI
 // (headless browsers have no real OS notification center to grant/deny).
@@ -178,4 +179,137 @@ test.describe("Settings > Notifications view", () => {
       )
       .toBeNull();
   });
+
+  test.describe("notification service selection", () => {
+    test("with no service chosen, push cannot be turned on", async ({
+      page,
+    }) => {
+      const mockServer = new MockServer();
+      await mockServer.setup(page);
+      await stubNotificationPermission(page, { initial: "default" });
+      await login(page);
+      await page.goto("/settings/notifications");
+
+      await expect(
+        page.locator('[data-testid="notification-service-unset"]'),
+      ).toBeVisible({ timeout: 10000 });
+      await expect(
+        page.locator('[data-testid="push-notifications-toggle"]'),
+      ).toHaveAttribute("disabled", "");
+      await expect(
+        page.locator('[data-testid="notification-service-change"]'),
+      ).toHaveAttribute("data-teststate", "unset");
+    });
+
+    test("entering a service DID resolves it and enables the push toggle", async ({
+      page,
+    }) => {
+      const mockServer = new MockServer();
+      await mockServer.setup(page);
+      await stubNotificationPermission(page, { initial: "default" });
+      await login(page);
+      await page.goto("/settings/notifications");
+
+      await page
+        .locator('[data-testid="notification-service-change"]')
+        .click({ timeout: 10000 });
+      await page
+        .locator('[data-testid="notification-service-input"]')
+        .fill(notificationService.did);
+      await page.locator('[data-testid="notification-service-save"]').click();
+
+      await expect(page.locator('[data-testid="toast"]')).toContainText(
+        notificationService.name,
+      );
+      await expect(
+        page.locator('[data-testid="notification-service-unset"]'),
+      ).toHaveCount(0);
+      await expect(
+        page.locator('[data-testid="push-notifications-toggle"]'),
+      ).not.toHaveAttribute("disabled", "");
+      expect(
+        await page.evaluate(() =>
+          localStorage.getItem("courier-push-service-did"),
+        ),
+      ).toBe(notificationService.did);
+    });
+
+    test("a previously chosen service is shown by name on load", async ({
+      page,
+    }) => {
+      const mockServer = new MockServer();
+      await mockServer.setup(page);
+      await stubNotificationPermission(page, { initial: "default" });
+      await login(page);
+      await selectNotificationService(page);
+      await page.goto("/settings/notifications");
+
+      const section = page.locator(
+        '[data-testid="settings-section-notification-service"]',
+      );
+      await expect(section).toContainText(notificationService.name, {
+        timeout: 10000,
+      });
+      await expect(section).toContainText(notificationService.did);
+      await expect(
+        page.locator('[data-testid="notification-service-change"]'),
+      ).toHaveAttribute("data-teststate", "set");
+    });
+
+    test("rejects input that isn't a DID without hitting the network", async ({
+      page,
+    }) => {
+      const mockServer = new MockServer();
+      await mockServer.setup(page);
+      await stubNotificationPermission(page, { initial: "default" });
+      await login(page);
+      await page.goto("/settings/notifications");
+
+      await page
+        .locator('[data-testid="notification-service-change"]')
+        .click({ timeout: 10000 });
+      await page
+        .locator('[data-testid="notification-service-input"]')
+        .fill("notifs.example.com");
+      await page.locator('[data-testid="notification-service-save"]').click();
+
+      await expect(
+        page.locator('[data-testid="notification-service-error"]'),
+      ).toBeVisible();
+      expect(
+        await page.evaluate(() =>
+          localStorage.getItem("courier-push-service-did"),
+        ),
+      ).toBeNull();
+    });
+
+    test("a service that won't resolve is rejected and not stored", async ({
+      page,
+    }) => {
+      const mockServer = new MockServer();
+      mockServer.failNotificationServiceLookup();
+      await mockServer.setup(page);
+      await stubNotificationPermission(page, { initial: "default" });
+      await login(page);
+      await page.goto("/settings/notifications");
+
+      await page
+        .locator('[data-testid="notification-service-change"]')
+        .click({ timeout: 10000 });
+      await page
+        .locator('[data-testid="notification-service-input"]')
+        .fill(notificationService.did);
+      await page.locator('[data-testid="notification-service-save"]').click();
+
+      await expect(
+        page.locator('[data-testid="notification-service-error"]'),
+      ).toBeVisible({ timeout: 10000 });
+      // The old state must survive a failed switch, so nothing is stored.
+      expect(
+        await page.evaluate(() =>
+          localStorage.getItem("courier-push-service-did"),
+        ),
+      ).toBeNull();
+    });
+  });
 });
diff --git a/tests/e2e/testData.js b/tests/e2e/testData.js
index 55e4713d..b8dec838 100644
--- a/tests/e2e/testData.js
+++ b/tests/e2e/testData.js
@@ -15,3 +15,15 @@ export const bskyLabeler = createLabelerView({
   handle: "moderation.bsky.app",
   displayName: "Bluesky Moderation",
 });
+
+// A dummy push notification service, served entirely by MockServer. The app
+// ships no default service, so tests that need one point at this.
+export const notificationService = {
+  did: "did:web:notifs.test",
+  endpoint: "https://notifs.test",
+  name: "Test Notifications",
+  authUrl: "https://notifs.test/authorize",
+  // Any valid base64url-encoded VAPID key; the browser never verifies it
+  // because MockServer also stubs PushManager.
+  vapidPublicKey: "QklMTFktVEVTVC1WQVBJRC1QVUJMSUMtS0VZLVZBTFVF",
+};
diff --git a/tests/unit/specs/courierPushService.test.js b/tests/unit/specs/courierPushService.test.js
index 4597de34..bf4bb515 100644
--- a/tests/unit/specs/courierPushService.test.js
+++ b/tests/unit/specs/courierPushService.test.js
@@ -221,10 +221,36 @@ describe("CourierPushService service selection", () => {
     }
   });
 
-  it("defaults to the deployment's service until one is chosen", () => {
+  // Impro suggests no service of its own, so nothing can happen until the
+  // user names one.
+  it("has no service until the user names one", () => {
     const { service } = createService();
-    assert.ok(service.isUsingDefaultService);
-    assert.ok(service.serviceDid.startsWith("did:"));
+    assert.equal(service.serviceDid, null);
+    assert.equal(service.hasService, false);
+  });
+
+  it("cannot be enabled while no service is named", () => {
+    const { service } = createService();
+    globalThis.localStorage.setItem("courier-push-enabled", "true");
+    assert.equal(service.isEnabled, false);
+  });
+
+  it("refuses to resolve a config while no service is named", async () => {
+    const { service } = createService();
+    await assert.rejects(() => service.fetchServiceConfig(), {
+      message: "No notification service selected",
+    });
+    assert.equal(fetchCalls.length, 0, "it must not hit the network");
+  });
+
+  it("naming a service persists it across launches", async () => {
+    const { service } = createService();
+    await service.selectService("did:web:notifs.example");
+    assert.equal(service.serviceDid, "did:web:notifs.example");
+    assert.equal(service.hasService, true);
+
+    const { service: relaunched } = createService();
+    assert.equal(relaunched.serviceDid, "did:web:notifs.example");
   });
 
   it("resolves a service through its DID document", async () => {
@@ -236,6 +262,7 @@ describe("CourierPushService service selection", () => {
 
   it("caches the resolved config instead of refetching every launch", async () => {
     const { service } = createService();
+    await service.selectService("did:web:notifs.example");
     await service.fetchServiceConfig();
     const afterFirst = fetchCalls.length;
     await service.fetchServiceConfig();
@@ -254,6 +281,7 @@ describe("CourierPushService service selection", () => {
 
   it("does not cache a failed lookup as the answer", async () => {
     const { service } = createService();
+    await service.selectService("did:web:notifs.example");
     globalThis.fetch = async () => {
       throw new Error("offline");
     };
@@ -280,6 +308,7 @@ describe("CourierPushService service selection", () => {
 
   it("switching services unregisters the old one first", async () => {
     const { service } = createService();
+    await service.selectService("did:web:notifs.example");
     globalThis.localStorage.setItem("courier-push-enabled", "true");
     const unregisterPush = mock.fn(async () => {});
     service.api.unregisterPush = unregisterPush;
@@ -293,22 +322,19 @@ describe("CourierPushService service selection", () => {
     assert.equal(service.isEnabled, false);
   });
 
-  it("switching back to the default clears the override", async () => {
+  it("switching services replaces the stored choice", async () => {
     const { service } = createService();
-    const theDefault = service.serviceDid;
+    await service.selectService("did:web:notifs.example");
     await service.selectService("did:web:elsewhere.example");
-    assert.ok(!service.isUsingDefaultService);
-
-    await service.selectService(theDefault);
-    assert.ok(service.isUsingDefaultService);
     assert.equal(
       globalThis.localStorage.getItem("courier-push-service-did"),
-      null,
+      "did:web:elsewhere.example",
     );
   });
 
   it("switching services drops the previous service's cached config", async () => {
     const { service } = createService();
+    await service.selectService("did:web:notifs.example");
     await service.fetchServiceConfig();
     await service.selectService("did:web:elsewhere.example");
     const before = fetchCalls.length;
-- 
2.51.2