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
6 changes: 6 additions & 0 deletions .changeset/pagination-interruptible-button-guard.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@astryxdesign/core': patch
---

[fix] `Pagination`'s `changeAction` is now interruptible — page changes run in a transition with optimistic page state, so rapid prev/next clicks advance through pages instead of being dropped. `Button`'s `clickAction` keeps its single-fire guard.
@cixzhang
37 changes: 26 additions & 11 deletions packages/core/src/Button/Button.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
*/

import {describe, it, expect, vi} from 'vitest';
import {render, screen} from '@testing-library/react';
import {render, screen, fireEvent, act} from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import {Button} from './Button';
import {Badge} from '../Badge/Badge';
Expand Down Expand Up @@ -228,11 +228,7 @@ describe('Button', () => {
order.push('clickAction');
});
render(
<Button
label="Test"
onClick={handleClick}
clickAction={handleAction}
/>,
<Button label="Test" onClick={handleClick} clickAction={handleAction} />,
);

await user.click(screen.getByRole('button'));
Expand All @@ -246,18 +242,37 @@ describe('Button', () => {
const handleClick = vi.fn((e: React.MouseEvent) => e.preventDefault());
const handleAction = vi.fn();
render(
<Button
label="Test"
onClick={handleClick}
clickAction={handleAction}
/>,
<Button label="Test" onClick={handleClick} clickAction={handleAction} />,
);

await user.click(screen.getByRole('button'));
expect(handleClick).toHaveBeenCalledTimes(1);
expect(handleAction).not.toHaveBeenCalled();
});

it('fires clickAction once on a fast double-click (no double-submit)', async () => {
let resolveAction: (() => void) | undefined;
const handleAction = vi.fn(
async () =>
new Promise<void>(resolve => {
resolveAction = resolve;
}),
);
render(<Button label="Pay" clickAction={handleAction} />);

const button = screen.getByRole('button');
await act(async () => {
fireEvent.click(button);
fireEvent.click(button);
});
expect(handleAction).toHaveBeenCalledTimes(1);

await act(async () => {
resolveAction?.();
await Promise.resolve();
});
});

// type/name/value/form props
it('defaults type to button', () => {
render(<Button label="Test" />);
Expand Down
2 changes: 2 additions & 0 deletions packages/core/src/Button/Button.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -513,6 +513,8 @@ export function Button({
const buttonGroup = useButtonGroup();

const [isPending, startTransition] = useTransition();
// clickAction is fire-once (submit/save/pay), so a same-tick double-click must
// dedupe — which neither isPending nor useOptimistic do. Hence the ref guard.
const actionInFlightRef = useRef(false);
const isLoadingState = isLoading || isPending;
const groupDisabled = buttonGroup?.isDisabled ?? false;
Expand Down
150 changes: 137 additions & 13 deletions packages/core/src/Pagination/Pagination.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
*/

import {describe, it, expect, vi} from 'vitest';
import {render, screen, within} from '@testing-library/react';
import {render, screen, within, fireEvent, act} from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import {Pagination, generatePageRange} from './Pagination';

Expand Down Expand Up @@ -346,6 +346,140 @@ describe('Pagination', () => {
});
});

// ---------------------------------------------------------------------------
// changeAction (interruptible, optimistic)
// ---------------------------------------------------------------------------

describe('changeAction', () => {
it('fires onChange then changeAction with the new page', async () => {
const user = userEvent.setup();
const order: string[] = [];
const onChange = vi.fn(() => order.push('onChange'));
const changeAction = vi.fn(() => {
order.push('changeAction');
});
render(
<Pagination
page={1}
onChange={onChange}
changeAction={changeAction}
totalPages={5}
/>,
);
await user.click(screen.getByRole('button', {name: 'Go to next page'}));
expect(onChange).toHaveBeenCalledWith(2);
expect(changeAction).toHaveBeenCalledWith(2);
expect(order).toEqual(['onChange', 'changeAction']);
});

it('shows the optimistic page while changeAction is pending', async () => {
const user = userEvent.setup();
let resolveAction: (() => void) | undefined;
const changeAction = vi.fn(
async () =>
new Promise<void>(resolve => {
resolveAction = resolve;
}),
);
render(
<Pagination
page={1}
onChange={() => {}}
changeAction={changeAction}
totalPages={5}
variant="compact"
/>,
);

// The committed `page` prop stays at 1, but the indicator optimistically
// reflects the page being navigated to.
await user.click(screen.getByRole('button', {name: 'Go to next page'}));
expect(changeAction).toHaveBeenCalledWith(2);
expect(screen.getByText('Page 2 of 5')).toBeInTheDocument();

await act(async () => {
resolveAction?.();
await Promise.resolve();
});
});

it('interrupts an in-flight action on rapid next clicks', async () => {
// Each click derives its target from the optimistic page, so clicking
// next twice before the action settles advances 1 -> 2 -> 3 instead of
// being dropped by a re-entry guard.
const resolvers: (() => void)[] = [];
const changeAction = vi.fn(
async () =>
new Promise<void>(resolve => {
resolvers.push(resolve);
}),
);
render(
<Pagination
page={1}
onChange={() => {}}
changeAction={changeAction}
totalPages={5}
variant="compact"
/>,
);

const next = screen.getByRole('button', {name: 'Go to next page'});
await act(async () => {
fireEvent.click(next);
});
expect(screen.getByText('Page 2 of 5')).toBeInTheDocument();
await act(async () => {
fireEvent.click(next);
});
expect(screen.getByText('Page 3 of 5')).toBeInTheDocument();

expect(changeAction).toHaveBeenCalledTimes(2);
expect(changeAction).toHaveBeenNthCalledWith(1, 2);
expect(changeAction).toHaveBeenNthCalledWith(2, 3);

await act(async () => {
resolvers.forEach(resolve => resolve());
await Promise.resolve();
});
});

it('supports a synchronous changeAction', async () => {
const user = userEvent.setup();
const changeAction = vi.fn((_page: number) => {});
const onChange = vi.fn();
render(
<Pagination
page={2}
onChange={onChange}
changeAction={changeAction}
totalPages={5}
/>,
);
await user.click(
screen.getByRole('button', {name: 'Go to previous page'}),
);
expect(onChange).toHaveBeenCalledWith(1);
expect(changeAction).toHaveBeenCalledWith(1);
});

it('does not fire changeAction when disabled', async () => {
const user = userEvent.setup();
const changeAction = vi.fn();
render(
<Pagination
page={1}
onChange={() => {}}
changeAction={changeAction}
totalPages={5}
isDisabled
/>,
);
await user.click(screen.getByRole('button', {name: 'Go to next page'}));
expect(changeAction).not.toHaveBeenCalled();
});
});

// ---------------------------------------------------------------------------
// Boundary states
// ---------------------------------------------------------------------------
Expand Down Expand Up @@ -459,12 +593,7 @@ describe('Pagination', () => {
describe('disabled state', () => {
it('disables all page buttons when isDisabled', () => {
render(
<Pagination
page={3}
onChange={() => {}}
totalPages={5}
isDisabled
/>,
<Pagination page={3} onChange={() => {}} totalPages={5} isDisabled />,
);
expect(
screen.getByRole('button', {name: 'Go to previous page'}),
Expand All @@ -480,12 +609,7 @@ describe('Pagination', () => {
const user = userEvent.setup();
const onChange = vi.fn();
render(
<Pagination
page={3}
onChange={onChange}
totalPages={5}
isDisabled
/>,
<Pagination page={3} onChange={onChange} totalPages={5} isDisabled />,
);
// Disabled buttons can't be clicked
await user.click(screen.getByRole('button', {name: 'Go to page 1'}));
Expand Down
Loading
Loading