fix(ToggleButton): run pressedChangeAction in a transition for pending state#3056
fix(ToggleButton): run pressedChangeAction in a transition for pending state#3056cixzhang wants to merge 2 commits into
Conversation
…g state pressedChangeAction was fired as a non-awaited promise, so the documented loading spinner never appeared and rapid clicks could fire the action multiple times. Wrap it in useTransition so the button shows its loading state (disabled + aria-busy) while the async action is pending, and guard against re-entrant clicks until it settles — matching Button's clickAction.
cixzhang
requested review from
ejhammond,
imdreamrunner,
josephfarina,
marie-lucas,
nynexman4464 and
thedjpetersen
as code owners
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
meta-cla
Bot
added
the
CLA Signed
🚀 Vercel Preview Deployment
|
PR Analysis Report📚 Storybook PreviewView Storybook for this PR 🧪 Sandbox PreviewView Sandbox for this PR No new or modified components detected. Bundle Size Summary
Accessibility AuditStatus: No accessibility violations detected. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
| await user.click(button); | ||
| await user.click(button); | ||
|
|
||
| // Second click is a no-op while the first action is still pending. |
There was a problem hiding this comment.
Wait why? It should call the action again.
For example, imagine this use case:
<ToggleButton
label="Favorite"
isPressed={false}
onPressedChange={() => {
setChecked(c => !c);
}}
pressedChangeAction={pressedChangeAction}
/>
If you click twice, it should go from true -> false (interrupted) -> true. The optimistic value would show true, false, true.
|
|
||
| // Wrap pressedChangeAction in a transition so the button shows a loading | ||
| // spinner while the async action is pending and re-entrant clicks are | ||
| // ignored until it settles. Mirrors Button's clickAction handling. |
There was a problem hiding this comment.
Yeah the benefit of the action pattern is that you don't have to guard against re-entrant clicks.
| expect(screen.getByTestId('bold-toggle')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('shows a loading spinner while pressedChangeAction is pending', async () => { |
There was a problem hiding this comment.
We should add a test and support sync actions.
For example, this should work:
<ToggleButton
label="Favorite"
isPressed={false}
onPressedChange={(pressed) => {
// this is NOT async
//
// routes may suspend on data, so loading will stay until
// we can either complete with suspense fallback, or the
// transition can complete with content. if the user clicks
// again while that is waiting, then it's interrupted to handle
// the new navigation.
if (pressed) {
router.go('/a');
} else {
router.go('/b');
}
}}
pressedChangeAction={pressedChangeAction}
/>| actionInFlightRef.current = true; | ||
| startTransition(async () => { | ||
| try { | ||
| await pressedChangeAction(newState); |
There was a problem hiding this comment.
The newState here needs to be tracked as an optimistic state. Because if you click while an action is in progress, it should be the current in progress value (which optimistic tracks) not the old value. This is a more obvious bug after you remove the actionInFlightRef entrance guard, and debounce the loading spinner so that it immediately shows the optimistic state.
…stic state Address review feedback: model the toggle on Switch's useOptimistic pattern. - Track pressed state optimistically so the new state shows immediately and a click mid-flight derives its next value from the in-progress state (true -> false -> true), instead of stalling on the committed value. - Remove the re-entry guard: the action is now interruptible. Re-clicks start a new transition rather than being dropped. - Run onPressedChange + pressedChangeAction inside the transition so a sync but suspending handler (e.g. a router navigation that suspends on data) also drives the pending state. pressedChangeAction now accepts void | Promise<void>. - Debounce the loading spinner so fast actions show the optimistic state without a spinner flash and stay interruptible.
3 participants
Summary
ToggleButton'spressedChangeActionprop is documented as an async action handler that shows a loading spinner while the promise is pending (see the prop's JSDoc andToggleButton.doc.mjs). In practice the action was fired and dropped:So the spinner never appeared, and rapid clicks could fire the action multiple times before it settled.
This wires
pressedChangeActionthroughuseTransition, matching howButton'sclickActionalready works:aria-busy).actionInFlightRefguard).pressedChangeAction's signature is unchanged, and the synchronousonPressedChangestill fires first.ToggleButtonis a thin wrapper overButton, so the pending state is fed into theButton's existingisLoadingpath (isLoading || isPending) and renders the same spinner.Test plan
aria-busywhilepressedChangeActionis pending, and clears once it resolves.pnpm --filter @astryxdesign/core typecheckand the fullToggleButtonsuite (22 tests) pass.Notes / follow-up
ToggleButtonGroup) delegates togroup.toggle(value)and has no async-action path; this PR intentionally scopes to the standalonepressedChangeActionbehavior. Happy to follow up on whether group-backed toggles should support an async action too.useOptimistic-style optimistic toggle (likeSwitch) was considered but left out:isPressedis consumer-controlled, so revert-on-failure semantics are ambiguous. This change keeps the minimal, documented behavior (auto spinner + re-entry guard).