User column drag and drop freeze fix

[mike12345567]

Description

Quick fix for drag and drop behaviour of relationship cells, appears empty cells were causing things to break in the re-render.

Appears to affect all relationship column types.

Addresses: #12070

Feature branch env

Feature Branch Link

[codecov-commenter]

Codecov Report

Merging #12076 (1d951a5) into master (a60690f) will not change coverage.
Report is 1 commits behind head on master.
The diff coverage is n/a.

❗ Current head 1d951a5 differs from pull request most recent head 30fadf0. Consider uploading reports for the commit 30fadf0 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@           Coverage Diff           @@
##           master   #12076   +/-   ##
=======================================
  Coverage   75.37%   75.37%           
=======================================
  Files         326      326           
  Lines       13904    13904           
  Branches     2931     2931           
=======================================
  Hits        10480    10480           
  Misses       3198     3198           
  Partials      226      226           

see 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[aptkingston]

This is a really strange one - the error thrown is a svelte internal error. The cells handle empty values just fine, so I reckon this is some really strange edge case with how svelte is remounting things inside unkeyed #each blocks.

I actually dug deep into this one, and the error is actually down to the Icon component that displays the X to remove a value. Removing this icon fixes this problem.

The change here works almost by accident, because if you move the logic into a reactive statement, something like:

$: foo = Array.isArray(value) ? value : []
...
{#each foo as relationship}
  ...

then the error still happens - so the actually array checking doesn't affect anything, but I think it's the extra reference to value is causing svelte to handle things internally differently.

The easy fix here is just to key the #each block (something like {#each value as relationship (relationship._id)), but I really don't want to do that because remounting every value badge should be unnecessary and introduces a big performance hit. The grid deliberately never remounts anything apart from swapping columns, which is why it has fantastic performance with thousands of rows.

[aptkingston]

After looking at it more I've learned something new about svelte! The grid has always deliberately remounted cells when the column changes, since values between column types are incompatible with each other. Turns out that when a parent component is remounted, child components are potentially not remounted - so all along grid cells have not actually been properly remounting when changing columns.

There's a larger change there I need to make to the grid to fix that issue, which will in turn properly fix this.

I'm going to get that sorted now as there may be other consequences, but I'm happy to merge this in the meantime to fix the issue, and I'll revert it when doing that larger change.

[mike12345567]

After looking at it more I've learned something new about svelte! The grid has always deliberately remounted cells when the column changes, since values between column types are incompatible with each other. Turns out that when a parent component is remounted, child components are potentially not remounted - so all along grid cells have not actually been properly remounting when changing columns.

There's a larger change there I need to make to the grid to fix that issue, which will in turn properly fix this.

I'm going to get that sorted now as there may be other consequences, but I'm happy to merge this in the meantime to fix the issue, and I'll revert it when doing that larger change.

Very interesting - yeah it does feel like something deeper is going on, as I was really sure how totally stopping the relationship cell from rendering when it was empty would affect things - this makes a lot more sense!

I'll merge this for now as it does buy time for the larger refactor and isn't really a major change in anyway.

[github-actions]

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