Skip to content

Commit d52a966

Browse files
imnasnainaecclaude
andcommitted
Address code review feedback on fix-create-proj
- Restructure createAndPersistProject to eliminate duplicate setCreateSourceIsSelect/setModal calls via a single exit path - Set isCreating during the discard-confirm createAndPersistProject path so the create modal buttons are disabled in that flow too - Update stale JSDoc: PendingReplace comment and component summary now accurately describe the immediate-persist New behavior - Split the old New-flow test into success and failure cases so resetActiveProject is asserted for the right reason in each - Add validation-failure test (parse succeeds, non-project shape) to cover lines 242-245 and restore 100% coverage - Fix no-type-assertion lint error in makeWebViewStateWithActiveProjectSpies - Update discard-confirm test to assert createProject is called after confirm Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 1ba82e0 commit d52a966

2 files changed

Lines changed: 132 additions & 24 deletions

File tree

src/__tests__/components/modals/ProjectModals.test.tsx

Lines changed: 110 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -292,6 +292,25 @@ function makeWebViewStateWithResetSpy(resetActiveProject: () => void) {
292292
];
293293
}
294294

295+
/**
296+
* Builds a `useWebViewState` stub with spies on both the setter and the reset for the
297+
* `'activeProject'` key, so tests can assert which path was taken after a create attempt.
298+
*
299+
* @param setActiveProject - Spy invoked when the `'activeProject'` slot is set.
300+
* @param resetActiveProject - Spy invoked when the `'activeProject'` slot is reset.
301+
* @returns A `useWebViewState`-shaped hook stub.
302+
*/
303+
function makeWebViewStateWithActiveProjectSpies(
304+
setActiveProject: jest.Mock,
305+
resetActiveProject: jest.Mock,
306+
) {
307+
return <T,>(key: string, defaultValue: T): [T, (v: T) => void, () => void] => [
308+
defaultValue,
309+
key === 'activeProject' ? setActiveProject : () => {},
310+
key === 'activeProject' ? resetActiveProject : () => {},
311+
];
312+
}
313+
295314
describe('ProjectModals', () => {
296315
beforeEach(() => {
297316
jest.mocked(papi.notifications.send).mockResolvedValue('notification-id');
@@ -452,7 +471,48 @@ describe('ProjectModals', () => {
452471
});
453472

454473
describe('new (create) flow', () => {
455-
it('starts an empty draft, clears the active project, and closes when not dirty', async () => {
474+
it('seeds the draft, calls createProject on the backend, and closes', async () => {
475+
jest.mocked(papi.commands.sendCommand).mockResolvedValueOnce(JSON.stringify(MOCK_PROJECT));
476+
const newDraft = jest.fn();
477+
const setModal = jest.fn();
478+
const setActiveProject = jest.fn();
479+
const resetActiveProject = jest.fn();
480+
render(
481+
<ProjectModals
482+
{...buildProps({
483+
modal: 'create',
484+
newDraft,
485+
setModal,
486+
useWebViewState: makeWebViewStateWithActiveProjectSpies(
487+
setActiveProject,
488+
resetActiveProject,
489+
),
490+
})}
491+
/>,
492+
);
493+
494+
await userEvent.click(screen.getByTestId('create-submit'));
495+
496+
await waitFor(() => expect(setActiveProject).toHaveBeenCalledWith(MOCK_PROJECT));
497+
expect(newDraft).toHaveBeenCalledWith({
498+
analysisLanguages: ['en'],
499+
suggestedName: 'New',
500+
suggestedDescription: 'Desc',
501+
});
502+
expect(papi.commands.sendCommand).toHaveBeenCalledWith(
503+
'interlinearizer.createProject',
504+
'source-proj',
505+
['en'],
506+
undefined,
507+
'New',
508+
'Desc',
509+
);
510+
expect(resetActiveProject).not.toHaveBeenCalled();
511+
expect(setModal).toHaveBeenCalledWith('none');
512+
});
513+
514+
it('falls back to resetActiveProject and closes when backend project creation fails', async () => {
515+
// Default mock returns undefined; JSON.parse(undefined) throws into the catch block.
456516
const newDraft = jest.fn();
457517
const setModal = jest.fn();
458518
const resetActiveProject = jest.fn();
@@ -469,23 +529,45 @@ describe('ProjectModals', () => {
469529

470530
await userEvent.click(screen.getByTestId('create-submit'));
471531

472-
// New starts a draft locally (carrying the typed name/description for the Save As prefill) and
473-
// clears the active project so Save routes to Save As; no backend project is created.
532+
await waitFor(() => expect(setModal).toHaveBeenCalledWith('none'));
474533
expect(newDraft).toHaveBeenCalledWith({
475534
analysisLanguages: ['en'],
476535
suggestedName: 'New',
477536
suggestedDescription: 'Desc',
478537
});
479-
expect(papi.commands.sendCommand).not.toHaveBeenCalledWith(
538+
expect(papi.commands.sendCommand).toHaveBeenCalledWith(
480539
'interlinearizer.createProject',
481-
expect.anything(),
482-
expect.anything(),
483-
expect.anything(),
484-
expect.anything(),
485-
expect.anything(),
540+
'source-proj',
541+
['en'],
542+
undefined,
543+
'New',
544+
'Desc',
486545
);
487546
expect(resetActiveProject).toHaveBeenCalledTimes(1);
488-
expect(setModal).toHaveBeenCalledWith('none');
547+
});
548+
549+
it('notifies and falls back to resetActiveProject when backend returns a non-project shape', async () => {
550+
jest.mocked(papi.commands.sendCommand).mockResolvedValueOnce(JSON.stringify({ bad: true }));
551+
const setModal = jest.fn();
552+
const resetActiveProject = jest.fn();
553+
render(
554+
<ProjectModals
555+
{...buildProps({
556+
modal: 'create',
557+
setModal,
558+
useWebViewState: makeWebViewStateWithResetSpy(resetActiveProject),
559+
})}
560+
/>,
561+
);
562+
563+
await userEvent.click(screen.getByTestId('create-submit'));
564+
565+
await waitFor(() => expect(setModal).toHaveBeenCalledWith('none'));
566+
expect(papi.notifications.send).toHaveBeenCalledWith({
567+
message: '%interlinearizer_error_create_project_failed%',
568+
severity: 'error',
569+
});
570+
expect(resetActiveProject).toHaveBeenCalledTimes(1);
489571
});
490572

491573
it('calls setModal with none when the create modal closes without a select source', async () => {
@@ -540,14 +622,22 @@ describe('ProjectModals', () => {
540622
expect(loadFromProject).not.toHaveBeenCalled();
541623
});
542624

543-
it('confirms before starting a new draft when the draft is dirty', async () => {
625+
it('confirms before creating a project when the draft is dirty', async () => {
544626
const newDraft = jest.fn();
545627
render(<ProjectModals {...buildProps({ modal: 'create', dirty: true, newDraft })} />);
546628

547629
await userEvent.click(screen.getByTestId('create-submit'));
548630
expect(screen.getByTestId('discard-modal')).toBeInTheDocument();
549-
// The new draft must not start until the user confirms discarding the current one.
631+
// Neither draft nor project creation should start until the user confirms discarding.
550632
expect(newDraft).not.toHaveBeenCalled();
633+
expect(papi.commands.sendCommand).not.toHaveBeenCalledWith(
634+
'interlinearizer.createProject',
635+
expect.anything(),
636+
expect.anything(),
637+
expect.anything(),
638+
expect.anything(),
639+
expect.anything(),
640+
);
551641

552642
await userEvent.click(screen.getByTestId('discard-confirm'));
553643
await waitFor(() =>
@@ -557,6 +647,14 @@ describe('ProjectModals', () => {
557647
suggestedDescription: 'Desc',
558648
}),
559649
);
650+
expect(papi.commands.sendCommand).toHaveBeenCalledWith(
651+
'interlinearizer.createProject',
652+
'source-proj',
653+
['en'],
654+
undefined,
655+
'New',
656+
'Desc',
657+
);
560658
});
561659

562660
it('disables the discard-confirm button while an open is in flight', async () => {

src/components/modals/ProjectModals.tsx

Lines changed: 22 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,9 @@ import { SelectInterlinearProjectModal } from './SelectInterlinearProjectModal';
1515
export type ModalState = 'none' | 'select' | 'create' | 'metadata' | 'saveAs';
1616

1717
/**
18-
* A draft-replacing action deferred behind the unsaved-changes confirmation: either starting a new
19-
* empty draft or opening an existing project into the draft.
18+
* A draft-replacing action deferred behind the unsaved-changes confirmation: either creating and
19+
* persisting a new project (then seeding the draft from it), or opening an existing project into
20+
* the draft.
2021
*/
2122
type PendingReplace =
2223
| { kind: 'new'; config: CreateDraftConfig }
@@ -27,8 +28,10 @@ type PendingReplace =
2728
* {@link SelectInterlinearProjectModal}, {@link CreateProjectModal}, {@link ProjectMetadataModal}, or
2829
* {@link SaveAsProjectModal}, with the {@link DiscardDraftConfirm} guard overlaid on top when a
2930
* draft-replacing action is pending (so canceling returns to the underlying modal with its state
30-
* intact); manages the shared WebView state for the active project; and routes New / Open / Save As
31-
* through the draft (rather than persisting projects directly on every edit).
31+
* intact); manages the shared WebView state for the active project; and routes project lifecycle
32+
* actions through the draft: New creates and persists a project immediately (so it appears in
33+
* Select right away) and seeds the draft from it; Open loads an existing project into the draft;
34+
* Save / Save As write the draft's analysis back to the active project or create a new one.
3235
*
3336
* @param props - Component props
3437
* @param props.activeProject - The currently active interlinear project (the Save target), read
@@ -222,6 +225,7 @@ export default function ProjectModals({
222225
...(config.name !== undefined && { suggestedName: config.name }),
223226
...(config.description !== undefined && { suggestedDescription: config.description }),
224227
});
228+
let created: InterlinearProjectSummary | undefined;
225229
try {
226230
const createdJson = await papi.commands.sendCommand(
227231
'interlinearizer.createProject',
@@ -231,19 +235,20 @@ export default function ProjectModals({
231235
config.name,
232236
config.description,
233237
);
234-
const created: unknown = JSON.parse(createdJson);
235-
if (!isInterlinearProjectSummary(created)) {
238+
const parsed: unknown = JSON.parse(createdJson);
239+
if (isInterlinearProjectSummary(parsed)) {
240+
created = parsed;
241+
} else {
236242
await papi.notifications
237243
.send({ message: '%interlinearizer_error_create_project_failed%', severity: 'error' })
238244
.catch(() => {});
239-
resetActiveProject();
240-
setCreateSourceIsSelect(false);
241-
setModal('none');
242-
return;
243245
}
244-
setActiveProject(created);
245246
} catch (e) {
246247
logger.error('Interlinearizer: failed to create project from New dialog', e);
248+
}
249+
if (created) {
250+
setActiveProject(created);
251+
} else {
247252
resetActiveProject();
248253
}
249254
setCreateSourceIsSelect(false);
@@ -302,7 +307,12 @@ export default function ProjectModals({
302307
if (pendingReplace.kind === 'open') {
303308
await openProject(pendingReplace.project);
304309
} else {
305-
await createAndPersistProject(pendingReplace.config);
310+
setIsCreating(true);
311+
try {
312+
await createAndPersistProject(pendingReplace.config);
313+
} finally {
314+
setIsCreating(false);
315+
}
306316
}
307317
} finally {
308318
setIsReplacing(false);

0 commit comments

Comments
 (0)