From 298cab2e5425b4ac12d5851657a382367bb54882 Mon Sep 17 00:00:00 2001 From: Coracle-Bot Date: Tue, 22 Sep 2026 23:11:32 +0000 Subject: [PATCH] Review what a health check will do before applying it (#620) --- e2e/USER_STORIES.md | 10 +- e2e/specs/onboarding.spec.ts | 11 +- src/app/components/HealthCheckItem.svelte | 10 +- src/app/components/HealthCheckReview.svelte | 93 +++++++++++ src/app/components/HomeHealthChecks.svelte | 10 +- src/app/healthChecks.ts | 162 +++++++++++++++----- 6 files changed, 241 insertions(+), 55 deletions(-) create mode 100644 src/app/components/HealthCheckReview.svelte diff --git a/e2e/USER_STORIES.md b/e2e/USER_STORIES.md index 0b1c4051..af65240f 100644 --- a/e2e/USER_STORIES.md +++ b/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 - each one applies. +- Relay health checks are listed alongside the inbox, each naming what is wrong. + 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. diff --git a/e2e/specs/onboarding.spec.ts b/e2e/specs/onboarding.spec.ts index bb8ace0f..0ff7df88 100644 --- a/e2e/specs/onboarding.spec.ts +++ b/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"]') diff --git a/src/app/components/HealthCheckItem.svelte b/src/app/components/HealthCheckItem.svelte index 01f3fa26..9d8d879d 100644 --- a/src/app/components/HealthCheckItem.svelte +++ b/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)])
+ class="flex items-center justify-between gap-3 px-4 py-3">
{healthCheck.title}

{healthCheck.description}

-
diff --git a/src/app/components/HealthCheckReview.svelte b/src/app/components/HealthCheckReview.svelte new file mode 100644 index 00000000..dd254060 --- /dev/null +++ b/src/app/components/HealthCheckReview.svelte @@ -0,0 +1,93 @@ + + + + + + Review changes + Nothing changes until you confirm. + + {#each publishing as plan (plan.summary)} +
+

{plan.summary}

+ {#each plan.changes as change (change.label)} +
+ {change.label} + {#each change.added as url (url)} + + + {displayRelayUrl(url)} + + {/each} + {#each change.removed as url (url)} + + + {displayRelayUrl(url)} + + {/each} +
+ {/each} +
+ {/each} + + + + + + diff --git a/src/app/components/HomeHealthChecks.svelte b/src/app/components/HomeHealthChecks.svelte index 653f7c4b..d2e24744 100644 --- a/src/app/components/HomeHealthChecks.svelte +++ b/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 = () => { - for (const healthCheck of $pending) { - $healthChecks.apply(healthCheck) - } - } + const reviewAll = () => reviewPlans($pending.map(check => $healthChecks.plan(check))) @@ -36,7 +32,7 @@ {/each} {#if $pending.length > 1}
- diff --git a/src/app/healthChecks.ts b/src/app/healthChecks.ts index 8a131b4d..82584773 100644 --- a/src/app/healthChecks.ts +++ b/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>({}) @@ -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: () => - this.app.use(MessagingRelayLists).setUrls(DEFAULT_MESSAGING_RELAYS).then(publish), + plan: context => + 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 => - this.app.use(RelayLists).setReadUrls(sample(5, context.readRelays)).then(publish), + plan: context => + 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 => - this.app.use(RelayLists).setWriteUrls(sample(5, context.writeRelays)).then(publish), + plan: context => + 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 => - this.app.use(MessagingRelayLists).setUrls(sample(5, context.messagingRelays)).then(publish), + plan: context => + 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 => - this.app - .use(SearchRelayLists) - .setUrls(context.searchRelays.filter(this.supportsSearch)) - .then(publish), + plan: context => + relayPlan( + "Drops the search relays that don't support search.", + "Search relays", + 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: () => { - const {secret} = session.get()!.data as {secret: string} + plan: () => ({ + 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, { - secret, - next: () => { - forceHealthChecks.update(assoc("backup-key", false)) - history.back() - }, - submitText: "Done", - }) - }, + pushModal(KeyDownload, { + secret, + next: () => { + forceHealthChecks.update(assoc("backup-key", false)) + history.back() + }, + 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)