fix(table): use updateData with targeted reformat for row updates#3982
Merged
fix(table): use updateData with targeted reformat for row updates#3982
Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThe table component's Changes
Sequence Diagram(s)sequenceDiagram
participant Component as Component
participant Pool as Pool
participant Tabulator as Tabulator
Component->>Component: receive newData, oldData
alt ids/order differ -> replace
Component->>Pool: releaseAll()
Component->>Tabulator: replaceData(newData)
else same ids/order -> incremental
Component->>Component: patched = fillMissingFields(newData, oldData)
Component->>Tabulator: updateData(patched)
Note right of Tabulator: updateData returns Promise
Tabulator-->>Component: Promise resolves
Component->>Tabulator: reformatChangedRows(newData, oldData) -> getRow(...).reformat()
Component->>Component: setSelection()
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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-3982/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/table/table.tsx (1)
333-346:⚠️ Potential issue | 🟠 MajorAwait
replaceData()before applying selection state, and merge duplicate branches.
setSelection()currently runs immediately afterreplaceData(newData). Although the code is wrapped insetTimeout, this only defers execution—it doesn't await the Promise. IfreplaceDatacompletes asynchronously (which it does), selection will be applied before the data replacement and render finish, risking stale row references.Additionally, the
shouldReplaceandhasRowUpdatesbranches are identical and should be merged.Proposed fix
- if (shouldReplace) { - this.pool.releaseAll(); - this.tabulator.replaceData(newData); - this.setSelection(); - - return; - } - - if (hasRowUpdates) { - this.pool.releaseAll(); - this.tabulator.replaceData(newData); - this.setSelection(); - - return; - } + if (shouldReplace || hasRowUpdates) { + this.pool.releaseAll(); + void this.tabulator + .replaceData(newData) + .then(() => this.setSelection()) + .catch(() => undefined); + + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/table/table.tsx` around lines 333 - 346, The two identical branches using shouldReplace and hasRowUpdates should be merged and made to await the asynchronous replace operation before calling setSelection: change the conditional to if (shouldReplace || hasRowUpdates) then call this.pool.releaseAll(); await this.tabulator.replaceData(newData); and only after that call this.setSelection(); ensure the enclosing method is async (or return the Promise from replaceData().then(...)) and remove the current setTimeout-based defer so selection is applied after replaceData completes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/components/table/table.tsx`:
- Around line 333-346: The two identical branches using shouldReplace and
hasRowUpdates should be merged and made to await the asynchronous replace
operation before calling setSelection: change the conditional to if
(shouldReplace || hasRowUpdates) then call this.pool.releaseAll(); await
this.tabulator.replaceData(newData); and only after that call
this.setSelection(); ensure the enclosing method is async (or return the Promise
from replaceData().then(...)) and remove the current setTimeout-based defer so
selection is applied after replaceData completes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f8fc1b2c-41d6-4709-88e3-2464dbb15e70
📒 Files selected for processing (1)
src/components/table/table.tsx
Befkadu1
force-pushed
the
fix/table-replace-data-on-row-updates
branch
from
b7acf65 to
58ae11f
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/table/table.tsx`:
- Around line 341-344: The code races selection updates because
tabulator.replaceData(...) returns a Promise but setSelection() is called
immediately; update both branches (the shouldReplace branch and the
hasRowUpdates branch around replaceData, pool.releaseAll and setSelection) to
await the replaceData promise (or use .then()) before calling
this.setSelection(), ensuring you call this.pool.releaseAll() at the same
correct point and reapply selection only after this.tabulator.replaceData(...)
has resolved.
🪄 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: 8bc6b8ed-45d6-464e-9afa-d1921c973d8e
📒 Files selected for processing (1)
src/components/table/table.tsx
Befkadu1
force-pushed
the
fix/table-replace-data-on-row-updates
branch
from
f8b1de1 to
c2a3b82
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/table/table.spec.ts`:
- Around line 32-34: Add an assertion that the mocked pool.releaseAll() is
invoked when replaceData() runs: in the test where (component as any).pool = {
releaseAll: vi.fn() } is set and replaceData() is called, assert that (component
as any).pool.releaseAll was called (e.g., using toHaveBeenCalled()) to verify
the pool cleanup behavior; locate the test that calls component.replaceData and
add the expect((component as any).pool.releaseAll).toHaveBeenCalled() assertion
after the call.
🪄 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: ba0771be-83f3-4135-a801-ce6f7786d1d5
📒 Files selected for processing (2)
src/components/table/table.spec.tssrc/components/table/table.tsx
Comment on lines
+32
to
+34
| (component as any).pool = { | ||
| releaseAll: vi.fn(), | ||
| }; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider asserting pool.releaseAll() is called.
The mock is set up but no test verifies that pool.releaseAll() is invoked before replaceData(). Adding an assertion would ensure the pool cleanup behavior is tested.
Example assertion to add in relevant tests
expect((component as any).pool.releaseAll).toHaveBeenCalled();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/table/table.spec.ts` around lines 32 - 34, Add an assertion
that the mocked pool.releaseAll() is invoked when replaceData() runs: in the
test where (component as any).pool = { releaseAll: vi.fn() } is set and
replaceData() is called, assert that (component as any).pool.releaseAll was
called (e.g., using toHaveBeenCalled()) to verify the pool cleanup behavior;
locate the test that calls component.replaceData and add the expect((component
as any).pool.releaseAll).toHaveBeenCalled() assertion after the call.
src/components/table/table.spec.ts
Outdated
| const tabulator = (component as any).tabulator; | ||
| expect(tabulator.replaceData).not.toHaveBeenCalled(); | ||
| expect(tabulator.updateData).toHaveBeenCalledWith(newData); | ||
| expect(tabulator.replaceData).toHaveBeenCalledWith(newData); |
Contributor
There was a problem hiding this comment.
Since releasing the pool is a key part of the fix (to avoid stale custom component references), consider adding an assertion here:
expect((component as any).pool.releaseAll).toHaveBeenCalled();This would verify the pool cleanup behavior that the PR description highlights.
Befkadu1
changed the title
Befkadu1
force-pushed
the
fix/table-replace-data-on-row-updates
branch
from
c5c4e31 to
b27462c
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/components/table/table.tsx (1)
334-347:⚠️ Potential issue | 🟠 MajorDon't let post-update work outlive the Tabulator instance that started it.
Line 337 still reapplies selection before
replaceData()has finished, and Lines 344-347 reusethis.tabulatorwithout checking whetherdisconnectedCallback()orinit()has already swapped it out. That can restore selection too early or makereformatChangedRows()hit a dead/new table.For Tabulator 6.x, do `replaceData()` and `updateData()` return Promises that resolve after the table data has been updated, and should follow-up work run from that promise chain?Suggested fix
setTimeout(() => { if (!this.tabulator || !this.initialized) { return; } + const tabulator = this.tabulator; + if (shouldReplace) { this.pool.releaseAll(); - this.tabulator.replaceData(newData); - this.setSelection(); + tabulator.replaceData(newData).then(() => { + if (this.destroyed || this.tabulator !== tabulator) { + return; + } + + this.setSelection(); + }); return; } if (!areRowsEqual(newData, oldData)) { const patched = this.fillMissingFields(newData, oldData); - this.tabulator.updateData(patched).then(() => { + tabulator.updateData(patched).then(() => { + if (this.destroyed || this.tabulator !== tabulator) { + return; + } + this.reformatChangedRows(newData, oldData); this.setSelection(); }); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/table/table.tsx` around lines 334 - 347, The post-update work currently runs without awaiting Tabulator's async completion and can act on a different instance if disconnectedCallback()/init() swapped it; capture the current table instance into a local variable (e.g., const tab = this.tabulator) before calling replaceData() or updateData(), await/then the returned Promise from tab.replaceData(...) and tab.updateData(...), and inside the then handler first verify tab === this.tabulator (return early if not) before calling this.setSelection() or this.reformatChangedRows(...); do the same instance-identity guard for the patched = this.fillMissingFields(...) + tab.updateData(patched).then(...) path and keep pool.releaseAll() where appropriate. Ensure you reference replaceData, updateData, reformatChangedRows, setSelection, fillMissingFields, pool.releaseAll, disconnectedCallback and init when making the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/components/table/table.tsx`:
- Around line 334-347: The post-update work currently runs without awaiting
Tabulator's async completion and can act on a different instance if
disconnectedCallback()/init() swapped it; capture the current table instance
into a local variable (e.g., const tab = this.tabulator) before calling
replaceData() or updateData(), await/then the returned Promise from
tab.replaceData(...) and tab.updateData(...), and inside the then handler first
verify tab === this.tabulator (return early if not) before calling
this.setSelection() or this.reformatChangedRows(...); do the same
instance-identity guard for the patched = this.fillMissingFields(...) +
tab.updateData(patched).then(...) path and keep pool.releaseAll() where
appropriate. Ensure you reference replaceData, updateData, reformatChangedRows,
setSelection, fillMissingFields, pool.releaseAll, disconnectedCallback and init
when making the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 227f37df-fff9-4af0-aef7-f23dd5d07454
📒 Files selected for processing (2)
src/components/table/table.spec.tssrc/components/table/table.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/table/table.tsx (1)
334-340:⚠️ Potential issue | 🟠 MajorAwait
replaceData()before reapplying selection.
replaceData()returns a Promise. CallingsetSelection()immediately can race the row rebuild. This was addressed for theupdateDatapath but thereplaceDatabranch still calls them synchronously.Suggested fix
if (shouldReplace) { this.pool.releaseAll(); - this.tabulator.replaceData(newData); - this.setSelection(); - - return; + this.tabulator.replaceData(newData).then(() => { + if (!this.tabulator) { + return; + } + + this.setSelection(); + }); + return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/table/table.tsx` around lines 334 - 340, The replaceData branch currently calls this.tabulator.replaceData(newData) synchronously and then this.setSelection(), which can race because replaceData() returns a Promise; change the flow to await this.tabulator.replaceData(newData) (making the enclosing method async if it isn't already), call this.pool.releaseAll() as before, then call this.setSelection() only after the awaited replaceData completes so selection is applied after rows are rebuilt (mirror the updateData path behavior).
♻️ Duplicate comments (1)
src/components/table/table.spec.ts (1)
77-89: 🧹 Nitpick | 🔵 TrivialConsider asserting
pool.releaseAll()is called for structural changes.This test verifies
replaceDatais called when IDs change, but doesn't assert thatpool.releaseAll()is invoked before it. Adding this assertion would ensure the pool cleanup behavior is tested.Suggested assertion
const tabulator = (component as any).tabulator; + expect((component as any).pool.releaseAll).toHaveBeenCalled(); expect(tabulator.replaceData).toHaveBeenCalledWith(newData); expect(tabulator.updateData).not.toHaveBeenCalled();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/table/table.spec.ts` around lines 77 - 89, The test should also assert that the pool is cleared when structural changes occur: in the 'replaces data when the dataset changes' spec, mock or spy on the component's pool.releaseAll and assert it was called (and called before invoking tabulator.replaceData) when calling (component as any).updateData(newData, oldData); keep references to the updateData method, the pool.releaseAll spy, and tabulator.replaceData to ensure the test verifies pool cleanup happens prior to replaceData.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/table/table.tsx`:
- Around line 454-466: The function reformatChangedRows currently declares
parameters on multiple lines and uses a traditional for loop; change its
signature to a single-line parameter list (reformatChangedRows(newData:
RowData[], oldData: RowData[])) and replace the indexed loop with a for-of loop
that iterates over newData (e.g., for (const newRow of newData) { ... }) while
finding the corresponding old row (by index or id) and comparing with isEqual;
keep using this.tabulator.getRow(newRow.id) and call row.reformat() when
different.
---
Outside diff comments:
In `@src/components/table/table.tsx`:
- Around line 334-340: The replaceData branch currently calls
this.tabulator.replaceData(newData) synchronously and then this.setSelection(),
which can race because replaceData() returns a Promise; change the flow to await
this.tabulator.replaceData(newData) (making the enclosing method async if it
isn't already), call this.pool.releaseAll() as before, then call
this.setSelection() only after the awaited replaceData completes so selection is
applied after rows are rebuilt (mirror the updateData path behavior).
---
Duplicate comments:
In `@src/components/table/table.spec.ts`:
- Around line 77-89: The test should also assert that the pool is cleared when
structural changes occur: in the 'replaces data when the dataset changes' spec,
mock or spy on the component's pool.releaseAll and assert it was called (and
called before invoking tabulator.replaceData) when calling (component as
any).updateData(newData, oldData); keep references to the updateData method, the
pool.releaseAll spy, and tabulator.replaceData to ensure the test verifies pool
cleanup happens prior to replaceData.
🪄 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: 9bcd9fae-5355-4845-9992-5f66265495fd
📒 Files selected for processing (2)
src/components/table/table.spec.tssrc/components/table/table.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/components/table/table.tsx (1)
334-338:⚠️ Potential issue | 🟠 MajorAwait
replaceData()before reapplying selection.At Line 336,
replaceData()is async; callingsetSelection()at Line 337 can race table rebuild and lose selection state.Suggested fix
if (shouldReplace) { this.pool.releaseAll(); - this.tabulator.replaceData(newData); - this.setSelection(); + this.tabulator.replaceData(newData).then(() => { + if (!this.tabulator || this.destroyed) { + return; + } + this.setSelection(); + }); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/table/table.tsx` around lines 334 - 338, The call to this.tabulator.replaceData(newData) is asynchronous and setSelection() is being invoked immediately after, causing a race; update the surrounding method (the function containing the shouldReplace branch) to be async and await this.tabulator.replaceData(newData) before calling this.setSelection(), keeping this.pool.releaseAll() before the await so the sequence is: releaseAll(), await replaceData(...), then setSelection(); ensure any callers handle the method's async signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/table/table.tsx`:
- Around line 456-457: The condition currently uses a truthy check
(!!newRow?.id) which excludes valid IDs like 0 or '' and prevents reformatting;
change the guard in the block that compares newRow to oldData (around newRow,
oldData, isEqual and this.tabulator.getRow) to explicitly check for
non-null/undefined IDs (e.g. newRow?.id != null) before calling
this.tabulator.getRow(newRow.id) so rows with ID 0 or empty-string are handled
correctly.
- Around line 344-351: The updateData() promise lacks a rejection handler;
update the call to this.tabulator.updateData(patched) to add a .catch(...) that
checks this.destroyed before doing anything, logs or ignores the error as
appropriate, and ensures the reconciliation steps
(this.reformatChangedRows(newData, oldData) and this.setSelection()) only run in
the successful .then branch. Keep the .then(...) block unchanged but append a
.catch(error => { if (!this.destroyed) { /* handle or log error */ } }) to
prevent unhandled promise rejections.
---
Duplicate comments:
In `@src/components/table/table.tsx`:
- Around line 334-338: The call to this.tabulator.replaceData(newData) is
asynchronous and setSelection() is being invoked immediately after, causing a
race; update the surrounding method (the function containing the shouldReplace
branch) to be async and await this.tabulator.replaceData(newData) before calling
this.setSelection(), keeping this.pool.releaseAll() before the await so the
sequence is: releaseAll(), await replaceData(...), then setSelection(); ensure
any callers handle the method's async signature.
🪄 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: a0349d9e-b3d8-4128-9d2f-23b6b583f11d
📒 Files selected for processing (1)
src/components/table/table.tsx
Comment on lines
+344
to
+351
| this.tabulator.updateData(patched).then(() => { | ||
| if (!this.tabulator) { | ||
| return; | ||
| } | ||
|
|
||
| this.reformatChangedRows(newData, oldData); | ||
| this.setSelection(); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file and the specific lines mentioned
cd src/components/table && wc -l table.tsx && echo "---" && sed -n '340,360p' table.tsxRepository: Lundalogik/lime-elements
Length of output: 680
🏁 Script executed:
# Also search for the tabulator.updateData pattern to see if it's used elsewhere
rg "updateData\(" src/components/table/table.tsx -B 2 -A 5Repository: Lundalogik/lime-elements
Length of output: 939
🏁 Script executed:
# Check for the destroyed property definition and other promise handling
rg "destroyed" src/components/table/table.tsx -B 1 -A 1Repository: Lundalogik/lime-elements
Length of output: 806
🏁 Script executed:
# Look for other .catch() patterns in the component to understand error handling strategy
rg "\.catch\(" src/components/table/table.tsx -B 2 -A 2Repository: Lundalogik/lime-elements
Length of output: 50
🏁 Script executed:
# Let's see what updateOrAddData does and check the broader context
rg "updateOrAddData|\.then\(" src/components/table/table.tsx -B 1 -A 3Repository: Lundalogik/lime-elements
Length of output: 432
🏁 Script executed:
# Look at the full method signature and context around line 344
sed -n '325,370p' src/components/table/table.tsxRepository: Lundalogik/lime-elements
Length of output: 1346
🏁 Script executed:
# Check if there's any global error handling or error listeners in the component
rg "onerror|error|reject" src/components/table/table.tsx -i | head -20Repository: Lundalogik/lime-elements
Length of output: 104
🏁 Script executed:
# Understand the tabulator library - search for its type definitions or imports
rg "import.*tabulator|from.*tabulator" src/components/table/table.tsx -iRepository: Lundalogik/lime-elements
Length of output: 143
Add rejection handler to updateData() promise to prevent unhandled promise rejections.
The promise chain at line 344 lacks a rejection handler. If Tabulator's updateData() rejects, it will surface as an unhandled promise rejection, preventing the reconciliation logic from running. Add a .catch() handler to handle rejection gracefully, consistent with the component's existing pattern of checking the destroyed property before operations.
Suggested fix
- this.tabulator.updateData(patched).then(() => {
- if (!this.tabulator) {
- return;
- }
-
- this.reformatChangedRows(newData, oldData);
- this.setSelection();
- });
+ this.tabulator
+ .updateData(patched)
+ .then(() => {
+ if (!this.tabulator) {
+ return;
+ }
+
+ this.reformatChangedRows(newData, oldData);
+ this.setSelection();
+ })
+ .catch(() => {
+ if (!this.tabulator || this.destroyed) {
+ return;
+ }
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/table/table.tsx` around lines 344 - 351, The updateData()
promise lacks a rejection handler; update the call to
this.tabulator.updateData(patched) to add a .catch(...) that checks
this.destroyed before doing anything, logs or ignores the error as appropriate,
and ensures the reconciliation steps (this.reformatChangedRows(newData, oldData)
and this.setSelection()) only run in the successful .then branch. Keep the
.then(...) block unchanged but append a .catch(error => { if (!this.destroyed) {
/* handle or log error */ } }) to prevent unhandled promise rejections.
Comment on lines
+456
to
+457
| if (!!newRow?.id && !isEqual(newRow, oldData[i])) { | ||
| const row = this.tabulator.getRow(newRow.id); |
Contributor
There was a problem hiding this comment.
Don’t use a truthy check for row IDs.
Line 456 uses !!newRow?.id, which skips valid IDs like 0 (and ''). Those rows will not be reformatted even when content changes.
Suggested fix
- if (!!newRow?.id && !isEqual(newRow, oldData[i])) {
+ if (newRow?.id != null && !isEqual(newRow, oldData[i])) {
const row = this.tabulator.getRow(newRow.id);📝 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.
Suggested change
| if (!!newRow?.id && !isEqual(newRow, oldData[i])) { | |
| const row = this.tabulator.getRow(newRow.id); | |
| if (newRow?.id != null && !isEqual(newRow, oldData[i])) { | |
| const row = this.tabulator.getRow(newRow.id); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/table/table.tsx` around lines 456 - 457, The condition
currently uses a truthy check (!!newRow?.id) which excludes valid IDs like 0 or
'' and prevents reformatting; change the guard in the block that compares newRow
to oldData (around newRow, oldData, isEqual and this.tabulator.getRow) to
explicitly check for non-null/undefined IDs (e.g. newRow?.id != null) before
calling this.tabulator.getRow(newRow.id) so rows with ID 0 or empty-string are
handled correctly.
FredrikWallstrom
approved these changes
Apr 9, 2026
Use updateData + row.reformat() on changed rows instead of replaceData for row content changes. This reduces table update cost from O(N) to O(k) where k is the number of changed rows. - fillMissingFields patches fields dropped by the backend as undefined, so updateData clears them from Tabulator's internal data model - After updateData resolves, reformatChangedRows calls row.reformat() only on rows where content actually differs, forcing custom component formatters to re-run with the updated data - replaceData is still used when row IDs or order change Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Expect updateData (not replaceData) for row content changes - Assert pool.releaseAll is not called for content-only changes - Add test for fillMissingFields patching dropped fields as undefined - Mock updateData to return a Promise and getRow to return reformat Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Befkadu1
force-pushed
the
fix/table-replace-data-on-row-updates
branch
from
cff202d to
331c2b4
Compare
Collaborator
|
🎉 This PR is included in version 39.12.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
updateData+row.reformat()on changed rows instead ofreplaceDatawhen the table detects row content changes (same IDs, same order, but different field values)undefinedbefore updating so Tabulator clears stale values from its internal data modelreplaceDataonly for structural changes (different IDs or reordered rows)Why this change?
Tabulator's
updateData()performs a partial merge: it matches rows by their index field and patches only the supplied properties. This causes two problems:Deleted properties are not removed — Internally,
row.updateDataonly iterates over fields present in the new data (for (let attrname in updatedData)). If a field was dropped from the API response (e.g. due to ACL filtering),updateDatapreserves the old value because it never visits that cell.Custom components may not re-render —
updateDataguards cell re-renders withcell.getValue() !== value(strict comparison). Custom component cells rendered via theElementPoolmay not reliably re-render through this path alone.How we solve both problems without
replaceDatafillMissingFields— Before callingupdateData, we compare old and new row data and explicitly set any missing fields toundefined. This ensuresupdateDatavisits those cells and clears the stale values from Tabulator's internal data model.row.reformat()— AfterupdateDataresolves (data model is now correct), we callrow.reformat()only on rows where content actually changed. This forces Tabulator to re-run all formatters for that row, including custom component formatters that read from the now-updated row data.Example scenario
A table displays helpdesk tickets with a
conversation.earliestunreadcolumn. When a user marks a conversation as read, the parent component fetches updated data and passes it to the table. The row IDs and order haven't changed — only the field value has. With the old code,updateDataalone either missed the removed field or didn't re-render the custom component icon. With this fix,fillMissingFieldspatches the dropped field,updateDataclears it in the data model, andreformat()forces the icon component to re-render.Performance: why
replaceDatadoesn't scaleThe previous approach used
replaceDatafor any row content change.replaceDatatears down all rows and rebuilds them — every cell, every formatter, every custom component. Consider a table with 500 rows and 100 columns where a user edits a single cell:replaceData(all 500 rows)updateData+reformat(2 changed rows)That's 250x less work for the common case of a few rows changing.
Even with pagination (100 rows/page),
replaceDatastill processes 10,000 cells per update. For tables without pagination (e.g. embedded tables, dashboards), the cost is unbounded.Cost breakdown of
replaceDataat scaleEach cell rebuild involves:
pool.get()+Object.assign(element, props)+ResizeObserversetupAt 500 rows × 100 columns, that's 50,000 formatter calls, up to 15,000
ResizeObserversetups (for 30 custom component columns), and a full DOM reflow. This easily exceeds a frame budget and causes visible jank.When
replaceDatais still usedreplaceData+pool.releaseAll()is reserved for structural changes where surgical updates aren't possible:In these cases, Tabulator can't match old rows to new rows by ID, so a full rebuild is the only option.
Linked issue
Closes Lundalogik/crm-client#801
Test plan
conversation.earliestunread) update correctly after changes made through related cardsfillMissingFieldsunit testreplaceData(covered by existing tests)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Refactor
Tests