fix(profile): stop work experience skills failing to save in silence

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<void>) =>
+  render(
+    <DirtyFormModal
+      isOpen
+      onRequestClose={jest.fn()}
+      onDiscard={jest.fn()}
+      onSave={onSave}
+    />,
+  );
+
+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<void>((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<void>((resolve) => {
+          resolveSave = resolve;
+        }),
+    );
+
+    render(
+      <DirtyFormModal
+        isOpen
+        onRequestClose={onRequestClose}
+        onDiscard={jest.fn()}
+        onSave={onSave}
+      />,
+    );
+
+    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));
+  });
+});

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<void>;
 }
 
 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 (
     <Modal
       isOpen={isOpen}
-      onRequestClose={onRequestClose}
+      // Dismissing mid-save would close whichever modal is current by the time
+      // the save settles, so the overlay and Escape are inert while it runs.
+      onRequestClose={isSaving ? undefined : onRequestClose}
       kind={Modal.Kind.FlexibleCenter}
       size={Modal.Size.Small}
-      shouldCloseOnOverlayClick
+      shouldCloseOnOverlayClick={!isSaving}
       isDrawerOnMobile
       drawerProps={{ displayCloseButton: false }}
     >
@@ -67,6 +85,7 @@ export default function DirtyFormModal({
             variant={ButtonVariant.Secondary}
             size={ButtonSize.Medium}
             onClick={handleDiscard}
+            disabled={isSaving}
           >
             Discard
           </Button>
@@ -75,6 +94,8 @@ export default function DirtyFormModal({
             variant={ButtonVariant.Primary}
             size={ButtonSize.Medium}
             onClick={handleSave}
+            disabled={isSaving}
+            loading={isSaving}
           >
             Save changes
           </Button>

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;

packages/shared/src/features/profile/components/ProfileSkills.spec.tsx

@@ -1,30 +1,44 @@
-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 (
     <QueryClientProvider client={new QueryClient()}>
       <FormProvider {...methods}>{children}</FormProvider>
     </QueryClientProvider>
   );
 };
 
-const renderComponent = () =>
+const renderComponent = (props: Omit<FormWrapperProps, 'children'> = {}) =>
   render(
-    <FormWrapper>
+    <FormWrapper {...props}>
       <ProfileSkills name="skills" />
     </FormWrapper>,
   );
@@ -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();
+  });
 });