Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove duplicates from the expandedNodeMap array #1031

Merged
merged 1 commit into from
Jan 7, 2020
Merged

Remove duplicates from the expandedNodeMap array #1031

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/browser/modules/D3Visualization/lib/visualization/components/graph.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ export default class Graph {
this.nodeMap[eNode.id] = eNode
this._nodes.push(eNode)
this.expandedNodeMap[node.id] = this.expandedNodeMap[node.id]
? this.expandedNodeMap[node.id].concat([eNode.id])
? [...new Set(this.expandedNodeMap[node.id].concat([eNode.id]))]
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so I have to investigate a little more but I have removed duplicates at this stage and it seems to work. I have to look into the nodes parameter a bit as to why it includes the multiples of the node relationships. It may be that thats where the change is required.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tracked back to https://github.com/neo4j/neo4j-browser/blob/master/src/shared/services/bolt/boltMappings.js#L214 and tried to put in a reduce here to remove duplicates but since this function is used in several places, it seems to cause other issues. So to narrow the scope to the particular problem, Im going to stick with my initial solution for now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense and I agree with keeping the scope small.

I think Hugo is tackling it at a lower level in #1003.
So maybe his PR fixes this bug as well @jk05.
Could you have a look if that's the case?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ill take a look 👍

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pulled it down locally and the bug still seems to be there...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so we can either can look to go ahead with this work to keep the scope smaller, or I can leave a comment on #1003 with a link to this PR (as the context of where Hugo is making the changes could potentially be a better fit than this work). What do you think @oskarhane?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change makes sense 👍

: [eNode.id]
}
}
Expand Down