From f7c4dfe2e8037faf3707b370a853288f204e3687 Mon Sep 17 00:00:00 2001 From: John O'Keefe Date: Tue, 8 Sep 2026 22:32:51 -0400 Subject: [PATCH] fix(sync): resolve CRE positions structurally with text as verification Drop-cap markup like

Convergence of Heaven and Earth

made getTextFromXPointer return just 'C'. The text search then matched the first 'C' in the chapter and stored doc-start (/4/2/1:0) with 'precision: exact', so the web reader reopened at the chapter start while the percentage looked mid-chapter. - Add convertByStructuralPath: walk the parsed CRE ElementPath against the raw XHTML (same-tag 1-based indexing, mirroring buildCREXPointer), map CharOffset into the target element's text, and build the CFI. Usable device text verifies the landing; disagreement falls through instead of storing a confident-but-wrong CFI. - Add usableContextText guard (>=8 runes, >=2 words): single chars can never claim an exact text-search hit in either direction (ConvertCREToStandard and reverseByTextSearch). - Add drop-cap regression fixtures plus usable-context unit tests. - Verified against the real book: DocFragment[26]/p[12]/span header now converts to epubcfi(/6/52!/4/28/2/1:0) structural both with 'C' and the full header, and round-trips back to DocFragment[26]. --- internal/sync/cfi_converter.go | 230 +++++++++++++++++++++++++++- internal/sync/cfi_converter_test.go | 147 ++++++++++++++++++ 2 files changed, 372 insertions(+), 5 deletions(-) diff --git a/internal/sync/cfi_converter.go b/internal/sync/cfi_converter.go index cf42213..8a3803b 100644 --- a/internal/sync/cfi_converter.go +++ b/internal/sync/cfi_converter.go @@ -324,16 +324,233 @@ func (c *CFIConverter) ConvertCREToStandard(xpointer string, storedPercentage fl }, nil } - if contextText != "" { - result := c.convertByTextSearch(body, xp, spine, href, storedPercentage, contextText) - if result != nil { + // Primary: structural walk of the CRE element path. This is exact when + // the live CRE DOM still lines up with the raw XHTML (same-tag indices, + // see buildCREXPointer for the counting rule). Text is the final + // verification, not a fallback: a usable context that disagrees with + // the structural landing rejects it so text search gets a chance. + if len(xp.ElementPath) > 0 { + if result := c.convertByStructuralPath(body, xp, spine, href, storedPercentage, contextText); result != nil { return result, nil } } + if usable, normalized := usableContextText(contextText); usable { + result := c.convertByTextSearch(body, xp, spine, href, storedPercentage, normalized) + if result != nil { + return result, nil + } + } else if normalizeWhitespace(contextText) != "" { + log.Printf("Bookhoard: ignoring too-short context %q for text search in %s (structural missed)", contextText, href) + } + return c.convertByPercentageOffset(body, xp, spine, href, storedPercentage) } +// usableContextText reports whether a device context string is distinctive +// enough to claim an "exact" text-search hit. Single characters from split +// inline markup (e.g. drop-cap Convergence) must never qualify: +// findTextInNode would match the first "C" in the chapter (doc start). +// Callers treat unusable as empty and fall through to the next strategy. +func usableContextText(s string) (bool, string) { + normalized := normalizeWhitespace(s) + if normalized == "" { + return false, "" + } + if utf8.RuneCountInString(normalized) < 8 { + return false, "" + } + if len(strings.Fields(normalized)) < 2 { + return false, "" + } + return true, normalized +} + +// resolveCREElementPath walks a parsed CRE element path against the raw +// XHTML body. Indexing mirrors buildCREXPointer: 1-based nth element with +// the same tag among element siblings (case-insensitive). Returns nil when +// the live CRE DOM has drifted too far from disk (splits, injected wrappers) +// so callers can fall through instead of landing on the wrong node. +func resolveCREElementPath(body *html.Node, path []pathStep) *html.Node { + current := body + for _, step := range path { + if step.tag == "" { + continue + } + if strings.EqualFold(step.tag, "body") { + continue + } + var matches []*html.Node + for child := current.FirstChild; child != nil; child = child.NextSibling { + if child.Type == html.ElementNode && strings.EqualFold(child.Data, step.tag) { + matches = append(matches, child) + } + } + if len(matches) == 0 { + return nil + } + idx := step.index - 1 + if idx < 0 || idx >= len(matches) { + return nil + } + current = matches[idx] + } + if current == body { + return nil + } + return current +} + +// firstTextDescendant returns the first text node with non-whitespace content +// under n (document order), for structural landings on elements. +func firstTextDescendant(n *html.Node) *html.Node { + var found *html.Node + var walk func(*html.Node) bool + walk = func(node *html.Node) bool { + if node.Type == html.TextNode && strings.TrimSpace(node.Data) != "" { + found = node + return true + } + for child := node.FirstChild; child != nil; child = child.NextSibling { + if walk(child) { + return true + } + } + return false + } + walk(n) + return found +} + +// textNodeAtRuneOffset walks text nodes under elem in document order and +// returns the node containing the rune offset plus the local offset within +// that node. Offsets beyond the end clamp to the last node. +func textNodeAtRuneOffset(elem *html.Node, offset int) (*html.Node, int) { + if offset < 0 { + offset = 0 + } + var target *html.Node + local := 0 + remaining := offset + var walk func(*html.Node) bool + walk = func(node *html.Node) bool { + if node.Type == html.TextNode { + length := utf8.RuneCountInString(node.Data) + if remaining < length { + target = node + local = remaining + return true + } + remaining -= length + target = node + local = length + return false + } + for child := node.FirstChild; child != nil; child = child.NextSibling { + if walk(child) { + return true + } + } + return false + } + walk(elem) + return target, local +} + +// blockFlattenedText returns the flattened visible text of the nearest +// non-inline block ancestor (span/a/em/etc. are transparent), so drop-cap +// splits like Convergence verify against the full words. +func blockFlattenedText(textNode *html.Node) string { + block := findBlockParent(textNode) + if block == nil { + if textNode.Parent != nil { + block = textNode.Parent + } else { + return strings.TrimSpace(textNode.Data) + } + } + segments := collectInlineText(block) + var runes []rune + for _, seg := range segments { + runes = append(runes, seg.runes...) + } + // Include leading direct text children the inline walk skips. + var direct []rune + for child := block.FirstChild; child != nil; child = child.NextSibling { + if child.Type == html.TextNode { + direct = append(direct, []rune(child.Data)...) + } + } + combined := string(direct) + if flattened := string(runes); flattened != "" { + if combined != "" { + combined += " " + flattened + } else { + combined = flattened + } + } + return normalizeWhitespace(combined) +} + +func (c *CFIConverter) convertByStructuralPath(body *html.Node, xp *CREXPointer, spine *spineCache, href string, storedPercentage float64, contextText string) *ConversionResult { + elem := resolveCREElementPath(body, xp.ElementPath) + if elem == nil { + return nil + } + + var textNode *html.Node + var localOffset int + if xp.CharOffset > 0 { + textNode, localOffset = textNodeAtRuneOffset(elem, xp.CharOffset) + } else { + textNode = firstTextDescendant(elem) + localOffset = 0 + } + if textNode == nil { + return nil + } + + // Text is the final verification: when the device sent usable words and + // they disagree with this structural landing, reject it and let text + // search / percentage decide rather than storing a confident-but-wrong CFI. + if usable, normalized := usableContextText(contextText); usable { + flat := blockFlattenedText(textNode) + if flat != "" && !strings.Contains(flat, normalized) && !strings.Contains(normalized, flat) { + // Compare a prefix too: device sends ~100 chars from the reader + // position while the block may be longer. + prefix := normalized + if utf8.RuneCountInString(prefix) > 40 { + runes := []rune(prefix) + prefix = string(runes[:40]) + } + if !strings.Contains(flat, prefix) { + log.Printf("Bookhoard: structural landing disagrees with context in %s (block %q vs ctx %q)", href, truncateForLog(flat, 80), truncateForLog(normalized, 80)) + return nil + } + } + } + + spineIndex := xp.FragmentIndex - 1 + cfi, err := buildCFI(spineIndex, textNode, localOffset) + if err != nil || cfi == "" { + return nil + } + log.Printf("Bookhoard: structural match → %s (precision: structural)", cfi) + return &ConversionResult{ + EPUBCFI: cfi, + Href: href, + Percentage: storedPercentage, + Precision: "structural", + } +} + +func truncateForLog(s string, n int) string { + if utf8.RuneCountInString(s) <= n { + return s + } + return string([]rune(s)[:n]) + "…" +} + func (c *CFIConverter) convertFragmentID(s string, storedPercentage float64) (*ConversionResult, error) { frag, err := ParseCREFragmentID(s) if err != nil { @@ -1306,8 +1523,11 @@ func (c *CFIConverter) ConvertStandardToCRE(epubcfi string, storedPercentage flo } func (c *CFIConverter) reverseByTextSearch(epubcfi string, storedPercentage float64, contextText string) (*ReverseConversionResult, error) { - normalizedCtx := normalizeWhitespace(contextText) - if normalizedCtx == "" { + usable, normalizedCtx := usableContextText(contextText) + if !usable { + if normalizeWhitespace(contextText) != "" { + log.Printf("Bookhoard: CFI→CRE ignoring too-short context %q", contextText) + } return &ReverseConversionResult{ Precision: "percentage", Percentage: storedPercentage, diff --git a/internal/sync/cfi_converter_test.go b/internal/sync/cfi_converter_test.go index 035cfc9..08d124f 100644 --- a/internal/sync/cfi_converter_test.go +++ b/internal/sync/cfi_converter_test.go @@ -519,3 +519,150 @@ func parseTestHTML(s string) *html.Node { } return doc } + +func TestUsableContextText(t *testing.T) { + if ok, _ := usableContextText("C"); ok { + t.Error("single char must not be usable context") + } + if ok, _ := usableContextText("Chapter"); ok { + t.Error("single word must not be usable context") + } + if ok, norm := usableContextText("Convergence of Heaven and Earth"); !ok || norm == "" { + t.Error("full header must be usable context") + } + if ok, _ := usableContextText(""); ok { + t.Error("empty must not be usable context") + } +} + +// Drop-cap regression:

