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.

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

feat: IPLD graph for explore page #672

Merged
merged 13 commits into from
Jun 13, 2018
Merged

Conversation

olizilla
Copy link
Member

@olizilla olizilla commented May 24, 2018

This PR adds a graph to the "explore" page to show the links from the current node and their target node type. It uses cytoscape and dagre layout.

screen shot 2018-06-11 at 12 55 45

CBOR node Storybook
screen shot 2018-06-11 at 12 56 19 screen shot 2018-05-24 at 11 27 42

Introducing GRAPH CRUMB!

It also adds a "graph crumb" component, which shows the node containment and path boundaries that have been traversed along the user provided path.

graph-crumb-explained

This change required upgrading the logic for resolving info from them the path. Previously we passed the cid+path to dag.get and tried to render whatever value came back. As dag-cbor node values don't provide the same info as dag-pb nodes (no links or multihash properties) this pushed complexity on to the UI code of working out how to handle the differences. This PR fixes that by normalising data from dag.get into a common shape. The normaliseDagNode function currently handles dag-pb and dag-cbor but the plan is it can be extended to normalise all the things.

resolveIpldPath gives the additional info about the path by recursively calling dag.get from the root cid to find all the path boundaries (links that have to be traversed) to resolve the entire path. The normalised array of nodes and pathBoundaries that were crossed are made available to the the UI so the Graph crumb component can visualise it.

License: MIT
Signed-off-by: Oli Evans [email protected]

Uses cytoscape to display all the links from a given node

License: MIT
Signed-off-by: Oli Evans <[email protected]>
@hacdias hacdias added the revamp label May 24, 2018
@whyrusleeping
Copy link
Member

In the case where you have "favorites/{1,2,3}" it would be cool to represent "favorites" as its own node in the graph.

License: MIT
Signed-off-by: Oli Evans <[email protected]>
@olizilla
Copy link
Member Author

@whyrusleeping I'm thinking about the graph demonstrating links between IPLD nodes, and visually associating the link target nodes with the table of links. It's not obvious in these screenshots but the data explorer section shows the complete, possibly complex and deeply nested object tree within a single node.

My current plan is to improve the graph to show the IPLD node that a given path traverses, so you can get a feel for the path you took to get to given node, like:

screen shot 2018-05-29 at 14 29 30

...but I wasn't thinking about using it to show the internal structure of a CBOR node. Should I reconsider? Do you think it'd be more useful if it did show nodes that represent the internal CBOR structure as well?

@whyrusleeping
Copy link
Member

@olizilla I think that showing internal structures (of every potential node type, not just cbor) would be very useful. cc @diasdavid

@olizilla
Copy link
Member Author

@whyrusleeping @diasdavid please do add ideas for IPLD graph visualisations over here ipfs/ipfs-gui#11

License: MIT
Signed-off-by: Oli Evans <[email protected]>
License: MIT
Signed-off-by: Oli Evans <[email protected]>
@hacdias hacdias added the status/in-progress In progress label Jun 6, 2018
…link normalisation!

License: MIT
Signed-off-by: Oli Evans <[email protected]>
License: MIT
Signed-off-by: Oli Evans <[email protected]>
Show the path boundaries and source nodes along a path in a neat breadcrumb style

License: MIT
Signed-off-by: Oli Evans <[email protected]>
Where you explore into a sub path of a cbor node,
we always show the info about the containing node,
but show the exact path in the url and graph crumb.

License: MIT
Signed-off-by: Oli Evans <[email protected]>
@olizilla olizilla requested a review from lidel June 11, 2018 12:15
@olizilla
Copy link
Member Author

Sorry, this PR has grown much larger than I would have liked, but there were a bunch of details to resolve in it around node normalisation, path resolving, and handling paths that land somewhere in the middle of a cbor node.

@olizilla
Copy link
Member Author

@lidel please can you take a look at this when you get a moment.

@olizilla
Copy link
Member Author

The dag node normalisation logic tries to nudge things towards a more coherent naming scheme; normaliseDagNode returns objects like:

