fix(@cockpit/dashboard): "Deployed Contracts" should auto-update after deployment #2020
Conversation
DeepCode Report (#634c13)DeepCode analyzed this pull request. |
| return ''; | ||
| .map((contract, key) => { | ||
| if (!contract) { | ||
| return null; |
There was a problem hiding this comment.
I'm sure there exists a good reason for this, but I find it unexpected that contract can be null here. contracts should IMO only hold valid objects so that the view doesn't have to worry about this kind of logic and simply render data.
Do you think we can maybe put a filter(contract => !!contract) on contracts inside the component before we iterate over it here?
There was a problem hiding this comment.
I agree that it's weird that a contract in the array would be falsey, but in the end doing the check here is not the end of the world, because using filter would be an additional loop that is not 100% needed
There was a problem hiding this comment.
Prior to the changes in this PR, the null check took place after the call to formatContractForDisplay. However, that function always returns an object, so the null check didn't make sense; also formatContractForDisplay isn't written to handle possibly falsy values. To be on the safe side, I simply moved the check up a bit, but I wasn't and am not sure if it's really possible that the contracts array might contain falsy values. It's definitely something we can investigate/refactor further, and I agree that contracts should hold only valid objects, but perhaps that's better left for future refactor PR/s.
| return ''; | ||
| .map((contract, key) => { | ||
| if (!contract) { | ||
| return null; |
There was a problem hiding this comment.
I agree that it's weird that a contract in the array would be falsey, but in the end doing the check here is not the end of the world, because using filter would be an additional loop that is not 100% needed
| @@ -2,7 +2,8 @@ import isToday from 'date-fns/isToday'; | |||
| import formatDistanceToNow from 'date-fns/formatDistanceToNow'; | |||
|
|
|||
| export function formatContractForDisplay(contract) { | |||
| let address = (contract.deployedAddress || contract.deployedAddress); | |||
There was a problem hiding this comment.
Wow, this condition was very useful.
I'm guessing the wanted line was (contract.deployedAddress || contract.address);
There was a problem hiding this comment.
That's what it used to be, according to git blame; it was changed in a big refactor PR, but based on the other changes in that PR it looked to me like it's okay to just check contract.deployedAddress rather than reintroduce || contract.address.
No description provided.