Fix data table pagination (#5059)

* Data table pagination add re-render tests highlighting issues with component state for total pages and current page

* add use effect to update component state if we see that initial state has changed

* do not use use effect

* only change when defaults change

* add more test cases

* also change table content

* add test coverage for onchange

* provide default page size

* appease types

* fix corner case

* Add changeset

Author: gwwar

.changeset/dry-pens-pay.md

@@ -0,0 +1,5 @@
+---
+'@primer/react': major
+---
+
+Fixes negative and invalid pages in data table pagination on re-render.

packages/react/src/DataTable/DataTable.stories.tsx

@@ -195,6 +195,11 @@ export const Playground = (args: DataTableProps<UniqueRow> & ColWidthArgTypes) =
 
   const align = args.align as CellAlignment
 
+  const [pageIndex, setPageIndex] = React.useState(0)
+  const start = pageIndex * parseInt(args.pageSize, 10)
+  const end = start + parseInt(args.pageSize, 10)
+  const rows = data.slice(start, end)
+
   return (
     <Table.Container>
       <Table.Title as="h2" id="repositories">
@@ -207,7 +212,7 @@ export const Playground = (args: DataTableProps<UniqueRow> & ColWidthArgTypes) =
         {...args}
         aria-labelledby="repositories"
         aria-describedby="repositories-subtitle"
-        data={data}
+        data={rows}
         columns={[
           {
             header: 'Repository',
@@ -276,12 +281,22 @@ export const Playground = (args: DataTableProps<UniqueRow> & ColWidthArgTypes) =
           },
         ]}
       />
+      <Table.Pagination
+        aria-label="Pagination for Repositories"
+        pageSize={parseInt(args.pageSize, 10)}
+        totalCount={data.length}
+        onChange={({pageIndex}) => {
+          setPageIndex(pageIndex)
+        }}
+        defaultPageIndex={parseInt(args.defaultPageIndex, 10)}
+      />
     </Table.Container>
   )
 }
 
 Playground.args = {
   cellPadding: 'normal',
+  pageSize: 5,
 }
 
 Playground.argTypes = {
@@ -318,6 +333,18 @@ Playground.argTypes = {
       disable: true,
     },
   },
