From de551dcd3a3fe86bf2029f0191b208e733604b34 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 9 Sep 2026 05:43:16 -0700 Subject: [PATCH] fix(desktop): key the vault panel by owner instead of syncing state in an effect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI lint: the owner-change wipe was a useEffect that reset state from a derived value (no-restricted-syntax) — replace it with what the guide prescribes: the mount site keys by (connection, profile), so an owner change remounts the panel and drafts/dialogs are gone by construction. Owner test mirrors the keyed mount; curly-brace lint in the test fixture. --- apps/desktop/src/app/settings/index.tsx | 6 ++-- .../settings/vault-settings.owner.test.tsx | 29 ++++++++++++++++--- .../src/app/settings/vault-settings.test.tsx | 3 ++ .../src/app/settings/vault-settings.tsx | 24 +++++---------- 4 files changed, 40 insertions(+), 22 deletions(-) diff --git a/apps/desktop/src/app/settings/index.tsx b/apps/desktop/src/app/settings/index.tsx index 3238e068a8..3d2eb0b21c 100644 --- a/apps/desktop/src/app/settings/index.tsx +++ b/apps/desktop/src/app/settings/index.tsx @@ -31,6 +31,7 @@ import { typeToFocusChar } from '@/lib/keybinds/composer-focus-keys' import { cn } from '@/lib/utils' import { $commandPaletteOpen, openCommandPalettePage } from '@/store/command-palette' import { confirm } from '@/store/confirm' +import { $activeConnectionId } from '@/store/connections' import { bindingsFor } from '@/store/keybinds' import { $localModelsEnabled } from '@/store/local-models-flag' import { notifyError } from '@/store/notifications' @@ -54,7 +55,7 @@ import { NotificationsSettings } from './notifications-settings' import { PROVIDER_VIEWS, ProvidersSettings, type ProviderView } from './providers-settings' import { SessionsSettings } from './sessions-settings' import type { SettingsPageProps, SettingsView as SettingsViewId } from './types' -import { VaultSettings } from './vault-settings' +import { vaultOwnerKey, VaultSettings } from './vault-settings' const SETTINGS_VIEWS: readonly SettingsViewId[] = [ ...SECTIONS.map(s => `config:${s.id}` as SettingsViewId), @@ -74,6 +75,7 @@ const SETTINGS_VIEWS: readonly SettingsViewId[] = [ export function SettingsView({ onClose, onConfigSaved, onMainModelChanged }: SettingsPageProps) { const scopeProfile = useStore($settingsScopeProfile) + const activeConnectionId = useStore($activeConnectionId) const { t } = useI18n() const navigate = useNavigate() const { hash, pathname, search } = useLocation() @@ -431,7 +433,7 @@ export function SettingsView({ onClose, onConfigSaved, onMainModelChanged }: Set ) : activeView === 'billing' ? ( ) : activeView === 'vault' ? ( - + ) : ( ) diff --git a/apps/desktop/src/app/settings/vault-settings.owner.test.tsx b/apps/desktop/src/app/settings/vault-settings.owner.test.tsx index 38418572d1..0623eff0d1 100644 --- a/apps/desktop/src/app/settings/vault-settings.owner.test.tsx +++ b/apps/desktop/src/app/settings/vault-settings.owner.test.tsx @@ -21,11 +21,14 @@ vi.mock('@/store/gateway', async importActual => ({ vi.mock('@/lib/haptics', () => ({ triggerHaptic: vi.fn() })) vi.mock('@/store/notifications', () => ({ notify: vi.fn(), notifyError: vi.fn() })) +import { useStore } from '@nanostores/react' + import { queryClient } from '@/lib/query-client' import { $activeGatewayProfile } from '@/store/profile' import { $gatewayState } from '@/store/session' +import { $settingsScopeProfile } from '@/store/settings-scope' -import { VaultSettings } from './vault-settings' +import { vaultOwnerKey, VaultSettings } from './vault-settings' stubResizeObserver() @@ -33,11 +36,19 @@ const sources = [ { name: 'bitwarden', display_name: 'Bitwarden', enabled: true, needs_unlock: true, unlocked: false, installed: true } ] +// Mirrors the production mount site (settings/index.tsx): the panel is keyed by its owner, so an +// owner change remounts it and every dialog/draft is gone by construction. +function KeyedVault() { + const profile = useStore($settingsScopeProfile) + + return +} + function mount() { return render( - + ) @@ -72,11 +83,19 @@ it('a master-password draft is wiped on a profile switch and never submitted to it("a late list response from profile A never paints under profile B", async () => { let resolveA!: (value: unknown) => void const held = new Promise(r => (resolveA = r)) + respond = async (profile, method) => { - if (method === 'vault.sources') return { sources } - if (profile === 'default' && method === 'vault.list') return held + if (method === 'vault.sources') { + return { sources } + } + + if (profile === 'default' && method === 'vault.list') { + return held + } + return { items: [] } } + mount() await waitFor(() => expect(calls.some(c => c.profile === 'default' && c.method === 'vault.list')).toBe(true)) @@ -94,9 +113,11 @@ it('vault.add secrets never enter the mutation cache', async () => { respond = async (_profile, method) => (method === 'vault.sources' ? { sources } : method === 'vault.list' ? { items: [] } : { id: 'created' }) const view = mount() fireEvent.click(await screen.findByRole('button', { name: 'Add credential' })) + for (const [label, value] of [['Label', 'fixture'], ['Site origin', 'https://example.com'], ['Identifier', 'fixture@example.com'], ['Password', 'fixture-retained-password']] as const) { fireEvent.change(screen.getByLabelText(label), { target: { value } }) } + fireEvent.click(screen.getByRole('button', { name: 'Save to vault' })) await waitFor(() => expect(calls.some(c => c.method === 'vault.add')).toBe(true)) expect((calls.find(c => c.method === 'vault.add')!.params.secret as Record).password).toBe('fixture-retained-password') diff --git a/apps/desktop/src/app/settings/vault-settings.test.tsx b/apps/desktop/src/app/settings/vault-settings.test.tsx index 9b29c58f11..a7b3aec1e4 100644 --- a/apps/desktop/src/app/settings/vault-settings.test.tsx +++ b/apps/desktop/src/app/settings/vault-settings.test.tsx @@ -146,14 +146,17 @@ describe('VaultSettings', () => { { name: 'onepassword', display_name: '1Password', enabled: true, needs_unlock: true, unlocked: false, installed: true }, { name: 'bitwarden', display_name: 'Bitwarden', enabled: false, needs_unlock: true, unlocked: false, installed: false } ] + requestGateway.mockImplementation(async (method: string) => { if (method === 'vault.list') { // An external item has no delete affordance; its manager is shown as a source badge instead. return { items: [{ ...LOGIN_ITEM, id: 'op:xyz', label: 'GitHub via 1Password', backend: 'onepassword' }] } } + if (method === 'vault.sources') { return { sources: sources.map(source => ({ ...source })) } } + if (method === 'vault.unlock') { sources[0] = { ...sources[0], unlocked: true } diff --git a/apps/desktop/src/app/settings/vault-settings.tsx b/apps/desktop/src/app/settings/vault-settings.tsx index d2571dc00d..21be8a860a 100644 --- a/apps/desktop/src/app/settings/vault-settings.tsx +++ b/apps/desktop/src/app/settings/vault-settings.tsx @@ -32,6 +32,7 @@ import { ListRow, Pill, SectionHeading, SettingsContent } from './primitives' // Vault data is private to one (connection, profile); the cache key carries that owner so a // late response from profile A can never paint under profile B. +export const vaultOwnerKey = (connectionId: null | string, profile: string) => `${connectionId ?? ''}::${profile}` const vaultQueryKey = (owner: string) => ['vault-items', owner] as const const vaultSourcesQueryKey = (owner: string) => ['vault-sources', owner] as const @@ -151,17 +152,20 @@ export function VaultSettings() { const v = t.settings.vault const gatewayState = useStore($gatewayState) const queryClient = useQueryClient() - // The owner this panel edits: pinned per render, and every RPC below goes through the owner's - // socket with an explicit profile — never the ambient foreground gateway. Changing owner - // (profile switch, connection swap) closes every dialog and wipes drafts (see the effect below). + // The owner this panel edits: every RPC below goes through the owner's socket with an explicit + // profile — never the ambient foreground gateway. The mount site keys the panel by this same + // owner, so a profile switch / connection swap remounts it: dialogs close and drafts (including a + // typed master password) are gone by construction rather than by cleanup code. const scopeProfile = useStore($settingsScopeProfile) const connectionId = useStore($activeConnectionId) - const owner = `${connectionId ?? ''}::${scopeProfile}` + const owner = vaultOwnerKey(connectionId, scopeProfile) + const requestGateway = useCallback( (method: string, params: Record = {}) => requestGatewayForProfile(scopeProfile, method, params), [scopeProfile] ) + const VAULT_QUERY_KEY = useMemo(() => vaultQueryKey(owner), [owner]) const VAULT_SOURCES_QUERY_KEY = useMemo(() => vaultSourcesQueryKey(owner), [owner]) const [searchParams, setSearchParams] = useSearchParams() @@ -178,18 +182,6 @@ export function VaultSettings() { const pendingMasterPassword = useRef('') const pendingSecret = useRef>(null) - useEffect(() => { - // A draft typed for one owner must not be submitted to another. - setAddOpen(false) - setForm(EMPTY_FORM) - setFormError(null) - setPendingDelete(null) - setUnlockTarget(null) - setMasterPassword('') - setUnlockError(null) - pendingMasterPassword.current = '' - pendingSecret.current = null - }, [owner]) const { data: sourcesData } = useQuery({ enabled: gatewayState === 'open',