refactor(html): unify duplicated DOM-walk recursion into one canonical walker

[lodyai]

Background

src/html.rs contains two DOM-walking helpers that are byte-for-byte identical
apart from the tag they match and the vector they populate:

• collect_tables (src/html.rs:106-113) — recurses collecting <table> nodes.
• collect_rows (src/html.rs:116-123) — recurses collecting <tr> nodes.

fn collect_tables(handle: &Handle, tables: &mut Vec<Handle>) {
    if is_element(handle, "table") { tables.push(handle.clone()); }
    for child in handle.children.borrow().iter() { collect_tables(child, tables); }
}

Problem

The recursion is duplicated, so any change to traversal (depth limits, ordering,
borrow handling) must be made twice. The neighbouring is_table_cell
(src/html.rs:103) implies a third potential walk over <td>/<th>.

Objective

Replace both functions with a single canonical walker and route all callers
through it.

• Add fn collect_elements(handle: &Handle, tag: &str, out: &mut Vec<Handle>),
  or a predicate-taking variant
  fn collect_matching(handle: &Handle, pred: impl Fn(&Handle) -> bool, out: &mut Vec<Handle>)
  so cell collection can reuse it too.
• Express collect_tables/collect_rows as thin call-throughs or remove them
  in favour of direct calls.

Required approach

1. One canonical walker. Exactly one recursive collection function; remove
   the duplicated bodies and route every caller through it.
2. Document it. Add an entry to docs/developers-guide.md (Internal API
   reference / HTML parser section) naming the canonical walker, its contract
   (pre-order traversal, clones matching handles), and the rule that new DOM
   collection must reuse it.
3. Property tests. Add proptest coverage over generated small HTML
   fragments asserting invariants such as: count returned by collect_elements
   equals the number of matching tags in the source; nesting order is
   deterministic; non-matching documents yield an empty vector.
4. Model checking (if applicable). The DOM input is unbounded; bounded model
   checking is not a good fit — record this rationale in the PR rather than
   adding a Kani harness.
5. Snapshot assertions. Add insta snapshot tests pinning the
   HTML-to-Markdown table conversion output for representative nested-table and
   multi-row fixtures, so traversal regressions surface as diffs.

All existing tests must pass; run the full make commit gate before merging.

References

• Requested by @leynos

[coderabbitai]

🔗 Related PRs

#279 - Align html5ever rcdom dependencies (#278) [merged]
#294 - Keep link reference definitions verbatim during wrap (#292) [merged]

---

📝 Issue Planner

_{Check the box below or use the @coderabbitai plan command to generate an implementation plan and prompts that you can use with your favorite coding assistant.}

✅ Successfully generated implementation plan.

💡 Use @coderabbitai <feedback> to replan

---

🧪 Issue enrichment is currently in open beta.

You can configure auto-planning by selecting labels in the issue_enrichment configuration.

To disable automatic issue enrichment, add the following to your .coderabbit.yaml:

issue_enrichment:
  auto_enrich:
    enabled: false

💬 Have feedback or questions? Drop into our discord!

[coderabbitai]

Coding Plan

Summary

• Introduce collect_matching, a predicate-taking recursive helper, as the single source of DOM-walking logic.
• Retain collect_tables and collect_rows as thin wrappers to preserve call-site semantics.
• Add proptest invariants validating count, ordering, and empty-vector behaviour.
• Pin conversion output with insta snapshots for nested and multi-row tables to detect traversal regressions.

Design Choices

Design Choice 1: Wrapper functions vs. direct calls

Options Considered:

1. Remove both functions and replace all call-sites with direct invocations of the canonical walker.
2. Retain both functions as one-liner wrappers delegating to the canonical walker.

Chosen Option: 2

Rationale: Option 2 — retaining thin wrappers preserves semantic call-site clarity (e.g., collect_tables is self-documenting) and avoids touching code that merely consumes the collected handles.

Design Choice 2: Canonical walker signature

