From c9117907dd863c81c95ddfb471cf68c353fba53f Mon Sep 17 00:00:00 2001 From: Alex Rawlings Date: Tue, 23 Jun 2026 10:31:35 -0600 Subject: [PATCH 1/2] Refactor modal layer, remove dead code, and fix review findings Quality cleanup stacked on the persisted-draft work, deferred out of that PR. Modal layer: - Add ModalShell, ConfirmDialog, useSubmitGuard, and parseLanguageTags and use them across the six project modals: shared overlay/dialog/title chrome, WipeConfirm and DiscardDraftConfirm collapsed into ConfirmDialog, and the duplicated comma-separated language parsing and double-submit guards unified. Dead code: - Remove the never-dispatched setAnalysis reducer and the unused selectPhraseAnalysisById selector (and their tests); drop needless exports on ProjectMetadataModalProps, ARC_CORNER_RADIUS, and RECENTER_FADE_EASING. Correctness: - isPhraseAnalysisLink rejects an empty tokens array (fail-safe at the load gate); resolveApprovedAnalysis uses findLast so it mutates the same approved link the read selectors surface under duplicate-link corruption. Tailwind: - Extract modal-actions, modal-form-label, and modal-error-box utilities for the repeated footer-row, form-label, and destructive-box class strings across the modals, and reuse the existing section-label utility in SegmentView instead of re-listing its classes. No unused custom utilities found. Docs/simplification: - Correct useGlossDispatch, ActiveProject (updateAnalysis), and firstIndex JSDoc; hoist ScriptureNavControls string keys to a module const; hoist the duplicated verseKey(liveScrRef) in InterlinearNavContext; extract closeCurrentVerse in usjBookExtractor. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/__tests__/store/analysisSlice.test.ts | 41 --- src/components/AnalysisStore.tsx | 6 +- src/components/InterlinearNavContext.tsx | 7 +- src/components/SegmentView.tsx | 6 +- .../controls/ScriptureNavControls.tsx | 12 +- src/components/modals/ConfirmDialog.tsx | 63 ++++ src/components/modals/CreateProjectModal.tsx | 103 +++--- src/components/modals/DiscardDraftConfirm.tsx | 41 +-- src/components/modals/ModalShell.tsx | 47 +++ .../modals/ProjectMetadataModal.tsx | 346 +++++++++--------- src/components/modals/SaveAsProjectModal.tsx | 220 +++++------ .../modals/SelectInterlinearProjectModal.tsx | 131 ++++--- src/components/modals/WipeModal.tsx | 107 +++--- src/components/recenter-fade.ts | 2 +- src/hooks/useSubmitGuard.ts | 47 +++ src/store/analysisSlice.ts | 40 +- src/tailwind.css | 12 + src/types/interlinearizer.d.ts | 3 +- src/types/token-layout.ts | 5 +- src/types/type-guards.ts | 1 + src/utils/language-tags.ts | 17 + src/utils/phrase-arc.ts | 2 +- 22 files changed, 656 insertions(+), 603 deletions(-) create mode 100644 src/components/modals/ConfirmDialog.tsx create mode 100644 src/components/modals/ModalShell.tsx create mode 100644 src/hooks/useSubmitGuard.ts create mode 100644 src/utils/language-tags.ts diff --git a/src/__tests__/store/analysisSlice.test.ts b/src/__tests__/store/analysisSlice.test.ts index d94a5079..7703b251 100644 --- a/src/__tests__/store/analysisSlice.test.ts +++ b/src/__tests__/store/analysisSlice.test.ts @@ -15,17 +15,14 @@ import { makePhraseLink } from '../test-helpers'; import { emptyAnalysis } from '../../types/empty-factories'; import { createPhrase, - defaultState, deleteMorphemes, deletePhrase, mergePhrases, selectApprovedGloss, selectApprovedMorphemes, selectPhraseLinkByTokenRef, - selectPhraseAnalysisById, selectPhraseGloss, selectPhraseLinks, - setAnalysis, selectSegmentFreeTranslation, updatePhrase, writeGloss, @@ -63,23 +60,6 @@ function makeAnalysis(ta: TokenAnalysis): TextAnalysis { }; } -describe('setAnalysis', () => { - it('replaces the full analysis in state', () => { - const store = createAnalysisStore(); - const next = makeAnalysis({ id: 'ta-1', surfaceText: 'word', gloss: { und: 'hi' } }); - - store.dispatch(setAnalysis(next)); - - expect(store.getState().analysis.analysis).toStrictEqual(next); - }); - - it('does not mutate analysisLanguage', () => { - const store = createAnalysisStore({ analysis: { ...defaultState, analysisLanguage: 'fr' } }); - store.dispatch(setAnalysis(emptyAnalysis())); - expect(store.getState().analysis.analysisLanguage).toBe('fr'); - }); -}); - describe('writeGloss', () => { it('removes an orphaned approved link and creates a fresh one', () => { // Orphaned state: an approved link whose analysisId has no matching TokenAnalysis. @@ -564,27 +544,6 @@ describe('selectPhraseLinkByTokenRef', () => { }); }); -describe('selectPhraseAnalysisById', () => { - it('returns the PhraseAnalysis for a known id', () => { - const link = makePhraseLink('phrase-1', ['tok-a']); - const store = createAnalysisStore({ - analysis: { analysis: makeAnalysisWithPhrase(link), analysisLanguage: 'und' }, - }); - - const result = selectPhraseAnalysisById(store.getState().analysis, 'phrase-1'); - - expect(result?.id).toBe('phrase-1'); - }); - - it('returns undefined for an unknown id', () => { - const store = createAnalysisStore(); - - const result = selectPhraseAnalysisById(store.getState().analysis, 'nonexistent'); - - expect(result).toBeUndefined(); - }); -}); - describe('writePhraseGloss', () => { it('writes the gloss for the active language on a phrase', () => { const link = makePhraseLink('phrase-1', ['tok-a']); diff --git a/src/components/AnalysisStore.tsx b/src/components/AnalysisStore.tsx index d483d30f..49102c7e 100644 --- a/src/components/AnalysisStore.tsx +++ b/src/components/AnalysisStore.tsx @@ -273,9 +273,9 @@ export function useAnalysis(): TextAnalysis { } /** - * Returns the stable `onGlossChange` callback from the nearest {@link AnalysisStoreProvider}. The - * callback creates or updates the approved `TokenAnalysis` for the token on each call, then - * synchronously invokes `onSave` and the `onGlossChange` spy. + * Returns a stable callback that creates or updates the approved `TokenAnalysis` for a token, then + * synchronously invokes `onSave` and the optional `onGlossChange` spy. The `onGlossChange` spy is + * test-only observability and has no effect on store behavior (see {@link AnalysisStoreProvider}). * * @returns A function `(tokenRef, surfaceText, value) => void`. * @throws When called outside an {@link AnalysisStoreProvider}. diff --git a/src/components/InterlinearNavContext.tsx b/src/components/InterlinearNavContext.tsx index 379a1b13..9e6b3845 100644 --- a/src/components/InterlinearNavContext.tsx +++ b/src/components/InterlinearNavContext.tsx @@ -325,6 +325,9 @@ export function InterlinearNavProvider({ /** Handle of the in-flight fade-in→idle timer, or `undefined` when none is pending. */ const fadeInTimeoutRef = useRef | undefined>(undefined); + /** Verse key of the live ref, computed once for the cross-book / mid-reveal render guards below. */ + const liveKey = verseKey(liveScrRef); + // Detect a cross-book navigation *during render* and start the fade-out synchronously, so the // curtain drops in the same commit the book ref changes — never a paint later (an effect-driven // fade-out would let the mounted views render one frame against the new book's ref before the @@ -339,8 +342,8 @@ export function InterlinearNavProvider({ setFadePhase('out'); } else if ( fadePhase === 'in' && - verseKey(liveScrRef) !== verseKey(prevLiveScrRef) && - !isInternalNavMarkerFresh(pendingInternalNavRef.current.get(verseKey(liveScrRef))) + liveKey !== verseKey(prevLiveScrRef) && + !isInternalNavMarkerFresh(pendingInternalNavRef.current.get(liveKey)) ) { // A follow-up external navigation landing mid-fade-in: the host resolves one picker selection // as two navigations (book change, then precise target), so the second routinely arrives while diff --git a/src/components/SegmentView.tsx b/src/components/SegmentView.tsx index 305c6e29..28fe6e39 100644 --- a/src/components/SegmentView.tsx +++ b/src/components/SegmentView.tsx @@ -177,11 +177,7 @@ export function SegmentView({ // it stays a bare verse number (the chapter is carried by SegmentListView's inline header). const verseLabelText = chapterLabelInVerse ? `${chapter}:${verse}` : `${verse}`; - const segmentHeader = ( - - {verseLabelText} - - ); + const segmentHeader = {verseLabelText}; /** * `false` until just after the first paint, then `true`. Gates the link-slot fade transition: the diff --git a/src/components/controls/ScriptureNavControls.tsx b/src/components/controls/ScriptureNavControls.tsx index 46528539..ddd63a1d 100644 --- a/src/components/controls/ScriptureNavControls.tsx +++ b/src/components/controls/ScriptureNavControls.tsx @@ -1,5 +1,4 @@ import { useLocalizedStrings, useRecentScriptureRefs } from '@papi/frontend/react'; -import { useMemo } from 'react'; import { BOOK_CHAPTER_CONTROL_STRING_KEYS, BookChapterControl, @@ -11,6 +10,13 @@ import { /** Fixed set of scroll-group IDs offered in the selector; `undefined` means "unlinked". */ const AVAILABLE_SCROLL_GROUPS = [undefined, 0, 1, 2, 3, 4]; +/** + * Localized string keys for {@link BookChapterControl}, hoisted to module scope so the array + * reference passed to `useLocalizedStrings` is stable across renders (a fresh array each render + * would make the hook re-fetch every render). Mirrors the `STRING_KEYS` pattern in the views. + */ +const STRING_KEYS = [...BOOK_CHAPTER_CONTROL_STRING_KEYS]; + /** * Props for {@link ScriptureNavControls}. Combines the scripture-reference fields from * `BookChapterControlProps` with the scroll-group fields from `ScrollGroupSelectorProps`. @@ -35,9 +41,7 @@ export default function ScriptureNavControls({ scrollGroupId, onChangeScrollGroupId, }: ScriptureNavControlsProps) { - const [localizedStrings] = useLocalizedStrings( - useMemo(() => [...BOOK_CHAPTER_CONTROL_STRING_KEYS], []), - ); + const [localizedStrings] = useLocalizedStrings(STRING_KEYS); const { recentScriptureRefs: recentRefs, addRecentScriptureRef: onAddRecentRef } = useRecentScriptureRefs(); diff --git a/src/components/modals/ConfirmDialog.tsx b/src/components/modals/ConfirmDialog.tsx new file mode 100644 index 00000000..33dd6c53 --- /dev/null +++ b/src/components/modals/ConfirmDialog.tsx @@ -0,0 +1,63 @@ +/** @file Shared confirm/cancel dialog for destructive or discard confirmations. */ +import { Button } from 'platform-bible-react'; +import { ModalShell } from './ModalShell'; + +/** + * A small modal asking the user to confirm or cancel a single action (a destructive wipe, a discard + * of unsaved work, etc.). Presentational: it renders the shared {@link ModalShell} chrome with a + * body paragraph and a secondary Cancel / destructive Confirm pair, and delegates the decision to + * the caller via {@link onConfirm} / {@link onCancel}. + * + * @param props - Component props + * @param props.titleId - DOM id wired to the dialog's `aria-labelledby` and title. + * @param props.title - Localized title text. + * @param props.body - Localized body text explaining the consequence. + * @param props.confirmLabel - Localized label for the destructive confirm button. + * @param props.cancelLabel - Localized label for the secondary cancel button. + * @param props.confirmTestId - `data-testid` placed on the confirm button so tests can target it. + * @param props.isSubmitting - When `true`, both buttons are disabled to prevent interaction while + * the caller is processing the confirmed action. + * @param props.onConfirm - Called when the user confirms the action. + * @param props.onCancel - Called when the user backs out, leaving state untouched. + * @returns The confirmation overlay. + */ +export function ConfirmDialog({ + titleId, + title, + body, + confirmLabel, + cancelLabel, + confirmTestId, + isSubmitting = false, + onConfirm, + onCancel, +}: Readonly<{ + titleId: string; + title: string; + body: string; + confirmLabel: string; + cancelLabel: string; + confirmTestId: string; + isSubmitting?: boolean; + onConfirm: () => void; + onCancel: () => void; +}>) { + return ( + +

{body}

+
+ + +
+ + ); +} diff --git a/src/components/modals/CreateProjectModal.tsx b/src/components/modals/CreateProjectModal.tsx index 35781efe..4b592336 100644 --- a/src/components/modals/CreateProjectModal.tsx +++ b/src/components/modals/CreateProjectModal.tsx @@ -1,6 +1,8 @@ import { useLocalizedStrings } from '@papi/frontend/react'; import { Button } from 'platform-bible-react'; import { useState, useCallback } from 'react'; +import { parseLanguageTags } from '../../utils/language-tags'; +import { ModalShell } from './ModalShell'; /** Localized string keys used by {@link CreateProjectModal}. */ const CREATE_PROJECT_MODAL_STRING_KEYS: `%${string}%`[] = [ @@ -68,10 +70,7 @@ export function CreateProjectModal({ * {@link onCreateDraft}. */ const handleSubmit = useCallback(() => { - const parsedLanguages = analysisLanguages - .split(',') - .map((t) => t.trim()) - .filter((t) => t.length > 0); + const parsedLanguages = parseLanguageTags(analysisLanguages); const normalizedAnalysisLanguages = parsedLanguages.length > 0 ? parsedLanguages : ['und']; onCreateDraft({ analysisLanguages: normalizedAnalysisLanguages, @@ -83,56 +82,50 @@ export function CreateProjectModal({ /* v8 ignore next */ if (stringsLoading) return undefined; return ( -
- -

- {localizedStrings['%interlinearizer_modal_create_title%']} -

- - setName(e.target.value)} - placeholder={localizedStrings['%interlinearizer_modal_create_name_placeholder%']} - /> - -