fix: account for non visible columns when dropping

[emigun]

Description

Fix an issue where moving columns does not work correctly when hidden columns are present.

Motivation

Dragging and dropping a column while hidden columns are present could place the dragged column in the wrong position, or make it disappear from the rendered table.

This happened because drag-and-drop reports indices based on the rendered columns, while the table reorders the internal full column list. When hidden columns exist, those index positions no longer match.

Fixes:

• Build a mapping from rendered column indices to internal columns indices.
• Use the resolved internal source and target indices when moving columns.

Tests included

• Included for each change/fix?
• Passing? (Merge will not be approved unless this is checked)

Summary by Sourcery

Fix column drag-and-drop reordering to correctly handle hidden columns in the dynamic material table.

Bug Fixes:

• Resolve incorrect column reordering and disappearance when dragging columns while some columns are hidden by mapping drag indices from displayed to internal columns.

Enhancements:

• Add a helper to determine which columns are considered displayed and use it to derive drag source and target columns instead of relying on raw indices.

[emigun]

dragDropData appears to be unused apart from the assignment in dropListDropped(). Should we remove it to avoid confusion, or is there an intended future use for it?

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• The new displayedColumns computation in dropListDropped assumes this.columns.filter(this.isColumnDisplayed) matches the actual rendered header order; if the template ever uses different display logic or ordering, drag-and-drop will desync, so consider centralizing the displayed-columns calculation in one place and reusing it here and in the template.
• The isColumnDisplayed helper hardcodes the display string values ("visible", "prevent-hidden"); if you already have an enum or constants for these states elsewhere, it would be more robust to reuse them instead of duplicating literal values.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The new `displayedColumns` computation in `dropListDropped` assumes `this.columns.filter(this.isColumnDisplayed)` matches the actual rendered header order; if the template ever uses different display logic or ordering, drag-and-drop will desync, so consider centralizing the displayed-columns calculation in one place and reusing it here and in the template.
- The `isColumnDisplayed` helper hardcodes the `display` string values (`"visible"`, `"prevent-hidden"`); if you already have an enum or constants for these states elsewhere, it would be more robust to reuse them instead of duplicating literal values.

## Individual Comments

### Comment 1
<location path="src/app/shared/modules/dynamic-material-table/table/dynamic-mat-table.component.ts" line_range="1103" />
<code_context>
   dragStarted(event: Event) {}

   dropListDropped(event: CdkDragDrop<string[]>) {
-    const columnPreviousIndex = event.item.data.columnIndex;
+    const displayedColumns = this.columns.filter((column) =>
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying `dropListDropped` by mapping visible column indices directly to `this.columns` indices to avoid name-based lookups and extra guards.

You can keep the “respect hidden columns” behavior while simplifying `dropListDropped` by mapping displayed indices directly to `this.columns` indices once, instead of filtering then doing `findIndex` lookups by name.

That makes the logic O(n) per drop (as now) but removes the extra indirection, guards, and string-based lookups, which lowers cognitive load:

```ts
dropListDropped(event: CdkDragDrop<string[]>) {
  // Build mapping once: displayed index -> underlying columns index
  const displayedColumnIndices = this.columns
    .map((col, index) => ({ col, index }))
    .filter(({ col }) => this.isColumnDisplayed(col));

  const from = displayedColumnIndices[event.previousIndex];
  const to = displayedColumnIndices[event.currentIndex];

  // Guard against out-of-range indices or no-op moves
  if (!from || !to || from.index === to.index) {
    return;
  }

  this.dragDropData.dropColumnIndex = to.index;
  this.moveColumn(from.index, to.index);
}
```

If this helper is only used here, you could inline it (or at least better describe what “displayed” means) to reduce indirection:

```ts
const displayedColumnIndices = this.columns
  .map((col, index) => ({ col, index }))
  .filter(({ col }) =>
    col.display === undefined ||
    col.display === 'visible' ||
    col.display === 'prevent-hidden'
  );
```

or rename the helper to match its semantics more clearly:

```ts
private isColumnReorderable(column: TableField<T>) {
  return (
    column.display === undefined ||
    column.display === "visible" ||
    column.display === "prevent-hidden"
  );
}
```
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}