Conversation
📝 WalkthroughWalkthroughAdds row drag-and-drop reordering to the table: new RowDragManager class, drag-handle styles, a Changes
Sequence Diagram(s)sequenceDiagram
participant Table as "Table Component\n(limel-table)"
participant Tabulator as "Tabulator"
participant RowMgr as "RowDragManager"
participant Pool as "ElementPool"
participant Emitter as "reorder EventEmitter"
Table->>RowMgr: init(pool, reorderEmitter, language)
RowMgr->>Pool: get('limel-drag-handle')
Pool-->>RowMgr: HTMLElement
RowMgr-->>Table: provide rowHeader formatter (uses element + setElementProperties)
Table->>Tabulator: create(options including rowHeader)
Note over Tabulator,RowMgr: User drags a row
Tabulator->>RowMgr: rowMoved(row)
RowMgr->>Tabulator: inspect prev/next rows
RowMgr->>Emitter: emit RowReorderEvent{fromRow,toRow,above}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
albinhallen
force-pushed
the
feat/table-rows-drag-n-drop
branch
2 times, most recently
from
1c87160 to
778f86f
Compare
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-3983/ |
albinhallen
force-pushed
the
feat/table-rows-drag-n-drop
branch
from
ff87b90 to
2121ff7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/examples/table-movable-rows.tsx`:
- Around line 27-41: The handleReorder method uses items.indexOf(fromRow) and
indexOf(toRow) which relies on reference equality and can fail if rows are
cloned; change to use findIndex comparing a stable identifier (e.g. row.id) for
both fromRow and toRow (use items.findIndex(item => item.id === fromRow.id) and
similarly for toRow), handle the case where a findIndex returns -1 (abort or
no-op), and remove the unnecessary "as Bird" cast since fromRow is already
typed; update references to tableData and RowReorderEvent<Bird> accordingly.
- Around line 46-57: The render method currently returns an array of elements;
replace that array literal with a single <Host> wrapper containing the
<limel-table ... /> and <limel-example-value ... /> elements so StencilJS
guidelines are followed. Locate the return in the render function that returns
the array with limel-table (using props tableData, columns, movableRows and
onReorder pointing to handleReorder) and limel-example-value (using rowOrder)
and wrap those JSX elements in <Host>...</Host> instead of returning an array.
In `@src/components/table/row-drag-manager.spec.ts`:
- Around line 60-80: The tests are for an old implementation; update them to use
the current RowDragManager constructor signature (RowDragManager(pool:
ElementPool, reorderEvent: EventEmitter<RowReorderEvent<any>>, language:
Languages)) and remove the five-parameter setup; create simple mocks for
ElementPool (with get returning an element) and EventEmitter (with emit spy),
instantiate manager with those mocks and 'en', then replace the old
pointer-event tests with tests for getRowHeaderDefinition() (assert
headerSort:false, resizable:false, frozen:true, rowHandle:true,
cssClass:'limel-table--drag-handle') and for handleRowMoved(row) (mock rows via
getData/getPrevRow/getNextRow and assert reorderEvent.emit called with correct
fromRow/toRow and above:true|false). Ensure references to RowDragManager,
getRowHeaderDefinition, and handleRowMoved are used to locate code.
In `@src/components/table/row-drag-manager.ts`:
- Around line 15-19: The constructor parameters in RowDragManager (pool,
reorderEvent, language) are never reassigned and should be made immutable;
update the constructor signature in row-drag-manager.ts to mark these parameters
as readonly (e.g., change "private pool: ElementPool, private reorderEvent:
EventEmitter<RowReorderEvent<any>>, private language: Languages" to use "private
readonly" for each) so the fields are readonly members of the class.
- Around line 14-79: The implementation must match the tests' expected API and
lifecycle: change the RowDragManager constructor signature to (getTableFunc,
getShadowRootFunc, pool, emitter, language) and remove reliance on Tabulator's
rowMoved event (handleRowMoved binding can remain but update usage), then add
the missing public methods getColumnDefinitions(columns), setup(), destroy(),
and setDisabled(disabled) implementing the custom drag strategy the tests expect
(attach pointer event listeners via addEventListener in setup using
getTableFunc/getShadowRootFunc and store handlers to remove in destroy; use pool
and emitter to create/emit drag events instead of Tabulator rowMoved). Ensure
getColumnDefinitions returns a column definition that renders the
limel-drag-handle (reuse getDragHandleFormatter), setup registers
pointerdown/pointermove/pointerup listeners and setDisabled toggles
adding/removing listeners or a flag to ignore events, and destroy removes all
listeners and clears any references.
🪄 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: 2c207983-4aca-4cb7-b8f2-940053d9a519
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (8)
src/components/table/examples/table-movable-rows.tsxsrc/components/table/partial-styles/_row-drag-handle.scsssrc/components/table/row-drag-manager.spec.tssrc/components/table/row-drag-manager.tssrc/components/table/table.scsssrc/components/table/table.tsxsrc/components/table/table.types.tssrc/components/tooltip/tooltip.tsx
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/row-drag-manager.spec.ts`:
- Around line 64-73: Tests are flaking because the module-level mock
setElementProperties retains call history between tests; add vi.clearAllMocks()
to the test suite setup (inside the existing beforeEach) so each test runs with
a fresh mock state—update the spec's beforeEach to call vi.clearAllMocks()
before invoking any setup that uses manager.getRowHeaderDefinition or other
helpers so the expectation on setElementProperties only reflects calls from the
current test.
🪄 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: fe0162bb-a8a7-41b8-b13f-b9525f14a94e
📒 Files selected for processing (1)
src/components/table/row-drag-manager.spec.ts
albinhallen
force-pushed
the
feat/table-rows-drag-n-drop
branch
2 times, most recently
from
d895486 to
a685c48
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/components/table/examples/table-movable-rows.tsx (1)
34-39:⚠️ Potential issue | 🟡 MinorUse
findIndexfortoIndexlookup and remove unnecessary cast.Line 34 uses
indexOf(toRow)which relies on reference equality and will return-1if Tabulator clones row objects. UsefindIndexconsistently with line 31. Also, theas Birdcast on line 39 is unnecessary sincefromRowis already typed asBird.♻️ Proposed fix
- let toIndex = items.indexOf(toRow); + let toIndex = items.findIndex((bird) => bird.name === toRow.name); if (!above) { toIndex += 1; } - items.splice(toIndex, 0, fromRow as Bird); + items.splice(toIndex, 0, fromRow);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/table/examples/table-movable-rows.tsx` around lines 34 - 39, Replace the indexOf-based lookup and the unnecessary cast: use findIndex like the earlier lookup (e.g., change items.indexOf(toRow) to items.findIndex(r => r.id === toRow.id or the same predicate used at line 31) to avoid reference-equality failures when Tabulator clones rows) and remove the redundant "as Bird" cast on fromRow so items.splice(toIndex, 0, fromRow) is used; ensure the same identification predicate as used elsewhere (line 31) is applied for consistency.
🤖 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/examples/table-movable-rows.tsx`:
- Around line 20-25: The class has members that are never reassigned — mark the
fields as readonly: change the columns declaration to readonly columns:
Array<Column<Bird>> and mark the handler method signature as private readonly
handleReorder (or make the method a readonly arrow property if pattern requires)
so both columns and handleReorder are immutable; update any usage sites (e.g.,
where handleReorder is passed as a callback) to the new readonly member form if
necessary.
- Around line 46-61: In the Host JSX return for the table-movable-rows example
remove the stray commas that are rendered as text nodes: delete the comma after
the <limel-table ... onReorder={this.handleReorder} /> element and the comma
after the <limel-example-value ... /> element so the Host children are just the
two JSX elements (Host, limel-table, limel-example-value, handleReorder).
In `@src/components/table/row-drag-manager.ts`:
- Around line 63-66: The property handleCellClick on the RowDragManager class is
an arrow-function field that is never reassigned; mark it readonly to reflect
immutability. Update the declaration of handleCellClick to be a readonly class
property (e.g., readonly handleCellClick = (ev: Event): void => { ... }) so
static analysis no longer flags it as mutable.
In `@src/components/tooltip/tooltip.tsx`:
- Around line 121-124: The teardown calls this.tooltipTimer.hide() but
TooltipTimer's constructor incorrectly assigns the hide callback to the show
callback, causing hide() to run the wrong function and flip open to true; update
the TooltipTimer constructor in tooltip-timer.ts so the constructor parameters
are stored to the proper fields (assign the hide callback to the hide callback
member and the show callback to the show callback member) and verify
TooltipTimer.hide() invokes the hide callback (and TooltipTimer.show() invokes
the show callback).
---
Duplicate comments:
In `@src/components/table/examples/table-movable-rows.tsx`:
- Around line 34-39: Replace the indexOf-based lookup and the unnecessary cast:
use findIndex like the earlier lookup (e.g., change items.indexOf(toRow) to
items.findIndex(r => r.id === toRow.id or the same predicate used at line 31) to
avoid reference-equality failures when Tabulator clones rows) and remove the
redundant "as Bird" cast on fromRow so items.splice(toIndex, 0, fromRow) is
used; ensure the same identification predicate as used elsewhere (line 31) is
applied for consistency.
🪄 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: 28711bfd-d459-4bd1-ab0a-ab533ca76632
📒 Files selected for processing (6)
src/components/table/examples/table-movable-rows.tsxsrc/components/table/partial-styles/_row-drag-handle.scsssrc/components/table/row-drag-manager.spec.tssrc/components/table/row-drag-manager.tssrc/components/table/table.tsxsrc/components/tooltip/tooltip.tsx
| public disconnectedCallback() { | ||
| this.removeListeners(); | ||
| this.tooltipTimer.hide(); | ||
| } |
There was a problem hiding this comment.
Line 123 now deterministically triggers the wrong callback during teardown
this.tooltipTimer.hide() (Line 123) calls TooltipTimer.hide(), and in src/components/tooltip/tooltip-timer.ts the constructor currently assigns hideCallback to showCallback. That means teardown can set open to true, which is a functional regression.
💡 Proposed fix (root cause in tooltip-timer.ts)
--- a/src/components/tooltip/tooltip-timer.ts
+++ b/src/components/tooltip/tooltip-timer.ts
@@
constructor(
showCallback: Function,
hideCallback: Function,
delayForShowing: number = DEFAULT_DELAY_FOR_SHOWING
) {
this.showCallback = showCallback;
- this.hideCallback = showCallback;
+ this.hideCallback = hideCallback;
this.delayForShowing = delayForShowing;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/tooltip/tooltip.tsx` around lines 121 - 124, The teardown
calls this.tooltipTimer.hide() but TooltipTimer's constructor incorrectly
assigns the hide callback to the show callback, causing hide() to run the wrong
function and flip open to true; update the TooltipTimer constructor in
tooltip-timer.ts so the constructor parameters are stored to the proper fields
(assign the hide callback to the hide callback member and the show callback to
the show callback member) and verify TooltipTimer.hide() invokes the hide
callback (and TooltipTimer.show() invokes the show callback).
There was a problem hiding this comment.
Hallucination? I doesn't look like that
There was a problem hiding this comment.
@albinhallen could you please make a separate PR for this change; and explain the issue in that PR? 🙏
I'm guessing you are changing this because the tooltip remains after drag and drop or something like that. But that's a separate problem and it's better to be handled in a separate PR
There was a problem hiding this comment.
If my assumption is correct about why you are doing it, you can test and use this example in limel-form for that secondary PR
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)
316-319:⚠️ Potential issue | 🟠 MajorRecreate the drag manager when its prerequisites change.
pageSizeChanged()only updates max page, and there are no@Watch('mode')/@Watch('language')hooks, even thoughinitRowDragManager()depends on all three props. That means a live table can keep row dragging enabled after pagination or remote mode is turned on, stay disabled after those constraints are removed, and keep using the old drag-handle language afterlanguagechanges.Also applies to: 444-449, 608-623
🤖 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 316 - 319, The pageSizeChanged handler only calls updateMaxPage but does not recreate the row drag manager even though initRowDragManager depends on pageSize, mode and language; add `@Watch`('mode') and `@Watch`('language') handlers and update pageSizeChanged to also recreate the drag manager (or add a shared helper like recreateRowDragManager called by all three watchers) so that changes to pageSize, mode or language tear down any existing manager (call an existing destroyRowDragManager or null-check and remove handlers) and then call initRowDragManager() to reinitialize with the current props; reference initRowDragManager, pageSizeChanged, and the new Watch('mode')/Watch('language') methods when making the change.
♻️ Duplicate comments (1)
src/components/table/examples/table-movable-rows.tsx (1)
27-40:⚠️ Potential issue | 🟡 MinorUse the same stable lookup for
toRowasfromRow.
fromRowalready avoids reference equality, buttoRowstill usesindexOf(). If the event detail is equivalent but not the exact same object instance,toIndexbecomes-1and this example inserts in the wrong place. Guard the lookup failure as well, and drop the redundant cast.♻️ Suggested update
private handleReorder = (event: CustomEvent<RowReorderEvent<Bird>>) => { const { fromRow, toRow, above } = event.detail; const items = [...this.tableData]; const fromIndex = items.findIndex((bird) => bird.name === fromRow.name); + if (fromIndex < 0) { + return; + } items.splice(fromIndex, 1); - let toIndex = items.indexOf(toRow); + let toIndex = items.findIndex((bird) => bird.name === toRow.name); + if (toIndex < 0) { + return; + } if (!above) { toIndex += 1; } - items.splice(toIndex, 0, fromRow as Bird); + items.splice(toIndex, 0, fromRow); this.tableData = items; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/table/examples/table-movable-rows.tsx` around lines 27 - 40, The handleReorder method uses a stable lookup for fromRow but uses indexOf(toRow) which can fail if toRow is an equivalent object rather than the same instance; change the toIndex lookup to the same stable findIndex strategy (e.g., match by unique key such as bird.name) and guard if findIndex returns -1 before adjusting for above, and remove the redundant "as Bird" cast when splicing fromRow into items; update references to fromRow, toRow, items and this.tableData in handleReorder accordingly.
🤖 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/partial-styles/_row-drag-handle.scss`:
- Line 1: The selector .limel-table--drag-handle is a BEM-style modifier used
inside a shadow-DOM component; rename this class to a small local non-BEM name
(e.g., .row-drag-handle or .dragHandle) in _row-drag-handle.scss and update all
places that reference it (the component's cssClass usage and any spec
assertions/tests) within the same PR so the style and tests stay in sync; ensure
you only change the class name string, run tests, and verify the shadowed
component still applies the class where needed.
---
Outside diff comments:
In `@src/components/table/table.tsx`:
- Around line 316-319: The pageSizeChanged handler only calls updateMaxPage but
does not recreate the row drag manager even though initRowDragManager depends on
pageSize, mode and language; add `@Watch`('mode') and `@Watch`('language') handlers
and update pageSizeChanged to also recreate the drag manager (or add a shared
helper like recreateRowDragManager called by all three watchers) so that changes
to pageSize, mode or language tear down any existing manager (call an existing
destroyRowDragManager or null-check and remove handlers) and then call
initRowDragManager() to reinitialize with the current props; reference
initRowDragManager, pageSizeChanged, and the new Watch('mode')/Watch('language')
methods when making the change.
---
Duplicate comments:
In `@src/components/table/examples/table-movable-rows.tsx`:
- Around line 27-40: The handleReorder method uses a stable lookup for fromRow
but uses indexOf(toRow) which can fail if toRow is an equivalent object rather
than the same instance; change the toIndex lookup to the same stable findIndex
strategy (e.g., match by unique key such as bird.name) and guard if findIndex
returns -1 before adjusting for above, and remove the redundant "as Bird" cast
when splicing fromRow into items; update references to fromRow, toRow, items and
this.tableData in handleReorder accordingly.
🪄 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: 223ca053-66d9-4836-bb55-a661f536c5a7
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
src/components/table/examples/table-movable-rows.tsxsrc/components/table/partial-styles/_row-drag-handle.scsssrc/components/table/row-drag-manager.spec.tssrc/components/table/row-drag-manager.tssrc/components/table/table.tsxsrc/components/tooltip/tooltip.tsx
albinhallen
force-pushed
the
feat/table-rows-drag-n-drop
branch
2 times, most recently
from
c964863 to
3d1f294
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/components/table/examples/table-movable-rows.tsx (1)
27-40:⚠️ Potential issue | 🟡 MinorStop using reference equality for the drop target.
Line 34 still uses
items.indexOf(toRow). If the event detail is not the same object instance stored intableData,indexOfreturns-1and the demo reorders incorrectly. Match both rows by the same stable field and move the existing array item instead of castingfromRow.Suggested fix
private handleReorder = (event: CustomEvent<RowReorderEvent<Bird>>) => { const { fromRow, toRow, above } = event.detail; const items = [...this.tableData]; const fromIndex = items.findIndex((bird) => bird.name === fromRow.name); - items.splice(fromIndex, 1); + if (fromIndex === -1) { + return; + } + + const [movedBird] = items.splice(fromIndex, 1); - let toIndex = items.indexOf(toRow); + let toIndex = items.findIndex((bird) => bird.name === toRow.name); + if (toIndex === -1) { + return; + } + if (!above) { toIndex += 1; } - items.splice(toIndex, 0, fromRow as Bird); + items.splice(toIndex, 0, movedBird); this.tableData = items; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/table/examples/table-movable-rows.tsx` around lines 27 - 40, The handler handleReorder uses reference equality (items.indexOf(toRow)) and casts event.detail.fromRow which breaks if the event provides different object instances; instead locate both indices with a stable key (e.g., use items.findIndex(b => b.name === fromRow.name) and items.findIndex(b => b.name === toRow.name)), remove the actual item from items (store it from items.splice(fromIndex,1)[0]) and insert that stored item at the computed toIndex (adjusting when above is false) before assigning this.tableData = items.
🤖 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/row-drag-manager.spec.ts`:
- Around line 32-154: Add a browser/e2e Vitest+Playwright test that performs a
real drag gesture on the rendered limel-table and asserts that the component
emits the 'reorder' event (this covers DOM wiring missing from RowDragManager
unit tests). Create a new spec that mounts limel-table with at least three rows,
simulate pointerdown + pointermove + pointerup (or use Playwright drag and drop)
on the row drag handle element (CSS class 'limel-table-drag-handle'), and listen
for the component's 'reorder' event to verify payload (fromRow/toRow/above)
matches the expected reorder; reference the RowDragManager behavior in
assertions to ensure parity with the existing unit tests. Ensure the test runs
in the existing Vitest Browser Mode configuration and uses the same mock data
shape as the unit tests (e.g., rows with id/name) so the emitted event structure
matches what handleRowMoved produces.
In `@src/components/table/table.tsx`:
- Around line 444-449: The row-drag state is only refreshed in the movableRows
watcher (updateMovableRows) so changes to mode or pageSize can leave
rowDragManager stale; update watchers for mode and pageSize (or centralize in a
method) to perform the same work as updateMovableRows: set this.rowDragManager =
null, call this.initRowDragManager(), and this.init() so drag handles are
re-evaluated whenever mode or pageSize changes; locate updateMovableRows,
initRowDragManager, and init to implement the same refresh logic or call a
shared helper from the mode and pageSize watchers.
---
Duplicate comments:
In `@src/components/table/examples/table-movable-rows.tsx`:
- Around line 27-40: The handler handleReorder uses reference equality
(items.indexOf(toRow)) and casts event.detail.fromRow which breaks if the event
provides different object instances; instead locate both indices with a stable
key (e.g., use items.findIndex(b => b.name === fromRow.name) and
items.findIndex(b => b.name === toRow.name)), remove the actual item from items
(store it from items.splice(fromIndex,1)[0]) and insert that stored item at the
computed toIndex (adjusting when above is false) before assigning this.tableData
= items.
🪄 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: d33aadcd-10b3-42ef-873d-4564f5c3586d
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
src/components/table/examples/table-movable-rows.tsxsrc/components/table/partial-styles/_row-drag-handle.scsssrc/components/table/row-drag-manager.spec.tssrc/components/table/row-drag-manager.tssrc/components/table/table.tsxsrc/components/tooltip/tooltip.tsx
| describe('RowDragManager', () => { | ||
| let manager: RowDragManager; | ||
| let mockPool: ReturnType<typeof createMockPool>; | ||
| let mockEmitter: ReturnType<typeof createMockEventEmitter>; | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| mockPool = createMockPool(); | ||
| mockEmitter = createMockEventEmitter(); | ||
|
|
||
| manager = new RowDragManager(mockPool as any, mockEmitter as any, 'en'); | ||
| }); | ||
|
|
||
| describe('getRowHeaderDefinition', () => { | ||
| it('returns a column definition with correct properties', () => { | ||
| const definition = manager.getRowHeaderDefinition() as any; | ||
|
|
||
| expect(definition.headerSort).toBe(false); | ||
| expect(definition.resizable).toBe(false); | ||
| expect(definition.frozen).toBe(true); | ||
| expect(definition.rowHandle).toBe(true); | ||
| expect(definition.cssClass).toEqual('limel-table-drag-handle'); | ||
| }); | ||
|
|
||
| it('provides a formatter that uses the element pool', () => { | ||
| const definition = manager.getRowHeaderDefinition() as any; | ||
| const formatter = definition.formatter as () => HTMLElement; | ||
|
|
||
| const element = formatter(); | ||
| expect(mockPool.get).toHaveBeenCalledWith('limel-drag-handle'); | ||
| expect(element).toBeTruthy(); | ||
| }); | ||
|
|
||
| it('sets drag handle properties via setElementProperties', () => { | ||
| const definition = manager.getRowHeaderDefinition() as any; | ||
| const formatter = definition.formatter as () => HTMLElement; | ||
|
|
||
| formatter(); | ||
| expect(setElementProperties).toHaveBeenCalledWith( | ||
| expect.any(HTMLElement), | ||
| { dragDirection: 'vertical', language: 'en' } | ||
| ); | ||
| }); | ||
|
|
||
| it('provides a cellClick handler that stops propagation', () => { | ||
| const definition = manager.getRowHeaderDefinition() as any; | ||
| const event = { | ||
| stopPropagation: vi.fn(), | ||
| preventDefault: vi.fn(), | ||
| }; | ||
|
|
||
| definition.cellClick(event); | ||
|
|
||
| expect(event.stopPropagation).toHaveBeenCalled(); | ||
| expect(event.preventDefault).toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('handleRowMoved', () => { | ||
| it('emits reorder event with above=false when row has a previous row', () => { | ||
| const prevRow = createMockRow({ id: 1, name: 'Alice' }); | ||
| const movedRow = createMockRow( | ||
| { id: 2, name: 'Bob' }, | ||
| prevRow, | ||
| false | ||
| ); | ||
|
|
||
| manager.handleRowMoved(movedRow as any); | ||
|
|
||
| expect(mockEmitter.emit).toHaveBeenCalledWith({ | ||
| fromRow: { id: 2, name: 'Bob' }, | ||
| toRow: { id: 1, name: 'Alice' }, | ||
| above: false, | ||
| }); | ||
| }); | ||
|
|
||
| it('emits reorder event with above=true when row is first (no previous row)', () => { | ||
| const nextRow = createMockRow({ id: 2, name: 'Bob' }); | ||
| const movedRow = createMockRow( | ||
| { id: 1, name: 'Alice' }, | ||
| false, | ||
| nextRow | ||
| ); | ||
|
|
||
| manager.handleRowMoved(movedRow as any); | ||
|
|
||
| expect(mockEmitter.emit).toHaveBeenCalledWith({ | ||
| fromRow: { id: 1, name: 'Alice' }, | ||
| toRow: { id: 2, name: 'Bob' }, | ||
| above: true, | ||
| }); | ||
| }); | ||
|
|
||
| it('does not emit when row has no neighbors', () => { | ||
| const movedRow = createMockRow( | ||
| { id: 1, name: 'Alice' }, | ||
| false, | ||
| false | ||
| ); | ||
|
|
||
| manager.handleRowMoved(movedRow as any); | ||
|
|
||
| expect(mockEmitter.emit).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('prefers previous row over next row for positioning', () => { | ||
| const prevRow = createMockRow({ id: 1, name: 'Alice' }); | ||
| const nextRow = createMockRow({ id: 3, name: 'Charlie' }); | ||
| const movedRow = createMockRow( | ||
| { id: 2, name: 'Bob' }, | ||
| prevRow, | ||
| nextRow | ||
| ); | ||
|
|
||
| manager.handleRowMoved(movedRow as any); | ||
|
|
||
| expect(mockEmitter.emit).toHaveBeenCalledWith({ | ||
| fromRow: { id: 2, name: 'Bob' }, | ||
| toRow: { id: 1, name: 'Alice' }, | ||
| above: false, | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add one browser-level reorder test.
These cases validate RowDragManager in isolation, but they never prove that limel-table emits reorder after a real drag gesture. One browser/e2e case around the table component would cover the DOM wiring that this suite cannot.
Based on learnings, the e2e test project already uses Vitest Browser Mode with Playwright, so this interaction can be covered in-browser.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/table/row-drag-manager.spec.ts` around lines 32 - 154, Add a
browser/e2e Vitest+Playwright test that performs a real drag gesture on the
rendered limel-table and asserts that the component emits the 'reorder' event
(this covers DOM wiring missing from RowDragManager unit tests). Create a new
spec that mounts limel-table with at least three rows, simulate pointerdown +
pointermove + pointerup (or use Playwright drag and drop) on the row drag handle
element (CSS class 'limel-table-drag-handle'), and listen for the component's
'reorder' event to verify payload (fromRow/toRow/above) matches the expected
reorder; reference the RowDragManager behavior in assertions to ensure parity
with the existing unit tests. Ensure the test runs in the existing Vitest
Browser Mode configuration and uses the same mock data shape as the unit tests
(e.g., rows with id/name) so the emitted event structure matches what
handleRowMoved produces.
Kiarokh
force-pushed
the
feat/table-rows-drag-n-drop
branch
from
3d1f294 to
62ab3ad
Compare
| import { capitalize } from 'lodash-es'; | ||
|
|
||
| /** | ||
| * Movable rows |
There was a problem hiding this comment.
Screen.Recording.2026-04-01.at.12.02.46.mov
Something isn't right. Scrolling on the page creates this laggy weird behaviour. I'm gonna just post this for now, until we can identify where the issue comes from.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/components/table/table.tsx (1)
444-449:⚠️ Potential issue | 🟠 MajorRe-evaluate row dragging when
pageSizeormodechanges.
initRowDragManager()correctly gates the feature on pagination and mode, but the manager is only rebuilt fromupdateMovableRows(). IfmovableRowsis alreadytrueand the parent later enables pagination or switches toremote, the table keeps the stale drag configuration until a full remount.♻️ Proposed fix
`@Watch`('pageSize') protected pageSizeChanged() { this.updateMaxPage(); + this.refreshRowDragManager(); } +@Watch('mode') +protected modeChanged() { + this.refreshRowDragManager(); +} + `@Watch`('movableRows') protected updateMovableRows() { + this.refreshRowDragManager(); +} + +private refreshRowDragManager() { this.rowDragManager = null; this.initRowDragManager(); this.init(); }Also applies to: 608-623
🤖 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 444 - 449, The table's row-drag manager is only rebuilt in updateMovableRows(), so changes to pageSize or mode leave a stale configuration; add watchers for the pageSize and mode properties (or have their existing watchers call the same logic) so that on changes you reset this.rowDragManager = null and call this.initRowDragManager(); this.init(); — i.e., ensure that the watchers for pageSize and mode invoke the same update flow as updateMovableRows() (referencing updateMovableRows, initRowDragManager, init, and rowDragManager).
🤖 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 444-449: The table's row-drag manager is only rebuilt in
updateMovableRows(), so changes to pageSize or mode leave a stale configuration;
add watchers for the pageSize and mode properties (or have their existing
watchers call the same logic) so that on changes you reset this.rowDragManager =
null and call this.initRowDragManager(); this.init(); — i.e., ensure that the
watchers for pageSize and mode invoke the same update flow as
updateMovableRows() (referencing updateMovableRows, initRowDragManager, init,
and rowDragManager).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4f406521-e32f-4951-a019-0a5de4fab28a
⛔ Files ignored due to path filters (2)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.mdpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
src/components/table/examples/table-movable-rows.tsxsrc/components/table/partial-styles/_row-drag-handle.scsssrc/components/table/row-drag-manager.spec.tssrc/components/table/row-drag-manager.tssrc/components/table/table.scsssrc/components/table/table.tsxsrc/components/table/table.types.tssrc/components/tooltip/tooltip.tsx
| value={rowOrder} | ||
| /> | ||
| </Host> | ||
| ); |
There was a problem hiding this comment.
I won't do a review about the technical changes. But this change one does not belong to this commit. It makes it hard for the reviewer to review the commit itself. It should have been rather squashed to the first commit.
I'd leave it up to you whether you want to have one commit first, relying on Tabulator's inbuilt stuff, and a second commit later, for using Lime Element's inbuilt drag and drop mechanism.
| console.warn( | ||
| 'Row dragging is not supported in remote mode. Please set `mode` to `local` to enable row dragging.' | ||
| ); | ||
| return; |
There was a problem hiding this comment.
if this approach is valid or inevitable, the consumer facing documentations (example's docs) should explain it too. Console warning is good, but not enough.
I also see that you mentioned it in the movableRows prop's docs. 👍 But the actual example should explain why this is the case (only works only with local and when no pagination).
For a "user" however, it might be strange if they suddenly lose their ability to sort manually. This feature in other words means for them that the table is sorted manually, not based on a column's values. So why should it matter? Can't I just save the table with my own sorting anymore, just because since yesterday, one new row was added and the table got a pagination…
Note that the user can still click the header of a column and resort it, then drag and drop rows. So I'm a bit confused about why we offer this feature, and how consumers or users expect to use it.
There was a problem hiding this comment.
For a "user" however, it might be strange if they suddenly lose their ability to sort manually. This feature in other words means for them that the table is sorted manually, not based on a column's values. So why should it matter? Can't I just save the table with my own sorting anymore, just because since yesterday, one new row was added and the table got a pagination…
Note that the user can still click the header of a column and resort it, then drag and drop rows. So I'm a bit confused about why we offer this feature, and how consumers or users expect to use it.
Totally agree about the use case. In forms, we have the sorting disabled so for specifically that use case it makes sense. Since then the order of which items should appear is inferred from the "moving"/sorting.
In that case it's also not necessarily about just displaying data, but also managing/configuring the order of data. It feels like in 99% of cases tables ar used for just displaying data, so that's why it's kind of a weird one :)
We've discussed using a List component instead, but feel we get more structure with a table.
There was a problem hiding this comment.
yes, I don't think list is the right choice. But we should at least properly add documentations.
Also if this is an absolute impossibility, the component should by itself force-disable those features, once this prop is set to true. Otherwise we can get conflicting behaviors.
However, I don't know why a user shouldn't be able to sort items manually, inside each page of a table separately. I can imagine legit use cases for that. Can't the table emit the new sorting, and the consumer store that somewhere? If pagination is a problem, then the consumer should decide on their own and disable it.
I don't think it's a good idea that the table does it for them.
Kiarokh
force-pushed
the
feat/table-rows-drag-n-drop
branch
from
eccee22 to
aa3423e
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (4)
src/components/table/examples/table-movable-rows.tsx (2)
20-27: 🧹 Nitpick | 🔵 TrivialMark
columnsandhandleReorderasreadonly.Per static analysis, these members are never reassigned.
♻️ Proposed fix
- private columns: Array<Column<Bird>> = [ + private readonly columns: Array<Column<Bird>> = [ { title: 'Name', field: 'name' }, { title: 'Binominal name', field: 'binominalName' }, { title: 'Nest type', field: 'nest', formatter: capitalize }, { title: 'Eggs per clutch', field: 'eggs', horizontalAlign: 'right' }, ]; - private handleReorder = (event: CustomEvent<RowReorderEvent<Bird>>) => { + private readonly handleReorder = (event: CustomEvent<RowReorderEvent<Bird>>) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/table/examples/table-movable-rows.tsx` around lines 20 - 27, Mark the class members that are never reassigned as readonly: add the readonly modifier to the private property columns (private readonly columns: Array<Column<Bird>>) and to the handler method declaration handleReorder (private readonly handleReorder = ...) so the static analyzer knows they are immutable; keep their existing types and implementation unchanged while only adding the readonly keyword to the two symbols columns and handleReorder.
31-39:⚠️ Potential issue | 🟡 MinorInconsistent index lookup and unnecessary cast.
Line 31 correctly uses
findIndexwithnamecomparison to avoid reference equality issues, but Line 34 still usesindexOf(toRow)which relies on reference equality. IftoRowis a clone,indexOfreturns-1and the splice inserts at index 0 (or -1 after the!aboveincrement).Also, the
as Birdcast on Line 39 is unnecessary sincefromRowis already typed asBird.🛡️ Proposed fix
const fromIndex = items.findIndex((bird) => bird.name === fromRow.name); items.splice(fromIndex, 1); - let toIndex = items.indexOf(toRow); + let toIndex = items.findIndex((bird) => bird.name === toRow.name); if (!above) { toIndex += 1; } - items.splice(toIndex, 0, fromRow as Bird); + items.splice(toIndex, 0, fromRow); this.tableData = items;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/table/examples/table-movable-rows.tsx` around lines 31 - 39, The code uses items.indexOf(toRow) which relies on reference equality and can return -1 for cloned rows; change the to-index lookup to use a stable key match like items.findIndex(b => b.name === toRow.name) (or id if available) and remove the unnecessary "as Bird" cast on fromRow; ensure you still adjust toIndex when !above and then perform items.splice(toIndex, 0, fromRow) so both lookups use the same equality strategy.src/components/table/row-drag-manager.ts (1)
63-66: 🧹 Nitpick | 🔵 TrivialMark
handleCellClickasreadonly.Per static analysis, this arrow function property is never reassigned.
♻️ Proposed fix
- private handleCellClick = (ev: Event): void => { + private readonly handleCellClick = (ev: Event): void => { ev.stopPropagation(); ev.preventDefault(); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/table/row-drag-manager.ts` around lines 63 - 66, The handleCellClick arrow property on the RowDragManager class is never reassigned; mark it as readonly to satisfy static analysis by changing its declaration to a readonly property (i.e., add the readonly modifier to the existing private handleCellClick = (ev: Event): void => { ... } declaration) while preserving the function body and type signature.src/components/table/table.tsx (1)
608-623:⚠️ Potential issue | 🟠 MajorRefresh row-drag state when
modeorpageSizechanges.The
initRowDragManagergates the feature on bothmode === 'local'and!this.pageSize, but only themovableRowswatcher triggers re-initialization. IfpageSizeormodechanges after initialization,rowDragManagercan become stale—keeping drag handles enabled in unsupported states, or never enabling them when the table becomes eligible.🛠️ Suggested fix
`@Watch`('pageSize') protected pageSizeChanged() { this.updateMaxPage(); + this.refreshRowDragManager(); } + `@Watch`('mode') + protected modeChanged() { + this.refreshRowDragManager(); + } `@Watch`('movableRows') protected updateMovableRows() { - this.rowDragManager = null; - this.initRowDragManager(); - this.init(); + this.refreshRowDragManager(); } + private refreshRowDragManager() { + this.rowDragManager = null; + this.initRowDragManager(); + this.init(); + }🤖 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 608 - 623, The initRowDragManager logic can become stale when mode or pageSize changes; update the component to re-run that logic whenever mode or pageSize toggles by adding watchers (or reactive effects) that call initRowDragManager, and inside initRowDragManager ensure you destroy/clear this.rowDragManager when conditions fail (e.g., when isRemoteMode() becomes true or pageSize is set) and create a new RowDragManager when movableRows is true, mode becomes local, and pageSize is falsy; reference initRowDragManager, movableRows, pageSize, isRemoteMode(), and rowDragManager to implement these watchers and proper cleanup.
🤖 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/examples/table-movable-rows.tsx`:
- Around line 20-27: Mark the class members that are never reassigned as
readonly: add the readonly modifier to the private property columns (private
readonly columns: Array<Column<Bird>>) and to the handler method declaration
handleReorder (private readonly handleReorder = ...) so the static analyzer
knows they are immutable; keep their existing types and implementation unchanged
while only adding the readonly keyword to the two symbols columns and
handleReorder.
- Around line 31-39: The code uses items.indexOf(toRow) which relies on
reference equality and can return -1 for cloned rows; change the to-index lookup
to use a stable key match like items.findIndex(b => b.name === toRow.name) (or
id if available) and remove the unnecessary "as Bird" cast on fromRow; ensure
you still adjust toIndex when !above and then perform items.splice(toIndex, 0,
fromRow) so both lookups use the same equality strategy.
In `@src/components/table/row-drag-manager.ts`:
- Around line 63-66: The handleCellClick arrow property on the RowDragManager
class is never reassigned; mark it as readonly to satisfy static analysis by
changing its declaration to a readonly property (i.e., add the readonly modifier
to the existing private handleCellClick = (ev: Event): void => { ... }
declaration) while preserving the function body and type signature.
In `@src/components/table/table.tsx`:
- Around line 608-623: The initRowDragManager logic can become stale when mode
or pageSize changes; update the component to re-run that logic whenever mode or
pageSize toggles by adding watchers (or reactive effects) that call
initRowDragManager, and inside initRowDragManager ensure you destroy/clear
this.rowDragManager when conditions fail (e.g., when isRemoteMode() becomes true
or pageSize is set) and create a new RowDragManager when movableRows is true,
mode becomes local, and pageSize is falsy; reference initRowDragManager,
movableRows, pageSize, isRemoteMode(), and rowDragManager to implement these
watchers and proper cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fa50e5a1-2fff-427d-a331-b48922bf65d1
⛔ Files ignored due to path filters (2)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.mdpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
src/components/table/examples/table-movable-rows.tsxsrc/components/table/partial-styles/_row-drag-handle.scsssrc/components/table/row-drag-manager.spec.tssrc/components/table/row-drag-manager.tssrc/components/table/table.scsssrc/components/table/table.tsxsrc/components/table/table.types.tssrc/components/tooltip/tooltip.tsx
| this.rowDragManager = null; | ||
| } | ||
|
|
||
| this.rowDragManager = null; |
There was a problem hiding this comment.
Screen.Recording.2026-04-01.at.14.43.16.mov
there is a suboptimal interaction here: while dragging a row, if you hover the region where all drag handles are located, nothing works. user should drag over the rest of the rows for the reordering to happen correctly
| @@ -1,74 +1,22 @@ | |||
| // common styles are written in `_row-selection.scss` and imported in `table.scss` | |||
| .limel-table-drag-handle { | |||
There was a problem hiding this comment.
I've committed this change as a separate commit, to avoid merge conflicts.
If you decide to squash the two commits of enabling drag and drop in the table, into one commit, this can be squashed to that single commit.
Kiarokh
force-pushed
the
feat/table-rows-drag-n-drop
branch
from
4b7e2cb to
5466fff
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/components/table/table.tsx (1)
316-319:⚠️ Potential issue | 🟠 MajorRefresh row-drag state when
pageSizeormodechanges.
rowDragManageris only rebuilt from themovableRowswatcher. If pagination is turned on later, or the table switches to or from remote mode, the drag handles and rowMoved listener stay in their old state. That leaves row dragging enabled in unsupported configurations, or never enables it once the table becomes eligible.♻️ Suggested fix
`@Watch`('pageSize') protected pageSizeChanged() { this.updateMaxPage(); + this.refreshRowDragManager(); + this.init(); } + +@Watch('mode') +protected modeChanged() { + this.refreshRowDragManager(); + this.init(); +} `@Watch`('movableRows') protected updateMovableRows() { - this.rowDragManager = null; - this.initRowDragManager(); + this.refreshRowDragManager(); this.init(); } + +private refreshRowDragManager() { + this.rowDragManager = null; + this.initRowDragManager(); +}Also applies to: 444-449, 608-623
🤖 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 316 - 319, pageSizeChanged currently only calls updateMaxPage but must also refresh the row-drag state; invoke the same row-drag rebuild/cleanup logic used by the movableRows watcher (e.g., call the method that destroys and rebuilds rowDragManager or a helper like rebuildRowDragManager) so drag handles and listeners are re-evaluated whenever pageSize or mode changes; also apply the same call inside the mode watcher so both pageSizeChanged and the mode watcher mirror the movableRows watcher behavior for rowDragManager.src/components/table/examples/table-movable-rows.tsx (1)
31-39:⚠️ Potential issue | 🟡 Minor
toRowlookup still depends on reference identity.Line 34 still uses
indexOf(toRow), and neither index is guarded. If the event carries a cloned or stale row object,findIndex/indexOfreturns-1,splice(-1, 1)removes the last item, and the example shows the wrong order.♻️ Suggested fix
const fromIndex = items.findIndex((bird) => bird.name === fromRow.name); - items.splice(fromIndex, 1); + if (fromIndex === -1) { + return; + } + + const [movedRow] = items.splice(fromIndex, 1); - let toIndex = items.indexOf(toRow); + let toIndex = items.findIndex((bird) => bird.name === toRow.name); + if (toIndex === -1) { + return; + } if (!above) { toIndex += 1; } - items.splice(toIndex, 0, fromRow as Bird); + items.splice(toIndex, 0, movedRow);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/table/examples/table-movable-rows.tsx` around lines 31 - 39, The current move logic uses reference identity for toRow (items.indexOf(toRow)) which breaks when the event carries a cloned/stale object; replace that indexOf with a findIndex that matches on a unique key (e.g., items.findIndex(b => b.name === toRow.name) or an id property) and guard both fromIndex and toIndex (ensure fromIndex >= 0 before removing, and if toIndex === -1 compute a sensible fallback — e.g., set toIndex = items.length when !above to append or toIndex = 0 when above — before calling splice) so splicing never removes the wrong element.
🤖 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/partial-styles/_row-selection.scss`:
- Around line 20-25: The header select-all checkbox stays at left: 0.375rem
while the selector column (.limel-table--row-selector) is shifted when
.has-movable-rows is present, causing the checkbox to overlap the drag-handle;
update the .select-all rule to also shift when nested inside .has-movable-rows
so it aligns with .limel-table--row-selector (use the same
var(--limel-table-drag-handle-width) offset or calc(var(...) + 0.375rem) if you
need to preserve the existing 0.375rem padding) and ensure you reference
.select-all and .limel-table--row-selector in the patch so the header control
moves in sync with the selector column.
In `@src/components/table/row-drag-manager.ts`:
- Around line 68-77: getDragHandleFormatter() currently calls
this.pool.get(LIMEL_DRAG_HANDLE) and never releases the acquired
limel-drag-handle, causing the pool's used map to grow; either ensure the
element is released back to the pool when the row-header cell is torn down (call
this.pool.release(element) from the row-header cell lifecycle/cleanup path where
the formatter's element is detached) or stop using the pool here and
instantiate/configure the drag-handle outside the pool (create a new
limel-drag-handle in getDragHandleFormatter and remove pool.get usage); update
references in getDragHandleFormatter, pool.get, LIMEL_DRAG_HANDLE and the
row-header cell cleanup code to implement the chosen approach.
---
Duplicate comments:
In `@src/components/table/examples/table-movable-rows.tsx`:
- Around line 31-39: The current move logic uses reference identity for toRow
(items.indexOf(toRow)) which breaks when the event carries a cloned/stale
object; replace that indexOf with a findIndex that matches on a unique key
(e.g., items.findIndex(b => b.name === toRow.name) or an id property) and guard
both fromIndex and toIndex (ensure fromIndex >= 0 before removing, and if
toIndex === -1 compute a sensible fallback — e.g., set toIndex = items.length
when !above to append or toIndex = 0 when above — before calling splice) so
splicing never removes the wrong element.
In `@src/components/table/table.tsx`:
- Around line 316-319: pageSizeChanged currently only calls updateMaxPage but
must also refresh the row-drag state; invoke the same row-drag rebuild/cleanup
logic used by the movableRows watcher (e.g., call the method that destroys and
rebuilds rowDragManager or a helper like rebuildRowDragManager) so drag handles
and listeners are re-evaluated whenever pageSize or mode changes; also apply the
same call inside the mode watcher so both pageSizeChanged and the mode watcher
mirror the movableRows watcher behavior for rowDragManager.
🪄 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: ef1267cb-3a11-469b-8b52-b2b48a7cee97
⛔ Files ignored due to path filters (2)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.mdpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
src/components/table/examples/table-movable-rows.tsxsrc/components/table/partial-styles/_row-drag-handle.scsssrc/components/table/partial-styles/_row-selection.scsssrc/components/table/row-drag-manager.spec.tssrc/components/table/row-drag-manager.tssrc/components/table/table.scsssrc/components/table/table.tsxsrc/components/table/table.types.tssrc/components/tooltip/tooltip.tsx
| private getDragHandleFormatter() { | ||
| return () => { | ||
| const element = this.pool.get(LIMEL_DRAG_HANDLE); | ||
| setElementProperties(element, { | ||
| dragDirection: 'vertical', | ||
| language: this.language, | ||
| }); | ||
|
|
||
| return element; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== RowDragManager pool usage ==="
rg -n -C2 'pool\.(get|release|releaseAll)\(' src/components/table/row-drag-manager.ts
echo
echo "=== ElementPool semantics ==="
rg -n -C2 'private pool|usedElements|public get\(|public release\(|public releaseAll\(' src/components/table/element-pool.ts
echo
echo "=== Table-level reset paths ==="
rg -n -C2 'releaseAll\(|clear\(|replaceData\(|destroy\(' src/components/table/table.tsxRepository: Lundalogik/lime-elements
Length of output: 2069
Release row-header drag handles from the formatter, or the pool grows indefinitely on redraws.
getDragHandleFormatter() acquires elements via pool.get() but never releases them. Each row render calls the formatter, acquiring a new drag handle from the pool and marking it as "used". When rows are virtualized or removed, these elements are orphaned but remain marked as used in the WeakMap, forcing the pool to create new elements on subsequent redraws. Over time, this accumulates unreleased limel-drag-handle instances until the table is reinitialized or destroyed.
Add a release mechanism tied to the row-header cell lifecycle, or switch to creating formatter elements outside the pool.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/table/row-drag-manager.ts` around lines 68 - 77,
getDragHandleFormatter() currently calls this.pool.get(LIMEL_DRAG_HANDLE) and
never releases the acquired limel-drag-handle, causing the pool's used map to
grow; either ensure the element is released back to the pool when the row-header
cell is torn down (call this.pool.release(element) from the row-header cell
lifecycle/cleanup path where the formatter's element is detached) or stop using
the pool here and instantiate/configure the drag-handle outside the pool (create
a new limel-drag-handle in getDragHandleFormatter and remove pool.get usage);
update references in getDragHandleFormatter, pool.get, LIMEL_DRAG_HANDLE and the
row-header cell cleanup code to implement the chosen approach.
5466fff to
cf97621
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
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/partial-styles/_row-selection.scss (1)
13-18:⚠️ Potential issue | 🟠 MajorHeader select-all checkbox misaligned when
movableRowsis enabled.The
.select-allpositioning atleft: 0.375remis not adjusted for.has-movable-rows, while.limel-table--row-selectoris shifted by--limel-table-drag-handle-widthon lines 23-25. When bothselectableandmovableRowsare enabled, the header checkbox will overlap the drag-handle column.🛠️ Proposed fix
.select-all { position: absolute; z-index: var(--limel-table-row-selector-z-index); left: 0.375rem; top: 0.1875rem; + + .has-movable-rows & { + left: calc(var(--limel-table-drag-handle-width) + 0.375rem); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/table/partial-styles/_row-selection.scss` around lines 13 - 18, The header select-all checkbox (.select-all) doesn't account for the movable-rows offset, so when .has-movable-rows is present it overlaps the drag-handle; update the CSS to shift .select-all when .has-movable-rows (or when inside .limel-table--row-selector with .has-movable-rows) by adding a rule that increases left by var(--limel-table-drag-handle-width) (e.g., left: calc(0.375rem + var(--limel-table-drag-handle-width))) so the checkbox aligns with the shifted .limel-table--row-selector column.
♻️ Duplicate comments (3)
src/components/table/row-drag-manager.ts (1)
63-66: 🧹 Nitpick | 🔵 TrivialMark
handleCellClickasreadonly.This arrow function property is never reassigned.
♻️ Proposed fix
- private handleCellClick = (ev: Event): void => { + private readonly handleCellClick = (ev: Event): void => { ev.stopPropagation(); ev.preventDefault(); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/table/row-drag-manager.ts` around lines 63 - 66, The property handleCellClick is defined as an arrow function and never reassigned; mark it readonly to reflect immutability and prevent accidental reassignment. Update the declaration of handleCellClick in the RowDragManager (or the class containing it) to include the readonly modifier so the signature becomes a readonly property assigned to the arrow function, keeping the existing implementation (ev.stopPropagation(); ev.preventDefault()) unchanged.src/components/table/examples/table-movable-rows.tsx (1)
47-52:⚠️ Potential issue | 🟡 Minor
toIndexlookup still uses reference equality.Line 47 uses
indexOf(toRow)which relies on reference equality. If Tabulator clones the row data, this will return-1and break the reorder. ThefromIndexlookup was fixed to usefindIndex, buttoIndexwas not.Also, the
as Birdcast on line 52 is unnecessary sincefromRowis already typed asBirdvia the genericRowReorderEvent<Bird>.🛠️ Proposed fix
- let toIndex = items.indexOf(toRow); + let toIndex = items.findIndex((bird) => bird.name === toRow.name); if (!above) { toIndex += 1; } - items.splice(toIndex, 0, fromRow as Bird); + items.splice(toIndex, 0, fromRow);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/table/examples/table-movable-rows.tsx` around lines 47 - 52, The toIndex lookup currently uses items.indexOf(toRow) which relies on reference equality and can return -1 if Tabulator clones row objects; change it to use the same predicate approach as the fromIndex fix (use items.findIndex(item => item.id === toRow.id or the same unique key used for rows) to locate toRow) and adjust the insertion accordingly; also remove the unnecessary "as Bird" cast on fromRow since fromRow is already typed via RowReorderEvent<Bird>.src/components/table/table.tsx (1)
608-616:⚠️ Potential issue | 🟠 Major
rowDragManagercan become stale whenmodeorpageSizechanges.The
initRowDragManager()method creates the manager unconditionally whenmovableRowsis true, but the documentation states the feature is "Only available inlocalmode without pagination." However, the watchers formode(nonexistent) andpageSize(lines 316-319) don't refresh the row drag state. Ifmodechanges fromlocaltoremote, orpageSizeis set after initialization,rowDragManagerremains active in an unsupported state.🛠️ Proposed fix
+ `@Watch`('mode') + protected modeChanged() { + this.refreshRowDragManager(); + this.init(); + } + `@Watch`('pageSize') protected pageSizeChanged() { this.updateMaxPage(); + this.refreshRowDragManager(); + this.init(); } `@Watch`('movableRows') protected updateMovableRows() { - this.rowDragManager = null; - this.initRowDragManager(); + this.refreshRowDragManager(); this.init(); } + private refreshRowDragManager() { + this.rowDragManager = null; + this.initRowDragManager(); + } + private initRowDragManager() { - if (this.movableRows) { + if (this.movableRows && this.mode === 'local' && !this.pageSize) { this.rowDragManager = new RowDragManager(🤖 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 608 - 616, initRowDragManager currently instantiates RowDragManager whenever movableRows is true but doesn't tear it down when table configuration changes (mode or pageSize), causing rowDragManager to remain active in unsupported states; update initRowDragManager and add/modify the watchers for mode and pageSize so they call a single reconcile function (e.g., ensureRowDragManager or refreshRowDragManager) that: checks this.movableRows, this.mode === 'local' and no pagination (pageSize unset/0), creates new RowDragManager(this.pool, this.reorder, this.language) only when allowed, and otherwise disposes/nulls the existing this.rowDragManager (calling any teardown on RowDragManager). Also ensure any existing creation first disposes the old instance to avoid leaks.
🤖 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/examples/table-movable-rows.tsx`:
- Line 31: Remove the unused private field sortableCols by deleting the
declaration "private sortableCols: string[] = ['name'];" from the component;
ensure no other code references it (search for sortableCols) and if there are no
usages remove any related type imports or comments, leaving the component
class/functional code unchanged.
In `@src/components/table/row-drag-manager.ts`:
- Around line 68-78: The drag-handle formatter (getDragHandleFormatter) acquires
elements from this.pool via pool.get(LIMEL_DRAG_HANDLE) but never releases them
per-row, causing the pool's "used" set to grow during virtualization; update
getDragHandleFormatter to attach a teardown that calls the pool release for the
returned element (e.g., call this.pool.release(element) or the pool's
equivalent) when the DOM node is removed or the row is destroyed—implement this
by adding a disconnected cleanup on the returned element (or a
MutationObserver/WeakRef cleanup tied to the element) so each acquired element
is released back into the pool when its row is virtualized out.
---
Outside diff comments:
In `@src/components/table/partial-styles/_row-selection.scss`:
- Around line 13-18: The header select-all checkbox (.select-all) doesn't
account for the movable-rows offset, so when .has-movable-rows is present it
overlaps the drag-handle; update the CSS to shift .select-all when
.has-movable-rows (or when inside .limel-table--row-selector with
.has-movable-rows) by adding a rule that increases left by
var(--limel-table-drag-handle-width) (e.g., left: calc(0.375rem +
var(--limel-table-drag-handle-width))) so the checkbox aligns with the shifted
.limel-table--row-selector column.
---
Duplicate comments:
In `@src/components/table/examples/table-movable-rows.tsx`:
- Around line 47-52: The toIndex lookup currently uses items.indexOf(toRow)
which relies on reference equality and can return -1 if Tabulator clones row
objects; change it to use the same predicate approach as the fromIndex fix (use
items.findIndex(item => item.id === toRow.id or the same unique key used for
rows) to locate toRow) and adjust the insertion accordingly; also remove the
unnecessary "as Bird" cast on fromRow since fromRow is already typed via
RowReorderEvent<Bird>.
In `@src/components/table/row-drag-manager.ts`:
- Around line 63-66: The property handleCellClick is defined as an arrow
function and never reassigned; mark it readonly to reflect immutability and
prevent accidental reassignment. Update the declaration of handleCellClick in
the RowDragManager (or the class containing it) to include the readonly modifier
so the signature becomes a readonly property assigned to the arrow function,
keeping the existing implementation (ev.stopPropagation(); ev.preventDefault())
unchanged.
In `@src/components/table/table.tsx`:
- Around line 608-616: initRowDragManager currently instantiates RowDragManager
whenever movableRows is true but doesn't tear it down when table configuration
changes (mode or pageSize), causing rowDragManager to remain active in
unsupported states; update initRowDragManager and add/modify the watchers for
mode and pageSize so they call a single reconcile function (e.g.,
ensureRowDragManager or refreshRowDragManager) that: checks this.movableRows,
this.mode === 'local' and no pagination (pageSize unset/0), creates new
RowDragManager(this.pool, this.reorder, this.language) only when allowed, and
otherwise disposes/nulls the existing this.rowDragManager (calling any teardown
on RowDragManager). Also ensure any existing creation first disposes the old
instance to avoid leaks.
🪄 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: b463b9d1-ce3c-4313-a4d6-84053ca13b38
⛔ Files ignored due to path filters (2)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.mdpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
src/components/table/examples/table-movable-rows.tsxsrc/components/table/partial-styles/_row-drag-handle.scsssrc/components/table/partial-styles/_row-selection.scsssrc/components/table/row-drag-manager.spec.tssrc/components/table/row-drag-manager.tssrc/components/table/table.scsssrc/components/table/table.tsxsrc/components/table/table.types.ts
albinhallen
force-pushed
the
feat/table-rows-drag-n-drop
branch
from
cf97621 to
21b2291
Compare
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/partial-styles/_row-selection.scss (1)
13-18:⚠️ Potential issue | 🟡 MinorAdd
.select-alloffset for movable rows.The
.select-allcheckbox doesn't adjust its position whenhas-movable-rowsis active. It remains atleft: 0.375remwhile the row selector column shifts right to accommodate the drag-handle, causing misalignment.🛠️ Proposed fix
.select-all { position: absolute; z-index: var(--limel-table-row-selector-z-index); left: 0.375rem; top: 0.1875rem; + + .has-movable-rows & { + left: calc(var(--limel-table-drag-handle-width) + 0.375rem); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/table/partial-styles/_row-selection.scss` around lines 13 - 18, The .select-all checkbox is not shifted when movable rows are enabled; add a sibling/ancestor selector to adjust its left offset when has-movable-rows is present (e.g., add a rule like ".has-movable-rows .select-all" that increases left by the drag-handle width using calc or a CSS variable) so the checkbox aligns with the shifted row-selector column; update _row-selection.scss and prefer using an existing CSS var for the drag-handle offset if available instead of a hardcoded value.
♻️ Duplicate comments (5)
src/components/table/row-drag-manager.ts (2)
63-66: 🧹 Nitpick | 🔵 TrivialMark
handleCellClickasreadonly.Per static analysis, this arrow function property is never reassigned.
♻️ Proposed fix
- private handleCellClick = (ev: Event): void => { + private readonly handleCellClick = (ev: Event): void => { ev.stopPropagation(); ev.preventDefault(); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/table/row-drag-manager.ts` around lines 63 - 66, The handleCellClick class property is never reassigned so mark it readonly to satisfy static analysis: update the property declaration for handleCellClick to include the readonly modifier (i.e., readonly handleCellClick = (ev: Event): void => { ... }) so the arrow-function member cannot be reassigned; leave the implementation (ev.stopPropagation(); ev.preventDefault();) unchanged.
68-78: 🧹 Nitpick | 🔵 TrivialPool elements accumulate without release during row virtualization.
The formatter acquires elements via
pool.get()but never releases them. When Tabulator virtualizes rows during scrolling, theselimel-drag-handleelements remain marked as "used" in the pool's WeakMap. Whilepool.releaseAll()is called during table reinitialization, extended scrolling through large datasets accumulates unreleased elements.This matches the existing pattern in
table-selection.ts, but consider adding per-row release logic for tables with many rows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/table/row-drag-manager.ts` around lines 68 - 78, The drag-handle formatter (getDragHandleFormatter) currently calls pool.get(LIMEL_DRAG_HANDLE) and never releases those elements, causing accumulation during row virtualization; fix this by ensuring each element returned by getDragHandleFormatter is released back to the pool when its row is removed/unrendered—e.g., attach the element to the row or cell (so you can identify it later) and call pool.release(element) from the Tabulator row unrender/unloaded handler or from a corresponding row cleanup/unformatter, or register a per-element cleanup when calling setElementProperties, ensuring you reference LIMEL_DRAG_HANDLE, pool.get, and pool.release (or pool.releaseAll if adapting existing pattern) and add the release call in the row removal/unrender path.src/components/table/examples/table-movable-rows.tsx (2)
31-36: 🧹 Nitpick | 🔵 TrivialMark
columnsandhandleReorderasreadonly.Per static analysis, these members are never reassigned.
♻️ Proposed fix
- private columns: Array<Column<Bird>> = [ + private readonly columns: Array<Column<Bird>> = [ { title: 'Name', field: 'name' }, { title: 'Binominal name', field: 'binominalName' }, { title: 'Nest type', field: 'nest', formatter: capitalize }, { title: 'Eggs per clutch', field: 'eggs', horizontalAlign: 'right' }, ]; - private handleReorder = (event: CustomEvent<RowReorderEvent<Bird>>) => { + private readonly handleReorder = (event: CustomEvent<RowReorderEvent<Bird>>) => {Also applies to: 38-38
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/table/examples/table-movable-rows.tsx` around lines 31 - 36, Mark the class members that are never reassigned as readonly: change the property declaration for columns (Array<Column<Bird>>) to readonly and add readonly to the handleReorder method signature (or mark it as a readonly arrow function property if it’s defined that way) so static analysis stops flagging reassignments; update the declarations for columns and handleReorder in the same class where they currently appear to include the readonly modifier.
45-50:⚠️ Potential issue | 🟡 Minor
indexOf(toRow)relies on reference equality.Line 45 uses
indexOf(toRow)which relies on reference equality. If Tabulator or the event mechanism clones row data objects, this returns-1and causes incorrect insertion. Line 42 was fixed to usefindIndex, but line 45 was not.Additionally, the
as Birdcast on line 50 is unnecessary sincefromRowis already typed asBird.🛠️ Proposed fix
const fromIndex = items.findIndex((bird) => bird.name === fromRow.name); items.splice(fromIndex, 1); - let toIndex = items.indexOf(toRow); + let toIndex = items.findIndex((bird) => bird.name === toRow.name); if (!above) { toIndex += 1; } - items.splice(toIndex, 0, fromRow as Bird); + items.splice(toIndex, 0, fromRow); this.tableData = items;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/table/examples/table-movable-rows.tsx` around lines 45 - 50, Replace the reference-equality lookup using items.indexOf(toRow) with a predicate-based search like items.findIndex(item => item.id === toRow.id) (or another unique key on Bird) so cloned row objects are matched correctly, and remove the unnecessary "as Bird" cast when inserting fromRow into items (use fromRow directly); target the variables/items and functions: items, toRow, fromRow, and the splice call.src/components/table/table.tsx (1)
608-616:⚠️ Potential issue | 🟠 Major
initRowDragManagerdoesn't enforce documented restrictions.The
movableRowsprop documentation states it's "Only available inlocalmode without pagination," butinitRowDragManager()creates the manager regardless ofmodeorpageSize. This allows drag handles to appear in unsupported configurations.🛠️ Proposed fix
private initRowDragManager() { - if (this.movableRows) { + const isLocalMode = this.mode === 'local'; + const hasPagination = !!this.pageSize; + + if (this.movableRows && isLocalMode && !hasPagination) { this.rowDragManager = new RowDragManager( this.pool, this.reorder, this.language ); + } else if (this.movableRows) { + console.warn( + 'limel-table: movableRows is only supported in local mode without pagination' + ); } }Additionally, per a past review comment, the
modeandpageSizewatchers should also callrefreshRowDragManager()to handle dynamic changes to these props.🤖 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 608 - 616, initRowDragManager currently only checks movableRows; change it to only create RowDragManager when this.movableRows AND this.mode === 'local' AND pagination is off (e.g. !this.pageSize or pageSize === 0 per your pagination semantics), and ensure it cleans up/destroys any existing this.rowDragManager when the conditions are not met; also update the watchers for mode and pageSize to call refreshRowDragManager() so dynamic prop changes create or tear down the manager via the existing refreshRowDragManager() helper (reference initRowDragManager, refreshRowDragManager, RowDragManager, movableRows, mode, pageSize).
🤖 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 444-449: When movableRows toggles, explicitly release the pooled
drag-handle elements before discarding the old RowDragManager: in
updateMovableRows(), if a rowDragManager or its pool exists call its
pool.releaseAll() (or this.pool.releaseAll() if pool is component-level) first,
then destroy/null the rowDragManager and call init(); this ensures the
formatter-acquired elements are released rather than orphaned when you later
recreate the manager in init().
---
Outside diff comments:
In `@src/components/table/partial-styles/_row-selection.scss`:
- Around line 13-18: The .select-all checkbox is not shifted when movable rows
are enabled; add a sibling/ancestor selector to adjust its left offset when
has-movable-rows is present (e.g., add a rule like ".has-movable-rows
.select-all" that increases left by the drag-handle width using calc or a CSS
variable) so the checkbox aligns with the shifted row-selector column; update
_row-selection.scss and prefer using an existing CSS var for the drag-handle
offset if available instead of a hardcoded value.
---
Duplicate comments:
In `@src/components/table/examples/table-movable-rows.tsx`:
- Around line 31-36: Mark the class members that are never reassigned as
readonly: change the property declaration for columns (Array<Column<Bird>>) to
readonly and add readonly to the handleReorder method signature (or mark it as a
readonly arrow function property if it’s defined that way) so static analysis
stops flagging reassignments; update the declarations for columns and
handleReorder in the same class where they currently appear to include the
readonly modifier.
- Around line 45-50: Replace the reference-equality lookup using
items.indexOf(toRow) with a predicate-based search like items.findIndex(item =>
item.id === toRow.id) (or another unique key on Bird) so cloned row objects are
matched correctly, and remove the unnecessary "as Bird" cast when inserting
fromRow into items (use fromRow directly); target the variables/items and
functions: items, toRow, fromRow, and the splice call.
In `@src/components/table/row-drag-manager.ts`:
- Around line 63-66: The handleCellClick class property is never reassigned so
mark it readonly to satisfy static analysis: update the property declaration for
handleCellClick to include the readonly modifier (i.e., readonly handleCellClick
= (ev: Event): void => { ... }) so the arrow-function member cannot be
reassigned; leave the implementation (ev.stopPropagation();
ev.preventDefault();) unchanged.
- Around line 68-78: The drag-handle formatter (getDragHandleFormatter)
currently calls pool.get(LIMEL_DRAG_HANDLE) and never releases those elements,
causing accumulation during row virtualization; fix this by ensuring each
element returned by getDragHandleFormatter is released back to the pool when its
row is removed/unrendered—e.g., attach the element to the row or cell (so you
can identify it later) and call pool.release(element) from the Tabulator row
unrender/unloaded handler or from a corresponding row cleanup/unformatter, or
register a per-element cleanup when calling setElementProperties, ensuring you
reference LIMEL_DRAG_HANDLE, pool.get, and pool.release (or pool.releaseAll if
adapting existing pattern) and add the release call in the row removal/unrender
path.
In `@src/components/table/table.tsx`:
- Around line 608-616: initRowDragManager currently only checks movableRows;
change it to only create RowDragManager when this.movableRows AND this.mode ===
'local' AND pagination is off (e.g. !this.pageSize or pageSize === 0 per your
pagination semantics), and ensure it cleans up/destroys any existing
this.rowDragManager when the conditions are not met; also update the watchers
for mode and pageSize to call refreshRowDragManager() so dynamic prop changes
create or tear down the manager via the existing refreshRowDragManager() helper
(reference initRowDragManager, refreshRowDragManager, RowDragManager,
movableRows, mode, pageSize).
🪄 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: 6e174135-c334-425c-8e73-30159a6da658
⛔ Files ignored due to path filters (2)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.mdpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
src/components/table/examples/table-movable-rows.tsxsrc/components/table/partial-styles/_row-drag-handle.scsssrc/components/table/partial-styles/_row-selection.scsssrc/components/table/row-drag-manager.spec.tssrc/components/table/row-drag-manager.tssrc/components/table/table.scsssrc/components/table/table.tsxsrc/components/table/table.types.ts
| @Watch('movableRows') | ||
| protected updateMovableRows() { | ||
| this.rowDragManager = null; | ||
| this.initRowDragManager(); | ||
| this.init(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Pool elements not released when movableRows toggles.
When movableRows changes, the watcher nulls rowDragManager and calls init(). While init() calls pool.releaseAll() before destroying Tabulator (line 530), the drag-handle elements acquired by the old RowDragManager formatter remain orphaned if the watcher fires before init() releases them.
Consider explicitly releasing pool elements before nulling the manager:
🛠️ Suggested fix
`@Watch`('movableRows')
protected updateMovableRows() {
+ this.pool.releaseAll();
this.rowDragManager = null;
this.initRowDragManager();
this.init();
}📝 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.
| @Watch('movableRows') | |
| protected updateMovableRows() { | |
| this.rowDragManager = null; | |
| this.initRowDragManager(); | |
| this.init(); | |
| } | |
| `@Watch`('movableRows') | |
| protected updateMovableRows() { | |
| this.pool.releaseAll(); | |
| this.rowDragManager = null; | |
| this.initRowDragManager(); | |
| this.init(); | |
| } |
🤖 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 444 - 449, When movableRows
toggles, explicitly release the pooled drag-handle elements before discarding
the old RowDragManager: in updateMovableRows(), if a rowDragManager or its pool
exists call its pool.releaseAll() (or this.pool.releaseAll() if pool is
component-level) first, then destroy/null the rowDragManager and call init();
this ensures the formatter-acquired elements are released rather than orphaned
when you later recreate the manager in init().
albinhallen
force-pushed
the
feat/table-rows-drag-n-drop
branch
from
21b2291 to
cd7b28a
Compare
|
@Kiarokh the issue with not being able to hover/move in the The scroll issue on the docs page seems to have been resolved, I don't see it anymore |
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/partial-styles/_row-selection.scss (1)
13-18:⚠️ Potential issue | 🟡 MinorMissing offset for
.select-allwhen movable rows are enabled.The
.limel-table--row-selectorcorrectly shifts right by--limel-table-drag-handle-widthwhen nested under.has-movable-rows(lines 23-25), but.select-allretains its fixedleft: 0.375remposition. This will cause the header "select all" checkbox to overlap the drag-handle column when bothselectableandmovableRowsare enabled.🛡️ Proposed fix
.select-all { position: absolute; z-index: var(--limel-table-row-selector-z-index); left: 0.375rem; top: 0.1875rem; + + .has-movable-rows & { + left: calc(var(--limel-table-drag-handle-width) + 0.375rem); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/table/partial-styles/_row-selection.scss` around lines 13 - 18, The .select-all rule currently uses a fixed left offset causing overlap when movable rows are enabled; update the stylesheet so that when .select-all is inside .has-movable-rows (or sibling to .limel-table--row-selector under .has-movable-rows) its left position is shifted by the drag handle width variable (--limel-table-drag-handle-width) e.g. compute left using calc(--limel-table-drag-handle-width + 0.375rem) to match the existing .limel-table--row-selector adjustment and prevent overlap with the drag-handle column.
♻️ Duplicate comments (3)
src/components/table/row-drag-manager.ts (1)
63-66: 🧹 Nitpick | 🔵 TrivialMark
handleCellClickasreadonly.Per static analysis feedback, this arrow function property is never reassigned.
♻️ Proposed fix
- private handleCellClick = (ev: Event): void => { + private readonly handleCellClick = (ev: Event): void => { ev.stopPropagation(); ev.preventDefault(); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/table/row-drag-manager.ts` around lines 63 - 66, The arrow function property handleCellClick should be marked readonly because it is never reassigned; update its declaration in the RowDragManager class (the handleCellClick property) to include the readonly modifier so it becomes readonly handleCellClick = (ev: Event): void => { ev.stopPropagation(); ev.preventDefault(); } ensuring the signature and behavior remain unchanged.src/components/table/examples/table-movable-rows.tsx (2)
31-36: 🧹 Nitpick | 🔵 TrivialMark
columnsasreadonly.Per static analysis feedback,
columnsis never reassigned and should be marked asreadonly.♻️ Proposed fix
- private columns: Array<Column<Bird>> = [ + private readonly columns: Array<Column<Bird>> = [ { title: 'Name', field: 'name' }, { title: 'Binominal name', field: 'binominalName' }, { title: 'Nest type', field: 'nest', formatter: capitalize }, { title: 'Eggs per clutch', field: 'eggs', horizontalAlign: 'right' }, ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/table/examples/table-movable-rows.tsx` around lines 31 - 36, The private property columns (type Array<Column<Bird>>) is never reassigned—mark it readonly by changing its declaration to include the readonly modifier (e.g., private readonly columns: Array<Column<Bird>> = [...] or private readonly columns: ReadonlyArray<Column<Bird>> = [...]) so the property and its type reflect immutability; update the declaration where columns is defined in the component.
42-50:⚠️ Potential issue | 🟡 MinorInconsistent lookup strategy:
toRowstill uses reference equality.Line 42 correctly uses
findIndexwith a property comparison forfromRow, but line 45 usesindexOf(toRow)which relies on reference equality. If Tabulator clones row data objects, this will return-1and cause incorrect reordering. Apply the samefindIndexpattern for consistency.Additionally, the
as Birdcast on line 50 is unnecessary sincefromRowis already typed asBird.🛡️ Proposed fix
const fromIndex = items.findIndex((bird) => bird.name === fromRow.name); items.splice(fromIndex, 1); - let toIndex = items.indexOf(toRow); + let toIndex = items.findIndex((bird) => bird.name === toRow.name); if (!above) { toIndex += 1; } - items.splice(toIndex, 0, fromRow as Bird); + items.splice(toIndex, 0, fromRow); this.tableData = items;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/table/examples/table-movable-rows.tsx` around lines 42 - 50, The move logic mixes reference and property-based lookups: replace items.indexOf(toRow) with a property-based find (e.g., items.findIndex((b) => b.name === toRow.name)) so toIndex is computed reliably even if row objects are cloned, and then use items.splice(fromIndex,1) / items.splice(toIndex,0,fromRow) as before; also remove the unnecessary "as Bird" cast on fromRow since fromRow is already typed as Bird.
🤖 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/row-drag-manager.ts`:
- Around line 68-78: The formatter getDragHandleFormatter currently acquires
elements from this.pool via pool.get(LIMEL_DRAG_HANDLE) but never returns them
to the pool, which can leak "used" pool entries for heavily virtualized tables;
modify getDragHandleFormatter (and the element returned) to register a
teardown/cleanup that calls this.pool.release(element) when the row is
unrendered/detached (follow the same per-row release pattern used in
table-selection.ts), so each acquired LIMEL_DRAG_HANDLE is released individually
instead of only relying on pool.releaseAll() during reinitialization.
---
Outside diff comments:
In `@src/components/table/partial-styles/_row-selection.scss`:
- Around line 13-18: The .select-all rule currently uses a fixed left offset
causing overlap when movable rows are enabled; update the stylesheet so that
when .select-all is inside .has-movable-rows (or sibling to
.limel-table--row-selector under .has-movable-rows) its left position is shifted
by the drag handle width variable (--limel-table-drag-handle-width) e.g. compute
left using calc(--limel-table-drag-handle-width + 0.375rem) to match the
existing .limel-table--row-selector adjustment and prevent overlap with the
drag-handle column.
---
Duplicate comments:
In `@src/components/table/examples/table-movable-rows.tsx`:
- Around line 31-36: The private property columns (type Array<Column<Bird>>) is
never reassigned—mark it readonly by changing its declaration to include the
readonly modifier (e.g., private readonly columns: Array<Column<Bird>> = [...]
or private readonly columns: ReadonlyArray<Column<Bird>> = [...]) so the
property and its type reflect immutability; update the declaration where columns
is defined in the component.
- Around line 42-50: The move logic mixes reference and property-based lookups:
replace items.indexOf(toRow) with a property-based find (e.g.,
items.findIndex((b) => b.name === toRow.name)) so toIndex is computed reliably
even if row objects are cloned, and then use items.splice(fromIndex,1) /
items.splice(toIndex,0,fromRow) as before; also remove the unnecessary "as Bird"
cast on fromRow since fromRow is already typed as Bird.
In `@src/components/table/row-drag-manager.ts`:
- Around line 63-66: The arrow function property handleCellClick should be
marked readonly because it is never reassigned; update its declaration in the
RowDragManager class (the handleCellClick property) to include the readonly
modifier so it becomes readonly handleCellClick = (ev: Event): void => {
ev.stopPropagation(); ev.preventDefault(); } ensuring the signature and behavior
remain unchanged.
🪄 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: ca492c8c-6b52-46ec-b9e5-990363d1be18
⛔ Files ignored due to path filters (2)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.mdpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
src/components/table/examples/table-movable-rows.tsxsrc/components/table/partial-styles/_row-drag-handle.scsssrc/components/table/partial-styles/_row-selection.scsssrc/components/table/row-drag-manager.spec.tssrc/components/table/row-drag-manager.tssrc/components/table/table.scsssrc/components/table/table.tsxsrc/components/table/table.types.ts
| private getDragHandleFormatter() { | ||
| return () => { | ||
| const element = this.pool.get(LIMEL_DRAG_HANDLE); | ||
| setElementProperties(element, { | ||
| dragDirection: 'vertical', | ||
| language: this.language, | ||
| }); | ||
|
|
||
| return element; | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider element release for heavily virtualized tables.
The formatter acquires elements via pool.get() but doesn't release them when rows are virtualized out of view. While pool.releaseAll() is called during table reinitialization, extended scrolling through tables with many rows will accumulate limel-drag-handle elements in the pool's "used" set.
This matches the existing pattern used by other formatters (e.g., table-selection.ts), so it's consistent with the codebase. For large tables with heavy virtualization, you may want to consider adding per-row release logic in the future.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/table/row-drag-manager.ts` around lines 68 - 78, The formatter
getDragHandleFormatter currently acquires elements from this.pool via
pool.get(LIMEL_DRAG_HANDLE) but never returns them to the pool, which can leak
"used" pool entries for heavily virtualized tables; modify
getDragHandleFormatter (and the element returned) to register a teardown/cleanup
that calls this.pool.release(element) when the row is unrendered/detached
(follow the same per-row release pattern used in table-selection.ts), so each
acquired LIMEL_DRAG_HANDLE is released individually instead of only relying on
pool.releaseAll() during reinitialization.
2 participants
Summary by CodeRabbit
New Features
Examples
Style
Tests
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: