fix nav-to-nearest mixin when there are no ancestors #6198
Conversation
| @@ -15,7 +15,7 @@ export default Mixin.create({ | |||
| navToNearestAncestor: task(function*(key) { | |||
| let ancestors = utils.ancestorKeysForKey(key); | |||
| let errored = false; | |||
| let nearest = ancestors.pop(); | |||
| let nearest = ancestors && ancestors.pop(); | |||
There was a problem hiding this comment.
Hey 👋
Just a little observation/suggestion here. I'm not even sure this is possible as I don't know how else you are using utils.ancestorKeysForKey.
Theoretically you could have this bug happening anywhere you are using utils.ancestorKeysForKey just it's not been noticed it yet, but you've only added the ancestors truthy check here.
Ideally, (although I'm not sure if its possible, and would be a bigger/riskier change). If utils.ancestorKeysForKey returned an empty array instead of null where there are no 'ancestorKeys', then your while here would still not run ([].pop() === undefined)
I just had a quick look to see where else you are using this method:
In namespace-picker you switch a null result back to an empty array anyway, and in key-value-header you do another truthy check on the return value which you could switch to a length check (Q: How many ancestors are there for this key? A: Zero)
So this is possible I 'think' (as long as my search picked up all places you are using this) and it's probably not that risky to do either and doing this would prevent any further bugs like this slipping in in the future.
TLDR; is You'd be changing it so the method would always return an array (no alarms and no surprises), not 'mostly an array, but sometimes null'.
Anyway, I just had a peek here whilst drinking my morning tea, thought you might like that ^ - no matter if not.
There was a problem hiding this comment.
Thanks for pointing this out - been waayyy too long since I've looked at the util code and took a wider view of its usage. I made the necessary changes / cleanup in #6205
The mixin that was added to navigate to the nearest existing secret when you deleted a secret didn't properly handle the case when a secret was at the top level.
In practice this means that a secret at
secrets/foo, when deleted via the UI, would be deleted, but the redirection would result in an uncaught error and it would look like nothing happened when you tried to delete the secret.This fixes the issue with the redirect erroring.
Fixes #6197