{
  cid: 'zHash1' // `cid` instead of `hash` or `multihash`
  type: 'dag-cbor' // or 'dag-pb'
  data: { /* the value of a dag-cbor node or the Data prop of a dag-pb */ }
  links: [
   {
      path: 'a/b/c' // in dag-cbor a link name may be a path, so standardise on `path`
      source: 'zHash1' // CID of node that contains this link.
      target: 'zHash2' // CID that the link points to.
  }
}

We map dag-cbor and dag-pb nodes returned from dag.get into this shape which then reduces the amount of conditional logic needed in the UI code. Adding the source cid to links is super useful as the link data can then be used in isolation from the parent node and still make sense, the source CID + path is a permanent-web alias for the target cid.

License: MIT
Signed-off-by: Oli Evans <[email protected]>
@olizilla olizilla requested a review from hacdias June 12, 2018 10:43
@olizilla
Copy link
Member Author

Am resolving the conflict now

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Test CID that links from dag-cbor to dag-pb: zdpuAs8sJjcmsPUfB1bUViftCZ8usnvs2cXrPH6MDyT4zrvSs

That ↑ graph looks really slick and the way graph crumb provides context is 👌
Introducing unified abstraction format for different dag types makes sense, but I would avoid reusing path as a field name (made separate comment about it).

Below is a list of issues I've found while playing with it, but since this PR is only about adding a graph, I'm 👍 to merge it and address these things in separate PRs / issues.
Let me know if I should move below to dedicated issues.


Small potential bugs / edge cases that need additional handling:

UX stuffs:

  • CID and size columns overlap in narrow views

  • When user clicks on next node link or enters a new hash via input at the top, nothing happens until it is successfully fetched from the network.

    • I strongly feel UI should immediately remove details of the old one node, update the CID at the top and display a "Fetching <CID> details from the swarm.." message to the user while CID details load in background.
    • UI should be resilient to IPFS swarm hiccups. Right now unreachable CIDs will hang UI forever (example). I think there should be 5-10s timeout displaying an option to continue lookup (this time without timeout), or at least message informing user that lookup is in progress, as suggested in previous point.
    • It should be possible to instantly abort lookup by entering a different hash.
      Right now nothing happens when this loads and I enter QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR and click "Explore"
  • Tiny things/ideas I've noticed while looking at it PR:

    • Vertical alignment of the triangle expander in Data section is bit off, but it may be due to custom font setup on my Linux box.
    • CID column gets cropped out when window size is narrow (example: cid, screenshot).
      May be a good idea to just add truncate class there, so that it gets text-overflow: ellipsis; sugar in edge cases. Looks a lot better than raw cut.
    • When user enters hash into top "Explore" field, we should provide some visual feedback, eg. make button darker and change cursor to pointer to indicate it is ready to be clicked (right now it looks 'inactive' and I was surprised it worked :)) )
    • Triangle expanders should have cursor: pointer; (pointer class) as additional hint they are actionable


makeNode ({target, path}, index) {
// TODO: pass link type info.
const type = target.startsWith('Q') ? 'dag-pb' : 'dag-cbor'
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, someone will take CIDv0 of dag-pb and convert it to CIDv1 as a test and get weird results, like I did just a moment ago 🙃

*/
export function normaliseDagPbLinks (node) {
return node.links.map(({name, size, multihash}) => ({
path: name,
Copy link
Member

Choose a reason for hiding this comment

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

🚲 🏠 since path is actually either a path (cbor) or a name (pb), perhaps context or label would be a less confusing term?

@olizilla
Copy link
Member Author

@lidel thanks for this, that's all really helpful. I strongly agree about making it super clear to the user what's happening and being robust when we find more exotic nodes. This PR gets things to a coherent point where it can work and is large, so I vote we merge it and tackle the issues raised in subsequent PRs.

Totally up for discussing, but right now I think path is a good name for the property as a name is a path of length 1. I see path as a superset of name, and we're using the term path for a "0 or more slash separated node traversal steps" ;-)

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Makes sense, I have no strong feelings about that name so flipping to 👍

@olizilla olizilla merged commit caaadfb into revamp Jun 13, 2018
@olizilla olizilla deleted the feat/revamp/ipld-graph-cytoscape branch June 13, 2018 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants