fix(pymupdf): unwrap table-cell line breaks so their words stay searchable

nextcloud_mcp_server/document_processors/pymupdf.py

@@ -2,6 +2,7 @@
 
 import logging
 import pathlib
+import re
 import tempfile
 from collections.abc import Awaitable, Callable
 from typing import Any, Optional
@@ -29,6 +30,47 @@
 # note attachment, a deck card) and so has no path to name.
 _UNNAMED = "<bytes>"
 
+# A markdown table row cannot contain a newline, so pymupdf4llm encodes every
+# line wrap *inside* a cell as a literal <br>. That is a rendering artifact of
+# how wide the column happened to be, not content -- but it lands in the
+# indexed text glued to the words on either side, so a cell reading
+# "ISO 27001" is embedded as "ISO<br>27001" and no search for "ISO 27001" can
+# match it. Narrow columns wrap constantly, so a form or questionnaire loses a
+# large share of its searchable phrases this way.
+#
+# One run of adjacent breaks, with the spaces around it, becomes one space. A
+# single substitution rather than "replace each tag, then collapse the runs":
+# collapsing afterwards operates on the whole line, so a legitimate double
+# space in an unrelated cell of the same row would be eaten as collateral. This
+# form can only ever rewrite the text it matched.
+#
+# The leading space is `?`, not `*`, deliberately. With `[ \t]*` the engine can
+# consume a whole run of spaces, fail to find a `<br>` after it, then retry one
+# character shorter -- quadratic in the length of any space run on the line,
+# and rendered tables are full of padding runs (python:S8786). One optional
+# character cannot backtrack, and one is all the common `word <br> word` case
+# needs; a wider run before a break survives as-is, which is the safer miss.
+_TABLE_CELL_BREAK_RE = re.compile(r"[ \t]?(?:<br\s*/?>[ \t]*)+", re.IGNORECASE)
+
+
+def _unwrap_table_cell_breaks(text: str) -> str:
+    """Turn a table cell's line-wrap markers back into spaces.
+
+    Scoped to table rows rather than applied to the whole page: only pymupdf4llm's
+    table renderer emits these, and a document that legitimately discusses the
+    ``<br>`` tag in prose should keep saying so.
+    """
+    # One scan to skip the split/join on the overwhelmingly common page that has
+    # no table at all. The regex rather than a substring test so the check
+    # cannot disagree with the substitution about what counts as the tag.
+    if not _TABLE_CELL_BREAK_RE.search(text):
+        return text
+    lines = text.split("\n")
+    for i, line in enumerate(lines):
+        if line.lstrip().startswith("|"):
+            lines[i] = _TABLE_CELL_BREAK_RE.sub(" ", line)
+    return "\n".join(lines)
+
 
 def _record_parse_mode(
     metadata: dict[str, Any], page_count: int, settings: Any
@@ -193,7 +235,10 @@ def _build_text_and_metadata(
         page_boundaries: list[dict[str, Any]] = []
         current_offset = 0
         for chunk in page_chunks:
-            text = chunk.get("text", "")
+            # Before the offsets are taken, not after: page_boundaries index
+            # into this text exactly, so rewriting it afterwards would slide
+            # every highlight off its words.
+            text = _unwrap_table_cell_breaks(chunk.get("text", ""))
             # 1-based, from pymupdf4llm's classic extractor (the worker forces
             # it via use_layout(False); layout mode would name this
             # ``page_number`` instead). The ``page`` key written below is *our*

tests/unit/test_pymupdf_table_breaks.py

@@ -0,0 +1,132 @@
+"""Table cells keep their words searchable across a rendered line wrap."""
+
+import time
+
+import pytest
+
+from nextcloud_mcp_server.document_processors.pymupdf import (
+    PyMuPDFProcessor,
+    _unwrap_table_cell_breaks,
+)
+
+pytestmark = pytest.mark.unit
+
+
+def test_break_inside_a_cell_becomes_a_space():
+    """ "ISO<br>27001" is unfindable by anyone searching for "ISO 27001"."""
+    row = "|Certifications|Do you<br>hold ISO<br>27001?|Yes|"
+
+    assert _unwrap_table_cell_breaks(row) == (
+        "|Certifications|Do you hold ISO 27001?|Yes|"
+    )
+
+
+@pytest.mark.parametrize("tag", ["<br>", "<br/>", "<br />", "<BR>", "<Br />"])
+def test_every_spelling_of_the_tag_is_handled(tag):
+    assert _unwrap_table_cell_breaks(f"| a{tag}b |") == "| a b |"
+
+
+def test_consecutive_breaks_collapse_to_one_space():
+    """Each tag substitutes independently, so a run would otherwise remain."""
+    assert _unwrap_table_cell_breaks("| a<br><br>b |") == "| a b |"
+
+
+def test_space_collapsing_is_confined_to_table_rows():
+    """Prose keeps its own spacing -- only the rewritten rows are normalised."""
+    text = "Indented  prose  keeps  spacing.\n\n| a<br><br>b |"
+
+    assert _unwrap_table_cell_breaks(text) == (
+        "Indented  prose  keeps  spacing.\n\n| a b |"
+    )
+
+
+def test_an_unrelated_cell_in_the_same_row_keeps_its_double_space():
+    """The collapse must not reach past the break it is repairing.
+
+    Substituting each tag and then collapsing runs operated on the whole line,
+    so a legitimate double space in a different cell of the same row was eaten
+    as collateral.
+    """
+    row = "| kept  spacing | a<br><br>b | more  spacing |"
+
+    assert _unwrap_table_cell_breaks(row) == ("| kept  spacing | a b | more  spacing |")
+
+
+def test_spaces_around_a_break_are_absorbed_into_the_one_space():
+    assert _unwrap_table_cell_breaks("| a <br> b |") == "| a b |"
+
+
+def test_a_long_space_run_without_a_break_is_not_quadratic():
+    """A padded row must not make the regex backtrack over its whole run.
+
+    A `[ \\t]*` prefix lets the engine eat a space run, fail to find a `<br>`,
+    then retry one character shorter -- quadratic in the run length, and
+    rendered tables are mostly padding (python:S8786).
+    """
+    padded = "| " + " " * 20000 + "|\n| a<br>b |"
+
+    start = time.perf_counter()
+    result = _unwrap_table_cell_breaks(padded)
+    elapsed = time.perf_counter() - start
+
+    assert result.endswith("| a b |")
+    assert " " * 20000 in result, "the padding itself must be left alone"
+    # Linear scanning finishes in microseconds; the quadratic form took seconds
+    # on this input. A whole second is a generous ceiling that still fails loudly.
+    assert elapsed < 1.0, f"took {elapsed:.2f}s -- regex is backtracking"
+
+
+def test_a_row_without_a_break_keeps_its_own_double_spaces():
+    """The early-out is per page, so a sibling row must not be rewritten.
+
+    One <br> anywhere on the page used to send every other table row through
+    the space collapse, silently eating double spaces the document contains.
+    """
+    text = "| kept  spacing | here |\n| a<br><br>b |"
+
+    assert _unwrap_table_cell_breaks(text) == "| kept  spacing | here |\n| a b |"
+
+
+def test_a_page_with_no_breaks_at_all_is_untouched():
+    text = "| kept  spacing |\n| and  here |"
+
+    assert _unwrap_table_cell_breaks(text) is text
+
+
+def test_prose_outside_a_table_is_left_alone():
+    """A document explaining HTML should keep saying <br>."""
+    text = "The <br> tag inserts a line break.\n\n| a<br>b |"
+
+    assert _unwrap_table_cell_breaks(text) == (
+        "The <br> tag inserts a line break.\n\n| a b |"
+    )
+
+
+def test_text_without_breaks_is_returned_unchanged():
+    text = "# Heading\n\n| a | b |\n"
+
+    assert _unwrap_table_cell_breaks(text) is text
+
+
+def test_page_boundaries_match_the_rewritten_text():
+    """The offsets are taken after the rewrite, so highlights stay on the words.
+
+    Rewriting the text after measuring would leave every offset past the first
+    table pointing several characters too far right.
+    """
+    processor = PyMuPDFProcessor(extract_images=False)
+    metadata: dict = {}
+    chunks = [
+        {"text": "| a<br>b |\n", "metadata": {"page": 1}},
+        {"text": "second page\n", "metadata": {"page": 2}},
+    ]
+
+    text = processor._build_text_and_metadata(chunks, None, metadata)
+
+    assert text == "| a b |\nsecond page\n"
+    boundaries = metadata["page_boundaries"]
+    assert boundaries[0]["end_offset"] == len("| a b |\n")
+    assert boundaries[1]["start_offset"] == boundaries[0]["end_offset"]
+    assert boundaries[-1]["end_offset"] == len(text)
+    for span in boundaries:
+        assert text[span["start_offset"] : span["end_offset"]]