morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[PR #2480] [MERGED] fix(browser): distinguish SVG links from HTML XLink attributes #5260

New issue

Closed

opened 2026-10-31 01:05:14 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:05:14 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/2480
Author: @zhangguiping-xydt
Created: 10/13/2026
Status: ✅ Merged
Merged: 10/13/2026
Merged by: @morluto

Base: main ← Head: fix/svg-html-integration-points

📝 Commits (3)

552024b fix(browser): respect SVG desc and title HTML integration points
b98f377 fix(browser): preserve moved SVG XLink destinations
14db1cb fix(browser): bound concurrent SVG identity reads

📊 Changes

6 files changed (+821 additions, -123 deletions)

View changed files

📝 src/browser/CdpCaptureDocuments.baseUrl.test.ts (+247 -37)
📝 src/browser/CdpCaptureDocuments.ts (+128 -32)
📝 src/browser/CdpCaptureDomSvg.ts (+27 -47)
📝 src/browser/CdpPageCapture.ts (+49 -5)
➕ tests/boundary/browser/cdpDomSvgIdentity.test.ts (+315 -0)
📝 tests/boundary/browser/cdpSvgXlinkHref.test.ts (+55 -2)

📄 Description

Summary

Correct SVG XLink destination extraction in HTML pages: exclude HTML attributes that are not links, while retaining genuine SVG links moved or created inside desc, title, or foreignObject. Bound concurrent identity reads so SVG-heavy captures do not wait for every candidate sequentially.

Fixes #2479. Addresses the moved-SVG finding and serial-query performance finding.

Problem and expected behavior

What Problem This Solves

In text/html, <svg><desc><a xlink:href="/false">…</a></desc></svg> creates an HTML anchor without a link destination; title behaves the same way. The original ancestry heuristic reports these as SVG links. Conversely, moving an existing SVG anchor into either container preserves its SVG namespace, so stopping at those ancestors drops valid links. createElementNS can also create genuine SVG anchors there.

User Impact

CLI and MCP page inspection exclude false HTML XLink destinations and preserve real SVG destinations after DOM manipulation. Ordinary href values still take precedence, including empty values. SVG identity reads overlap within a bounded window rather than imposing one sequential round trip per candidate in dense SVG pages.

Change and scope

Why This Change Was Made

Parent position and tag casing do not establish element identity; SVG and MathML can both have a lowercase a. The CDP adapter describes eligible elements using the snapshot's backendNodeId, validates the returned identity, and reads Chrome's isSVG flag. The obsolete HTML ancestor walk and cache are removed. XML/XHTML namespace resolution remains independent.

The snapshot projector normalizes at most eight metadata nodes per batch and yields only eligible identity queries. capturePageDom sends that batch concurrently, then applies results in snapshot order even if replies arrive out of order. Both pending metadata and outstanding identity reads are bounded by eight, independently of page size; actual concurrency depends on candidate density. This keeps one snapshot traversal and a small number of pending replies/cancellation listeners.

Queries are limited to allowed-origin HTML elements with xlink:href and no ordinary href. Missing IDs/nodes and mismatched replies retain observed attributes and mark metadata incomplete with an explicit unknown-destination limitation. An unsupported method stops further batches. Fatal failures abort and settle sibling reads before session cleanup; caller cancellation and disconnection still propagate.

This is a routine passive-inspection correction; a new target-execution scope decision is not applicable.

Contract and boundary impact

Semantic owner: CDP snapshot projection and node identity decoding in src/browser/.
CLI/MCP contracts: no schema/tool-catalog changes; corrected metadata.dom_urls and existing completeness/limitations fields.
Compatibility: HTML identity reads use CDP DOM.describeNode/Node.isSVG; unavailable descriptions remain explicit. Existing XML/XHTML behavior is retained.
Evidence/provenance: snapshot backend IDs bind identity reads to captured elements; attribute names, output order, URL policy, and unknowns are preserved.
Authority and cleanup: read-only commands on the selected CDP session, with existing target selection, origin policy, and detach ownership. No target script execution, instrumentation, mutation, or new network destination.
Generated metadata, package, installation, or migration impact: none.

Expected effects:

Read captured data.
Attach through the existing selected browser session.
Contact the existing CDP endpoint; verification used loopback only.

Evidence and regression coverage

Evidence

Executed on Linux x64, Node 24.18.0, npm 11.16.0, Chrome 138.0.7204.157. An isolated page contains two false HTML XLink attributes and eleven genuine links, including moved/created/nested-created SVG anchors under both desc and title. Chrome's namespaceURI, href, and :any-link provide an independent oracle.

Public workflow	Ancestry-cutoff head `552024b`	Final head `14db1cb`
Compiled `inspect-web-page` CLI	5 of 11 valid destinations; six missing	All 11; no false HTML destinations
Real stdio `inspect_web_page` MCP	Same six missing destinations	Same 11 valid destinations

Restored destinations include /true-moved-desc, /true-created-desc, /true-nested-desc, and their title counterparts. /false-desc and /false-title remain absent. The expanded real-Chrome regression failed before the identity correction (1 failed, 2 passed).

The concurrency regressions also failed on serial head b98f377 (2 failed, 7 passed): a withheld reply prevented any second request from being sent. They now prove a maximum of eight in-flight reads across multiple batches, ordered output under reversed replies, and cancellation of hung sibling requests on fatal failure. Further coverage includes missing-ID alignment, partial metadata, unsupported methods, origin restrictions, cancellation, disconnection, cleanup, MathML negative controls, XHTML, and standalone SVG.

Observations and derived destinations remain distinguishable.
Identity and source provenance are preserved.
Incomplete and unavailable outcomes remain explicit.

Validation performed

npm run test:focused -- src/browser/CdpCaptureDocuments.baseUrl.test.ts tests/boundary/browser/cdpDomSvgIdentity.test.ts tests/boundary/browser/cdpSvgXlinkHref.test.ts — 66 passed, including three real-Chrome tests.
After adding the missing-ID alignment regression: npm run test:focused -- src/browser/CdpCaptureDocuments.baseUrl.test.ts tests/boundary/browser/cdpDomSvgIdentity.test.ts tests/boundary/browser/cdpBrowserProvider.document_script.test.ts tests/boundary/browser/cdpBrowserProvider.lifecycle_failures.test.ts — 100 passed. There are 103 distinct passing tests across these runs; production code is identical between them.
npm run check — passed typecheck, lint, formatting, knip, metadata, module boundaries, and architecture guards; lint: 0 errors, 1000 warnings.
Compiled CLI and real stdio MCP at final commit 14db1cb8618ba3ec4e52cf3505c12dfd0dab67b8 — exact destination sets verified for both the eleven-link functional fixture and the 1,024-link performance fixture.
git diff --check — passed.

Performance measurements

Compared serial head b98f377 with final head 14db1cb. Both use the same loaded, isolated Chrome page containing 1,024 SVG anchors (<svg><a xlink:href="/icon/0"/>…</svg>), zero observation window, and fresh CLI/MCP processes. A loopback WebSocket relay forwards real CDP traffic unchanged; the second condition adds 5 ms to each DOM.describeNode reply independently. This is controlled latency, not a remote-browser deployment test. First and repeat captures are reported separately; OS caches are uncontrolled, so these are not strict cold-start measurements.

Every run produced exactly the same ordered 1,024 destinations and issued exactly 1,024 identity reads. Measured maximum in-flight reads changed from 1 to 8. The identity phase is measured from the first request through the last delivered reply.

Adapter / added delay	Capture	Total before → after (s)	Identity phase before → after (s)
CLI / 0 ms	first	3.657 → 3.131	1.190 → 0.492
MCP / 0 ms	first	4.568 → 3.139	1.282 → 0.279
CLI / 0 ms	repeat	3.374 → 2.358	1.028 → 0.427
MCP / 0 ms	repeat	3.903 → 3.172	1.025 → 0.257
CLI / 5 ms	first	9.077 → 2.890	7.147 → 1.012
MCP / 5 ms	first	10.531 → 4.602	7.583 → 1.179
CLI / 5 ms	repeat	9.720 → 3.509	7.625 → 1.480
MCP / 5 ms	repeat	10.635 → 4.270	7.833 → 1.104

Client-family resources (ranges across the four captures per adapter):

Adapter	CPU before → after (s)	Sampled peak RSS before → after (MiB)
CLI	3.34–3.61 → 2.90–3.13	316.50–321.50 → 315.10–317.40
MCP	4.19–5.08 → 3.92–4.46	450.80–473.20 → 467.00–471.20

RSS is sampled at 10 ms across the client process family; browser and relay CPU/memory are excluded. Output sizes were unchanged (CLI 379,844 bytes; MCP 379,843 bytes). Token counts, broader website workloads, and strict cold-start behavior were not measured. The timing benefit depends on latency and candidate density; the number of CDP reads is unchanged.

Compatibility, safety, and release

No breaking schema or package changes. Production only reads browser metadata. Fixture scripts create/move nodes on the owned page. Coverage is Linux/Chrome; other OS/browser versions, the published npm package, and the full browser lane were not run. Dynamic XML namespace mutations and attribute-namespace reconstruction remain outside this HTML element-identity correction. Optional Frida installation was skipped after an unrelated download stalled; this browser path does not use it (npm ci --ignore-scripts, then npm run prepare).

Before merge

Concrete problem and focused Conventional Commit title.
Scope and authority resolved for this routine fix; linked issue provided.
Changed behavior and meaningful failure paths tested.
Public CLI/MCP behavior demonstrated; no owning documentation or generated contract changes needed.
Final diff checked for unrelated changes, secrets, and unsupported claims.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/2480 **Author:** [@zhangguiping-xydt](https://github.com/zhangguiping-xydt) **Created:** 10/13/2026 **Status:** ✅ Merged **Merged:** 10/13/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/svg-html-integration-points` --- ### 📝 Commits (3) - [`552024b`](https://github.com/morluto/rea/commit/552024b53eb7f12995d9d93f558471e57c9371a4) fix(browser): respect SVG desc and title HTML integration points - [`b98f377`](https://github.com/morluto/rea/commit/b98f377d9e18fb9261d885bbc88f5b6435f946ba) fix(browser): preserve moved SVG XLink destinations - [`14db1cb`](https://github.com/morluto/rea/commit/14db1cb8618ba3ec4e52cf3505c12dfd0dab67b8) fix(browser): bound concurrent SVG identity reads ### 📊 Changes **6 files changed** (+821 additions, -123 deletions) <details> <summary>View changed files</summary> 📝 `src/browser/CdpCaptureDocuments.baseUrl.test.ts` (+247 -37) 📝 `src/browser/CdpCaptureDocuments.ts` (+128 -32) 📝 `src/browser/CdpCaptureDomSvg.ts` (+27 -47) 📝 `src/browser/CdpPageCapture.ts` (+49 -5) ➕ `tests/boundary/browser/cdpDomSvgIdentity.test.ts` (+315 -0) 📝 `tests/boundary/browser/cdpSvgXlinkHref.test.ts` (+55 -2) </details> ### 📄 Description ## Summary Correct SVG XLink destination extraction in HTML pages: exclude HTML attributes that are not links, while retaining genuine SVG links moved or created inside `desc`, `title`, or `foreignObject`. Bound concurrent identity reads so SVG-heavy captures do not wait for every candidate sequentially. Fixes #2479. Addresses the [moved-SVG finding](https://github.com/morluto/rea/pull/2480#discussion_r4255476799) and [serial-query performance finding](https://github.com/morluto/rea/pull/2480#discussion_r4255869195). ## Problem and expected behavior ### What Problem This Solves In `text/html`, `<svg><desc><a xlink:href="/false">…</a></desc></svg>` creates an HTML anchor without a link destination; `title` behaves the same way. The original ancestry heuristic reports these as SVG links. Conversely, moving an existing SVG anchor into either container preserves its SVG namespace, so stopping at those ancestors drops valid links. `createElementNS` can also create genuine SVG anchors there. ### User Impact CLI and MCP page inspection exclude false HTML XLink destinations and preserve real SVG destinations after DOM manipulation. Ordinary `href` values still take precedence, including empty values. SVG identity reads overlap within a bounded window rather than imposing one sequential round trip per candidate in dense SVG pages. ## Change and scope ### Why This Change Was Made Parent position and tag casing do not establish element identity; SVG and MathML can both have a lowercase `a`. The CDP adapter describes eligible elements using the snapshot's `backendNodeId`, validates the returned identity, and reads Chrome's `isSVG` flag. The obsolete HTML ancestor walk and cache are removed. XML/XHTML namespace resolution remains independent. The snapshot projector normalizes at most eight metadata nodes per batch and yields only eligible identity queries. `capturePageDom` sends that batch concurrently, then applies results in snapshot order even if replies arrive out of order. Both pending metadata and outstanding identity reads are bounded by eight, independently of page size; actual concurrency depends on candidate density. This keeps one snapshot traversal and a small number of pending replies/cancellation listeners. Queries are limited to allowed-origin HTML elements with `xlink:href` and no ordinary `href`. Missing IDs/nodes and mismatched replies retain observed attributes and mark metadata incomplete with an explicit unknown-destination limitation. An unsupported method stops further batches. Fatal failures abort and settle sibling reads before session cleanup; caller cancellation and disconnection still propagate. This is a routine passive-inspection correction; a new target-execution scope decision is not applicable. ## Contract and boundary impact - Semantic owner: CDP snapshot projection and node identity decoding in `src/browser/`. - CLI/MCP contracts: no schema/tool-catalog changes; corrected `metadata.dom_urls` and existing completeness/limitations fields. - Compatibility: HTML identity reads use CDP `DOM.describeNode`/`Node.isSVG`; unavailable descriptions remain explicit. Existing XML/XHTML behavior is retained. - Evidence/provenance: snapshot backend IDs bind identity reads to captured elements; attribute names, output order, URL policy, and unknowns are preserved. - Authority and cleanup: read-only commands on the selected CDP session, with existing target selection, origin policy, and detach ownership. No target script execution, instrumentation, mutation, or new network destination. - Generated metadata, package, installation, or migration impact: none. Expected effects: - [x] Read captured data. - [x] Attach through the existing selected browser session. - [x] Contact the existing CDP endpoint; verification used loopback only. ## Evidence and regression coverage ### Evidence Executed on Linux x64, Node 24.18.0, npm 11.16.0, Chrome 138.0.7204.157. An isolated page contains two false HTML XLink attributes and eleven genuine links, including moved/created/nested-created SVG anchors under both `desc` and `title`. Chrome's `namespaceURI`, `href`, and `:any-link` provide an independent oracle. | Public workflow | Ancestry-cutoff head `552024b` | Final head `14db1cb` | | --- | --- | --- | | Compiled `inspect-web-page` CLI | 5 of 11 valid destinations; six missing | All 11; no false HTML destinations | | Real stdio `inspect_web_page` MCP | Same six missing destinations | Same 11 valid destinations | Restored destinations include `/true-moved-desc`, `/true-created-desc`, `/true-nested-desc`, and their `title` counterparts. `/false-desc` and `/false-title` remain absent. The expanded real-Chrome regression failed before the identity correction (1 failed, 2 passed). The concurrency regressions also failed on serial head `b98f377` (2 failed, 7 passed): a withheld reply prevented any second request from being sent. They now prove a maximum of eight in-flight reads across multiple batches, ordered output under reversed replies, and cancellation of hung sibling requests on fatal failure. Further coverage includes missing-ID alignment, partial metadata, unsupported methods, origin restrictions, cancellation, disconnection, cleanup, MathML negative controls, XHTML, and standalone SVG. - [x] Observations and derived destinations remain distinguishable. - [x] Identity and source provenance are preserved. - [x] Incomplete and unavailable outcomes remain explicit. ## Validation performed - `npm run test:focused -- src/browser/CdpCaptureDocuments.baseUrl.test.ts tests/boundary/browser/cdpDomSvgIdentity.test.ts tests/boundary/browser/cdpSvgXlinkHref.test.ts` — 66 passed, including three real-Chrome tests. - After adding the missing-ID alignment regression: `npm run test:focused -- src/browser/CdpCaptureDocuments.baseUrl.test.ts tests/boundary/browser/cdpDomSvgIdentity.test.ts tests/boundary/browser/cdpBrowserProvider.document_script.test.ts tests/boundary/browser/cdpBrowserProvider.lifecycle_failures.test.ts` — 100 passed. There are 103 distinct passing tests across these runs; production code is identical between them. - `npm run check` — passed typecheck, lint, formatting, knip, metadata, module boundaries, and architecture guards; lint: 0 errors, 1000 warnings. - Compiled CLI and real stdio MCP at final commit `14db1cb8618ba3ec4e52cf3505c12dfd0dab67b8` — exact destination sets verified for both the eleven-link functional fixture and the 1,024-link performance fixture. - `git diff --check` — passed. ### Performance measurements Compared serial head `b98f377` with final head `14db1cb`. Both use the same loaded, isolated Chrome page containing 1,024 SVG anchors (`<svg><a xlink:href="/icon/0"/>…</svg>`), zero observation window, and fresh CLI/MCP processes. A loopback WebSocket relay forwards real CDP traffic unchanged; the second condition adds 5 ms to each `DOM.describeNode` reply independently. This is controlled latency, not a remote-browser deployment test. First and repeat captures are reported separately; OS caches are uncontrolled, so these are not strict cold-start measurements. Every run produced exactly the same ordered 1,024 destinations and issued exactly 1,024 identity reads. Measured maximum in-flight reads changed from 1 to 8. The identity phase is measured from the first request through the last delivered reply. | Adapter / added delay | Capture | Total before → after (s) | Identity phase before → after (s) | | --- | --- | --- | --- | | CLI / 0 ms | first | 3.657 → 3.131 | 1.190 → 0.492 | | MCP / 0 ms | first | 4.568 → 3.139 | 1.282 → 0.279 | | CLI / 0 ms | repeat | 3.374 → 2.358 | 1.028 → 0.427 | | MCP / 0 ms | repeat | 3.903 → 3.172 | 1.025 → 0.257 | | CLI / 5 ms | first | 9.077 → 2.890 | 7.147 → 1.012 | | MCP / 5 ms | first | 10.531 → 4.602 | 7.583 → 1.179 | | CLI / 5 ms | repeat | 9.720 → 3.509 | 7.625 → 1.480 | | MCP / 5 ms | repeat | 10.635 → 4.270 | 7.833 → 1.104 | Client-family resources (ranges across the four captures per adapter): | Adapter | CPU before → after (s) | Sampled peak RSS before → after (MiB) | | --- | --- | --- | | CLI | 3.34–3.61 → 2.90–3.13 | 316.50–321.50 → 315.10–317.40 | | MCP | 4.19–5.08 → 3.92–4.46 | 450.80–473.20 → 467.00–471.20 | RSS is sampled at 10 ms across the client process family; browser and relay CPU/memory are excluded. Output sizes were unchanged (CLI 379,844 bytes; MCP 379,843 bytes). Token counts, broader website workloads, and strict cold-start behavior were not measured. The timing benefit depends on latency and candidate density; the number of CDP reads is unchanged. ## Compatibility, safety, and release No breaking schema or package changes. Production only reads browser metadata. Fixture scripts create/move nodes on the owned page. Coverage is Linux/Chrome; other OS/browser versions, the published npm package, and the full browser lane were not run. Dynamic XML namespace mutations and attribute-namespace reconstruction remain outside this HTML element-identity correction. Optional Frida installation was skipped after an unrelated download stalled; this browser path does not use it (`npm ci --ignore-scripts`, then `npm run prepare`). ## Before merge - [x] Concrete problem and focused Conventional Commit title. - [x] Scope and authority resolved for this routine fix; linked issue provided. - [x] Changed behavior and meaningful failure paths tested. - [x] Public CLI/MCP behavior demonstrated; no owning documentation or generated contract changes needed. - [x] Final diff checked for unrelated changes, secrets, and unsupported claims. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:05:14 +00:00