Options Considered:

1. collect_elements(handle, tag, out) — simple, tag-based.
2. collect_matching(handle, pred, out) — predicate-based, supports compound checks like is_table_cell.

Chosen Option: 2

Rationale: Option 2 — the predicate variant is more flexible, allows is_table_cell (which matches two tags) to be passed directly, and aligns with the ticket's hint about a third potential walk.

💡 User Tips

Regenerate the plan with different choices with @coderabbitai <feedback>.

Implementation Steps

Phase 1: Core Refactoring — Canonical Walker

Introduce a single recursive helper and route existing callers through it.

Task 1: Add `collect_matching` helper

Implement the canonical DOM walker that accepts a predicate closure.

• Create fn collect_matching<F>(handle: &Handle, pred: F, out: &mut Vec<Handle>) where F: Fn(&Handle) -> bool in src/html.rs, immediately below is_table_cell.
• Implement pre-order depth-first traversal: if pred(handle) is true, push handle.clone(); then recurse into children.
• Keep the signature private (pub(crate) unnecessary; module is already private).

Task 2: Convert `collect_tables` and `collect_rows` to wrappers

Remove duplicated recursion; delegate to collect_matching.

• Rewrite collect_tables as a one-liner: call collect_matching(handle, |h| is_element(h, "table"), out).
• Rewrite collect_rows similarly for "tr".
• Verify existing callers (table_lines_to_markdown, table_node_to_markdown) compile unchanged.

Task 3: Verify existing unit tests pass

Run the test suite to confirm no regressions.

• Execute cargo test --lib and cargo test --test '*' to check all existing assertions.
• Fix any compilation or assertion failures before proceeding.

🤖 Prompt for AI agents

Implement the core refactoring in `src/html.rs` to eliminate duplicated
DOM-walking logic by introducing a single canonical walker function.

- In `src/html.rs`, immediately below the `is_table_cell` function, add a new
private function:
- Signature: `fn collect_matching<F>(handle: &Handle, pred: F, out: &mut
Vec<Handle>) where F: Fn(&Handle) -> bool`
  - Implement pre-order depth-first traversal:
    - If `pred(handle)` evaluates to `true`, push `handle.clone()` into `out`
    - Recurse into all children of `handle`
  - Do NOT add `pub` or `pub(crate)` — the module is already private
- Rewrite `collect_tables` as a one-liner that calls `collect_matching(handle,
|h| is_element(h, "table"), out)`
- Rewrite `collect_rows` as a one-liner that calls `collect_matching(handle, |h|
is_element(h, "tr"), out)`
- Confirm that all existing callers — `table_lines_to_markdown` and
`table_node_to_markdown` — compile unchanged
- Run `cargo test --lib` and `cargo test --test '*'` and fix any compilation or
assertion failures before proceeding

Phase 2: Test Coverage — Property and Snapshot Tests

Add proptest invariants and insta snapshots as specified.

Task 1: Property tests for `collect_matching`

Add proptest coverage to src/html_tests.rs.

• Create a strategy that generates small HTML fragments containing a variable number of <table>, <tr>, and <td> tags (e.g., 0–4 nested tables, 0–6 rows each).
• Property 1: count returned by collect_matching for a given tag equals the number of that tag in the source string.
• Property 2: ordering of collected handles is deterministic (call twice, compare).
• Property 3: a document containing no matching tags yields an empty vector.
• Use prop_assert! / prop_assert_eq! for assertions.

Task 2: Snapshot tests for table conversion output

Add insta snapshots in tests/table/ covering nested-table and multi-row fixtures.

• Create tests/table/html_snapshots.rs (or extend an existing file).
• Define fixtures for: single-row table, multi-row table, nested table (table inside a cell), table with <thead> and <tbody>.
• Use the codebase convention:

  insta::with_settings!({
      snapshot_path => "../snapshots",
      prepend_module_to_snapshot => false,
  }, { insta::assert_snapshot!(name, output); });

