diff --git a/apps/web/e2e/user-profile-edit.spec.ts b/apps/web/e2e/user-profile-edit.spec.ts new file mode 100644 index 0000000..612ab13 --- /dev/null +++ b/apps/web/e2e/user-profile-edit.spec.ts @@ -0,0 +1,74 @@ +/** + * E2E: Settings → About you edits a fact in place: its wording, category and the spaces it + * shows in. + */ +import { expect, test } from "@playwright/test"; +import type { UserProfileState } from "@rakazo/contracts"; +import { captureScreenshot, completeOnboarding, openUserSettings, rpc, signup } from "./helpers"; + +test("a fact's wording, category and spaces are edited in place", async ({ page }, testInfo) => { + const stamp = Date.now(); + await signup(page, `profile-edit-${stamp}@rakazo.test`, "password12", "Edit Tester"); + await completeOnboarding(page); + const side = await rpc<{ id: string }>(page, "spaces/create", { name: "Side project" }); + await rpc(page, "profile/update", { + addFacts: [{ category: "preferences", value: "Prefers tea" }], + }); + + const settings = await openUserSettings(page, "profile"); + const profile = settings.getByTestId("user-profile-settings"); + const tea = profile.getByTestId("user-fact").filter({ hasText: "Prefers tea" }); + await tea.getByRole("button", { name: "Edit" }).click(); + + const editor = profile.getByTestId("user-fact-editor"); + const wording = editor.getByRole("textbox", { name: "Fact" }); + await expect(wording).toBeFocused(); + await wording.fill("Prefers green tea"); + await editor.getByRole("combobox", { name: "Category" }).selectOption("habits"); + await editor.getByRole("button", { name: "All spaces" }).click(); + await page.getByRole("menuitemcheckbox", { name: "Side project" }).click(); + await expect(page.getByRole("menuitemcheckbox", { name: "All spaces" })).not.toBeChecked(); + await page.keyboard.press("Escape"); + await expect(editor.getByRole("button", { name: "Side project" })).toBeVisible(); + await captureScreenshot(page, testInfo, "user-profile-edit"); + + await editor.getByRole("button", { name: "Save", exact: true }).click(); + await expect(editor).toHaveCount(0); + const edited = profile + .getByTestId("user-facts-habits") + .getByTestId("user-fact") + .filter({ hasText: "Prefers green tea" }); + await expect(edited).toContainText("Side project"); + await expect(tea).toHaveCount(0); + const teaFact = async () => + (await rpc(page, "profile/get", null)).facts.find((fact) => + fact.value.includes("tea"), + ); + expect(await teaFact()).toMatchObject({ + value: "Prefers green tea", + category: "habits", + allSpaces: false, + spaceIds: [side.id], + }); + + // Escape leaves the fact as it was; showing it everywhere again clears the limit. + await edited.getByRole("button", { name: "Edit" }).click(); + await editor.getByRole("textbox", { name: "Fact" }).fill("Discarded"); + await page.keyboard.press("Escape"); + await expect(editor).toHaveCount(0); + await expect(edited).toContainText("Prefers green tea"); + await expect(settings).toBeVisible(); + + await edited.getByRole("button", { name: "Edit" }).click(); + await editor.getByRole("button", { name: "Side project" }).click(); + await page.getByRole("menuitemcheckbox", { name: "All spaces" }).click(); + await page.keyboard.press("Escape"); + await editor.getByRole("button", { name: "Save", exact: true }).click(); + await expect(editor).toHaveCount(0); + await expect(edited).not.toContainText("Side project"); + expect(await teaFact()).toMatchObject({ + value: "Prefers green tea", + allSpaces: true, + spaceIds: [], + }); +}); diff --git a/apps/web/src/components/UserFactEditor.tsx b/apps/web/src/components/UserFactEditor.tsx new file mode 100644 index 0000000..a7b7e9a --- /dev/null +++ b/apps/web/src/components/UserFactEditor.tsx @@ -0,0 +1,129 @@ +import { Trans, useLingui } from "@lingui/react/macro"; +import type { UserFact, UserFactCategory } from "@rakazo/contracts"; +import { USER_FACT_CATEGORIES, USER_FACT_VALUE_MAX_LENGTH } from "@rakazo/contracts"; +import { + Button, + DropdownMenu, + DropdownMenuCheckboxItem, + DropdownMenuContent, + DropdownMenuSeparator, + DropdownMenuTrigger, + Input, + NativeSelect, + NativeSelectOption, +} from "@rakazo/ui-web"; +import { ChevronDownIcon } from "lucide-react"; +import { useState } from "react"; +import type { UserFactDraft } from "../lib/user-profile-view"; +import { factSpaceNames, toggleFactSpace, userFactDraft } from "../lib/user-profile-view"; + +/** Edits one fact in place: its wording, category and, with more than one space, where it shows. */ +export function UserFactEditor({ + fact, + spaces, + categoryLabels, + busy, + onSave, + onCancel, +}: { + fact: UserFact; + /** The user's spaces; the visibility control appears with more than one. */ + spaces: ReadonlyArray<{ id: string; name: string }>; + categoryLabels: Record; + busy: boolean; + onSave: (draft: UserFactDraft) => void; + onCancel: () => void; +}) { + const { t } = useLingui(); + const [draft, setDraft] = useState(() => userFactDraft(fact)); + const allSpaces = t`All spaces`; + const showSpaces = spaces.length > 1 || draft.spaceIds !== null; + + return ( +
  • +
    { + event.preventDefault(); + if (draft.value.trim()) onSave(draft); + }} + onKeyDown={(event) => { + // The spaces menu renders in a portal; its Escape only closes the menu. + if (event.key === "Escape" && event.currentTarget.contains(event.target as Node)) { + event.stopPropagation(); + onCancel(); + } + }} + > + setDraft({ ...draft, value: event.target.value })} + /> +
    + + setDraft({ ...draft, category: event.target.value as UserFactCategory }) + } + > + {USER_FACT_CATEGORIES.map((category) => ( + + {categoryLabels[category]} + + ))} + + {showSpaces ? ( + + } + > + + {factSpaceNames(draft.spaceIds, spaces).join(", ") || allSpaces} + + + + setDraft({ ...draft, spaceIds: null })} + > + {allSpaces} + + + {spaces.map((space) => ( + + setDraft({ + ...draft, + spaceIds: toggleFactSpace(draft.spaceIds, space.id, checked), + }) + } + > + + {space.name} + + + ))} + + + ) : null} +
    + + +
    +
    +
    +
  • + ); +} diff --git a/apps/web/src/components/UserProfileSettings.tsx b/apps/web/src/components/UserProfileSettings.tsx index 6a8e2e3..3811a9a 100644 --- a/apps/web/src/components/UserProfileSettings.tsx +++ b/apps/web/src/components/UserProfileSettings.tsx @@ -10,7 +10,7 @@ import { AlertDialogTitle, Button, } from "@rakazo/ui-web"; -import { XIcon } from "lucide-react"; +import { PencilIcon, XIcon } from "lucide-react"; import type { ReactNode } from "react"; import { useEffect, useId, useState } from "react"; import { getActiveUiLocale } from "../lib/i18n"; @@ -20,9 +20,12 @@ import { CLEARED_PROFILE_FIELD, displayLanguageTag, displayRegionTag, + factSpaceNames, filledProfileFields, groupUserFacts, + userFactEdit, } from "../lib/user-profile-view"; +import { UserFactEditor } from "./UserFactEditor"; function Row({ value, @@ -51,14 +54,30 @@ function Row({ ); } -/** Everything the user's bots know about the user, with consent and forget controls. */ +/** Everything the user's bots know about the user, with edit, consent and forget controls. */ export function UserProfileSettings() { const { t } = useLingui(); const [state, setState] = useState(null); + const [spaces, setSpaces] = useState>([]); + const [editingId, setEditingId] = useState(null); const [busy, setBusy] = useState(false); const [error, setError] = useState(null); const [forgetOpen, setForgetOpen] = useState(false); + useEffect(() => { + let cancelled = false; + // Only names a fact's spaces; without them the profile still works. + rpc.spaces + .list() + .then((navigation) => { + if (!cancelled) setSpaces(navigation.spaces.map(({ id, name }) => ({ id, name }))); + }) + .catch(() => undefined); + return () => { + cancelled = true; + }; + }, []); + useEffect(() => { let cancelled = false; rpc.profile @@ -74,14 +93,16 @@ export function UserProfileSettings() { }; }, [t]); - async function apply(change: () => Promise) { - if (busy) return; + async function apply(change: () => Promise): Promise { + if (busy) return false; setBusy(true); setError(null); try { setState(await change()); + return true; } catch (err) { setError(err instanceof Error ? err.message : t`Couldn't save`); + return false; } finally { setBusy(false); } @@ -178,36 +199,75 @@ export function UserProfileSettings() { {categoryLabels[group.category]}
      - {group.facts.map((fact) => ( - ( - <> - {fact.status === "pending" ? ( + {group.facts.map((fact) => + fact.id === editingId ? ( + setEditingId(null)} + onSave={(draft) => { + const edit = userFactEdit(fact, draft); + if (!edit) { + setEditingId(null); + return; + } + void apply(() => rpc.profile.update({ editFacts: [edit] })).then((saved) => { + if (saved) setEditingId(null); + }); + }} + /> + ) : ( + ( + <> + {fact.status === "pending" ? ( + + ) : null} - ) : null} - {deleteButton( - describedBy, - () => void apply(() => rpc.profile.forget({ factIds: [fact.id] })), - )} - - )} - /> - ))} + {deleteButton( + describedBy, + () => void apply(() => rpc.profile.forget({ factIds: [fact.id] })), + )} + + )} + /> + ), + )}
    ))} diff --git a/apps/web/src/lib/user-profile-view.test.ts b/apps/web/src/lib/user-profile-view.test.ts index 0843590..0f774c6 100644 --- a/apps/web/src/lib/user-profile-view.test.ts +++ b/apps/web/src/lib/user-profile-view.test.ts @@ -3,8 +3,12 @@ import { describe, expect, it } from "vitest"; import { displayLanguageTag, displayRegionTag, + factSpaceNames, filledProfileFields, groupUserFacts, + toggleFactSpace, + userFactDraft, + userFactEdit, } from "./user-profile-view"; function fact(overrides: Partial & Pick): UserFact { @@ -74,3 +78,43 @@ describe("user profile view", () => { expect(displayRegionTag("not a tag", "en")).toBe("not a tag"); }); }); + +describe("user fact editing", () => { + const tea = fact({ id: "tea", value: "Prefers tea", category: "preferences" }); + const limited = fact({ id: "lin", allSpaces: false, spaceIds: ["space-b", "space-a"] }); + + it("sends only what changed, or nothing", () => { + expect(userFactEdit(tea, userFactDraft(tea))).toBeNull(); + expect(userFactEdit(tea, { ...userFactDraft(tea), value: " Prefers tea " })).toBeNull(); + expect( + userFactEdit(limited, { ...userFactDraft(limited), spaceIds: ["space-a", "space-b"] }), + ).toBeNull(); + expect( + userFactEdit(tea, { value: " Prefers green tea ", category: "habits", spaceIds: null }), + ).toEqual({ id: "tea", value: "Prefers green tea", category: "habits" }); + expect(userFactEdit(tea, { ...userFactDraft(tea), spaceIds: ["space-a"] })).toEqual({ + id: "tea", + spaceIds: ["space-a"], + }); + expect(userFactEdit(limited, { ...userFactDraft(limited), spaceIds: null })).toEqual({ + id: "lin", + spaceIds: null, + }); + }); + + it("limits a fact to checked spaces and shows it everywhere when none is left", () => { + expect(toggleFactSpace(null, "space-a", true)).toEqual(["space-a"]); + expect(toggleFactSpace(["space-a"], "space-b", true)).toEqual(["space-a", "space-b"]); + expect(toggleFactSpace(["space-a", "space-b"], "space-a", false)).toEqual(["space-b"]); + expect(toggleFactSpace(["space-a"], "space-a", false)).toBeNull(); + }); + + it("names the spaces of a limited fact", () => { + const spaces = [ + { id: "space-a", name: "Home" }, + { id: "space-b", name: "Side project" }, + ]; + expect(factSpaceNames(["space-b", "gone"], spaces)).toEqual(["Side project"]); + expect(factSpaceNames(null, spaces)).toEqual([]); + }); +}); diff --git a/apps/web/src/lib/user-profile-view.ts b/apps/web/src/lib/user-profile-view.ts index 97877dc..623e9be 100644 --- a/apps/web/src/lib/user-profile-view.ts +++ b/apps/web/src/lib/user-profile-view.ts @@ -1,6 +1,15 @@ -import type { UserFact, UserFactCategory, UserProfile } from "@rakazo/contracts"; +import type { UpdateUserProfile, UserFact, UserFactCategory, UserProfile } from "@rakazo/contracts"; import { USER_FACT_CATEGORIES } from "@rakazo/contracts"; +/** What the fact editor holds: null spaces means every space. */ +export interface UserFactDraft { + value: string; + category: UserFactCategory; + spaceIds: string[] | null; +} + +type UserFactEdit = NonNullable[number]; + export type ProfileField = "timeZone" | "language" | "locale" | "communicationStyle" | "summary"; const PROFILE_FIELDS: ProfileField[] = [ @@ -75,3 +84,49 @@ export function displayRegionTag(tag: string, uiLocale: string): string { return tag; } } + +export function userFactDraft(fact: UserFact): UserFactDraft { + return { + value: fact.value, + category: fact.category, + spaceIds: fact.allSpaces ? null : fact.spaceIds, + }; +} + +/** The `editFacts` entry for what changed in the draft, or null when nothing did. */ +export function userFactEdit(fact: UserFact, draft: UserFactDraft): UserFactEdit | null { + const edit: UserFactEdit = { id: fact.id }; + const value = draft.value.trim(); + if (value !== fact.value) edit.value = value; + if (draft.category !== fact.category) edit.category = draft.category; + const before = userFactDraft(fact).spaceIds; + const sameSpaces = + before === null || draft.spaceIds === null + ? before === draft.spaceIds + : before.length === draft.spaceIds.length && + draft.spaceIds.every((spaceId) => before.includes(spaceId)); + if (!sameSpaces) edit.spaceIds = draft.spaceIds; + return Object.keys(edit).length > 1 ? edit : null; +} + +/** Checks or unchecks one space; unchecking the last one shows the fact everywhere again. */ +export function toggleFactSpace( + spaceIds: string[] | null, + spaceId: string, + checked: boolean, +): string[] | null { + const current = spaceIds ?? []; + const next = checked + ? [...current.filter((id) => id !== spaceId), spaceId] + : current.filter((id) => id !== spaceId); + return next.length > 0 ? next : null; +} + +/** Names of the spaces a limited fact shows in, in the order of `spaces`; unknown ids are skipped. */ +export function factSpaceNames( + spaceIds: string[] | null, + spaces: ReadonlyArray<{ id: string; name: string }>, +): string[] { + if (!spaceIds) return []; + return spaces.filter((space) => spaceIds.includes(space.id)).map((space) => space.name); +} diff --git a/packages/db/src/user-profile.postgres.test.ts b/packages/db/src/user-profile.postgres.test.ts index 03e10b0..d1c218c 100644 --- a/packages/db/src/user-profile.postgres.test.ts +++ b/packages/db/src/user-profile.postgres.test.ts @@ -179,6 +179,66 @@ describePostgres("user profile (PostgreSQL)", () => { }); }); + it("edits a fact in place, refusing a wording another fact already has", async () => { + const { userId, homeSpaceId } = await newUser(); + const work = await createSpaceForMember(prisma, { + currentSpaceId: homeSpaceId, + userId, + name: "Work", + }); + const [tea, coffee] = await updateUserProfile(prisma, userId, { + addFacts: [ + { category: "preferences", value: "Prefers tea", source: "user" }, + { category: "preferences", value: "Drinks coffee at work", source: "conversation" }, + ], + }); + await prisma.userFact.update({ + where: { id: tea!.id }, + data: { embedding: [0.1, 0.2], embeddingModel: "test-embedding" }, + }); + + // Same wording, new category and visibility: the embedding still fits. + await updateUserProfile(prisma, userId, { + editFacts: [{ id: tea!.id, value: " Prefers tea ", category: "habits", spaceIds: [work.id] }], + }); + expect(await prisma.userFact.findUniqueOrThrow({ where: { id: tea!.id } })).toMatchObject({ + value: "Prefers tea", + category: "habits", + allSpaces: false, + embedding: [0.1, 0.2], + embeddingModel: "test-embedding", + }); + expect( + (await getUserProfileState(prisma, userId)).facts.find((fact) => fact.id === tea!.id), + ).toMatchObject({ allSpaces: false, spaceIds: [work.id] }); + + for (const value of ["“prefers TEA!”", "..."]) { + await expect( + updateUserProfile(prisma, userId, { + editFacts: [ + { id: coffee!.id, category: "work" }, + { id: coffee!.id, value }, + ], + }), + ).rejects.toBeInstanceOf(InvalidUserFactError); + } + // The whole update failed, the category change included. + expect(await prisma.userFact.findUniqueOrThrow({ where: { id: coffee!.id } })).toMatchObject({ + value: "Drinks coffee at work", + category: "preferences", + }); + + // A fact may take a wording that an earlier edit in the same update gave up. + await updateUserProfile(prisma, userId, { + editFacts: [ + { id: tea!.id, value: "Prefers green tea" }, + { id: coffee!.id, value: "prefers tea" }, + ], + }); + const values = (await getUserProfileState(prisma, userId)).facts.map((fact) => fact.value); + expect(values.sort()).toEqual(["Prefers green tea", "prefers tea"]); + }); + it("limits a fact to the user's own spaces", async () => { const owner = await newUser(); const other = await newUser("Grace"); diff --git a/packages/db/src/user-profile.ts b/packages/db/src/user-profile.ts index c8f80f3..da24418 100644 --- a/packages/db/src/user-profile.ts +++ b/packages/db/src/user-profile.ts @@ -250,36 +250,54 @@ async function addFacts( return results; } -async function editFact( +/** + * Applies the user's fact edits in order. A new wording must still say something and must not + * repeat another fact, compared the way new facts are. + */ +async function editFacts( tx: Prisma.TransactionClient, userId: string, - edit: UserFactEdit, + edits: UserFactEdit[], ): Promise { - const fact = await tx.userFact.findFirst({ - where: { id: edit.id, userId }, - select: { id: true }, + if (edits.length === 0) return; + const facts = await tx.userFact.findMany({ + where: { userId }, + select: { id: true, value: true }, }); - if (!fact) throw new UserFactNotFoundError(); - const data: Prisma.UserFactUpdateInput = {}; - if (edit.category !== undefined) { - data.category = edit.category; - data.sensitive = isSensitiveUserFactCategory(edit.category); + const values = new Map(facts.map((fact) => [fact.id, fact.value])); + for (const edit of edits) { + const current = values.get(edit.id); + if (current === undefined) throw new UserFactNotFoundError(); + const data: Prisma.UserFactUpdateInput = {}; + if (edit.category !== undefined) { + data.category = edit.category; + data.sensitive = isSensitiveUserFactCategory(edit.category); + } + if (edit.value !== undefined && edit.value.trim() !== current) { + const value = edit.value.trim(); + const key = normalizeUserFactValue(value); + if (!key) throw new InvalidUserFactError("Fact is empty"); + for (const [id, other] of values) { + if (id !== edit.id && normalizeUserFactValue(other) === key) { + throw new InvalidUserFactError("This fact already exists"); + } + } + values.set(edit.id, value); + data.value = value; + // The stored embedding described the old wording. + data.embedding = []; + data.embeddingModel = null; + } + if (edit.spaceIds !== undefined) { + const spaces = visibleSpacesOf(edit.spaceIds); + data.allSpaces = spaces === null; + data.visibleSpaces = { + deleteMany: {}, + ...(spaces ? { create: spaces.map((spaceId) => ({ spaceId })) } : {}), + }; + } + if (Object.keys(data).length > 0) await tx.userFact.update({ where: { id: edit.id }, data }); } - if (edit.value !== undefined) { - data.value = edit.value.trim(); - // The stored embedding described the old wording. - data.embedding = []; - data.embeddingModel = null; - } - if (edit.spaceIds !== undefined) { - const spaces = visibleSpacesOf(edit.spaceIds); - data.allSpaces = spaces === null; - data.visibleSpaces = { - deleteMany: {}, - ...(spaces ? { create: spaces.map((spaceId) => ({ spaceId })) } : {}), - }; - } - await tx.userFact.update({ where: { id: fact.id }, data }); } async function confirmFacts( @@ -341,7 +359,7 @@ export async function updateUserProfile( }); } const added = await addFacts(tx, userId, update.addFacts ?? []); - for (const edit of update.editFacts ?? []) await editFact(tx, userId, edit); + await editFacts(tx, userId, update.editFacts ?? []); await confirmFacts(tx, userId, update.confirmFactIds ?? []); return added; }); diff --git a/packages/testkit/src/user-profile.postgres.test.ts b/packages/testkit/src/user-profile.postgres.test.ts index 11b3ede..462e461 100644 --- a/packages/testkit/src/user-profile.postgres.test.ts +++ b/packages/testkit/src/user-profile.postgres.test.ts @@ -161,6 +161,47 @@ describeWithDatabase("user profile through the API and the main bot", () => { expect(forgotten.facts).toEqual([]); expect(forgotten.profile).toMatchObject({ timeZone: null, language: null, summary: "" }); }); + + it("edits a fact's wording, category and spaces for its owner only", async () => { + const cookie = await signup(app, `profile-edit-${stamp}@rakazo.test`, "Profile Edit"); + const intruder = await signup(app, `profile-edit-other-${stamp}@rakazo.test`, "Intruder"); + const side = await rpc<{ id: string }>(app, cookie, "spaces/create", { name: "Side" }); + const elsewhere = await rpc<{ id: string }>(app, intruder, "spaces/create", { name: "Theirs" }); + const added = await rpc(app, cookie, "profile/update", { + addFacts: [ + { category: "preferences", value: "Prefers tea" }, + { category: "work", value: "Works late" }, + ], + }); + const tea = added.facts.find((fact) => fact.value === "Prefers tea")!; + + const edited = await rpc(app, cookie, "profile/update", { + editFacts: [ + { id: tea.id, value: "Prefers green tea", category: "habits", spaceIds: [side.id] }, + ], + }); + expect(edited.facts.find((fact) => fact.id === tea.id)).toMatchObject({ + value: "Prefers green tea", + category: "habits", + allSpaces: false, + spaceIds: [side.id], + }); + + const edit = (body: unknown, who = cookie) => raw(app, who, "profile/update", body); + expect((await edit({ editFacts: [{ id: tea.id, value: "works late!" }] })).status).toBe(400); + expect((await edit({ editFacts: [{ id: tea.id, spaceIds: [elsewhere.id] }] })).status).toBe( + 400, + ); + expect((await edit({ editFacts: [{ id: tea.id, value: "Stolen" }] }, intruder)).status).toBe( + 404, + ); + expect((await rpc(app, intruder, "profile/get")).facts).toEqual([]); + expect( + (await rpc(app, cookie, "profile/get")).facts.find( + (fact) => fact.id === tea.id, + ), + ).toMatchObject({ value: "Prefers green tea", spaceIds: [side.id] }); + }); }); async function signup(app: App, email: string, name: string) {