Skip to content

fix(mcp): print the path shared by browser_find matches once - #42127

Closed
Ishaan (guptaishaan) wants to merge 1 commit into
microsoft:mainfrom
guptaishaan:fix-42077
Closed

fix(mcp): print the path shared by browser_find matches once#42127
Ishaan (guptaishaan) wants to merge 1 commit into
microsoft:mainfrom
guptaishaan:fix-42077

Conversation

@guptaishaan

Copy link
Copy Markdown

Fixes #42077

browser_find rendered every context window as an independent snippet and recomputed
ancestorIndices() for each one, so the path from the root was re-emitted once per match. On a page
where several matches sit under a common deep ancestor, that made find produce more output than
the full snapshot it is supposed to replace: on a 6-card job board fixture, browser_snapshot was
59 lines and browser_find was 84.

This collects the ancestor paths and context windows of all matches into one sorted set of line
indices and renders them as a single tree. A shared path is printed once, and the existing ellipsis
rule now marks the gap between windows. Same fixture after the change: 54 lines. The ----
separator between snippets is gone, since there is only one tree now.

Verified on Ubuntu 20.04 x64, node 22.14, chromium r1237 and firefox 1539. New test in
tests/mcp/find.spec.ts fails before the change and passes after. All existing find and cli-find
tests pass on chromium and firefox. I could not run webkit, its binaries do not install on this
host, so if webkit numbers refs differently the new test may need the same adjustment the existing
ref-asserting tests would.

Two things in the issue are deliberately not addressed here. The full ancestor chain for a single
match is unchanged, that is the behavior added in #41654 and it is a product decision rather than a
bug. There is still no --max-results on find. The issue's third point, no depth limit on
snapshot, is already fixed on main - browser_snapshot honors depth both page-wide and relative
to a target root.

Thanks to David Condrey (@dcondrey) for the report.

browser_find rendered every context window as its own snippet and
recomputed the ancestor path for each one, so the path from the root was
re-emitted once per match. With several matches under a common deep
ancestor this made find produce more output than the full snapshot it is
meant to replace: on a 6-card job board fixture browser_snapshot was 59
lines and browser_find was 84.

Collect the ancestor paths and context windows of all matches into one
sorted set of line indices and render them as a single tree. The shared
path is printed once and the existing ellipsis rule marks the gap
between windows. Same fixture is now 54 lines. The ---- separator
between snippets is gone, there is only one tree.

Fixes: microsoft#42077

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Ready to approve

The change is localized, matches the stated intent, and is covered by a new regression test plus existing find-related tests.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR reduces browser_find output verbosity in the MCP backend by rendering all match context windows as a single combined tree, so shared ancestor paths are printed once (instead of once per snippet), aligning find output closer to being a cheaper alternative to full snapshots.

Changes:

  • Merge all match windows’ line indices (including required ancestor lines) into one sorted set and render a single tree, removing per-snippet repetition and the ---- separator.
  • Preserve the existing ellipsis rule to mark gaps between non-path context windows within the combined output.
  • Add a regression test ensuring a shared path for multiple matches is printed once and that the gap between windows is ellipsized.
File summaries
File Description
packages/playwright-core/src/tools/backend/find.ts Combines multiple context windows into one rendered tree so shared ancestor paths aren’t repeated.
tests/mcp/find.spec.ts Adds coverage for multiple matches sharing a deep ancestor path and verifies the combined output behavior.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Test results for "MCP"

1 failed
❌ [chromium] › mcp/cli-killall.spec.ts:28 › kill-all kills only filtered pid @mcp-macos-latest-chromium

8062 passed, 1284 skipped


Merge workflow run.

@guptaishaan

Copy link
Copy Markdown
Author

The failing macos-latest - chromium check is not from this change.

The failure is tests/mcp/cli-killall.spec.ts:28 kill-all kills only filtered pid, timing out after 30s waiting for cli('open', ...) to report a daemon pid. That test spawns the CLI daemon and never calls browser_find, so it does not touch find.ts, the only source file in this PR. Everything in tests/mcp/find.spec.ts passed in that job, including the new regression test (619 passed, 1 failed).

The same job flakes on main with a different cli-*.spec.ts test each run, always one CLI daemon test timing out at 30s while the rest of the suite is green:

The last one predates the commit on this PR, so this is pre-existing on the base branch.

I have not changed anything, and I have left the flaky test alone since de-flaking it is unrelated to this PR. A re-run should turn the check green.

@dcondrey

Copy link
Copy Markdown

Thanks for picking this up. I read through it and reimplemented the before/after rendering locally to see what the output looks like on the page shape from the issue. The dedup itself is a real win — on a 4-card job-board fixture I get 55 lines down to 37. Two things I ran into.

The ellipsis rule doesn't survive losing ----

The rule was written for per-snippet rendering, where ---- marked the discontinuity between windows. This PR removes the separator but leaves the condition unchanged:

if (i > 0 && index > indices[i - 1] + 1 && !path.has(index) && !path.has(indices[i - 1]))

path contains matched lines and their ancestors. When matches sit in different subtrees, the first new line at a window boundary is an ancestor of the next match, so path.has(index) is true and no ellipsis is emitted — and there's no ---- anymore either.

Card shape generic > link > generic > heading, which is what the site in the issue actually produces, with one non-matching card between two matching ones:

- generic [ref=e2]:
  - generic [ref=e3]:
    - generic [ref=e4]:
      - generic [ref=e10]:
        - link [ref=e11] [cursor=pointer]:
          - /url: /job
          - generic [ref=e12]:
            - heading "Research Scientist 1" [level=3] [ref=e13]
            - paragraph [ref=e14]: Remote
            - paragraph [ref=e15]: Full time
            - paragraph [ref=e16]: Posted today
      - generic [ref=e24]:
        - link [ref=e25] [cursor=pointer]:
          - /url: /job
          - generic [ref=e26]:
            - heading "Research Scientist 2" [level=3] [ref=e27]
            - paragraph [ref=e28]: Remote
            - paragraph [ref=e29]: Full time
            - paragraph [ref=e30]: Posted today

The card at e17e23 is gone, with no ... and no separator. Zero ellipses in the whole output. It reads as a complete, contiguous list of exactly two cards under e4, and a consumer has no signal that a sibling was dropped. Before this change ---- at least marked the break, so for this arrangement the merged tree is less faithful than what it replaces.

Suppressing only on a genuine path jump — previous rendered line is an ancestor of this one — seems to cover it:

const prev = indices[i - 1];
const pathJump = indents[prev] < indents[index] && (path.has(index) || path.has(prev));
if (i > 0 && index > prev + 1 && !pathJump)
  out.push(' '.repeat(indents[index]) + '...');

I ran that against shows the path from the root to the match, marks gaps within off-path context with an ellipsis, and the new prints the path shared by several matches once — all three render identically to the current behavior, and the job board above gets its ... back.

Relatedly, repeatedPage puts both matches under one identical ancestor chain, which is the case that already works. The divergent-branch case above is the one that regresses and isn't covered. And two stringContaining assertions can't really show the path is printed once — nothing fails if it's printed twice.

Minor: the tool description still says matches are returned "each shown under its path from the root of the tree", which described the old per-snippet output.

On scope

Not a criticism of the patch, just calibrating against what I filed. The example in the issue was a single match, and single-match output is unchanged here — the eight levels of generic [ref=eN] still come back as they did. --max-results and ancestor-chain-length control, the two things Pavel Feldman (@pavelfeldman) said were addable, aren't in this PR.

Worth noting that even after the merge, find isn't much cheaper than the thing it replaces: your fixture is 54 lines against a 59-line snapshot, and mine is 37 against 36. Once matches are plural, the ancestor chain isn't what dominates — the number of matches is. So I think the bounding options are the load-bearing part of the issue, and I'd rather see them than the dedup if it's one or the other.

@pavelfeldman

Copy link
Copy Markdown
Member

Looks like 2 AIs talking to each other! I like the single tree, but I don't like misrepresenting the gaps. Looking for a clean solution w/o wall of agentic slop to parse.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CLI]: find returns the full ancestor chain, and neither find nor snapshot can bound their output

4 participants