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

[cbcoutinho]

A markdown table row cannot contain a newline, so pymupdf4llm encodes every
line wrap inside a cell as a literal <br>. That marker records how wide
the column happened to be, not what the document says, and it lands in the
indexed text glued to the words on either side: a cell reading "ISO 27001"
is embedded as "ISO
27001", which no search for "ISO 27001" can match.

Narrow columns wrap constantly, so forms and questionnaires lose a large
share of their searchable phrases. Measured on three table-heavy documents:
19, 60 and 8 affected rows.

Replace the marker with a space, scoped to table rows -- prose that
legitimately discusses the <br> tag keeps saying so. The rewrite happens
before page_boundaries are measured, so the offsets pdf_highlighter and
the chunker index with stay exact rather than sliding right of their words.

Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com

---

_{Stack created with GitHub Stacks CLI • Give Feedback 💬}

---

This PR was generated with the help of AI, and reviewed by a Human

[claude]

Claude finished @cbcoutinho's task in 3m 15s —— View job

---

Review: PR #1264 — unwrap table-cell <br> breaks

• Gather context (PR body, comments, diff)
• Read nextcloud_mcp_server/document_processors/pymupdf.py changes
• Read tests/unit/test_pymupdf_table_breaks.py
• Check regex safety / ReDoS concerns (per round-4 Sonar fix)
• Verify offset-exactness claim (rewrite happens before page_boundaries measured)
• Post review feedback

Note: the sandbox's Bash tool required interactive approval for every command in this session (including git fetch, uv run pytest, even a bare python3 -c), so I could not execute the test suite / ruff / ty myself here. This is a code-reading review; the PR's own CI logs (3351 unit tests, ruff/ty, SonarCloud gate) are the executed source of truth, and I traced the regex by hand against the scenarios below to sanity-check them independently.

Summary

This is a clean, well-scoped fix, and it shows in the four review rounds already reflected in the comment history — each finding (over-reaching space collapse, the stale spreadsheet.py reference, the python:S8786 quadratic regex) was tracked down and fixed with a matching regression test. I don't have new blocking findings. A couple of optional notes below.

Correctness (traced by hand)

• _TABLE_CELL_BREAK_RE = re.compile(r"[ \t]?(?:<br\s*/?>[ \t]*)+", re.IGNORECASE) — single substitution, confirmed it can only rewrite the span it matched, so unrelated double-spaces in a sibling cell survive (tests/unit/test_pymupdf_table_breaks.py:43-52).
• The leading [ \t]? (vs. the earlier [ \t]*) is the right fix for python:S8786: with ?, re.search over a <br>-free padded line costs O(n) rather than O(n²), since each starting offset now does at most one bounded backtrack instead of re-scanning the whole trailing run. Matches the measured 20000-space timing in the PR comment (4.4s → sub-ms).
• One residual, minor, and I think acceptable asymmetry: a <br> b (two spaces on each side) collapses to a b — the trailing run is fully absorbed ([ \t]* inside the repeated group, safe because nothing follows it to force backtracking) but only the single space adjacent to the tag is absorbed on the leading side, so an extra leading space survives. This is the explicitly-chosen "safer miss" from the round-4 writeup, not a new issue — just flagging that it's a slight behavioral asymmetry between the two sides of the tag in case a future reader is surprised by it.
• Offset exactness: confirmed _unwrap_table_cell_breaks runs on chunk.get("text", "") before current_offset/page_boundaries are computed in _build_text_and_metadata (pymupdf.py:241-256), so the claim in the PR description holds and is covered by test_page_boundaries_match_the_rewritten_text.

Test coverage

Thorough — tag spelling variants, consecutive breaks, per-row and per-line isolation (prose vs. table row, break-free sibling row), the offset-exactness contract, and a timing guard for the ReDoS fix. Two very small optional gaps, neither blocking:

• No case with a literal tab (\t) in the padding, only spaces — the character class includes \t but it's untested.
• The known line.lstrip().startswith("|") heuristic limitation (prose starting with a literal | that also contains <br>) is acknowledged in the round-2 comment and intentionally left; agree with leaving it, not worth a test for documented-and-accepted behavior.

Style / conventions

Matches repo conventions: re import correctly alphabetized, full type hints, comments explain why (the ReDoS rationale, the single-substitution rationale) rather than what, consistent with the file's existing dense-comment style elsewhere (e.g. the filetype="pdf" block above it).

No security concerns — this only rewrites literal <br> markup lifted from already-parsed PDF text into markdown; no new I/O, no injection surface.

[cbcoutinho]

Round-1 nit addressed in ba7dcebb:

• Consecutive breaks collapsing to multiple spaces: now normalised with one extra regex, confined to the table rows being rewritten so prose keeps its own spacing. It matches how spreadsheet.py already normalises whitespace inside a cell, and still happens before page_boundaries are measured — covered by two new cases.

3344 unit tests pass; ruff/ty green. Sonar reports no issues on this PR.

[cbcoutinho]

Round-2 finding addressed in cd1d64a3:

• 🟡 Space-collapse leaking onto rows without a break: you are right and I had talked myself out of exactly this. The early-out is per page, so one <br> anywhere sent every other |-prefixed line on that page through _SPACE_RUN_RE, quietly eating double spaces the document really contains. Now gated on unwrapped != line, so only lines this function actually rewrote get normalised — with the regression test you asked for (a break-free row sharing a page with one that has a break), plus one for a page with no breaks at all.

• 🟢 The spreadsheet.py precedent in the commit message: good catch, and worse than a stale reference — that file genuinely does not exist at this point in the stack (it arrives two commits later in feat(processors): index .doc/.docx, .xls/.xlsx and .msg documents #1265), so the message compared against code the reader cannot see. Removed rather than reworded; the amended message says so explicitly.

• 🟢 line.lstrip().startswith("|") sweeping in prose that starts with a literal pipe: acknowledged and left. It is the same heuristic the surrounding code uses to recognise a pymupdf4llm table row, and the failure mode is now much narrower than before this round: such a line is only touched if it also contains a <br>.

3348 unit tests pass; ruff/ty green.

[cbcoutinho]

Round-3 finding addressed in b314f614 — and you were right to push back twice.

• 🟡 Collapse still reaching unrelated cells in the same row: my round-2 fix narrowed the filter (only lines the substitution changed) but left exactly the case you name: within such a line, _SPACE_RUN_RE still ran over the whole row, so a legitimate double space in a different cell was eaten.

  Rather than narrow the filter a third time I took your suggestion and dropped the two-step entirely. One regex — [ \t]*(?:<br\s*/?>[ \t]*)+ → a single space — matches a run of adjacent breaks plus surrounding spaces, so it can only ever rewrite the text it matched. That removes the whole class of over-reach instead of trimming its edges, and it is less code than the version it replaces.

  Tests: the mixed-cell row you asked for (| kept spacing | a<br><br>b | more spacing |), spaces absorbed around a break, plus the two from last round.

• 🟢 The regex matching <br/>, <br />, <BR>: kept. It costs nothing, and the check and the substitution share one pattern so they cannot disagree about what a tag is — which is why the case-insensitive form went in originally.

3350 unit tests pass; ruff/ty green.

[cbcoutinho]

CI caught a real one that I introduced in the last round — fixed in ac04416d.

SonarCloud python:S8786 on the new regex: [ \t]*(?:<br\s*/?>[ \t]*)+ has super-linear backtracking. The leading [ \t]* lets the engine consume a whole run of spaces, fail to find a <br> after it, and retry one character shorter. A rendered table is mostly padding runs, so this is reachable by ordinary input rather than a crafted one. It failed the quality gate (new_maintainability_rating=2), which is the check working exactly as intended.

Measured on a single padded row before and after:

spaces	[ \t]*	[ \t]?
2,000	0.056s	0.0001s
8,000	0.709s	0.0004s
20,000	4.371s	0.0011s

Clearly quadratic. The leading quantifier is now ?: one optional character cannot backtrack, and one is all the common word <br> word case needs. A wider run of spaces immediately before a break is now left alone rather than absorbed — the safer miss, and consistent with this round's theme of not reaching past the break being repaired.

Added a timing guard on a 20k-space row (1s ceiling) — I checked it genuinely discriminates rather than passing either way: the previous form takes 4.4s on that input.

3351 unit tests pass; ruff/ty green.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud