Skip to content

feat: new fetchRows event#712

Draft
spike-rabbit wants to merge 1 commit into
mainfrom
feat/fetch-rows-event
Draft

feat: new fetchRows event#712
spike-rabbit wants to merge 1 commit into
mainfrom
feat/fetch-rows-event

Conversation

@spike-rabbit

Copy link
Copy Markdown
Member

What kind of change does this PR introduce? (check one with "x")

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior? (You can also link to an open issue here)
page event is used for external paging/sorting.

What is the new behavior?

fetchRows event is used for external paging/sorting.

Does this PR introduce a breaking change? (check one with "x")

  • Yes
  • No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

The key difference is, that the page event requests always an entire page data using the page size of the table.

This has two major flaws:

  • the table cannot fetch additional buffer rows to enable smoother virtual scrolling
  • the page currently includes half visible rows, to avoid blanks. This breaks paging with a pagination, since half visible rows never become fully visible. See Incorrect Page Size Calculation in Table Pagination #146

This new event allows us to modify the ranges better and change the page behavior in the next major release.

DEPRECATED:
DatatableComponent.page event is deprecated in favor of fetchRows.

Application code must be adjusted to fetch rows based on the requested index instead of fetching a specific page.

The page events nature using the page as a foundation has severe restrictions:

  • The table cannot fetch additional buffer rows to enable smoother virtual scrolling
  • The page currently includes half visible rows to avoid blanks, which breaks paging with pagination since half visible rows never become fully visible. See Incorrect Page Size Calculation in Table Pagination #146

@spike-rabbit spike-rabbit requested a review from a team as a code owner June 11, 2026 14:45

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new fetchRows output event to DatatableComponent to replace the deprecated page event, enabling index-based row fetching for better virtual scrolling support. It updates internal logic, tests, and several demo components to use this new event. The review feedback correctly identifies a critical issue across multiple demo components where the offset property (which expects a 0-based page index) is incorrectly set directly to the startIndex row index. Actionable suggestions are provided to calculate the correct page index by dividing the start index by the page or chunk size.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

