Skip to content

fix(graph2md): writeGraphData uses zero lineCount when startLine absent#103

Merged
greynewell merged 2 commits intomainfrom
fix-graph-data-linecount
Apr 9, 2026
Merged

fix(graph2md): writeGraphData uses zero lineCount when startLine absent#103
greynewell merged 2 commits intomainfrom
fix-graph-data-linecount

Conversation

@greynewell
Copy link
Copy Markdown
Contributor

@greynewell greynewell commented Apr 9, 2026

Summary

  • writeGraphData computed lineCount only when startLine > 0 && endLine > 0
  • For nodes where the API omits startLine (value is 0), the condition was false and lineCount stayed 0
  • The lc field in the embedded graph visualisation JSON was therefore 0 (or absent via omitempty) for these nodes
  • Meanwhile, every frontmatter writer (writeFunctionFrontmatter, writeFileFrontmatter, etc.) already defaulted startLine to 1 when it was missing — producing the correct line_count in the markdown
  • Fix: apply the same effectiveStart=1 logic in writeGraphData so the graph JSON data is consistent with the frontmatter text

Test plan

  • TestGraphDataLineCountMissingStartLine: function node with endLine=50, no startLine — verifies lc=50 in graph_data JSON
  • Existing TestLineCountMissingStartLine (frontmatter text) still passes
  • go test ./internal/archdocs/graph2md/... passes
  • go build ./... passes

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved line count calculation in graph documentation to handle cases where start line information is missing or invalid, ensuring more accurate line counts are generated.
  • Tests

    • Added test coverage for edge cases in line count calculation.

writeGraphData computed lineCount only when startLine > 0 && endLine > 0.
For nodes without a startLine (API returns 0), the condition was false and
lineCount remained 0, so the graph visualisation data showed lc=0 even
though the same node's frontmatter correctly computed line_count=endLine
(using effectiveStart=1).

Fix: mirror the effectiveStart=1 defaulting used by all frontmatter writers
— if endLine > 0 but startLine <= 0, treat startLine as 1.

Adds TestGraphDataLineCountMissingStartLine to catch the regression.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 9, 2026

Walkthrough

This PR fixes how graph nodes calculate their line counts. Previously, it required both startLine and endLine to be positive. Now it only needs endLine to be positive, and if startLine is missing or zero, it defaults to 1. A test was added to verify this behavior.

Changes

Cohort / File(s) Summary
Line Count Calculation Logic
internal/archdocs/graph2md/graph2md.go
Modified the writeGraphData function to compute line count (LC) even when startLine is missing or ≤0. When endLine > 0, the function now clamps startLine to a minimum of 1 and calculates line count as endLine - effectiveStart + 1.
Test Coverage for Missing startLine
internal/archdocs/graph2md/graph2md_test.go
Added a helper function parseGraphData to extract and parse graph data from rendered markdown. Added TestGraphDataLineCountMissingStartLine to verify that a node without startLine but with endLine=50 correctly generates a line count of 50.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

Suggested reviewers

  • jonathanpopham

Poem

🎵 When endLine shows up all alone,
No startLine? That's fine, clamp it home!
Default to one, do the math with care,
Graph nodes now count what's really there. 📊

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main fix: handling the case where startLine is absent by using zero lineCount, which is the core behavioral change in writeGraphData.
Description check ✅ Passed The description covers all required sections: Summary explains the problem and solution clearly, Test plan details what was tested with checkboxes. The description provides good context and motivation for the fix.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-graph-data-linecount

Comment @coderabbitai help to get the list of available commands and usage tips.

goimports rejects manually aligned spaces; use single space.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@greynewell greynewell merged commit 8fa3e47 into main Apr 9, 2026
6 of 7 checks passed
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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/archdocs/graph2md/graph2md_test.go`:
- Line 223: The test ignores the error returned by os.ReadDir(outDir) (entries,
_ := os.ReadDir(outDir)); update the graph2md_test.go test to capture the error
(entries, err := os.ReadDir(outDir)), check err != nil and fail early with a
clear message (e.g., t.Fatalf("ReadDir(%s) failed: %v", outDir, err)) so
subsequent assertions (like checking for function markdown files) don't mask the
real I/O failure.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 16e04f55-46b5-4c52-ac72-c198b06d2733

📥 Commits

Reviewing files that changed from the base of the PR and between 7ab1310 and c344285.

📒 Files selected for processing (2)
  • internal/archdocs/graph2md/graph2md.go
  • internal/archdocs/graph2md/graph2md_test.go

}

// Find the function's markdown file
entries, _ := os.ReadDir(outDir)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Handle ReadDir errors explicitly.

At Line 223, the error is ignored. If directory read fails, the test may fail later with a less useful message (“function markdown file not found”) instead of the real I/O error.

Suggested fix
-	entries, _ := os.ReadDir(outDir)
+	entries, err := os.ReadDir(outDir)
+	if err != nil {
+		t.Fatalf("ReadDir: %v", err)
+	}
📝 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
entries, _ := os.ReadDir(outDir)
entries, err := os.ReadDir(outDir)
if err != nil {
t.Fatalf("ReadDir: %v", err)
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/archdocs/graph2md/graph2md_test.go` at line 223, The test ignores
the error returned by os.ReadDir(outDir) (entries, _ := os.ReadDir(outDir));
update the graph2md_test.go test to capture the error (entries, err :=
os.ReadDir(outDir)), check err != nil and fail early with a clear message (e.g.,
t.Fatalf("ReadDir(%s) failed: %v", outDir, err)) so subsequent assertions (like
checking for function markdown files) don't mask the real I/O failure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant