Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions src/features/bible/components/DraftingUI.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,14 @@ import { DraftingGridVerse, DraftingTargetColumn } from './DraftingGridVerse';
import { DraftingHeader } from './DraftingHeader';
import { DraftingResourceSidebar } from './DraftingResourceSidebar';

/**
* Stable empty snapshot for renders before the first check result settles —
* a module-level constant so the `ChecksPanel` prop reference doesn't change
* (and re-render) on every DraftingUI render. FindingRow falls back to
* `finding.surf` on a miss, so "empty" is always safe.
*/
const EMPTY_VERSE_TEXT_SNAPSHOT: ReadonlyMap<string, string> = new Map<string, string>();

const RESOURCE_NAMES: ResourceName[] = [
{ id: 'UWTranslationNotes', name: 'TN' },
{ id: 'Images', name: 'Images & Maps' },
Expand Down Expand Up @@ -253,6 +261,14 @@ export const DraftingUI: React.FC<DraftingUIProps> = ({
globalRules,
});

// As-checked verse-text snapshot, hydrated onto the settled check result by
// useRepeatedWordsCheck (NOT derived from live `verses` — the findings'
// offsets are relative to the text as it was checked, so the two must travel
// together and update only when a new result arrives). `checkQuery.data` is
// referentially stable per settled result, so this reference only changes
// when new findings do.
const verseTextBySntId = checkQuery.data?.verseTextBySntId ?? EMPTY_VERSE_TEXT_SNAPSHOT;

// Active-finding count drives both notification dots; computed once here and
// threaded to the tab and the closed-panel toggle button (S5, §6.4). When the
// Checks feature is disabled it is forced to 0 so neither dot can flash while
Expand Down Expand Up @@ -634,6 +650,7 @@ export const DraftingUI: React.FC<DraftingUIProps> = ({
globalIgnoresAvailable={globalIgnoresAvailable}
isError={checkQuery.isError}
resolved={resolved}
verseTextBySntId={verseTextBySntId}
onIgnoreEverywhere={ignoreEverywhere}
onIgnoreHere={ignoreHere}
onStopIgnoringEverywhere={stopIgnoringEverywhere}
Expand Down
175 changes: 164 additions & 11 deletions src/features/checks/components/ChecksPanel.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,23 @@ const handlers = () => ({
onStopIgnoringEverywhere: vi.fn(),
});

const renderPanel = (resolved: ResolvedFindings, over: Partial<{ isError: boolean }> = {}) =>
/**
* Empty as-checked snapshot: the pre-existing grouping/toggle/wiring tests
* exercise the fallback path (rows render `finding.surf` bare), which keeps
* their text assertions unchanged.
*/
const emptySnapshot: ReadonlyMap<string, string> = new Map();

const renderPanel = (
resolved: ResolvedFindings,
over: Partial<{ isError: boolean; verseTextBySntId: ReadonlyMap<string, string> }> = {}
) =>
renderWithProviders(
<ChecksPanel
globalIgnoresAvailable
isError={over.isError ?? false}
resolved={resolved}
verseTextBySntId={over.verseTextBySntId ?? emptySnapshot}
{...handlers()}
/>
);
Expand Down Expand Up @@ -105,7 +116,13 @@ describe('ChecksPanel — Show Ignored toggle', () => {
inactive: [inactive('JDG 4:1', 'and and', 'occurrence')],
};
const { user } = renderWithProviders(
<ChecksPanel globalIgnoresAvailable isError={false} resolved={resolved} {...handlers()} />
<ChecksPanel
globalIgnoresAvailable
isError={false}
resolved={resolved}
verseTextBySntId={emptySnapshot}
{...handlers()}
/>
);

// Default OFF: the inactive snippet is not shown.
Expand All @@ -127,7 +144,13 @@ describe('ChecksPanel — Show Ignored toggle', () => {
inactive: [inactive('JDG 4:2', 'and and', 'occurrence')],
};
const { user } = renderWithProviders(
<ChecksPanel globalIgnoresAvailable isError={false} resolved={resolved} {...handlers()} />
<ChecksPanel
globalIgnoresAvailable
isError={false}
resolved={resolved}
verseTextBySntId={emptySnapshot}
{...handlers()}
/>
);

await user.click(screen.getByRole('switch', { name: 'Show Ignored' }));
Expand All @@ -150,15 +173,27 @@ describe('ChecksPanel — Show Ignored toggle', () => {
inactive: [inactive('JDG 4:1', 'and and', 'global')],
};
const { user, unmount } = renderWithProviders(
<ChecksPanel globalIgnoresAvailable isError={false} resolved={resolved} {...handlers()} />
<ChecksPanel
globalIgnoresAvailable
isError={false}
resolved={resolved}
verseTextBySntId={emptySnapshot}
{...handlers()}
/>
);
await user.click(screen.getByRole('switch', { name: 'Show Ignored' }));
expect(screen.getByText('and and (in JDG 4:1)')).toBeInTheDocument();
unmount();

// Fresh mount = fresh session: toggle is OFF again.
renderWithProviders(
<ChecksPanel globalIgnoresAvailable isError={false} resolved={resolved} {...handlers()} />
<ChecksPanel
globalIgnoresAvailable
isError={false}
resolved={resolved}
verseTextBySntId={emptySnapshot}
{...handlers()}
/>
);
expect(screen.queryByText('and and (in JDG 4:1)')).not.toBeInTheDocument();
});
Expand Down Expand Up @@ -188,7 +223,13 @@ describe('ChecksPanel — "Show Ignored" toggle visibility', () => {
inactive: [inactive('JDG 4:1', 'the the', 'legitimate')],
};
renderWithProviders(
<ChecksPanel globalIgnoresAvailable isError={false} resolved={resolved} {...handlers()} />
<ChecksPanel
globalIgnoresAvailable
isError={false}
resolved={resolved}
verseTextBySntId={emptySnapshot}
{...handlers()}
/>
);
expect(screen.queryByTestId('ignored-section')).not.toBeInTheDocument();
});
Expand All @@ -201,7 +242,13 @@ describe('ChecksPanel — "Show Ignored" toggle visibility', () => {
inactive: [inactive('JDG 4:1', 'the the', 'occurrence')],
};
renderWithProviders(
<ChecksPanel globalIgnoresAvailable isError={false} resolved={resolved} {...handlers()} />
<ChecksPanel
globalIgnoresAvailable
isError={false}
resolved={resolved}
verseTextBySntId={emptySnapshot}
{...handlers()}
/>
);
expect(screen.getByText('No issues found')).toBeInTheDocument();
expect(screen.queryByTestId('ignored-section')).not.toBeInTheDocument();
Expand All @@ -213,7 +260,13 @@ describe('ChecksPanel — "Show Ignored" toggle visibility', () => {
inactive: [inactive('JDG 4:1', 'the the', 'occurrence')],
};
const { user } = renderWithProviders(
<ChecksPanel globalIgnoresAvailable isError={false} resolved={resolved} {...handlers()} />
<ChecksPanel
globalIgnoresAvailable
isError={false}
resolved={resolved}
verseTextBySntId={emptySnapshot}
{...handlers()}
/>
);
// Toggle is OFF: zero state is visible.
expect(screen.getByText('No issues found')).toBeInTheDocument();
Expand All @@ -235,7 +288,13 @@ describe('ChecksPanel — "Show Ignored" toggle visibility', () => {
],
};
const { user } = renderWithProviders(
<ChecksPanel globalIgnoresAvailable isError={false} resolved={resolved} {...handlers()} />
<ChecksPanel
globalIgnoresAvailable
isError={false}
resolved={resolved}
verseTextBySntId={emptySnapshot}
{...handlers()}
/>
);
await user.click(screen.getByRole('switch', { name: 'Show Ignored' }));

Expand All @@ -254,7 +313,13 @@ describe('ChecksPanel — callback wiring', () => {
inactive: [],
};
const { user } = renderWithProviders(
<ChecksPanel globalIgnoresAvailable isError={false} resolved={resolved} {...h} />
<ChecksPanel
globalIgnoresAvailable
isError={false}
resolved={resolved}
verseTextBySntId={emptySnapshot}
{...h}
/>
);
await user.click(screen.getByRole('button', { name: 'Ignore Here' }));
expect(h.onIgnoreHere).toHaveBeenCalledWith('JDG 4:1|the the|0');
Expand All @@ -268,10 +333,98 @@ describe('ChecksPanel — callback wiring', () => {
inactive: [inactiveFinding],
};
const { user } = renderWithProviders(
<ChecksPanel globalIgnoresAvailable isError={false} resolved={resolved} {...h} />
<ChecksPanel
globalIgnoresAvailable
isError={false}
resolved={resolved}
verseTextBySntId={emptySnapshot}
{...h}
/>
);
await user.click(screen.getByRole('switch', { name: 'Show Ignored' }));
await user.click(screen.getByRole('button', { name: 'Undo Ignore' }));
expect(h.onUndo).toHaveBeenCalledWith(inactiveFinding);
});
});

describe('ChecksPanel — scroll container', () => {
it('owns its own vertical scrolling (the LeftPanel tabpanel slot clips overflow)', () => {
// Regression: without h-full + overflow-y-auto on the panel root, findings
// past the fold were unreachable ("lost to the land of the living").
renderPanel({ active: [active('JDG 4:1', 'the the')], inactive: [] });
const root = screen.getByTestId('checks-panel-scroll');
expect(root).toHaveClass('h-full', 'overflow-y-auto');
});
});

describe('ChecksPanel — verse-context snapshot threading', () => {
it('threads verseTextBySntId down so rows render the windowed highlight', () => {
// 'the the' at [4, 11) in the as-checked verse text for JDG 4:1.
const verse = 'And the the light was made';
const finding: ResolvedFinding = {
...active('JDG 4:1', 'the the'),
finding: { ...active('JDG 4:1', 'the the').finding, surf: 'the the', start_position: 4 },
};
renderPanel(
{ active: [finding], inactive: [] },
{ verseTextBySntId: new Map([['JDG 4:1', verse]]) }
);
const highlight = screen.getByTestId('verse-highlight');
expect(highlight.textContent).toBe('the the');
expect(highlight.parentElement?.textContent).toBe('And the the light was made');
});

it('overlapping triple: two findings in one verse each render their own window', () => {
// "the the the" produces TWO findings (ordinals 0 and 1) with their own
// start_positions; each card highlights its own span independently.
const verse = 'And the the the light';
const base = active('JDG 4:1', 'the the');
const first: ResolvedFinding = {
...base,
finding: { ...base.finding, surf: 'the the', start_position: 4 },
ordinal: 0,
occurrenceKey: 'JDG 4:1|the the|0',
};
const second: ResolvedFinding = {
...base,
finding: { ...base.finding, surf: 'the the', start_position: 8 },
ordinal: 1,
occurrenceKey: 'JDG 4:1|the the|1',
};
renderPanel(
{ active: [first, second], inactive: [] },
{ verseTextBySntId: new Map([['JDG 4:1', verse]]) }
);
const highlights = screen.getAllByTestId('verse-highlight');
expect(highlights).toHaveLength(2);
// Both highlight a 'the the' span; the windows around them differ by offset.
const rows = highlights.map(h => h.parentElement?.textContent);
expect(rows).toEqual(['And the the the light', 'And the the the light']);
expect(highlights[0].textContent).toBe('the the');
expect(highlights[1].textContent).toBe('the the');
});

it('ignoring one occurrence of an overlapping pair leaves the other card active', async () => {
// Reuses the cascade contract: "Ignore Here" is keyed by the FULL
// occurrence key including the ordinal, so ignoring card 0 must not ignore
// card 1 (the panel simply forwards the row's own occurrenceKey).
const base = active('JDG 4:1', 'the the');
const first: ResolvedFinding = { ...base, ordinal: 0, occurrenceKey: 'JDG 4:1|the the|0' };
const second: ResolvedFinding = { ...base, ordinal: 1, occurrenceKey: 'JDG 4:1|the the|1' };
const h = handlers();
const { user } = renderWithProviders(
<ChecksPanel
globalIgnoresAvailable
isError={false}
resolved={{ active: [first, second], inactive: [] }}
verseTextBySntId={emptySnapshot}
{...h}
/>
);
const buttons = screen.getAllByRole('button', { name: 'Ignore Here' });
expect(buttons).toHaveLength(2);
await user.click(buttons[0]);
expect(h.onIgnoreHere).toHaveBeenCalledTimes(1);
expect(h.onIgnoreHere).toHaveBeenCalledWith('JDG 4:1|the the|0');
});
});
21 changes: 18 additions & 3 deletions src/features/checks/components/ChecksPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,13 @@ import { FindingRow } from './FindingRow';
export interface ChecksPanelProps {
/** Cascade output: `{ active[], inactive[] }` from `useResolvedFindings`. */
resolved: ResolvedFindings;
/**
* As-checked verse text keyed by `snt_id` — the hydrated snapshot that
* travels with the check result (captured in `useRepeatedWordsCheck`), NOT
* the live drafting text. Each row windows its finding against this so the
* offsets always line up and the cards hold still between checks.
*/
verseTextBySntId: ReadonlyMap<string, string>;
/** The check query errored — show the inline red line (W9). */
isError: boolean;
/** Hide the global "Ignore Everywhere" affordance when unavailable (W8). */
Expand Down Expand Up @@ -97,6 +104,7 @@ const groupByVerse = (findings: ResolvedFinding[]): VerseGroup[] => {
const renderGroups = (
groups: VerseGroup[],
globalIgnoresAvailable: boolean,
verseTextBySntId: ReadonlyMap<string, string>,
handlers: Pick<
ChecksPanelProps,
'onIgnoreHere' | 'onIgnoreEverywhere' | 'onUndo' | 'onStopIgnoringEverywhere'
Expand All @@ -111,6 +119,7 @@ const renderGroups = (
key={row.occurrenceKey}
globalIgnoresAvailable={globalIgnoresAvailable}
resolved={row}
verseTextBySntId={verseTextBySntId}
onIgnoreEverywhere={handlers.onIgnoreEverywhere}
onIgnoreHere={handlers.onIgnoreHere}
onStopIgnoringEverywhere={handlers.onStopIgnoringEverywhere}
Expand All @@ -135,6 +144,7 @@ const renderGroups = (
*/
export const ChecksPanel: React.FC<ChecksPanelProps> = ({
resolved,
verseTextBySntId,
isError,
globalIgnoresAvailable,
onIgnoreHere,
Expand All @@ -156,7 +166,10 @@ export const ChecksPanel: React.FC<ChecksPanelProps> = ({
const showZeroState = !isError && activeGroups.length === 0 && !showingIgnored;

return (
<div className='px-1 py-3'>
// Like ResourcePanel, each tab body owns its own scrolling: the LeftPanel
// tabpanel is a clipped `min-h-0 flex-1` slot, so without `overflow-y-auto`
// here findings past the fold would be unreachable.
<div className='h-full overflow-y-auto px-1 py-3' data-testid='checks-panel-scroll'>
{isError && (
<p className='mb-2 text-sm text-red-500' role='alert'>
Checks failed to refresh
Expand All @@ -171,7 +184,9 @@ export const ChecksPanel: React.FC<ChecksPanelProps> = ({
{showZeroState ? (
<p className='py-4 text-center text-sm font-bold'>No issues found</p>
) : (
<div>{renderGroups(activeGroups, globalIgnoresAvailable, handlers)}</div>
<div>
{renderGroups(activeGroups, globalIgnoresAvailable, verseTextBySntId, handlers)}
</div>
)}

{hasIgnored && (
Expand All @@ -189,7 +204,7 @@ export const ChecksPanel: React.FC<ChecksPanelProps> = ({

{showingIgnored && (
<div className='mt-3' data-testid='ignored-section'>
{renderGroups(ignoredGroups, globalIgnoresAvailable, handlers)}
{renderGroups(ignoredGroups, globalIgnoresAvailable, verseTextBySntId, handlers)}
</div>
)}
</AccordionContent>
Expand Down
Loading
Loading