diff --git a/src/renderer/components/Loading.test.tsx b/src/renderer/components/Loading.test.tsx new file mode 100644 index 000000000..a89235815 --- /dev/null +++ b/src/renderer/components/Loading.test.tsx @@ -0,0 +1,36 @@ +import { act } from '@testing-library/react'; + +import { renderWithProviders } from '../__helpers__/test-utils'; + +import { Loading } from './Loading'; + +describe('renderer/components/Loading.tsx', () => { + it('should render itself & its children', async () => { + let tree: ReturnType | null = null; + + await act(async () => { + tree = renderWithProviders(); + }); + + expect(tree!.getByText('Loading')).toBeInTheDocument(); + expect(tree!.container.querySelector('img')).toHaveAttribute('alt', '⏳'); + expect(tree!.container).toMatchSnapshot(); + }); + + it('should keep the same emoji when re-rendering', async () => { + let tree: ReturnType | null = null; + + await act(async () => { + tree = renderWithProviders(); + }); + + const emojiSrc = tree!.container.querySelector('img')?.getAttribute('src'); + expect(emojiSrc).toBeTruthy(); + + await act(async () => { + tree!.rerender(); + }); + + expect(tree!.container.querySelector('img')).toHaveAttribute('src', emojiSrc!); + }); +}); diff --git a/src/renderer/components/Loading.tsx b/src/renderer/components/Loading.tsx new file mode 100644 index 000000000..0d219d696 --- /dev/null +++ b/src/renderer/components/Loading.tsx @@ -0,0 +1,13 @@ +import { useMemo } from 'react'; + +import { Constants } from '../constants'; + +import { EmojiSplash } from './layout/EmojiSplash'; + +import { randomElement } from '../utils/core/random'; + +export function Loading() { + const emoji = useMemo(() => randomElement(Constants.EMOJIS.LOADING), []); + + return ; +} diff --git a/src/renderer/components/__snapshots__/Loading.test.tsx.snap b/src/renderer/components/__snapshots__/Loading.test.tsx.snap new file mode 100644 index 000000000..41070ebcc --- /dev/null +++ b/src/renderer/components/__snapshots__/Loading.test.tsx.snap @@ -0,0 +1,53 @@ +// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html + +exports[`renderer/components/Loading.tsx > should render itself & its children 1`] = ` +
+
+
+
+
+ ⏳ +
+
+ Loading +
+
+
+
+
+`; diff --git a/src/renderer/constants.ts b/src/renderer/constants.ts index f1b846a23..e6371f153 100644 --- a/src/renderer/constants.ts +++ b/src/renderer/constants.ts @@ -64,6 +64,7 @@ export const Constants = { // Emojis for different states and events EMOJIS: { ALL_READ: ['🎉', '🎊', '🥳', '👏', '🙌', '😎', '🏖️', '🚀', '✨', '🏆'], + LOADING: ['⏳', '⌛'], ERRORS: { ACTION_FORBIDDEN: ['🚫'], BAD_CREDENTIALS: ['🔓'], diff --git a/src/renderer/hooks/notifications/useNotificationsQuery.ts b/src/renderer/hooks/notifications/useNotificationsQuery.ts index 8810a1e0f..f5a199730 100644 --- a/src/renderer/hooks/notifications/useNotificationsQuery.ts +++ b/src/renderer/hooks/notifications/useNotificationsQuery.ts @@ -1,6 +1,6 @@ import { useCallback, useMemo } from 'react'; -import { keepPreviousData, type QueryKey, useQuery, useQueryClient } from '@tanstack/react-query'; +import { type QueryKey, useQuery, useQueryClient } from '@tanstack/react-query'; import { Constants } from '../../constants'; @@ -46,6 +46,20 @@ export interface NotificationsQueryResult { removeAccountNotifications: (account: Account) => void; } +/** + * Keep previous notifications visible across query-key changes only when they + * contain actual content or account errors. An account set with empty inboxes is + * not content: it must not make a new account set look settled. Returning + * undefined lets the query report initial loading instead. + */ +function keepNonEmptyPreviousData( + previousData: AccountNotifications[] | undefined, +): AccountNotifications[] | undefined { + return previousData?.some((account) => account.notifications.length > 0 || account.error !== null) + ? previousData + : undefined; +} + /** * The read side of notification state: the TanStack Query observer (optionally * polling), the filter projection, derived counts, and the derived status and @@ -129,7 +143,7 @@ export const useNotificationsQuery = ({ poll }: { poll: boolean }): Notification // This allows filter changes to instantly update without refetching select: selectFilteredNotifications, - placeholderData: keepPreviousData, + placeholderData: keepNonEmptyPreviousData, // Fresh for at least one poll cycle at the fastest possible interval, so // data isn't treated as stale before a refetch could realistically have diff --git a/src/renderer/hooks/useNotifications.test.tsx b/src/renderer/hooks/useNotifications.test.tsx index 091d8456e..ea7cafac6 100644 --- a/src/renderer/hooks/useNotifications.test.tsx +++ b/src/renderer/hooks/useNotifications.test.tsx @@ -192,6 +192,109 @@ describe('renderer/hooks/useNotifications.ts', () => { }); }); + describe('account set changes', () => { + it('reports loading when a new account set starts from an empty settled result', async () => { + useAccountsStore.setState({ accounts: [] }); + getAllNotificationsMock.mockResolvedValueOnce([]); + + const { result } = renderNotificationsHook(); + await waitFor(() => expect(result.current.status).toBe('success')); + + let resolveFetch: ((data: AccountNotifications[]) => void) | undefined; + getAllNotificationsMock.mockImplementationOnce( + () => + new Promise((resolve) => { + resolveFetch = resolve; + }), + ); + + act(() => { + useAccountsStore.setState({ accounts: [mockGitHubCloudAccount] }); + }); + + await waitFor(() => expect(result.current.status).toBe('loading')); + expect(getAllNotificationsMock).toHaveBeenCalledTimes(2); + + await act(async () => { + resolveFetch?.(mockSingleAccountNotifications); + }); + + await waitFor(() => expect(result.current.status).toBe('success')); + expect(result.current.notifications).toEqual(mockSingleAccountNotifications); + }); + + it('reports loading when the account set changes from an empty settled inbox', async () => { + getAllNotificationsMock.mockResolvedValueOnce([ + { + account: mockGitHubCloudAccount, + notifications: [], + error: null, + }, + ]); + + const { result } = renderNotificationsHook(); + await waitFor(() => expect(result.current.status).toBe('success')); + expect(result.current.notifications).toHaveLength(1); + expect(result.current.notifications[0].notifications).toEqual([]); + + let resolveFetch: ((data: AccountNotifications[]) => void) | undefined; + getAllNotificationsMock.mockImplementationOnce( + () => + new Promise((resolve) => { + resolveFetch = resolve; + }), + ); + + act(() => { + useAccountsStore.setState({ + accounts: [mockGitHubCloudAccount, mockGitHubEnterpriseServerAccount], + }); + }); + + await waitFor(() => expect(result.current.status).toBe('loading')); + + await act(async () => { + resolveFetch?.(mockMultipleAccountNotifications); + }); + + await waitFor(() => expect(result.current.status).toBe('success')); + expect(result.current.notifications).toEqual(mockMultipleAccountNotifications); + }); + + it('keeps non-empty notifications visible while a new account set loads', async () => { + getAllNotificationsMock.mockResolvedValueOnce(mockSingleAccountNotifications); + + const { result } = renderNotificationsHook(); + await waitFor(() => expect(result.current.status).toBe('success')); + expect(result.current.notifications).toEqual(mockSingleAccountNotifications); + + let resolveFetch: ((data: AccountNotifications[]) => void) | undefined; + getAllNotificationsMock.mockImplementationOnce( + () => + new Promise((resolve) => { + resolveFetch = resolve; + }), + ); + + act(() => { + useAccountsStore.setState({ + accounts: [mockGitHubCloudAccount, mockGitHubEnterpriseServerAccount], + }); + }); + + await waitFor(() => expect(result.current.isFetching).toBe(true)); + expect(result.current.status).toBe('success'); + expect(result.current.notifications).toEqual(mockSingleAccountNotifications); + + await act(async () => { + resolveFetch?.(mockMultipleAccountNotifications); + }); + + await waitFor(() => expect(result.current.isFetching).toBe(false)); + expect(result.current.notifications).toEqual(mockMultipleAccountNotifications); + }); + }); + describe('status stability during background refetches', () => { it('keeps status as error while a retry during an ongoing outage is in flight', async () => { let rejectRetry: (() => void) | undefined; @@ -264,6 +367,36 @@ describe('renderer/hooks/useNotifications.ts', () => { await waitFor(() => expect(result.current.isFetching).toBe(false)); expect(result.current.status).toBe('success'); }); + + it('keeps status as success while a settled empty inbox refetches', async () => { + let resolveRefetch: ((data: AccountNotifications[]) => void) | undefined; + getAllNotificationsMock.mockResolvedValueOnce([]).mockImplementationOnce( + () => + new Promise((resolve) => { + resolveRefetch = resolve; + }), + ); + + const { result } = renderNotificationsHook(); + + await waitFor(() => expect(result.current.status).toBe('success')); + expect(result.current.notifications).toEqual([]); + + act(() => { + void result.current.refetchNotifications(); + }); + + await waitFor(() => expect(result.current.isFetching).toBe(true)); + expect(result.current.status).toBe('success'); + expect(result.current.notifications).toEqual([]); + + await act(async () => { + resolveRefetch?.([]); + }); + + await waitFor(() => expect(result.current.isFetching).toBe(false)); + expect(result.current.status).toBe('success'); + }); }); describe('polling', () => { diff --git a/src/renderer/routes/Notifications.test.tsx b/src/renderer/routes/Notifications.test.tsx index 5a7b38bf5..b8ea46d5e 100644 --- a/src/renderer/routes/Notifications.test.tsx +++ b/src/renderer/routes/Notifications.test.tsx @@ -15,6 +15,10 @@ vi.mock('../components/AllRead', () => ({ AllRead: () =>

AllRead

, })); +vi.mock('../components/Loading', () => ({ + Loading: () =>

Loading

, +})); + vi.mock('../components/Oops', () => ({ Oops: () =>

Oops

, })); @@ -31,9 +35,61 @@ describe('renderer/routes/Notifications.tsx', () => { it('should render itself & its children (all read notifications)', () => { const tree = renderWithProviders(); + expect(screen.getByText('AllRead')).toBeInTheDocument(); + expect(screen.queryByText('Loading')).not.toBeInTheDocument(); expect(tree.container).toMatchSnapshot(); }); + it('should render loading state instead of all-read while notifications are loading', () => { + const tree = renderWithProviders(, { + status: 'loading', + }); + + expect(screen.getByText('Loading')).toBeInTheDocument(); + expect(screen.queryByText('AllRead')).not.toBeInTheDocument(); + expect(tree.container).toMatchSnapshot(); + }); + + it('should keep notifications visible during a background refresh', () => { + renderWithProviders(, { + notifications: mockMultipleAccountNotifications, + hasNotifications: true, + isFetching: true, + }); + + expect(screen.getByText('NotificationList')).toBeInTheDocument(); + expect(screen.queryByText('Loading')).not.toBeInTheDocument(); + }); + + it('should keep all-read visible during a background refresh with an empty inbox', () => { + renderWithProviders(, { + isFetching: true, + }); + + expect(screen.getByText('AllRead')).toBeInTheDocument(); + expect(screen.queryByText('Loading')).not.toBeInTheDocument(); + }); + + it('should prefer the offline state over loading', () => { + renderWithProviders(, { + isOnline: false, + status: 'loading', + }); + + expect(screen.getByText('Oops')).toBeInTheDocument(); + expect(screen.queryByText('Loading')).not.toBeInTheDocument(); + }); + + it('should prefer the error state over loading', () => { + renderWithProviders(, { + globalError: Errors.UNKNOWN, + status: 'error', + }); + + expect(screen.getByText('Oops')).toBeInTheDocument(); + expect(screen.queryByText('Loading')).not.toBeInTheDocument(); + }); + it('should render itself & its children (show account header)', () => { const tree = renderWithProviders(, { notifications: [mockMultipleAccountNotifications[0]], diff --git a/src/renderer/routes/Notifications.tsx b/src/renderer/routes/Notifications.tsx index 2e5d65196..ba8f2e203 100644 --- a/src/renderer/routes/Notifications.tsx +++ b/src/renderer/routes/Notifications.tsx @@ -6,6 +6,7 @@ import { useAccountsStore, useFiltersStore, useSettingsStore } from '../stores'; import { AllRead } from '../components/AllRead'; import { Page } from '../components/layout/Page'; +import { Loading } from '../components/Loading'; import { NotificationList } from '../components/notifications/NotificationList'; import { Oops } from '../components/Oops'; @@ -42,6 +43,10 @@ export function NotificationsRoute() { return ; } + if (status === 'loading') { + return ; + } + if (!hasNotifications && noAccountErrors) { return ; } diff --git a/src/renderer/routes/__snapshots__/Notifications.test.tsx.snap b/src/renderer/routes/__snapshots__/Notifications.test.tsx.snap index 913470782..4555631c9 100644 --- a/src/renderer/routes/__snapshots__/Notifications.test.tsx.snap +++ b/src/renderer/routes/__snapshots__/Notifications.test.tsx.snap @@ -73,3 +73,11 @@ exports[`renderer/routes/Notifications.tsx > should render itself & its children `; + +exports[`renderer/routes/Notifications.tsx > should render loading state instead of all-read while notifications are loading 1`] = ` +
+

+ Loading +

+
+`;