fix: render inter-domain relationships in graph2md#7
Open
jonathanpopham wants to merge 3 commits intomainfrom
Open
fix: render inter-domain relationships in graph2md#7jonathanpopham wants to merge 3 commits intomainfrom
jonathanpopham wants to merge 3 commits intomainfrom
Conversation
Domain entity pages were missing connections between domains because the relationship switch block silently dropped domain-to-domain edges. Add domainEdge struct and domainRelatesOut/domainRelatesIn index maps, populate them via a default case for any relationship connecting two Domain nodes, and render them in writeGraphData, writeMermaidDiagram, and writeDomainBody. Closes #6
WalkthroughParses inter-domain edges into new internal indexes, threads them through renderContext, and renders Related Domains in domain body text, graph_data frontmatter, and Mermaid diagrams; also adds input validation and early output-directory creation. Changes
Sequence Diagram(s)sequenceDiagram
participant Parser as Graph Parser
participant Indexer as Domain Indexer
participant Renderer as Render Engine
participant Writer as File Writer
Parser->>Indexer: emit nodes & relationships
Indexer->>Indexer: build domainRelatesOut / domainRelatesIn
Indexer->>Renderer: pass nodes, rels, domainRelates maps
Renderer->>Renderer: generate domain body, graph_data, mermaid (include relatesTo edges)
Renderer->>Writer: write markdown files with frontmatter & diagrams
Writer-->>Renderer: confirm write
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/graph2md/graph2md_test.go (1)
56-101: Consider populatingdomainNodeByNamefor completeness.Right now the helper sets up
nodeLookupandslugLookup, butdomainNodeByNamestays empty (line 93). This is fine for the current tests, but if you later add a test that exercisesdomainLink()(which looks up domains by name), it'll return plain text instead of a link.Quick fix if you ever need it:
♻️ Suggested enhancement
for i := range allNodes { n := &allNodes[i] nodeLookup[n.ID] = n slugLookup[n.ID] = toSlug(getStr(n.Properties, "name")) + if hasLabel(n, "Domain") { + // Populate domainNodeByName so domainLink() works + domainNodeByName[getStr(n.Properties, "name")] = n.ID + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/graph2md/graph2md_test.go` around lines 56 - 101, The helper domainRenderCtx builds nodeLookup and slugLookup but leaves domainNodeByName empty; update domainRenderCtx to populate domainNodeByName by iterating allNodes (and center) and mapping each node's name (getStr(node.Properties,"name")) or slug to the node ID using the same toSlug logic so domainNodeByName[nameOrSlug] = node.ID; ensure you handle center the same way as other nodes and use the existing toSlug/getStr helpers so domainLink() can resolve names to links.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/graph2md/graph2md_test.go`:
- Around line 407-424: The extractGraphDataJSON test helper can panic when the
closing quote pattern isn't found; change the logic in extractGraphDataJSON to
compute endIdx := strings.Index(rest, "\"\n") and if endIdx == -1 call t.Fatalf
with a clear message (including the offending frontmatter or rest) instead of
slicing with -1, then build full using rest[:endIdx] as before and proceed with
json.Unmarshal; this prevents a slice bounds panic and gives a helpful test
failure message.
---
Nitpick comments:
In `@internal/graph2md/graph2md_test.go`:
- Around line 56-101: The helper domainRenderCtx builds nodeLookup and
slugLookup but leaves domainNodeByName empty; update domainRenderCtx to populate
domainNodeByName by iterating allNodes (and center) and mapping each node's name
(getStr(node.Properties,"name")) or slug to the node ID using the same toSlug
logic so domainNodeByName[nameOrSlug] = node.ID; ensure you handle center the
same way as other nodes and use the existing toSlug/getStr helpers so
domainLink() can resolve names to links.
- Guard against panic in extractGraphDataJSON when closing quote not found - Populate domainNodeByName in test helper so domainLink() resolves correctly
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/graph2md/graph2md_test.go (1)
23-31: Unused helper functionmakeSubdomainNode.Hey, heads up — this helper is defined but never actually called anywhere in the test file. It's like buying ingredients for a recipe you never cooked.
If you're planning to add subdomain tests later, totally fine to keep it around. Otherwise, consider removing it to keep things tidy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/graph2md/graph2md_test.go` around lines 23 - 31, The helper function makeSubdomainNode is defined but never used; either remove the unused function to tidy the test file or, if you intend to test subdomains, add tests that call makeSubdomainNode (referencing the makeSubdomainNode function and the Node type) so it is exercised; pick one and apply the change consistently in graph2md_test.go.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/graph2md/graph2md_test.go`:
- Around line 23-31: The helper function makeSubdomainNode is defined but never
used; either remove the unused function to tidy the test file or, if you intend
to test subdomains, add tests that call makeSubdomainNode (referencing the
makeSubdomainNode function and the Node type) so it is exercised; pick one and
apply the change consistently in graph2md_test.go.
Intra-domain relationships were being promoted to domain-level edges, creating noisy self-referencing loops in mermaid diagrams and graph data. Skip relationships where start and end node are the same domain.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/graph2md/graph2md.go (1)
1623-1639: Consider preserving the original relationship type in graph_data.This works correctly, but FYI: you're capturing the original
de.relType(like "DOMAIN_RELATES" or "dependsOn") in the struct, but here you're using the generic "relatesTo" for all edges.If downstream consumers of
graph_data(like visualization tools) could benefit from knowing the specific relationship type, you could usede.relTypeinstead:// Instead of: }{[]string{de.nodeID}, "relatesTo", false}) // Could be: }{[]string{de.nodeID}, de.relType, false})That said, if you want graph_data to have a normalized/consistent schema (all domain relations as "relatesTo"), the current approach is totally valid. Just wanted to flag the trade-off.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/graph2md/graph2md.go` around lines 1623 - 1639, The current loops that append to relSets for c.domainRelatesOut and c.domainRelatesIn always set relType to the literal "relatesTo" which loses the original de.relType; modify the append payloads in the loops that build relSets (the blocks referencing c.domainRelatesOut[c.node.ID] and c.domainRelatesIn[c.node.ID]) to use de.relType instead of the hardcoded "relatesTo" so the struct carries the original relationship type (preserve reverse as currently set for incoming edges).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/graph2md/graph2md.go`:
- Around line 216-223: The default case that builds
domainRelatesOut/domainRelatesIn can append duplicate domainEdge entries when
allRels contains the same relationship from multiple files; change the logic in
that block (the default: case that uses startNode/endNode, hasLabel, and appends
domainEdge) to check for an existing edge before appending by comparing nodeID
and relType (or maintain a temporary set keyed by fmt.Sprintf("%d|%s") or
similar) and only append when not already present, and apply the same dedupe
when adding to both domainRelatesOut[rel.StartNode] and
domainRelatesIn[rel.EndNode].
---
Nitpick comments:
In `@internal/graph2md/graph2md.go`:
- Around line 1623-1639: The current loops that append to relSets for
c.domainRelatesOut and c.domainRelatesIn always set relType to the literal
"relatesTo" which loses the original de.relType; modify the append payloads in
the loops that build relSets (the blocks referencing
c.domainRelatesOut[c.node.ID] and c.domainRelatesIn[c.node.ID]) to use
de.relType instead of the hardcoded "relatesTo" so the struct carries the
original relationship type (preserve reverse as currently set for incoming
edges).
Comment on lines
+216
to
+223
| default: | ||
| // Capture domain-to-domain relationships (any type connecting two Domain nodes) | ||
| startNode := nodeLookup[rel.StartNode] | ||
| endNode := nodeLookup[rel.EndNode] | ||
| if startNode != nil && endNode != nil && hasLabel(startNode, "Domain") && hasLabel(endNode, "Domain") && rel.StartNode != rel.EndNode { | ||
| domainRelatesOut[rel.StartNode] = append(domainRelatesOut[rel.StartNode], domainEdge{nodeID: rel.EndNode, relType: rel.Type}) | ||
| domainRelatesIn[rel.EndNode] = append(domainRelatesIn[rel.EndNode], domainEdge{nodeID: rel.StartNode, relType: rel.Type}) | ||
| } |
There was a problem hiding this comment.
Potential duplicate domain edges when merging multiple graph files.
Hey, quick heads-up: allRels at line 130 appends relationships from each input file without deduplication. So if the same domain-to-domain relationship appears in multiple files, you'll get duplicate entries in domainRelatesOut/domainRelatesIn. This would show the same related domain multiple times in the "Related Domains" section and Mermaid diagram.
For most real-world cases this probably won't happen, but if you want to be defensive, you could dedupe by checking if an edge with the same nodeID and relType already exists before appending.
Example scenario: If graph1.json and graph2.json both contain the edge Auth --DOMAIN_RELATES--> Payments, the Auth domain page would list Payments twice.
🛡️ Optional fix to deduplicate edges
default:
// Capture domain-to-domain relationships (any type connecting two Domain nodes)
startNode := nodeLookup[rel.StartNode]
endNode := nodeLookup[rel.EndNode]
if startNode != nil && endNode != nil && hasLabel(startNode, "Domain") && hasLabel(endNode, "Domain") && rel.StartNode != rel.EndNode {
+ // Check for duplicate before appending
+ isDuplicateOut := false
+ for _, existing := range domainRelatesOut[rel.StartNode] {
+ if existing.nodeID == rel.EndNode && existing.relType == rel.Type {
+ isDuplicateOut = true
+ break
+ }
+ }
+ if !isDuplicateOut {
+ domainRelatesOut[rel.StartNode] = append(domainRelatesOut[rel.StartNode], domainEdge{nodeID: rel.EndNode, relType: rel.Type})
+ }
+ isDuplicateIn := false
+ for _, existing := range domainRelatesIn[rel.EndNode] {
+ if existing.nodeID == rel.StartNode && existing.relType == rel.Type {
+ isDuplicateIn = true
+ break
+ }
+ }
+ if !isDuplicateIn {
+ domainRelatesIn[rel.EndNode] = append(domainRelatesIn[rel.EndNode], domainEdge{nodeID: rel.StartNode, relType: rel.Type})
+ }
- domainRelatesOut[rel.StartNode] = append(domainRelatesOut[rel.StartNode], domainEdge{nodeID: rel.EndNode, relType: rel.Type})
- domainRelatesIn[rel.EndNode] = append(domainRelatesIn[rel.EndNode], domainEdge{nodeID: rel.StartNode, relType: rel.Type})
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| default: | |
| // Capture domain-to-domain relationships (any type connecting two Domain nodes) | |
| startNode := nodeLookup[rel.StartNode] | |
| endNode := nodeLookup[rel.EndNode] | |
| if startNode != nil && endNode != nil && hasLabel(startNode, "Domain") && hasLabel(endNode, "Domain") && rel.StartNode != rel.EndNode { | |
| domainRelatesOut[rel.StartNode] = append(domainRelatesOut[rel.StartNode], domainEdge{nodeID: rel.EndNode, relType: rel.Type}) | |
| domainRelatesIn[rel.EndNode] = append(domainRelatesIn[rel.EndNode], domainEdge{nodeID: rel.StartNode, relType: rel.Type}) | |
| } | |
| default: | |
| // Capture domain-to-domain relationships (any type connecting two Domain nodes) | |
| startNode := nodeLookup[rel.StartNode] | |
| endNode := nodeLookup[rel.EndNode] | |
| if startNode != nil && endNode != nil && hasLabel(startNode, "Domain") && hasLabel(endNode, "Domain") && rel.StartNode != rel.EndNode { | |
| // Check for duplicate before appending | |
| isDuplicateOut := false | |
| for _, existing := range domainRelatesOut[rel.StartNode] { | |
| if existing.nodeID == rel.EndNode && existing.relType == rel.Type { | |
| isDuplicateOut = true | |
| break | |
| } | |
| } | |
| if !isDuplicateOut { | |
| domainRelatesOut[rel.StartNode] = append(domainRelatesOut[rel.StartNode], domainEdge{nodeID: rel.EndNode, relType: rel.Type}) | |
| } | |
| isDuplicateIn := false | |
| for _, existing := range domainRelatesIn[rel.EndNode] { | |
| if existing.nodeID == rel.StartNode && existing.relType == rel.Type { | |
| isDuplicateIn = true | |
| break | |
| } | |
| } | |
| if !isDuplicateIn { | |
| domainRelatesIn[rel.EndNode] = append(domainRelatesIn[rel.EndNode], domainEdge{nodeID: rel.StartNode, relType: rel.Type}) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/graph2md/graph2md.go` around lines 216 - 223, The default case that
builds domainRelatesOut/domainRelatesIn can append duplicate domainEdge entries
when allRels contains the same relationship from multiple files; change the
logic in that block (the default: case that uses startNode/endNode, hasLabel,
and appends domainEdge) to check for an existing edge before appending by
comparing nodeID and relType (or maintain a temporary set keyed by
fmt.Sprintf("%d|%s") or similar) and only append when not already present, and
apply the same dedupe when adding to both domainRelatesOut[rel.StartNode] and
domainRelatesIn[rel.EndNode].
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
graph2mdsilently dropped domain-to-domain relationshipsdomainEdgestruct anddomainRelatesOut/domainRelatesInindex maps that capture any relationship connecting two Domain nodeswriteGraphData(),writeMermaidDiagram(), andwriteDomainBody()to render outgoing and incoming domain relationshipsgraph2md_test.go) with 7 tests including end-to-endTest plan
go test ./...— all 7 tests passgo vet ./...— cleanCloses #6
Summary by CodeRabbit
New Features
Improvements
Tests
Note
Medium Risk
Changes markdown/frontmatter generation for
Domainpages by indexing previously-ignored relationships and emitting new edges/sections, which can affect site output and any consumers ofgraph_data/Mermaid diagrams. Test coverage is added, but behavior changes may surface formatting or size-limit edge cases on large graphs.Overview
graph2mdnow indexes any relationship between twoDomainnodes (captured asdomainRelatesOut/domainRelatesIn) instead of silently dropping them.Domain pages render these connections in three places: a new Related Domains section in the body (with relationship type and incoming/outgoing labeling), additional
relatesToedges ingraph_datafrontmatter, and labeled arrows in the Mermaid domain diagram.Adds a new
graph2md_test.gosuite with unit and end-to-end tests asserting relationship indexing and the new rendered outputs.Written by Cursor Bugbot for commit 33ef5a4. This will update automatically on new commits. Configure here.