fix(table): preserve page and scroll position on data refresh#4156
fix(table): preserve page and scroll position on data refresh#4156devbymadde wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe table’s remote paginator refresh flow now preserves the controlled page and scroll position, avoids unnecessary data rebuilds, and suppresses refresh-generated events. Unit and e2e tests cover page clamping, restoration, event behavior, and asynchronous updates. ChangesRemote paginator refresh
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-4156/ |
In remote mode, any refresh that rebuilds the rows — a `totalRows`/ `pageSize` change or a row being removed — round-trips through Tabulator's `replaceData()`, which resets its internal pagination to the first page and snaps the row container to top-left. Re-entering the ajax pipeline then emitted `changePage`/`load` for page 1 back to the consumer, so a paginated list (e.g. an object list on page 3) jumped back to page 1 and lost its scroll position whenever a row was deleted or merged on a later page. Fixes, all scoped to remote mode: - Guard the internal `changePage`/`load` emissions in `requestData` and `handleDataSorting` while a paginator refresh is in flight, using a depth counter so overlapping refreshes (`totalRows` and `pageSize` changing in the same tick) can't clear the guard early. The returned `last_page` still updates the paginator UI. - After `replaceData()`, restore Tabulator to the controlled `page` (clamped to the new page count) so `getPage()` and the paginator UI stay in sync. When the current page no longer exists (last row on the last page removed) the clamp is emitted as a genuine `changePage`. - Snapshot and restore the scroll position around the row rebuilds on both the data-change and paginator-refresh paths. - Skip the paginator-refresh round-trip entirely when the page count is unchanged (the common case, e.g. removing one row of many), avoiding two redundant full row rebuilds per data change. Covered by a spec suite for the guard/clamp/restore/skip logic and e2e tests that reproduce the page reset and scroll loss against real Tabulator via the `load` event. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
devbymadde
force-pushed
the
fix/table-preserve-page-on-row-count-change
branch
from
9492dfb to
84620ce
Compare
devbymadde
changed the title
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/table/table.e2e.tsx`:
- Around line 384-394: Update the zero-event assertions for changePageToFirst
and loadForFirstPage to use toHaveLength(0) directly on each filtered event
collection, preserving the existing filters and expected zero counts.
In `@src/components/table/table.tsx`:
- Around line 751-765: Update the pagination refresh method containing
calculatePageCount and targetPage to return immediately when pageSize is unset
or falsy, before calculating maxPage or invoking replaceData/setPage. Preserve
the existing pagination behavior when pageSize is configured.
- Around line 387-390: Update the row-id change path around preserveScroll and
replaceData in the table component to restore the current controlled page after
the remote data replacement completes. Reuse the existing setPage(targetPage)
logic alongside scroll restoration, ensuring same-page-count replacements on
page 3 restore that page even when refreshRemotePaginator() skips its guard, and
add a regression test for this scenario.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0961f94f-f48f-430b-89c3-7af8cb7b0dc3
📒 Files selected for processing (3)
src/components/table/table.e2e.tsxsrc/components/table/table.spec.tssrc/components/table/table.tsx
| const changePageToFirst = changePageSpy.events.filter( | ||
| (event: CustomEvent) => event.detail === 1 | ||
| ); | ||
| expect(changePageToFirst.length).toBe(0); | ||
|
|
||
| // The URL offset in the consumer is driven by the `load` event, not | ||
| // `changePage`, so a `load` for page 1 would also snap the list back. | ||
| const loadForFirstPage = loadSpy.events.filter( | ||
| (event: CustomEvent) => event.detail?.page === 1 | ||
| ); | ||
| expect(loadForFirstPage.length).toBe(0); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the targeted test section with line numbers.
sed -n '350,410p' src/components/table/table.e2e.tsx | cat -n
# See how similar zero-length assertions are written in this file.
rg -n "toBe\\(0\\)|toHaveLength\\(0\\)|length\\).toBe\\(0\\)" src/components/table/table.e2e.tsx
# Sample nearby test style for collection assertions in the repo.
rg -n "toHaveLength\\(|\\.length\\)\\.toBe\\(0\\)" src/components -g '*.e2e.tsx' -g '*.spec.ts' -g '*.test.ts'Repository: Lundalogik/lime-elements
Length of output: 7163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '350,410p' src/components/table/table.e2e.tsx | cat -n
rg -n "toBe\\(0\\)|toHaveLength\\(0\\)|length\\)\\.toBe\\(0\\)" src/components/table/table.e2e.tsx
rg -n "toHaveLength\\(|\\.length\\)\\.toBe\\(0\\)" src/components -g '*.e2e.tsx' -g '*.spec.ts' -g '*.test.ts'Repository: Lundalogik/lime-elements
Length of output: 7163
Use toHaveLength(0) for these zero-event assertions. It reads more clearly than checking .length directly.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 394-394: Prefer a more specific assertion instead of this generic one, e.g. "expect(loadForFirstPage).toHaveLength(0)".
[warning] 387-387: Prefer a more specific assertion instead of this generic one, e.g. "expect(changePageToFirst).toHaveLength(0)".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/table/table.e2e.tsx` around lines 384 - 394, Update the
zero-event assertions for changePageToFirst and loadForFirstPage to use
toHaveLength(0) directly on each filtered event collection, preserving the
existing filters and expected zero counts.
Source: Linters/SAST tools
| const restoreScroll = this.preserveScroll(); | ||
| this.tabulator | ||
| .replaceData(newData) | ||
| .then(restoreScroll, () => {}); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file size ==\n'
wc -l src/components/table/table.tsx
printf '\n== around lines 350-420 ==\n'
sed -n '350,420p' src/components/table/table.tsx
printf '\n== around lines 730-780 ==\n'
sed -n '730,780p' src/components/table/table.tsx
printf '\n== search for page restore / refreshRemotePaginator / replaceData ==\n'
rg -n "refreshRemotePaginator|replaceData\\(|setPage\\(|this\\.page\\b|preserveScroll\\(" src/components/table/table.tsxRepository: Lundalogik/lime-elements
Length of output: 5077
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== around lines 250-360 ==\n'
sed -n '250,360p' src/components/table/table.tsx
printf '\n== around lines 900-1020 ==\n'
sed -n '900,1020p' src/components/table/table.tsx
printf '\n== search for paginator refresh depth / page change emit / data watch ==\n'
rg -n "paginatorRefreshDepth|changePage\\.emit|paginationInitialPage|paginationCurrentPage|pageLoaded|pageChanged|dataChanged|tableBuilt|renderComplete" src/components/table/table.tsxRepository: Lundalogik/lime-elements
Length of output: 8250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('src/components/table/table.tsx')
lines = p.read_text().splitlines()
for start,end in [(250,360),(900,1020)]:
print(f"\n== {start}-{end} ==")
for i in range(start-1, min(end, len(lines))):
print(f"{i+1:4d}: {lines[i]}")
PYRepository: Lundalogik/lime-elements
Length of output: 8881
🌐 Web query:
Tabulator replaceData remote pagination current page reset behavior setPage
💡 Result:
In Tabulator, the replaceData function is primarily designed for loading complete datasets into the table [1][2]. When using remote pagination, replaceData does not function as a standard data refresh for the current page; it often resets the table to the first page or produces unexpected behavior because Tabulator expects to fetch data from the server for each page request [1][2][3]. Because remote pagination relies on the server to provide only the data for the specific page being viewed, there is no local "full dataset" for replaceData to replace [1][2]. Consequently, using replaceData with remote pagination is generally discouraged [4]. If you need to refresh the current page while maintaining the user's position, consider these approaches: 1. Refreshing Data: Use the refreshData function to trigger a reload of the current page from the server [2]. 2. Manual Page Persistence: If you must perform a complex update, you can manually capture the current page index using the getPage function before the update, and then restore it using the setPage function after the data has been loaded [1]. Since replaceData returns a promise, you should execute setPage within the resolution of that promise to ensure the data is loaded first [5]: table.replaceData.then( => { table.setPage(pageNumber); }); Note that with remote pagination, any change to the page index via setPage will inherently trigger an additional server request to fetch that page's data [2].
Citations:
- 1: ReplaceData using Remote Pagination tabulator-tables/tabulator#1920
- 2: setPage/setMaxPage with optional reload tabulator-tables/tabulator#3642
- 3: ReplaceData using Remote Pagination tabulator-tables/tabulator#1920
- 4: replaceData with remote pagination tabulator-tables/tabulator#4702
- 5: https://www.tabulator.info/docs/6.x/update/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== data prop declaration and surrounding comments ==\n'
rg -n "Prop\\(|`@Prop`\\(|data:" src/components/table/table.tsx
sed -n '150,250p' src/components/table/table.tsx
printf '\n== search for remote-mode guards around updateData/replaceData ==\n'
rg -n "isRemoteMode\\(|shouldReplace|replaceData\\(newData\\)|updateData\\(patched\\)|updateOrAddData\\(" src/components/table/table.tsxRepository: Lundalogik/lime-elements
Length of output: 4147
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('src/components/table/table.tsx')
lines = p.read_text().splitlines()
for start,end in [(150,250)]:
print(f"\n== {start}-{end} ==")
for i in range(start-1, min(end, len(lines))):
print(f"{i+1:4d}: {lines[i]}")
PYRepository: Lundalogik/lime-elements
Length of output: 3291
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== around lines 670-700 ==\n'
sed -n '670,700p' src/components/table/table.tsx
printf '\n== around lines 780-840 ==\n'
sed -n '780,840p' src/components/table/table.tsx
printf '\n== around lastPaginatorPageCount initialization/usages ==\n'
rg -n "lastPaginatorPageCount|calculatePageCount\\(" src/components/table/table.tsxRepository: Lundalogik/lime-elements
Length of output: 3867
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1090,1135p' src/components/table/table.tsxRepository: Lundalogik/lime-elements
Length of output: 1685
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '720,735p' src/components/table/table.tsx
sed -n '1090,1135p' src/components/table/table.tsxRepository: Lundalogik/lime-elements
Length of output: 2238
🌐 Web query:
site:tabulator.info replaceData pagination reset page remote pagination
💡 Result:
In Tabulator, the replaceData function is designed to silently replace all data in the table without resetting certain states like the current scroll position, filters, or sorting [1][2]. Because it is intended for silent, background updates, it does not automatically reset pagination to the first page when used [1][2]. If you are using remote pagination and need to reset the table to the first page when performing a data update, you should explicitly call the setPage function immediately after or in conjunction with your data loading method. To reset the page to 1, use the following: table.setPage(1).then(function{ table.replaceData; }); Alternatively, you can chain these actions using the promise returned by the replaceData method if the logic requires the data to load first: table.replaceData.then(function{ table.setPage(1); }); Note that while certain user interactions—such as changing filters, sorting, or toggling group visibility—automatically revert the table to the first page [3], the replaceData method deliberately avoids this behavior to maintain the user's current context [1][2].
Citations:
- 1: https://www.tabulator.info/docs/6.x/update/
- 2: https://tabulator.info/docs/5.5/update
- 3: https://www.tabulator.info/docs/6.x/page/
🌐 Web query:
Tabulator replaceData current page preserved pagination docs
💡 Result:
The Tabulator replaceData function is designed to replace all data in a table while preserving the current scroll position, sorting, and filtering state without triggering the AJAX loading popup [1][2]. However, concerning pagination, there is an important distinction between client-side and remote pagination: 1. Client-Side Pagination: When using client-side pagination, replaceData generally maintains the table state [3]. 2. Remote Pagination: When using remote pagination, the data is managed by the server, not Tabulator itself [4]. In this context, replaceData may not inherently preserve the current page, as the library treats the data update as a new set of information to be handled by the server [4]. If you are using remote pagination and wish to maintain the current page after an update, you can manually persist the page number [4]: - Use the getPage function to retrieve the current page number before performing the update [4]. - Use the setPage function to restore that page number after the update process is complete [4]. Since the replaceData method returns a promise, it is recommended to trigger setPage within that promise to ensure the table has finished processing the new data [2]: table.replaceData.then(function { table.setPage(currentPageNumber); }); If you are using setData instead of replaceData, be aware that setData is intended to reset the table entirely, including returning the pagination to the first page [3].
Citations:
- 1: https://www.tabulator.info/docs/6.x/update/
- 2: https://tabulator.info/docs/5.4/update
- 3: setData tabulator-tables/tabulator#1586
- 4: ReplaceData using Remote Pagination tabulator-tables/tabulator#1920
Restore the current page after remote replaceData() updates.
When a row-id change forces replaceData(newData), this path restores scroll but not this.page. In remote pagination, Tabulator can fall back to page 1 while the controlled page prop stays on the old page, and the refreshRemotePaginator() guard skips the page restore when the page count hasn’t changed. Reuse the same setPage(targetPage) logic here and add a regression for a same-page-count replacement on page 3.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/table/table.tsx` around lines 387 - 390, Update the row-id
change path around preserveScroll and replaceData in the table component to
restore the current controlled page after the remote data replacement completes.
Reuse the existing setPage(targetPage) logic alongside scroll restoration,
ensuring same-page-count replacements on page 3 restore that page even when
refreshRemotePaginator() skips its guard, and add a regression test for this
scenario.
| const maxPage = this.calculatePageCount(); | ||
|
|
||
| if (maxPage === this.lastPaginatorPageCount) { | ||
| return; | ||
| } | ||
|
|
||
| // `replaceData` resolves through `requestData`, which rejects when the | ||
| // component is destroyed, and Tabulator's ajax pipeline can reject too. | ||
| // Since the watchers call this without awaiting, swallow rejections here | ||
| // to avoid an unhandled promise rejection. | ||
| const restoreScroll = this.preserveScroll(); | ||
|
|
||
| const targetPage = Math.min(this.page, Math.max(maxPage, FIRST_PAGE)); | ||
|
|
||
| this.paginatorRefreshDepth++; | ||
| try { | ||
| await this.tabulator.replaceData(); | ||
| if (this.tabulator.getPage?.() !== targetPage) { | ||
| await this.tabulator.setPage(targetPage); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Avoid paginating when pageSize is unset.
totalRowsChanged() always invokes this method, but pagination is disabled when pageSize is falsy. calculatePageCount() then returns NaN, so Line 759 can call setPage(NaN) and emit changePage(NaN).
Proposed fix
private async refreshRemotePaginator() {
- if (!this.isRemoteMode() || !this.tabulator || !this.initialized) {
+ if (
+ !this.isRemoteMode() ||
+ !this.pageSize ||
+ !this.tabulator ||
+ !this.initialized
+ ) {
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const maxPage = this.calculatePageCount(); | |
| if (maxPage === this.lastPaginatorPageCount) { | |
| return; | |
| } | |
| // `replaceData` resolves through `requestData`, which rejects when the | |
| // component is destroyed, and Tabulator's ajax pipeline can reject too. | |
| // Since the watchers call this without awaiting, swallow rejections here | |
| // to avoid an unhandled promise rejection. | |
| const restoreScroll = this.preserveScroll(); | |
| const targetPage = Math.min(this.page, Math.max(maxPage, FIRST_PAGE)); | |
| this.paginatorRefreshDepth++; | |
| try { | |
| await this.tabulator.replaceData(); | |
| if (this.tabulator.getPage?.() !== targetPage) { | |
| await this.tabulator.setPage(targetPage); | |
| const maxPage = this.calculatePageCount(); | |
| if (maxPage === this.lastPaginatorPageCount) { | |
| return; | |
| } | |
| const restoreScroll = this.preserveScroll(); | |
| const targetPage = Math.min(this.page, Math.max(maxPage, FIRST_PAGE)); | |
| this.paginatorRefreshDepth++; | |
| try { | |
| await this.tabulator.replaceData(); | |
| if (this.tabulator.getPage?.() !== targetPage) { | |
| await this.tabulator.setPage(targetPage); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/table/table.tsx` around lines 751 - 765, Update the pagination
refresh method containing calculatePageCount and targetPage to return
immediately when pageSize is unset or falsy, before calculating maxPage or
invoking replaceData/setPage. Preserve the existing pagination behavior when
pageSize is configured.
| if (shouldReplace) { | ||
| this.pool.releaseAll(); | ||
| this.tabulator.replaceData(newData); | ||
| const restoreScroll = this.preserveScroll(); |
There was a problem hiding this comment.
Tested this branch in the webclient and found a regression: after creating a new record, it used to show up highlighted right away — now it's nowhere to be seen. The highlight itself still works (activeRow just sets a CSS class), but nothing actually scrolls to the row. What made new records visible before was the very snap-to-top this PR removes: the new record sorts to the top of the list, and the scroll reset happened to reveal it. Now the position is kept, so the record sits highlighted somewhere off-screen — or on a different page if you weren't on page 1.
| this.tabulator.replaceData(newData); | ||
| const restoreScroll = this.preserveScroll(); | ||
| this.tabulator | ||
| .replaceData(newData) |
There was a problem hiding this comment.
Another thing I ran into while testing: deleting a row can make the table go white for a moment before the scroll position is put back. See video:
Screen.Recording.2026-07-13.at.08.40.47.mov
| * all row elements (e.g. `replaceData`), so wrap such operations with this | ||
| * and invoke the returned callback once the rebuild has settled. | ||
| */ | ||
| private preserveScroll(): () => void { |
There was a problem hiding this comment.
Deleting a row while scrolled all the way down leaves the table a bit above the bottom instead of staying at the bottom. Expected behavior is to stay at the bottom of the table, see video:
Screen.Recording.2026-07-13.at.08.57.19.mov
| this.lastPaginatorPageCount = maxPage; | ||
| restoreScroll(); | ||
|
|
||
| if (targetPage !== this.page) { |
There was a problem hiding this comment.
There's a follow-up problem when the clamp kicks in: currentLoad never gets updated during the refresh (the guard skips it), so it still says page 3. If the user later navigates back to page 3, requestData builds the same load object again, isEqual matches, and no load event is emitted — the consumer never fetches that page's data. Resetting currentLoad when emitting the clamp would avoid that.
| if (scrollContainer) { | ||
| scrollContainer.scrollTop = scrollTop; | ||
| scrollContainer.scrollLeft = scrollLeft; | ||
| this.lastPaginatorPageCount = maxPage; |
There was a problem hiding this comment.
This stores the page count that was computed before the awaits, and that value can be wrong: when totalRows and pageSize change in the same tick, the watchers fire one prop at a time, so the first refresh runs with half-updated props. If that in-between count gets stored here, a later change that happens to land on the same number is skipped, and the paginator keeps showing the old count. Rare in practice, but recomputing calculatePageCount() after the awaits would avoid it.
2 participants
What & why
In remote mode,
limel-tablesnapped a paginated list back to page 1 and lost its scroll position whenever the rows were rebuilt — for example when a row was deleted or two objects were merged while the user was on a later page (e.g. page 3 of a list). The root cause is thatreplaceData()resets Tabulator's internal pagination to the first page and scrolls the row container to top-left, and re-entering the ajax pipeline then emittedchangePage/loadfor page 1 back to the consumer.This regressed after the remote-paginator refresh was added, and affected any consumer of
limel-tablein remote mode (the object list in lime-webclient being the visible one).Changes
All changes are scoped to remote mode.
changePage/loademissions inrequestDataandhandleDataSortingwhile a paginator refresh is in flight, using a depth counter so overlapping refreshes (totalRowsandpageSizechanging in the same tick) can't clear the guard early. The returnedlast_pagestill updates the paginator UI.replaceData(), restore Tabulator to the controlledpage(clamped to the new page count) sogetPage()and the paginator UI stay in sync. When the current page no longer exists (the last row on the last page was removed), the clamp is emitted as a genuinechangePage.Testing
Summary by CodeRabbit
Review:
Browsers tested:
(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)
Windows:
Linux:
macOS:
Mobile: