fix(sync): understand cross-block text; never store a guessed locator

Tonight's failures all traced to one blind spot: the converter could only reason about text within a single block. A position at a chapter heading sends walk-up context (heading + the paragraphs below, joined by the plugin's block capture); a selection can span several paragraphs. Neither shape could be verified (containment compared one block against a multi-block quote, so the CORRECT structural landing at the heading was rejected) nor matched by text search (it never crossed block boundaries). The ladder then fell to the percentage rung — which labeled its char-count guess Precision "exact" — and that confidently-wrong CFI was stored: reading positions reopened paragraphs away from the true spot, and a highlight echo overwrote the row's good web CFIs with a garbage start anchor that made the highlight unpaintable ("disappeared"). Four changes, all in the forward converter and its consumers: - Quote verification: after the structural walk lands, read the whitespace-normalized document text forward from the landing point (crossing block boundaries; inline spans join directly so drop-cap splits still read as one word). A usable context must be a prefix of that stream — which is exactly what device captures are: the text from the position onward, or the selection between two anchors. The old single-block containment checks remain as secondary acceptance. - Cross-block text search: the search rung matches against the whole document flattened in reading order, with every rune mapped back to its source node and offset. A context spanning blocks now matches, and the matched extent yields a true range end (EndEPUBCFI) that highlights use as their end anchor, threaded through the facade as CanonicalLocator.EndCFI. - Honest labels: the percentage rung returns Precision "percentage" — a char-count estimate must never masquerade as an exact anchor. - Confident-only storage: progress adopts a converted locator solely at structural/exact precision (section hrefs keep their legacy handling; anything lower stores percentage only), and highlight conversion returns CFIs only at structural/exact precision — a low-confidence echo yields empty, which applyLWW coalescing turns into preservation of the row's existing web CFIs instead of clobbering them. Tests: walk-up context at a heading verifies structurally and lands in the heading; a block-spanning context is found by search with a range end landing in the following paragraph; the percentage rung is honestly labeled; all drop-cap guards stay green.
2026-09-09 20:15:12 -04:00
parent ce3ae31ced
commit b6f507b9e5
4 changed files with 293 additions and 43 deletions
@@ -12,6 +12,7 @@ import (
 	"strconv"
 	"strings"
 	"sync"
+	"unicode"
 	"unicode/utf8"

 	"golang.org/x/net/html"
@@ -245,7 +246,12 @@ func parseElementPart(part string) (string, int) {
 }

 type ConversionResult struct {
-	EPUBCFI    string
+	EPUBCFI string
+	// EndEPUBCFI is set when the conversion matched a context that is a
+	// quote of the document (text search): the range end anchor of the
+	// quoted text, valid across block boundaries. Selections use it as
+	// the highlight end; point positions ignore it.
+	EndEPUBCFI string
 	Href       string
 	Percentage float64
 	Precision  string
@@ -510,23 +516,22 @@ func (c *CFIConverter) convertByStructuralPath(body *html.Node, xp *CREXPointer,
 		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.
+	// Text is the final verification, in quote form: a usable context must
+	// be a prefix of the document as read forward from this landing point.
+	// Device captures are exactly that shape — a position's context is the
+	// text from the position onward (the plugin's walk-up may concatenate
+	// the heading with the paragraphs below), and a selection is the
+	// document text between its two anchors. The single-block containment
+	// checks stay as secondary acceptance for reverse shapes.
 	if usable, normalized := usableContextText(contextText); usable {
+		doc := documentTextFrom(body, textNode, localOffset, utf8.RuneCountInString(normalized)+64)
 		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
-			}
+		if !(strings.HasPrefix(doc, normalized) ||
+			strings.Contains(flat, normalized) ||
+			strings.Contains(normalized, flat)) {
+			log.Printf("Bookhoard: structural landing disagrees with context in %s (reads %q vs ctx %q)",
+				href, truncateForLog(doc, 80), truncateForLog(normalized, 80))
+			return nil
 		}
 	}

@@ -592,28 +597,149 @@ func (c *CFIConverter) convertFragmentID(s string, storedPercentage float64) (*C
 	}, nil
 }

+// docRunePos records which text node (and rune offset within it) produced
+// each rune of the flattened document text.
+type docRunePos struct {
+	node *html.Node
+	off  int
+}
+
+// flattenDocumentForSearch renders the document's text as a
+// whitespace-normalized rune stream in document order, crossing block
+// boundaries: a single space separates blocks, while inline spans join
+// directly (drop-cap splits like <span>C</span>onvergence read as one
+// word). Every emitted rune carries its source (node, rune offset) so
+// matches map back to CFIs.
+func flattenDocumentForSearch(body *html.Node) ([]rune, []docRunePos) {
+	var runes []rune
+	var poss []docRunePos
+	pendingSpace := false
+	var lastBlock *html.Node
+
+	appendNode := func(n *html.Node) {
+		block := findBlockParent(n)
+		if lastBlock != nil && block != lastBlock {
+			pendingSpace = true
+		}
+		lastBlock = block
+		off := 0
+		for _, r := range n.Data {
+			if unicode.IsSpace(r) {
+				pendingSpace = true
+			} else {
+				if pendingSpace && len(runes) > 0 {
+					runes = append(runes, ' ')
+					poss = append(poss, docRunePos{node: n, off: off})
+				}
+				pendingSpace = false
+				runes = append(runes, r)
+				poss = append(poss, docRunePos{node: n, off: off})
+			}
+			off++
+		}
+	}
+
+	var walk func(n *html.Node)
+	walk = func(n *html.Node) {
+		if n.Type == html.TextNode {
+			if strings.TrimSpace(n.Data) != "" {
+				appendNode(n)
+			}
+			return
+		}
+		for c := n.FirstChild; c != nil; c = c.NextSibling {
+			walk(c)
+		}
+	}
+	walk(body)
+	return runes, poss
+}
+
+// documentTextFrom reads the whitespace-normalized document text starting
+// at rune `offset` in `start` (typically a structural landing point),
+// crossing block boundaries — the document "as read" from that position.
+// Capped at maxRunes.
+func documentTextFrom(body *html.Node, start *html.Node, offset, maxRunes int) string {
+	runes, poss := flattenDocumentForSearch(body)
+	begin := -1
+	for i := range poss {
+		if poss[i].node == start && poss[i].off >= offset {
+			begin = i
+			break
+		}
+	}
+	if begin < 0 {
+		return ""
+	}
+	// The separator space before a block's first content rune shares its
+	// (node, offset) — skip it so the read starts on real text.
+	if runes[begin] == ' ' && begin+1 < len(runes) {
+		begin++
+	}
+	end := len(runes)
+	if begin+maxRunes < end {
+		end = begin + maxRunes
+	}
+	return string(runes[begin:end])
+}
+
 func (c *CFIConverter) convertByTextSearch(body *html.Node, xp *CREXPointer, spine *spineCache, href string, storedPercentage float64, contextText string) *ConversionResult {
 	normalizedCtx := normalizeWhitespace(contextText)
 	if normalizedCtx == "" {
 		return nil
 	}

-	match, matchOffset := findTextInNode(body, normalizedCtx)
-	if match == nil {
-		log.Printf("Bookhoard: text search no match for %q in %s", normalizedCtx, href)
+	// Match against the whole document flattened in reading order — the
+	// context may span block boundaries (a selection covering several
+	// paragraphs, a walk-up capture joining a heading with what follows).
+	runes, poss := flattenDocumentForSearch(body)
+	text := string(runes)
+
+	words := strings.Fields(normalizedCtx)
+	quoted := make([]string, len(words))
+	for i, w := range words {
+		quoted[i] = regexp.QuoteMeta(w)
+	}
+	pattern := strings.Join(quoted, `\s+`)
+	re, err := regexp.Compile(pattern)
+	if err != nil {
+		return nil
+	}
+	loc := re.FindStringIndex(text)
+	if loc == nil {
+		log.Printf("Bookhoard: text search no match for %q in %s", truncateForLog(normalizedCtx, 60), href)
 		return nil
 	}

+	startRune := utf8.RuneCountInString(text[:loc[0]])
+	endRune := utf8.RuneCountInString(text[:loc[1]]) // exclusive
 	spineIndex := xp.FragmentIndex - 1
-	cfi, err := buildCFI(spineIndex, match, matchOffset)
-	if err != nil || cfi == "" {
+
+	startPos := poss[startRune]
+	startCFI, err := buildCFI(spineIndex, startPos.node, startPos.off)
+	if err != nil || startCFI == "" {
 		log.Printf("Bookhoard: text search found match but buildCFI failed: %v", err)
 		return nil
 	}

-	log.Printf("Bookhoard: text search matched %q → %s (precision: exact)", normalizedCtx, cfi)
+	// The matched context is a quote of the document: its extent gives
+	// selections a true range end, across block boundaries.
+	endCFI := ""
+	if endRune > startRune && endRune <= len(poss) {
+		endPos := poss[endRune-1]
+		endOff := endPos.off + 1
+		if nRunes := utf8.RuneCountInString(endPos.node.Data); endOff > nRunes {
+			endOff = nRunes
+		}
+		if ec, err := buildCFI(spineIndex, endPos.node, endOff); err == nil && ec != "" {
+			endCFI = ec
+		}
+	}
+
+	log.Printf("Bookhoard: text search matched %q → %s (precision: exact)", truncateForLog(normalizedCtx, 60), startCFI)
 	return &ConversionResult{
-		EPUBCFI:    cfi,
+		EPUBCFI:    startCFI,
+		EndEPUBCFI: endCFI,
 		Href:       href,
 		Percentage: storedPercentage,
 		Precision:  "exact",
@@ -780,7 +906,10 @@ func (c *CFIConverter) convertByPercentageOffset(body *html.Node, xp *CREXPointe
 				EPUBCFI:    cfi,
 				Href:       href,
 				Percentage: storedPercentage,
-				Precision:  "exact",
+				// This is a char-count estimate, not an exact landing: say
+				// so. Callers gate storage on precision, and a guess must
+				// never masquerade as an exact anchor.
+				Precision: "percentage",
 			}, nil
 		}
 	}
@@ -667,6 +667,96 @@ func TestReverseIgnoresSingleCharContext(t *testing.T) {
 	}
 }

+// A position at a chapter heading sends walk-up context: the heading text
+// concatenated with the paragraphs below (block walk-up in the plugin).
+// The structural landing at the heading is correct and must verify — the
+// context is a prefix of the document as read from the landing.
+func TestWalkUpContextVerifiesAtHeading(t *testing.T) {
+	c := NewCFIConverter(writeDropCapEPUB(t))
+
+	// ch10: h1[1] "Chapter 10", h1[2] "The Three C's of the New Covenant",
+	// then paragraphs. Position at h1[2]'s text start; the device captured
+	// the heading plus the following paragraph.
+	xp := "/body/DocFragment[2]/body/h1[2]/text().0"
+	ctx := "The Three C's of the New Covenant The Cleansing Life of Christ"
+
+	result, err := c.ConvertCREToStandard(xp, 0.52, ctx)
+	if err != nil {
+		t.Fatalf("ConvertCREToStandard error: %v", err)
+	}
+	t.Logf("walk-up heading → %s (%s)", result.EPUBCFI, result.Precision)
+	if result.Precision != "structural" {
+		t.Fatalf("expected structural precision (quote verification), got %s (%s)", result.Precision, result.EPUBCFI)
+	}
+	if strings.HasSuffix(result.EPUBCFI, "/4/2/1:0)") {
+		t.Errorf("collapsed to doc start: %s", result.EPUBCFI)
+	}
+	// The landing must be the heading, not a paragraph below it.
+	reverse, rerr := c.ConvertStandardToCRE(result.EPUBCFI, result.Percentage, "")
+	if rerr != nil {
+		t.Fatalf("reverse conversion error: %v", rerr)
+	}
+	if !strings.Contains(reverse.XPointer, "h1[2]") {
+		t.Errorf("expected landing in h1[2], got %s", reverse.XPointer)
+	}
+}
+
+// A selection spanning blocks (heading tail into the next paragraph) must
+// be findable by text search across block boundaries, and the matched
+// quote's extent gives a true range end.
+func TestCrossBlockSearchSpansBlocks(t *testing.T) {
+	c := NewCFIConverter(writeDropCapEPUB(t))
+
+	// No element path → structural rung skipped, text search runs.
+	xp := "/body/DocFragment[2]/body"
+	ctx := "New Covenant The Cleansing Life of Christ"
+
+	result, err := c.ConvertCREToStandard(xp, 0.52, ctx)
+	if err != nil {
+		t.Fatalf("ConvertCREToStandard error: %v", err)
+	}
+	t.Logf("cross-block search → %s … %s (%s)", result.EPUBCFI, result.EndEPUBCFI, result.Precision)
+	if result.Precision != "exact" {
+		t.Fatalf("expected exact text-search precision, got %s", result.Precision)
+	}
+	if result.EPUBCFI == "" || result.EndEPUBCFI == "" {
+		t.Fatalf("expected range anchors, got %q…%q", result.EPUBCFI, result.EndEPUBCFI)
+	}
+	if result.EPUBCFI == result.EndEPUBCFI {
+		t.Fatalf("range collapsed: %s", result.EPUBCFI)
+	}
+
+	// The start lands in the heading, the end in the following paragraph.
+	startRev, err1 := c.ConvertStandardToCRE(result.EPUBCFI, result.Percentage, "")
+	endRev, err2 := c.ConvertStandardToCRE(result.EndEPUBCFI, result.Percentage, "")
+	if err1 != nil || err2 != nil {
+		t.Fatalf("reverse conversions failed: %v %v", err1, err2)
+	}
+	if !strings.Contains(startRev.XPointer, "h1[2]") {
+		t.Errorf("expected start in h1[2], got %s", startRev.XPointer)
+	}
+	if !strings.Contains(endRev.XPointer, "p[1]") {
+		t.Errorf("expected end in p[1] (following paragraph), got %s", endRev.XPointer)
+	}
+}
+
+// The percentage rung is a char-count estimate: it must never label its
+// landing "exact". Feed it an unmatchable context so the ladder falls all
+// the way through.
+func TestPercentageFallbackIsHonestlyLabeled(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, "zzz qqq vvv uuu www")
+	if err != nil {
+		t.Fatalf("ConvertCREToStandard error: %v", err)
+	}
+	t.Logf("unmatchable context → %s (%s)", result.EPUBCFI, result.Precision)
+	if result.Precision != "percentage" {
+		t.Errorf("percentage rung must not claim exact, got %s", result.Precision)
+	}
+}
+
 // The bookmark route supplies no context (bookmark text is a display
 // label, never book text), so the facade must still resolve the drop-cap
 // xpointer structurally instead of collapsing to the document start.
@@ -14,7 +14,11 @@ const (
 )

 type CanonicalLocator struct {
-	CFI        string
+	CFI string
+	// EndCFI carries the matched context's range end (text-search
+	// conversions of selections) so callers can anchor a true highlight
+	// range across block boundaries.
+	EndCFI     string
 	Precision  string
 	Percentage float64
 }
@@ -94,7 +98,7 @@ func ConvertToCanonical(
 			return CanonicalLocator{CFI: devicePos, Precision: "fallback", Percentage: percentage}
 		}
 		if result.EPUBCFI != "" {
-			return CanonicalLocator{CFI: result.EPUBCFI, Precision: result.Precision, Percentage: result.Percentage}
+			return CanonicalLocator{CFI: result.EPUBCFI, EndCFI: result.EndEPUBCFI, Precision: result.Precision, Percentage: result.Percentage}
 		}
 		if result.Href != "" {
 			return CanonicalLocator{CFI: result.Href, Precision: result.Precision, Percentage: result.Percentage}