Skip to content

fix: pagination controls submit the parent form when nested in a dialog #117

Description

@edjcd

Summary

components/shared/Pagination.tsx wraps @clickhouse/click-ui's Pagination, which renders its controls as bare <button> elements with no explicit type attribute. Per the HTML spec, the missing-value default for <button type> is the Submit Button state, so wherever this component is rendered inside a <form>, activating a pagination control submits that form instead of changing page.

This affects every paginated list nested in a dialog form:

  • access/EditRoleDialog.tsx — members list inside <form onSubmit={handleSubmit}>
  • access/EditGroupDialog.tsx — same structure, same defect
  • shared/FormDialog.tsx — generic dialog wrapper, so any future paginated child inherits the behaviour

The user-visible result is that the list cannot be paged at all: the submit path runs the save mutation and closes the dialog (or bounces to the first tab when validation fails), and no request is issued for the next offset. Standalone usages (RolesTab, GroupsTab, AuditLogTab) are not affected because they are not nested in a form.

The backend pagination contract is correct and unchanged — the client simply never issues the request for the next offset.


Correction to earlier assumptions

Two things I had assumed before reading the actual code turned out to be wrong:

What I assumed What the code actually shows
The prev/next buttons live in Pagination.tsx and need type="button" Pagination.tsx does not render any buttons. It is a thin wrapper around @clickhouse/click-ui's Pagination. The buttons belong to the third-party library.
The ESLint rule valid-button-type.js enforces an explicit HTML type on buttons It only validates the click-ui variant enum (primary, secondary, empty, danger, ghost). It never inspects the HTML type attribute.

The relevant precedent is commit 82abf71a"chore: Bump Dependencies & Fix Add / Remove Item form submission":

Updated the onClick handlers in both AddItemButton and TrashButton to prevent default button behavior before executing the provided onClick function.

The project has already hit and patched exactly this class of bug — but only button by button. Pagination was never addressed. That is the angle of this PR.


Root cause

Component tree:

EditRoleDialog / EditGroupDialog
  └── <form onSubmit={handleSubmit}>     (entire dialog is a form)
        └── Tabs.Content value="members"
              └── <MemberList />
                    └── <Pagination />           src/components/shared/Pagination.tsx
                          └── <CUIPagination />  @clickhouse/click-ui
                                └── <button>     no type attribute

HTML spec: "The missing value default and invalid value default for the type attribute of <button> are the Submit Button state."

So every click on prev / next / a page number triggers:

form submithandleSubmitdoSubmit()updateMutation.mutate()onSuccessonClose()

The dialog closes (or jumps to the details tab if the name is empty) and the page never changes.

Unlike AddItemButton / TrashButton, we cannot apply the e.preventDefault() fix here — CUIPagination only exposes onChange(page: number), never the DOM event. click-ui also exposes no prop to set the underlying button type.


Changes

Primary fix

  • hooks/useButtonType.ts (new): stamps type="button" onto descendant <button> elements that have no explicit type. Follows the existing convention for patching non-parameterisable click-ui output (useStripAriaExpanded, and the DatePickerCell wrapper in AuditLogTab).
  • components/shared/Pagination.tsx: wraps CUIPagination in a div with the hook ref; re-stamps on every currentPage / totalPages change because click-ui re-renders its button set when either value changes.

Defense-in-depth

  • EditRoleDialog.tsx, EditGroupDialog.tsx, FormDialog.tsx: handleSubmit checks SubmitEvent.submitter and only proceeds when data-dialog-action="submit" is present on the explicit save button.

Note: this assumes click-ui's Button forwards unknown props to the DOM node. Happy to drop this layer if maintainers prefer the Pagination-only fix.

Tests

  • Pagination.test.tsx: pagination inside a <form> does not trigger onSubmit when changing page.
  • e2e/access.spec.ts (optional): open Edit role → Members → paginate → dialog stays open.

Audited, not changed

  • RolesTab.tsx, GroupsTab.tsx, AuditLogTab.tsx<Pagination> rendered outside any <form>
  • AddItemButton.tsx, TrashButton.tsx — already patched via preventDefault in 82abf71a
  • UserSearchInline.tsx, SelectedMemberList.tsx, EditButton.tsx, KebabMenu.tsx, StatusToggle.tsx, SearchInput.tsx — audited for the same risk inside dialog forms; no changes needed in this PR

Optional follow-up

  • ESLint rule require-explicit-button-type under tools/eslint-plugin-click-ui/rules/

Test plan

  • Open Edit roleMembers tab → click next/prev/page number → dialog stays open, page changes
  • Open Edit groupMembers tab → same behaviour
  • Click Save → dialog submits and closes as before
  • Standalone pagination in Roles, Groups, Audit log tabs still works
  • Pagination.test.tsx — "does not submit the parent form when changing page" passes

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions