Fix empty section content + surface richer citations#53
Conversation
Fix empty section content + surface richer citations
PDF sectioning: multi-line display headings (a hero title wrapped across
several lines, e.g. "THE SYSTEM, END-TO-END" / "How Vectorless" /
"actually works.") were each detected as a separate heading, producing
"purely structural" leaf Sections with empty Content. Those sections
returned empty content to the section browser and /v1/sections/{id}.
Two-layer fix in pkg/parser/pdf.go:
- Merge consecutive same-level heading rows on the same page with no body
between them into one heading, so the following body attaches to the
whole title (root cause).
- foldEmptyLeafSections: a lossless post-parse pass that removes any
remaining empty-content leaf and carries its title onto the sibling
body it heads (bold markdown heading line), unioning page ranges.
Internal nodes (empty headings with real children) are untouched.
Citations: raise defaultTreeWalkMaxCitations 3 -> 6. The cap is a ceiling
that only bites on over-emission; the prompt + confidence gating still
drive the minimal sufficient set, so precision-critical single-pick
answers are unchanged, while genuinely multi-location answers now surface
every location they rely on instead of being truncated at three.
@
📝 WalkthroughWalkthroughThis PR increases the default TreeWalk ChangesTreeWalk max_citations default increase
Empty leaf section folding in PDF parser
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/config/config.go (1)
468-476: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale doc comment still says "Default: 3".
The
MaxCitationsfield comment saysDefault: 3butDefault()now sets it to 6 (Line 763). Update the comment to avoid confusing future readers/config authors.📝 Proposed fix
// MaxCitations caps how many distinct page ranges the final - // answer may cite. Default: 3. Set to 0 to use the strategy's + // answer may cite. Default: 6. Set to 0 to use the strategy's // built-in default. This is a confidence backstop, not a🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/config/config.go` around lines 468 - 476, The MaxCitations doc comment is stale and still claims the default is 3 even though Default() sets it to 6. Update the comment attached to the MaxCitations field in config.go so it matches the actual default value and remains consistent with the Default function.pkg/parser/empty_sections_test.go (1)
14-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for an empty leaf whose next sibling is an empty internal node.
The suite doesn't exercise the scenario where the only carrier following an empty leaf is an internal node (children, empty
Content). Given the folding pass attaches pending titles to that node'sContent, a test here would pin down the behavior flagged inpdf.goand prevent silent title loss.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/parser/empty_sections_test.go` around lines 14 - 88, Add a test case in TestFoldEmptyLeafSections for an empty leaf followed by an empty internal node, since foldEmptyLeafSections currently isn’t pinned down for that path. Create a Section sequence where the first item has empty Content and the next item has Children with empty Content, then assert the folded result preserves the pending title by attaching it to that internal node’s Content while keeping its children intact. Use the existing foldEmptyLeafSections and Section symbols to place the new case alongside the other subtests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/config/config.go`:
- Around line 468-476: The MaxCitations doc comment is stale and still claims
the default is 3 even though Default() sets it to 6. Update the comment attached
to the MaxCitations field in config.go so it matches the actual default value
and remains consistent with the Default function.
In `@pkg/parser/empty_sections_test.go`:
- Around line 14-88: Add a test case in TestFoldEmptyLeafSections for an empty
leaf followed by an empty internal node, since foldEmptyLeafSections currently
isn’t pinned down for that path. Create a Section sequence where the first item
has empty Content and the next item has Children with empty Content, then assert
the folded result preserves the pending title by attaching it to that internal
node’s Content while keeping its children intact. Use the existing
foldEmptyLeafSections and Section symbols to place the new case alongside the
other subtests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d27098c0-0bfd-4224-8217-57a9608cfa94
📒 Files selected for processing (6)
internal/config/config_treewalk_test.gopkg/config/config.gopkg/config/config_test.gopkg/parser/empty_sections_test.gopkg/parser/pdf.gopkg/retrieval/treewalk_strategy.go
@
What
Two engine fixes reported from the dashboard playground / section browser.
1. Empty section content (root cause + safety net)
Multi-line display headings (a hero title wrapped across lines, e.g.
THE SYSTEM, END-TO-END/How Vectorless/actually works.) were each detected as a separate heading, each producing a "purely structural" leafSectionwith emptyContent. Those sections returned empty content to the browser and/v1/sections/{id}.parseDoc) — consecutive same-level heading rows on the same page with no body between them merge into one heading; the following body attaches to the whole title.foldEmptyLeafSections— lossless post-parse pass removes any remaining empty-content leaf and carries its title onto the sibling body it heads (as a bold markdown heading line), unioning page ranges. Empty internal nodes (headings with real children) are left untouched. Applied on both the font-heuristic and outline parse paths.2. Richer citations
defaultTreeWalkMaxCitations3 → 6. The cap is a ceiling that only bites on over-emission; the prompt + confidence gating still drive the minimal sufficient set, so precision-critical single-pick answers are unchanged, while genuinely multi-location answers surface every location they rely on.Tests
pkg/parser/empty_sections_test.go(fold behaviour, accumulation, internal-node preservation, trailing/lone-leaf edge cases).go test ./...green.@
Summary by CodeRabbit
New Features
Bug Fixes