Conversation
📝 WalkthroughWalkthroughThis PR introduces row drag-and-drop reordering functionality to the table component. It adds a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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.
| @Watch('movableRows') | ||
| protected updateMovableRows() { | ||
| this.rowDragManager = null; | ||
| this.initRowDragManager(); | ||
| this.init(); | ||
| } |
There was a problem hiding this comment.
Refresh row-drag state when mode or pageSize changes.
Line 609 gates the feature on both mode and pageSize, but the refresh path only runs from the movableRows watcher at Lines 444-449. If either prerequisite changes later, rowDragManager goes stale and the table can keep drag handles enabled in unsupported states, or never enable them after 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: 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 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.
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.
src/components/table/table.tsx
Outdated
| 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.
albinhallen
force-pushed
the
feat/table-rows-drag-n-drop
branch
from
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
| @State() | ||
| private tableData: Bird[] = [...data]; | ||
|
|
||
| private sortableCols: string[] = ['name']; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove unused sortableCols field.
This field is declared but never referenced anywhere in the component.
♻️ Proposed fix
- private sortableCols: string[] = ['name'];
-📝 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.
| private sortableCols: string[] = ['name']; | |
| 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 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` at 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.
| 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
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if other formatters in the codebase follow the same pool.get() without release pattern
rg -n "pool\.get\(" src/components/table/ -A3 -B1Repository: Lundalogik/lime-elements
Length of output: 5506
🏁 Script executed:
#!/bin/bash
# Check if pool.releaseAll() is called in table.tsx
rg -n "pool\.releaseAll\(\)" src/components/table/ -B3 -A3Repository: Lundalogik/lime-elements
Length of output: 1443
🏁 Script executed:
#!/bin/bash
# Check imports in row-drag-manager.ts for absolute vs relative paths
head -n 30 src/components/table/row-drag-manager.ts | grep -E "^import|^from"Repository: Lundalogik/lime-elements
Length of output: 387
🏁 Script executed:
#!/bin/bash
# Search for pool.release() calls (individual releases, not releaseAll)
rg -n "pool\.release\(" src/components/table/ -B2 -A2Repository: Lundalogik/lime-elements
Length of output: 885
Pool element acquisition pattern matches existing formatters, but consider individual releases for virtualized row cleanup.
The formatter acquires elements via pool.get() but doesn't release them when rows are destroyed or virtualized out of view. While pool.releaseAll() is called during table reinitialization (in table.tsx at lines 356 and 530), heavy scrolling through virtualized rows accumulates limel-drag-handle elements in the pool's "used" set until the next full table reinit. This matches the consistent pattern used by table-selection.ts and other column formatters, but adding per-row release logic could prevent memory accumulation in tables with many rows during extended scrolling.
🤖 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) 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.
2 participants
Summary by CodeRabbit
New Features
movableRowspropertyreorderevent emitted when rows are rearrangedExamples
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: