From a02ddb61c037c79730cc9adf03aac8d643259cc5 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Tue, 1 Sep 2026 16:40:10 -0400 Subject: [PATCH 1/6] Seal the read-only import and make its sync land Every editing affordance the read-only import view still offered is gone, and a sync now reaches the view it refreshed. - Merge, baseline-split, and arc-split controls read the store's read-only flag; the import view's segmentation dispatch is inert as a backstop, and a phrase mode entered on the draft no longer carries into the import. - The import's analysis is cleared before each fetch, so the store's mount-time seed cannot pin pre-sync content in the view; a fetch that brings back nothing says so in the view area. - The accepted first-open offer gets its intended single-door report. - Opening an import runs through the select modal's submit guard, and the first-open probe gives up rather than hanging the tab. Co-Authored-By: Claude Opus 5 (1M context) --- contributions/localizedStrings.json | 1 + src/__tests__/components/ArcOverlay.test.tsx | 31 ++++ .../components/Interlinearizer.test.tsx | 14 +- .../components/InterlinearizerLoader.test.tsx | 134 +++++++++++++++++- src/__tests__/components/SegmentView.test.tsx | 12 +- .../components/modals/ProjectModals.test.tsx | 39 ++++- .../hooks/usePt9ImportAvailability.test.ts | 43 +++++- src/components/ArcOverlay.tsx | 11 +- src/components/InterlinearizerLoader.tsx | 58 +++++--- src/components/SegmentListView.tsx | 9 +- src/components/SegmentView.tsx | 33 +++-- src/components/modals/ProjectModals.tsx | 17 ++- .../modals/SelectInterlinearProjectModal.tsx | 10 +- src/hooks/usePt9ImportAvailability.ts | 37 ++++- user-questions.md | 5 +- 15 files changed, 390 insertions(+), 64 deletions(-) diff --git a/contributions/localizedStrings.json b/contributions/localizedStrings.json index e0bbe074..d3f5fb54 100644 --- a/contributions/localizedStrings.json +++ b/contributions/localizedStrings.json @@ -165,6 +165,7 @@ "%interlinearizer_error_pt9Import_failed%": "Could not import the Paratext 9 interlinear data. Please try again.", "%interlinearizer_warning_pt9Import_sourceEmpty%": "The project's Paratext 9 interlinear files are missing, so the imported data was left as it was.", "%interlinearizer_error_createEditableCopy_failed%": "Could not copy the imported project. Please try again.", + "%interlinearizer_error_pt9Import_load_failed%": "The imported interlinear data could not be loaded. Try syncing from Paratext 9.", "%interlinearizer_modal_select_importPt9%": "Import from Paratext 9", "%interlinearizer_readonly_chip%": "Read-only", diff --git a/src/__tests__/components/ArcOverlay.test.tsx b/src/__tests__/components/ArcOverlay.test.tsx index d3acd4bc..7874d8ec 100644 --- a/src/__tests__/components/ArcOverlay.test.tsx +++ b/src/__tests__/components/ArcOverlay.test.tsx @@ -8,6 +8,26 @@ import type { ArcPath } from '../../utils/phrase-arc'; import { makePhraseLink } from '../test-helpers'; import { withTooltipProvider } from './test-helpers'; +jest.mock('../../components/AnalysisStore'); + +/** The manual AnalysisStore mock's test-only controls. */ +interface AnalysisStoreReadOnlyMock { + __setMockAnalysisReadOnly: (value: boolean) => void; +} + +function isAnalysisStoreReadOnlyMock(m: unknown): m is AnalysisStoreReadOnlyMock { + return !!m && typeof m === 'object' && '__setMockAnalysisReadOnly' in m; +} + +const analysisStoreMock: unknown = jest.requireMock('../../components/AnalysisStore'); +if (!isAnalysisStoreReadOnlyMock(analysisStoreMock)) + throw new Error('Expected the AnalysisStore manual mock with read-only controls'); +const { __setMockAnalysisReadOnly: setMockAnalysisReadOnly } = analysisStoreMock; + +afterEach(() => { + setMockAnalysisReadOnly(false); +}); + /** Builds a minimal `ArcPath` fixture. */ function makeArcPath(phraseId: string, splitAfterTokenRef = 'tok-a'): ArcPath { // `d` is derived from splitAfterTokenRef so distinct split points yield distinct @@ -75,6 +95,17 @@ describe('ArcOverlay', () => { expect(screen.queryByTestId('split-arc-btn')).not.toBeInTheDocument(); }); + it('draws the arcs but no split buttons for a read-only analysis', () => { + const phraseLink = makePhraseLink('p1', ['tok-a', 'tok-b']); + setMockAnalysisReadOnly(true); + renderOverlay({ + arcPaths: [makeArcPath('p1', 'tok-a')], + phraseLinkById: new Map([['p1', phraseLink]]), + }); + expect(document.querySelectorAll('path')).toHaveLength(1); + expect(screen.queryByTestId('split-arc-btn')).not.toBeInTheDocument(); + }); + it('renders a split button in view mode even when the arc phrase is neither hovered nor focused', () => { const phraseLink = makePhraseLink('p1', ['tok-a', 'tok-b']); renderOverlay({ diff --git a/src/__tests__/components/Interlinearizer.test.tsx b/src/__tests__/components/Interlinearizer.test.tsx index a624945f..0db88d63 100644 --- a/src/__tests__/components/Interlinearizer.test.tsx +++ b/src/__tests__/components/Interlinearizer.test.tsx @@ -101,9 +101,12 @@ const mockPhraseLinkById = new Map(); /** Read once per `Interlinearizer` render, so this doubles as a render counter. */ let phraseLinkByIdMapReads = 0; +/** What the mocked `useAnalysisReadOnly` reports; reset in `beforeEach`. */ +let mockReadOnly = false; + jest.mock('../../components/AnalysisStore', () => ({ __esModule: true, - useAnalysisReadOnly: () => false, + useAnalysisReadOnly: () => mockReadOnly, /** * Pass-through provider stub that renders children directly, keeping AnalysisStore.tsx out of * scope. @@ -424,6 +427,7 @@ beforeEach(() => { // The phrase-link map is a plain Map (not a jest mock), so resetMocks does not clear it. mockPhraseLinkById.clear(); capturedSegmentation = undefined; + mockReadOnly = false; // The merge control's label comes from a localized string. mockKeyAsValueLocalizedStrings(); }); @@ -1641,6 +1645,14 @@ describe('between-rows merge control', () => { expect(button).toHaveAttribute('title', 'Merge'); }); + it('renders no merge control for a read-only analysis', () => { + // Omitted rather than disabled: a read-only analysis offers no boundary editing at all. + mockReadOnly = true; + renderInterlinearizer({ book: GEN_1_MULTI_BOOK }); + expect(screen.queryByTestId('segment-merge-btn')).not.toBeInTheDocument(); + expect(screen.queryByTestId('segment-merge-indicator')).not.toBeInTheDocument(); + }); + it('renders no merge control while a phrase mode is active', () => { // A merge mid-mode could re-segment the phrase the mode UI is operating on, so the between-rows // control is omitted entirely (not merely disabled) throughout a phrase edit. diff --git a/src/__tests__/components/InterlinearizerLoader.test.tsx b/src/__tests__/components/InterlinearizerLoader.test.tsx index 5bebca51..d9daf87a 100644 --- a/src/__tests__/components/InterlinearizerLoader.test.tsx +++ b/src/__tests__/components/InterlinearizerLoader.test.tsx @@ -4,7 +4,7 @@ import papi, { logger } from '@papi/frontend'; import { useData, useLocalizedStrings, useSetting } from '@papi/frontend/react'; import type { SerializedVerseRef } from '@sillsdev/scripture'; -import { act, cleanup, fireEvent, render, screen } from '@testing-library/react'; +import { act, cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import type { Book, DraftProject, PhraseAnalysisLink, TextAnalysis } from 'interlinearizer'; import type { Dispatch, ReactNode, SetStateAction } from 'react'; @@ -1180,7 +1180,7 @@ describe('InterlinearizerLoader', () => { ); }); - it('returns to the plain view when an offer-run report is closed', async () => { + it('offers an offer-run report no way out but opening the import it just made', async () => { mockOfferProbe(); mockImportCommands({ importResult: { outcome: 'imported', projectId: 'import-1', report: IMPORT_REPORT }, @@ -1193,11 +1193,15 @@ describe('InterlinearizerLoader', () => { ); await screen.findByTestId('pt9-import-report'); + expect( + screen.queryByRole('button', { name: '%interlinearizer_pt9ImportModal_close%' }), + ).not.toBeInTheDocument(); + await userEvent.click( - screen.getByRole('button', { name: '%interlinearizer_pt9ImportModal_close%' }), + screen.getByRole('button', { name: '%interlinearizer_pt9ImportModal_open%' }), ); - expect(screen.getByTestId('project-modals')).toHaveAttribute('data-modal', 'none'); + expect(await screen.findByTestId('pt9-import-banner')).toBeInTheDocument(); }); it('persists the empty draft and runs no import on No', async () => { @@ -1447,6 +1451,25 @@ describe('InterlinearizerLoader', () => { ); }); + it('titles the open-path sync as a sync while it runs', async () => { + mockSendCommand.mockImplementation(async (...args) => { + if (args[0] === 'interlinearizer.importPt9Project') return new Promise(() => {}); + return JSON.stringify(emptyDraft(testProjectId)); + }); + mockPdpGet.mockResolvedValue({ + getPt9InterlinearManifest: async () => ({ 'Lexicon.xml': 'bbbb2222' }), + }); + await act(async () => { + renderLoader(); + }); + await userEvent.click(screen.getByTestId('tab-toolbar-project-menu')); + await userEvent.click(screen.getByTestId('select-modal-open-import')); + + expect(await screen.findByTestId('pt9-import-running')).toHaveTextContent( + '%interlinearizer_pt9ImportModal_syncing%', + ); + }); + it('opens the stored import with a warning when the open-path sync fails', async () => { mockImportCommands(); mockPdpGet.mockRejectedValue(new Error('provider unavailable')); @@ -1545,6 +1568,109 @@ describe('InterlinearizerLoader', () => { ); }); + it('reseeds the view with the analysis a sync fetched, not the one it replaced', async () => { + const syncedAnalysis: TextAnalysis = { + ...emptyAnalysis(), + tokenAnalyses: [ + { ...FIXTURE_STAMPS, id: 'ta-synced', surfaceText: 'In', gloss: { en: 'in' } }, + ], + }; + let synced = false; + mockSendCommand.mockImplementation(async (...args) => { + if (args[0] === 'interlinearizer.getProject') + return JSON.stringify( + synced + ? { ...FRESH_IMPORT_SUMMARY, analysis: syncedAnalysis } + : { ...STUB_IMPORT_PROJECT, analysis: emptyAnalysis() }, + ); + if (args[0] === 'interlinearizer.importPt9Project') { + synced = true; + return JSON.stringify({ + outcome: 'imported', + projectId: 'import-1', + report: IMPORT_REPORT, + }); + } + return JSON.stringify(emptyDraft(testProjectId)); + }); + await renderImportView(); + expect(capturedStoreProps?.initialAnalysis).toEqual(emptyAnalysis()); + + await userEvent.click(screen.getByTestId('pt9-sync-button')); + + await waitFor(() => expect(capturedStoreProps?.initialAnalysis).toEqual(syncedAnalysis)); + }); + + it('empties the import view when a refresh brings back no analysis, rather than keeping the pre-sync one', async () => { + let synced = false; + mockSendCommand.mockImplementation(async (...args) => { + // Once synced, the record comes back as a valid summary carrying no analysis at all: the + // sync's own summary fetch is satisfied, while the refresh behind it finds nothing to show. + if (args[0] === 'interlinearizer.getProject') + return JSON.stringify( + synced ? FRESH_IMPORT_SUMMARY : { ...STUB_IMPORT_PROJECT, analysis: emptyAnalysis() }, + ); + if (args[0] === 'interlinearizer.importPt9Project') { + synced = true; + return JSON.stringify({ + outcome: 'imported', + projectId: 'import-1', + report: IMPORT_REPORT, + }); + } + return JSON.stringify(emptyDraft(testProjectId)); + }); + await renderImportView(); + expect(screen.getByTestId('interlinearizer')).toBeInTheDocument(); + + await userEvent.click(screen.getByTestId('pt9-sync-button')); + + await waitFor(() => expect(screen.queryByTestId('interlinearizer')).not.toBeInTheDocument()); + expect(screen.getByTestId('pt9-import-load-error')).toHaveTextContent( + '%interlinearizer_error_pt9Import_load_failed%', + ); + expect(screen.getByTestId('pt9-import-banner')).toBeInTheDocument(); + }); + + it('hands the import view a segmentation dispatch that cannot write the draft', async () => { + mockImportCommands(); + await renderImportView(); + mockSendCommand.mockClear(); + + act(() => { + capturedInterlinearizerProps?.segmentationDispatch.merge('GEN 1:2:0'); + capturedInterlinearizerProps?.segmentationDispatch.split('GEN 1:1:2'); + capturedInterlinearizerProps?.segmentationDispatch.move('GEN 1:1:2', 'GEN 1:1:4'); + }); + + expect(mockSendCommand).not.toHaveBeenCalled(); + }); + + it('drops a phrase mode entered on the draft when an import opens', async () => { + mockImportCommands(); + await act(async () => { + renderLoader(); + }); + act(() => { + capturedInterlinearizerProps?.setPhraseMode({ + kind: 'edit', + phraseId: 'phrase-1', + originalTokens: [{ tokenRef: 'GEN 1:1:0', surfaceText: 'In' }], + }); + }); + expect(capturedInterlinearizerProps?.phraseMode).toEqual({ + kind: 'edit', + phraseId: 'phrase-1', + originalTokens: [{ tokenRef: 'GEN 1:1:0', surfaceText: 'In' }], + }); + + await userEvent.click(screen.getByTestId('tab-toolbar-project-menu')); + await userEvent.click(screen.getByTestId('select-modal-open-import')); + + expect(await screen.findByTestId('pt9-import-banner')).toBeInTheDocument(); + expect(capturedInterlinearizerProps?.phraseMode).toEqual({ kind: 'view' }); + }); + it('falls back to the platform language when the import declares no analysis language', async () => { mockImportCommands(); await act(async () => diff --git a/src/__tests__/components/SegmentView.test.tsx b/src/__tests__/components/SegmentView.test.tsx index 0532b4e7..f3f536c6 100644 --- a/src/__tests__/components/SegmentView.test.tsx +++ b/src/__tests__/components/SegmentView.test.tsx @@ -47,9 +47,12 @@ const mockUsePhraseDispatch = jest.fn, []>().m /** Stable mock fn capturing `useSegmentFreeTranslationDispatch` calls so tests can assert on them. */ const mockSegmentFreeTranslationDispatch = jest.fn(); +/** What the mocked `useAnalysisReadOnly` reports; reset in `beforeEach`. */ +let mockReadOnly = false; + jest.mock('../../components/AnalysisStore', () => ({ __esModule: true, - useAnalysisReadOnly: () => false, + useAnalysisReadOnly: () => mockReadOnly, AnalysisStoreProvider({ children }: Readonly<{ children: ReactNode; analysisLanguage: string }>) { return children; }, @@ -226,6 +229,7 @@ function requiredProps(): { describe('SegmentView', () => { beforeEach(() => { + mockReadOnly = false; mockKeyAsValueLocalizedStrings(); mockUsePhraseLinkMap.mockReturnValue(new Map()); mockUsePhraseDispatch.mockReturnValue({ @@ -585,6 +589,12 @@ describe('SegmentView', () => { expect(screen.queryByTestId('baseline-split-gap')).not.toBeInTheDocument(); }); + it('shows no split gap for a read-only analysis even with Alt held', () => { + mockReadOnly = true; + renderBaseline(); + expect(screen.queryByTestId('baseline-split-gap')).not.toBeInTheDocument(); + }); + it('shows no split gap while a phrase mode is active even with Alt held', () => { renderBaseline({ phraseMode: { kind: 'confirm-unlink', phraseId: 'p1' } }); expect(screen.queryByTestId('baseline-split-gap')).not.toBeInTheDocument(); diff --git a/src/__tests__/components/modals/ProjectModals.test.tsx b/src/__tests__/components/modals/ProjectModals.test.tsx index 7916cf92..7c4e24d7 100644 --- a/src/__tests__/components/modals/ProjectModals.test.tsx +++ b/src/__tests__/components/modals/ProjectModals.test.tsx @@ -1,7 +1,7 @@ /// /// -import { render, screen, waitFor } from '@testing-library/react'; +import { act, render, screen, waitFor } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import papi from '@papi/frontend'; import type { DraftProject } from 'interlinearizer'; @@ -31,6 +31,13 @@ const MOCK_PROJECT_2: InterlinearProjectSummary = { name: 'French Project', }; +/** A Paratext 9 import row, which the select modal routes to `onOpenImport`, not the draft. */ +const MOCK_IMPORT_PROJECT: InterlinearProjectSummary = { + ...MOCK_PROJECT, + id: 'import-1', + pt9Import: { fileHashes: {}, importedAt: '2026-08-01T00:00:00.000Z' }, +}; + /** A full project (with analysis) returned by the mocked `interlinearizer.getProject` command. */ const MOCK_FULL_PROJECT = { ...MOCK_PROJECT, analysis: emptyAnalysis() }; @@ -90,6 +97,13 @@ jest.mock('../../../components/modals/SelectInterlinearProjectModal', () => ({ + @@ -1306,6 +1320,29 @@ describe('ProjectModals Paratext 9 import routing', () => { expect(screen.queryByTestId('discard-modal')).not.toBeInTheDocument(); }); + it('holds the picker inert while an import is opening', async () => { + let finishOpen: (() => void) | undefined; + const onOpenImport = jest.fn( + () => + new Promise((resolve) => { + finishOpen = resolve; + }), + ); + render(); + expect(screen.getByTestId('select-modal')).toHaveAttribute('data-is-opening', 'false'); + + await userEvent.click(screen.getByTestId('select-select-import')); + + expect(onOpenImport).toHaveBeenCalledWith(MOCK_IMPORT_PROJECT); + expect(screen.getByTestId('select-modal')).toHaveAttribute('data-is-opening', 'true'); + + await act(async () => { + finishOpen?.(); + }); + + expect(screen.getByTestId('select-modal')).toHaveAttribute('data-is-opening', 'false'); + }); + it('opens an openRequest project through the draft-open flow', async () => { jest .mocked(papi.commands.sendCommand) diff --git a/src/__tests__/hooks/usePt9ImportAvailability.test.ts b/src/__tests__/hooks/usePt9ImportAvailability.test.ts index 461599e0..5043e1e3 100644 --- a/src/__tests__/hooks/usePt9ImportAvailability.test.ts +++ b/src/__tests__/hooks/usePt9ImportAvailability.test.ts @@ -1,7 +1,7 @@ /// -import { renderHook, waitFor } from '@testing-library/react'; -import papi from '@papi/frontend'; +import { act, renderHook, waitFor } from '@testing-library/react'; +import papi, { logger } from '@papi/frontend'; import usePt9ImportAvailability, { usePt9ImportProbe } from '../../hooks/usePt9ImportAvailability'; import { getMockedPdpGet, makeStubProject } from '../test-helpers'; @@ -118,6 +118,45 @@ describe('usePt9ImportProbe', () => { await waitFor(() => expect(result.current).toBe('unavailable')); }); + it('gives up and reports unavailable when the manifest read never answers', async () => { + jest.useFakeTimers(); + // A provider that accepts the call and never responds - the hang the timeout exists for. + mockPdpGet.mockResolvedValue({ getPt9InterlinearManifest: () => new Promise(() => {}) }); + + const { result } = renderHook(() => usePt9ImportProbe('src-project', true)); + await act(async () => { + await Promise.resolve(); + }); + expect(result.current).toBe('pending'); + + act(() => { + jest.advanceTimersByTime(15_000); + }); + + expect(result.current).toBe('unavailable'); + expect(jest.mocked(logger.warn)).toHaveBeenCalledWith( + expect.stringContaining('went unanswered'), + ); + jest.useRealTimers(); + }); + + it('keeps the answer the manifest read gave and drops the timeout behind it', async () => { + jest.useFakeTimers(); + mockManifest({ 'Lexicon.xml': 'aaaa1111' }); + + const { result } = renderHook(() => usePt9ImportProbe('src-project', true)); + await act(async () => { + await Promise.resolve(); + }); + act(() => { + jest.advanceTimersByTime(15_000); + }); + + expect(result.current).toBe('available'); + expect(jest.mocked(logger.warn)).not.toHaveBeenCalled(); + jest.useRealTimers(); + }); + it('stays pending and never probes while disabled', () => { const { result } = renderHook(() => usePt9ImportProbe('src-project', false)); diff --git a/src/components/ArcOverlay.tsx b/src/components/ArcOverlay.tsx index bb0efc3a..0c9e7626 100644 --- a/src/components/ArcOverlay.tsx +++ b/src/components/ArcOverlay.tsx @@ -5,6 +5,7 @@ import { memo, useState, useCallback } from 'react'; import type { PhraseMode } from '../types/phrase-mode'; import { resolvedOrEmpty, tooltipContentOrUndefined } from '../utils/localized-strings'; import { computeSplitFreeRefs, getArcStrokeProps, type ArcPath } from '../utils/phrase-arc'; +import { useAnalysisReadOnly } from './AnalysisStore'; /** * Identifies one specific arc boundary by phrase id and the token immediately before the split, @@ -109,9 +110,9 @@ type ArcOverlayProps = Readonly<{ }>; /** - * Renders the phrase-arc SVG layer and (in view mode) the split-button overlay on top of a token - * row. Intended to sit as a sibling of the row inside the `arc-container` element that owns the - * coordinate space the arc paths were measured in. + * Renders the phrase-arc SVG layer and (in view mode, for an editable analysis) the split-button + * overlay on top of a token row. Intended to sit as a sibling of the row inside the `arc-container` + * element that owns the coordinate space the arc paths were measured in. * * @returns The SVG + split-button overlay, or `undefined` when there are no arcs to draw. */ @@ -131,6 +132,9 @@ export function ArcOverlay({ }: ArcOverlayProps) { const [splitHoveredArc, setSplitHoveredArc] = useState(); + // The arcs themselves are the read-only view's phrase rendering; only splitting them is an edit. + const readOnly = useAnalysisReadOnly(); + const splitTooltip = tooltipContentOrUndefined(resolvedOrEmpty(splitHereLabel)); /** @@ -289,6 +293,7 @@ export function ArcOverlay({ )} {phraseMode.kind === 'view' && + !readOnly && sortedArcPaths // When simplifyPhrases is on, only the focused phrase keeps its split button; every // other phrase's button is hidden while its arc stays drawn. diff --git a/src/components/InterlinearizerLoader.tsx b/src/components/InterlinearizerLoader.tsx index ffed9dca..24275ddd 100644 --- a/src/components/InterlinearizerLoader.tsx +++ b/src/components/InterlinearizerLoader.tsx @@ -31,7 +31,7 @@ import { isInterlinearProjectSummary, isTextAnalysis, isWordToken } from '../typ import { isPt9ImportReport } from '../converters/pt9'; import { toProjectSummary } from '../types/interlinear-project-summary'; import useSubmitGuard from '../hooks/useSubmitGuard'; -import type { SegmentationDispatch } from './SegmentationStore'; +import { NO_OP_SEGMENTATION_DISPATCH, type SegmentationDispatch } from './SegmentationStore'; import type { InterlinearProjectSummary } from '../types/interlinear-project-summary'; import Interlinearizer from './Interlinearizer'; import { AnalysisStoreProvider } from './AnalysisStore'; @@ -154,6 +154,7 @@ const DEFAULT_CATALOG_LAYOUT: PanelLayout = { [VIEW_PANEL_ID]: 75, [CATALOG_PANE const STRING_KEYS = [ '%interlinearizer_error_load_book_heading%', '%interlinearizer_error_process_book_heading%', + '%interlinearizer_error_pt9Import_load_failed%', '%interlinearizer_loading%', '%interlinearizer_analysisCatalog_resize%', '%interlinearizer_banner_pt9Import%', @@ -305,16 +306,24 @@ function InterlinearizerLoaderInner({ const isImportView = activeProject?.pt9Import !== undefined; /** - * The import's stored analysis, fetched fresh whenever the import view opens or a sync bumps the - * project's `updatedAt`. Never the draft: the user's in-progress draft survives viewing an import - * untouched. + * The import's stored analysis, or `undefined` while a fetch is in flight and after one failed. + * Fetched fresh whenever the import view opens or a sync bumps the project's `updatedAt`. Never + * the draft: the user's in-progress draft survives viewing an import untouched. */ const [importAnalysis, setImportAnalysis] = useState(undefined); + + /** Whether the import's analysis could not be read; the view area says so in place of it. */ + const [importLoadFailed, setImportLoadFailed] = useState(false); useEffect(() => { + setImportLoadFailed(false); if (!isImportView || !activeProject) { setImportAnalysis(undefined); return undefined; } + // The store below seeds on mount alone and remounts on the `updatedAt` a sync bumps, taking + // whichever analysis is held at that commit. Clearing first keeps a pre-sync one from being + // pinned in the view for as long as it stays open. + setImportAnalysis(undefined); let ignore = false; (async () => { try { @@ -331,12 +340,14 @@ function InterlinearizerLoaderInner({ if (isTextAnalysis(analysis)) { setImportAnalysis(analysis); } else { + setImportLoadFailed(true); await papi.notifications .send({ message: '%interlinearizer_error_load_projects_failed%', severity: 'error' }) .catch(() => {}); } } catch (e) { logger.error('Interlinearizer: failed to load the imported analysis', e); + if (!ignore) setImportLoadFailed(true); await papi.notifications .send({ message: '%interlinearizer_error_load_projects_failed%', severity: 'error' }) .catch(() => {}); @@ -485,12 +496,17 @@ function InterlinearizerLoaderInner({ }, [verseBook, segmentationVersion, draftVersion, isDraftLoading]); /** - * Boundary-editing operations exposed through the segmentation context. Each reads the draft's - * latest boundary delta synchronously (so rapid edits compose correctly), applies the relevant - * pure transform against the original verse book, and auto-saves the normalized result — clearing - * the field back to `undefined` when the edit restores the default verse segmentation. + * Boundary-editing operations exposed through the segmentation context, inert while an import is + * the view. Each reads the draft's latest boundary delta synchronously (so rapid edits compose + * correctly), applies the relevant pure transform against the original verse book, and auto-saves + * the normalized result — clearing the field back to `undefined` when the edit restores the + * default verse segmentation. */ const segmentationDispatch = useMemo(() => { + // An import is read-only and its view is not backed by the draft, so a boundary edit reached + // from it has nowhere legitimate to land: the controls are absent there, and this keeps any + // that slips through from rewriting the draft the import is preserving. + if (isImportView) return NO_OP_SEGMENTATION_DISPATCH; /** * Auto-saves the result of a boundary transform, clearing the segmentation field back to * `undefined` when the edit restores the default verse segmentation. @@ -515,7 +531,7 @@ function InterlinearizerLoaderInner({ apply(moveBoundary(verseBook, getDraftSnapshot()?.segmentation, fromRef, toRef)); }, }; - }, [verseBook, getDraftSnapshot, autosaveSegmentation]); + }, [autosaveSegmentation, getDraftSnapshot, isImportView, verseBook]); // The active reference handed to the interlinearizer. The host emits `verseNum: 0` both for a // chapter's verse-0 superscription (which has its own segment) and for a plain whole-chapter @@ -591,20 +607,22 @@ function InterlinearizerLoaderInner({ const [phraseMode, setPhraseMode] = useState({ kind: 'view' }); - // Reset phraseMode whenever the draft is replaced wholesale (New / Open / Wipe) so stale - // edit/confirm-unlink state is never passed to the newly mounted Interlinearizer. + // Reset phraseMode whenever the draft is replaced wholesale (New / Open / Wipe), and whenever the + // view crosses between the draft and an import, so stale edit/confirm-unlink state is never + // passed to the newly mounted Interlinearizer. An import opens without touching the draft or its + // version, and a mode carried into that read-only view renders its edit-target affordances. useEffect(() => { setPhraseMode({ kind: 'view' }); - }, [draftVersion]); + }, [draftVersion, isImportView]); /** What the Paratext 9 import modal shows while `modal` is `'importPt9'`. */ const [pt9Phase, setPt9Phase] = useState({ kind: 'running' }); /** - * Which run the import modal belongs to: a first import from the select modal or the first-open - * offer (report offers Open; closing returns where the run began), a manual sync (report offers - * Close), or the automatic sync on open (running state only; the view opens itself when the run - * settles). + * Which run the import modal belongs to: a first import from the select modal (report offers + * Close and Open, closing returning to the select modal), the accepted first-open offer (report + * offers Open alone, dismissal included), a manual sync (report offers Close), or the automatic + * sync on open (running state only; the view opens itself when the run settles). */ const [pt9Mode, setPt9Mode] = useState<'import' | 'offer' | 'sync' | 'autoSync'>('import'); @@ -1052,6 +1070,12 @@ function InterlinearizerLoaderInner({ {resolvedOrEmpty(localizedStrings['%interlinearizer_loading%'])}

)} + + {importLoadFailed && ( +

+ {localizedStrings['%interlinearizer_error_pt9Import_load_failed%']} +

+ )} ); @@ -1258,7 +1282,7 @@ function InterlinearizerLoaderInner({ {modal === 'importPt9' && ( diff --git a/src/components/SegmentListView.tsx b/src/components/SegmentListView.tsx index 435e4a45..cd71a7d3 100644 --- a/src/components/SegmentListView.tsx +++ b/src/components/SegmentListView.tsx @@ -15,6 +15,7 @@ import { buildSegmentLabels } from '../utils/segment-labels'; import { segmentContainsVerse } from '../utils/verse-ref'; import { buildVerseStartLabels } from '../utils/verse-superscripts'; import { useAltHeldValue } from './AltHeldContext'; +import { useAnalysisReadOnly } from './AnalysisStore'; import { useFocus, useFocusActions } from './FocusStore'; import { useSegmentation } from './SegmentationStore'; import MemoizedSegmentView from './SegmentView'; @@ -193,6 +194,7 @@ export default function SegmentListView({ }: SegmentListViewProps) { const { tokenRef: focusedTokenRef } = useFocus(); const { selectSegment } = useFocusActions(); + const readOnly = useAnalysisReadOnly(); const [localizedStrings] = useLocalizedStrings(HEADER_STRING_KEYS); const recenterTooltip = tooltipContentOrUndefined( @@ -420,9 +422,10 @@ export default function SegmentListView({ // window: merge dispatches against the delta, not the DOM, so the topmost windowed // segment's boundary with a culled predecessor is still editable. const canMerge = mergeableSegmentIds.has(seg.id); - // Omit the merge control while a phrase mode is active: a merge could re-segment the - // phrase the mode UI is operating on. - const showMergeControl = canMerge && phraseMode.kind === 'view'; + // Omit the merge control while a phrase mode is active (a merge could re-segment the + // phrase the mode UI is operating on) and for a read-only analysis, which offers no + // boundary editing at all. + const showMergeControl = canMerge && phraseMode.kind === 'view' && !readOnly; return ( {showMergeControl && } diff --git a/src/components/SegmentView.tsx b/src/components/SegmentView.tsx index 0f55d614..95960090 100644 --- a/src/components/SegmentView.tsx +++ b/src/components/SegmentView.tsx @@ -19,8 +19,8 @@ import { buildRenderUnits, groupTokens, resolveFocusContext } from '../utils/tok import { resolvedOrEmpty, tooltipContentOrUndefined } from '../utils/localized-strings'; import { resolveSplitAnchor } from '../utils/split-anchor'; import { slotVerseLabel, verseStartToken } from '../utils/verse-superscripts'; -import { usePhraseLinkByIdMap, usePhraseLinkMap } from './AnalysisStore'; import { useAltHeldValue } from './AltHeldContext'; +import { useAnalysisReadOnly, usePhraseLinkByIdMap, usePhraseLinkMap } from './AnalysisStore'; import MemoizedArcOverlay from './ArcOverlay'; import SegmentFreeTranslationInput from './SegmentFreeTranslationInput'; import { PhraseStripProvider } from './PhraseStripContext'; @@ -302,6 +302,7 @@ export function SegmentView({ const [localizedStrings] = useLocalizedStrings(STRING_KEYS); const { dispatch, formerBoundaries, straddledBoundaryRefs } = useSegmentation(); + const readOnly = useAnalysisReadOnly(); const phraseLinkByRef = usePhraseLinkMap(); const phraseLinkById = usePhraseLinkByIdMap(); @@ -363,19 +364,27 @@ export function SegmentView({ /** * Split anchor by the char offset of the gap that precedes it, for baseline-text mode: for each * eligible word-word pair the split anchor's leading gap (the inter-token region just before the - * anchor token) becomes a splittable gap. A pair is eligible only in `view` mode and only when - * its word boundary is not a mid-phrase (straddled) boundary — the same rules the token-chip - * marker uses. A split at a former boundary dispatches the original removed default start (which - * may be leading punctuation) so the delta can normalize back to the default segmentation; - * otherwise the anchor comes from the punctuation-travel rule. The gap is keyed by the offset of - * the token the split actually lands before (the dispatched ref's own token), so the highlighted - * caret sits exactly where the boundary will fall — including a former boundary whose - * leading-punctuation ref is a few characters left of the word anchor. The `altHeld` gate is - * applied at render time, not here, so this map stays stable across Alt presses. + * anchor token) becomes a splittable gap. + * + * A pair is eligible only in `view` mode, only when the analysis is editable (a read-only one has + * no splittable gap anywhere), and only when its word boundary is not a mid-phrase (straddled) + * boundary — the same rules the token-chip marker uses. + * + * A split at a former boundary dispatches the original removed default start (which may be + * leading punctuation) so the delta can normalize back to the default segmentation; otherwise the + * anchor comes from the punctuation-travel rule. + * + * The gap is keyed by the offset of the token the split actually lands before (the dispatched + * ref's own token), so the highlighted caret sits exactly where the boundary will fall — + * including a former boundary whose leading-punctuation ref is a few characters left of the word + * anchor. + * + * The `altHeld` gate is applied at render time, not here, so this map stays stable across Alt + * presses. */ const splitGapByOffset = useMemo(() => { const map = new Map(); - if (phraseMode.kind !== 'view') return map; + if (readOnly || phraseMode.kind !== 'view') return map; const { tokens, baselineText } = segment; const tokenByRef = new Map(tokens.map((t) => [t.ref, t])); let prevWord: Token | undefined; @@ -398,7 +407,7 @@ export function SegmentView({ pendingPunct = []; }); return map; - }, [segment, phraseMode.kind, straddledBoundaryRefs, formerBoundaries]); + }, [formerBoundaries, phraseMode.kind, readOnly, segment, straddledBoundaryRefs]); /** * The ordered baseline-text render pieces: plain-text runs, inline verse superscripts, and diff --git a/src/components/modals/ProjectModals.tsx b/src/components/modals/ProjectModals.tsx index fedbfae6..54174302 100644 --- a/src/components/modals/ProjectModals.tsx +++ b/src/components/modals/ProjectModals.tsx @@ -62,7 +62,8 @@ type PendingReplace = * modal; the caller owns the import run and its modal. * @param props.onOpenImport - Called instead of the draft-open flow when the user picks a Paratext * 9 import in the select modal: the import opens read-only without touching the draft, so no - * unsaved-work confirmation is needed. + * unsaved-work confirmation is needed. Resolves once the import is open, which may be after a + * sync; the select modal stays inert until then. * @param props.openRequest - A project the caller asks to open through the normal draft-open flow, * unsaved-work confirmation included; each new `requestId` performs one open. Used to open a * freshly created editable copy. @@ -104,7 +105,7 @@ export default function ProjectModals({ project: InterlinearProjectSummary & { pt9Import: NonNullable; }, - ) => void; + ) => Promise; openRequest?: { project: InterlinearProjectSummary; requestId: number }; projectId: string; setModal: (modal: ModalState) => void; @@ -154,9 +155,10 @@ export default function ProjectModals({ const replaceGuard = useSubmitGuard(); /** - * Guards opening a project straight from the select modal — the path taken when the draft has no - * unsaved work, so no confirmation intervenes. `isSubmitting` suppresses that modal's dismissal - * while the open is in flight. An open deferred behind the confirmation has its own guard. + * Guards opening a project straight from the select modal — a Paratext 9 import, or an ordinary + * project when the draft has no unsaved work, so no confirmation intervenes. `isSubmitting` + * suppresses that modal's dismissal while the open is in flight. An open deferred behind the + * confirmation has its own guard. */ const openGuard = useSubmitGuard(); @@ -320,9 +322,12 @@ export default function ProjectModals({ // A Paratext 9 import opens read-only without touching the draft, so it needs neither the // unsaved-work confirmation nor the draft-open flow. Rebuilt with the provenance the check // just proved present, since narrowing one property does not narrow the object being passed. + // Guarded like the draft open beside it: opening an import can take a manifest probe and a + // whole sync, and an unguarded modal would stay live for the duration - long enough for a + // second choice to settle first and then be overwritten by the import arriving late. const { pt9Import } = project; if (pt9Import !== undefined) { - onOpenImport({ ...project, pt9Import }); + openGuard.runGuarded(() => onOpenImport({ ...project, pt9Import })); return; } if (hasUnsavedWork) setPendingReplace({ kind: 'open', project }); diff --git a/src/components/modals/SelectInterlinearProjectModal.tsx b/src/components/modals/SelectInterlinearProjectModal.tsx index a47c602c..5d30975e 100644 --- a/src/components/modals/SelectInterlinearProjectModal.tsx +++ b/src/components/modals/SelectInterlinearProjectModal.tsx @@ -29,11 +29,11 @@ const SELECT_INTERLINEAR_PROJECT_STRING_KEYS: `%${string}%`[] = [ * @param props.activeProjectId - ID of the project currently open as the active Save target, if * any; the matching list entry is highlighted and badged so the user can tell which project the * draft is currently working against. - * @param props.isOpening - When `true`, a project the user already chose is still being loaded into - * the draft, so the modal's controls go inert for the duration: the open completes regardless, so - * letting the modal be dismissed would read as having canceled it, and choosing another project - * would race the open already in flight. A caller may leave it `false` for an open the user - * cannot reach this modal behind. + * @param props.isOpening - When `true`, a project the user already chose is still opening, so the + * modal's controls go inert for the duration: the open completes regardless, so letting the modal + * be dismissed would read as having canceled it, and choosing another project would race the open + * already in flight. A caller may leave it `false` for an open the user cannot reach this modal + * behind. * @param props.onSelect - Called with the chosen project when the user picks an existing one. * @param props.onCreateNew - Called when the user chooses to create a new project instead. * @param props.onImportPt9 - Called when the user chooses to import the source's Paratext 9 diff --git a/src/hooks/usePt9ImportAvailability.ts b/src/hooks/usePt9ImportAvailability.ts index 6341d160..129ef10b 100644 --- a/src/hooks/usePt9ImportAvailability.ts +++ b/src/hooks/usePt9ImportAvailability.ts @@ -5,12 +5,20 @@ import type { InterlinearProjectSummary } from '../types/interlinear-project-sum /** What a probe knows so far: undetermined, found data, or found none (failures included). */ export type Pt9ProbeState = 'pending' | 'available' | 'unavailable'; +/** + * How long one probe may stay unanswered before it is treated as having found nothing. A project + * data provider that accepts the call and never responds would otherwise leave the probe pending + * for the life of the tab. + */ +const PROBE_TIMEOUT_MS = 15_000; + /** * Probes whether the source project serves Paratext 9 interlinear data, one manifest read per * enablement: `pending` until an enabled probe answers, `available` when the manifest lists any - * file, `unavailable` when it is empty or the probe fails - a source without the projectInterface - * is the common failure, and it simply has nothing to import. Each probe starts from `pending`, so - * a failing re-probe cannot leave a stale answer standing. + * file, `unavailable` when it is empty, the probe fails, or it goes unanswered for + * {@link PROBE_TIMEOUT_MS} - a source without the projectInterface is the common failure, and it + * simply has nothing to import. Each probe starts from `pending`, so a failing re-probe cannot + * leave a stale answer standing. * * @param sourceProjectId - Platform.Bible project ID to probe. * @param enabled - Whether to probe at all; while `false` the state stays `pending`. @@ -21,7 +29,21 @@ export function usePt9ImportProbe(sourceProjectId: string, enabled: boolean): Pt useEffect(() => { if (!enabled) return undefined; setState('pending'); - let ignore = false; + let settled = false; + let timer: ReturnType | undefined; + /** Takes the run's first answer - the manifest read's or the timeout's - and drops the rest. */ + const settle = (next: Exclude) => { + if (settled) return; + settled = true; + clearTimeout(timer); + setState(next); + }; + timer = setTimeout(() => { + logger.warn( + `Interlinearizer: PT9 import probe for ${sourceProjectId} went unanswered; treating the source as having nothing to import`, + ); + settle('unavailable'); + }, PROBE_TIMEOUT_MS); (async () => { try { const pdp = await papi.projectDataProviders.get( @@ -29,14 +51,15 @@ export function usePt9ImportProbe(sourceProjectId: string, enabled: boolean): Pt sourceProjectId, ); const manifest = await pdp.getPt9InterlinearManifest(); - if (!ignore) setState(Object.keys(manifest).length > 0 ? 'available' : 'unavailable'); + settle(Object.keys(manifest).length > 0 ? 'available' : 'unavailable'); } catch (e) { logger.debug(`Interlinearizer: PT9 import probe failed for ${sourceProjectId}`, e); - if (!ignore) setState('unavailable'); + settle('unavailable'); } })(); return () => { - ignore = true; + settled = true; + clearTimeout(timer); }; }, [sourceProjectId, enabled]); diff --git a/user-questions.md b/user-questions.md index 73c95f43..e6c8b434 100644 --- a/user-questions.md +++ b/user-questions.md @@ -312,8 +312,9 @@ Decisions made during development that we'd like reviewed: match the project's text"), the view header banner ("Imported from Paratext 9 - read-only - last synced {date}"), the copy dialog's prefilled name ("Copy of Paratext 9 Interlinear"), and the warning shown when a sync cannot refresh ("...couldn't be refreshed; showing the last - imported data"). Do these read right, and should "Paratext 9" be spelled out or abbreviated - anywhere? + imported data"), and what the view says in place of an import whose data cannot be read ("The + imported interlinear data could not be loaded. Try syncing from Paratext 9."). Do these read + right, and should "Paratext 9" be spelled out or abbreviated anywhere? ## First-open offer to convert Paratext 9 interlinear data From 6cf0e878899f77a6bd4636d94d2343d0b2b88f16 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Tue, 1 Sep 2026 16:56:07 -0400 Subject: [PATCH 2/6] Bound the manifest read and catch the report's Open Reading a Paratext 9 manifest now goes through one helper that gives up when the provider never answers, so no caller can wait on it forever. - The select modal is held inert for the whole of an import open, so a hung manifest read had left it with no Escape, no outside-click, and a disabled Cancel. A read that never answers is now an ordinary failure: one warning, the stored import opens, the modal comes back. - The first-open probe reads through the same helper and keeps its plain try/catch. - A rejecting fetch behind the report's Open is logged and notified rather than escaping the click handler unhandled. Co-Authored-By: Claude Opus 5 (1M context) --- .../components/InterlinearizerLoader.test.tsx | 78 ++++++++++++++++++- .../hooks/usePt9ImportAvailability.test.ts | 43 +--------- src/__tests__/utils/pt9-manifest.test.ts | 51 ++++++++++++ src/components/InterlinearizerLoader.tsx | 24 +++--- src/hooks/usePt9ImportAvailability.ts | 46 +++-------- src/utils/pt9-manifest.ts | 40 ++++++++++ 6 files changed, 195 insertions(+), 87 deletions(-) create mode 100644 src/__tests__/utils/pt9-manifest.test.ts create mode 100644 src/utils/pt9-manifest.ts diff --git a/src/__tests__/components/InterlinearizerLoader.test.tsx b/src/__tests__/components/InterlinearizerLoader.test.tsx index d9daf87a..5e797151 100644 --- a/src/__tests__/components/InterlinearizerLoader.test.tsx +++ b/src/__tests__/components/InterlinearizerLoader.test.tsx @@ -15,6 +15,7 @@ import { RECENTER_FADE_MS } from '../../components/recenter-fade'; import useInterlinearizerBookData from '../../hooks/useInterlinearizerBookData'; import useOptimisticBooleanSetting from '../../hooks/useOptimisticBooleanSetting'; import { emptyAnalysis, emptyDraft } from '../../types/empty-factories'; +import { PT9_MANIFEST_TIMEOUT_MS } from '../../utils/pt9-manifest'; import type { PhraseMode } from '../../types/phrase-mode'; import type { ViewOptions } from '../../types/view-options'; import type { SegmentationDispatch } from '../../components/SegmentationStore'; @@ -1470,6 +1471,41 @@ describe('InterlinearizerLoader', () => { ); }); + it('opens the stored import with a warning when the manifest read on open never answers', async () => { + // The select modal is held inert for the whole open, so the wait has to end for the user to + // get out of it. + jest.useFakeTimers(); + try { + mockImportCommands(); + jest.mocked(papi.notifications.send).mockResolvedValue('notification-id'); + mockPdpGet.mockResolvedValue({ + getPt9InterlinearManifest: jest.fn(() => new Promise(() => {})), + }); + await act(async () => { + renderLoader(); + }); + fireEvent.click(screen.getByTestId('tab-toolbar-project-menu')); + fireEvent.click(screen.getByTestId('select-modal-open-import')); + expect(screen.getByTestId('project-modals')).toHaveAttribute('data-modal', 'select'); + + await act(async () => { + jest.advanceTimersByTime(PT9_MANIFEST_TIMEOUT_MS); + }); + + expect(jest.mocked(papi.notifications.send)).toHaveBeenCalledWith({ + message: '%interlinearizer_warning_pt9Sync_failed%', + severity: 'warning', + }); + expect(screen.getByTestId('project-modals')).toHaveAttribute('data-modal', 'none'); + expect(screen.getByTestId('project-modals')).toHaveAttribute( + 'data-active-project-name', + 'Paratext 9 Interlinear', + ); + } finally { + jest.useRealTimers(); + } + }); + it('opens the stored import with a warning when the open-path sync fails', async () => { mockImportCommands(); mockPdpGet.mockRejectedValue(new Error('provider unavailable')); @@ -1668,7 +1704,9 @@ describe('InterlinearizerLoader', () => { await userEvent.click(screen.getByTestId('select-modal-open-import')); expect(await screen.findByTestId('pt9-import-banner')).toBeInTheDocument(); - expect(capturedInterlinearizerProps?.phraseMode).toEqual({ kind: 'view' }); + await waitFor(() => + expect(capturedInterlinearizerProps?.phraseMode).toEqual({ kind: 'view' }), + ); }); it('falls back to the platform language when the import declares no analysis language', async () => { @@ -1738,6 +1776,44 @@ describe('InterlinearizerLoader', () => { expect(screen.getByTestId('project-modals')).toHaveAttribute('data-modal', 'importPt9'); }); + it('notifies and stays on the report when the Open fetch rejects', async () => { + jest.mocked(papi.notifications.send).mockResolvedValue('notification-id'); + let imported = false; + mockSendCommand.mockImplementation(async (...args) => { + if (args[0] === 'interlinearizer.getProject' && imported) + throw new Error('storage offline'); + if (args[0] === 'interlinearizer.importPt9Project') { + imported = true; + return JSON.stringify({ + outcome: 'imported', + projectId: 'import-1', + report: IMPORT_REPORT, + }); + } + return JSON.stringify(emptyDraft(testProjectId)); + }); + await act(async () => { + renderLoader(); + }); + await userEvent.click(screen.getByTestId('tab-toolbar-project-menu')); + await userEvent.click(screen.getByTestId('select-modal-import-pt9')); + await screen.findByTestId('pt9-import-report'); + + await userEvent.click( + screen.getByRole('button', { name: '%interlinearizer_pt9ImportModal_open%' }), + ); + + expect(jest.mocked(logger.error)).toHaveBeenCalledWith( + 'Interlinearizer: failed to load the imported project for opening', + expect.any(Error), + ); + expect(jest.mocked(papi.notifications.send)).toHaveBeenCalledWith({ + message: '%interlinearizer_error_load_projects_failed%', + severity: 'error', + }); + expect(screen.getByTestId('pt9-import-report')).toBeInTheDocument(); + }); + it('notifies when the copy command returns no project', async () => { mockImportCommands({ copyJson: '{}' }); jest.mocked(papi.notifications.send).mockRejectedValue(new Error('ui offline')); diff --git a/src/__tests__/hooks/usePt9ImportAvailability.test.ts b/src/__tests__/hooks/usePt9ImportAvailability.test.ts index 5043e1e3..461599e0 100644 --- a/src/__tests__/hooks/usePt9ImportAvailability.test.ts +++ b/src/__tests__/hooks/usePt9ImportAvailability.test.ts @@ -1,7 +1,7 @@ /// -import { act, renderHook, waitFor } from '@testing-library/react'; -import papi, { logger } from '@papi/frontend'; +import { renderHook, waitFor } from '@testing-library/react'; +import papi from '@papi/frontend'; import usePt9ImportAvailability, { usePt9ImportProbe } from '../../hooks/usePt9ImportAvailability'; import { getMockedPdpGet, makeStubProject } from '../test-helpers'; @@ -118,45 +118,6 @@ describe('usePt9ImportProbe', () => { await waitFor(() => expect(result.current).toBe('unavailable')); }); - it('gives up and reports unavailable when the manifest read never answers', async () => { - jest.useFakeTimers(); - // A provider that accepts the call and never responds - the hang the timeout exists for. - mockPdpGet.mockResolvedValue({ getPt9InterlinearManifest: () => new Promise(() => {}) }); - - const { result } = renderHook(() => usePt9ImportProbe('src-project', true)); - await act(async () => { - await Promise.resolve(); - }); - expect(result.current).toBe('pending'); - - act(() => { - jest.advanceTimersByTime(15_000); - }); - - expect(result.current).toBe('unavailable'); - expect(jest.mocked(logger.warn)).toHaveBeenCalledWith( - expect.stringContaining('went unanswered'), - ); - jest.useRealTimers(); - }); - - it('keeps the answer the manifest read gave and drops the timeout behind it', async () => { - jest.useFakeTimers(); - mockManifest({ 'Lexicon.xml': 'aaaa1111' }); - - const { result } = renderHook(() => usePt9ImportProbe('src-project', true)); - await act(async () => { - await Promise.resolve(); - }); - act(() => { - jest.advanceTimersByTime(15_000); - }); - - expect(result.current).toBe('available'); - expect(jest.mocked(logger.warn)).not.toHaveBeenCalled(); - jest.useRealTimers(); - }); - it('stays pending and never probes while disabled', () => { const { result } = renderHook(() => usePt9ImportProbe('src-project', false)); diff --git a/src/__tests__/utils/pt9-manifest.test.ts b/src/__tests__/utils/pt9-manifest.test.ts new file mode 100644 index 00000000..7c7851d6 --- /dev/null +++ b/src/__tests__/utils/pt9-manifest.test.ts @@ -0,0 +1,51 @@ +/// + +import papi from '@papi/frontend'; +import { PT9_MANIFEST_TIMEOUT_MS, readPt9Manifest } from '../../utils/pt9-manifest'; +import { getMockedPdpGet } from '../test-helpers'; + +const mockPdpGet = getMockedPdpGet(papi); + +describe('readPt9Manifest', () => { + it('resolves the manifest the source project serves', async () => { + mockPdpGet.mockResolvedValue({ + getPt9InterlinearManifest: async () => ({ 'Lexicon.xml': 'aaaa1111' }), + }); + + await expect(readPt9Manifest('src-project')).resolves.toEqual({ 'Lexicon.xml': 'aaaa1111' }); + expect(mockPdpGet).toHaveBeenCalledWith('platformScripture.Pt9Interlinear', 'src-project'); + }); + + it('rejects when the source serves no Pt9Interlinear projectInterface', async () => { + mockPdpGet.mockRejectedValue(new Error('no such projectInterface')); + + await expect(readPt9Manifest('src-project')).rejects.toThrow('no such projectInterface'); + }); + + it('rejects when the read goes unanswered, so a caller behind blocking UI can finish', async () => { + jest.useFakeTimers(); + // A provider that accepts the call and never responds - the hang the timeout exists for. + mockPdpGet.mockResolvedValue({ getPt9InterlinearManifest: () => new Promise(() => {}) }); + + const read = readPt9Manifest('src-project'); + const settled = jest.fn(); + read.catch(settled); + await Promise.resolve(); + expect(settled).not.toHaveBeenCalled(); + + jest.advanceTimersByTime(PT9_MANIFEST_TIMEOUT_MS); + + await expect(read).rejects.toThrow('went unanswered'); + jest.useRealTimers(); + }); + + it('leaves no timer pending once the read answers', async () => { + jest.useFakeTimers(); + mockPdpGet.mockResolvedValue({ getPt9InterlinearManifest: async () => ({}) }); + + await expect(readPt9Manifest('src-project')).resolves.toEqual({}); + + expect(jest.getTimerCount()).toBe(0); + jest.useRealTimers(); + }); +}); diff --git a/src/components/InterlinearizerLoader.tsx b/src/components/InterlinearizerLoader.tsx index 24275ddd..6d0d7611 100644 --- a/src/components/InterlinearizerLoader.tsx +++ b/src/components/InterlinearizerLoader.tsx @@ -51,6 +51,7 @@ import { firstVerseNumber, segmentContainsVerse } from '../utils/verse-ref'; import { resolvedOrEmpty } from '../utils/localized-strings'; import usePanelResizeKeys from '../hooks/usePanelResizeKeys'; import { isPt9TooLargeError } from '../utils/pt9-import-error'; +import { readPt9Manifest } from '../utils/pt9-manifest'; /** Host-injected callback to update this WebView's definition (used to toggle the tab title). */ type UpdateWebViewDefinition = WebViewProps['updateWebViewDefinition']; @@ -702,17 +703,14 @@ function InterlinearizerLoaderInner({ /** * Opens a Paratext 9 import from the select modal: probes the manifest and, when the source files * changed since the last import, syncs first behind the import modal's running state - closing - * straight into the view, with no report step on the open path. Every failure opens the stored - * (stale) import with one warning instead of blocking access to it. + * straight into the view, with no report step on the open path. Every failure - a manifest read + * that never answers included - opens the stored (stale) import with one warning instead of + * blocking access to it, which also releases the select modal the open is holding inert. */ const openImportedProject = useCallback( async (project: InterlinearProjectSummary & { pt9Import: Pt9ImportProvenance }) => { try { - const pdp = await papi.projectDataProviders.get( - 'platformScripture.Pt9Interlinear', - projectId, - ); - const manifest = await pdp.getPt9InterlinearManifest(); + const manifest = await readPt9Manifest(projectId); if (fileHashesEqual(manifest, project.pt9Import.fileHashes)) { setActiveProject(project); setModal('none'); @@ -747,11 +745,19 @@ function InterlinearizerLoaderInner({ [projectId, fetchSummary, setActiveProject], ); - /** Opens the freshly imported project from the report into the read-only view. */ + /** + * Opens the freshly imported project from the report into the read-only view. A fetch that fails + * leaves the report standing, so the Open can be taken again once whatever broke is fixed. + */ const handlePt9Open = useCallback(async () => { /* v8 ignore next -- Open only renders on a report, which always sets the imported id first */ if (!pt9ImportedId) return; - const summary = await fetchSummary(pt9ImportedId); + let summary: InterlinearProjectSummary | undefined; + try { + summary = await fetchSummary(pt9ImportedId); + } catch (e) { + logger.error('Interlinearizer: failed to load the imported project for opening', e); + } if (summary) { setActiveProject(summary); setModal('none'); diff --git a/src/hooks/usePt9ImportAvailability.ts b/src/hooks/usePt9ImportAvailability.ts index 129ef10b..47174cbb 100644 --- a/src/hooks/usePt9ImportAvailability.ts +++ b/src/hooks/usePt9ImportAvailability.ts @@ -1,24 +1,17 @@ -import papi, { logger } from '@papi/frontend'; +import { logger } from '@papi/frontend'; import { useEffect, useState } from 'react'; import type { InterlinearProjectSummary } from '../types/interlinear-project-summary'; +import { readPt9Manifest } from '../utils/pt9-manifest'; /** What a probe knows so far: undetermined, found data, or found none (failures included). */ export type Pt9ProbeState = 'pending' | 'available' | 'unavailable'; -/** - * How long one probe may stay unanswered before it is treated as having found nothing. A project - * data provider that accepts the call and never responds would otherwise leave the probe pending - * for the life of the tab. - */ -const PROBE_TIMEOUT_MS = 15_000; - /** * Probes whether the source project serves Paratext 9 interlinear data, one manifest read per * enablement: `pending` until an enabled probe answers, `available` when the manifest lists any - * file, `unavailable` when it is empty, the probe fails, or it goes unanswered for - * {@link PROBE_TIMEOUT_MS} - a source without the projectInterface is the common failure, and it - * simply has nothing to import. Each probe starts from `pending`, so a failing re-probe cannot - * leave a stale answer standing. + * file, `unavailable` when it is empty or the read fails - a source without the projectInterface is + * the common failure, and it simply has nothing to import. Each probe starts from `pending`, so a + * failing re-probe cannot leave a stale answer standing. * * @param sourceProjectId - Platform.Bible project ID to probe. * @param enabled - Whether to probe at all; while `false` the state stays `pending`. @@ -29,37 +22,18 @@ export function usePt9ImportProbe(sourceProjectId: string, enabled: boolean): Pt useEffect(() => { if (!enabled) return undefined; setState('pending'); - let settled = false; - let timer: ReturnType | undefined; - /** Takes the run's first answer - the manifest read's or the timeout's - and drops the rest. */ - const settle = (next: Exclude) => { - if (settled) return; - settled = true; - clearTimeout(timer); - setState(next); - }; - timer = setTimeout(() => { - logger.warn( - `Interlinearizer: PT9 import probe for ${sourceProjectId} went unanswered; treating the source as having nothing to import`, - ); - settle('unavailable'); - }, PROBE_TIMEOUT_MS); + let ignore = false; (async () => { try { - const pdp = await papi.projectDataProviders.get( - 'platformScripture.Pt9Interlinear', - sourceProjectId, - ); - const manifest = await pdp.getPt9InterlinearManifest(); - settle(Object.keys(manifest).length > 0 ? 'available' : 'unavailable'); + const manifest = await readPt9Manifest(sourceProjectId); + if (!ignore) setState(Object.keys(manifest).length > 0 ? 'available' : 'unavailable'); } catch (e) { logger.debug(`Interlinearizer: PT9 import probe failed for ${sourceProjectId}`, e); - settle('unavailable'); + if (!ignore) setState('unavailable'); } })(); return () => { - settled = true; - clearTimeout(timer); + ignore = true; }; }, [sourceProjectId, enabled]); diff --git a/src/utils/pt9-manifest.ts b/src/utils/pt9-manifest.ts new file mode 100644 index 00000000..a525b901 --- /dev/null +++ b/src/utils/pt9-manifest.ts @@ -0,0 +1,40 @@ +import papi from '@papi/frontend'; + +/** + * How long a manifest read may go unanswered before {@link readPt9Manifest} gives up on it. A + * project data provider that accepts the call and never responds would otherwise leave its caller + * waiting for the life of the tab. + */ +export const PT9_MANIFEST_TIMEOUT_MS = 15_000; + +/** + * Reads the source project's Paratext 9 interlinear manifest: every interlinear file it serves, by + * path, with the hash an import compares against to tell whether the source has changed. An empty + * manifest means the source serves no interlinear data at all. + * + * @throws If the source has no `platformScripture.Pt9Interlinear` projectInterface, the read + * rejects, or it goes unanswered for {@link PT9_MANIFEST_TIMEOUT_MS}. A caller that waits on this + * behind blocking UI can therefore always finish. + */ +export function readPt9Manifest(sourceProjectId: string): Promise> { + let timer: ReturnType | undefined; + const timeout = new Promise((_resolve, reject) => { + timer = setTimeout( + () => + reject( + new Error( + `Paratext 9 interlinear manifest read for ${sourceProjectId} went unanswered after ${PT9_MANIFEST_TIMEOUT_MS}ms`, + ), + ), + PT9_MANIFEST_TIMEOUT_MS, + ); + }); + const read = (async () => { + const pdp = await papi.projectDataProviders.get( + 'platformScripture.Pt9Interlinear', + sourceProjectId, + ); + return pdp.getPt9InterlinearManifest(); + })(); + return Promise.race([read, timeout]).finally(() => clearTimeout(timer)); +} From 91852d87e60f6864620592fec44a7c686653080d Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Tue, 1 Sep 2026 17:11:46 -0400 Subject: [PATCH 3/6] Tag the import analysis with the version it was fetched for The view derives what to show from the tag rather than having an effect clear the previous analysis: a fetched analysis and the version it belongs to now reach the view in the same commit, so the commit that carries a sync's new modification time has no pre-sync analysis to paint - previously it mounted the whole interlinear tree on the old content for a frame before the placeholder replaced it. Also records that the manifest timeout bounds the wait rather than the read, PAPI offering no cancellation. Co-Authored-By: Claude Opus 5 (1M context) --- src/components/InterlinearizerLoader.tsx | 58 +++++++++++++----------- src/utils/pt9-manifest.ts | 4 +- 2 files changed, 34 insertions(+), 28 deletions(-) diff --git a/src/components/InterlinearizerLoader.tsx b/src/components/InterlinearizerLoader.tsx index 6d0d7611..1193e8d3 100644 --- a/src/components/InterlinearizerLoader.tsx +++ b/src/components/InterlinearizerLoader.tsx @@ -307,31 +307,26 @@ function InterlinearizerLoaderInner({ const isImportView = activeProject?.pt9Import !== undefined; /** - * The import's stored analysis, or `undefined` while a fetch is in flight and after one failed. - * Fetched fresh whenever the import view opens or a sync bumps the project's `updatedAt`. Never - * the draft: the user's in-progress draft survives viewing an import untouched. + * Which version of the import the view is on: its id and the modification time a sync bumps. + * `undefined` while the draft is the view. */ - const [importAnalysis, setImportAnalysis] = useState(undefined); + const importTag = + isImportView && activeProject ? `${activeProject.id}:${activeProject.updatedAt}` : undefined; - /** Whether the import's analysis could not be read; the view area says so in place of it. */ - const [importLoadFailed, setImportLoadFailed] = useState(false); + /** + * The import analysis last fetched, under the version tag it was fetched for; no `analysis` when + * that fetch found none to show. Tagged rather than cleared as the version changes, so a fetched + * analysis and the version it belongs to always reach the view in the same commit - an analysis + * the sync has already replaced is never one the view can paint. + */ + const [importLoad, setImportLoad] = useState<{ tag: string; analysis?: TextAnalysis }>(); useEffect(() => { - setImportLoadFailed(false); - if (!isImportView || !activeProject) { - setImportAnalysis(undefined); - return undefined; - } - // The store below seeds on mount alone and remounts on the `updatedAt` a sync bumps, taking - // whichever analysis is held at that commit. Clearing first keeps a pre-sync one from being - // pinned in the view for as long as it stays open. - setImportAnalysis(undefined); + if (importTag === undefined || !activeProject) return undefined; + const { id } = activeProject; let ignore = false; (async () => { try { - const json = await papi.commands.sendCommand( - 'interlinearizer.getProject', - activeProject.id, - ); + const json = await papi.commands.sendCommand('interlinearizer.getProject', id); const parsed: unknown = json ? JSON.parse(json) : undefined; const analysis = parsed && typeof parsed === 'object' && 'analysis' in parsed @@ -339,16 +334,16 @@ function InterlinearizerLoaderInner({ : undefined; if (ignore) return; if (isTextAnalysis(analysis)) { - setImportAnalysis(analysis); + setImportLoad({ tag: importTag, analysis }); } else { - setImportLoadFailed(true); + setImportLoad({ tag: importTag }); await papi.notifications .send({ message: '%interlinearizer_error_load_projects_failed%', severity: 'error' }) .catch(() => {}); } } catch (e) { logger.error('Interlinearizer: failed to load the imported analysis', e); - if (!ignore) setImportLoadFailed(true); + if (!ignore) setImportLoad({ tag: importTag }); await papi.notifications .send({ message: '%interlinearizer_error_load_projects_failed%', severity: 'error' }) .catch(() => {}); @@ -357,8 +352,17 @@ function InterlinearizerLoaderInner({ return () => { ignore = true; }; - // eslint-disable-next-line react-hooks/exhaustive-deps -- updatedAt stands in for the analysis a sync replaced - }, [isImportView, activeProject?.id, activeProject?.updatedAt]); + // eslint-disable-next-line react-hooks/exhaustive-deps -- the tag names the version to fetch; the project object changes for edits that leave it alone + }, [importTag]); + + /** The fetch's outcome for the version on screen, or `undefined` while one is still in flight. */ + const importLoaded = importLoad?.tag === importTag ? importLoad : undefined; + + /** The analysis the import view renders; `undefined` until this version's fetch has landed one. */ + const importAnalysis = importLoaded?.analysis; + + /** Whether this version's analysis could not be read; the view area says so in place of it. */ + const importLoadFailed = importLoaded !== undefined && importLoaded.analysis === undefined; // Whether any gloss input currently holds uncommitted text. Gloss writes are deferred to blur, so // the persisted `dirty` flag does not flip until then; tracking in-progress edits here lets the @@ -1157,10 +1161,10 @@ function InterlinearizerLoaderInner({ importAnalysis === undefined ? ( {loadingOrErrorPanel} ) : ( - // Keyed on id + updatedAt so a sync (which bumps updatedAt) reseeds by remounting, the - // same non-reactive-seed contract the draft-backed store relies on. + // Keyed on the version tag so a sync reseeds by remounting, the same non-reactive-seed + // contract the draft-backed store relies on. > { let timer: ReturnType | undefined; From 3dd56e1e5608f0a30d85ea117c9f91b629a04856 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Tue, 1 Sep 2026 17:13:48 -0400 Subject: [PATCH 4/6] Trim the import-analysis comments to what outlives the code Co-Authored-By: Claude Opus 5 (1M context) --- src/components/InterlinearizerLoader.tsx | 14 ++++++-------- src/utils/pt9-manifest.ts | 7 +++---- 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/src/components/InterlinearizerLoader.tsx b/src/components/InterlinearizerLoader.tsx index 1193e8d3..2544906e 100644 --- a/src/components/InterlinearizerLoader.tsx +++ b/src/components/InterlinearizerLoader.tsx @@ -315,9 +315,8 @@ function InterlinearizerLoaderInner({ /** * The import analysis last fetched, under the version tag it was fetched for; no `analysis` when - * that fetch found none to show. Tagged rather than cleared as the version changes, so a fetched - * analysis and the version it belongs to always reach the view in the same commit - an analysis - * the sync has already replaced is never one the view can paint. + * that fetch found none to show. An analysis the sync has already replaced is therefore never one + * the view can paint. */ const [importLoad, setImportLoad] = useState<{ tag: string; analysis?: TextAnalysis }>(); useEffect(() => { @@ -352,16 +351,15 @@ function InterlinearizerLoaderInner({ return () => { ignore = true; }; - // eslint-disable-next-line react-hooks/exhaustive-deps -- the tag names the version to fetch; the project object changes for edits that leave it alone + // eslint-disable-next-line react-hooks/exhaustive-deps -- the tag names the version to fetch; the project object also changes for edits that leave the analysis alone }, [importTag]); - /** The fetch's outcome for the version on screen, or `undefined` while one is still in flight. */ + /** The fetch's outcome for the version on screen; `undefined` until that version has one. */ const importLoaded = importLoad?.tag === importTag ? importLoad : undefined; - /** The analysis the import view renders; `undefined` until this version's fetch has landed one. */ const importAnalysis = importLoaded?.analysis; - /** Whether this version's analysis could not be read; the view area says so in place of it. */ + /** Whether the version on screen is one whose analysis could not be read. */ const importLoadFailed = importLoaded !== undefined && importLoaded.analysis === undefined; // Whether any gloss input currently holds uncommitted text. Gloss writes are deferred to blur, so @@ -709,7 +707,7 @@ function InterlinearizerLoaderInner({ * changed since the last import, syncs first behind the import modal's running state - closing * straight into the view, with no report step on the open path. Every failure - a manifest read * that never answers included - opens the stored (stale) import with one warning instead of - * blocking access to it, which also releases the select modal the open is holding inert. + * blocking access to it. */ const openImportedProject = useCallback( async (project: InterlinearProjectSummary & { pt9Import: Pt9ImportProvenance }) => { diff --git a/src/utils/pt9-manifest.ts b/src/utils/pt9-manifest.ts index 9e28040a..73af0865 100644 --- a/src/utils/pt9-manifest.ts +++ b/src/utils/pt9-manifest.ts @@ -13,10 +13,9 @@ export const PT9_MANIFEST_TIMEOUT_MS = 15_000; * manifest means the source serves no interlinear data at all. * * @throws If the source has no `platformScripture.Pt9Interlinear` projectInterface, the read - * rejects, or it goes unanswered for {@link PT9_MANIFEST_TIMEOUT_MS}. A caller that waits on this - * behind blocking UI can therefore always finish. The timeout bounds the wait, not the read: PAPI - * offers no cancellation, so an unanswered read stays outstanding and its late result is - * dropped. + * rejects, or it goes unanswered for {@link PT9_MANIFEST_TIMEOUT_MS}. That timeout bounds the + * wait, not the read: PAPI offers no cancellation, so an unanswered read stays outstanding and + * its late result is dropped. */ export function readPt9Manifest(sourceProjectId: string): Promise> { let timer: ReturnType | undefined; From ddde7383332b51254bbdcdc06d5390cc2d285d6f Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Wed, 2 Sep 2026 12:36:12 -0400 Subject: [PATCH 5/6] Report a failed import load once, in the panel The imported-analysis fetch sent a toast on top of the panel's own failure line, so one failure produced two messages with different advice - and the toast in the catch ran even for a fetch a sync or a switch back to the draft had already superseded. The panel line is now the whole report: it stays on screen next to the empty view instead of disappearing. Also seals two ways a stale mode or hover could outlive the control it came from: the import view pins its phrase mode to view, since the reset effect only covers crossing into the import and a mode set from inside it has no crossing to reset it; and a split hover clears when the analysis turns read-only, since the button that vanishes never fires its own mouse-leave. The read-only mock boilerplate that had been copied into three test files moves to a module of its own, and resets in beforeEach rather than afterEach. Co-Authored-By: Claude Opus 5 (1M context) --- .../analysis-store-read-only-mock.ts | 31 ++++++++ src/__tests__/components/ArcOverlay.test.tsx | 75 +++++++++++++++---- .../components/InterlinearizerLoader.test.tsx | 46 +++++++++--- src/__tests__/components/MorphemeBox.test.tsx | 17 +---- src/__tests__/components/TokenChip.test.tsx | 17 +---- src/components/ArcOverlay.tsx | 11 ++- src/components/InterlinearizerLoader.tsx | 25 +++---- 7 files changed, 149 insertions(+), 73 deletions(-) create mode 100644 src/__tests__/analysis-store-read-only-mock.ts diff --git a/src/__tests__/analysis-store-read-only-mock.ts b/src/__tests__/analysis-store-read-only-mock.ts new file mode 100644 index 00000000..96b7a55a --- /dev/null +++ b/src/__tests__/analysis-store-read-only-mock.ts @@ -0,0 +1,31 @@ +/** + * Shared access to the manual `AnalysisStore` mock's read-only switch, for the several test files + * that render components under `useAnalysisReadOnly`. + * + * This lives apart from `test-helpers` on purpose: that module imports the real + * `AnalysisStoreProvider`, so in a file that mocks `AnalysisStore` its `withAnalysisStore` would + * silently render the mock's provider instead. + */ + +/** The manual AnalysisStore mock's test-only controls. */ +interface AnalysisStoreReadOnlyMock { + __setMockAnalysisReadOnly: (value: boolean) => void; +} + +function isAnalysisStoreReadOnlyMock(m: unknown): m is AnalysisStoreReadOnlyMock { + return !!m && typeof m === 'object' && '__setMockAnalysisReadOnly' in m; +} + +/** + * Sets what the mocked `useAnalysisReadOnly` returns. Resolves the mock on each call rather than at + * import time, so importing this module never depends on `jest.mock` having run first. + * + * @param value Whether the mocked store reports the analysis as read-only. + */ +export function setMockAnalysisReadOnly(value: boolean): void { + const analysisStoreMock: unknown = jest.requireMock('../components/AnalysisStore'); + if (!isAnalysisStoreReadOnlyMock(analysisStoreMock)) + throw new Error('Expected the AnalysisStore manual mock with read-only controls'); + const { __setMockAnalysisReadOnly: setReadOnly } = analysisStoreMock; + setReadOnly(value); +} diff --git a/src/__tests__/components/ArcOverlay.test.tsx b/src/__tests__/components/ArcOverlay.test.tsx index 7874d8ec..8e16a5bf 100644 --- a/src/__tests__/components/ArcOverlay.test.tsx +++ b/src/__tests__/components/ArcOverlay.test.tsx @@ -5,26 +5,13 @@ import { render, screen } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import { ArcOverlay } from '../../components/ArcOverlay'; import type { ArcPath } from '../../utils/phrase-arc'; +import { setMockAnalysisReadOnly } from '../analysis-store-read-only-mock'; import { makePhraseLink } from '../test-helpers'; import { withTooltipProvider } from './test-helpers'; jest.mock('../../components/AnalysisStore'); -/** The manual AnalysisStore mock's test-only controls. */ -interface AnalysisStoreReadOnlyMock { - __setMockAnalysisReadOnly: (value: boolean) => void; -} - -function isAnalysisStoreReadOnlyMock(m: unknown): m is AnalysisStoreReadOnlyMock { - return !!m && typeof m === 'object' && '__setMockAnalysisReadOnly' in m; -} - -const analysisStoreMock: unknown = jest.requireMock('../../components/AnalysisStore'); -if (!isAnalysisStoreReadOnlyMock(analysisStoreMock)) - throw new Error('Expected the AnalysisStore manual mock with read-only controls'); -const { __setMockAnalysisReadOnly: setMockAnalysisReadOnly } = analysisStoreMock; - -afterEach(() => { +beforeEach(() => { setMockAnalysisReadOnly(false); }); @@ -65,7 +52,13 @@ function requiredProps(): Parameters[0] { * require. */ function renderOverlay(overrides: Partial[0]> = {}) { - return render(withTooltipProvider()); + const props = { ...requiredProps(), ...overrides }; + const result = render(withTooltipProvider()); + return { + ...result, + /** Re-renders with the same props, for a test that changed what the mocked store reports. */ + rerenderOverlay: () => result.rerender(withTooltipProvider()), + }; } describe('ArcOverlay', () => { @@ -255,6 +248,56 @@ describe('ArcOverlay', () => { expect(onSplitHoverChange).toHaveBeenLastCalledWith(new Set()); }); + it('clears the freed-token preview when the analysis turns read-only mid-hover', async () => { + const onSplitHoverChange = jest.fn(); + const phraseLink = makePhraseLink('p1', ['tok-a', 'tok-b']); + const { rerenderOverlay } = renderOverlay({ + arcPaths: [makeArcPath('p1', 'tok-a')], + hoveredPhraseId: 'p1', + phraseLinkById: new Map([['p1', phraseLink]]), + tokenDocOrder: new Map([ + ['tok-a', 0], + ['tok-b', 1], + ]), + onSplitHoverChange, + }); + await userEvent.hover(screen.getByTestId('split-arc-btn')); + expect(onSplitHoverChange).toHaveBeenLastCalledWith(new Set(['tok-a', 'tok-b'])); + + // The button vanishes with the mouse still over it, so no mouse-leave of its own ever fires. + setMockAnalysisReadOnly(true); + rerenderOverlay(); + + expect(screen.queryByTestId('split-arc-btn')).not.toBeInTheDocument(); + expect(onSplitHoverChange).toHaveBeenLastCalledWith(new Set()); + }); + + it('clears the phrase highlight when the analysis turns read-only mid-reshape-hover', async () => { + const onHoverPhrase = jest.fn(); + // Four-token phrase: splitting after tok-b leaves both halves ≥ 2, the reshape preview. + const phraseLink = makePhraseLink('p1', ['tok-a', 'tok-b', 'tok-c', 'tok-d']); + const { rerenderOverlay } = renderOverlay({ + arcPaths: [makeArcPath('p1', 'tok-b')], + hoveredPhraseId: 'p1', + phraseLinkById: new Map([['p1', phraseLink]]), + tokenDocOrder: new Map([ + ['tok-a', 0], + ['tok-b', 1], + ['tok-c', 2], + ['tok-d', 3], + ]), + onHoverPhrase, + }); + await userEvent.hover(screen.getByTestId('split-arc-btn')); + expect(onHoverPhrase).toHaveBeenLastCalledWith('p1'); + + setMockAnalysisReadOnly(true); + rerenderOverlay(); + + expect(screen.queryByTestId('split-arc-btn')).not.toBeInTheDocument(); + expect(onHoverPhrase).toHaveBeenLastCalledWith(undefined); + }); + it('does not call onSplitHoverChange with free refs on enter when no token would become free (both halves ≥ 2)', async () => { const onSplitHoverChange = jest.fn(); // Four-token phrase: splitting after tok-b gives before=[tok-a,tok-b] and after=[tok-c,tok-d], both ≥ 2. diff --git a/src/__tests__/components/InterlinearizerLoader.test.tsx b/src/__tests__/components/InterlinearizerLoader.test.tsx index 5e797151..36e5f6ab 100644 --- a/src/__tests__/components/InterlinearizerLoader.test.tsx +++ b/src/__tests__/components/InterlinearizerLoader.test.tsx @@ -1666,6 +1666,11 @@ describe('InterlinearizerLoader', () => { '%interlinearizer_error_pt9Import_load_failed%', ); expect(screen.getByTestId('pt9-import-banner')).toBeInTheDocument(); + // The panel line is the whole message: no toast doubles it with different advice. + expect(jest.mocked(papi.notifications.send)).not.toHaveBeenCalledWith({ + message: '%interlinearizer_error_load_projects_failed%', + severity: 'error', + }); }); it('hands the import view a segmentation dispatch that cannot write the draft', async () => { @@ -1709,6 +1714,23 @@ describe('InterlinearizerLoader', () => { ); }); + it('keeps the import view in view mode however the phrase mode is set under it', async () => { + mockImportCommands(); + await renderImportView(); + + act(() => { + capturedInterlinearizerProps?.setPhraseMode({ + kind: 'edit', + phraseId: 'phrase-1', + originalTokens: [{ tokenRef: 'GEN 1:1:0', surfaceText: 'In' }], + }); + }); + + // The reset effect only covers crossing into the import; a mode set from inside it would + // otherwise stand, since nothing crosses back. + expect(capturedInterlinearizerProps?.phraseMode).toEqual({ kind: 'view' }); + }); + it('falls back to the platform language when the import declares no analysis language', async () => { mockImportCommands(); await act(async () => @@ -1723,8 +1745,8 @@ describe('InterlinearizerLoader', () => { expect(screen.getByTestId('interlinearizer')).toBeInTheDocument(); }); - it('notifies when the imported analysis fetch rejects', async () => { - jest.mocked(papi.notifications.send).mockRejectedValue(new Error('ui offline')); + it('reports a rejected imported-analysis fetch in the panel and sends no toast', async () => { + jest.mocked(papi.notifications.send).mockResolvedValue('notification-id'); mockSendCommand.mockImplementation(async (...args) => { if (args[0] === 'interlinearizer.getProject') throw new Error('storage offline'); return JSON.stringify(emptyDraft(testProjectId)); @@ -1733,10 +1755,10 @@ describe('InterlinearizerLoader', () => { renderLoader({ useWebViewState: makeWebViewState({ activeProject: STUB_IMPORT_PROJECT }) }), ); - expect(jest.mocked(papi.notifications.send)).toHaveBeenCalledWith({ - message: '%interlinearizer_error_load_projects_failed%', - severity: 'error', - }); + expect(screen.getByTestId('pt9-import-load-error')).toHaveTextContent( + '%interlinearizer_error_pt9Import_load_failed%', + ); + expect(jest.mocked(papi.notifications.send)).not.toHaveBeenCalled(); }); it('shows the in-modal failure when the import result carries no valid report', async () => { @@ -1899,8 +1921,8 @@ describe('InterlinearizerLoader', () => { ); }); - it('notifies when the imported analysis fails to load', async () => { - jest.mocked(papi.notifications.send).mockRejectedValue(new Error('ui offline')); + it('reports an imported analysis that never loads in the panel and sends no toast', async () => { + jest.mocked(papi.notifications.send).mockResolvedValue('notification-id'); // An empty response is the never-written case; the effect treats it like a malformed one. mockSendCommand.mockImplementation(async (...args) => args[0] === 'interlinearizer.getProject' ? '' : JSON.stringify(emptyDraft(testProjectId)), @@ -1909,10 +1931,10 @@ describe('InterlinearizerLoader', () => { renderLoader({ useWebViewState: makeWebViewState({ activeProject: STUB_IMPORT_PROJECT }) }), ); - expect(jest.mocked(papi.notifications.send)).toHaveBeenCalledWith({ - message: '%interlinearizer_error_load_projects_failed%', - severity: 'error', - }); + expect(screen.getByTestId('pt9-import-load-error')).toHaveTextContent( + '%interlinearizer_error_pt9Import_load_failed%', + ); + expect(jest.mocked(papi.notifications.send)).not.toHaveBeenCalled(); }); }); diff --git a/src/__tests__/components/MorphemeBox.test.tsx b/src/__tests__/components/MorphemeBox.test.tsx index 535cc9cf..bc148919 100644 --- a/src/__tests__/components/MorphemeBox.test.tsx +++ b/src/__tests__/components/MorphemeBox.test.tsx @@ -7,6 +7,7 @@ import type { MorphemeAnalysis } from 'interlinearizer'; import * as AnalysisStore from '../../components/AnalysisStore'; import { MorphemeBox, MorphemeGlossInput } from '../../components/MorphemeBox'; import { TOKEN_CHIP_LABEL_KEYS } from '../../components/PhraseStripContext'; +import { setMockAnalysisReadOnly } from '../analysis-store-read-only-mock'; import { makeWordToken } from '../test-helpers'; jest.mock('../../components/AnalysisStore'); @@ -324,22 +325,8 @@ describe('MorphemeGlossInput', () => { }); }); -/** The manual AnalysisStore mock's test-only controls. */ -interface AnalysisStoreReadOnlyMock { - __setMockAnalysisReadOnly: (value: boolean) => void; -} - -function isAnalysisStoreReadOnlyMock(m: unknown): m is AnalysisStoreReadOnlyMock { - return !!m && typeof m === 'object' && '__setMockAnalysisReadOnly' in m; -} - -const analysisStoreMock: unknown = jest.requireMock('../../components/AnalysisStore'); -if (!isAnalysisStoreReadOnlyMock(analysisStoreMock)) - throw new Error('Expected the AnalysisStore manual mock with read-only controls'); -const { __setMockAnalysisReadOnly: setMockAnalysisReadOnly } = analysisStoreMock; - describe('MorphemeBox read-only', () => { - afterEach(() => { + beforeEach(() => { setMockAnalysisReadOnly(false); }); diff --git a/src/__tests__/components/TokenChip.test.tsx b/src/__tests__/components/TokenChip.test.tsx index 8d5e6e30..92d2e15a 100644 --- a/src/__tests__/components/TokenChip.test.tsx +++ b/src/__tests__/components/TokenChip.test.tsx @@ -9,6 +9,7 @@ import * as AnalysisStore from '../../components/AnalysisStore'; import { AnalysisStoreProvider } from '../../components/AnalysisStore'; import { InertTokenChip, TokenChip } from '../../components/TokenChip'; import { emptyAnalysis } from '../../types/empty-factories'; +import { setMockAnalysisReadOnly } from '../analysis-store-read-only-mock'; import { FIXTURE_STAMPS, makePunctToken, makeWordToken } from '../test-helpers'; import { mockKeyAsValueLocalizedStrings } from './test-helpers'; @@ -788,22 +789,8 @@ describe('TokenChip', () => { }); }); -/** The manual AnalysisStore mock's test-only controls. */ -interface AnalysisStoreReadOnlyMock { - __setMockAnalysisReadOnly: (value: boolean) => void; -} - -function isAnalysisStoreReadOnlyMock(m: unknown): m is AnalysisStoreReadOnlyMock { - return !!m && typeof m === 'object' && '__setMockAnalysisReadOnly' in m; -} - -const analysisStoreMock: unknown = jest.requireMock('../../components/AnalysisStore'); -if (!isAnalysisStoreReadOnlyMock(analysisStoreMock)) - throw new Error('Expected the AnalysisStore manual mock with read-only controls'); -const { __setMockAnalysisReadOnly: setMockAnalysisReadOnly } = analysisStoreMock; - describe('TokenChip read-only', () => { - afterEach(() => { + beforeEach(() => { setMockAnalysisReadOnly(false); }); diff --git a/src/components/ArcOverlay.tsx b/src/components/ArcOverlay.tsx index 0c9e7626..6793f346 100644 --- a/src/components/ArcOverlay.tsx +++ b/src/components/ArcOverlay.tsx @@ -1,7 +1,7 @@ import type { PhraseAnalysisLink } from 'interlinearizer'; import { Link2Off } from 'lucide-react'; import { Button, Tooltip, TooltipContent, TooltipTrigger } from 'platform-bible-react'; -import { memo, useState, useCallback } from 'react'; +import { memo, useState, useCallback, useEffect } from 'react'; import type { PhraseMode } from '../types/phrase-mode'; import { resolvedOrEmpty, tooltipContentOrUndefined } from '../utils/localized-strings'; import { computeSplitFreeRefs, getArcStrokeProps, type ArcPath } from '../utils/phrase-arc'; @@ -174,6 +174,15 @@ export function ArcOverlay({ onHoverPhrase(undefined); }, [onHoverPhrase]); + // A split button that goes away because the analysis turned read-only never fires its own + // mouse-leave, so whatever preview the hover put up - freed tokens dimmed, or the whole phrase + // highlighted - would stay on a view that no longer offers the split. Take it down here. + useEffect(() => { + if (!readOnly || splitHoveredArc === undefined) return; + if (splitHoveredArc.kind === 'free') handleSplitHoverLeave(); + else handleReshapeHoverLeave(); + }, [readOnly, splitHoveredArc, handleSplitHoverLeave, handleReshapeHoverLeave]); + if (arcPaths.length === 0) return undefined; /** diff --git a/src/components/InterlinearizerLoader.tsx b/src/components/InterlinearizerLoader.tsx index 2544906e..e43aa929 100644 --- a/src/components/InterlinearizerLoader.tsx +++ b/src/components/InterlinearizerLoader.tsx @@ -169,6 +169,9 @@ const STRING_KEYS = [ */ const PT9_CHECKING_DELAY_MS = 400; +/** The phrase mode a read-only view is always in, shared so its identity stays stable. */ +const VIEW_PHRASE_MODE: PhraseMode = { kind: 'view' }; + /** The provenance an import project carries; the open-import path requires it present. */ type Pt9ImportProvenance = NonNullable; @@ -332,20 +335,12 @@ function InterlinearizerLoaderInner({ ? parsed.analysis : undefined; if (ignore) return; - if (isTextAnalysis(analysis)) { - setImportLoad({ tag: importTag, analysis }); - } else { - setImportLoad({ tag: importTag }); - await papi.notifications - .send({ message: '%interlinearizer_error_load_projects_failed%', severity: 'error' }) - .catch(() => {}); - } + // Either outcome is reported by the panel's own line rather than a toast: it stays on + // screen next to the empty view, and there is only one message to reconcile. + setImportLoad(isTextAnalysis(analysis) ? { tag: importTag, analysis } : { tag: importTag }); } catch (e) { logger.error('Interlinearizer: failed to load the imported analysis', e); if (!ignore) setImportLoad({ tag: importTag }); - await papi.notifications - .send({ message: '%interlinearizer_error_load_projects_failed%', severity: 'error' }) - .catch(() => {}); } })(); return () => { @@ -608,14 +603,16 @@ function InterlinearizerLoaderInner({ /** Whether the destructive wipe dialog (book / whole-draft scope picker) is open. */ const [wipeModalOpen, setWipeModalOpen] = useState(false); - const [phraseMode, setPhraseMode] = useState({ kind: 'view' }); + const [phraseMode, setPhraseMode] = useState(VIEW_PHRASE_MODE); // Reset phraseMode whenever the draft is replaced wholesale (New / Open / Wipe), and whenever the // view crosses between the draft and an import, so stale edit/confirm-unlink state is never // passed to the newly mounted Interlinearizer. An import opens without touching the draft or its // version, and a mode carried into that read-only view renders its edit-target affordances. + // Crossing into the import is all this covers; the render below pins the import view's mode + // outright, since a mode set from inside that view has no crossing to reset it. useEffect(() => { - setPhraseMode({ kind: 'view' }); + setPhraseMode(VIEW_PHRASE_MODE); }, [draftVersion, isImportView]); /** What the Paratext 9 import modal shows while `modal` is `'importPt9'`. */ @@ -1096,7 +1093,7 @@ function InterlinearizerLoaderInner({ book={book} continuousScroll={continuousScroll} scrRef={activeScrRef} - phraseMode={phraseMode} + phraseMode={isImportView ? VIEW_PHRASE_MODE : phraseMode} setPhraseMode={setPhraseMode} viewOptions={viewOptions} segmentationDispatch={segmentationDispatch} From 4263d159289398fdbc022934aaea6763c17ddd48 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Wed, 2 Sep 2026 12:40:11 -0400 Subject: [PATCH 6/6] Drop an Open fetch the user has already walked away from Adopted from the review fixes on #295. The report stays dismissable while the Open's summary fetch is in flight, so Escape back to the picker and then a summary landing switched the active project and closed the picker the user had returned to. The handler now checks the report is still the modal on screen before acting, which also keeps its failure notice out of whatever they moved on to. Two smaller things from the same review: the failed-load line no longer prints under "Loading..." or a book error, which would have contradicted them; and `Pt9ImportModal`'s `onOpen` doc said Open renders in `import` mode alone, which stopped being true when the offer report was given its Open. Co-Authored-By: Claude Opus 5 (1M context) --- .../components/InterlinearizerLoader.test.tsx | 46 +++++++++++++++++++ src/components/InterlinearizerLoader.tsx | 15 +++++- src/components/modals/Pt9ImportModal.tsx | 4 +- 3 files changed, 61 insertions(+), 4 deletions(-) diff --git a/src/__tests__/components/InterlinearizerLoader.test.tsx b/src/__tests__/components/InterlinearizerLoader.test.tsx index 36e5f6ab..041c013d 100644 --- a/src/__tests__/components/InterlinearizerLoader.test.tsx +++ b/src/__tests__/components/InterlinearizerLoader.test.tsx @@ -1798,6 +1798,52 @@ describe('InterlinearizerLoader', () => { expect(screen.getByTestId('project-modals')).toHaveAttribute('data-modal', 'importPt9'); }); + it('drops an Open fetch that lands after the user has closed the report', async () => { + jest.mocked(papi.notifications.send).mockResolvedValue('notification-id'); + let imported = false; + let releaseSummary: (() => void) | undefined; + mockSendCommand.mockImplementation(async (...args) => { + if (args[0] === 'interlinearizer.getProject' && imported) { + // Hold the summary until the test has taken the user off the report. + await new Promise((resolve) => { + releaseSummary = resolve; + }); + return JSON.stringify(FRESH_IMPORT_SUMMARY); + } + if (args[0] === 'interlinearizer.importPt9Project') { + imported = true; + return JSON.stringify({ + outcome: 'imported', + projectId: 'import-1', + report: IMPORT_REPORT, + }); + } + return JSON.stringify(emptyDraft(testProjectId)); + }); + await act(async () => { + renderLoader(); + }); + await userEvent.click(screen.getByTestId('tab-toolbar-project-menu')); + await userEvent.click(screen.getByTestId('select-modal-import-pt9')); + await screen.findByTestId('pt9-import-report'); + + await userEvent.click( + screen.getByRole('button', { name: '%interlinearizer_pt9ImportModal_open%' }), + ); + await userEvent.click( + screen.getByRole('button', { name: '%interlinearizer_pt9ImportModal_close%' }), + ); + expect(screen.getByTestId('project-modals')).toHaveAttribute('data-modal', 'select'); + + await act(async () => { + releaseSummary?.(); + }); + + // The select modal the user went back to is still theirs, and no project has been switched in. + expect(screen.getByTestId('project-modals')).toHaveAttribute('data-modal', 'select'); + expect(screen.getByTestId('project-modals')).not.toHaveAttribute('data-active-project-name'); + }); + it('notifies and stays on the report when the Open fetch rejects', async () => { jest.mocked(papi.notifications.send).mockResolvedValue('notification-id'); let imported = false; diff --git a/src/components/InterlinearizerLoader.tsx b/src/components/InterlinearizerLoader.tsx index e43aa929..9a761481 100644 --- a/src/components/InterlinearizerLoader.tsx +++ b/src/components/InterlinearizerLoader.tsx @@ -600,6 +600,14 @@ function InterlinearizerLoaderInner({ const [modal, setModal] = useState('none'); + /** + * The modal on screen, for an async handler that must not act on a dialog the user has left. + * Assigned during render rather than from an effect so a promise resolving in the same tick as + * the dismissal still sees the move. + */ + const modalRef = useRef(modal); + modalRef.current = modal; + /** Whether the destructive wipe dialog (book / whole-draft scope picker) is open. */ const [wipeModalOpen, setWipeModalOpen] = useState(false); @@ -746,7 +754,9 @@ function InterlinearizerLoaderInner({ /** * Opens the freshly imported project from the report into the read-only view. A fetch that fails - * leaves the report standing, so the Open can be taken again once whatever broke is fixed. + * leaves the report standing, so the Open can be taken again once whatever broke is fixed. The + * report stays dismissable while that fetch is in flight, and one settling after the user has + * left it neither switches the project nor reports into whatever they moved on to. */ const handlePt9Open = useCallback(async () => { /* v8 ignore next -- Open only renders on a report, which always sets the imported id first */ @@ -757,6 +767,7 @@ function InterlinearizerLoaderInner({ } catch (e) { logger.error('Interlinearizer: failed to load the imported project for opening', e); } + if (modalRef.current !== 'importPt9') return; if (summary) { setActiveProject(summary); setModal('none'); @@ -1076,7 +1087,7 @@ function InterlinearizerLoaderInner({

)} - {importLoadFailed && ( + {!hasError && !showLoading && importLoadFailed && (

{localizedStrings['%interlinearizer_error_pt9Import_load_failed%']}

diff --git a/src/components/modals/Pt9ImportModal.tsx b/src/components/modals/Pt9ImportModal.tsx index d1998e5b..6608b207 100644 --- a/src/components/modals/Pt9ImportModal.tsx +++ b/src/components/modals/Pt9ImportModal.tsx @@ -118,8 +118,8 @@ function ReportRow({ label, value }: Readonly<{ label: string; value: string }>) * report; `offer` titles it the same but the report carries a single Open, dismissal included - * the user already chose to convert, so the report is information on the way in, not a fork; * `sync` titles it as a sync and offers only Close (the project is already open). - * @param props.onOpen - Called when the user opens the imported project from the report; only - * rendered in `import` mode. + * @param props.onOpen - Called when the user opens the imported project from the report; not + * offered in `sync` mode, where the project is already open. * @param props.onClose - Called when the user dismisses the report or failure. */ export function Pt9ImportModal({