Skip to content

feat: gracefully handle partial failure for multiple accounts #1419

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Aug 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/__mocks__/notifications-mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,19 @@ export const mockAccountNotifications: AccountNotifications[] = [
{
account: mockGitHubCloudAccount,
notifications: mockGitHubNotifications,
error: null,
},
{
account: mockGitHubEnterpriseServerAccount,
notifications: mockEnterpriseNotifications,
error: null,
},
];

export const mockSingleAccountNotifications: AccountNotifications[] = [
{
account: mockGitHubCloudAccount,
notifications: [mockGitHubNotifications[0]],
error: null,
},
];
37 changes: 34 additions & 3 deletions src/components/AccountNotifications.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,18 @@ jest.mock('./RepositoryNotifications', () => ({
}));

describe('components/AccountNotifications.tsx', () => {
it('should render itself (github.com with notifications) - group by repositories', () => {
// The read emoji randomly rotates, but then the snapshots would never match
// Have to make it consistent so the emojis are always the same
beforeEach(() => {
global.Math.random = jest.fn(() => 0.1);
});

it('should render itself - group notifications by repositories', () => {
const props = {
account: mockGitHubCloudAccount,
notifications: mockGitHubNotifications,
showAccountHostname: true,
error: null,
};

const tree = render(
Expand All @@ -28,11 +35,12 @@ describe('components/AccountNotifications.tsx', () => {
expect(tree).toMatchSnapshot();
});

it('should render itself (github.com with notifications) - group by date', () => {
it('should render itself - group notifications by date', () => {
const props = {
account: mockGitHubCloudAccount,
notifications: mockGitHubNotifications,
showAccountHostname: true,
error: null,
};

const tree = render(
Expand All @@ -45,10 +53,31 @@ describe('components/AccountNotifications.tsx', () => {
expect(tree).toMatchSnapshot();
});

it('should render itself (github.com without notifications)', () => {
it('should render itself - no notifications', () => {
const props = {
account: mockGitHubCloudAccount,
notifications: [],
showAccountHostname: true,
error: null,
};

const tree = render(
<AppContext.Provider value={{ settings: mockSettings }}>
<AccountNotifications {...props} />
</AppContext.Provider>,
);
expect(tree).toMatchSnapshot();
});

it('should render itself - account error', () => {
const props = {
account: mockGitHubCloudAccount,
notifications: [],
error: {
title: 'Error title',
descriptions: ['Error description'],
emojis: ['🔥'],
},
showAccountHostname: true,
};

Expand All @@ -69,6 +98,7 @@ describe('components/AccountNotifications.tsx', () => {
account: mockGitHubCloudAccount,
notifications: [],
showAccountHostname: true,
error: null,
};

await act(async () => {
Expand All @@ -90,6 +120,7 @@ describe('components/AccountNotifications.tsx', () => {
account: mockGitHubCloudAccount,
notifications: mockGitHubNotifications,
showAccountHostname: true,
error: null,
};

await act(async () => {
Expand Down
20 changes: 16 additions & 4 deletions src/components/AccountNotifications.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,24 @@ import {
ChevronLeftIcon,
ChevronUpIcon,
} from '@primer/octicons-react';
import { type FC, type MouseEvent, useContext, useState } from 'react';
import { type FC, type MouseEvent, useContext, useMemo, useState } from 'react';
import { AppContext } from '../context/App';
import { type Account, Opacity, Size } from '../types';
import { type Account, type GitifyError, Opacity, Size } from '../types';
import type { Notification } from '../typesGitHub';
import { cn } from '../utils/cn';
import { openAccountProfile } from '../utils/links';
import { AllRead } from './AllRead';
import { HoverGroup } from './HoverGroup';
import { NotificationRow } from './NotificationRow';
import { Oops } from './Oops';
import { RepositoryNotifications } from './RepositoryNotifications';
import { InteractionButton } from './buttons/InteractionButton';
import { PlatformIcon } from './icons/PlatformIcon';

interface IAccountNotifications {
account: Account;
notifications: Notification[];
error: GitifyError | null;
showAccountHostname: boolean;
}

Expand All @@ -40,6 +43,11 @@ export const AccountNotifications: FC<IAccountNotifications> = (
),
);

const hasNotifications = useMemo(
() => notifications.length > 0,
[notifications],
);

const [showAccountNotifications, setShowAccountNotifications] =
useState(true);

Expand Down Expand Up @@ -68,8 +76,10 @@ export const AccountNotifications: FC<IAccountNotifications> = (
{showAccountHostname && (
<div
className={cn(
'group flex items-center justify-between px-3 py-1.5 text-sm font-semibold',
'bg-gray-300 dark:bg-gray-darkest dark:text-white',
'group flex items-center justify-between px-3 py-1.5 text-sm font-semibold dark:text-white',
props.error
? 'bg-red-300 dark:bg-red-500'
: 'bg-gray-300 dark:bg-gray-darkest',
Opacity.LOW,
)}
onClick={toggleAccountNotifications}
Expand Down Expand Up @@ -103,6 +113,8 @@ export const AccountNotifications: FC<IAccountNotifications> = (

{showAccountNotifications && (
<>
{props.error && <Oops error={props.error} />}
{!hasNotifications && !props.error && <AllRead />}
{groupByRepository
? Object.values(groupedNotifications).map((repoNotifications) => {
const repoSlug = repoNotifications[0].repository.full_name;
Expand Down
2 changes: 1 addition & 1 deletion src/components/AllRead.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ export const AllRead: FC = () => {

return (
<div className="flex flex-1 flex-col items-center justify-center bg-white p-4 text-black dark:bg-gray-dark dark:text-white">
<h1 className="mb-5 text-5xl">{emoji}</h1>
<h1 className="mt-2 mb-5 text-5xl">{emoji}</h1>

<h2 className="mb-2 text-xl font-semibold">No new notifications.</h2>
</div>
Expand Down
2 changes: 1 addition & 1 deletion src/components/Oops.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ export const Oops: FC<IOops> = ({ error }: IOops) => {

return (
<div className="flex flex-1 flex-col items-center justify-center p-4">
<h1 className="mb-5 text-5xl">{emoji}</h1>
<h1 className="mt-2 mb-5 text-5xl">{emoji}</h1>

<h2 className="mb-2 text-xl font-semibold">{error.title}</h2>
{error.descriptions.map((description, i) => {
Expand Down
Loading