Sanitize document titles + drop repeated icon-glyph marker rows#56
Conversation
Sanitize document titles + drop repeated icon-glyph marker rows Title sanitize: an ingest title with invalid UTF-8 bytes (e.g. a client that mangled an em-dash in the multipart field) made the Postgres insert fail with "db write failed" and 500 the whole upload. sanitizeTitle now coerces the title to valid UTF-8 and strips control chars before the DB write; the parsed-title path is likewise run through cleanForLLM. A bad title byte can no longer fail an ingest. Cover-page furniture: isRepeatedMarkerLine drops rows that are mostly the same short token repeated — the artifact of an icon glyph rendered as text and stamped once per item (e.g. the ORCID "iD" logo on a paper byline, "iD iD iD iD"), which the font heuristic otherwise surfaced as junk author-block sections. Real prose never repeats an identical <=2-char token as >=60% of a >=3-token line, so this is safe. @
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds a ChangesDocument title sanitization
PDF repeated-marker boilerplate filtering
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)Not applicable — changes are isolated logic/heuristic additions without multi-component interaction flows. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/ingest/ingest.go (1)
705-722: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winCleaned title could become empty, overwriting the seeded title with blank.
The guard at Line 711 checks
doc.Title != ""on the raw title, but the value actually written iscleanForLLM(doc.Title)(Line 719). Ifdoc.Titleis non-empty but consists only of characterscleanForLLMstrips (NUL / most C0 controls), the cleaned result is"", andSetDocumentTitlewill overwrite the row's filename-seeded title with an empty string — worse than the pre-PR behavior of writing the (non-empty) raw title.🛡️ Proposed fix
} else if doc.Title != "" && !isLikelyMojibakeTitle(doc.Title) { // Otherwise only overwrite the row's title (seeded with the // filename at upload time) when the parsed title looks usable. // Watermarked PDFs whose overlay text shares a Y coordinate with // the real title produce mojibake like "GGlloobbaall SSttrraatteeggyy" // — we'd rather keep the original filename than show that to a user. // cleanForLLM strips any invalid-UTF-8 bytes so the title write can // never fail the persist. - if err := store.SetDocumentTitle(ctx, docID, cleanForLLM(doc.Title)); err != nil { - return err + if cleaned := cleanForLLM(doc.Title); cleaned != "" { + if err := store.SetDocumentTitle(ctx, docID, cleaned); err != nil { + return err + } } }🤖 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/ingest/ingest.go` around lines 705 - 722, The title update in persistTree can still write an empty string because the non-empty check is done on doc.Title before cleanForLLM(doc.Title) is applied. Update the logic in Pipeline.persistTree to validate the cleaned title instead of the raw parsed title, and only call store.SetDocumentTitle when the cleaned value is still non-empty (and still not a mojibake title). Keep the sticky titleOverride behavior unchanged.
🤖 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.
Inline comments:
In `@internal/handler/documents.go`:
- Around line 139-156: The sanitizeTitle function is incorrectly removing valid
U+FFFD replacement characters by checking r == utf8.RuneError after
strings.ToValidUTF8 has already normalized invalid bytes. Update sanitizeTitle
so it only strips actual invalid input during the ToValidUTF8 step and does not
discard legitimate “�” runes during the rune loop; keep the control-character
handling and whitespace normalization intact.
---
Outside diff comments:
In `@pkg/ingest/ingest.go`:
- Around line 705-722: The title update in persistTree can still write an empty
string because the non-empty check is done on doc.Title before
cleanForLLM(doc.Title) is applied. Update the logic in Pipeline.persistTree to
validate the cleaned title instead of the raw parsed title, and only call
store.SetDocumentTitle when the cleaned value is still non-empty (and still not
a mojibake title). Keep the sticky titleOverride behavior unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fec40b7d-a89c-48cc-9224-3be69bc9767a
📒 Files selected for processing (5)
internal/handler/documents.gointernal/handler/sanitize_title_test.gopkg/ingest/ingest.gopkg/parser/pdf.gopkg/parser/repeated_marker_test.go
| func sanitizeTitle(s string) string { | ||
| if !utf8.ValidString(s) { | ||
| s = strings.ToValidUTF8(s, "") | ||
| } | ||
| var b strings.Builder | ||
| b.Grow(len(s)) | ||
| for _, r := range s { | ||
| if r == utf8.RuneError { | ||
| continue | ||
| } | ||
| if r < 0x20 { // drop control chars incl. tab/newline — titles are one line | ||
| b.WriteRune(' ') | ||
| continue | ||
| } | ||
| b.WriteRune(r) | ||
| } | ||
| return strings.TrimSpace(multiSpaceRe.ReplaceAllString(b.String(), " ")) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
RuneError check drops legitimate U+FFFD characters.
utf8.RuneError and '\uFFFD' are the same value. Since s was already coerced via strings.ToValidUTF8(s, "") above, for _, r := range s can no longer decode an actual invalid byte to RuneError here — any r == utf8.RuneError at this point is a legitimately-encoded "�" character in the input, not a decode failure. The current check silently strips these valid characters instead of a decode error.
🐛 Proposed fix
var b strings.Builder
b.Grow(len(s))
- for _, r := range s {
- if r == utf8.RuneError {
- continue
- }
- if r < 0x20 { // drop control chars incl. tab/newline — titles are one line
+ for _, r := range s {
+ if r < 0x20 { // drop control chars incl. tab/newline — titles are one line
b.WriteRune(' ')
continue
}
b.WriteRune(r)
}📝 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.
| func sanitizeTitle(s string) string { | |
| if !utf8.ValidString(s) { | |
| s = strings.ToValidUTF8(s, "") | |
| } | |
| var b strings.Builder | |
| b.Grow(len(s)) | |
| for _, r := range s { | |
| if r == utf8.RuneError { | |
| continue | |
| } | |
| if r < 0x20 { // drop control chars incl. tab/newline — titles are one line | |
| b.WriteRune(' ') | |
| continue | |
| } | |
| b.WriteRune(r) | |
| } | |
| return strings.TrimSpace(multiSpaceRe.ReplaceAllString(b.String(), " ")) | |
| } | |
| func sanitizeTitle(s string) string { | |
| if !utf8.ValidString(s) { | |
| s = strings.ToValidUTF8(s, "") | |
| } | |
| var b strings.Builder | |
| b.Grow(len(s)) | |
| for _, r := range s { | |
| if r < 0x20 { // drop control chars incl. tab/newline — titles are one line | |
| b.WriteRune(' ') | |
| continue | |
| } | |
| b.WriteRune(r) | |
| } | |
| return strings.TrimSpace(multiSpaceRe.ReplaceAllString(b.String(), " ")) | |
| } |
🤖 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 `@internal/handler/documents.go` around lines 139 - 156, The sanitizeTitle
function is incorrectly removing valid U+FFFD replacement characters by checking
r == utf8.RuneError after strings.ToValidUTF8 has already normalized invalid
bytes. Update sanitizeTitle so it only strips actual invalid input during the
ToValidUTF8 step and does not discard legitimate “�” runes during the rune loop;
keep the control-character handling and whitespace normalization intact.
1 participant
@
What
Two ingest/parse robustness fixes.
Title sanitize
An ingest
titlewith invalid UTF-8 bytes (a client that mangled an em-dash in the multipart field) made the Postgres insert fail withdb write failed→ 500 on the whole upload.sanitizeTitlenow coerces the title to valid UTF-8 + strips control chars before the DB write; the parsed-title path is likewise run throughcleanForLLM. A bad title byte can no longer fail an ingest.Cover-page furniture
isRepeatedMarkerLinedrops rows that are mostly the same short token repeated — an icon glyph rendered as text and stamped once per item (e.g. the ORCIDiDlogo on a paper byline →iD iD iD iD), which the font heuristic otherwise surfaced as junk author-block sections. Guard: only fires when an identical ≤2-char token is ≥60% of a ≥3-token line, so real prose and author lines with names are untouched.Scope note
GDPR-style justified-text word-splitting (
r ights,par ticular) is a deeper, riskier word-rejoin and is intentionally not in this PR.Tests
TestSanitizeTitle,TestIsRepeatedMarkerLine(drop/keep boundaries).go build ./...+go test ./...green.@
Summary by CodeRabbit