diff --git a/packages/shared/src/components/modals/DirtyFormModal.spec.tsx b/packages/shared/src/components/modals/DirtyFormModal.spec.tsx new file mode 100644 index 0000000000..d8553b511e --- /dev/null +++ b/packages/shared/src/components/modals/DirtyFormModal.spec.tsx @@ -0,0 +1,100 @@ +import React from 'react'; +import { act, render, screen, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import DirtyFormModal from './DirtyFormModal'; + +const mockCloseModal = jest.fn(); + +jest.mock('../../hooks/useLazyModal', () => ({ + useLazyModal: () => ({ closeModal: mockCloseModal }), +})); + +const renderModal = (onSave: () => void | Promise) => + render( + , + ); + +describe('DirtyFormModal', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('closes immediately for a synchronous save', async () => { + renderModal(jest.fn()); + + await userEvent.click(screen.getByRole('button', { name: 'Save changes' })); + + expect(mockCloseModal).toHaveBeenCalledTimes(1); + }); + + it('stays open until an async save settles', async () => { + let resolveSave: () => void; + const onSave = jest.fn( + () => + new Promise((resolve) => { + resolveSave = resolve; + }), + ); + + renderModal(onSave); + + await userEvent.click(screen.getByRole('button', { name: 'Save changes' })); + + expect(onSave).toHaveBeenCalled(); + expect(mockCloseModal).not.toHaveBeenCalled(); + expect(screen.getByRole('button', { name: 'Discard' })).toBeDisabled(); + + await act(async () => { + resolveSave(); + }); + + await waitFor(() => expect(mockCloseModal).toHaveBeenCalledTimes(1)); + }); + + it('cannot be dismissed while an async save is in flight', async () => { + let resolveSave: () => void; + const onRequestClose = jest.fn(); + const onSave = jest.fn( + () => + new Promise((resolve) => { + resolveSave = resolve; + }), + ); + + render( + , + ); + + await userEvent.click(screen.getByRole('button', { name: 'Save changes' })); + + await userEvent.keyboard('{Escape}'); + expect(onRequestClose).not.toHaveBeenCalled(); + expect(mockCloseModal).not.toHaveBeenCalled(); + + await act(async () => { + resolveSave(); + }); + + await waitFor(() => expect(mockCloseModal).toHaveBeenCalledTimes(1)); + }); + + it('closes after a rejected save so the form and its error stay visible', async () => { + const onSave = jest.fn(() => Promise.reject(new Error('nope'))); + + renderModal(onSave); + + await userEvent.click(screen.getByRole('button', { name: 'Save changes' })); + + await waitFor(() => expect(mockCloseModal).toHaveBeenCalledTimes(1)); + }); +}); diff --git a/packages/shared/src/components/modals/DirtyFormModal.tsx b/packages/shared/src/components/modals/DirtyFormModal.tsx index 88e9111e30..f84f55dd65 100644 --- a/packages/shared/src/components/modals/DirtyFormModal.tsx +++ b/packages/shared/src/components/modals/DirtyFormModal.tsx @@ -1,5 +1,5 @@ import type { ReactElement } from 'react'; -import React from 'react'; +import React, { useState } from 'react'; import type { LazyModalCommonProps } from './common/Modal'; import { Modal } from './common/Modal'; import { Button, ButtonSize, ButtonVariant } from '../buttons/Button'; @@ -12,7 +12,7 @@ import { useLazyModal } from '../../hooks/useLazyModal'; interface DirtyFormModalProps extends LazyModalCommonProps { onDiscard: () => void; - onSave: () => void; + onSave: () => void | Promise; } export default function DirtyFormModal({ @@ -22,12 +22,28 @@ export default function DirtyFormModal({ onSave, }: DirtyFormModalProps): ReactElement { const { closeModal } = useLazyModal(); + const [isSaving, setIsSaving] = useState(false); - const handleSave = () => { - if (onSave) { - onSave(); + const handleSave = async () => { + const result = onSave?.(); + + // Callers that save synchronously keep the original fire-and-forget close. + if (!(result instanceof Promise)) { + closeModal(); + return; + } + + setIsSaving(true); + + try { + await result; + } catch { + // The caller owns surfacing the failure; the modal closes either way so + // the user lands back on their still-unsaved form. + } finally { + setIsSaving(false); + closeModal(); } - closeModal(); }; const handleDiscard = () => { @@ -38,10 +54,12 @@ export default function DirtyFormModal({ return ( @@ -67,6 +85,7 @@ export default function DirtyFormModal({ variant={ButtonVariant.Secondary} size={ButtonSize.Medium} onClick={handleDiscard} + disabled={isSaving} > Discard @@ -75,6 +94,8 @@ export default function DirtyFormModal({ variant={ButtonVariant.Primary} size={ButtonSize.Medium} onClick={handleSave} + disabled={isSaving} + loading={isSaving} > Save changes diff --git a/packages/shared/src/features/profile/common.ts b/packages/shared/src/features/profile/common.ts index c44521080d..6e10a17028 100644 --- a/packages/shared/src/features/profile/common.ts +++ b/packages/shared/src/features/profile/common.ts @@ -11,3 +11,11 @@ export const profileSecondaryFieldStyles = { outerLabel: '!px-0 !typo-callout', baseField: '!h-12', }; + +/** + * Mirrors the work experience limits enforced by the API + * (`src/common/schema/profile.ts`); the form blocks these client side so a + * save is never rejected for something the input could have prevented. + */ +export const maxProfileSkills = 50; +export const maxProfileSkillLength = 100; diff --git a/packages/shared/src/features/profile/components/ProfileSkills.spec.tsx b/packages/shared/src/features/profile/components/ProfileSkills.spec.tsx index a1fbc8d0fe..21cdb44af1 100644 --- a/packages/shared/src/features/profile/components/ProfileSkills.spec.tsx +++ b/packages/shared/src/features/profile/components/ProfileSkills.spec.tsx @@ -1,20 +1,34 @@ -import React, { type ReactNode } from 'react'; +import React, { useEffect, type ReactNode } from 'react'; import { act, fireEvent, render, screen } from '@testing-library/react'; import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import type { UseFormReturn } from 'react-hook-form'; import { FormProvider, useForm } from 'react-hook-form'; import ProfileSkills from './ProfileSkills'; +import { maxProfileSkillLength, maxProfileSkills } from '../common'; + +const mockDisplayToast = jest.fn(); + +jest.mock('../../../hooks/useToastNotification', () => ({ + useToastNotification: () => ({ displayToast: mockDisplayToast }), +})); type FormWrapperProps = { children: ReactNode; + skills?: string[]; + onReady?: (methods: UseFormReturn<{ skills: string[] }>) => void; }; -const FormWrapper = ({ children }: FormWrapperProps) => { - const methods = useForm({ +const FormWrapper = ({ children, skills = [], onReady }: FormWrapperProps) => { + const methods = useForm<{ skills: string[] }>({ defaultValues: { - skills: [], + skills, }, }); + useEffect(() => { + onReady?.(methods); + }, [methods, onReady]); + return ( {children} @@ -22,9 +36,9 @@ const FormWrapper = ({ children }: FormWrapperProps) => { ); }; -const renderComponent = () => +const renderComponent = (props: Omit = {}) => render( - + , ); @@ -35,8 +49,15 @@ const advanceDebounce = () => { }); }; +const submitSkills = (input: HTMLElement, value: string) => { + fireEvent.change(input, { target: { value } }); + advanceDebounce(); + fireEvent.keyDown(input, { code: 'Enter', key: 'Enter' }); +}; + describe('ProfileSkills', () => { beforeEach(() => { + jest.clearAllMocks(); jest.useFakeTimers(); }); @@ -80,4 +101,125 @@ describe('ProfileSkills', () => { expect(input).toHaveValue(''); }); + it('does not add a skill that only differs by casing', () => { + renderComponent({ skills: ['React'] }); + + const input = screen.getByPlaceholderText('Search skills'); + submitSkills(input, 'react'); + + expect(screen.getAllByRole('button', { name: /react/i })).toHaveLength(1); + }); + + it('blocks adding past the limit and shows the limit copy', () => { + const skills = Array.from( + { length: maxProfileSkills }, + (_, index) => `skill-${index}`, + ); + renderComponent({ skills }); + + const input = screen.getByPlaceholderText('Search skills'); + submitSkills(input, 'one too many'); + + expect( + screen.queryByRole('button', { name: 'one too many' }), + ).not.toBeInTheDocument(); + expect( + screen.getByText(`You can add up to ${maxProfileSkills} skills.`), + ).toBeInTheDocument(); + expect(mockDisplayToast).toHaveBeenCalledWith( + `You can add up to ${maxProfileSkills} skills. 1 skill was not added.`, + ); + }); + + it('caps a pasted batch that exceeds the limit and reports the remainder', () => { + const skills = Array.from( + { length: maxProfileSkills - 1 }, + (_, index) => `skill-${index}`, + ); + renderComponent({ skills }); + + const input = screen.getByPlaceholderText('Search skills'); + submitSkills(input, 'first,second,third'); + + expect(screen.getByRole('button', { name: 'first' })).toBeInTheDocument(); + expect( + screen.queryByRole('button', { name: 'second' }), + ).not.toBeInTheDocument(); + expect(mockDisplayToast).toHaveBeenCalledWith( + `You can add up to ${maxProfileSkills} skills. 2 skills were not added.`, + ); + }); + + it('does not add a skill longer than the allowed length', () => { + renderComponent(); + + const input = screen.getByPlaceholderText('Search skills'); + const tooLong = 'a'.repeat(maxProfileSkillLength + 1); + submitSkills(input, tooLong); + + expect( + screen.queryByRole('button', { name: tooLong }), + ).not.toBeInTheDocument(); + expect(mockDisplayToast).toHaveBeenCalledWith( + `Skills can be up to ${maxProfileSkillLength} characters. 1 skill was not added.`, + ); + }); + + it('reports every reason a pasted batch was trimmed, not just the first', () => { + const skills = Array.from( + { length: maxProfileSkills - 1 }, + (_, index) => `skill-${index}`, + ); + renderComponent({ skills }); + + const input = screen.getByPlaceholderText('Search skills'); + const tooLong = 'a'.repeat(maxProfileSkillLength + 1); + submitSkills(input, `first,second,${tooLong}`); + + expect(screen.getByRole('button', { name: 'first' })).toBeInTheDocument(); + expect(mockDisplayToast).toHaveBeenCalledWith( + `You can add up to ${maxProfileSkills} skills. Skills can be up to ${maxProfileSkillLength} characters. 2 skills were not added.`, + ); + }); + + it('renders an array level server error', () => { + let methods: UseFormReturn<{ skills: string[] }>; + renderComponent({ + onReady: (form) => { + methods = form; + }, + }); + + act(() => { + methods.setError('skills', { + type: 'too_big', + message: 'You can add up to 50 skills.', + }); + }); + + expect( + screen.getByText('You can add up to 50 skills.'), + ).toBeInTheDocument(); + }); + + it('renders an item level server error stored as a sparse array', () => { + let methods: UseFormReturn<{ skills: string[] }>; + renderComponent({ + skills: ['a', 'b', 'c', 'd'], + onReady: (form) => { + methods = form; + }, + }); + + act(() => { + methods.setError('skills.3', { + type: 'too_big', + message: 'Skills can be up to 100 characters.', + }); + }); + + expect( + screen.getByText('Skills can be up to 100 characters.'), + ).toBeInTheDocument(); + }); }); diff --git a/packages/shared/src/features/profile/components/ProfileSkills.tsx b/packages/shared/src/features/profile/components/ProfileSkills.tsx index 70412150b8..d0ce056f5f 100644 --- a/packages/shared/src/features/profile/components/ProfileSkills.tsx +++ b/packages/shared/src/features/profile/components/ProfileSkills.tsx @@ -2,7 +2,8 @@ import React, { useRef, useState } from 'react'; import type { ReactElement } from 'react'; import type { PopoverContentProps } from '@radix-ui/react-popover'; import { Popover, PopoverAnchor } from '@radix-ui/react-popover'; -import { Controller, useFormContext } from 'react-hook-form'; +import type { FieldError } from 'react-hook-form'; +import { Controller, useFormContext, useFormState } from 'react-hook-form'; import { useQuery } from '@tanstack/react-query'; import { TextField } from '../../../components/fields/TextField'; import { FeedbackIcon, SearchIcon } from '../../../components/icons'; @@ -10,19 +11,54 @@ import { IconSize } from '../../../components/Icon'; import { TagElement } from '../../../components/feeds/FeedSettings/TagElement'; import { PopoverContent } from '../../../components/popover/Popover'; import useDebounceFn from '../../../hooks/useDebounceFn'; +import { useToastNotification } from '../../../hooks/useToastNotification'; import { GenericLoaderSpinner } from '../../../components/utilities/loaders'; import { Typography, TypographyType, } from '../../../components/typography/Typography'; import { getKeywordAutocompleteOptions } from '../../opportunity/queries'; +import { maxProfileSkillLength, maxProfileSkills } from '../common'; type ProfileSkillsProps = { name: string; }; +const skillsHint = + 'Add commas (,) to add multiple skills. Press Enter to submit them.'; +const limitHint = `You can add up to ${maxProfileSkills} skills.`; + +// The API stores skills under slugify(value), so "React" and "react" are the +// same skill to it but two entries here. +const skillKey = (skill: string) => skill.trim().toLowerCase(); + +/** + * A rejected skill arrives either as an array-level issue (path `skills`) or as + * an item-level one (path `skills.3`), which react-hook-form stores as a sparse + * array with no message on the root. Reading `error.message` alone would render + * nothing for the second shape. + */ +const getSkillsError = ( + error: FieldError | FieldError[] | undefined, +): string | undefined => { + if (!error) { + return undefined; + } + + if (Array.isArray(error)) { + return error.find((item) => item?.message)?.message ?? limitHint; + } + + return error.message ?? limitHint; +}; + const ProfileSkills = ({ name }: ProfileSkillsProps): ReactElement => { const { control } = useFormContext(); + // useController subscribes to its own name exactly, so a server issue on + // `skills.3` never reaches the Controller. useFormState subscribes to the + // whole subtree, which covers both the array and the item level paths. + const { errors } = useFormState({ control, name }); + const { displayToast } = useToastNotification(); const [query, setQuery] = useState(''); const [open, setOpen] = useState(false); const inputRef = useRef(null); @@ -57,16 +93,75 @@ const ProfileSkills = ({ name }: ProfileSkillsProps): ReactElement => { name={name} render={({ field }) => { const skills = Array.isArray(field.value) ? field.value : []; + const isAtLimit = skills.length >= maxProfileSkills; + const error = getSkillsError( + errors[name] as FieldError | FieldError[] | undefined, + ); + + const addSkills = (candidates: string[]) => { + const seen = new Set(skills.map(skillKey)); + const room = maxProfileSkills - skills.length; + const accepted: string[] = []; + let overLimit = 0; + let tooLong = 0; + + candidates + .map((candidate) => candidate.trim()) + .filter(Boolean) + .forEach((skill) => { + if (seen.has(skillKey(skill))) { + return; + } + + if (skill.length > maxProfileSkillLength) { + tooLong += 1; + return; + } + + if (accepted.length >= room) { + overLimit += 1; + return; + } - const addSkill = (skill: string) => { - if (skills.includes(skill)) { - return; + seen.add(skillKey(skill)); + accepted.push(skill); + }); + + // Dropping part of a paste silently is the bug being fixed, so always + // say what was left out, and why, for every reason it happened. + const rejected = overLimit + tooLong; + + if (rejected) { + const reasons = [ + overLimit && limitHint, + tooLong && + `Skills can be up to ${maxProfileSkillLength} characters.`, + ].filter(Boolean); + + displayToast( + `${reasons.join(' ')} ${rejected} ${ + rejected === 1 ? 'skill was' : 'skills were' + } not added.`, + ); + } + + if (accepted.length) { + field.onChange([...skills, ...accepted]); } - field.onChange([...skills, skill]); }; const removeSkill = (skill: string) => { - field.onChange(skills.filter((s: string) => s !== skill)); + field.onChange( + skills.filter((s: string) => skillKey(s) !== skillKey(skill)), + ); + }; + + const getHint = () => { + if (error) { + return error; + } + + return isAtLimit ? limitHint : skillsHint; }; return ( @@ -89,8 +184,9 @@ const ProfileSkills = ({ name }: ProfileSkillsProps): ReactElement => { ) : undefined } - hint="Add commas (,) to add multiple skills. Press Enter to submit them." + hint={getHint()} hintIcon={} + valid={!error} value={query} onChange={({ target }) => { if (target.value === '') { @@ -108,20 +204,7 @@ const ProfileSkills = ({ name }: ProfileSkillsProps): ReactElement => { if (e.key === 'Enter') { e.preventDefault(); - const newSkills = query - .split(',') - .map((k) => k.trim()) - .filter(Boolean) - .filter((k) => !skills.includes(k)); - - if (newSkills.length === 0) { - if (query) { - clearQuery(); - } - return; - } - - field.onChange([...skills, ...newSkills]); + addSkills(query.split(',')); clearQuery(); return; } @@ -146,7 +229,9 @@ const ProfileSkills = ({ name }: ProfileSkillsProps): ReactElement => { >
{autocompleteKeywords?.map(({ keyword }) => { - const isSelected = skills.includes(keyword); + const isSelected = skills.some( + (skill: string) => skillKey(skill) === skillKey(keyword), + ); return ( { if (isSelected) { removeSkill(keyword); } else { - addSkill(keyword); + addSkills([keyword]); } }} /> diff --git a/packages/shared/src/hooks/useDirtyForm.ts b/packages/shared/src/hooks/useDirtyForm.ts index a7a3822632..3e87704ac8 100644 --- a/packages/shared/src/hooks/useDirtyForm.ts +++ b/packages/shared/src/hooks/useDirtyForm.ts @@ -4,7 +4,9 @@ import { useLazyModal } from './useLazyModal'; import { LazyModal } from '../components/modals/common/types'; export interface UseDirtyFormOptions { - onSave: () => void; + // A promise keeps DirtyFormModal open until the save settles, so a rejected + // save cannot look like a successful one. + onSave: () => void | Promise; onDiscard?: () => void; } diff --git a/packages/shared/src/hooks/useUserExperienceForm.spec.tsx b/packages/shared/src/hooks/useUserExperienceForm.spec.tsx index 769e53e986..d5544a48d4 100644 --- a/packages/shared/src/hooks/useUserExperienceForm.spec.tsx +++ b/packages/shared/src/hooks/useUserExperienceForm.spec.tsx @@ -8,14 +8,17 @@ import { upsertUserWorkExperience, UserExperienceType, } from '../graphql/user/profile'; +import { labels } from '../lib/labels'; // Mock dependencies jest.mock('next/router', () => ({ useRouter: jest.fn(), })); +const mockDisplayToast = jest.fn(); + jest.mock('./useToastNotification', () => ({ - useToastNotification: () => ({ displayToast: jest.fn() }), + useToastNotification: () => ({ displayToast: mockDisplayToast }), })); // Mock the GraphQL mutations @@ -147,15 +150,8 @@ describe('useUserExperienceForm', () => { { wrapper: createWrapper() }, ); - act(() => { - result.current.methods.reset({ - ...existingExperience, - type: undefined as never, - }); - }); - await act(async () => { - result.current.save?.(); + await result.current.save?.(); }); await waitFor(() => { @@ -554,7 +550,12 @@ describe('useUserExperienceForm', () => { }; const { result } = renderHook( - () => useUserExperienceForm({ defaultValues: openSourceExperience }), + () => + useUserExperienceForm({ + // The missing repository URL is the point of the test, so this + // fixture cannot satisfy the form values type. + defaultValues: openSourceExperience as unknown as BaseUserExperience, + }), { wrapper: createWrapper() }, ); @@ -564,4 +565,146 @@ describe('useUserExperienceForm', () => { expect(isValid).toBe(false); }); }); + describe('server validation errors', () => { + const zodError = ( + issues: { path: (string | number)[]; message: string }[], + ) => ({ + response: { + errors: [ + { + message: 'Validation error', + extensions: { + code: 'ZOD_VALIDATION_ERROR', + issues: issues.map((issue) => ({ ...issue, code: 'too_big' })), + }, + }, + ], + }, + }); + + it('should surface a rejected skills array on the form and as a toast', async () => { + (upsertUserWorkExperience as jest.Mock).mockRejectedValue( + zodError([ + { path: ['skills'], message: 'You can add up to 50 skills.' }, + ]), + ); + + const { result } = setupWorkExperienceForm(); + + await act(async () => { + await result.current.save?.(); + }); + + await waitFor(() => { + expect(mockDisplayToast).toHaveBeenCalledWith( + 'You can add up to 50 skills.', + ); + }); + expect( + result.current.methods.getFieldState('skills').error, + ).toBeDefined(); + expect(mockRouter.push).not.toHaveBeenCalled(); + expect(result.current.methods.getValues('title')).toBe( + 'Software Engineer', + ); + }); + + it('should surface an item level skills issue as a toast', async () => { + (upsertUserWorkExperience as jest.Mock).mockRejectedValue( + zodError([ + { + path: ['skills', 3], + message: 'Skills can be up to 100 characters.', + }, + ]), + ); + + const { result } = setupWorkExperienceForm(); + + await act(async () => { + await result.current.save?.(); + }); + + await waitFor(() => { + expect(mockDisplayToast).toHaveBeenCalledWith( + 'Skills can be up to 100 characters.', + ); + }); + expect( + result.current.methods.getFieldState('skills.3' as never).error, + ).toBeDefined(); + expect(mockRouter.push).not.toHaveBeenCalled(); + }); + + it('should keep the generic toast for non zod errors', async () => { + (upsertUserWorkExperience as jest.Mock).mockRejectedValue({ + response: { + errors: [{ message: 'Something exploded', extensions: {} }], + }, + }); + + const { result } = setupWorkExperienceForm(); + + await act(async () => { + await result.current.save?.(); + }); + + await waitFor(() => { + expect(mockDisplayToast).toHaveBeenCalledWith('Something exploded'); + }); + expect(mockRouter.push).not.toHaveBeenCalled(); + }); + }); + + describe('dirty form save', () => { + it('should not run the mutation when the form is invalid', async () => { + const { result } = setupWorkExperienceForm(); + + act(() => { + result.current.methods.setValue('title', ''); + }); + + await act(async () => { + await result.current.save?.(); + }); + + expect(upsertUserWorkExperience).not.toHaveBeenCalled(); + expect(mockDisplayToast).toHaveBeenCalledWith(labels.error.formInvalid); + expect(mockRouter.push).not.toHaveBeenCalled(); + }); + + it('should resolve only once the save settles and keep the values on failure', async () => { + let rejectMutation: (error: unknown) => void; + (upsertUserWorkExperience as jest.Mock).mockReturnValue( + new Promise((_, reject) => { + rejectMutation = reject; + }), + ); + + const { result } = setupWorkExperienceForm(); + + let settled = false; + let savePromise: Promise; + await act(async () => { + savePromise = Promise.resolve(result.current.save?.()).then(() => { + settled = true; + }); + }); + + expect(settled).toBe(false); + + await act(async () => { + rejectMutation({ + response: { errors: [{ message: 'Nope', extensions: {} }] }, + }); + await savePromise; + }); + + expect(settled).toBe(true); + expect(result.current.methods.getValues('title')).toBe( + 'Software Engineer', + ); + expect(mockRouter.push).not.toHaveBeenCalled(); + }); + }); }); diff --git a/packages/shared/src/hooks/useUserExperienceForm.ts b/packages/shared/src/hooks/useUserExperienceForm.ts index 65e31e8890..625935d149 100644 --- a/packages/shared/src/hooks/useUserExperienceForm.ts +++ b/packages/shared/src/hooks/useUserExperienceForm.ts @@ -16,12 +16,20 @@ import { import { useDirtyForm } from './useDirtyForm'; import { generateQueryKey, RequestKey } from '../lib/query'; import { ApiError } from '../graphql/common'; -import type { ApiErrorResult } from '../graphql/common'; +import type { + ApiErrorResult, + ApiResponseError, + ApiZodErrorExtension, +} from '../graphql/common'; import { labels } from '../lib/labels'; import { applyZodErrorsToForm } from '../lib/form'; import { useToastNotification } from './useToastNotification'; import { webappUrl } from '../lib/constants'; import { useUserExperiencesByType } from '../features/profile/hooks/useUserExperiencesByType'; +import { + maxProfileSkillLength, + maxProfileSkills, +} from '../features/profile/common'; import { useAuthContext } from '../contexts/AuthContext'; import { useLogContext } from '../contexts/LogContext'; import { LogEvent } from '../lib/log'; @@ -73,6 +81,21 @@ export const userExperienceInputBaseSchema = z .default(null), repository: repositorySchema, repositorySearch: z.string().optional(), + skills: z + .array( + z + .string() + .trim() + .normalize() + .nonempty() + .max( + maxProfileSkillLength, + `Skills can be up to ${maxProfileSkillLength} characters.`, + ), + ) + .max(maxProfileSkills, `You can add up to ${maxProfileSkills} skills.`) + .optional() + .default([]), }) .refine( (data) => { @@ -100,8 +123,29 @@ export const userExperienceInputBaseSchema = z }, ); -type BaseUserExperience = Omit< +/** + * What the form actually holds, which `UserExperience` does not describe: the + * date fields arrive from the page as serialized strings and become Dates once + * the month/year selects write to them, and the rest are form-only fields or + * fields specific to one experience type. + */ +export type UserExperienceFormValues = Omit< UserExperience, + 'startedAt' | 'endedAt' +> & { + startedAt?: string | Date | null; + endedAt?: string | Date | null; + current?: boolean; + skills?: string[]; + repositorySearch?: string; + employmentType?: number | null; + locationType?: number | null; + externalLocationId?: string | null; + grade?: string | null; +}; + +type BaseUserExperience = Omit< + UserExperienceFormValues, 'id' | 'createdAt' | 'company' | 'customCompanyName' > & { id?: string; @@ -123,7 +167,7 @@ const useUserExperienceForm = ({ const dirtyFormRef = useRef | null>(null); const router = useRouter(); const { displayToast } = useToastNotification(); - const methods = useForm({ + const methods = useForm({ defaultValues, reValidateMode: 'onSubmit', resolver: zodResolver(userExperienceInputBaseSchema), @@ -139,12 +183,15 @@ const useUserExperienceForm = ({ { condition: isNewExperience }, ); - const { mutate, isPending } = useMutation({ - mutationFn: (data: UserExperience | UserExperienceWork) => { - const input = { ...data, type } as UserExperience | UserExperienceWork; + const { mutateAsync, isPending } = useMutation({ + mutationFn: (data: UserExperienceFormValues) => { + // The mutations are typed in the GraphQL shape, which the form values + // deliberately differ from: the API parses skills as strings and the + // dates as Dates, and returns them as UserSkill[] and strings. + const input = { ...data, type } as unknown as UserExperienceWork; return type === UserExperienceType.Work - ? upsertUserWorkExperience(input as UserExperienceWork, id) + ? upsertUserWorkExperience(input, id) : upsertUserGeneralExperience(input, id); }, onSuccess: (result, vars) => { @@ -165,23 +212,46 @@ const useUserExperienceForm = ({ router.push(`${webappUrl}settings/profile/experience/${type}`); }, onError: (error: ApiErrorResult) => { - if ( - error.response?.errors?.[0]?.extensions?.code === - ApiError.ZodValidationError - ) { - applyZodErrorsToForm({ - error, - setError: methods.setError, - }); - } else { - displayToast( - error.response?.errors?.[0]?.message || labels.error.generic, - ); + const responseError = error.response?.errors?.[0]; + + if (responseError?.extensions?.code !== ApiError.ZodValidationError) { + displayToast(responseError?.message || labels.error.generic); + return; } + + applyZodErrorsToForm({ + error, + setError: methods.setError, + }); + + // The GraphQL message for a zod error is always a generic "Validation + // error", and not every field renders its own error, so surface the first + // issue as a toast to guarantee the rejection is visible. + const [issue] = (responseError as ApiResponseError) + .extensions.issues; + displayToast(issue?.message || labels.error.generic); }, }); const dirtyForm = useDirtyForm(methods.formState.isDirty, { - onSave: () => mutate({ ...methods.getValues(), type }), + // getValues() rather than the resolver output: the client schema is a + // subset of the form, so parsed values would drop fields like + // employmentType or grade. trigger() gives us validation without that. + onSave: async () => { + const isValid = await methods.trigger(); + + if (!isValid) { + // The modal closes either way, so say why nothing was saved: some + // fields (type, for one) have no input that could show the error. + displayToast(labels.error.formInvalid); + return; + } + + try { + await mutateAsync({ ...methods.getValues(), type }); + } catch { + // handled by the mutation's onError + } + }, onDiscard: () => { methods.reset(); }, diff --git a/packages/shared/src/lib/labels.ts b/packages/shared/src/lib/labels.ts index 93323170f6..74eabdb0ec 100644 --- a/packages/shared/src/lib/labels.ts +++ b/packages/shared/src/lib/labels.ts @@ -5,6 +5,7 @@ export const labels = { error: { generic: '🚫 Something went wrong, please try again.', rateLimit: '⌛️ Rate limit exceeded, please try again later.', + formInvalid: '🚫 Please fix the highlighted fields before saving.', }, squads: { forbidden: '🚫 You no longer have access to this Squad.',