Skip to content

fix(studio): drop the resurrected WebMCP polyfill assertion - #3556

Open
miguel-heygen wants to merge 1 commit into
mainfrom
fix/webmcp-flaky-polyfill-assertion
Open

fix(studio): drop the resurrected WebMCP polyfill assertion#3556
miguel-heygen wants to merge 1 commit into
mainfrom
fix/webmcp-flaky-polyfill-assertion

Conversation

@miguel-heygen

Copy link
Copy Markdown
Collaborator

main is going red at random for open PRs on one line in
packages/studio/src/webmcp/useStudioAgentTools.test.tsx.

da6514d45 removed expect(document).not.toHaveProperty("modelContext") after
the @mcp-b/global polyfill fallback landed. #3516 merged from a branch cut
before that commit and brought the line back, together with the old test title.
This re-applies da6514d45's change to that one case; nothing else moves.

Why it is flaky rather than just wrong

useStudioAgentTools awaits a dynamic import("@mcp-b/global") inside its
mount effect. Whether document.modelContext exists when the assertion runs
depends only on whether that import has resolved yet, and await act()
usually returns first. main won that race and went green; another PR's run
lost it and went red.

Made it deterministic on the current tip by inserting a 100ms wait before the
assertion:

 ❯ src/webmcp/useStudioAgentTools.test.tsx:191:26
     191|     expect(document).not.toHaveProperty("modelContext");
 Test Files  1 failed (1)
       Tests  1 failed | 8 passed (9)

The failure dump shows all five tools registered through the polyfill, so the
real behaviour is the opposite of what the line asserted. Installing
document.modelContext where the browser has none is exactly the polyfill's
job. What the case is actually for is that mounting does not throw, so a
browser without native WebMCP still boots Studio.

Checked for other resurrections

3337cc899 is a squash with a single parent, so its diff is its full net
change to main. Reviewed every hunk in the four files that existed before it
(StudioAgentTools.tsx, useStudioAgentTools.ts, useStudioAgentTools.test.tsx,
tools/selectionTools.test.ts). Only this one hunk reverses earlier work; the
rest are additive, and the selectionTools.test.ts shrink is a helper
extraction into the new webmcpTestUtils.ts. The #3514 polyfill code in
useStudioAgentTools.ts is untouched.

Verification

  • Target file run 10x: 10/10 green, 9 tests each.
  • Same 100ms probe re-applied on top of this change: still 9 passed. The test
    no longer depends on who wins the race.
  • Full studio suite: 412 passed | 1 skipped (413) files,
    4591 passed | 18 todo (4609) tests.
  • oxfmt --check and oxlint clean on the changed file; pre-commit lint,
    fallow and typecheck gates all passed.

#3516 merged from a branch cut before da6514d and brought back
`expect(document).not.toHaveProperty("modelContext")`, along with the old
test title. This re-applies that fix.

The assertion is racy, not merely wrong. The hook awaits a dynamic import
of the `@mcp-b/global` polyfill inside its mount effect, so whether
`document.modelContext` exists when the assertion runs depends only on
whether that import has resolved yet. `await act()` usually returns first,
which is why main went green while another PR's run went red. Inserting a
100ms wait before the assertion on the current tip makes it fail every
time, and the failure dump shows all five tools registered through the
polyfill: the real behaviour is the opposite of what the line asserted.

Installing `document.modelContext` where the browser has none is the
polyfill's job. What the case is really for is that mounting does not
throw, so a browser without native WebMCP still boots Studio.
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.

1 participant