Convergence of Heaven and Earth

. +// A device sitting on that header used to send context "C", which text +// search matched at the chapter heading ("Chapter 10") and stored doc-start +// (/4/2/1:0). Structural resolution must land in the header paragraph. +func writeDropCapEPUB(t *testing.T) string { + t.Helper() + + docs := []struct { + name string + body string + }{ + {"ch9.xhtml", `

Chapter 9

Is the Kaphar of Christ the Gospel?

Opening of chapter nine with some text.

`}, + {"ch10.xhtml", `

Chapter 10

The Three C's of the New Covenant

The Cleansing Life of Christ

Present yourselves as slaves for obedience, you are slaves of that same one whom you obey, either of sin resulting in death.

Convergence of Heaven and Earth

The overarching goal of Christ is to converge heaven and earth.

`}, + } + + containerXML := ` + + + + +` + + manifest := "" + spineRefs := "" + for _, d := range docs { + id := d.name[:len(d.name)-len(".xhtml")] + manifest += " \n" + spineRefs += " \n" + } + opf := ` + + + dropcap-fixture + Dropcap Fixture + + +` + manifest + ` + +` + spineRefs + ` +` + + path := t.TempDir() + "/dropcap.epub" + f, err := os.Create(path) + if err != nil { + t.Fatal(err) + } + defer f.Close() + zw := zip.NewWriter(f) + write := func(name, content string) { + w, err := zw.Create(name) + if err != nil { + t.Fatal(err) + } + if _, err := w.Write([]byte(content)); err != nil { + t.Fatal(err) + } + } + write("META-INF/container.xml", containerXML) + write("OEBPS/content.opf", opf) + for _, d := range docs { + write("OEBPS/"+d.name, "\n"+d.body+"\n") + } + if err := zw.Close(); err != nil { + t.Fatal(err) + } + return path +} + +func TestDropCapSingleCharDoesNotHitDocStart(t *testing.T) { + c := NewCFIConverter(writeDropCapEPUB(t)) + + // CRE xpointer into the drop-cap header paragraph: + // DocFragment[2] = ch10 (1-based), body/p[3] = Convergence header + // (same-tag indexing, mirrors buildCREXPointer). + xp := "/body/DocFragment[2]/body/p[3]/span[1]/text().0" + result, err := c.ConvertCREToStandard(xp, 0.52, "C") + if err != nil { + t.Fatalf("ConvertCREToStandard error: %v", err) + } + t.Logf("Drop-cap single-char → %s (%s)", result.EPUBCFI, result.Precision) + if result.EPUBCFI == "" { + t.Fatal("expected non-empty epubcfi") + } + if strings.HasSuffix(result.EPUBCFI, "/4/2/1:0)") { + t.Errorf("single-char context collapsed to doc start: %s", result.EPUBCFI) + } +} + +func TestDropCapStructuralLandsInHeader(t *testing.T) { + c := NewCFIConverter(writeDropCapEPUB(t)) + + xp := "/body/DocFragment[2]/body/p[3]/span[1]/text().0" + result, err := c.ConvertCREToStandard(xp, 0.52, "Convergence of Heaven and Earth") + if err != nil { + t.Fatalf("ConvertCREToStandard error: %v", err) + } + t.Logf("Drop-cap structural → %s (%s)", result.EPUBCFI, result.Precision) + if result.EPUBCFI == "" { + t.Fatal("expected non-empty epubcfi") + } + if strings.HasSuffix(result.EPUBCFI, "/4/2/1:0)") { + t.Errorf("full header context still collapsed to doc start: %s", result.EPUBCFI) + } + if result.Precision != "structural" && result.Precision != "exact" { + t.Errorf("expected structural/exact precision, got %s", result.Precision) + } + + // Round-trip back to CRE must stay in the same fragment (web → mobile). + reverse, err := c.ConvertStandardToCRE(result.EPUBCFI, result.Percentage, "Convergence of Heaven and Earth") + if err != nil { + t.Fatalf("reverse conversion error: %v", err) + } + if reverse.XPointer == "" { + t.Fatal("expected non-empty reverse XPointer") + } + if !strings.Contains(reverse.XPointer, "DocFragment[2]") { + t.Errorf("expected reverse into DocFragment[2], got %s", reverse.XPointer) + } +} + +func TestReverseIgnoresSingleCharContext(t *testing.T) { + c := NewCFIConverter(writeDropCapEPUB(t)) + + reverse, err := c.ConvertStandardToCRE("epubcfi(/6/4!/4/99999/1:0)", 0.5, "C") + if err != nil { + t.Fatalf("reverse conversion error: %v", err) + } + if reverse.Precision != "percentage" { + t.Errorf("single-char reverse context must fall back to percentage, got %s", reverse.Precision) + } +}