fix: stale selected item reference in NE details panel #115
Conversation
Codecov Report
@@ Coverage Diff @@
## master #115 +/- ##
==========================================
+ Coverage 70.59% 70.68% +0.08%
==========================================
Files 387 387
Lines 6516 6525 +9
Branches 1044 1047 +3
==========================================
+ Hits 4600 4612 +12
+ Misses 1916 1913 -3
Continue to review full report at Codecov.
|
| executions.find( | ||
| ({ cacheKey }) => cacheKey === selectedExecutionKey | ||
| ) || null, |
There was a problem hiding this comment.
If .find doesn't find a match it will return undefined. Curious to know if there is a meaningful difference between returning undefined vs null?
There was a problem hiding this comment.
Short answer: No, not really. The consuming code should handle the undefined case in the same manner as the null case.
Longer answer: It's a matter of preference.
I prefer using null when explicitly setting a value to the "not defined" case and letting it be undefined when it truly was never defined or set. There are also cases where updating a value to be undefined is a little difficult or error-prone (like merging objects or attempting to delete properties). Setting to null feels cleaner in these cases.
To reflect that, the type of the returned value in the state is DetailedNodeExecution | null. It's initialized to null and set to null when the selection is cleared. So allowing it to be undefined here would cause a type error.
src/components/Executions/Tables/useNodeExecutionsTableState.ts
Outdated
Show resolved
Hide resolved
## [0.17.2](http://github.com/lyft/flyteconsole/compare/v0.17.1...v0.17.2) (2020-11-06) ### Bug Fixes * stale selected item reference in NE details panel ([#115](http://github.com/lyft/flyteconsole/issues/115)) ([12c486f](http://github.com/lyft/flyteconsole/commit/12c486f86b2e06a81ed3415f6e60e0bf874a0b88))
|
🎉 This PR is included in version 0.17.2 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
TL;DR
Fixes an issue where status of a NodeExecution doesn't update in the DetailsPanel if you have the panel open when the status changes.
Type
Are all requirements met?
Complete description
We were assigning the selected item for the Details Panel using an object reference, instead of an id reference. When the node executions update, new copies of the items are returned. But the DetailsPanel was still rendering the previously referenced object. To fix it, I updated the state tracking the selected item to be by id / cache key and am now computing the object to return using a memoized
findcall on the array of executions whenever the array or the selected id change.Also added a test to cover the regression.
Tracking Issue
flyteorg/flyte#505
Follow-up issue
NA