onFetchRows(event: FetchRowsEvent) {
this.isLoading.update(v => v + 1);
this.serverResultsService.getResults(this.page()).subscribe(pagedData => {
this.offset.set(event.startIndex);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

In ngx-datatable, the offset property represents the page index (0-based page number), not the starting row index. Setting this.offset.set(event.startIndex) will cause the table to incorrectly think it is on a page equal to the row index (e.g., page 20 instead of page 1 when pageSize is 20).

It should be calculated by dividing the startIndex by the page size.

Suggested change
this.offset.set(event.startIndex);
this.offset.set(Math.floor(event.startIndex / this.pageSize()));

this.serverResultsService.getResults(this.page()).subscribe(pagedData => {
this.page.set(pagedData.page);
this.rows.set(pagedData.data);
this.offset.set(event.startIndex);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

In ngx-datatable, the offset property represents the page index (0-based page number), not the starting row index. Setting this.offset.set(event.startIndex) will cause the table to incorrectly think it is on a page equal to the row index (e.g., page 20 instead of page 1 when pageSize is 20).

It should be calculated by dividing the startIndex by the page size.

Suggested change
this.offset.set(event.startIndex);
this.offset.set(Math.floor(event.startIndex / this.pageSize()));

// Pagesize can change depending on browser size
this.pageNumber = pageInfo.offset;
onFetchRows(event: FetchRowsEvent) {
this.offset.set(event.startIndex);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

In ngx-datatable, the offset property represents the page index (0-based page number), not the starting row index. Setting this.offset.set(event.startIndex) will cause the table to incorrectly think it is on a page equal to the row index.

For virtual scrolling, the page index should be calculated based on the chunk size (event.endIndex - event.startIndex).

Suggested change
this.offset.set(event.startIndex);
this.offset.set(Math.floor(event.startIndex / (event.endIndex - event.startIndex)));

this.page.set(pagedData.page);
this.rows.set(pagedData.data);
onFetchRows(event: FetchRowsEvent) {
this.offset.set(event.startIndex);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

In ngx-datatable, the offset property represents the page index (0-based page number), not the starting row index. Setting this.offset.set(event.startIndex) will cause the table to incorrectly think it is on a page equal to the row index (e.g., page 20 instead of page 1 when pageSize is 20).

It should be calculated by dividing the startIndex by the page size.

Suggested change
this.offset.set(event.startIndex);
this.offset.set(Math.floor(event.startIndex / this.pageSize()));

import { By } from '@angular/platform-browser';

import { ColumnMode, SortPropDir } from '../types/public.types';
import { ColumnMode, FetchRowsEvent, SortEvent, SortPropDir } from '../types/public.types';
@spike-rabbit spike-rabbit force-pushed the feat/fetch-rows-event branch from 5c990cb to 19dd8c2 Compare June 11, 2026 18:52
This `fetchRows` replaces the `page` event.
The key difference is, that the `page` event requests always
an entire page data using the page size of the table.

This has two major flaws:

- the table cannot fetch additional buffer rows
  to enable smoother virtual scrolling
- the page currently includes half visible rows, to avoid blanks.
  This breaks paging with a pagination, since half visible rows never become
  fully visible. See #146

This new event allows us to modify the ranges better
and change the page behavior in the next major release.

DEPRECATED:
`DatatableComponent.page` event is deprecated in favor of `fetchRows`.

Application code must be adjusted to fetch rows based on the requested index
instead of fetching a specific page.

The `page` events nature using the page as a foundation has severe restrictions:
- The table cannot fetch additional buffer rows to enable smoother virtual scrolling
- The page currently includes half visible rows to avoid blanks, which breaks paging with pagination
  since half visible rows never become fully visible. See #146
@spike-rabbit spike-rabbit force-pushed the feat/fetch-rows-event branch from 19dd8c2 to d73de59 Compare June 11, 2026 19:57
@spike-rabbit spike-rabbit removed the request for review from chintankavathia June 11, 2026 21:09
// This ensures pager displays correctly when rowsOffset is used
effect(() => {
const rowsOffset = this.rowsOffset();
if (rowsOffset != null && rowsOffset >= 0) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be ignored when externalPaging is false

@fh1ch fh1ch added the enhancement New feature or request label Jun 12, 2026
@fh1ch fh1ch requested a review from Copilot June 12, 2026 13:03

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a new row-range–based fetchRows event intended for external paging/sorting, and updates the demo application to use it instead of the existing (now deprecated) page event. This helps decouple server fetching from page-based assumptions (page size, half-visible rows) and lays groundwork for improved virtual scrolling behavior.

Changes:

  • Added FetchRowsEvent and a new (fetchRows) output on DatatableComponent; marked (page) as deprecated.
  • Added rowsOffset input to support row-index–based offset synchronization with the pager.
  • Updated server-side paging demo components and mock service to fetch by [startIndex, endIndex) ranges, and added unit tests for fetchRows emission.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/app/summary/server-side-paging-summary.component.ts Switch summary server-side paging demo from (page) to (fetchRows) and row-index range fetching.
src/app/paging/virtual-server-side.component.ts Update virtual server-side demo to request/cache fetched row chunks via fetchRows.
src/app/paging/server-side-paging.component.ts Switch basic server-side paging demo to fetchRows and row-index offset tracking.
src/app/paging/scrolling-no-virtual.component.ts Switch non-virtual scrolling demo to fetchRows and row-index offset tracking.
src/app/paging/mock-server-results-service.ts Replace page-based mock API with [startIndex, endIndex) range-based fetching result.
projects/ngx-datatable/src/lib/types/public.types.ts Add new public FetchRowsEvent type.
projects/ngx-datatable/src/lib/components/datatable.component.ts Add rowsOffset input + fetchRows output; emit fetchRows on init/page/sort; deprecate page.
projects/ngx-datatable/src/lib/components/datatable.component.spec.ts Add tests asserting fetchRows emissions for init, paging, and sorting scenarios.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +715 to +720
readonly correctedOffset = computed(() => {
const offset = this.offset();
const rowCount = this.rowCount();
const pageSize = this.pageSize();
return Math.max(Math.min(offset, Math.ceil(rowCount / pageSize) - 1), 0);
});
Comment on lines +748 to +754
effect(() => {
const rowsOffset = this.rowsOffset();
if (rowsOffset != null && rowsOffset >= 0) {
const pageSize = this.pageSize();
untracked(() => this.offset.set(Math.floor(rowsOffset / pageSize)));
}
});
Comment on lines +245 to +252
/**
* Row-based offset for external paging.
* When provided, overrides `offset` (page-based) by converting rows to page number.
* Represents the starting row index (0-based) of the first visible row.
*
* Default value: `null`
*/
readonly rowsOffset = input<number | null>(null);
Comment on lines +187 to +191
export interface FetchRowsEvent {
startIndex: number;
endIndex: number;
sorts: SortPropDir[];
}
Comment on lines +517 to +521
/**
* Emitted when the table needs new rows for external paging/sorting.
* Provides row indexes that should be fetched.
*/
readonly fetchRows = output<FetchRowsEvent>();
@spike-rabbit spike-rabbit marked this pull request as draft June 12, 2026 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants