Repository navigation
fix(app): read pairing links without URL.parse - #59240
SU-AN-coder wants to merge 2 commits into
Conversation
URL.parse landed in Safari 18, so on iOS 17 the connect screen threw a TypeError while reading the link and the error handler blamed the server address. new URL in a try/catch is the same test and works everywhere.
github-actions
Bot
added
the
needs:compliance
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
This fixes the connect screen on browsers without URL.parse. I built the web app and ran the server from source on the base and on this branch. Then I ran the #59186 reproduction in headless Chromium with URL.parse deleted. On the base, the form shows "Could not connect" and no request carries Authorization. With this change it connects, and authorized requests go out (screenshots below). The new test fails on the base and passes here. tsc -b in packages/app is clean.
The public API is unchanged.
One point worth addressing: the web bundle still calls URL.parse in packages/gui-extensions/src/pairing/remote.ts (the pairing settings page) and packages/gui-extensions/src/microsoft-office/links.ts. Both will still throw on Safari 17. A guarded URL.parse polyfill in packages/app/src/runtime/polyfills.ts, next to the ones #59007 added, would fix all of them and any future uses. Pairing could then keep calling URL.parse directly. See the inline comment.
Related: #52345
|
|
||
| const CODE = /^[A-Za-z0-9_-]+$/ | ||
|
|
||
| // URL.parse is Safari 18+; on iOS 17 it is undefined, so reading a pairing link threw a TypeError before any |
There was a problem hiding this comment.
This fixes the two calls in this file, but the web app also bundles packages/gui-extensions. There, pairing/remote.ts (remoteAddress) and microsoft-office/links.ts (externalUrl) still call URL.parse, so on Safari 17 they'll throw the same TypeError. src/runtime/polyfills.ts already polyfills other missing APIs for older Safari (see #59007). A guarded if (!URL.parse) URL.parse = ... there, built on new URL in a try/catch, would cover every call site with one fix. This file could then keep using URL.parse.
URL.parse landed in Safari 18, so the connect screen threw while reading a pairing link and the web bundle's other callers (the pairing page, the office links) would throw too. A guarded polyfill next to the others in runtime/polyfills.ts stands in with new URL in a try/catch; the pair link reader keeps calling URL.parse.
github-actions
Bot
removed
the
needs:compliance
|
Done in 7268cbf: the guarded polyfill is in The test changed with it: it removes |
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
There was a problem hiding this comment.
The earlier point is addressed. URL.parse is now polyfilled in runtime/polyfills.ts, which covers the pairing reader and both gui-extensions callers. I rebuilt the web app on the base and on this branch and reran the #59186 reproduction in headless Chromium with URL.parse deleted. On the base it still fails with "Could not connect" and sends no authorized requests. With this change the polyfill is installed before app code runs, and the connect screen goes through. Tests in src/servers/connect and src/runtime/polyfills.test.ts pass, and tsc -b is clean.
The public API is unchanged. Nothing new to raise.
Issue for this PR
Closes #59186
Type of change
What does this PR do?
URL.parselanded in Safari 18. On iOS 17 it isundefined, so reading a pairing link threw aTypeErrorbefore any request went out, and the connect screen blamed the address.Following the review, this adds a guarded polyfill in
packages/app/src/runtime/polyfills.ts, next toMap.groupBy/Promise.withResolvers/Promise.try: whenURL.parseis missing it stands in withnew URLin a try/catch, returningnullfor anything that does not parse. The pair link reader keeps callingURL.parsedirectly, so the two other callers the review named (gui-extensions/src/pairing/remote.ts,gui-extensions/src/microsoft-office/links.ts) are covered by the same install instead of each needing its own guard.polyfills.tsgainedexport {}so the test can import it after removing the built-in; the file is imported for its side effects either way.How did you verify your code works?
The test removes
URL.parse, imports the polyfill, then reads a pair link and a legacy#tokenlink. Without the polyfill the same read throws, which is what the test showed ondev. It does not simulate iOS 17 beyond that, and I have no iOS 17 device here.bun test --conditions=solid --isolate --preload ../ui/test/solid-jsx.ts --preload ./happydom.ts src/servers/connect— 7 pass, 0 failbun run test:unitinpackages/app— 776 pass, 0 failbun run typecheck:tsc— clean; rootbun run lint:changed— 0 problemsScreenshots / recordings
Not a visual change: the screen and its text stay the same, the exception before the request is what goes away.
Checklist
Added by the opencode bot (AI-generated).
Related: #52345