From 33ca8efb0f1cd2a662486b3f03e48bfe6b931726 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Wed, 28 Oct 2026 08:58:11 +0100 Subject: [PATCH] fix: keep shared tags safe when members edit at once or the server fails The first version wrote the whole tag list on every change, guessed a group when the server wouldn't say, and treated a failed read like a server without metadata. Now: - Definitions are a map by tag id under `tags`, and each change patches one tag (`metadata/org.bulwarkmail.tags/tags/`), so two members adding tags at once both keep theirs. Fields and colors this version doesn't know survive. The `{ list }` and bare-array shapes are still read and converted by the first save. - Shared tags appear only for confirmed group principals. A failed principal lookup keeps the user's own tags and asks again later. - "No metadata advertised" (0.16) and "the read failed" are separate. A failed read keeps the last good tags; a failed save keeps the change, says so, and sends it with the next good read after the client's pause. - Push reloads wait for 5 s of quiet and skip the echo of our own save; a returning tab reloads definitions only, and the scan for unnamed tags runs once per sign-in and then at most every 30 minutes. - The editor names its buttons after the tag, shows the delete warning and 44 px buttons on phones, returns focus to the edit button, and asks for Inbox rights only for confirmed groups. --- .../org/sections/shared-tags-section.tsx | 2 +- .../__tests__/shared-tag-settings.test.tsx | 123 +++++-- components/settings/shared-tag-settings.tsx | 149 +++++--- hooks/__tests__/use-account-tags.test.tsx | 8 +- lib/__tests__/shared-tags.test.ts | 119 ++++--- lib/jmap/bulwark-metadata.ts | 17 + lib/jmap/client-interface.ts | 2 + lib/jmap/client.ts | 41 ++- lib/shared-tags.ts | 183 ++++++---- locales/de/common.json | 13 +- locales/en/common.json | 13 +- stores/__tests__/shared-tags-store.test.ts | 323 +++++++++++++----- stores/shared-tags-store.ts | 310 ++++++++++++----- 13 files changed, 922 insertions(+), 381 deletions(-) diff --git a/components/admin/org/sections/shared-tags-section.tsx b/components/admin/org/sections/shared-tags-section.tsx index 4bd0c35d5..8b19486e1 100644 --- a/components/admin/org/sections/shared-tags-section.tsx +++ b/components/admin/org/sections/shared-tags-section.tsx @@ -15,7 +15,7 @@ export function SharedTagsSection({ draft }: { draft: OrgPolicyDraft }) {
{restrictionKeysForPage('org-sharing') diff --git a/components/settings/__tests__/shared-tag-settings.test.tsx b/components/settings/__tests__/shared-tag-settings.test.tsx index 114a61cb4..6d582d9c9 100644 --- a/components/settings/__tests__/shared-tag-settings.test.tsx +++ b/components/settings/__tests__/shared-tag-settings.test.tsx @@ -1,23 +1,31 @@ -import { fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { act, fireEvent, render, screen, waitFor } from '@testing-library/react'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +// Names with their tag in them: the global mock returns the bare key. +vi.mock('next-intl', () => ({ + useTranslations: () => (key: string, params?: { name?: string }) => (params?.name ? `${key}:${params.name}` : key), + useLocale: () => 'en', +})); + import { SharedTagSettings, maySetSharedTags } from '../shared-tag-settings'; import { DEFAULT_POLICY } from '@/lib/admin/types'; import { useAuthStore } from '@/stores/auth-store'; import { useEmailStore } from '@/stores/email-store'; import { usePolicyStore } from '@/stores/policy-store'; import { useSettingsStore } from '@/stores/settings-store'; -import { resetSharedTags } from '@/stores/shared-tags-store'; +import { resetSharedTags, sharedTagsKey, useSharedTagsStore } from '@/stores/shared-tags-store'; // Settings > Folders & tags > Shared tags: the editor of each group // mailbox's tags, read-only when the admin locked them, and a note instead of -// an editor where the server keeps no metadata. +// an editor where the server keeps no metadata or couldn't be read. -const URGENT = { id: 'urgent', label: 'Urgent', color: 'red' }; +const URGENT = { label: 'Urgent', color: 'red' }; const RIGHTS = { mayReadItems: true, mayAddItems: true, mayRemoveItems: true, maySetSeen: true, maySetKeywords: true, mayCreateChild: true, mayRename: true, mayDelete: true, maySubmit: true }; -let stored: Record; +let stored: Record | null; let keywords: Record; -let metadata: boolean; +let advertised: boolean; +let principalType: string; const client = { getAccountId: () => 'me', getAccountInfo: (id: string) => ({ isPersonal: id === 'me' }), @@ -25,12 +33,21 @@ const client = { { id: 'me', name: 'org-alice@example.org', isPrimary: true, capabilities: { mail: true } }, { id: 'team', name: 'org-team@example.org', isPrimary: false, capabilities: { mail: true } }, ], - getPrincipalTypes: async () => ({ team: 'group' }), - supportsBulwarkMetadata: () => metadata, - getBulwarkMetadata: vi.fn(async () => stored), - setBulwarkMetadata: vi.fn(async (_id: string | undefined, key: string, value: unknown) => { stored = { ...stored, [key]: value }; }), + getPrincipalTypes: async () => ({ team: principalType }), + advertisesBulwarkMetadata: () => advertised, + supportsBulwarkMetadata: () => advertised, + getBulwarkMetadata: vi.fn(async () => (stored ? { tags: stored } : null)), + patchBulwarkMetadata: vi.fn(async (_id: string | undefined, _key: string, patches: Array<{ path: string[]; value: unknown }>) => { + const tags = { ...((stored?.tags ?? {}) as Record) }; + for (const { path, value } of patches) { + if (value === null) delete tags[path[1]]; + else tags[path[1]] = value; + } + stored = { tags }; + return { newState: 'sX' }; + }), discoverKeywords: async () => ({ keywords, scanned: 0, total: 0, complete: true }), - getMailboxes: async () => [], + getMailboxes: vi.fn(async () => []), }; function setRights(rights: Record) { @@ -43,11 +60,13 @@ function setLocked(locked: boolean) { beforeEach(() => { resetSharedTags(); - stored = { tags: { list: [URGENT] } }; + stored = { tags: { urgent: { ...URGENT } } }; keywords = {}; - metadata = true; + advertised = true; + principalType = 'group'; client.getBulwarkMetadata.mockClear(); - client.setBulwarkMetadata.mockClear(); + client.patchBulwarkMetadata.mockClear(); + client.getMailboxes.mockClear(); useSettingsStore.setState({ emailKeywords: [], nestedTags: false }); useAuthStore.setState({ client: client as never, @@ -60,26 +79,53 @@ beforeEach(() => { afterEach(() => resetSharedTags()); describe('SharedTagSettings', () => { - it('lets a member who may change the Inbox add a shared tag', async () => { + it('lets a member who may change the Inbox add a shared tag, as one patch', async () => { render(); expect(await screen.findByText('Urgent')).toBeInTheDocument(); fireEvent.click(screen.getByText('add')); fireEvent.change(screen.getByLabelText('name'), { target: { value: 'Waiting' } }); fireEvent.click(screen.getByText('save')); - await waitFor(() => expect(client.setBulwarkMetadata).toHaveBeenCalledWith('team', 'tags', { - list: [URGENT, { id: 'waiting', label: 'Waiting', color: 'blue' }], - })); + await waitFor(() => expect(client.patchBulwarkMetadata).toHaveBeenCalledWith('team', 'tags', [ + { path: ['tags', 'waiting'], value: { label: 'Waiting', color: 'blue' } }, + ])); expect(await screen.findByText('Waiting')).toBeInTheDocument(); }); + it('names the buttons after their tag and puts focus back on the edit button after saving', async () => { + render(); + fireEvent.click(await screen.findByLabelText('edit_tag:Urgent')); + fireEvent.change(screen.getByLabelText('name'), { target: { value: 'Very urgent' } }); + fireEvent.click(screen.getByText('save')); + // The color wasn't picked, so it isn't sent: whatever is stored stays. + await waitFor(() => expect(client.patchBulwarkMetadata).toHaveBeenCalledWith('team', 'tags', [ + { path: ['tags', 'urgent'], value: { label: 'Very urgent', color: 'red' } }, + ])); + await waitFor(() => expect(document.activeElement).toBe(screen.getByLabelText('edit_tag:Very urgent'))); + }); + + it('warns that deleting is for everyone, in words, before it deletes', async () => { + render(); + fireEvent.click(await screen.findByLabelText('delete_tag:Urgent')); + const confirm = screen.getByTestId('shared-tag-delete-confirm'); + const warning = screen.getByText('delete_confirm'); + expect(confirm).toContainElement(warning); + // Shown at every width, not only from the sm breakpoint up. + expect(warning.className).not.toMatch(/(^|\s)hidden(\s|$)/); + fireEvent.click(screen.getByText('delete')); + await waitFor(() => expect(client.patchBulwarkMetadata).toHaveBeenCalledWith('team', 'tags', [{ path: ['tags', 'urgent'], value: null }])); + }); + it('shows the tags read-only with "Set by your organisation" when the admin locked them', async () => { setLocked(true); + keywords = { '$label:found': 1 }; const { container } = render(); expect(await screen.findByText('Urgent')).toBeInTheDocument(); + await screen.findByText('found_on_messages'); expect(container.querySelector('[data-org-locked]')).not.toBeNull(); expect(screen.queryByText('add')).not.toBeInTheDocument(); - expect(screen.queryByLabelText('edit')).not.toBeInTheDocument(); - expect(screen.queryByLabelText('delete')).not.toBeInTheDocument(); + expect(screen.queryByLabelText(/^edit_tag/)).not.toBeInTheDocument(); + expect(screen.queryByLabelText(/^delete_tag/)).not.toBeInTheDocument(); + expect(screen.queryByLabelText(/^name_it/)).not.toBeInTheDocument(); }); it('is read-only with a note for a member without the right to change the Inbox', async () => { @@ -89,23 +135,50 @@ describe('SharedTagSettings', () => { expect(screen.queryByText('add')).not.toBeInTheDocument(); }); + it('asks for no rights of a mailbox a person shared', async () => { + principalType = 'individual'; + useEmailStore.setState({ mailboxes: [] }); + render(); + await waitFor(() => expect(useSharedTagsStore.getState().entries[sharedTagsKey('login-1', 'team')]?.status).toBe('personal')); + expect(client.getMailboxes).not.toHaveBeenCalled(); + expect(screen.queryByTestId('shared-tags-account')).not.toBeInTheDocument(); + }); + it('says in one line that the server keeps no shared tags, with no editor', async () => { - metadata = false; + advertised = false; render(); expect(await screen.findByTestId('shared-tags-unsupported')).toHaveTextContent('unsupported'); expect(screen.queryByText('add')).not.toBeInTheDocument(); expect(client.getBulwarkMetadata).not.toHaveBeenCalled(); }); + it('says so in one line when the shared tags couldn\'t be read', async () => { + stored = null; + render(); + expect(await screen.findByTestId('shared-tags-unavailable')).toHaveTextContent('unavailable'); + expect(screen.queryByText('add')).not.toBeInTheDocument(); + }); + + it('keeps a change the server didn\'t take and says it will try again', async () => { + render(); + await screen.findByText('Urgent'); + client.patchBulwarkMetadata.mockRejectedValueOnce(new TypeError('Failed to fetch')); + fireEvent.click(screen.getByText('add')); + fireEvent.change(screen.getByLabelText('name'), { target: { value: 'Waiting' } }); + await act(async () => { fireEvent.click(screen.getByText('save')); }); + expect(await screen.findByTestId('shared-tags-pending')).toHaveTextContent('save_pending'); + expect(screen.getByText('Waiting')).toBeInTheDocument(); + }); + it('offers to name a tag found on the messages, keeping its id', async () => { keywords = { '$label:waiting-for-customer': 2 }; render(); - fireEvent.click(await screen.findByLabelText('name_it')); + fireEvent.click(await screen.findByLabelText('name_it:waiting-for-customer')); fireEvent.change(screen.getByLabelText('name'), { target: { value: 'Customer' } }); fireEvent.click(screen.getByText('save')); - await waitFor(() => expect(client.setBulwarkMetadata).toHaveBeenCalledWith('team', 'tags', { - list: [URGENT, { id: 'waiting-for-customer', label: 'Customer', color: expect.any(String) }], - })); + await waitFor(() => expect(client.patchBulwarkMetadata).toHaveBeenCalledWith('team', 'tags', [ + { path: ['tags', 'waiting-for-customer'], value: { label: 'Customer', color: expect.any(String) } }, + ])); }); }); diff --git a/components/settings/shared-tag-settings.tsx b/components/settings/shared-tag-settings.tsx index 1301b5259..7b7da3701 100644 --- a/components/settings/shared-tag-settings.tsx +++ b/components/settings/shared-tag-settings.tsx @@ -1,6 +1,6 @@ "use client"; -import { useEffect, useMemo, useState } from "react"; +import { useEffect, useMemo, useRef, useState } from "react"; import { useTranslations } from "next-intl"; import { Check, Loader2, Pencil, Plus, Trash2, Users, X } from "@/components/icons"; import { TagBadge } from "@/components/email/tag-badge"; @@ -9,12 +9,12 @@ import { useRestriction } from "@/hooks/use-restriction"; import { MetadataTooLargeError } from "@/lib/jmap/bulwark-metadata"; import type { MailboxRights } from "@/lib/jmap/types"; import { composeKeywordId, MAX_KEYWORD_ID_LENGTH } from "@/lib/keyword-nesting"; -import { MAX_SHARED_TAG_LABEL, groupAccountTags, removeSharedTag, upsertSharedTag } from "@/lib/shared-tags"; +import { MAX_SHARED_TAG_LABEL, groupAccountTags, type SharedTagChange } from "@/lib/shared-tags"; import { cn } from "@/lib/utils"; import { useAuthStore } from "@/stores/auth-store"; import { useEmailStore } from "@/stores/email-store"; -import { useSettingsStore, type KeywordDefinition } from "@/stores/settings-store"; -import { saveSharedTags, sharedTagsKey, useSharedTagsStore } from "@/stores/shared-tags-store"; +import { KEYWORD_PALETTE, useSettingsStore, type KeywordDefinition } from "@/stores/settings-store"; +import { SharedTagsSaveDeferredError, saveSharedTags, sharedTagsKey, useSharedTagsStore } from "@/stores/shared-tags-store"; import { toast } from "@/stores/toast-store"; import { toUnicodeEmail } from "@/lib/idn"; import { KeywordColorPicker } from "./keyword-settings"; @@ -28,13 +28,16 @@ export function maySetSharedTags(rights: Partial | null | undefin return typeof rights.maySetMetadata === "boolean" ? rights.maySetMetadata : rights.mayRename === true; } -/** The user's rights on the Inbox of a group account, from the folder list or asked once. */ -function useInboxRights(scope: TagScope): Partial | null | undefined { +/** + * The user's rights on the Inbox of a confirmed group account, from the + * folder list or asked once. Nothing is asked for any other account. + */ +function useInboxRights(scope: TagScope, enabled: boolean): Partial | null | undefined { const fromStore = useEmailStore((state) => state.mailboxes.find((mailbox) => mailbox.accountId === scope.accountId && mailbox.role === "inbox")?.myRights); const [fetched, setFetched] = useState | null | undefined>(undefined); useEffect(() => { - if (fromStore) return; + if (!enabled || fromStore) return; let cancelled = false; const client = clientForLogin(scope.loginId) as unknown as { getMailboxes?: (accountId?: string) => Promise> } | null; if (!client?.getMailboxes) return; @@ -42,8 +45,8 @@ function useInboxRights(scope: TagScope): Partial | null | undefi .then((mailboxes) => { if (!cancelled) setFetched(mailboxes.find((m) => m.role === "inbox")?.myRights ?? null); }) .catch(() => { if (!cancelled) setFetched(null); }); return () => { cancelled = true; }; - }, [fromStore, scope.loginId, scope.accountId]); - return fromStore ?? fetched; + }, [enabled, fromStore, scope.loginId, scope.accountId]); + return enabled ? fromStore ?? fetched : undefined; } function SharedTagForm({ @@ -59,17 +62,22 @@ function SharedTagForm({ fixedId?: string; takenIds: string[]; saving: boolean; - onSave: (tag: KeywordDefinition) => void; + /** `color` only when it was picked (or the tag is new), so a color this version doesn't know survives a rename. */ + onSave: (id: string, value: { label: string; color?: string }) => void; onCancel: () => void; }) { const t = useTranslations("shared_tags"); const [label, setLabel] = useState(initial?.label ?? ""); const [color, setColor] = useState(initial?.color ?? "blue"); + const [colorPicked, setColorPicked] = useState(false); const id = fixedId ?? composeKeywordId(null, label); const duplicate = !fixedId && id.length > 0 && takenIds.includes(id.toLowerCase()); const tooLong = !fixedId && id.length > MAX_KEYWORD_ID_LENGTH; const valid = label.trim().length > 0 && id.length > 0 && !duplicate && !tooLong; - const save = () => { if (valid && !saving) onSave({ id, label: label.trim(), color }); }; + const keepsColor = !!initial && !colorPicked; + const save = () => { + if (valid && !saving) onSave(id, keepsColor ? { label: label.trim() } : { label: label.trim(), color }); + }; return (
@@ -94,14 +102,14 @@ function SharedTagForm({ {tooLong &&

{t("too_long")}

}
-

{t("colour")}

- +

{t("color")}

+ { setColor(next); setColorPicked(true); }} />
-
@@ -235,10 +272,11 @@ function SharedAccountTagEditor({ scope, name }: { scope: TagScope; name: string
@@ -246,9 +284,9 @@ function SharedAccountTagEditor({ scope, name }: { scope: TagScope; name: string @@ -268,6 +306,9 @@ function SharedAccountTagEditor({ scope, name }: { scope: TagScope; name: string )} {shared.map((tag) => row(tag, false))} {discovered.map((tag) => row(tag, true))} + {(entry?.pending.length ?? 0) > 0 && ( +

{t("save_pending")}

+ )} {locked ? ( ) : !canWrite ? ( @@ -279,14 +320,14 @@ function SharedAccountTagEditor({ scope, name }: { scope: TagScope; name: string takenIds={takenIds} saving={saving} onCancel={() => setEditing(null)} - onSave={async (next) => { if (await save((list) => upsertSharedTag(list, next))) setEditing(null); }} + onSave={async (id, value) => { if (await save({ id, value })) { setEditing(null); setFocusId(id); } }} /> ) : (