From 01e5cd69cf19b9170c1aa256635feb7686ca6472 Mon Sep 17 00:00:00 2001 From: Stefan Hildebrandt <695494+hildebrandttk@users.noreply.github.com> Date: Sat, 27 Jun 2026 13:07:48 +0200 Subject: [PATCH] fix(identity): sync default sender identity per account (#507) The default sender identity (`preferredPrimaryId`) lived only in the browser-local `identity-storage` store and was never written to the synced settings, so the choice was lost on clearing site data / switching browsers and never appeared in exported settings. Persist it in the synced settings store, keyed **per account** (`preferredIdentityIds: Record`), mirroring the existing per-account `allMailFolderIds`. Per-account keying is required because JMAP identity ids are account-scoped and would otherwise collide across accounts / the unified mailbox. This supersedes the earlier username-keyed fix that had landed on main: the username-keyed map, `loadIdentities()` fallback write, and the `applyPreferredIdentityOrdering` store action (plus its settings-store hook) are removed so a single account-keyed mechanism remains. - settings-store: `preferredIdentityIds` (accountId -> identityId) in state, defaults, export, import (non-record guard), rehydrate coercion, v6 migration. - auth-store: `applyPreferredIdentity(accountId?)` reorders the active account's identities once synced settings load, and performs the one-time migration of the pre-#507 browser-local default into the synced map (keyed by accountId). Invoked from every `loadFromServer().finally()` (login / OAuth / SSO / switch / restore). `loadIdentities()` now only applies the local fallback ordering. - identity-manager-modal: the star action writes the choice by `activeAccountId`. - identity-store: `preferredPrimaryId` kept as a local (sync-off) fallback. - tests: per-account independence, export/import round-trip, import guard, and applyPreferredIdentity reorder / active-account gating / local-default migration. --- .../identity/identity-manager-modal.tsx | 14 ++- .../apply-preferred-identity.test.ts | 90 +++++++++++++++ .../settings-store-preferred-identity.test.ts | 58 ++++++++++ stores/auth-store.ts | 104 +++++++++++------- stores/identity-store.ts | 9 +- stores/settings-store.ts | 42 ++++--- 6 files changed, 249 insertions(+), 68 deletions(-) create mode 100644 stores/__tests__/apply-preferred-identity.test.ts create mode 100644 stores/__tests__/settings-store-preferred-identity.test.ts diff --git a/components/identity/identity-manager-modal.tsx b/components/identity/identity-manager-modal.tsx index 521ac5f1..5f42a563 100644 --- a/components/identity/identity-manager-modal.tsx +++ b/components/identity/identity-manager-modal.tsx @@ -9,6 +9,7 @@ import { ConfirmDialog } from '@/components/ui/confirm-dialog'; import { IdentityForm } from './identity-form'; import { useIdentityStore } from '@/stores/identity-store'; import { useAuthStore } from '@/stores/auth-store'; +import { useAccountStore } from '@/stores/account-store'; import { useSettingsStore } from '@/stores/settings-store'; function useSyncIdentities() { @@ -207,15 +208,16 @@ export function IdentityManagerModal({ isOpen, onClose }: IdentityManagerModalPr const handleSetPrimary = useCallback((identity: Identity) => { setPreferredPrimary(identity.id); - // Persist to the synced settings (keyed by username, matching how - // loadIdentities reads it back) so the choice survives a new browser / - // cleared site data and reaches other devices (#507). - const username = useAuthStore.getState().username || ''; - if (username) { + // Persist the choice per account in the synced settings store so it + // survives clearing site data, follows the user across devices, and shows + // up in exported settings (issue #507). JMAP identity ids are account- + // scoped, so the default is keyed by the active account. + const activeAccountId = useAccountStore.getState().activeAccountId; + if (activeAccountId) { const current = useSettingsStore.getState().preferredIdentityIds; useSettingsStore.getState().updateSetting('preferredIdentityIds', { ...current, - [username]: identity.id, + [activeAccountId]: identity.id, }); } // Re-sort: move the preferred identity to the front diff --git a/stores/__tests__/apply-preferred-identity.test.ts b/stores/__tests__/apply-preferred-identity.test.ts new file mode 100644 index 00000000..6443a65f --- /dev/null +++ b/stores/__tests__/apply-preferred-identity.test.ts @@ -0,0 +1,90 @@ +import { describe, it, expect, beforeEach } from 'vitest'; +import { applyPreferredIdentity, useAuthStore } from '../auth-store'; +import { useIdentityStore } from '../identity-store'; +import { useAccountStore } from '../account-store'; +import { useSettingsStore } from '../settings-store'; +import type { Identity } from '@/lib/jmap/types'; + +const makeIdentity = (overrides: Partial = {}): Identity => ({ + id: 'id-1', + name: 'Test User', + email: 'test@example.com', + mayDelete: true, + ...overrides, +}); + +const IDS = [ + makeIdentity({ id: 'id-1', name: 'Alice', email: 'alice@example.com' }), + makeIdentity({ id: 'id-2', name: 'Bob', email: 'bob@example.com' }), + makeIdentity({ id: 'id-3', name: 'Carol', email: 'carol@example.com' }), +]; + +/** + * applyPreferredIdentity() is the single mechanism that honours the synced, + * per-account default sender identity (#507). These tests drive the real + * zustand stores directly (as the other auth-store tests do). + */ +describe('applyPreferredIdentity (issue #507)', () => { + beforeEach(() => { + useIdentityStore.setState({ identities: [...IDS], preferredPrimaryId: null }); + useAuthStore.setState({ identities: [...IDS], primaryIdentity: IDS[0] }); + useAccountStore.setState({ activeAccountId: 'acc-1' }); + useSettingsStore.setState({ preferredIdentityIds: {} }); + }); + + it('reorders the active account so the synced preferred identity is primary', () => { + useSettingsStore.setState({ preferredIdentityIds: { 'acc-1': 'id-3' } }); + + applyPreferredIdentity('acc-1'); + + expect(useAuthStore.getState().identities.map((i) => i.id)).toEqual(['id-3', 'id-1', 'id-2']); + expect(useAuthStore.getState().primaryIdentity?.id).toBe('id-3'); + expect(useIdentityStore.getState().preferredPrimaryId).toBe('id-3'); + }); + + it('defaults to the active account when no accountId is passed', () => { + useSettingsStore.setState({ preferredIdentityIds: { 'acc-1': 'id-2' } }); + + applyPreferredIdentity(); + + expect(useAuthStore.getState().identities[0].id).toBe('id-2'); + }); + + it('is a no-op when the target is not the active account', () => { + useSettingsStore.setState({ preferredIdentityIds: { 'acc-2': 'id-3' } }); + + applyPreferredIdentity('acc-2'); + + // active account's live ordering must be untouched + expect(useAuthStore.getState().identities.map((i) => i.id)).toEqual(['id-1', 'id-2', 'id-3']); + }); + + it('is a no-op when the account has no synced default and no local fallback', () => { + applyPreferredIdentity('acc-1'); + + expect(useAuthStore.getState().identities.map((i) => i.id)).toEqual(['id-1', 'id-2', 'id-3']); + expect(useSettingsStore.getState().preferredIdentityIds).toEqual({}); + }); + + it('migrates the pre-#507 browser-local default into the synced map, keyed by accountId', () => { + // No synced entry, but a local (identity-storage) preferred primary exists. + useIdentityStore.setState({ preferredPrimaryId: 'id-2' }); + + applyPreferredIdentity('acc-1'); + + // adopted, persisted per account, and applied to the live ordering + expect(useSettingsStore.getState().preferredIdentityIds).toEqual({ 'acc-1': 'id-2' }); + expect(useAuthStore.getState().identities[0].id).toBe('id-2'); + }); + + it('prefers the synced value over the local fallback', () => { + useIdentityStore.setState({ preferredPrimaryId: 'id-2' }); + useSettingsStore.setState({ preferredIdentityIds: { 'acc-1': 'id-3' } }); + + applyPreferredIdentity('acc-1'); + + expect(useAuthStore.getState().identities[0].id).toBe('id-3'); + // the synced value is not overwritten by the migration + expect(useSettingsStore.getState().preferredIdentityIds).toEqual({ 'acc-1': 'id-3' }); + }); +}); diff --git a/stores/__tests__/settings-store-preferred-identity.test.ts b/stores/__tests__/settings-store-preferred-identity.test.ts new file mode 100644 index 00000000..230efad1 --- /dev/null +++ b/stores/__tests__/settings-store-preferred-identity.test.ts @@ -0,0 +1,58 @@ +import { describe, it, expect, beforeEach } from 'vitest'; +import { useSettingsStore } from '../settings-store'; + +describe('settings-store per-account preferredIdentityIds (issue #507)', () => { + beforeEach(() => { + useSettingsStore.setState({ preferredIdentityIds: {} }); + }); + + it('defaults to an empty record (no account has a synced default)', () => { + expect(useSettingsStore.getState().preferredIdentityIds).toEqual({}); + }); + + it('keeps each account default independent', () => { + useSettingsStore.setState({ + preferredIdentityIds: { 'acct-1': 'b', 'acct-2': 'c' }, + }); + const map = useSettingsStore.getState().preferredIdentityIds; + expect(map['acct-1']).toBe('b'); + expect(map['acct-2']).toBe('c'); + expect(map['acct-3']).toBeUndefined(); + }); + + it('round-trips through export -> import so the choice survives clearing site data', () => { + useSettingsStore.setState({ preferredIdentityIds: { 'acct-1': 'b' } }); + const json = useSettingsStore.getState().exportSettings(); + // Appears in exported JSON (issue #507 acceptance criterion). + expect(JSON.parse(json).preferredIdentityIds).toEqual({ 'acct-1': 'b' }); + + // Simulate a fresh browser: clear, then import the exported settings. + useSettingsStore.setState({ preferredIdentityIds: {} }); + expect(useSettingsStore.getState().importSettings(json)).toBe(true); + expect(useSettingsStore.getState().preferredIdentityIds).toEqual({ 'acct-1': 'b' }); + }); + + describe('importSettings non-record guard', () => { + it('ignores a legacy array shape', () => { + useSettingsStore.setState({ preferredIdentityIds: { 'acct-1': 'b' } }); + const ok = useSettingsStore.getState().importSettings( + JSON.stringify({ preferredIdentityIds: ['b'] }), + ); + expect(ok).toBe(true); + expect(useSettingsStore.getState().preferredIdentityIds).toEqual({ 'acct-1': 'b' }); + }); + + it('ignores a null value', () => { + useSettingsStore.setState({ preferredIdentityIds: { 'acct-1': 'b' } }); + useSettingsStore.getState().importSettings(JSON.stringify({ preferredIdentityIds: null })); + expect(useSettingsStore.getState().preferredIdentityIds).toEqual({ 'acct-1': 'b' }); + }); + + it('accepts a proper per-account record', () => { + useSettingsStore.getState().importSettings( + JSON.stringify({ preferredIdentityIds: { 'acct-9': 'a' } }), + ); + expect(useSettingsStore.getState().preferredIdentityIds).toEqual({ 'acct-9': 'a' }); + }); + }); +}); diff --git a/stores/auth-store.ts b/stores/auth-store.ts index 85b39ec6..c117dffc 100644 --- a/stores/auth-store.ts +++ b/stores/auth-store.ts @@ -49,7 +49,6 @@ interface AuthState { clearError: () => void; syncIdentities: () => void; refreshIdentities: () => Promise; - applyPreferredIdentityOrdering: () => void; getClientForAccount: (accountId: string) => JMAPClient | undefined; getAllConnectedClients: () => Map; } @@ -198,25 +197,16 @@ function sortIdentities(rawIdentities: Identity[], username: string): Identity[] } function loadIdentities(rawIdentities: Identity[], username: string): { identities: Identity[]; primaryIdentity: Identity | null } { - const settings = useSettingsStore.getState(); - const preferredMap = settings.preferredIdentityIds || {}; - let preferredPrimaryId = preferredMap[username] ?? null; - - // One-time migration: builds before #507 stored the preferred identity only - // in the browser-local identity-storage (never synced). If the synced - // settings have no entry for this account yet, adopt that legacy local value - // and write it into the synced settings so it persists across devices. - if (preferredPrimaryId == null) { - const legacy = useIdentityStore.getState().preferredPrimaryId; - if (legacy) { - preferredPrimaryId = legacy; - settings.updateSetting('preferredIdentityIds', { ...preferredMap, [username]: legacy }); - } - } + // The synced per-account default sender identity (#507) is keyed by + // AccountEntry.id and re-applied by applyPreferredIdentity() once + // loadFromServer resolves (the accountId isn't known here). At load time we + // only honour the browser-local fallback (identity-storage) so the ordering + // is stable before - or entirely without - settings sync. + const preferredPrimaryId = useIdentityStore.getState().preferredPrimaryId; const identities = sortIdentities(rawIdentities, username); - // If user has a preferred primary, move it to front + // If a local preferred primary is set, move it to the front. if (preferredPrimaryId) { const idx = identities.findIndex((id) => id.id === preferredPrimaryId); if (idx > 0) { @@ -227,12 +217,60 @@ function loadIdentities(rawIdentities: Identity[], username: string): { identiti const primaryIdentity = identities[0] ?? null; useIdentityStore.getState().setIdentities(identities); - // Mirror the resolved choice into the identity store so the identity-manager - // UI (the ⭐ marker) reflects the active account's preferred identity. - useIdentityStore.setState({ preferredPrimaryId }); return { identities, primaryIdentity }; } +/** + * Re-apply the per-account default sender identity once synced settings are + * available (issue #507). The choice is stored server-side in the settings + * store (`preferredIdentityIds`, keyed by AccountEntry.id), so it can only be + * applied after `loadFromServer` resolves. It reorders the account's identities + * so the preferred one is primary - the composer defaults its `From` to + * identities[0]. No-op when nothing is configured for the account. + * + * Also performs the one-time migration of the pre-#507 browser-local default + * (identity-storage) into the synced per-account map, keyed by accountId. + * + * @param accountId The account to apply for; defaults to the active account. + */ +export function applyPreferredIdentity(accountId?: string | null): void { + const targetId = accountId ?? useAccountStore.getState().activeAccountId; + if (!targetId) return; + + const idStore = useIdentityStore.getState(); + // Only touch the live identity store when it currently holds this account's + // identities (true for the active account). Switching snapshots/restores the + // ordering per account, so a background account's order is restored later. + // The local fallback below also belongs to the active account, so gate first. + if (useAccountStore.getState().activeAccountId !== targetId) return; + + let preferred = useSettingsStore.getState().preferredIdentityIds[targetId] ?? null; + + // One-time migration: before #507 the default lived only in the browser-local + // identity-storage (never synced). If the synced map has no entry for this + // account yet, adopt that local value and persist it (keyed by accountId) so + // it follows the user across devices. + if (!preferred) { + const legacy = idStore.preferredPrimaryId; + if (legacy) { + preferred = legacy; + const current = useSettingsStore.getState().preferredIdentityIds; + useSettingsStore.getState().updateSetting('preferredIdentityIds', { ...current, [targetId]: legacy }); + } + } + if (!preferred) return; + + idStore.setPreferredPrimary(preferred); + const ids = [...idStore.identities]; + const idx = ids.findIndex((i) => i.id === preferred); + if (idx > 0) { + const [p] = ids.splice(idx, 1); + ids.unshift(p); + idStore.setIdentities(ids); + } + useAuthStore.setState({ identities: ids, primaryIdentity: ids[0] ?? null }); +} + function getLocaleLoginPath(): string { if (typeof window === 'undefined') return '/en/login'; @@ -638,6 +676,7 @@ export const useAuthStore = create()( if (!config.settingsSyncEnabled) return; useSettingsStore.getState().loadFromServer(username, serverUrl).finally(() => { useSettingsStore.getState().enableSync(username, serverUrl); + applyPreferredIdentity(accountId); }); }).catch(() => {}); @@ -840,6 +879,7 @@ export const useAuthStore = create()( if (!config.settingsSyncEnabled) return; useSettingsStore.getState().loadFromServer(username, serverUrl).finally(() => { useSettingsStore.getState().enableSync(username, serverUrl); + applyPreferredIdentity(accountId); }); }).catch(() => {}); @@ -977,6 +1017,7 @@ export const useAuthStore = create()( if (!cfg.settingsSyncEnabled) return; useSettingsStore.getState().loadFromServer(username, ssoServerUrl).finally(() => { useSettingsStore.getState().enableSync(username, ssoServerUrl); + applyPreferredIdentity(accountId); }); }).catch(() => {}); @@ -1363,6 +1404,7 @@ export const useAuthStore = create()( if (!config.settingsSyncEnabled) return; useSettingsStore.getState().loadFromServer(targetAccount.username, targetAccount.serverUrl).finally(() => { useSettingsStore.getState().enableSync(targetAccount.username, targetAccount.serverUrl); + applyPreferredIdentity(targetAccount.id); }); }).catch(() => {}); }, @@ -1534,6 +1576,7 @@ export const useAuthStore = create()( if (!config.settingsSyncEnabled) return; useSettingsStore.getState().loadFromServer(targetAccount.username, targetAccount.serverUrl).finally(() => { useSettingsStore.getState().enableSync(targetAccount.username, targetAccount.serverUrl); + applyPreferredIdentity(targetAccount.id); }); }).catch(() => {}); return; @@ -1647,6 +1690,7 @@ export const useAuthStore = create()( if (!config.settingsSyncEnabled) return; useSettingsStore.getState().loadFromServer(state.username || '', state.serverUrl!).finally(() => { useSettingsStore.getState().enableSync(state.username || '', state.serverUrl!); + applyPreferredIdentity(accountId); }); }).catch(() => {}); return; @@ -1718,6 +1762,7 @@ export const useAuthStore = create()( if (!config.settingsSyncEnabled) return; useSettingsStore.getState().loadFromServer(username, serverUrl).finally(() => { useSettingsStore.getState().enableSync(username, serverUrl); + applyPreferredIdentity(accountId); }); }).catch(() => {}); return; @@ -1761,25 +1806,6 @@ export const useAuthStore = create()( set({ identities, primaryIdentity }); }, - // Re-sort the already-loaded identities to honor the active account's - // synced preferred-primary identity, without a network round-trip. Used - // after settings load from the server so a fresh browser reflects the - // synced default (#507). - applyPreferredIdentityOrdering: () => { - const { username, identities } = get(); - if (!username || identities.length === 0) return; - const preferredId = useSettingsStore.getState().preferredIdentityIds?.[username] ?? null; - useIdentityStore.setState({ preferredPrimaryId: preferredId }); - if (!preferredId) return; - const idx = identities.findIndex((id) => id.id === preferredId); - if (idx <= 0) return; // already first, or not present - const reordered = [...identities]; - const [preferred] = reordered.splice(idx, 1); - reordered.unshift(preferred); - useIdentityStore.getState().setIdentities(reordered); - set({ identities: reordered, primaryIdentity: reordered[0] ?? null }); - }, - refreshIdentities: async () => { const { client, username } = get(); if (!client || !username) return; diff --git a/stores/identity-store.ts b/stores/identity-store.ts index e34a3223..e233be78 100644 --- a/stores/identity-store.ts +++ b/stores/identity-store.ts @@ -123,7 +123,14 @@ export const useIdentityStore = create()( }), { name: 'identity-storage', - // Only persist sub-addressing data, not identities (they're server-side) + // Only persist sub-addressing data, not identities (they're server-side). + // The default sender identity (`preferredPrimaryId`) is the per-account + // value for the *active* account; it is kept here purely as a local + // fallback so the choice survives a reload when settings sync is off. + // The durable, cross-device, exportable source of truth is the synced + // settings store, keyed per account (`preferredIdentityIds`), which is + // re-applied via applyPreferredIdentity() once server settings load and + // overrides this value per account (issue #507). partialize: (state) => ({ subAddress: state.subAddress, preferredPrimaryId: state.preferredPrimaryId, diff --git a/stores/settings-store.ts b/stores/settings-store.ts index 1cd4ccc5..21e68627 100644 --- a/stores/settings-store.ts +++ b/stores/settings-store.ts @@ -178,13 +178,6 @@ interface SettingsState { requestReadReceiptDefault: boolean; // Pre-check "request read receipt" in the composer readReceiptResponse: ReadReceiptResponse; // How to respond to incoming read-receipt requests - // Identities - // Per-account default ("preferred primary") sender identity, keyed by - // username (the same key settings sync uses). A JMAP identity id is only - // meaningful within its own account, so this must be account-scoped. Synced - // so the choice survives a new browser / cleared site data (#507). - preferredIdentityIds: Record; - // Privacy & Security sessionTimeout: number; // minutes (0 = never) trustedSenders: string[]; // Email addresses that can load external content @@ -258,6 +251,12 @@ interface SettingsState { // explicit [] = "no folders". (Replaced the legacy global string[] | null.) allMailFolderIds: Record; + // Per-account default sender identity, keyed by AccountEntry.id -> JMAP + // Identity id. Synced (and exported) so the chosen default survives clearing + // site data and follows the user across browsers/devices (issue #507). Kept + // per account because JMAP identity ids are account-scoped and would collide. + preferredIdentityIds: Record; + // Email Display disableThreading: boolean; // Show emails as individual messages instead of grouped by conversation @@ -396,9 +395,6 @@ const DEFAULT_SETTINGS = { requestReadReceiptDefault: false, readReceiptResponse: 'ask' as ReadReceiptResponse, - // Identities - preferredIdentityIds: {} as Record, - // Privacy & Security sessionTimeout: 0, // Never trustedSenders: [] as string[], @@ -452,6 +448,7 @@ const DEFAULT_SETTINGS = { // All Mail view (gated) enableAllMailView: false, allMailFolderIds: {} as Record, + preferredIdentityIds: {} as Record, enableCrossUnreadView: false, enableCrossStarredView: false, @@ -609,7 +606,6 @@ export const useSettingsStore = create()( signatureSeparatorEnabled: state.signatureSeparatorEnabled, requestReadReceiptDefault: state.requestReadReceiptDefault, readReceiptResponse: state.readReceiptResponse, - preferredIdentityIds: state.preferredIdentityIds, sessionTimeout: state.sessionTimeout, emailNotificationsEnabled: state.emailNotificationsEnabled, emailNotificationSound: state.emailNotificationSound, @@ -637,6 +633,7 @@ export const useSettingsStore = create()( includeGroupInUnified: state.includeGroupInUnified, enableAllMailView: state.enableAllMailView, allMailFolderIds: state.allMailFolderIds, + preferredIdentityIds: state.preferredIdentityIds, enableCrossUnreadView: state.enableCrossUnreadView, enableCrossStarredView: state.enableCrossStarredView, enableCrossAllView: state.enableCrossAllView, @@ -694,8 +691,8 @@ export const useSettingsStore = create()( if (key === 'allMailFolderIds' && !isPlainRecord(settings[key])) { return; } - // Defensive: a non-record (e.g. a legacy scalar) would break the - // per-account map lookups - ignore it. + // Per-account map (accountId -> identityId); ignore any legacy + // global/non-record value rather than corrupting the map. if (key === 'preferredIdentityIds' && !isPlainRecord(settings[key])) { return; } @@ -877,14 +874,10 @@ export const useSettingsStore = create()( get().importSettings(JSON.stringify(settings)); isLoadingFromServer = false; syncLog('Settings loaded from server successfully'); - // Re-apply the (possibly server-updated) per-account preferred - // sender identity to the already-loaded identities, so a fresh - // browser reflects the synced default without waiting for the next - // identity refresh. Dynamic import avoids a static import cycle - // (auth-store imports this store). (#507) - import('./auth-store') - .then(({ useAuthStore }) => useAuthStore.getState().applyPreferredIdentityOrdering()) - .catch(() => {}); + // The per-account preferred sender identity (#507) is re-applied by + // applyPreferredIdentity() in auth-store, invoked from the + // loadFromServer().finally() of every login / switch / restore path, + // so no extra hook is needed here. return true; } return false; @@ -897,7 +890,7 @@ export const useSettingsStore = create()( }), { name: 'settings-storage', - version: 5, + version: 6, migrate: (persisted, version) => { const state = persisted as Record; if (version < 2 && state.listDensity) { @@ -925,6 +918,11 @@ export const useSettingsStore = create()( if (version < 5 || !isPlainRecord(state.allMailFolderIds)) { state.allMailFolderIds = {}; } + // v6: introduced the per-account default-identity map (issue #507). + // Coerce any missing/legacy value to an empty record. + if (version < 6 || !isPlainRecord(state.preferredIdentityIds)) { + state.preferredIdentityIds = {}; + } return state as unknown as SettingsState; }, onRehydrateStorage: () => {