Correct topic count when node kind is not found in clipboardNodesMap#4690
Conversation
nucleogenesis
left a comment
There was a problem hiding this comment.
I tested this locally and it worked perfectly and moved things where I wanted them. I'm happy that this solution ended up being a bit less complicated than I thought when we were looking at it together 😄
I left one non-blocking code comment where I may be making incorrect assumptions to begin with so if that's the case feel free to ignore.
| // Check contentNodesMap for node kind if missing in clipboardNodesMap | ||
| if (!kind && node?.source_node_id) { | ||
| const contentNode = contentNodesValues.find(n => n.node_id === node.source_node_id); | ||
| kind = contentNode?.kind; |
There was a problem hiding this comment.
Non-blocking question: did you find that the ?. was necessary here because kind and/or source_node_id was missing on some contents or was it to just avoid unhandled errors just in case?
I ask because if we should expect kind on every contentNode (which I think we should?) then using the ?. might result in hard-to-debug issues when it isn't because it'll just count broken nodes as "resources" in any case here.
It's not crucial here, but in the if/else below it might be worth making the second condition like this (shortened for GH formatting).
if(kind === 'topic) { topicCount++ }
else if (kind) { resourceCount++ } // it's not undefined or null, so we know it's a non-topic resource
else { console.error("`kind` missing from content...") }But, again, this only really matters if we should be able to expect kind to always be defined in this context -- which may not be the case?
There was a problem hiding this comment.
Yes, we should be able to expect kind on every contentNode, the ? is in place to avoid any potential errors, but you are right about potential difficulties with debugging so I will make this change.
rtibbles
left a comment
There was a problem hiding this comment.
Requested change was made - code updates look good to me.
Summary
Description of the change(s) you made
This pull request modifies the computed property
topicAndResourceCount()to address instances where items in theclipboardNodesMapmay be missing the nodekind. It introduces the use ofcontentNodesMapto retrieve thekindwhen it is not present in theclipboardNodesMap.Reviewer guidance
How can a reviewer test these changes?
References
Fixes #3744
Comments
Contributor's Checklist
PR process:
CHANGELOGlabel been added to this PR. Note: items with this label will be added to the CHANGELOG at a later timedocslabel has been added if this introduces a change that needs to be updated in the user docs?requirements.txtfiles also included in this PRStudio-specifc:
notranslateclass been added to elements that shouldn't be translated by Google Chrome's automatic translation feature (e.g. icons, user-generated text)pages,components, andlayoutsdirectories as described in the docsTesting:
Reviewer's Checklist
This section is for reviewers to fill out.
yarnandpip)