From d44a12284223627e87493089a314c63bf5daf8b6 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Fri, 24 Jan 2025 12:28:54 -0500 Subject: [PATCH 1/5] refactor: migrate to text inputs to primer Signed-off-by: Adam Setch --- .../components/fields/FieldInput.test.tsx | 27 - src/renderer/components/fields/FieldInput.tsx | 57 - .../__snapshots__/FieldInput.test.tsx.snap | 114 -- src/renderer/components/layout/Contents.tsx | 7 +- src/renderer/components/layout/Page.test.tsx | 18 +- src/renderer/components/layout/Page.tsx | 7 +- .../__snapshots__/Contents.test.tsx.snap | 4 +- .../layout/__snapshots__/Page.test.tsx.snap | 81 +- src/renderer/components/primitives/Footer.tsx | 15 +- src/renderer/components/primitives/Header.tsx | 40 +- .../__snapshots__/Footer.test.tsx.snap | 68 +- .../__snapshots__/Header.test.tsx.snap | 158 ++- src/renderer/routes/Accounts.tsx | 11 +- src/renderer/routes/Filters.tsx | 2 +- .../routes/LoginWithOAuthApp.test.tsx | 16 +- src/renderer/routes/LoginWithOAuthApp.tsx | 279 ++-- .../LoginWithPersonalAccessToken.test.tsx | 10 +- .../routes/LoginWithPersonalAccessToken.tsx | 289 ++-- src/renderer/routes/Settings.tsx | 2 +- .../__snapshots__/Accounts.test.tsx.snap | 612 ++++---- .../__snapshots__/Filters.test.tsx.snap | 208 +-- .../LoginWithOAuthApp.test.tsx.snap | 1264 ++++++++++------- ...LoginWithPersonalAccessToken.test.tsx.snap | 1082 ++++++++------ .../__snapshots__/Settings.test.tsx.snap | 324 ++--- src/renderer/utils/auth/utils.ts | 4 + 25 files changed, 2541 insertions(+), 2158 deletions(-) delete mode 100644 src/renderer/components/fields/FieldInput.test.tsx delete mode 100644 src/renderer/components/fields/FieldInput.tsx delete mode 100644 src/renderer/components/fields/__snapshots__/FieldInput.test.tsx.snap diff --git a/src/renderer/components/fields/FieldInput.test.tsx b/src/renderer/components/fields/FieldInput.test.tsx deleted file mode 100644 index 0e2035a9a..000000000 --- a/src/renderer/components/fields/FieldInput.test.tsx +++ /dev/null @@ -1,27 +0,0 @@ -import { render } from '@testing-library/react'; -import { Form } from 'react-final-form'; -import { FieldInput, type IFieldInput } from './FieldInput'; - -describe('renderer/components/fields/FieldInput.tsx', () => { - const props: IFieldInput = { - name: 'appearance', - label: 'Appearance', - placeholder: 'This is some placeholder text', - helpText: 'This is some helper text', - }; - - it('should render', () => { - const tree = render( -
{}} - hand - render={({ handleSubmit }) => ( - - - - )} - />, - ); - expect(tree).toMatchSnapshot(); - }); -}); diff --git a/src/renderer/components/fields/FieldInput.tsx b/src/renderer/components/fields/FieldInput.tsx deleted file mode 100644 index c58207c8f..000000000 --- a/src/renderer/components/fields/FieldInput.tsx +++ /dev/null @@ -1,57 +0,0 @@ -import type { FC, ReactNode } from 'react'; -import { Field } from 'react-final-form'; -import { cn } from '../../utils/cn'; - -export interface IFieldInput { - name: string; - type?: string; - label: string; - placeholder?: string; - helpText?: ReactNode | string; - required?: boolean; -} - -export const FieldInput: FC = ({ - label, - name, - placeholder, - helpText, - type = 'text', - required = false, -}) => { - return ( - - {({ input, meta: { touched, error } }) => ( -
- - - - - {helpText && ( -
{helpText}
- )} - - {touched && error && ( -
{error}
- )} -
- )} - - ); -}; diff --git a/src/renderer/components/fields/__snapshots__/FieldInput.test.tsx.snap b/src/renderer/components/fields/__snapshots__/FieldInput.test.tsx.snap deleted file mode 100644 index f2d15383f..000000000 --- a/src/renderer/components/fields/__snapshots__/FieldInput.test.tsx.snap +++ /dev/null @@ -1,114 +0,0 @@ -// Jest Snapshot v1, https://goo.gl/fbAQLP - -exports[`renderer/components/fields/FieldInput.tsx should render 1`] = ` -{ - "asFragment": [Function], - "baseElement": -
-
-
- - -
- This is some helper text -
-
-
-
- , - "container":
-
-
- - -
- This is some helper text -
-
-
-
, - "debug": [Function], - "findAllByAltText": [Function], - "findAllByDisplayValue": [Function], - "findAllByLabelText": [Function], - "findAllByPlaceholderText": [Function], - "findAllByRole": [Function], - "findAllByTestId": [Function], - "findAllByText": [Function], - "findAllByTitle": [Function], - "findByAltText": [Function], - "findByDisplayValue": [Function], - "findByLabelText": [Function], - "findByPlaceholderText": [Function], - "findByRole": [Function], - "findByTestId": [Function], - "findByText": [Function], - "findByTitle": [Function], - "getAllByAltText": [Function], - "getAllByDisplayValue": [Function], - "getAllByLabelText": [Function], - "getAllByPlaceholderText": [Function], - "getAllByRole": [Function], - "getAllByTestId": [Function], - "getAllByText": [Function], - "getAllByTitle": [Function], - "getByAltText": [Function], - "getByDisplayValue": [Function], - "getByLabelText": [Function], - "getByPlaceholderText": [Function], - "getByRole": [Function], - "getByTestId": [Function], - "getByText": [Function], - "getByTitle": [Function], - "queryAllByAltText": [Function], - "queryAllByDisplayValue": [Function], - "queryAllByLabelText": [Function], - "queryAllByPlaceholderText": [Function], - "queryAllByRole": [Function], - "queryAllByTestId": [Function], - "queryAllByText": [Function], - "queryAllByTitle": [Function], - "queryByAltText": [Function], - "queryByDisplayValue": [Function], - "queryByLabelText": [Function], - "queryByPlaceholderText": [Function], - "queryByRole": [Function], - "queryByTestId": [Function], - "queryByText": [Function], - "queryByTitle": [Function], - "rerender": [Function], - "unmount": [Function], -} -`; diff --git a/src/renderer/components/layout/Contents.tsx b/src/renderer/components/layout/Contents.tsx index d7d4a1e25..b50b08272 100644 --- a/src/renderer/components/layout/Contents.tsx +++ b/src/renderer/components/layout/Contents.tsx @@ -1,3 +1,4 @@ +import { Box } from '@primer/react'; import type { FC, ReactNode } from 'react'; interface IContents { @@ -5,5 +6,9 @@ interface IContents { } export const Contents: FC = (props: IContents) => { - return
{props.children}
; + return ( + + {props.children} + + ); }; diff --git a/src/renderer/components/layout/Page.test.tsx b/src/renderer/components/layout/Page.test.tsx index 9c1727373..bf51c7ec6 100644 --- a/src/renderer/components/layout/Page.test.tsx +++ b/src/renderer/components/layout/Page.test.tsx @@ -2,22 +2,8 @@ import { render } from '@testing-library/react'; import { Page } from './Page'; describe('renderer/components/layout/Page.tsx', () => { - it('should render itself & its children - full', () => { - const tree = render( - - Test - , - ); - - expect(tree).toMatchSnapshot(); - }); - - it('should render itself & its children - screen', () => { - const tree = render( - - Test - , - ); + it('should render itself & its children', () => { + const tree = render(Test); expect(tree).toMatchSnapshot(); }); diff --git a/src/renderer/components/layout/Page.tsx b/src/renderer/components/layout/Page.tsx index 2d335f6bb..123ccdac0 100644 --- a/src/renderer/components/layout/Page.tsx +++ b/src/renderer/components/layout/Page.tsx @@ -1,16 +1,15 @@ +import { Box } from '@primer/react'; import type { FC, ReactNode } from 'react'; -import { cn } from '../../utils/cn'; interface IPage { children: ReactNode; id: string; - type: 'h-full' | 'h-screen'; } export const Page: FC = (props: IPage) => { return ( -
+ {props.children} -
+ ); }; diff --git a/src/renderer/components/layout/__snapshots__/Contents.test.tsx.snap b/src/renderer/components/layout/__snapshots__/Contents.test.tsx.snap index 94a38f280..ef2f6fb2f 100644 --- a/src/renderer/components/layout/__snapshots__/Contents.test.tsx.snap +++ b/src/renderer/components/layout/__snapshots__/Contents.test.tsx.snap @@ -6,7 +6,7 @@ exports[`renderer/components/layout/Contents.tsx should render itself & its chil "baseElement":
Test
@@ -14,7 +14,7 @@ exports[`renderer/components/layout/Contents.tsx should render itself & its chil , "container":
Test
diff --git a/src/renderer/components/layout/__snapshots__/Page.test.tsx.snap b/src/renderer/components/layout/__snapshots__/Page.test.tsx.snap index 4fbafd1e1..2942df524 100644 --- a/src/renderer/components/layout/__snapshots__/Page.test.tsx.snap +++ b/src/renderer/components/layout/__snapshots__/Page.test.tsx.snap @@ -1,12 +1,12 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`renderer/components/layout/Page.tsx should render itself & its children - full 1`] = ` +exports[`renderer/components/layout/Page.tsx should render itself & its children 1`] = ` { "asFragment": [Function], "baseElement":
Test @@ -15,82 +15,7 @@ exports[`renderer/components/layout/Page.tsx should render itself & its children , "container":
- Test -
-
, - "debug": [Function], - "findAllByAltText": [Function], - "findAllByDisplayValue": [Function], - "findAllByLabelText": [Function], - "findAllByPlaceholderText": [Function], - "findAllByRole": [Function], - "findAllByTestId": [Function], - "findAllByText": [Function], - "findAllByTitle": [Function], - "findByAltText": [Function], - "findByDisplayValue": [Function], - "findByLabelText": [Function], - "findByPlaceholderText": [Function], - "findByRole": [Function], - "findByTestId": [Function], - "findByText": [Function], - "findByTitle": [Function], - "getAllByAltText": [Function], - "getAllByDisplayValue": [Function], - "getAllByLabelText": [Function], - "getAllByPlaceholderText": [Function], - "getAllByRole": [Function], - "getAllByTestId": [Function], - "getAllByText": [Function], - "getAllByTitle": [Function], - "getByAltText": [Function], - "getByDisplayValue": [Function], - "getByLabelText": [Function], - "getByPlaceholderText": [Function], - "getByRole": [Function], - "getByTestId": [Function], - "getByText": [Function], - "getByTitle": [Function], - "queryAllByAltText": [Function], - "queryAllByDisplayValue": [Function], - "queryAllByLabelText": [Function], - "queryAllByPlaceholderText": [Function], - "queryAllByRole": [Function], - "queryAllByTestId": [Function], - "queryAllByText": [Function], - "queryAllByTitle": [Function], - "queryByAltText": [Function], - "queryByDisplayValue": [Function], - "queryByLabelText": [Function], - "queryByPlaceholderText": [Function], - "queryByRole": [Function], - "queryByTestId": [Function], - "queryByText": [Function], - "queryByTitle": [Function], - "rerender": [Function], - "unmount": [Function], -} -`; - -exports[`renderer/components/layout/Page.tsx should render itself & its children - screen 1`] = ` -{ - "asFragment": [Function], - "baseElement": -
-
- Test -
-
- , - "container":
-
Test diff --git a/src/renderer/components/primitives/Footer.tsx b/src/renderer/components/primitives/Footer.tsx index c1d2d637b..861d9f95c 100644 --- a/src/renderer/components/primitives/Footer.tsx +++ b/src/renderer/components/primitives/Footer.tsx @@ -1,4 +1,4 @@ -import { Stack } from '@primer/react'; +import { Box, Stack } from '@primer/react'; import type { FC, ReactNode } from 'react'; interface IFooter { @@ -8,13 +8,10 @@ interface IFooter { export const Footer: FC = (props: IFooter) => { return ( - - {props.children} - + + + {props.children} + + ); }; diff --git a/src/renderer/components/primitives/Header.tsx b/src/renderer/components/primitives/Header.tsx index 189474a11..1d72ffb42 100644 --- a/src/renderer/components/primitives/Header.tsx +++ b/src/renderer/components/primitives/Header.tsx @@ -2,7 +2,7 @@ import { type FC, useContext } from 'react'; import { useNavigate } from 'react-router-dom'; import { ArrowLeftIcon, type Icon } from '@primer/octicons-react'; -import { IconButton, Stack } from '@primer/react'; +import { Box, IconButton, Stack } from '@primer/react'; import { AppContext } from '../../context/App'; import { Title } from './Title'; @@ -19,24 +19,26 @@ export const Header: FC = (props: IHeader) => { const { fetchNotifications } = useContext(AppContext); return ( - - { - navigate(-1); - if (props.fetchOnBack) { - fetchNotifications(); - } - }} - data-testid="header-nav-back" - /> + + + { + navigate(-1); + if (props.fetchOnBack) { + fetchNotifications(); + } + }} + data-testid="header-nav-back" + /> - - {props.children} - - + + {props.children} + + + ); }; diff --git a/src/renderer/components/primitives/__snapshots__/Footer.test.tsx.snap b/src/renderer/components/primitives/__snapshots__/Footer.test.tsx.snap index 10e487af2..d347c3a50 100644 --- a/src/renderer/components/primitives/__snapshots__/Footer.test.tsx.snap +++ b/src/renderer/components/primitives/__snapshots__/Footer.test.tsx.snap @@ -6,7 +6,27 @@ exports[`renderer/components/primitives/Footer.tsx should render itself & its ch "baseElement":
+
+ Test +
+
+
+ , + "container":
+
+
- , - "container":
-
- Test -
, "debug": [Function], "findAllByAltText": [Function], @@ -89,7 +97,27 @@ exports[`renderer/components/primitives/Footer.tsx should render itself & its ch "baseElement":
+
+ Test +
+
+
+ , + "container":
+
+
- , - "container":
-
- Test -
, "debug": [Function], "findAllByAltText": [Function], diff --git a/src/renderer/components/primitives/__snapshots__/Header.test.tsx.snap b/src/renderer/components/primitives/__snapshots__/Header.test.tsx.snap index 7c4323457..f05169e64 100644 --- a/src/renderer/components/primitives/__snapshots__/Header.test.tsx.snap +++ b/src/renderer/components/primitives/__snapshots__/Header.test.tsx.snap @@ -5,12 +5,94 @@ exports[`renderer/components/primitives/Header.tsx should render itself & its ch "asFragment": [Function], "baseElement":
+
+
+ + +
+ +

+ Test Header +

+
+ +
+
+
+ , + "container":
+
- , - "container":
-
- - -
- -

- Test Header -

-
- -
, "debug": [Function], "findAllByAltText": [Function], diff --git a/src/renderer/routes/Accounts.tsx b/src/renderer/routes/Accounts.tsx index bc411c52c..cf22e5e31 100644 --- a/src/renderer/routes/Accounts.tsx +++ b/src/renderer/routes/Accounts.tsx @@ -28,9 +28,12 @@ import { Footer } from '../components/primitives/Footer'; import { Header } from '../components/primitives/Header'; import { AppContext } from '../context/App'; import { type Account, Size } from '../types'; -import { getAccountUUID, refreshAccount } from '../utils/auth/utils'; +import { + formatRequiredScopes, + getAccountUUID, + refreshAccount, +} from '../utils/auth/utils'; import { updateTrayIcon, updateTrayTitle } from '../utils/comms'; -import { Constants } from '../utils/constants'; import { getAuthMethodIcon, getPlatformIcon } from '../utils/icons'; import { openAccountProfile, @@ -77,7 +80,7 @@ export const AccountsRoute: FC = () => { }, []); return ( - +
Accounts
@@ -164,7 +167,7 @@ export const AccountsRoute: FC = () => { openDeveloperSettings(account)} size="small" diff --git a/src/renderer/routes/Filters.tsx b/src/renderer/routes/Filters.tsx index b46d71e29..9996608f3 100644 --- a/src/renderer/routes/Filters.tsx +++ b/src/renderer/routes/Filters.tsx @@ -38,7 +38,7 @@ export const FiltersRoute: FC = () => { }; return ( - +
Filters
diff --git a/src/renderer/routes/LoginWithOAuthApp.test.tsx b/src/renderer/routes/LoginWithOAuthApp.test.tsx index 7a8c6b9dd..1bea30bd8 100644 --- a/src/renderer/routes/LoginWithOAuthApp.test.tsx +++ b/src/renderer/routes/LoginWithOAuthApp.test.tsx @@ -3,7 +3,7 @@ import { MemoryRouter } from 'react-router-dom'; import { AppContext } from '../context/App'; import type { AuthState, ClientID, ClientSecret, Hostname } from '../types'; import * as comms from '../utils/comms'; -import { LoginWithOAuthAppRoute, validate } from './LoginWithOAuthApp'; +import { LoginWithOAuthAppRoute, validateForm } from './LoginWithOAuthApp'; const mockNavigate = jest.fn(); jest.mock('react-router-dom', () => ({ @@ -62,9 +62,9 @@ describe('renderer/routes/LoginWithOAuthApp.tsx', () => { let values = { ...emptyValues, }; - expect(validate(values).hostname).toBe('Required'); - expect(validate(values).clientId).toBe('Required'); - expect(validate(values).clientSecret).toBe('Required'); + expect(validateForm(values).hostname).toBe('Hostname is required'); + expect(validateForm(values).clientId).toBe('Client ID is required'); + expect(validateForm(values).clientSecret).toBe('Client Secret is required'); values = { ...emptyValues, @@ -72,9 +72,11 @@ describe('renderer/routes/LoginWithOAuthApp.tsx', () => { clientId: '!@£INVALID-.1' as ClientID, clientSecret: '!@£INVALID-.1' as ClientSecret, }; - expect(validate(values).hostname).toBe('Invalid hostname.'); - expect(validate(values).clientId).toBe('Invalid client id.'); - expect(validate(values).clientSecret).toBe('Invalid client secret.'); + expect(validateForm(values).hostname).toBe('Hostname is invalid'); + expect(validateForm(values).clientId).toBe('Client ID format is invalid'); + expect(validateForm(values).clientSecret).toBe( + 'Client Secret format is invalid', + ); }); describe("'Create new OAuth App' button", () => { diff --git a/src/renderer/routes/LoginWithOAuthApp.tsx b/src/renderer/routes/LoginWithOAuthApp.tsx index da22b9dd3..c1e099a5d 100644 --- a/src/renderer/routes/LoginWithOAuthApp.tsx +++ b/src/renderer/routes/LoginWithOAuthApp.tsx @@ -1,13 +1,22 @@ -import { type FC, useCallback, useContext } from 'react'; -import { Form, type FormRenderProps } from 'react-final-form'; +import { type FC, useCallback, useContext, useMemo, useState } from 'react'; import { useNavigate } from 'react-router-dom'; import { BookIcon, PersonIcon, SignInIcon } from '@primer/octicons-react'; -import { Button, Stack, Text, Tooltip } from '@primer/react'; +import { + Box, + Button, + FormControl, + Stack, + Text, + TextInput, + Tooltip, +} from '@primer/react'; +import { Banner } from '@primer/react/experimental'; import { logError } from '../../shared/logger'; -import { FieldInput } from '../components/fields/FieldInput'; +import { Contents } from '../components/layout/Contents'; import { Page } from '../components/layout/Page'; +import { Footer } from '../components/primitives/Footer'; import { Header } from '../components/primitives/Header'; import { AppContext } from '../context/App'; import type { ClientID, ClientSecret, Hostname, Token } from '../types'; @@ -21,7 +30,7 @@ import { import { openExternalLink } from '../utils/comms'; import { Constants } from '../utils/constants'; -interface IValues { +interface IFormData { hostname?: Hostname; clientId?: ClientID; clientSecret?: ClientSecret; @@ -33,25 +42,25 @@ interface IFormErrors { clientSecret?: string; } -export const validate = (values: IValues): IFormErrors => { +export const validateForm = (values: IFormData): IFormErrors => { const errors: IFormErrors = {}; if (!values.hostname) { - errors.hostname = 'Required'; + errors.hostname = 'Hostname is required'; } else if (!isValidHostname(values.hostname)) { - errors.hostname = 'Invalid hostname.'; + errors.hostname = 'Hostname is invalid'; } if (!values.clientId) { - errors.clientId = 'Required'; + errors.clientId = 'Client ID is required'; } else if (!isValidClientId(values.clientId)) { - errors.clientId = 'Invalid client id.'; + errors.clientId = 'Client ID format is invalid'; } if (!values.clientSecret) { - errors.clientSecret = 'Required'; + errors.clientSecret = 'Client Secret is required'; } else if (!isValidToken(values.clientSecret as unknown as Token)) { - errors.clientSecret = 'Invalid client secret.'; + errors.clientSecret = 'Client Secret format is invalid'; } return errors; @@ -62,110 +71,180 @@ export const LoginWithOAuthAppRoute: FC = () => { const { loginWithOAuthApp } = useContext(AppContext); - const renderForm = (formProps: FormRenderProps) => { - const { handleSubmit, submitting, pristine, values } = formProps; + const [isValidCredentialsForHost, setIsValidCredentialsForHost] = + useState(true); - return ( - -
- - - - on GitHub then paste your - - - client id and client secret below. - - - } - /> - - - - - - - - - + const [formData, setFormData] = useState({ + hostname: 'github.com' as Hostname, + clientId: '' as ClientID, + clientSecret: '' as ClientSecret, + } as IFormData); - - - - - -
+ const [errors, setErrors] = useState({} as IFormErrors); + + const hasErrors = useMemo(() => { + return ( + Object.values(errors).some((error) => error !== '') || + !isValidCredentialsForHost ); + }, [errors, isValidCredentialsForHost]); + + const handleSubmit = async (e: React.FormEvent) => { + e.preventDefault(); + + const newErrors = validateForm(formData); + + setErrors(newErrors); + + if (!newErrors.hostname && !newErrors.clientId && !newErrors.clientSecret) { + verifyLoginCredentials(formData); + } + }; + + const handleInputChange = (e: React.ChangeEvent) => { + const { name, value } = e.target; + setFormData({ + ...formData, + [name]: value, + }); }; - const login = useCallback( - async (data: IValues) => { + const verifyLoginCredentials = useCallback( + async (data: IFormData) => { + setIsValidCredentialsForHost(true); try { await loginWithOAuthApp(data as LoginOAuthAppOptions); navigate(-1); } catch (err) { logError('loginWithOAuthApp', 'Failed to login with OAuth App', err); + setIsValidCredentialsForHost(false); } }, [loginWithOAuthApp], ); return ( -
+
Login with OAuth App
-
-
- {renderForm} -
-
-
+ + + + {hasErrors && ( + + + {errors.hostname && {errors.hostname}} + {errors.clientId && {errors.clientId}} + {errors.clientSecret && {errors.clientSecret}} + {!isValidCredentialsForHost && ( + + Failed to validate provided Client ID and Secret against{' '} + {formData.hostname} + + )} + + + } + /> + )} + + + Hostname + + + Change only if you are using GitHub Enterprise Server + + + + + + {/* */} + + + and use your client id/secret below. + + {/* */} + + + Client ID + + + + Client Secret + + + + + +
+ + + + + +
+ + ); }; diff --git a/src/renderer/routes/LoginWithPersonalAccessToken.test.tsx b/src/renderer/routes/LoginWithPersonalAccessToken.test.tsx index 16bbb5085..c4fffe2f3 100644 --- a/src/renderer/routes/LoginWithPersonalAccessToken.test.tsx +++ b/src/renderer/routes/LoginWithPersonalAccessToken.test.tsx @@ -4,7 +4,7 @@ import { AppContext } from '../context/App'; import * as comms from '../utils/comms'; import { LoginWithPersonalAccessTokenRoute, - validate, + validateForm, } from './LoginWithPersonalAccessToken'; const mockNavigate = jest.fn(); @@ -54,16 +54,16 @@ describe('renderer/routes/LoginWithPersonalAccessToken.tsx', () => { let values = { ...emptyValues, }; - expect(validate(values).hostname).toBe('Required'); - expect(validate(values).token).toBe('Required'); + expect(validateForm(values).hostname).toBe('Required'); + expect(validateForm(values).token).toBe('Required'); values = { ...emptyValues, hostname: 'hello', token: '!@£INVALID-.1', }; - expect(validate(values).hostname).toBe('Invalid hostname.'); - expect(validate(values).token).toBe('Invalid token.'); + expect(validateForm(values).hostname).toBe('Invalid hostname.'); + expect(validateForm(values).token).toBe('Invalid token.'); }); describe("'Generate a PAT' button", () => { diff --git a/src/renderer/routes/LoginWithPersonalAccessToken.tsx b/src/renderer/routes/LoginWithPersonalAccessToken.tsx index 540e3c825..7e410620a 100644 --- a/src/renderer/routes/LoginWithPersonalAccessToken.tsx +++ b/src/renderer/routes/LoginWithPersonalAccessToken.tsx @@ -1,18 +1,34 @@ -import { type FC, useCallback, useContext, useState } from 'react'; -import { Form, type FormRenderProps } from 'react-final-form'; +import { type FC, useCallback, useContext, useMemo, useState } from 'react'; import { useNavigate } from 'react-router-dom'; -import { BookIcon, KeyIcon, SignInIcon } from '@primer/octicons-react'; -import { Button, Stack, Text, Tooltip } from '@primer/react'; +import { + BookIcon, + EyeClosedIcon, + EyeIcon, + KeyIcon, + SignInIcon, +} from '@primer/octicons-react'; +import { + Box, + Button, + FormControl, + Stack, + Text, + TextInput, + Tooltip, +} from '@primer/react'; +import { Banner } from '@primer/react/experimental'; import { logError } from '../../shared/logger'; -import { FieldInput } from '../components/fields/FieldInput'; +import { Contents } from '../components/layout/Contents'; import { Page } from '../components/layout/Page'; +import { Footer } from '../components/primitives/Footer'; import { Header } from '../components/primitives/Header'; import { AppContext } from '../context/App'; import type { Hostname, Token } from '../types'; import type { LoginPersonalAccessTokenOptions } from '../utils/auth/types'; import { + formatRequiredScopes, getNewTokenURL, isValidHostname, isValidToken, @@ -20,7 +36,7 @@ import { import { openExternalLink } from '../utils/comms'; import { Constants } from '../utils/constants'; -interface IValues { +interface IFormData { token?: Token; hostname?: Hostname; } @@ -30,106 +46,70 @@ interface IFormErrors { hostname?: string; } -export const validate = (values: IValues): IFormErrors => { +export const validateForm = (values: IFormData): IFormErrors => { const errors: IFormErrors = {}; if (!values.hostname) { - errors.hostname = 'Required'; + errors.hostname = 'Hostname is required'; } else if (!isValidHostname(values.hostname)) { - errors.hostname = 'Invalid hostname.'; + errors.hostname = 'Hostname format is invalid'; } if (!values.token) { - errors.token = 'Required'; + errors.token = 'Token is required'; } else if (!isValidToken(values.token)) { - errors.token = 'Invalid token.'; + errors.token = 'Token format is invalid (must be 40 characters long)'; } return errors; }; export const LoginWithPersonalAccessTokenRoute: FC = () => { - const { loginWithPersonalAccessToken } = useContext(AppContext); const navigate = useNavigate(); - const [isValidToken, setIsValidToken] = useState(true); - const renderForm = (formProps: FormRenderProps) => { - const { handleSubmit, submitting, pristine, values } = formProps; + const { loginWithPersonalAccessToken } = useContext(AppContext); + + const [isValidCredentialsForHost, setIsValidCredentialsForHost] = + useState(true); + const [showPassword, setShowPassword] = useState(false); + + const [formData, setFormData] = useState({ + hostname: 'github.com' as Hostname, + token: '' as Token, + } as IFormData); + const [errors, setErrors] = useState({} as IFormErrors); + + const hasErrors = useMemo(() => { return ( - -
- - - Change only if you are using GitHub Enterprise Server. - - - - on GitHub then paste your token below. - - - The required scopes will be automatically selected for you. - - - } - /> - - {!isValidToken && ( -
- This token could not be validated with {values.hostname}. -
- )} - - - - - - - - - - -
+ Object.values(errors).some((error) => error !== '') || + !isValidCredentialsForHost ); + }, [errors, isValidCredentialsForHost]); + + const handleSubmit = async (e: React.FormEvent) => { + e.preventDefault(); + + const newErrors = validateForm(formData); + + setErrors(newErrors); + + if (!newErrors.hostname && !newErrors.token) { + verifyLoginCredentials(formData); + } + }; + + const handleInputChange = (e: React.ChangeEvent) => { + const { name, value } = e.target; + setFormData({ + ...formData, + [name]: value, + }); }; - const login = useCallback( - async (data: IValues) => { - setIsValidToken(true); + const verifyLoginCredentials = useCallback( + async (data: IFormData) => { + setIsValidCredentialsForHost(true); try { await loginWithPersonalAccessToken( data as LoginPersonalAccessTokenOptions, @@ -141,28 +121,137 @@ export const LoginWithPersonalAccessTokenRoute: FC = () => { 'Failed to login with PAT', err, ); - setIsValidToken(false); + setIsValidCredentialsForHost(false); } }, [loginWithPersonalAccessToken], ); return ( - <> +
Login with Personal Access Token
-
-
- {renderForm} -
-
- + + + {hasErrors && ( + + + {errors.hostname && {errors.hostname}} + {errors.token && {errors.token}} + {!isValidCredentialsForHost && ( + + Failed to validate provided token against{' '} + {formData.hostname} + + )} + + + } + /> + )} + + + Hostname + + + Change only if you are using GitHub Enterprise Server + + + + + + + + + + on GitHub to paste the token below. + + + + + The{' '} + + required scopes + {' '} + will be automatically selected for you. + + + + + Token + setShowPassword(!showPassword)} + icon={showPassword ? EyeClosedIcon : EyeIcon} + aria-label={showPassword ? 'Hide token' : 'Show token'} + /> + } + block + /> + + + + +
+ + + + + +
+ + ); }; diff --git a/src/renderer/routes/Settings.tsx b/src/renderer/routes/Settings.tsx index 26685da09..67ff6950b 100644 --- a/src/renderer/routes/Settings.tsx +++ b/src/renderer/routes/Settings.tsx @@ -23,7 +23,7 @@ export const SettingsRoute: FC = () => { }, [resetSettings]); return ( - +
Settings
diff --git a/src/renderer/routes/__snapshots__/Accounts.test.tsx.snap b/src/renderer/routes/__snapshots__/Accounts.test.tsx.snap index fcf558fff..8633f1dca 100644 --- a/src/renderer/routes/__snapshots__/Accounts.test.tsx.snap +++ b/src/renderer/routes/__snapshots__/Accounts.test.tsx.snap @@ -2,59 +2,34 @@ exports[`renderer/routes/Accounts.tsx Account interactions should render with PAT scopes warning 1`] = `
- - -
-

+ +
- Accounts -

-
- + +

+ Accounts +

+
+ +
- + +
`; exports[`renderer/routes/Accounts.tsx Account interactions should set account as primary account 1`] = `
- - -
-

+ +
- Accounts -

-
- + +

+ Accounts +

+
+ +
- + +
`; exports[`renderer/routes/Accounts.tsx General should render itself & its children 1`] = `
- - -
-

+ +
- Accounts -

-
- + +

+ Accounts +

+
+ +
- + +
`; diff --git a/src/renderer/routes/__snapshots__/Filters.test.tsx.snap b/src/renderer/routes/__snapshots__/Filters.test.tsx.snap index 1a288aa44..c79e05f1d 100644 --- a/src/renderer/routes/__snapshots__/Filters.test.tsx.snap +++ b/src/renderer/routes/__snapshots__/Filters.test.tsx.snap @@ -2,59 +2,34 @@ exports[`renderer/routes/Filters.tsx General should render itself & its children 1`] = `
- - -
-

+ +
- Filters -

-
- + +

+ Filters +

+
+ +
- - - + + +
`; diff --git a/src/renderer/routes/__snapshots__/LoginWithOAuthApp.test.tsx.snap b/src/renderer/routes/__snapshots__/LoginWithOAuthApp.test.tsx.snap index bbdccd7db..40ba05b54 100644 --- a/src/renderer/routes/__snapshots__/LoginWithOAuthApp.test.tsx.snap +++ b/src/renderer/routes/__snapshots__/LoginWithOAuthApp.test.tsx.snap @@ -5,57 +5,35 @@ exports[`renderer/routes/LoginWithOAuthApp.tsx renders correctly 1`] = ` "asFragment": [Function], "baseElement":
-
+
- - -
-

- Login with OAuth App -

-
- -
-
-
-
-
- - -
-
-
- - - on GitHub then paste your - -
- - - client id and client secret - - below. - -
-
-
-
- - -
-
- - -
+ +
- - + +

- - + Login with OAuth App +

- +
-
-
- , - "container":
-
-
- -
- -

- Login with OAuth App -

-
- -
-
-
-
- -
-
-
- - - on GitHub then paste your + Hostname +
+ + * -
- + + + + + + - - client id and client secret - - below. - -
+ Change only if you are using GitHub Enterprise Server + +
-
-
- - -
-
- - -
-
- - - + and use your + + client id/secret + + below. + +
+
-
+
+ + + + +
+
+
+
+
+ + + +
+
+ +
+
+ , + "container":
+
+
+
+ + +
+ +

+ Login with OAuth App +

- +
+
+
+
+
+ + + + + + + Change only if you are using GitHub Enterprise Server + + +
+
+ + + and use your + + client id/secret + + below. + +
+
+ + + + +
+
+ + + + +
+
+
+
+
+ + + + +
+
+
, "debug": [Function], diff --git a/src/renderer/routes/__snapshots__/LoginWithPersonalAccessToken.test.tsx.snap b/src/renderer/routes/__snapshots__/LoginWithPersonalAccessToken.test.tsx.snap index c62d9a7ec..302b59970 100644 --- a/src/renderer/routes/__snapshots__/LoginWithPersonalAccessToken.test.tsx.snap +++ b/src/renderer/routes/__snapshots__/LoginWithPersonalAccessToken.test.tsx.snap @@ -6,219 +6,357 @@ exports[`renderer/routes/LoginWithPersonalAccessToken.tsx renders correctly 1`] "baseElement":
- -
- -

- Login with Personal Access Token -

+ + + +
+ +

+ Login with Personal Access Token +

+
+
- -
-
-
+
- +
- - + + + + + + + Change only if you are using GitHub Enterprise Server + + +
- - Change only if you are using GitHub Enterprise Server. - -
+ + + + + Generate a PAT + + + + + on GitHub to paste the token below. + +
+ + The + + + + required scopes + + + + will be automatically selected for you. + +
+
+ + + + -
- - The required scopes will be automatically selected for you. - -
+ +
-
- - -
+
+
+ +
+
+ +
+
+ , + "container":
+
+
+
+ + +
+ +

+ Login with Personal Access Token +

+
+ +
+
+
+
+
+
+ + + + + + Change only if you are using GitHub Enterprise Server + + +
+
+
- + + on GitHub to paste the token below. + +
+ + The + + + + required scopes + + + + will be automatically selected for you. +
- -
-
-
- , - "container":
-
- - -
- -

- Login with Personal Access Token -

-
- -
-
-
-
-
- -
-
- - Change only if you are using GitHub Enterprise Server. - -
+ Token +
+ + * + + + + + + -
- - The required scopes will be automatically selected for you. - -
+ +
-
- - -
+
+
- - - + + Login + + +
- -
+
+
, "debug": [Function], diff --git a/src/renderer/routes/__snapshots__/Settings.test.tsx.snap b/src/renderer/routes/__snapshots__/Settings.test.tsx.snap index 73503354c..f5345dd2e 100644 --- a/src/renderer/routes/__snapshots__/Settings.test.tsx.snap +++ b/src/renderer/routes/__snapshots__/Settings.test.tsx.snap @@ -2,59 +2,34 @@ exports[`renderer/routes/Settings.tsx should render itself & its children 1`] = `
- - -
-

+ +
- Settings -

-
- + +

+ Settings +

+
+ +
- - - -
-
- + +
+
- - - - + + - - - + + + +
diff --git a/src/renderer/utils/auth/utils.ts b/src/renderer/utils/auth/utils.ts index d0a3504ff..878ec920b 100644 --- a/src/renderer/utils/auth/utils.ts +++ b/src/renderer/utils/auth/utils.ts @@ -284,3 +284,7 @@ export function hasAccounts(auth: AuthState) { export function hasMultipleAccounts(auth: AuthState) { return auth.accounts.length > 1; } + +export function formatRequiredScopes() { + return Constants.AUTH_SCOPE.join(', '); +} From ec27b09e8ce02e47805b2f4636c976ecbf92fcd4 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Fri, 24 Jan 2025 14:27:39 -0500 Subject: [PATCH 2/5] refactor: migrate to text inputs to primer Signed-off-by: Adam Setch --- package.json | 1 - pnpm-lock.yaml | 15 - .../routes/LoginWithOAuthApp.test.tsx | 40 +- src/renderer/routes/LoginWithOAuthApp.tsx | 246 ++-- .../LoginWithPersonalAccessToken.test.tsx | 38 +- .../routes/LoginWithPersonalAccessToken.tsx | 240 ++-- .../LoginWithOAuthApp.test.tsx.snap | 852 ++++++------- ...LoginWithPersonalAccessToken.test.tsx.snap | 1084 ++++++++--------- 8 files changed, 1253 insertions(+), 1263 deletions(-) diff --git a/package.json b/package.json index ab1ba9f7f..29228184c 100644 --- a/package.json +++ b/package.json @@ -171,7 +171,6 @@ "nprogress": "0.2.0", "postcss": "8.5.1", "postcss-loader": "8.1.1", - "react-final-form": "6.5.9", "rimraf": "6.0.1", "semver": "7.6.3", "styled-components": "6.1.14", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 4870b9a01..9b3dd2103 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -138,9 +138,6 @@ importers: postcss-loader: specifier: 8.1.1 version: 8.1.1(postcss@8.5.1)(typescript@5.7.3)(webpack@5.97.1) - react-final-form: - specifier: 6.5.9 - version: 6.5.9(final-form@4.20.10)(react@19.0.0) rimraf: specifier: 6.0.1 version: 6.0.1 @@ -3638,12 +3635,6 @@ packages: peerDependencies: react: ^19.0.0 - react-final-form@6.5.9: - resolution: {integrity: sha512-x3XYvozolECp3nIjly+4QqxdjSSWfcnpGEL5K8OBT6xmGrq5kBqbA6+/tOqoom9NwqIPPbxPNsOViFlbKgowbA==} - peerDependencies: - final-form: ^4.20.4 - react: ^16.8.0 || ^17.0.0 || ^18.0.0 - react-intersection-observer@9.13.1: resolution: {integrity: sha512-tSzDaTy0qwNPLJHg8XZhlyHTgGW6drFKTtvjdL+p6um12rcnp8Z5XstE+QNBJ7c64n5o0Lj4ilUleA41bmDoMw==} peerDependencies: @@ -8598,12 +8589,6 @@ snapshots: react: 19.0.0 scheduler: 0.25.0 - react-final-form@6.5.9(final-form@4.20.10)(react@19.0.0): - dependencies: - '@babel/runtime': 7.24.1 - final-form: 4.20.10 - react: 19.0.0 - react-intersection-observer@9.13.1(react-dom@19.0.0(react@19.0.0))(react@19.0.0): dependencies: react: 19.0.0 diff --git a/src/renderer/routes/LoginWithOAuthApp.test.tsx b/src/renderer/routes/LoginWithOAuthApp.test.tsx index 1bea30bd8..e2f1398f8 100644 --- a/src/renderer/routes/LoginWithOAuthApp.test.tsx +++ b/src/renderer/routes/LoginWithOAuthApp.test.tsx @@ -72,10 +72,12 @@ describe('renderer/routes/LoginWithOAuthApp.tsx', () => { clientId: '!@£INVALID-.1' as ClientID, clientSecret: '!@£INVALID-.1' as ClientSecret, }; - expect(validateForm(values).hostname).toBe('Hostname is invalid'); - expect(validateForm(values).clientId).toBe('Client ID format is invalid'); + expect(validateForm(values).hostname).toBe('Hostname format is invalid'); + expect(validateForm(values).clientId).toBe( + 'Client ID format is invalid (must be 20 characters long)', + ); expect(validateForm(values).clientSecret).toBe( - 'Client Secret format is invalid', + 'Client Secret format is invalid (must be 40 characters long)', ); }); @@ -89,6 +91,10 @@ describe('renderer/routes/LoginWithOAuthApp.tsx', () => { , ); + fireEvent.change(screen.getByTestId('login-hostname'), { + target: { value: '' }, + }); + fireEvent.click(screen.getByTestId('login-create-oauth-app')); expect(openExternalLinkMock).toHaveBeenCalledTimes(0); @@ -103,7 +109,7 @@ describe('renderer/routes/LoginWithOAuthApp.tsx', () => { , ); - fireEvent.change(screen.getByLabelText('Hostname'), { + fireEvent.change(screen.getByTestId('login-hostname'), { target: { value: 'company.github.com' }, }); @@ -128,13 +134,13 @@ describe('renderer/routes/LoginWithOAuthApp.tsx', () => { , ); - fireEvent.change(screen.getByLabelText('Hostname'), { + fireEvent.change(screen.getByTestId('login-hostname'), { target: { value: 'github.com' }, }); - fireEvent.change(screen.getByLabelText('Client ID'), { + fireEvent.change(screen.getByTestId('login-clientId'), { target: { value: '1234567890_ASDFGHJKL' }, }); - fireEvent.change(screen.getByLabelText('Client Secret'), { + fireEvent.change(screen.getByTestId('login-clientSecret'), { target: { value: '1234567890_asdfghjklPOIUYTREWQ0987654321' }, }); @@ -155,21 +161,29 @@ describe('renderer/routes/LoginWithOAuthApp.tsx', () => { , ); - fireEvent.change(screen.getByLabelText('Hostname'), { + fireEvent.change(screen.getByTestId('login-hostname'), { target: { value: 'test' }, }); - fireEvent.change(screen.getByLabelText('Client ID'), { + fireEvent.change(screen.getByTestId('login-clientId'), { target: { value: '123' }, }); - fireEvent.change(screen.getByLabelText('Client Secret'), { + fireEvent.change(screen.getByTestId('login-clientSecret'), { target: { value: 'abc' }, }); fireEvent.click(screen.getByTestId('login-submit')); - expect(screen.getByText('Invalid hostname.')).toBeTruthy(); - expect(screen.getByText('Invalid client id.')).toBeTruthy(); - expect(screen.getByText('Invalid client secret.')).toBeTruthy(); + expect(screen.getByText('Hostname format is invalid')).toBeTruthy(); + expect( + screen.getByText( + 'Client ID format is invalid (must be 20 characters long)', + ), + ).toBeTruthy(); + expect( + screen.getByText( + 'Client Secret format is invalid (must be 40 characters long)', + ), + ).toBeTruthy(); }); it('should open help docs in the browser', async () => { diff --git a/src/renderer/routes/LoginWithOAuthApp.tsx b/src/renderer/routes/LoginWithOAuthApp.tsx index c1e099a5d..bc6c966d0 100644 --- a/src/renderer/routes/LoginWithOAuthApp.tsx +++ b/src/renderer/routes/LoginWithOAuthApp.tsx @@ -3,7 +3,6 @@ import { useNavigate } from 'react-router-dom'; import { BookIcon, PersonIcon, SignInIcon } from '@primer/octicons-react'; import { - Box, Button, FormControl, Stack, @@ -40,6 +39,7 @@ interface IFormErrors { hostname?: string; clientId?: string; clientSecret?: string; + invalidCredentialsForHost?: string; } export const validateForm = (values: IFormData): IFormErrors => { @@ -48,19 +48,21 @@ export const validateForm = (values: IFormData): IFormErrors => { if (!values.hostname) { errors.hostname = 'Hostname is required'; } else if (!isValidHostname(values.hostname)) { - errors.hostname = 'Hostname is invalid'; + errors.hostname = 'Hostname format is invalid'; } if (!values.clientId) { errors.clientId = 'Client ID is required'; } else if (!isValidClientId(values.clientId)) { - errors.clientId = 'Client ID format is invalid'; + errors.clientId = + 'Client ID format is invalid (must be 20 characters long)'; } if (!values.clientSecret) { errors.clientSecret = 'Client Secret is required'; } else if (!isValidToken(values.clientSecret as unknown as Token)) { - errors.clientSecret = 'Client Secret format is invalid'; + errors.clientSecret = + 'Client Secret format is invalid (must be 40 characters long)'; } return errors; @@ -71,9 +73,6 @@ export const LoginWithOAuthAppRoute: FC = () => { const { loginWithOAuthApp } = useContext(AppContext); - const [isValidCredentialsForHost, setIsValidCredentialsForHost] = - useState(true); - const [formData, setFormData] = useState({ hostname: 'github.com' as Hostname, clientId: '' as ClientID, @@ -83,15 +82,10 @@ export const LoginWithOAuthAppRoute: FC = () => { const [errors, setErrors] = useState({} as IFormErrors); const hasErrors = useMemo(() => { - return ( - Object.values(errors).some((error) => error !== '') || - !isValidCredentialsForHost - ); - }, [errors, isValidCredentialsForHost]); - - const handleSubmit = async (e: React.FormEvent) => { - e.preventDefault(); + return Object.values(errors).some((error) => error !== ''); + }, [errors]); + const handleSubmit = async () => { const newErrors = validateForm(formData); setErrors(newErrors); @@ -111,13 +105,14 @@ export const LoginWithOAuthAppRoute: FC = () => { const verifyLoginCredentials = useCallback( async (data: IFormData) => { - setIsValidCredentialsForHost(true); try { await loginWithOAuthApp(data as LoginOAuthAppOptions); navigate(-1); } catch (err) { logError('loginWithOAuthApp', 'Failed to login with OAuth App', err); - setIsValidCredentialsForHost(false); + setErrors({ + invalidCredentialsForHost: `Failed to validate provided Client ID and Secret against ${data.hostname}`, + }); } }, [loginWithOAuthApp], @@ -127,124 +122,123 @@ export const LoginWithOAuthAppRoute: FC = () => {
Login with OAuth App
- - - {hasErrors && ( - - - {errors.hostname && {errors.hostname}} - {errors.clientId && {errors.clientId}} - {errors.clientSecret && {errors.clientSecret}} - {!isValidCredentialsForHost && ( - - Failed to validate provided Client ID and Secret against{' '} - {formData.hostname} - - )} - - - } - /> - )} - - - Hostname - - - Change only if you are using GitHub Enterprise Server - - - - - - {/* */} - - - and use your client id/secret below. + + {hasErrors && ( + + + {errors.hostname && {errors.hostname}} + {errors.clientId && {errors.clientId}} + {errors.clientSecret && {errors.clientSecret}} + {errors.invalidCredentialsForHost && ( + {errors.invalidCredentialsForHost} + )} + - {/* */} - - - Client ID - - - - Client Secret - - - - - -
- + } + /> + )} + + + Hostname + + + Change only if you are using GitHub Enterprise Server + + + + + - + + and use your client id/secret below. + + + + Client ID + + + + Client Secret + + + + +
+ -
- + + + +
); }; diff --git a/src/renderer/routes/LoginWithPersonalAccessToken.test.tsx b/src/renderer/routes/LoginWithPersonalAccessToken.test.tsx index c4fffe2f3..e5d3caa65 100644 --- a/src/renderer/routes/LoginWithPersonalAccessToken.test.tsx +++ b/src/renderer/routes/LoginWithPersonalAccessToken.test.tsx @@ -54,16 +54,18 @@ describe('renderer/routes/LoginWithPersonalAccessToken.tsx', () => { let values = { ...emptyValues, }; - expect(validateForm(values).hostname).toBe('Required'); - expect(validateForm(values).token).toBe('Required'); + expect(validateForm(values).hostname).toBe('Hostname is required'); + expect(validateForm(values).token).toBe('Token is required'); values = { ...emptyValues, hostname: 'hello', token: '!@£INVALID-.1', }; - expect(validateForm(values).hostname).toBe('Invalid hostname.'); - expect(validateForm(values).token).toBe('Invalid token.'); + expect(validateForm(values).hostname).toBe('Hostname format is invalid'); + expect(validateForm(values).token).toBe( + 'Token format is invalid (must be 40 characters long)', + ); }); describe("'Generate a PAT' button", () => { @@ -80,7 +82,7 @@ describe('renderer/routes/LoginWithPersonalAccessToken.tsx', () => { , ); - fireEvent.change(screen.getByLabelText('Hostname'), { + fireEvent.change(screen.getByTestId('login-hostname'), { target: { value: '' }, }); @@ -123,12 +125,12 @@ describe('renderer/routes/LoginWithPersonalAccessToken.tsx', () => { , ); - fireEvent.change(screen.getByLabelText('Token'), { - target: { value: '1234567890123456789012345678901234567890' }, + fireEvent.change(screen.getByTestId('login-hostname'), { + target: { value: 'github.com' }, }); - fireEvent.change(screen.getByLabelText('Hostname'), { - target: { value: 'github.com' }, + fireEvent.change(screen.getByTestId('login-token'), { + target: { value: '1234567890123456789012345678901234567890' }, }); fireEvent.click(screen.getByTestId('login-submit')); @@ -156,12 +158,12 @@ describe('renderer/routes/LoginWithPersonalAccessToken.tsx', () => { , ); - fireEvent.change(screen.getByLabelText('Token'), { - target: { value: '1234567890123456789012345678901234567890' }, + fireEvent.change(screen.getByTestId('login-hostname'), { + target: { value: 'github.com' }, }); - fireEvent.change(screen.getByLabelText('Hostname'), { - target: { value: 'github.com' }, + fireEvent.change(screen.getByTestId('login-token'), { + target: { value: '1234567890123456789012345678901234567890' }, }); fireEvent.click(screen.getByTestId('login-submit')); @@ -181,17 +183,19 @@ describe('renderer/routes/LoginWithPersonalAccessToken.tsx', () => { , ); - fireEvent.change(screen.getByLabelText('Hostname'), { + fireEvent.change(screen.getByTestId('login-hostname'), { target: { value: 'test' }, }); - fireEvent.change(screen.getByLabelText('Token'), { + fireEvent.change(screen.getByTestId('login-token'), { target: { value: '123' }, }); fireEvent.click(screen.getByTestId('login-submit')); - expect(screen.getByText('Invalid hostname.')).toBeDefined(); - expect(screen.getByText('Invalid token.')).toBeDefined(); + expect(screen.getByText('Hostname format is invalid')).toBeDefined(); + expect( + screen.getByText('Token format is invalid (must be 40 characters long)'), + ).toBeDefined(); }); it('should open help docs in the browser', async () => { diff --git a/src/renderer/routes/LoginWithPersonalAccessToken.tsx b/src/renderer/routes/LoginWithPersonalAccessToken.tsx index 7e410620a..e384083f8 100644 --- a/src/renderer/routes/LoginWithPersonalAccessToken.tsx +++ b/src/renderer/routes/LoginWithPersonalAccessToken.tsx @@ -9,7 +9,6 @@ import { SignInIcon, } from '@primer/octicons-react'; import { - Box, Button, FormControl, Stack, @@ -44,6 +43,7 @@ interface IFormData { interface IFormErrors { token?: string; hostname?: string; + invalidCredentialsForHost?: string; } export const validateForm = (values: IFormData): IFormErrors => { @@ -69,8 +69,6 @@ export const LoginWithPersonalAccessTokenRoute: FC = () => { const { loginWithPersonalAccessToken } = useContext(AppContext); - const [isValidCredentialsForHost, setIsValidCredentialsForHost] = - useState(true); const [showPassword, setShowPassword] = useState(false); const [formData, setFormData] = useState({ @@ -81,15 +79,10 @@ export const LoginWithPersonalAccessTokenRoute: FC = () => { const [errors, setErrors] = useState({} as IFormErrors); const hasErrors = useMemo(() => { - return ( - Object.values(errors).some((error) => error !== '') || - !isValidCredentialsForHost - ); - }, [errors, isValidCredentialsForHost]); - - const handleSubmit = async (e: React.FormEvent) => { - e.preventDefault(); + return Object.values(errors).some((error) => error !== ''); + }, [errors]); + const handleSubmit = async () => { const newErrors = validateForm(formData); setErrors(newErrors); @@ -109,7 +102,6 @@ export const LoginWithPersonalAccessTokenRoute: FC = () => { const verifyLoginCredentials = useCallback( async (data: IFormData) => { - setIsValidCredentialsForHost(true); try { await loginWithPersonalAccessToken( data as LoginPersonalAccessTokenOptions, @@ -121,7 +113,9 @@ export const LoginWithPersonalAccessTokenRoute: FC = () => { 'Failed to login with PAT', err, ); - setIsValidCredentialsForHost(false); + setErrors({ + invalidCredentialsForHost: `Failed to validate provided token against ${data.hostname}`, + }); } }, [loginWithPersonalAccessToken], @@ -131,127 +125,125 @@ export const LoginWithPersonalAccessTokenRoute: FC = () => {
Login with Personal Access Token
- - - {hasErrors && ( - - - {errors.hostname && {errors.hostname}} - {errors.token && {errors.token}} - {!isValidCredentialsForHost && ( - - Failed to validate provided token against{' '} - {formData.hostname} - - )} - - - } + + {hasErrors && ( + + + {errors.hostname && {errors.hostname}} + {errors.token && {errors.token}} + {errors.invalidCredentialsForHost && ( + {errors.invalidCredentialsForHost} + )} + + + } + /> + )} + + + Hostname + + + Change only if you are using GitHub Enterprise Server + + + - )} - - - Hostname - - - Change only if you are using GitHub Enterprise Server - - - - - - - - - - on GitHub to paste the token below. - - - - - The{' '} - - required scopes - {' '} - will be automatically selected for you. + + + + + + + on GitHub to paste the token below. - - Token - setShowPassword(!showPassword)} - icon={showPassword ? EyeClosedIcon : EyeIcon} - aria-label={showPassword ? 'Hide token' : 'Show token'} - /> - } - block - /> - + + The{' '} + + required scopes + {' '} + will be automatically selected for you. + - -
- - - + + Token + setShowPassword(!showPassword)} + icon={showPassword ? EyeClosedIcon : EyeIcon} + aria-label={showPassword ? 'Hide token' : 'Show token'} + /> + } + data-testid="login-token" + block + /> + + + +
+ -
- + + + +
); }; diff --git a/src/renderer/routes/__snapshots__/LoginWithOAuthApp.test.tsx.snap b/src/renderer/routes/__snapshots__/LoginWithOAuthApp.test.tsx.snap index 40ba05b54..465035284 100644 --- a/src/renderer/routes/__snapshots__/LoginWithOAuthApp.test.tsx.snap +++ b/src/renderer/routes/__snapshots__/LoginWithOAuthApp.test.tsx.snap @@ -85,428 +85,6 @@ exports[`renderer/routes/LoginWithOAuthApp.tsx renders correctly 1`] = ` -
-
-
-
- - - - - - - Change only if you are using GitHub Enterprise Server - - -
-
- - - and use your - - client id/secret - - below. - -
-
- - - - -
-
- - - - -
-
-
-
-
- - - - -
-
-
- - - , - "container":
-
-
-
- - -
- -

- Login with OAuth App -

-
- -
-
-
@@ -557,8 +135,10 @@ exports[`renderer/routes/LoginWithOAuthApp.tsx renders correctly 1`] = ` aria-required="true" class="UnstyledTextInput-sc-14ypya-0 dnEFlY" data-component="input" + data-testid="login-hostname" id=":r1:" name="hostname" + placeholder="github.com" type="text" value="github.com" /> @@ -679,8 +259,10 @@ exports[`renderer/routes/LoginWithOAuthApp.tsx renders correctly 1`] = ` aria-required="true" class="UnstyledTextInput-sc-14ypya-0 dnEFlY" data-component="input" + data-testid="login-clientId" id=":r3:" name="clientId" + placeholder="The 20 character Client ID as generated on GitHub" type="text" value="" /> @@ -724,8 +306,10 @@ exports[`renderer/routes/LoginWithOAuthApp.tsx renders correctly 1`] = ` aria-required="true" class="UnstyledTextInput-sc-14ypya-0 dnEFlY" data-component="input" + data-testid="login-clientSecret" id=":r4:" name="clientSecret" + placeholder="The 40 character Client Secret as generated on GitHub" type="text" value="" /> @@ -800,7 +384,7 @@ exports[`renderer/routes/LoginWithOAuthApp.tsx renders correctly 1`] = ` data-loading="false" data-testid="login-submit" sx="[object Object]" - type="submit" + type="button" >
- +
+ + , + "container":
+
+
+
+ + +
+ +

+ Login with OAuth App +

+
+ +
+
+
+
+
+ + + + + + + Change only if you are using GitHub Enterprise Server + + +
+
+ + + and use your + + client id/secret + + below. + +
+
+ + + + +
+
+ + + + +
+
+
+
+
+ + + + +
+
, "debug": [Function], diff --git a/src/renderer/routes/__snapshots__/LoginWithPersonalAccessToken.test.tsx.snap b/src/renderer/routes/__snapshots__/LoginWithPersonalAccessToken.test.tsx.snap index 302b59970..2c303f2cc 100644 --- a/src/renderer/routes/__snapshots__/LoginWithPersonalAccessToken.test.tsx.snap +++ b/src/renderer/routes/__snapshots__/LoginWithPersonalAccessToken.test.tsx.snap @@ -85,282 +85,102 @@ exports[`renderer/routes/LoginWithPersonalAccessToken.tsx renders correctly 1`] -
-
- - - - - - Change only if you are using GitHub Enterprise Server - + Hostname +
+ + * + -
-
+ + + + -
- - - on GitHub to paste the token below. - -
- The - - - - required scopes - - - - will be automatically selected for you. + Change only if you are using GitHub Enterprise Server -
-
- - - - - - - - -
+
- -
- + + on GitHub to paste the token below. + +
+ + The + + + + required scopes + + + + will be automatically selected for you. + +
+
+ + + + + + - + +
+ + +
+
+ -
+ +
-
+ , @@ -518,292 +517,112 @@ exports[`renderer/routes/LoginWithPersonalAccessToken.tsx renders correctly 1`] d="M10.5 0a5.499 5.499 0 1 1-1.288 10.848l-.932.932a.749.749 0 0 1-.53.22H7v.75a.749.749 0 0 1-.22.53l-.5.5a.749.749 0 0 1-.53.22H5v.75a.749.749 0 0 1-.22.53l-.5.5a.749.749 0 0 1-.53.22h-2A1.75 1.75 0 0 1 0 14.25v-2c0-.199.079-.389.22-.53l4.932-4.932A5.5 5.5 0 0 1 10.5 0Zm-4 5.5c-.001.431.069.86.205 1.269a.75.75 0 0 1-.181.768L1.5 12.56v1.69c0 .138.112.25.25.25h1.69l.06-.06v-1.19a.75.75 0 0 1 .75-.75h1.19l.06-.06v-1.19a.75.75 0 0 1 .75-.75h1.19l1.023-1.025a.75.75 0 0 1 .768-.18A4 4 0 1 0 6.5 5.5ZM11 6a1 1 0 1 1 0-2 1 1 0 0 1 0 2Z" /> -

- Login with Personal Access Token -

- - - - -
-
-
-
- - - - - - - Change only if you are using GitHub Enterprise Server - - -
-
-
- - - on GitHub to paste the token below. - -
- - The - - - - required scopes - - - - will be automatically selected for you. - +

+ Login with Personal Access Token +

-
+
+
+
+
+
+
+ + * -
+ + + + + + + Change only if you are using GitHub Enterprise Server + +
-
-
- + + on GitHub to paste the token below. + +
+ + The + + + + required scopes + + + + will be automatically selected for you. + +
+
+ + + + + + + +
+ + +
+
+ -
+ +
-
+ , "debug": [Function], From 2e1a889001e829e0b5af38694bedf9c3b86bd85d Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Fri, 24 Jan 2025 14:35:11 -0500 Subject: [PATCH 3/5] refactor: migrate to text inputs to primer Signed-off-by: Adam Setch --- src/renderer/routes/LoginWithOAuthApp.tsx | 24 +++- .../routes/LoginWithPersonalAccessToken.tsx | 12 +- .../LoginWithOAuthApp.test.tsx.snap | 120 +++++++++++++++--- ...LoginWithPersonalAccessToken.test.tsx.snap | 4 +- 4 files changed, 132 insertions(+), 28 deletions(-) diff --git a/src/renderer/routes/LoginWithOAuthApp.tsx b/src/renderer/routes/LoginWithOAuthApp.tsx index bc6c966d0..aa1b897e2 100644 --- a/src/renderer/routes/LoginWithOAuthApp.tsx +++ b/src/renderer/routes/LoginWithOAuthApp.tsx @@ -1,7 +1,13 @@ import { type FC, useCallback, useContext, useMemo, useState } from 'react'; import { useNavigate } from 'react-router-dom'; -import { BookIcon, PersonIcon, SignInIcon } from '@primer/octicons-react'; +import { + BookIcon, + EyeClosedIcon, + EyeIcon, + PersonIcon, + SignInIcon, +} from '@primer/octicons-react'; import { Button, FormControl, @@ -73,6 +79,8 @@ export const LoginWithOAuthAppRoute: FC = () => { const { loginWithOAuthApp } = useContext(AppContext); + const [maskToken, setMaskToken] = useState(true); + const [formData, setFormData] = useState({ hostname: 'github.com' as Hostname, clientId: '' as ClientID, @@ -178,14 +186,14 @@ export const LoginWithOAuthAppRoute: FC = () => { Create new OAuth App - and use your client id/secret below. + and use your client id & secret below. Client ID { Client Secret { ? 'danger.emphasis' : 'border.default', }} + trailingAction={ + setMaskToken(!maskToken)} + icon={maskToken ? EyeIcon : EyeClosedIcon} + aria-label={maskToken ? 'Show token' : 'Hide token'} + /> + } data-testid="login-clientSecret" block /> diff --git a/src/renderer/routes/LoginWithPersonalAccessToken.tsx b/src/renderer/routes/LoginWithPersonalAccessToken.tsx index e384083f8..053228acd 100644 --- a/src/renderer/routes/LoginWithPersonalAccessToken.tsx +++ b/src/renderer/routes/LoginWithPersonalAccessToken.tsx @@ -69,7 +69,7 @@ export const LoginWithPersonalAccessTokenRoute: FC = () => { const { loginWithPersonalAccessToken } = useContext(AppContext); - const [showPassword, setShowPassword] = useState(false); + const [maskClientSecret, setMaskClientSecret] = useState(true); const [formData, setFormData] = useState({ hostname: 'github.com' as Hostname, @@ -199,8 +199,8 @@ export const LoginWithPersonalAccessTokenRoute: FC = () => { Token { }} trailingAction={ setShowPassword(!showPassword)} - icon={showPassword ? EyeClosedIcon : EyeIcon} - aria-label={showPassword ? 'Hide token' : 'Show token'} + onClick={() => setMaskClientSecret(!maskClientSecret)} + icon={maskClientSecret ? EyeIcon : EyeClosedIcon} + aria-label={maskClientSecret ? 'Show token' : 'Hide token'} /> } data-testid="login-token" diff --git a/src/renderer/routes/__snapshots__/LoginWithOAuthApp.test.tsx.snap b/src/renderer/routes/__snapshots__/LoginWithOAuthApp.test.tsx.snap index 465035284..7551718b3 100644 --- a/src/renderer/routes/__snapshots__/LoginWithOAuthApp.test.tsx.snap +++ b/src/renderer/routes/__snapshots__/LoginWithOAuthApp.test.tsx.snap @@ -216,7 +216,7 @@ exports[`renderer/routes/LoginWithOAuthApp.tsx renders correctly 1`] = ` - client id/secret + client id & secret below. @@ -262,7 +262,7 @@ exports[`renderer/routes/LoginWithOAuthApp.tsx renders correctly 1`] = ` data-testid="login-clientId" id=":r3:" name="clientId" - placeholder="The 20 character Client ID as generated on GitHub" + placeholder="Your generated client id (20 characters)" type="text" value="" /> @@ -297,7 +297,7 @@ exports[`renderer/routes/LoginWithOAuthApp.tsx renders correctly 1`] = ` + + + + @@ -331,11 +375,11 @@ exports[`renderer/routes/LoginWithOAuthApp.tsx renders correctly 1`] = ` + + @@ -752,11 +840,11 @@ exports[`renderer/routes/LoginWithOAuthApp.tsx renders correctly 1`] = `