• Store resulting .snap files in tests/snapshots/ with descriptive names (e.g., html_nested_table.snap).

Task 3: Run full test suite

Confirm all new and existing tests pass.

• Execute make (or equivalent commit gate) to run lints, tests, and any integration checks.
• Review new snapshots with cargo insta review and commit accepted baselines.

🤖 Prompt for AI agents

Add property tests and insta snapshot tests to validate the behaviour of
`collect_matching` and the table conversion output.

**Property tests — `src/html_tests.rs`:**
- Add proptest-based tests using a strategy that generates small HTML fragments
containing a variable number of `<table>`, `<tr>`, and `<td>` tags (e.g., 0–4
nested tables, 0–6 rows each)
- Implement the following three property invariants using `prop_assert!` /
`prop_assert_eq!`:
- Property 1: the count of handles returned by `collect_matching` for a given
tag equals the number of that tag's occurrences in the source string
- Property 2: the ordering of collected handles is deterministic — call
`collect_matching` twice on the same input and compare results
  - Property 3: a document containing no matching tags yields an empty vector

**Snapshot tests — `tests/table/html_snapshots.rs`:**
- Create `tests/table/html_snapshots.rs` (or extend an existing file in that
directory)
- Define fixtures for:
  - Single-row table
  - Multi-row table
  - Nested table (a table inside a cell)
  - Table with `<thead>` and `<tbody>`
- Follow the codebase convention for snapshots:
  ~~~rust
  insta::with_settings!({
      snapshot_path => "../snapshots",
      prepend_module_to_snapshot => false,
  }, { insta::assert_snapshot!(name, output); });
  ~~~
- Store `.snap` files in `tests/snapshots/` with descriptive names such as
`html_nested_table.snap`
- Execute `make` (or equivalent commit gate) to run all lints, tests, and
integration checks; review new snapshots with `cargo insta review` and commit
accepted baselines

Phase 3: Documentation and PR Rationale

Update the developers guide and record the model-checking rationale.

Task 1: Document canonical walker in developers-guide.md

Add an entry to the Internal API reference section.

• In docs/developers-guide.md, under ## Internal API reference (around line 63), add a bullet for src/html.rs:
  • `collect_matching`: pre-order depth-first traversal; clones matching handles into the output vector; all new DOM collection must reuse this helper.
• Under ## Stateful pipeline helpers → ### HtmlTableState, add a note that collect_tables and collect_rows delegate to collect_matching.

Task 2: Record model-checking rationale

Document why Kani is not used.

• In the PR description (or a code comment above collect_matching), state: "DOM input is unbounded; bounded model checking is not a good fit for verifying traversal over arbitrary tree depth. Property tests cover invariants via random small-tree generation instead."

🤖 Prompt for AI agents

Update `docs/developers-guide.md` and add an inline rationale comment to
`src/html.rs` to complete the documentation for the canonical walker.

**`docs/developers-guide.md` changes:**
- Under `## Internal API reference` (around line 63), add a bullet for
`src/html.rs`:
- `` `collect_matching`: `` pre-order depth-first traversal; clones matching
handles into the output vector; all new DOM collection must reuse this helper
- Under `## Stateful pipeline helpers` → `### HtmlTableState`, add a note that
`collect_tables` and `collect_rows` delegate to `collect_matching`

**`src/html.rs` rationale comment:**
- Add a comment directly above the `collect_matching` function stating: "DOM
input is unbounded; bounded model checking is not a good fit for verifying
traversal over arbitrary tree depth. Property tests cover invariants via random
small-tree generation instead."

Research

The project is a Markdown processing tool written in Rust. src/html.rs converts HTML <table> blocks to GFM Markdown using html5ever and markup5ever_rcdom. The two functions collect_tables and collect_rows share identical recursion logic differing only in the tag matched. Tests live in src/html_tests.rs (unit and property) and tests/table/ (integration). Documentation follows British English conventions in docs/developers-guide.md, which already contains sections for the Internal API reference and HTML parser coupling.