+  pageSize: {
+    control: {
+      defaultValue: 5,
+      type: 'number',
+      min: 1,
+    },
+  },
+  defaultPageIndex: {
+    control: {
+      type: 'number',
+    },
+  },
   cellPadding: {
     control: {
       type: 'radio',

packages/react/src/DataTable/Pagination.tsx

@@ -192,9 +192,15 @@ export function Pagination({
     totalCount,
   })
   const truncatedPageCount = pageCount > 2 ? Math.min(pageCount - 2, MAX_TRUNCATED_STEP_COUNT) : 0
-  const [offsetStartIndex, setOffsetStartIndex] = useState(() => {
-    return getDefaultOffsetStartIndex(pageIndex, pageCount, truncatedPageCount)
-  })
+  const defaultOffset = getDefaultOffsetStartIndex(pageIndex, pageCount, truncatedPageCount)
+  const [defaultOffsetStartIndex, setDefaultOffsetStartIndex] = useState(defaultOffset)
+  const [offsetStartIndex, setOffsetStartIndex] = useState(defaultOffsetStartIndex)
+
+  if (defaultOffsetStartIndex !== defaultOffset) {
+    setOffsetStartIndex(defaultOffset)
+    setDefaultOffsetStartIndex(defaultOffset)
+  }
+
   const offsetEndIndex = offsetStartIndex + truncatedPageCount - 1
   const hasLeadingTruncation = offsetStartIndex >= 2
   const hasTrailingTruncation = pageCount - 1 - offsetEndIndex > 1
@@ -333,15 +339,15 @@ function getDefaultOffsetStartIndex(pageIndex: number, pageCount: number, trunca
   // When the current page is closer to the end of the list than the beginning
   if (pageIndex > pageCount - 1 - pageIndex) {
     if (pageCount - 1 - pageIndex >= truncatedPageCount) {
-      return pageIndex - 3
+      return Math.max(pageIndex - 3, 1)
     }
-    return pageCount - 1 - truncatedPageCount
+    return Math.max(pageCount - 1 - truncatedPageCount, 1)
   }
 
   // When the current page is closer to the beginning of the list than the end
   if (pageIndex < pageCount - 1 - pageIndex) {
     if (pageIndex >= truncatedPageCount) {
-      return pageIndex - 3
+      return Math.max(pageIndex - 3, 1)
     }
     return 1
   }
@@ -350,7 +356,7 @@ function getDefaultOffsetStartIndex(pageIndex: number, pageCount: number, trunca
   if (pageIndex < truncatedPageCount) {
     return pageIndex
   }
-  return pageIndex - 3
+  return Math.max(pageIndex - 3, 1)
 }
 
 type RangeProps = {
@@ -491,7 +497,7 @@ type PaginationResult = {
 function usePagination(config: PaginationConfig): PaginationResult {
   const {defaultPageIndex, onChange, pageSize, totalCount} = config
   const pageCount = Math.ceil(totalCount / pageSize)
-  const [pageIndex, setPageIndex] = useState(() => {
+  const [defaultIndex, setDefaultIndex] = useState(() => {
     if (defaultPageIndex !== undefined) {
       if (defaultPageIndex >= 0 && defaultPageIndex < pageCount) {
         return defaultPageIndex
@@ -510,6 +516,13 @@ function usePagination(config: PaginationConfig): PaginationResult {
 
     return 0
   })
+  const [pageIndex, setPageIndex] = useState(defaultIndex)
+  const validDefaultPageCount = defaultPageIndex !== undefined && defaultPageIndex >= 0 && defaultPageIndex < pageCount
+  if (validDefaultPageCount && defaultIndex !== defaultPageIndex) {
+    setDefaultIndex(defaultPageIndex)
+    setPageIndex(defaultPageIndex)
+    onChange?.({pageIndex: defaultPageIndex})
+  }
   const pageStart = pageIndex * pageSize
   const pageEnd = Math.min(pageIndex * pageSize + pageSize, totalCount - 1)
   const hasNextPage = pageIndex + 1 < pageCount

packages/react/src/DataTable/__tests__/Pagination.test.tsx

@@ -84,6 +84,27 @@ describe('Table.Pagination', () => {
       await user.click(getPreviousPage())
       expect(onChange).not.toHaveBeenCalled()
     })
+
+    it('should rerender many pages correctly', async () => {
+      const onChange = jest.fn()
+
+      const {rerender} = render(
+        <Pagination aria-label="Test label" onChange={onChange} defaultPageIndex={0} pageSize={25} totalCount={25} />,
+      )
+      expect(getPages()).toHaveLength(1)
+      expect(getCurrentPage()).toEqual(getPage(0))
+      expect(getPageRange()).toEqual('1 through 25 of 25')
+
+      rerender(
+        <Pagination onChange={onChange} aria-label="Test label" defaultPageIndex={2} pageSize={5} totalCount={300} />,
+      )
+      expect(getPageRange()).toEqual('11 through 15 of 300')
+      expect(getCurrentPage()).toEqual(getPage(2))
+      expect(getInvalidPages()).toHaveLength(0)
+      expect(onChange).toHaveBeenCalledWith({
+        pageIndex: 2,
+      })
+    })
   })
 
   describe('with two pages', () => {
@@ -114,6 +135,32 @@ describe('Table.Pagination', () => {
       expect(getPageRange()).toEqual('1 through 25 of 50')
     })
 
+    it('should rerender pager with correct page highlighted when clicking on pages and defaultPageIndex set', async () => {
+      const user = userEvent.setup()
+      const onChange = jest.fn()
+
+      render(
+        <Pagination aria-label="Test label" onChange={onChange} defaultPageIndex={3} pageSize={25} totalCount={200} />,
+      )
+
+      expect(getPageRange()).toEqual('76 through 100 of 200')
+      expect(getCurrentPage()).toEqual(getPage(3))
+
+      await user.click(getPage(1))
+      expect(onChange).toHaveBeenCalledWith({
+        pageIndex: 1,
+      })
+      expect(getPageRange()).toEqual('26 through 50 of 200')
+      expect(getCurrentPage()).toEqual(getPage(1))
+
+      await user.click(getPage(0))
+      expect(onChange).toHaveBeenCalledWith({
+        pageIndex: 0,
+      })
+      expect(getPageRange()).toEqual('1 through 25 of 200')
+      expect(getCurrentPage()).toEqual(getPage(0))
+    })
+
     it('should call `onChange` when using the keyboard to interact with pages', async () => {
       const user = userEvent.setup()
       const onChange = jest.fn()
@@ -165,6 +212,31 @@ describe('Table.Pagination', () => {
       })
     })
 
+    it('should rerender pager with correct page highlighted when clicking on previous or next and defaultPageIndex set', async () => {
+      const user = userEvent.setup()
+      const onChange = jest.fn()
+
+      render(
+        <Pagination aria-label="Test label" onChange={onChange} defaultPageIndex={3} pageSize={25} totalCount={200} />,
+      )
+
+      expect(getPageRange()).toEqual('76 through 100 of 200')
+
+      await user.click(getNextPage())
+      expect(onChange).toHaveBeenCalledWith({
+        pageIndex: 4,
+      })
+      expect(getPageRange()).toEqual('101 through 125 of 200')
+      expect(getCurrentPage()).toEqual(getPage(4))
+
+      await user.click(getPreviousPage())
+      expect(onChange).toHaveBeenCalledWith({
+        pageIndex: 3,
+      })
+      expect(getPageRange()).toEqual('76 through 100 of 200')
+      expect(getCurrentPage()).toEqual(getPage(3))
+    })
+
     it('should call `onChange` when using the keyboard to interact with previous or next', async () => {
       const user = userEvent.setup()
       const onChange = jest.fn()
@@ -197,6 +269,27 @@ describe('Table.Pagination', () => {
         pageIndex: 0,
       })
     })
+
+    it('should rerender many pages correctly', async () => {
+      const onChange = jest.fn()
+
+      const {rerender} = render(
+        <Pagination aria-label="Test label" onChange={onChange} defaultPageIndex={1} pageSize={25} totalCount={50} />,
+      )
+      expect(getPages()).toHaveLength(2)
+      expect(getCurrentPage()).toEqual(getPage(1))
+      expect(getPageRange()).toEqual('26 through 50 of 50')
+
+      rerender(
+        <Pagination aria-label="Test label" onChange={onChange} defaultPageIndex={0} pageSize={5} totalCount={300} />,
+      )
+      expect(getPageRange()).toEqual('1 through 5 of 300')
+      expect(getCurrentPage()).toEqual(getPage(0))
+      expect(getInvalidPages()).toHaveLength(0)
+      expect(onChange).toHaveBeenCalledWith({
+        pageIndex: 0,
+      })
+    })
   })
 
   describe('with three or more pages', () => {
@@ -242,6 +335,34 @@ describe('Table.Pagination', () => {
       expect(pages).toHaveLength(7)
     })
   })
+
+  it('should rerender many pages correctly', async () => {
+    const onChange = jest.fn()
+    const {rerender} = render(
+      <Pagination aria-label="Test label" onChange={onChange} defaultPageIndex={1} pageSize={10} totalCount={1000} />,
+    )
+    expect(getPages()).toHaveLength(8)
+    expect(getCurrentPage()).toEqual(getPage(1))
+    expect(getPageRange()).toEqual('11 through 20 of 1000')
+
+    rerender(
+      <Pagination aria-label="Test label" onChange={onChange} defaultPageIndex={0} pageSize={5} totalCount={300} />,
+    )
+    expect(getPageRange()).toEqual('1 through 5 of 300')
+    expect(getFirstPage()).toEqual(getCurrentPage())
+    expect(getInvalidPages()).toHaveLength(0)
+    expect(onChange).toHaveBeenCalledWith({
+      pageIndex: 0,
+    })
+  })
+
+  it('when rendering 3 pages and the second page is selected we should render a page number not ...', async () => {
+    const onChange = jest.fn()
+    render(<Pagination aria-label="Test label" onChange={onChange} defaultPageIndex={1} pageSize={2} totalCount={6} />)
+    expect(getPageRange()).toEqual('3 through 4 of 6')
+    expect(getCurrentPage()).toEqual(getPage(1))
+    expect(getInvalidPages()).toHaveLength(0)
+  })
 })
 
 function getPages() {
@@ -288,6 +409,10 @@ function getLastPage() {
   return pages[pages.length - 1]
 }
 
+function getInvalidPages() {
+  return getPages().filter(p => p.textContent?.match(/Page\s-/g) || p.textContent?.match(/Page\s0$/g))
+}
+
 function getPageRange() {
   const element = document.querySelector('.TablePaginationRange')
   if (element && element.textContent) {