diff --git a/client/src/components/Settings/PhotoProvidersSection.test.tsx b/client/src/components/Settings/PhotoProvidersSection.test.tsx index cff5e5f70..db953ba5f 100644 --- a/client/src/components/Settings/PhotoProvidersSection.test.tsx +++ b/client/src/components/Settings/PhotoProvidersSection.test.tsx @@ -1,4 +1,4 @@ -// FE-COMP-PHOTOPROVIDERS-001 to FE-COMP-PHOTOPROVIDERS-027 +// FE-COMP-PHOTOPROVIDERS-001 to FE-COMP-PHOTOPROVIDERS-028 import { render, screen, waitFor } from '../../../tests/helpers/render'; import userEvent from '@testing-library/user-event'; import { http, HttpResponse } from 'msw'; @@ -280,6 +280,31 @@ describe('PhotoProvidersSection', () => { expect(await screen.findByText('Could not connect to Immich: Auth failed')).toBeInTheDocument(); }); + it('FE-COMP-PHOTOPROVIDERS-028: a settings read landing after the status probe does not flip the badge to connected', async () => { + let releaseSettings!: () => void; + const settingsGate = new Promise(r => (releaseSettings = r)); + let statusAnswered = false; + server.use( + // Stored credentials make the settings read say connected; the probe says the server is down. + http.get('/api/addons/immich/settings', async () => { + await settingsGate; + return HttpResponse.json({ url: 'https://photos.example.com', connected: true }); + }), + http.get('/api/addons/immich/status', () => { + statusAnswered = true; + return HttpResponse.json({ connected: false }); + }), + ); + seedMemoriesEnabled(); + render(); + await screen.findByText('Immich'); + await waitFor(() => expect(statusAnswered).toBe(true)); + releaseSettings(); + await waitFor(() => expect(screen.getByDisplayValue('https://photos.example.com')).toBeInTheDocument()); + expect(screen.getByText('Not connected')).toBeInTheDocument(); + expect(screen.queryByText('Connected')).not.toBeInTheDocument(); + }); + it('FE-COMP-PHOTOPROVIDERS-016: Test button is disabled while test is in progress', async () => { const user = userEvent.setup(); let resolveTest!: () => void; diff --git a/client/src/components/Settings/PhotoProvidersSection.tsx b/client/src/components/Settings/PhotoProvidersSection.tsx index ae2db0df1..ac1794a8a 100644 --- a/client/src/components/Settings/PhotoProvidersSection.tsx +++ b/client/src/components/Settings/PhotoProvidersSection.tsx @@ -27,7 +27,7 @@ export default function PhotoProvidersSection(): React.ReactElement { isProviderSaveDisabled, handleSaveProvider, handleTestProvider, - } = usePhotoProviderConnections({ statusRouteOwnsBadge: false }) + } = usePhotoProviderConnections() const renderPhotoProviderSection = (provider: PhotoProviderAddon): React.ReactElement => { const fields = getProviderFields(provider) diff --git a/client/src/components/Settings/usePhotoProviderConnections.test.ts b/client/src/components/Settings/usePhotoProviderConnections.test.ts index cbf202302..7d88bfd36 100644 --- a/client/src/components/Settings/usePhotoProviderConnections.test.ts +++ b/client/src/components/Settings/usePhotoProviderConnections.test.ts @@ -81,8 +81,8 @@ function stubGets(byPath: Record Promise)>) { }); } -function render(statusRouteOwnsBadge: boolean) { - return renderHook(() => usePhotoProviderConnections({ statusRouteOwnsBadge })); +function render() { + return renderHook(() => usePhotoProviderConnections()); } beforeEach(() => { @@ -111,7 +111,7 @@ describe('usePhotoProviderConnections', () => { false ); stubGets({}); - const { result } = render(true); + const { result } = render(); expect(result.current.memoriesEnabled).toBe(false); expect(result.current.activePhotoProviders.map((p) => p.id)).toEqual(['immich']); }); @@ -122,43 +122,37 @@ describe('usePhotoProviderConnections', () => { '/addons/immich/settings': { data: { url: 'https://p.example', api_key: 'secret', include_shared: true } }, '/addons/immich/status': { data: { connected: true } }, }); - const { result } = render(true); + const { result } = render(); await waitFor(() => expect(result.current.providerValues.immich?.url).toBe('https://p.example')); expect(result.current.providerValues.immich).toEqual({ url: 'https://p.example', shared: 'true' }); await waitFor(() => expect(result.current.providerConnected.immich).toBe(true)); }); - it('FE-COMP-PHOTOCONN-004: with the status route owning the badge, a late settings read does not override it', async () => { + it('FE-COMP-PHOTOCONN-004: the status route owns the badge, so a late settings read does not override it', async () => { seedProviders([PROVIDER]); let resolveSettings!: (r: Resp) => void; stubGets({ '/addons/immich/settings': () => new Promise((r) => (resolveSettings = r)), '/addons/immich/status': { data: { connected: true } }, }); - const { result } = render(true); + const { result } = render(); await waitFor(() => expect(result.current.providerConnected.immich).toBe(true)); await act(async () => resolveSettings({ data: { url: 'u', connected: false } })); expect(result.current.providerValues.immich?.url).toBe('u'); expect(result.current.providerConnected.immich).toBe(true); }); - it('FE-COMP-PHOTOCONN-005: without that, the later settings read sets the badge', async () => { - seedProviders([PROVIDER]); - let resolveSettings!: (r: Resp) => void; - stubGets({ - '/addons/immich/settings': () => new Promise((r) => (resolveSettings = r)), - '/addons/immich/status': { data: { connected: true } }, - }); - const { result } = render(false); + it('FE-COMP-PHOTOCONN-005: a provider without a status route takes the badge from the settings read', async () => { + seedProviders([{ ...PROVIDER, config: { settings_get: '/addons/immich/settings' } }]); + stubGets({ '/addons/immich/settings': { data: { url: 'u', connected: true } } }); + const { result } = render(); await waitFor(() => expect(result.current.providerConnected.immich).toBe(true)); - await act(async () => resolveSettings({ data: { url: 'u', connected: false } })); - expect(result.current.providerConnected.immich).toBe(false); }); it('FE-COMP-PHOTOCONN-006: save is blocked while a required field is blank', async () => { seedProviders([PROVIDER]); stubGets({ '/addons/immich/settings': { data: {} }, '/addons/immich/status': { data: {} } }); - const { result } = render(true); + const { result } = render(); await waitFor(() => expect(result.current.providerValues.immich?.url).toBe('')); expect(result.current.isProviderSaveDisabled(PROVIDER)).toBe(true); act(() => result.current.handleProviderFieldChange('immich', 'url', 'https://a')); @@ -173,7 +167,7 @@ describe('usePhotoProviderConnections', () => { '/addons/immich/status': { data: { connected: false } }, }); const put = vi.spyOn(apiClient, 'put').mockResolvedValue({ data: {} }); - const { result } = render(true); + const { result } = render(); await waitFor(() => expect(result.current.providerValues.immich?.url).toBe('https://a')); act(() => result.current.handleProviderFieldChange('immich', 'url', ' https://b ')); get.mockClear(); @@ -188,7 +182,7 @@ describe('usePhotoProviderConnections', () => { seedProviders([PROVIDER]); stubGets({ '/addons/immich/settings': { data: {} }, '/addons/immich/status': { data: {} } }); const put = vi.spyOn(apiClient, 'put').mockRejectedValue(new Error('x')); - const { result } = render(true); + const { result } = render(); await act(() => result.current.handleSaveProvider(PROVIDER)); expect(toast.error).toHaveBeenCalledWith('memories.saveError(Immich)'); put.mockClear(); @@ -203,7 +197,7 @@ describe('usePhotoProviderConnections', () => { '/addons/immich/status': { data: {} }, }); const post = vi.spyOn(apiClient, 'post').mockResolvedValue({ data: { connected: true } }); - const { result } = render(true); + const { result } = render(); await waitFor(() => expect(result.current.providerValues.immich?.url).toBe('https://a')); await act(() => result.current.handleTestProvider(PROVIDER)); expect(post).toHaveBeenCalledWith('/addons/immich/test', { url: 'https://a', include_shared: false }); @@ -220,10 +214,10 @@ describe('usePhotoProviderConnections', () => { seedProviders([PROVIDER]); stubGets({ '/addons/immich/settings': { data: {} }, '/addons/immich/status': { data: {} } }); vi.spyOn(apiClient, 'post').mockResolvedValue({ data: { connected: false, error: 'bad key' } }); - const phone = render(true); + const phone = render(); await act(() => phone.result.current.handleTestProvider(PROVIDER)); expect(toast.error).toHaveBeenLastCalledWith('memories.connectionError(Immich): bad key'); - const desktop = render(false); + const desktop = render(); await act(() => desktop.result.current.handleTestProvider(PROVIDER)); expect(toast.error).toHaveBeenLastCalledWith('memories.connectionError(Immich): bad key'); vi.mocked(apiClient.post).mockResolvedValue({ data: { connected: false } }); diff --git a/client/src/components/Settings/usePhotoProviderConnections.ts b/client/src/components/Settings/usePhotoProviderConnections.ts index 2d379c785..8fa325349 100644 --- a/client/src/components/Settings/usePhotoProviderConnections.ts +++ b/client/src/components/Settings/usePhotoProviderConnections.ts @@ -49,22 +49,13 @@ export const getProviderFields = (provider: PhotoProviderAddon): ProviderField[] return [...(provider.fields || [])].sort((a, b) => a.sort_order - b.sort_order); }; -export interface PhotoProviderConnectionOptions { - /** - * When the provider has a status route, let only that route set the badge and - * ignore the `connected` flag of the settings read, so the two responses cannot - * race. The phone does this; the desktop takes whichever answer lands last. - */ - statusRouteOwnsBadge: boolean; -} - /** * The photo provider connection cards behind both settings shells: every enabled * photo_provider addon with its declared fields, seeded and hydrated values * (secrets never prefilled), the connection badge, and save and test. The desktop * section and its phone twin render their own markup over this. */ -export function usePhotoProviderConnections({ statusRouteOwnsBadge }: PhotoProviderConnectionOptions) { +export function usePhotoProviderConnections() { const { t } = useTranslation(); const toast = useToast(); const { isEnabled: addonEnabled, addons } = useAddonStore(); @@ -156,7 +147,10 @@ export function usePhotoProviderConnections({ statusRouteOwnsBadge }: PhotoProvi ...prev, [provider.id]: { ...(prev[provider.id] || {}), ...nextValues }, })); - if (!(statusRouteOwnsBadge && cfg.status_get) && typeof res.data?.connected === 'boolean') { + // A provider with a status route lets only that route set the badge: the settings + // read can only say credentials are stored, and taking whichever of the two answers + // lands last would show an unreachable server as connected. + if (!cfg.status_get && typeof res.data?.connected === 'boolean') { setProviderConnected((prev) => ({ ...prev, [provider.id]: !!res.data.connected })); } }) @@ -169,7 +163,7 @@ export function usePhotoProviderConnections({ statusRouteOwnsBadge }: PhotoProvi return () => { isCancelled = true; }; - }, [activePhotoProviders, activeProviderSignature, statusRouteOwnsBadge]); + }, [activePhotoProviders, activeProviderSignature]); const handleProviderFieldChange = (providerId: string, key: string, value: string) => { setProviderValues((prev) => ({ diff --git a/client/src/mobile/screens/settings/MPhotoProvidersSection.tsx b/client/src/mobile/screens/settings/MPhotoProvidersSection.tsx index 29f9e2820..09ae85e99 100644 --- a/client/src/mobile/screens/settings/MPhotoProvidersSection.tsx +++ b/client/src/mobile/screens/settings/MPhotoProvidersSection.tsx @@ -30,7 +30,7 @@ export default function MPhotoProvidersSection(): React.ReactElement { isProviderSaveDisabled, handleSaveProvider, handleTestProvider, - } = usePhotoProviderConnections({ statusRouteOwnsBadge: true }) + } = usePhotoProviderConnections() const renderPhotoProviderSection = (provider: PhotoProviderAddon): React.ReactElement => { const fields = getProviderFields(provider)