remove useEffect from state management in useDraggableTableControlled #3161
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3161 +/- ##
==========================================
+ Coverage 85.18% 85.19% +0.01%
==========================================
Files 1253 1253
Lines 27539 27542 +3
Branches 7335 7336 +1
==========================================
+ Hits 23458 23465 +7
+ Misses 4081 4077 -4
... and 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
|
The code looks good. I also tested the image and the table and the dropdown items both worked correctly |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
https://issues.redhat.com/browse/RHOAIENG-12561
Description
useDraggableTableControlledwas usinguseEffectto keep track of draggable items internally. While this was functional it would also lead to a double render whenever the incoming items array changed. The negative side effect of this is that the first render would supply data to the consumer that was stale until the next render. As a result, components which rely on object identity of the table items would be rendering against stale data until the new state was applied in theuseEffect.As noted in the linked issue, the connection type field would "blip" render an error icon indicating that the field's env var was a duplicate. The cause of this is because the check for duplicates would filter the given row data from the full set of table row data by object identity. Since the component was using stale data on the first render pass, the object identity check would fail until the 2nd pass.
The fix here is to eliminate the use of
useEffectfor setting state. The internal state is only required when dragging because it's a temporary state held by the hook until the drag completes.How Has This Been Tested?
To ensure the error icon still renders, create two fields with the same env var value.
Test Impact
Tests already exist for testing drag.
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main