fix: broken selection of nested node executions #118
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
podehnal
approved these changes
Nov 10, 2020
service-github-lyft-semantic-release
added a commit
that referenced
this pull request
Nov 11, 2020
## [0.17.3](http://github.com/lyft/flyteconsole/compare/v0.17.2...v0.17.3) (2020-11-11) ### Bug Fixes * broken selection of nested node executions ([#118](http://github.com/lyft/flyteconsole/issues/118)) ([3f8d9ce](http://github.com/lyft/flyteconsole/commit/3f8d9ced1c1ddcc6f578cd5a592446a88d4992d1))
|
🎉 This PR is included in version 0.17.3 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TL;DR
#115 broke selection of nested items. This PR refactors how executions are referenced in the NodeExecutionsTable to allow lookup of nested NodeExecutions by ID, restoring selection of nested executions based on the updated logic in the previous PR.
Type
Are all requirements met?
Complete description
I naively fixed the previous issue by keeping track of the selected ID and looking it up in the
NodeExecutionsTablestate hook. That had the effect of only allowing selection of the first level of items, since those are the only ones present in the list we were searching.To mitigate this, I added functionality to the
ExecutionDataCacheto look up a previously fetchedNodeExecutionby its ID orcacheKey. This allows setting of the selection from any level of the list while still being able to look up the selected item at the top layer to provide to theDetailsPanel.This proved to be a little more complicated than I would like, as the
DetailsPanelexpects a decorated version ofNodeExecution(DetailedNodeExecution), which had been created by a separate hook in theNodeExecutionsTablecomponent and its descendants. To solve this, I moved the decoration ofNodeExecutionsinto theExecutionDataCacheitself, so that fetching the list ofNodeExecutionswill immediately result in a list ofDetailedNodeExecutionitems and all components in the tree expect to receive this type directly instead of constructing it themselves.Unit tests also needed to be updated, since our mocking there was a little messy.
Tracking Issue
NA
Follow-up issue
NA