Review what a health check will do before applying it (#620)

e2e/USER_STORIES.md

@@ -55,7 +55,10 @@ Acceptance:
 - The dashboard shows a pending "Back Up Your Key" health check until the existing
   backup flow completes successfully; leaving the modal keeps the check pending.
 - Reloading preserves both the pending reminder and a completed backup.
-- Applying all recommendations opens the backup flow alongside the automatic relay fixes.
+- Applying all recommendations first lists the relay changes, and leaving that
+  review changes nothing. Confirming it opens the backup flow alongside the relay fixes.
+- The "Back Up Your Key" check opens the backup flow directly, since it has nothing
+  to review.
 - Choosing the encrypted download requires a password of at least 12 characters
   and produces a file containing an ncryptsec rather than a plain nsec.
 - The display name entered during signup appears on the new user's own profile.
@@ -1691,8 +1694,9 @@ Acceptance:
   disappears once its space is read.
 - A conversation carries an unread dot, and "Mark all read" empties the inbox.
 - Selecting a conversation opens it.
-- Relay health checks are listed alongside the inbox, with the recommendation
+- Relay health checks are listed alongside the inbox, each naming what is wrong.
-  each one applies.
+  Applying one opens a review naming the relays it adds and removes, and
+  publishes nothing until it is confirmed.
 - Hosting is offered whether or not she hosts a space: a shortcut to the hosting
   panel when she has one, an invitation to start one when she doesn't.
 

e2e/specs/onboarding.spec.ts

@@ -134,16 +134,23 @@ test("US-002 sign up by generating a new key", async ({seed, visit}) => {
   const backupCheck = page.getByRole("group", {name: "Back Up Your Key"})
 
   await page.getByRole("button", {name: "Apply all recommendations"}).click()
+  await expect(page.getByRole("heading", {name: "Review changes"})).toBeVisible()
+  await page.getByRole("button", {name: "Go back"}).click()
+  await expect(backupCheck).toBeVisible()
+
+  await page.getByRole("button", {name: "Apply all recommendations"}).click()
+  await page.getByRole("button", {name: "Confirm"}).click()
+  await expect(page.getByRole("heading", {name: "Review changes"})).toHaveCount(0)
   await expect(page.getByRole("heading", {name: "Backup your Key"})).toBeVisible()
   await page.getByRole("button", {name: "Go back"}).click()
   await expect(backupCheck).toBeVisible()
 
-  await backupCheck.getByRole("button", {name: "Back Up"}).click()
+  await backupCheck.getByRole("button", {name: "Fix"}).click()
   await expect(page.getByRole("heading", {name: "Backup your Key"})).toBeVisible()
   await page.getByRole("button", {name: "Go back"}).click()
   await expect(page.getByText("Back Up Your Key")).toBeVisible()
 
-  await backupCheck.getByRole("button", {name: "Back Up"}).click()
+  await backupCheck.getByRole("button", {name: "Fix"}).click()
 
   const doneButton = page.getByRole("button", {name: "Done"})
   const password = page.locator('input[type="password"]')

src/app/components/HealthCheckItem.svelte

@@ -3,7 +3,7 @@
   import Icon from "@lib/components/Icon.svelte"
   import Button from "@lib/components/Button.svelte"
   import type {HealthCheck} from "@app/healthChecks"
-  import {healthChecks} from "@app/healthChecks"
+  import {healthChecks, reviewPlans} from "@app/healthChecks"
 
   type Props = {
     healthCheck: HealthCheck
@@ -11,19 +11,19 @@
 
   const {healthCheck}: Props = $props()
 
-  const apply = () => $healthChecks.apply(healthCheck)
+  const start = () => reviewPlans([$healthChecks.plan(healthCheck)])
 </script>
 
 <div
   role="group"
   aria-label={healthCheck.title}
-  class="flex items-start justify-between gap-3 px-4 py-3">
+  class="flex items-center justify-between gap-3 px-4 py-3">
   <div class="flex min-w-0 flex-col gap-1">
     <strong class="text-sm">{healthCheck.title}</strong>
     <p class="text-sm opacity-75">{healthCheck.description}</p>
   </div>
-  <Button class="button button-neutral button-sm shrink-0" onclick={apply}>
+  <Button class="button button-neutral button-sm shrink-0" onclick={start}>
     <Icon icon={Stars} size={4} />
-    {healthCheck.action}
+    Fix
   </Button>
 </div>

src/app/components/HealthCheckReview.svelte

@@ -0,0 +1,93 @@
+<script lang="ts">
+  import {displayRelayUrl} from "@welshman/util"
+  import {preventDefault} from "@lib/html"
+  import Stars from "@assets/icons/stars.svg?dataurl"
+  import AddCircle from "@assets/icons/add-circle.svg?dataurl"
+  import MinusCircle from "@assets/icons/minus-circle.svg?dataurl"
+  import AltArrowLeft from "@assets/icons/alt-arrow-left.svg?dataurl"
+  import Icon from "@lib/components/Icon.svelte"
+  import Button from "@lib/components/Button.svelte"
+  import Spinner from "@lib/components/Spinner.svelte"
+  import Modal from "@lib/components/Modal.svelte"
+  import ModalBody from "@lib/components/ModalBody.svelte"
+  import ModalHeader from "@lib/components/ModalHeader.svelte"
+  import ModalTitle from "@lib/components/ModalTitle.svelte"
+  import ModalSubtitle from "@lib/components/ModalSubtitle.svelte"
+  import ModalFooter from "@lib/components/ModalFooter.svelte"
+  import type {HealthCheckPlan} from "@app/healthChecks"
+  import {clearModals} from "@app/modal"
+
+  type Props = {
+    plans: HealthCheckPlan[]
+  }
+
+  const {plans}: Props = $props()
+
+  const publishing = plans.filter(plan => plan.changes.length > 0)
+  const interactive = plans.filter(plan => plan.changes.length === 0)
+
+  let loading = $state(false)
+
+  const back = () => history.back()
+
+  const confirm = async () => {
+    loading = true
+
+    try {
+      await Promise.all(publishing.map(plan => plan.apply()))
+    } finally {
+      loading = false
+    }
+
+    clearModals()
+
+    for (const plan of interactive) {
+      plan.apply()
+    }
+  }
+</script>
+
+<Modal tag="form" onsubmit={preventDefault(confirm)}>
+  <ModalBody>
+    <ModalHeader>
+      <ModalTitle>Review changes</ModalTitle>
+      <ModalSubtitle>Nothing changes until you confirm.</ModalSubtitle>
+    </ModalHeader>
+    {#each publishing as plan (plan.summary)}
+      <div class="flex flex-col gap-2">
+        <p>{plan.summary}</p>
+        {#each plan.changes as change (change.label)}
+          <div class="flex flex-col gap-1 rounded-xl bg-surface-more p-3">
+            <strong class="text-sm">{change.label}</strong>
+            {#each change.added as url (url)}
+              <span class="flex min-w-0 items-center gap-2 text-sm">
+                <Icon icon={AddCircle} size={4} class="shrink-0 text-success" />
+                <span class="min-w-0 truncate">{displayRelayUrl(url)}</span>
+              </span>
+            {/each}
+            {#each change.removed as url (url)}
+              <span class="flex min-w-0 items-center gap-2 text-sm">
+                <Icon icon={MinusCircle} size={4} class="shrink-0 text-error" />
+                <span class="min-w-0 truncate">{displayRelayUrl(url)}</span>
+              </span>
+            {/each}
+          </div>
+        {/each}
+      </div>
+    {/each}
+  </ModalBody>
+  <ModalFooter>
+    <Button class="button button-link" onclick={back} disabled={loading}>
+      <Icon icon={AltArrowLeft} />
+      Go back
+    </Button>
+    <Button type="submit" class="button button-primary" disabled={loading}>
+      {#if loading}
+        <Spinner size="sm" />
+      {:else}
+        <Icon icon={Stars} size={4} />
+      {/if}
+      Confirm
+    </Button>
+  </ModalFooter>
+</Modal>

src/app/components/HomeHealthChecks.svelte

@@ -7,15 +7,11 @@
   import Button from "@lib/components/Button.svelte"
   import HomeSection from "@app/components/HomeSection.svelte"
   import HealthCheckItem from "@app/components/HealthCheckItem.svelte"
-  import {healthChecks} from "@app/healthChecks"
+  import {healthChecks, reviewPlans} from "@app/healthChecks"
 
   const pending = $healthChecks.pending.$
 
-  const applyAll = () => {
+  const reviewAll = () => reviewPlans($pending.map(check => $healthChecks.plan(check)))
-    for (const healthCheck of $pending) {
-      $healthChecks.apply(healthCheck)
-    }
-  }
 </script>
 
 <HomeSection title="Health checks" icon={Stethoscope}>
@@ -36,7 +32,7 @@
       {/each}
       {#if $pending.length > 1}
         <div class="flex justify-center p-3">
-          <Button class="button button-primary button-sm" onclick={applyAll}>
+          <Button class="button button-primary button-sm" onclick={reviewAll}>
             <Icon icon={Stars} size={4} />
             Apply all recommendations
           </Button>

src/app/healthChecks.ts

@@ -1,5 +1,6 @@
 import {derived, get, writable} from "svelte/store"
 import {assoc, sample} from "@welshman/lib"
+import {normalizeRelayUrl} from "@welshman/util"
 import {
   MessagingRelayLists,
   RelayLists,
@@ -8,11 +9,14 @@ import {
   projection,
   publish,
 } from "@welshman/app"
-import type {IApp, Projection} from "@welshman/app"
+import type {Command, IApp, Projection} from "@welshman/app"
+import {errorMessage} from "@lib/util"
 import KeyDownload from "@app/components/KeyDownload.svelte"
+import HealthCheckReview from "@app/components/HealthCheckReview.svelte"
 import {session, usePlugin} from "@app/core"
 import {DEFAULT_RELAYS, DEFAULT_MESSAGING_RELAYS} from "@app/env"
 import {pushModal} from "@app/modal"
+import {pushToast} from "@app/toast"
 
 export type HealthCheckContext = {
   readRelays: string[]
@@ -21,13 +25,58 @@ export type HealthCheckContext = {
   searchRelays: string[]
 }
 
+export type HealthCheckChange = {
+  label: string
+  added: string[]
+  removed: string[]
+}
+
+export type HealthCheckPlan = {
+  summary: string
+  changes: HealthCheckChange[]
+  apply: () => unknown
+}
+
 export type HealthCheck = {
   id: string
   title: string
   description: string
-  action: string
   isPending: (context: HealthCheckContext) => boolean
-  apply: (context: HealthCheckContext) => unknown
+  plan: (context: HealthCheckContext) => HealthCheckPlan
+}
+
+const publishRelayList = async (command: Command) => {
+  const error = await publish(command).waitForError()
+
+  if (error) {
+    pushToast({theme: "error", message: `Your relays couldn't be saved: ${errorMessage(error)}`})
+  }
+}
+
+const relayPlan = (
+  summary: string,
+  label: string,
+  current: string[],
+  next: string[],
+  apply: (urls: string[]) => unknown,
+): HealthCheckPlan => {
+  const currentUrls = current.map(normalizeRelayUrl)
+  const nextUrls = next.map(normalizeRelayUrl)
+  const added = nextUrls.filter(url => !currentUrls.includes(url))
+  const removed = currentUrls.filter(url => !nextUrls.includes(url))
+  const changes = added.length + removed.length > 0 ? [{label, added, removed}] : []
+
+  return {summary, changes, apply: () => apply(nextUrls)}
+}
+
+export const reviewPlans = (plans: HealthCheckPlan[]) => {
+  if (plans.some(plan => plan.changes.length > 0)) {
+    pushModal(HealthCheckReview, {plans})
+  } else {
+    for (const plan of plans) {
+      plan.apply()
+    }
+  }
 }
 
 export const forceHealthChecks = writable<Record<string, boolean>>({})
@@ -73,94 +122,131 @@ export class HealthChecks {
       id: "missing-inbox-relays",
       title: "Missing Inbox Relays",
       description: "Other people aren't currently able to reliably tag you in public notes.",
-      action: "Update",
       isPending: context => context.readRelays.length <= 1,
-      apply: () => this.app.use(RelayLists).setReadUrls(DEFAULT_RELAYS).then(publish),
+      plan: context =>
+        relayPlan(
+          "Sets your inbox relays to the ones this app recommends.",
+          "Inbox relays",
+          context.readRelays,
+          DEFAULT_RELAYS,
+          urls => this.app.use(RelayLists).setReadUrls(urls).then(publishRelayList),
+        ),
     },
     {
       id: "missing-outbox-relays",
       title: "Missing Outbox Relays",
       description: "Other people aren't currently able to reliably find your public notes.",
-      action: "Update",
       isPending: context => context.writeRelays.length <= 1,
-      apply: () => this.app.use(RelayLists).setWriteUrls(DEFAULT_RELAYS).then(publish),
+      plan: context =>
+        relayPlan(
+          "Sets your outbox relays to the ones this app recommends.",
+          "Outbox relays",
+          context.writeRelays,
+          DEFAULT_RELAYS,
+          urls => this.app.use(RelayLists).setWriteUrls(urls).then(publishRelayList),
+        ),
     },
     {
       id: "missing-dm-relays",
       title: "Missing DM Relays",
       description: "You aren't currently able to reliably send or receive direct messages.",
-      action: "Update",
       isPending: context => context.messagingRelays.length <= 1,
-      apply: () =>
+      plan: context =>
-        this.app.use(MessagingRelayLists).setUrls(DEFAULT_MESSAGING_RELAYS).then(publish),
+        relayPlan(
+          "Sets your DM relays to the ones this app recommends.",
+          "DM relays",
+          context.messagingRelays,
+          DEFAULT_MESSAGING_RELAYS,
+          urls => this.app.use(MessagingRelayLists).setUrls(urls).then(publishRelayList),
+        ),
     },
     {
       id: "too-many-inbox-relays",
       title: "Too Many Inbox Relays",
       description:
         "You have more inbox relays than is really necessary, which can affect resource usage.",
-      action: "Prune Selections",
       isPending: context => context.readRelays.length > 8,
-      apply: context =>
+      plan: context =>
-        this.app.use(RelayLists).setReadUrls(sample(5, context.readRelays)).then(publish),
+        relayPlan(
+          "Keeps five of your inbox relays and drops the rest.",
+          "Inbox relays",
+          context.readRelays,
+          sample(5, context.readRelays),
+          urls => this.app.use(RelayLists).setReadUrls(urls).then(publishRelayList),
+        ),
     },
     {
       id: "too-many-outbox-relays",
       title: "Too Many Outbox Relays",
       description:
         "You have more outbox relays than is really necessary, which can affect resource usage.",
-      action: "Prune Selections",
       isPending: context => context.writeRelays.length > 8,
-      apply: context =>
+      plan: context =>
-        this.app.use(RelayLists).setWriteUrls(sample(5, context.writeRelays)).then(publish),
+        relayPlan(
+          "Keeps five of your outbox relays and drops the rest.",
+          "Outbox relays",
+          context.writeRelays,
+          sample(5, context.writeRelays),
+          urls => this.app.use(RelayLists).setWriteUrls(urls).then(publishRelayList),
+        ),
     },
     {
       id: "too-many-dm-relays",
       title: "Too Many DM Relays",
       description:
         "You have more DM relays than is really necessary, which can affect resource usage.",
-      action: "Prune Selections",
       isPending: context => context.messagingRelays.length > 8,
-      apply: context =>
+      plan: context =>
-        this.app.use(MessagingRelayLists).setUrls(sample(5, context.messagingRelays)).then(publish),
+        relayPlan(
+          "Keeps five of your DM relays and drops the rest.",
+          "DM relays",
+          context.messagingRelays,
+          sample(5, context.messagingRelays),
+          urls => this.app.use(MessagingRelayLists).setUrls(urls).then(publishRelayList),
+        ),
     },
     {
       id: "invalid-search-relays",
       title: "Invalid Search Relays",
       description: "Some of your search relays don't support search.",
-      action: "Remove Invalid",
       isPending: context => context.searchRelays.some(url => !this.supportsSearch(url)),
-      apply: context =>
+      plan: context =>
-        this.app
+        relayPlan(
-          .use(SearchRelayLists)
+          "Drops the search relays that don't support search.",
-          .setUrls(context.searchRelays.filter(this.supportsSearch))
+          "Search relays",
-          .then(publish),
+          context.searchRelays,
+          context.searchRelays.filter(this.supportsSearch),
+          urls => this.app.use(SearchRelayLists).setUrls(urls).then(publishRelayList),
+        ),
     },
     {
       id: "backup-key",
       title: "Back Up Your Key",
       description: "Save a backup of your private key in a secure place.",
-      action: "Back Up",
       isPending: () => false,
-      apply: () => {
+      plan: () => ({
-        const {secret} = session.get()!.data as {secret: string}
+        summary: "Opens the key backup dialog, where you choose how to save your key.",
+        changes: [],
+        apply: () => {
+          const {secret} = session.get()!.data as {secret: string}
 
-        pushModal(KeyDownload, {
+          pushModal(KeyDownload, {
-          secret,
+            secret,
-          next: () => {
+            next: () => {
-            forceHealthChecks.update(assoc("backup-key", false))
+              forceHealthChecks.update(assoc("backup-key", false))
-            history.back()
+              history.back()
-          },
+            },
-          submitText: "Done",
+            submitText: "Done",
-        })
+          })
-      },
+        },
+      }),
     },
   ]
 
   isPending = (healthCheck: HealthCheck) =>
     get(forceHealthChecks)[healthCheck.id] ?? healthCheck.isPending(this.context.get())
 
-  apply = (healthCheck: HealthCheck) => healthCheck.apply(this.context.get())
+  plan = (healthCheck: HealthCheck) => healthCheck.plan(this.context.get())
 }
 
 export const healthChecks = usePlugin(HealthChecks)