---

🚀 Next Steps

🤖 All AI agent prompts combined

Task: 1

Implement the core refactoring in `src/html.rs` to eliminate duplicated
DOM-walking logic by introducing a single canonical walker function.

- In `src/html.rs`, immediately below the `is_table_cell` function, add a new
private function:
- Signature: `fn collect_matching<F>(handle: &Handle, pred: F, out: &mut
Vec<Handle>) where F: Fn(&Handle) -> bool`
  - Implement pre-order depth-first traversal:
    - If `pred(handle)` evaluates to `true`, push `handle.clone()` into `out`
    - Recurse into all children of `handle`
  - Do NOT add `pub` or `pub(crate)` — the module is already private
- Rewrite `collect_tables` as a one-liner that calls `collect_matching(handle,
|h| is_element(h, "table"), out)`
- Rewrite `collect_rows` as a one-liner that calls `collect_matching(handle, |h|
is_element(h, "tr"), out)`
- Confirm that all existing callers — `table_lines_to_markdown` and
`table_node_to_markdown` — compile unchanged
- Run `cargo test --lib` and `cargo test --test '*'` and fix any compilation or
assertion failures before proceeding
===============================================================================

Task: 2

Add property tests and insta snapshot tests to validate the behaviour of
`collect_matching` and the table conversion output.

**Property tests — `src/html_tests.rs`:**
- Add proptest-based tests using a strategy that generates small HTML fragments
containing a variable number of `<table>`, `<tr>`, and `<td>` tags (e.g., 0–4
nested tables, 0–6 rows each)
- Implement the following three property invariants using `prop_assert!` /
`prop_assert_eq!`:
- Property 1: the count of handles returned by `collect_matching` for a given
tag equals the number of that tag's occurrences in the source string
- Property 2: the ordering of collected handles is deterministic — call
`collect_matching` twice on the same input and compare results
  - Property 3: a document containing no matching tags yields an empty vector

**Snapshot tests — `tests/table/html_snapshots.rs`:**
- Create `tests/table/html_snapshots.rs` (or extend an existing file in that
directory)
- Define fixtures for:
  - Single-row table
  - Multi-row table
  - Nested table (a table inside a cell)
  - Table with `<thead>` and `<tbody>`
- Follow the codebase convention for snapshots:
  ~~~rust
  insta::with_settings!({
      snapshot_path => "../snapshots",
      prepend_module_to_snapshot => false,
  }, { insta::assert_snapshot!(name, output); });
  ~~~
- Store `.snap` files in `tests/snapshots/` with descriptive names such as
`html_nested_table.snap`
- Execute `make` (or equivalent commit gate) to run all lints, tests, and
integration checks; review new snapshots with `cargo insta review` and commit
accepted baselines
===============================================================================

Task: 3

Update `docs/developers-guide.md` and add an inline rationale comment to
`src/html.rs` to complete the documentation for the canonical walker.

**`docs/developers-guide.md` changes:**
- Under `## Internal API reference` (around line 63), add a bullet for
`src/html.rs`:
- `` `collect_matching`: `` pre-order depth-first traversal; clones matching
handles into the output vector; all new DOM collection must reuse this helper
- Under `## Stateful pipeline helpers` → `### HtmlTableState`, add a note that
`collect_tables` and `collect_rows` delegate to `collect_matching`

**`src/html.rs` rationale comment:**
- Add a comment directly above the `collect_matching` function stating: "DOM
input is unbounded; bounded model checking is not a good fit for verifying
traversal over arbitrary tree depth. Property tests cover invariants via random
small-tree generation instead."

💡 Iterate on the plan with: `@coderabbitai` <feedback>

Example Feedback
- `@coderabbitai` You can skip phase 3. Add a simple unit test case for phase 2.
- `@coderabbitai` For design choice 1 go ahead with option 3 and replan.

---

💬 Have feedback or questions? Drop into our discord!