Skip to content

Commit a05ca88

Browse files
Fix code-review findings across segmentation persistence and boundary
editing Persistence: Open now loads a project's stored segmentation (validated) into the draft, and Save As / Overwrite send segmentationJson alongside the analysis, so custom boundaries survive the full save/open loop instead of being silently wiped. markSynced compares both the analysis and segmentation snapshots, so a boundary edit made during a save round-trip keeps the draft dirty. Boundary edits: the cross-segment pull anchors the moved boundary on the next word token (merging wholly when only trailing punctuation remains), so a pull can no longer strand a punctuation-only segment or record a punctuation ref in addedStarts. Former boundaries are exposed as a word-anchored map so punct-initial verses show the restore tick and split back to the exact default boundary. Boundary controls disable while a phrase mode is active, closing a path that could restore a phrase spanning two segments. Segment window: an explicit segmentationVersion signal (threaded from the loader) replaces the verse-key inference, so a merge absorbing the active verse no longer flashes a recenter fade and a re-tokenized book recenters again. Performance: the loader's book/formerBoundaries memos key on version counters (plus isDraftLoading for the initial load) instead of the draft identity, so gloss autosaves no longer re-run full-book resegmentation; the not-mid-phrase guard is a straddled-boundary set precomputed once per phrase-link change, replacing per-slot link scans and store subscriptions. Cleanups: useBookIndexes owns segmentOrder/fullTokenOrder/wordRefByOrder in its single pass; autosaveDraft unifies the analysis/segmentation autosave pipelines; ContinuousView shares one holdCentered loop and reuses commitPendingActiveSegment; saveAnalysis's command schema documents segmentationJson; user-questions.md describes the shipped always-visible boundary controls.
1 parent d60ed58 commit a05ca88

21 files changed

Lines changed: 1213 additions & 337 deletions

src/__tests__/components/Interlinearizer.test.tsx

Lines changed: 132 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,13 @@ jest.mock('../../components/AnalysisStore', () => ({
126126
* @returns An empty `Map`.
127127
*/
128128
usePhraseLinkMap: () => new Map(),
129-
usePhraseLinkByIdMap: () => new Map(),
129+
/**
130+
* Returns the test-owned phrase-link map so straddled-boundary tests can seed phrases the
131+
* component's `straddledBoundaryRefs` memo sees.
132+
*
133+
* @returns The current phrase-link-by-id map.
134+
*/
135+
usePhraseLinkByIdMap: () => mockPhraseLinkById,
130136
/**
131137
* Returns a getter over the test-owned phrase-link map so force-break tests can seed straddling
132138
* phrases.
@@ -444,7 +450,7 @@ function renderInterlinearizer({
444450
showMorphology = false,
445451
showFreeTranslation = false,
446452
segmentationDispatch,
447-
formerBoundaryRefs,
453+
formerBoundaries,
448454
}: {
449455
book?: Book;
450456
continuousScroll?: boolean;
@@ -456,15 +462,15 @@ function renderInterlinearizer({
456462
showMorphology?: boolean;
457463
showFreeTranslation?: boolean;
458464
segmentationDispatch?: SegmentationDispatch;
459-
formerBoundaryRefs?: ReadonlySet<string>;
465+
formerBoundaries?: ReadonlyMap<string, string>;
460466
} = {}) {
461467
return render(
462468
withNav(
463469
<Interlinearizer
464470
book={book}
465471
continuousScroll={continuousScroll}
466472
segmentationDispatch={segmentationDispatch}
467-
formerBoundaryRefs={formerBoundaryRefs}
473+
formerBoundaries={formerBoundaries}
468474
scrRef={scrRef}
469475
analysisLanguage="und"
470476
phraseMode={{ kind: 'view' }}
@@ -1551,19 +1557,134 @@ describe('focus preservation across segmentation edits', () => {
15511557
});
15521558
});
15531559

1554-
describe('former boundary refs', () => {
1555-
it('provides the supplied formerBoundaryRefs to the views through the segmentation context', () => {
1556-
const formerBoundaryRefs: ReadonlySet<string> = new Set(['GEN 1:2:0']);
1560+
describe('former boundaries', () => {
1561+
it('provides the supplied formerBoundaries map to the views through the segmentation context', () => {
1562+
// Anchor word ref → removed default start ref (distinct when the verse opens with punctuation).
1563+
const formerBoundaries: ReadonlyMap<string, string> = new Map([['GEN 1:2:1', 'GEN 1:2:0']]);
15571564
renderInterlinearizer({
15581565
book: GEN_1_MULTI_BOOK,
15591566
continuousScroll: true,
1560-
formerBoundaryRefs,
1567+
formerBoundaries,
15611568
});
1562-
expect(capturedSegmentation?.formerBoundaryRefs).toBe(formerBoundaryRefs);
1569+
expect(capturedSegmentation?.formerBoundaries).toBe(formerBoundaries);
15631570
});
15641571

1565-
it('defaults formerBoundaryRefs to an empty set when the loader supplies none', () => {
1572+
it('defaults formerBoundaries to an empty map when the loader supplies none', () => {
15661573
renderInterlinearizer({ book: GEN_1_MULTI_BOOK, continuousScroll: true });
1567-
expect(capturedSegmentation?.formerBoundaryRefs?.size).toBe(0);
1574+
expect(capturedSegmentation?.formerBoundaries?.size).toBe(0);
1575+
});
1576+
});
1577+
1578+
describe('straddled boundary refs', () => {
1579+
/**
1580+
* Renders with continuous scroll on (so the stubbed ContinuousView captures the segmentation
1581+
* context) and returns the straddled-boundary set the component computed. Phrase links must be
1582+
* seeded into `mockPhraseLinkById` before calling.
1583+
*
1584+
* @param book - The book fixture to render.
1585+
* @returns The `straddledBoundaryRefs` set captured from the segmentation context.
1586+
*/
1587+
function renderAndCaptureStraddled(book: Book): ReadonlySet<string> {
1588+
renderInterlinearizer({ book, continuousScroll: true });
1589+
const straddled = capturedSegmentation?.straddledBoundaryRefs;
1590+
if (!straddled) throw new Error('expected a captured straddled-boundary set');
1591+
return straddled;
1592+
}
1593+
1594+
it('blocks the word refs strictly inside a contiguous three-word phrase', () => {
1595+
mockPhraseLinkById.set('p1', makePhraseLink('p1', ['GEN 1:1:0', 'GEN 1:2:0', 'GEN 1:3:0']));
1596+
const straddled = renderAndCaptureStraddled(makeLargeBook(4));
1597+
// A boundary before the 2nd or 3rd word would cut the phrase.
1598+
expect(straddled.has('GEN 1:2:0')).toBe(true);
1599+
expect(straddled.has('GEN 1:3:0')).toBe(true);
1600+
});
1601+
1602+
it('does not block the phrase-leading word ref or the word after the phrase', () => {
1603+
mockPhraseLinkById.set('p1', makePhraseLink('p1', ['GEN 1:1:0', 'GEN 1:2:0', 'GEN 1:3:0']));
1604+
const straddled = renderAndCaptureStraddled(makeLargeBook(4));
1605+
// A boundary before the phrase's first word or after its last word leaves it intact.
1606+
expect(straddled.has('GEN 1:1:0')).toBe(false);
1607+
expect(straddled.has('GEN 1:4:0')).toBe(false);
1608+
});
1609+
1610+
it('blocks the gap word ref inside a discontiguous phrase', () => {
1611+
// The phrase spans words 1 and 3; word 2 sits in the gap, but a boundary before it would still
1612+
// put the phrase's halves in different segments.
1613+
mockPhraseLinkById.set('p1', makePhraseLink('p1', ['GEN 1:1:0', 'GEN 1:3:0']));
1614+
const straddled = renderAndCaptureStraddled(makeLargeBook(4));
1615+
expect(straddled.has('GEN 1:2:0')).toBe(true);
1616+
});
1617+
1618+
it('exposes an empty set when no phrase links exist', () => {
1619+
const straddled = renderAndCaptureStraddled(makeLargeBook(4));
1620+
expect(straddled.size).toBe(0);
1621+
});
1622+
});
1623+
1624+
describe('segmentationVersion pass-through', () => {
1625+
/**
1626+
* Builds the wrapped `<Interlinearizer>` element for the pass-through tests, varying only the
1627+
* book and segmentation version between renders.
1628+
*
1629+
* @param book - The (re)segmented book to render.
1630+
* @param segmentationVersion - The boundary-edit counter forwarded to the segment window.
1631+
* @returns The element wrapped in a nav provider.
1632+
*/
1633+
function versionedEl(book: Book, segmentationVersion: number): ReactNode {
1634+
return withNav(
1635+
<Interlinearizer
1636+
book={book}
1637+
continuousScroll={false}
1638+
scrRef={defaultScrRef}
1639+
segmentationVersion={segmentationVersion}
1640+
analysisLanguage="und"
1641+
phraseMode={{ kind: 'view' }}
1642+
setPhraseMode={() => {}}
1643+
viewOptions={{ ...allFalseViewOptions }}
1644+
/>,
1645+
);
1646+
}
1647+
1648+
it('redraws in place without a recenter fade when a boundary edit bumps segmentationVersion', () => {
1649+
const { container, rerender } = render(versionedEl(GEN_1_MULTI_BOOK, 0));
1650+
const merged = resegmentBook(GEN_1_MULTI_BOOK, {
1651+
removedVerseStarts: ['GEN 1:2:0'],
1652+
addedStarts: [],
1653+
});
1654+
act(() => {
1655+
rerender(versionedEl(merged, 1));
1656+
});
1657+
// Were the prop dropped on the way to the segment window, the segments-identity change would be
1658+
// classified as a re-tokenization and fade the list out.
1659+
expect(container.querySelector('.tw\\:gap-2.tw\\:transition-opacity')).toHaveStyle({
1660+
opacity: '1',
1661+
});
1662+
});
1663+
1664+
it('fades the list for a segments change without a segmentationVersion bump', () => {
1665+
jest.useFakeTimers();
1666+
try {
1667+
const { container, rerender } = render(versionedEl(GEN_1_MULTI_BOOK, 0));
1668+
const merged = resegmentBook(GEN_1_MULTI_BOOK, {
1669+
removedVerseStarts: ['GEN 1:2:0'],
1670+
addedStarts: [],
1671+
});
1672+
act(() => {
1673+
rerender(versionedEl(merged, 0));
1674+
});
1675+
// Same segments change, same version: a re-tokenization, so the recenter fade runs. This
1676+
// guards the positive case above against passing vacuously in a setup where fades never fire.
1677+
expect(container.querySelector('.tw\\:gap-2.tw\\:transition-opacity')).toHaveStyle({
1678+
opacity: '0',
1679+
});
1680+
act(() => {
1681+
jest.advanceTimersByTime(RECENTER_FADE_MS);
1682+
});
1683+
expect(container.querySelector('.tw\\:gap-2.tw\\:transition-opacity')).toHaveStyle({
1684+
opacity: '1',
1685+
});
1686+
} finally {
1687+
jest.useRealTimers();
1688+
}
15681689
});
15691690
});

0 commit comments

Comments
 (0)