Repository navigation
Short origin follow-ups from the #72 review - #80
Conversation
The REST link responses gained short_origin in #72, but the MCP link tools did not, so an assistant on a deployment that pins SHORT_ORIGIN built URLs from whatever host it knew. That is the wrong-domain case the setting exists to prevent. The annotation now lives in withShortOrigin() (src/short-origin.ts), and both surfaces use it: fromLinkResult() for REST and a withOrigin() helper on the MCP agent for create, get, update, disable, enable, list, search, list-by-owner and bundle links. fromLinkResult() takes the request context instead of a resolved origin. Nine handlers used to repeat resolveShortOrigin(c.env, c.req.url), and a new link route that forgot it answered without a field the schema marks required. MCP leaves the field off when nothing is pinned. An MCP request often arrives on the mcp.* host, which is not where short links live, so its origin is no answer. get_link_qr lists base_url as required exactly when the deployment pins nothing. #72 made it optional everywhere, so a client that trusted the schema on a default deployment left it out and the call failed. Test hardening: three MCP description tests built the agent with no env, a state the runtime never produces. init() now reads the environment to shape the get_link_qr schema, so the fixture gets the test env. Their assertions are unchanged.
Seven admin routes passed shortOrigin, theme, t and lang to <Layout> one prop at a time, so #72 had to touch every route to add one shell prop, and the next one would too. getPageData() now returns them as one layout object that each route spreads. A new shell prop is added in one place.
#72 made copyText() resolve false instead of throwing when the clipboard is missing or refuses the write. The callers ignored the answer and always showed the success toast. On plain http, the deployment #72 adds support for, the API key modal said "API key copied". The user clicked Done, and the key, shown only once, was gone. quickShorten() and createDuplicate() announced "created & copied" for a copy that never happened. copyUrl() now returns the outcome. On success it confirms the copy. On a failure it raises an error toast that names the URL to copy by hand. copyRawKey() selects the key and says to copy it by hand. The two create flows share copyThenOpen(), which waits for the write before leaving the page, since the browser can abort a pending clipboard write on unload. It confirms "created & copied" only when the copy landed. The existing copyUrl() unit tests now await the promise copyUrl() returns before reading the toasts, because the toast waits for the write to settle. Their assertions are unchanged. The e2e comment that described a synchronous toast is updated. New e2e steps drive both copy buttons with navigator.clipboard removed.
#72 pinned SHORT_ORIGIN on the e2e server for the whole suite, so no browser test covered the default most deployments run with. A regression where data-short-origin renders empty, or where the client stops falling back to location.origin, passed yarn e2e. A second wrangler dev now boots with the variable unset, on the next port and its own state directory. default-origin.spec.ts signs in there, creates a link, and checks that the copy button, the clipboard and the QR modal all name that host. Both servers now boot fresh on every run. A reused server carries the environment it was started with: a server left running from before #72 failed the origin specs for a reason no test named. A port in use now fails the run up front instead.
Link.from_dict() read data["short_origin"] strictly, so after a pip upgrade every call that returns a link raised KeyError against a Worker not yet redeployed: links.get/list/create/update/disable/enable, slugs.lookup and bundles.links. The field now falls back to the origin of the client's base_url, which is the origin such a deployment hands out. Each resource passes that origin to from_dict(). Link also gained short_origin as a required field ahead of delta_pct. That broke every Link(...) built by hand, including a positional delta_pct. The field moves last with a default, so the constructor accepts what it accepted before #72. The README example builds the URL from the primary slug instead of slugs[0]. After a custom slug becomes primary, slugs[0] can name a different URL from the one the copy button and the QR code hand out. No spec change, so the hash stays.
The transport hands back the camel-cased JSON as is, so against a Worker not yet redeployed every link came back with shortOrigin undefined, although the type promises a string. Each method that returns a link now fills in the origin of the client's baseUrl when the response has no short_origin. Those methods are links.get, list, create, update, disable and enable, slugs.lookup and bundles.links. That is the origin such a deployment hands out. The Python and Dart SDKs make the same fallback. The Python and Dart changes also make the Link constructor accept a missing short origin again. TypeScript has no constructor: Link is an interface, and the field stays required there because every Link the client returns now carries it. The README example builds the URL from the primary slug instead of slugs[0], which can name a different URL from the copy button and the QR code once a custom slug is primary. No spec change, so the hash stays.
Link.fromJson() cast json['short_origin'] as String. After a pub upgrade, every call that returns a link threw a TypeError against a Worker not yet redeployed, because null is not a String. The field now falls back to the origin of the client's base URL, the origin such a deployment hands out. Each resource passes ShrtnrBaseClient.origin to fromJson(). Uri.origin drops any credentials in the base URL. Link's constructor also gained shortOrigin as a required named parameter, which broke every Link(...) built by hand. The parameter is optional now and defaults to empty, so the constructor accepts what it accepted before #72. The README example builds the URL from the primary slug instead of slugs.first, which can name a different URL from the copy button and the QR code once a custom slug is primary. No spec change, so the hash stays.
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
shrtnr | c8439a3 | Oct 20 2026, 12:09 AM |
DennisAlund
left a comment
DennisAlund
left a comment
There was a problem hiding this comment.
Review of the short origin follow-ups: 15 inline findings, ranked roughly by severity. The first four can break a real call or a real run. The rest cover parity, UX, test accuracy and cleanup.
| # Build a short URL on the origin the deployment hands links out on | ||
| # Build a short URL on the origin the deployment hands links out on, from the | ||
| # primary slug: the one the copy button and the QR code use | ||
| config = client.config.get() |
There was a problem hiding this comment.
config.get() fails on the deployments the new fallback paragraph promises to support.
/_/api/config arrived in #72 together with short_origin. On a Worker that has not been redeployed, client.config.get() raises ShrtnrError(404). Meanwhile link.short_origin works there through the new fallback. A reader who follows this example after a pip upgrade gets the 404.
The TypeScript (README.md:133) and Dart (README.md:135) examples have the same problem. Two ways out: build the example from link.short_origin, or give config.get() the same base-URL fallback on a 404 in all three SDKs.
There was a problem hiding this comment.
Fixed in 3cbd4b1. All three README examples now build the URL from the link's own short origin (link.short_origin / link.shortOrigin), which the SDK fills in on older deployments. The comment notes that config.get() needs a deployment that serves /_/api/config.
| function copyThenOpen(link) { | ||
| var primary = pickPrimarySlug(link); | ||
| var copied = primary ? copyUrl(primary) : Promise.resolve(false); | ||
| return copied.then(function(ok) { |
There was a problem hiding this comment.
Navigation now waits on the clipboard with no upper bound.
quickShorten() and createDuplicate() only navigate once writeText() settles. If the write never settles (for example an embedded webview, or a browser holding a permission prompt open), the link exists but the page shows no toast and never navigates. A second click creates another link.
Race the copy against a short timeout (about 1 s) and navigate on whichever finishes first. A timeout counts as ok === false.
There was a problem hiding this comment.
Fixed in c0e09c0. copyThenOpen() races the write against a 1.5 s timer. A write that never settles counts as a failed copy, and the link page opens, which shows the short URL. New unit test: opens the link page when the write never settles (fake timers, for both quickShorten and createDuplicate).
| def _origin_of(base_url: str) -> str: | ||
| """The origin of the client's base URL: what a deployment older than the | ||
| ``short_origin`` field hands short URLs out on.""" | ||
| parts = urlsplit(base_url) |
There was a problem hiding this comment.
urlsplit() can raise in the constructor, which breaks the documented error contract.
Shrtnr(base_url="http://[::1", api_key=...) now raises ValueError('Invalid IPv6 URL') from _SyncResource.__init__. The httpx.Client built a few lines earlier in Shrtnr.__init__ then never gets closed. Before this change, the first request raised ShrtnrError(0), which test_malformed_base_url_wraps_as_status_0 pins.
TypeScript (originOf) and Dart (on Object) both catch this and fall back to the URL as given. Wrap this call in try/except ValueError and return base_url.rstrip("/") to match them.
There was a problem hiding this comment.
Fixed in 5be5502. _origin_of() catches the ValueError from urlsplit() and .port and falls back to the base URL as given, so the constructor succeeds and the first call raises ShrtnrError(0), as in TS and Dart. Parametrized test covers http://[::1 and a non-numeric port.
| url: `${UNPINNED_URL}/_/health`, | ||
| timeout: 120_000, | ||
| reuseExistingServer: false, | ||
| env: { E2E_PORT: String(UNPINNED_PORT), E2E_STATE: ".wrangler/e2e-state-unpinned" }, |
There was a problem hiding this comment.
The unpinned server can still be pinned by .dev.vars.
e2e-server.sh leaves SHORT_ORIGIN unset by omitting --var SHORT_ORIGIN. wrangler dev still loads .dev.vars from the project root. A developer who adds SHORT_ORIGIN there to try the feature then runs default-origin.spec.ts against a pinned server, and it fails with no hint about why. That is the stale-environment failure the reuseExistingServer: false change set out to remove.
Always pass --var "SHORT_ORIGIN:$SHORT_ORIGIN", even when empty. configuredShortOrigin() treats a blank value as unset. Check that --var takes precedence over .dev.vars in wrangler 4.
There was a problem hiding this comment.
Fixed in c6448ca. scripts/e2e-server.sh always passes --var SHORT_ORIGIN:$SHORT_ORIGIN. I checked against wrangler 4.127.1 with a .dev.vars that sets SHORT_ORIGIN=https://oddb.it: without the flag the page rendered https://oddb.it, and with the empty flag it rendered the request origin.
| # netloc keeps any user:password@ prefix; an origin never carries it, and | ||
| # a short URL built from it would hand the credentials out. | ||
| host = parts.netloc.rpartition("@")[2] | ||
| return f"{parts.scheme}://{host}" if parts.scheme and host else base_url.rstrip("/") |
There was a problem hiding this comment.
Python's fallback origin is not normalized the way the server, TypeScript and Dart normalize it.
URL.origin and Uri.origin lowercase the host and drop a default port. This keeps both as typed. With base_url="https://Shrtnr.Example:443", Python reports https://Shrtnr.Example:443 while TypeScript and Dart report https://shrtnr.example. That is one deployment and two different short URLs.
CLAUDE.md asks the SDKs to stay in step on base-URL handling. httpx.URL(base_url) already ships with the SDK and returns the scheme, host and port normalized.
There was a problem hiding this comment.
Fixed in 5be5502. The fallback now lowercases scheme and host, drops a default port and keeps IPv6 brackets, matching URL.origin / Uri.origin. Parametrized test: https://Shrtnr.TEST:443 gives https://shrtnr.test, HTTP://shrtnr.test:80/api gives http://shrtnr.test, and a non-default port and [::1] are kept.
| // link page that opens next shows the short URL. | ||
| function copyThenOpen(link) { | ||
| var primary = pickPrimarySlug(link); | ||
| var copied = primary ? copyUrl(primary) : Promise.resolve(false); |
There was a problem hiding this comment.
copyThenOpen() gets toasts from copyUrl() that it throws away right after.
On success, Copied {url} is replaced in the same tick by created & copied. On failure, the red "copy it by hand" toast flashes and the navigation discards it.
Call copyText(shortOrigin() + '/' + primary) directly and keep the one toast this function decides on.
There was a problem hiding this comment.
Fixed in c0e09c0. copyThenOpen() calls copyText() directly, so only 'created & copied' shows on success, and no toast flashes before the navigation on failure.
| """Get a link by ID. Optional range scopes the click-count window.""" | ||
| url = self._url(f"/_/api/links/{id}", {"range": range}) | ||
| return Link.from_dict(self._request("GET", url, headers=self._headers())) | ||
| return Link.from_dict(self._request("GET", url, headers=self._headers()), self._origin) |
There was a problem hiding this comment.
The fallback is repeated at every call site, so the next method that returns a link can miss it.
Python has 16 hand-written self._origin arguments. TypeScript and Dart have 8 withShortOrigin and defaultOrigin: call sites each. A future method that returns a Link and skips this brings back the KeyError, TypeError or undefined this PR fixes.
The server commit moved the lookup into fromLinkResult(c, ...) for exactly this reason. The SDKs could do the same: a _link(data) and _links(data) helper on the resource base classes, a requestLink and requestLinks pair on HttpClient, and a helper on ShrtnrBaseClient.
There was a problem hiding this comment.
Fixed in 174b671 (TS HttpClient.requestLink()), 8fa4129 (Python _request_link() / _request_links()) and c8439a3 (Dart requestLink() / requestLinks()). The origin is private to the transport in TS and Dart, so a resource cannot pass it by hand. The Python fallback test now covers create, update, disable and enable too, as the TS and Dart suites already did.
| // Both become false here rather than a throw, and every caller reads the | ||
| // answer: a toast that announces a copy that never happened is worse than | ||
| // none, since the user moves on without the text. | ||
| function copyText(text) { |
There was a problem hiding this comment.
This reports the plain-http failure accurately, but it does not fix it.
On http://192.168.x.x, navigator.clipboard is undefined, so every chip click and every API key copy now fails, with an error toast each time. A hidden <textarea> plus document.execCommand('copy') still works in insecure contexts when it runs inside the click gesture, and both copyRawKey() and the chip handler run inside one.
Try navigator.clipboard first, then the execCommand fallback. Keep the error toasts for the case where both fail.
There was a problem hiding this comment.
Not done in this PR. e2e/short-origin.spec.ts › "without a clipboard" pins the current rule: no navigator.clipboard means an error toast naming the URL. With an execCommand fallback, Chromium on localhost would copy successfully and that spec would fail. Changing it is a behavior decision for the maintainer, not a review fix, so I'm leaving it open.
| id: id ?? this.id, | ||
| name: name ?? this.name, | ||
| description: identical(description, _unset) ? this.description : description as String?, | ||
| description: identical(description, _unset) |
There was a problem hiding this comment.
Unrelated dart format changes ride along in the fix commit.
e923741 also reformats Bundle.copyWith, BundleWithSummary, ClickStats, TimelineSummary and the shrtnr.dart export list. CLAUDE.md says: "One coherent change per commit (one refactor, one feature, one fix) ... Do not batch unrelated changes." The formatting buries the two-line behavior change. A separate formatting commit would keep the fix easy to review.
There was a problem hiding this comment.
Won't fix. e923741 is already pushed, and splitting it means rewriting published history with a force push, which this repo never does. Every commit after it keeps formatting out of the behavior change.
| }, | ||
| { | ||
| // SHORT_ORIGIN unset, the default most deployments run with. | ||
| command: "bash scripts/e2e-server.sh", |
There was a problem hiding this comment.
The second server boots on every Playwright run.
That includes yarn perf (playwright test e2e/perf.spec.ts) and single-spec runs that never reach default-origin.spec.ts. Each one now pays for a second migration and wrangler dev boot, and needs port 8798 free or it fails up front.
You could start this server only when the default-origin project runs, for example behind an env flag that yarn perf leaves unset.
There was a problem hiding this comment.
Fixed in 18f3290. yarn perf sets E2E_PINNED_ONLY=1, which drops the second server and the default-origin project. yarn e2e still boots both.
The no-clipboard and rejected-write cases asserted 'Copied ...'. They passed only because the stub t() ignored its key and the stub toast() dropped the type, so a copyUrl() that announced a failed copy as a success stayed green. The stub now echoes the key and records the type, and both cases assert the copyFailedUrl error toast the code emits.
On the links list the 'Copy it by hand: {url}' toast is the only place the
full pinned-origin URL appears, and it hid after 3 s like every other toast.
It now stays for 10 s. toast() takes the duration and clears the previous
toast's hide timer, which used to hide a newer message early.
copyThenOpen() waited on navigator.clipboard.writeText() with no limit. A write the browser leaves pending (an unanswered permission prompt, a webview that never settles it) left the user on the page with no toast and no navigation, and a second click created a second link. The wait now ends after 1.5 s and the link page opens, as it does for a failed copy. It also calls copyText() directly. copyUrl()'s own toast was replaced in the same tick by 'created & copied' on success and discarded by the navigation on failure.
The second e2e server left out --var SHORT_ORIGIN to run with the variable unset. wrangler dev still loads .dev.vars, so a developer with SHORT_ORIGIN set there got a pinned server on :8798, and default-origin.spec.ts failed with no hint why. The script now always passes the flag; an empty value overrides .dev.vars (checked against wrangler 4.127.1 with a .dev.vars that sets SHORT_ORIGIN: the page renders the request origin).
Its header claimed to catch a script that ignores location.origin. The server always renders the request origin into data-short-origin when SHORT_ORIGIN is unset, so the script's fallback never runs in this spec: a shortOrigin() returning pinned || '' still passes. The header now names what the spec guards (the server's fallback) and points at the unit test that covers the script's.
Every Playwright run booted the second wrangler server, including yarn perf, which runs one spec against the pinned server. It paid for a second migration and boot, and failed up front when :8798 was taken. yarn perf now sets E2E_PINNED_ONLY, which drops that server and the default-origin project.
…owups # Conflicts: # sdk/dart/README.md # sdk/python/README.md # sdk/typescript/README.md
The example in all three SDK READMEs called client.config.get(), which answers 404 on a Worker deployed before #72: the deployments the fallback paragraph says the SDK supports. It now reads the link's own short origin, which the SDK fills in there. It also picked the primary slug, else the first slug. The copy button and the QR code (pickPrimarySlug) try the first custom slug before the generated one, so a link with no flagged primary printed a different URL than the UI handed out.
Two departures from the server and the other SDKs in _origin_of(): - It kept the host's case and a default port. https://Shrtnr.Example:443 gave https://Shrtnr.Example:443, where the server, TypeScript (URL.origin) and Dart (Uri.origin) give https://shrtnr.example. Scheme and host are now lowercased and a default port dropped. - urlsplit() raises ValueError for an unclosed IPv6 bracket, so Shrtnr(base_url='http://[::1') raised from the constructor and leaked the httpx.Client built just before. TypeScript and Dart construct the client and raise their documented error on the first call. An unparseable base URL now falls back to itself and fails there with ShrtnrError(0).
Link.fromJson replaced only a null short_origin (??). Python (or) and TypeScript (truthy check) also replace an empty string, so a response carrying short_origin "" gave the base origin there and '' in Dart, which then built '/abc'. Dart now falls back on an empty value too. The same case is pinned in the TypeScript and Python suites, where it already held.
Each of the eight Link-returning methods appended .then((value) => withShortOrigin(value, this.http.origin)) by hand. The next method that returns a Link and forgets it brings back the undefined shortOrigin this branch fixes. HttpClient.requestLink() now applies the fallback, and the origin is private to it, so a resource can no longer pass it around itself.
Sixteen call sites passed self._origin to Link.from_dict() by hand. The next method that returns a Link and forgets it brings back the KeyError this branch fixes. _request_link() and _request_links() on the shared resource base now apply the fallback, as HttpClient.requestLink() does in TypeScript. The fallback test now covers create, update, disable and enable as well, matching the TypeScript and Dart suites, which already swept every Link-returning method.
Eight call sites passed defaultOrigin: _http.origin to Link.fromJson() by hand. The next method that returns a Link and forgets it brings back the TypeError this branch fixes. ShrtnrBaseClient.requestLink() and requestLinks() now apply the fallback, as the TypeScript and Python SDKs do, and the origin is private to the base client.
1 participant
Follow-up fixes from the review of #72. One commit per fix.
Fixes
KeyErrorand Dart threw aTypeErroron every call that returns a link when the Worker had not yet been redeployed. TypeScript returnedshortOriginasundefined. All three now fall back to the origin of the client's base URL. That origin drops a trailing slash, a path and any credentials.short_originwhen the deployment pins one. This covers create, get, update, disable, enable, list, search, list-by-owner and bundle links. With nothing pinned the field stays off, because an MCP request often arrives on themcp.*host.get_link_qrlistsbase_urlas required exactly when the deployment pins noSHORT_ORIGIN.delta_pctworks as before. In TypeScriptLinkis an interface, and the field stays required because every link the client returns carries it.fromLinkResult(c, result)resolves the origin from the request context instead of nine handlers repeating the lookup. The admin routes spread onelayoutobject into<Layout>.wrangler devruns withSHORT_ORIGINunset, anddefault-origin.spec.tschecks the copy button, the clipboard and the QR modal against it. Both e2e servers now boot fresh on every run, so a leftover server can no longer run the suite with the wrong environment.slugs[0].Notes for review
env, which never happens at runtime, so they now get the test env. The existingcopyUrl()unit tests nowawaitthe promisecopyUrl()returns, because the toast waits for the write. Their assertions are unchanged.main. The surface change from Pin short URLs to SHORT_ORIGIN #72 and these fixes call for minor bumps: npm 1.3.0, PyPI 1.3.0 and pub 2.3.0, with CHANGELOG entries covering both PRs. The spec did not change, so the hashes stay.Verification
yarn test: 1670 passed. Typecheck clean.yarn e2e: full suite green, including the new default-origin and no-clipboard steps.scripts/test-sdks-e2e.sh: TypeScript, Python and Dart all pass against a live wrangler dev.