From 7fd367333c02fba7578924df3639d698b7bc396d Mon Sep 17 00:00:00 2001 From: Chris Bongers Date: Fri, 11 Sep 2026 13:44:46 +0200 Subject: [PATCH 1/2] fix(profile): stop work experience skills failing to save in silence Saving an experience with too many skills, or one skill over the length cap, was rejected by the API and the user was told nothing: the client schema had no skills field, so nothing was caught before the request, and ProfileSkills never rendered its error, so the rejection landed nowhere. The save is a single transaction, so the whole section was lost, and the still-dirty form then offered "Save changes" on the way out, which failed just as silently until the user discarded their work. Three layers, none of which alone is enough: - The input enforces the limits it knows about. Skills are deduped on the API's slugify() identity, a paste is capped rather than dropped whole, and whatever did not fit is named in a toast. - Rejections are visible. ProfileSkills renders the error, and a zod error always raises a toast as well, since fields like companyId have nowhere to show one. ProfileSkills reads it through useFormState: useController subscribes to its exact name, so an issue on `skills.3` never reached the Controller at all. - A failed save no longer costs the user the section. The dirty-form path validates first and awaits the mutation, so DirtyFormModal stays open until it settles and the form is never reset on failure. Client validation that blocks the save says so too, otherwise the modal just closes and we are back to the original bug. useDirtyForm's onSave may now return a promise; sync callers keep the previous fire-and-forget close. Co-Authored-By: Claude Opus 5 (1M context) --- .../components/modals/DirtyFormModal.spec.tsx | 68 ++++++++ .../src/components/modals/DirtyFormModal.tsx | 31 +++- .../shared/src/features/profile/common.ts | 8 + .../profile/components/ProfileSkills.spec.tsx | 137 ++++++++++++++- .../profile/components/ProfileSkills.tsx | 129 ++++++++++++--- packages/shared/src/hooks/useDirtyForm.ts | 4 +- .../src/hooks/useUserExperienceForm.spec.tsx | 156 +++++++++++++++++- .../shared/src/hooks/useUserExperienceForm.ts | 76 +++++++-- packages/shared/src/lib/labels.ts | 1 + scripts/typecheck-strict-changed.js | 9 + 10 files changed, 559 insertions(+), 60 deletions(-) create mode 100644 packages/shared/src/components/modals/DirtyFormModal.spec.tsx 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 00000000000..9eb92778a09 --- /dev/null +++ b/packages/shared/src/components/modals/DirtyFormModal.spec.tsx @@ -0,0 +1,68 @@ +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('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 88e9111e306..73ce16b6890 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 = () => { @@ -67,6 +83,7 @@ export default function DirtyFormModal({ variant={ButtonVariant.Secondary} size={ButtonSize.Medium} onClick={handleDiscard} + disabled={isSaving} > Discard @@ -75,6 +92,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 c44521080d0..6e10a170280 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 a1fbc8d0fec..efdf2d3b01b 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,108 @@ 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('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 70412150b8f..9af82e315fa 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,73 @@ 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; + } - const addSkill = (skill: string) => { - if (skills.includes(skill)) { - return; + if (accepted.length >= room) { + overLimit += 1; + 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. + if (overLimit) { + displayToast( + `${limitHint} ${overLimit} ${ + overLimit === 1 ? 'skill was' : 'skills were' + } not added.`, + ); + } else if (tooLong) { + displayToast( + `Skills can be up to ${maxProfileSkillLength} characters. ${tooLong} ${ + tooLong === 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 +182,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 +202,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 +227,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 a7a38226323..3e87704ac8e 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 769e53e9863..a000686ef7e 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(() => { @@ -564,4 +560,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' as never).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 65e31e88908..b0a6092989a 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) => { @@ -139,7 +162,7 @@ const useUserExperienceForm = ({ { condition: isNewExperience }, ); - const { mutate, isPending } = useMutation({ + const { mutateAsync, isPending } = useMutation({ mutationFn: (data: UserExperience | UserExperienceWork) => { const input = { ...data, type } as UserExperience | UserExperienceWork; @@ -165,23 +188,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 93323170f6d..74eabdb0ec4 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.', diff --git a/scripts/typecheck-strict-changed.js b/scripts/typecheck-strict-changed.js index e2383a17665..164a3af1733 100644 --- a/scripts/typecheck-strict-changed.js +++ b/scripts/typecheck-strict-changed.js @@ -213,6 +213,15 @@ const strictSkipList = new Set([ // errors on other lines predate that change. 'packages/shared/src/hooks/useBanner.ts', 'packages/shared/src/hooks/useFeedSettings.ts', + // Experience form spec — touched only to cover the skills save failure + // (ENG-1886). Its strict errors are all one pre-existing cause: the form + // is typed `useForm` but holds values that type does not + // describe (Date rather than string dates, `current`, `skills` as + // string[]), so every fixture and setValue call mismatches. Fixing it + // means introducing a form-values type and settling where the page's + // serialized `startedAt` string becomes a Date — a refactor across the + // hook, the edit page and every experience form, not this bug fix. + 'packages/shared/src/hooks/useUserExperienceForm.spec.tsx', ]); const changedFiles = getChangedTypescriptFiles().filter( From 9ee6409a1f169c0dfa01fe85549a92482aeb614b Mon Sep 17 00:00:00 2001 From: Chris Bongers Date: Fri, 11 Sep 2026 14:31:07 +0200 Subject: [PATCH 2/2] fix(profile): address review notes on the skills save fix - A paste that both overflows the cap and carries over-length entries reported only the first reason and dropped the rest of the count silently, which is the failure this PR exists to remove. One toast now names every reason and the true number left out. - DirtyFormModal could be dismissed by Escape or the overlay mid-save, so the save would later close whichever modal was current by then. Both are inert while a save is in flight. - Types the form values instead of skip-listing the spec. The form was declared as UserExperience while holding values that type does not describe, so every fixture and setValue call in the spec mismatched. UserExperienceFormValues says what the form actually holds, including dates that arrive from the page serialized and become Dates once the month/year selects write to them. The GraphQL shape stays at the mutation boundary, where the cast already lived. Co-Authored-By: Claude Opus 5 (1M context) --- .../components/modals/DirtyFormModal.spec.tsx | 32 +++++++++++++++++ .../src/components/modals/DirtyFormModal.tsx | 6 ++-- .../profile/components/ProfileSkills.spec.tsx | 17 ++++++++++ .../profile/components/ProfileSkills.tsx | 22 ++++++------ .../src/hooks/useUserExperienceForm.spec.tsx | 9 +++-- .../shared/src/hooks/useUserExperienceForm.ts | 34 ++++++++++++++++--- scripts/typecheck-strict-changed.js | 9 ----- 7 files changed, 101 insertions(+), 28 deletions(-) diff --git a/packages/shared/src/components/modals/DirtyFormModal.spec.tsx b/packages/shared/src/components/modals/DirtyFormModal.spec.tsx index 9eb92778a09..d8553b511eb 100644 --- a/packages/shared/src/components/modals/DirtyFormModal.spec.tsx +++ b/packages/shared/src/components/modals/DirtyFormModal.spec.tsx @@ -56,6 +56,38 @@ describe('DirtyFormModal', () => { 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'))); diff --git a/packages/shared/src/components/modals/DirtyFormModal.tsx b/packages/shared/src/components/modals/DirtyFormModal.tsx index 73ce16b6890..f84f55dd651 100644 --- a/packages/shared/src/components/modals/DirtyFormModal.tsx +++ b/packages/shared/src/components/modals/DirtyFormModal.tsx @@ -54,10 +54,12 @@ export default function DirtyFormModal({ return ( diff --git a/packages/shared/src/features/profile/components/ProfileSkills.spec.tsx b/packages/shared/src/features/profile/components/ProfileSkills.spec.tsx index efdf2d3b01b..21cdb44af1d 100644 --- a/packages/shared/src/features/profile/components/ProfileSkills.spec.tsx +++ b/packages/shared/src/features/profile/components/ProfileSkills.spec.tsx @@ -165,6 +165,23 @@ describe('ProfileSkills', () => { ); }); + 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({ diff --git a/packages/shared/src/features/profile/components/ProfileSkills.tsx b/packages/shared/src/features/profile/components/ProfileSkills.tsx index 9af82e315fa..d0ce056f5f6 100644 --- a/packages/shared/src/features/profile/components/ProfileSkills.tsx +++ b/packages/shared/src/features/profile/components/ProfileSkills.tsx @@ -128,17 +128,19 @@ const ProfileSkills = ({ name }: ProfileSkillsProps): ReactElement => { }); // Dropping part of a paste silently is the bug being fixed, so always - // say what was left out. - if (overLimit) { - displayToast( - `${limitHint} ${overLimit} ${ - overLimit === 1 ? 'skill was' : 'skills were' - } not added.`, - ); - } else if (tooLong) { + // 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( - `Skills can be up to ${maxProfileSkillLength} characters. ${tooLong} ${ - tooLong === 1 ? 'skill was' : 'skills were' + `${reasons.join(' ')} ${rejected} ${ + rejected === 1 ? 'skill was' : 'skills were' } not added.`, ); } diff --git a/packages/shared/src/hooks/useUserExperienceForm.spec.tsx b/packages/shared/src/hooks/useUserExperienceForm.spec.tsx index a000686ef7e..d5544a48d48 100644 --- a/packages/shared/src/hooks/useUserExperienceForm.spec.tsx +++ b/packages/shared/src/hooks/useUserExperienceForm.spec.tsx @@ -550,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() }, ); @@ -596,7 +601,7 @@ describe('useUserExperienceForm', () => { ); }); expect( - result.current.methods.getFieldState('skills' as never).error, + result.current.methods.getFieldState('skills').error, ).toBeDefined(); expect(mockRouter.push).not.toHaveBeenCalled(); expect(result.current.methods.getValues('title')).toBe( diff --git a/packages/shared/src/hooks/useUserExperienceForm.ts b/packages/shared/src/hooks/useUserExperienceForm.ts index b0a6092989a..625935d1491 100644 --- a/packages/shared/src/hooks/useUserExperienceForm.ts +++ b/packages/shared/src/hooks/useUserExperienceForm.ts @@ -123,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; @@ -146,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), @@ -163,11 +184,14 @@ const useUserExperienceForm = ({ ); const { mutateAsync, isPending } = useMutation({ - mutationFn: (data: UserExperience | UserExperienceWork) => { - const input = { ...data, type } as UserExperience | UserExperienceWork; + 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) => { diff --git a/scripts/typecheck-strict-changed.js b/scripts/typecheck-strict-changed.js index 164a3af1733..e2383a17665 100644 --- a/scripts/typecheck-strict-changed.js +++ b/scripts/typecheck-strict-changed.js @@ -213,15 +213,6 @@ const strictSkipList = new Set([ // errors on other lines predate that change. 'packages/shared/src/hooks/useBanner.ts', 'packages/shared/src/hooks/useFeedSettings.ts', - // Experience form spec — touched only to cover the skills save failure - // (ENG-1886). Its strict errors are all one pre-existing cause: the form - // is typed `useForm` but holds values that type does not - // describe (Date rather than string dates, `current`, `skills` as - // string[]), so every fixture and setValue call mismatches. Fixing it - // means introducing a form-values type and settling where the page's - // serialized `startedAt` string becomes a Date — a refactor across the - // hook, the edit page and every experience form, not this bug fix. - 'packages/shared/src/hooks/useUserExperienceForm.spec.tsx', ]); const changedFiles = getChangedTypescriptFiles().filter(