Repository navigation
🍕 Stop doubling the site path in exported/imported urls, and say why imports fail - #279
Conversation
… urls alone `remove` stripped only the origin from an absolute `url`/`customUrl`, so on a site mounted under a path (https://nymag.com/strategist) the exported /strategist/gift-guides had the whole prefix added again on import: https://nymag.com/strategist/strategist/gift-guides. It applies to every "url" key in a dispatch, so component data (nav, trending links) was hit, not only pages. - remove: only relativize urls on the exported site's own host, dropping its origin and site path (http/https and case insensitive). Urls on any other host (images, affiliate links, other sites) are left as they are. - add: only prefix paths on the site (a single leading /). Absolute urls, #anchors, mailto: and //cdn urls are left alone; a page's own absolute url/customUrl is still moved to the target origin.
- failed requests: the message has the status code and the reason amphora
gave in the response body ("400 Bad Request: Cannot publish: ..."), and
the result has a `status` field. An empty status text (http/2) no longer
gives an empty message.
- network failures: report the cause node puts on the error
("fetch failed: getaddrinfo ENOTFOUND host") instead of just "fetch failed".
- yaml bootstraps: report the line and column, the section it is counted from
when piped from stdin, and the lines around the error. The message used to
be cut at the first colon, which chopped off the line number.
- json dispatches: report the input line and the part of it the parser choked
on, instead of the first 30 characters.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
coverage: 85.385% (+0.5%) from 84.881% — jordan/strategist-url-clay-import-06711c into master |
Leaving every url on another host alone also stopped moving links to the site itself that sit on another environment's host (https://nymag.com/strategist/beauty in the data of https://localhost.nymag.com/strategist, or https://www.thecut.com/ in a thecut bootstrap), which `add` used to move to the target. A url is on the site if it is on the site's host, or on another host of the same domain that is the site's and not another service's: - a site mounted on a path (nymag.com/strategist): the path has to be inside the site's, so nymag.com/intelligencer and nymag.com/newsletters are not. - a site on the root of its host (thecut.com): the host has to be the domain or its www, so pyxis.nymag.com and mediakit.nymag.com are not. Checked by importing the sites first-run bootstraps (7 sites, ~2000 url values) with the old and the new code: every same-site link comes out the same as before, and what differs is social links, other services and other sites that the old code moved onto the importing site's host.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 781465e9b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…dle country domains and the site root Review findings on the on-site rule, plus two more found while probing them: - a site under a path (https://nymag.com/strategist) counted every url on its own host as the site's, so a link to a sister site (nymag.com/intelligencer) was exported as /intelligencer and imported as /strategist/intelligencer. The path now decides for the site's own host as well, not only for its other hosts. - domains were the last two labels, so publisher.co.uk and merchant.co.uk were both "co.uk": other sites on the suffix could count as the site, and a root-mounted .co.uk/.com.au site no longer moved its own www links, which rc.11 did. Country domains now keep the label above co/com/org/etc, and an ip address is its own domain. This is a rule of thumb, not the public suffix list. - the site itself, a ?query and a #fragment after the site path were exported as "", "?a=b" and "#top", which import reads as no path at all, so the link was lost or stayed relative to whatever page it was on. They are now /, /?a=b and /#top. - a trailing slash on the site prefix (-p https://x.dev.nymag.com/strategist/, as import-featurebranch-pages.js passes) made the path check never match.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4ef8bff3c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| types = require('./types'); | ||
| types = require('./types'), | ||
| PROTOCOL = /^https?:\/\//i, | ||
| ORIGIN = /^https?:\/\/[^/]+/i, |
There was a problem hiding this comment.
For a root-mounted site, a valid slashless root URL such as https://domain.com?a=b or https://domain.com#top loses its query or fragment because [^/]+ consumes that suffix as part of the origin. Consequently, stripSite exports either value as /, and addSite can likewise discard the suffix when remapping an absolute URL; the origin expression should also stop at ? and #.
Useful? React with 👍 / 👎.
The origin was read up to the next `/`, so for a url with no path before its query or fragment (`https://domain.com?a=b`, `https://domain.com#top`) the `?a=b` / `#top` was taken as part of the origin. `remove` exported it as `/` and `add` moved it to the target host without it. Stop the origin at `?` and `#` too, and use the same origin match for the import prefix.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec8660ca3f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…irst colon The port was dropped by splitting the host at the first colon, which turns every bracketed ipv6 literal into the same `[2001`. Two different addresses then counted as one domain, so an external ipv6 url with the site's path was relativized on export and moved to the target host on import. Take the whole bracketed literal as the name, and as its own domain like an ipv4 address.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a15a4d9c58
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return isInSitePath(url.replace(ORIGIN, ''), sitePath); | ||
| } | ||
|
|
||
| return host === siteHost || _.includes([domain, `www.${domain}`], getHostname(host)); |
There was a problem hiding this comment.
For a root-mounted site on a subdomain, equivalent port spellings make this return false because host === siteHost compares the port too and the subdomain is neither the apex nor www. For example, exporting component data with prefix https://stg.example.com:443 leaves an on-site link such as https://stg.example.com/article absolute, so a later import to another environment does not move that link. Compare the two hostnames directly before applying the apex/www restriction to other same-domain hosts.
Useful? React with 👍 / 👎.
For a site on the root of a subdomain, a url on the site's own host only counted when its host matched the prefix's character for character, port included. With the prefix `https://stg.example.com:443`, the link `https://stg.example.com/article` was left absolute and an import to another environment did not move it. Compare the hostnames, as a site on a path and an ip address already did.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Why
The Strategist nav on prod had urls like
https://nymag.com/strategist/strategist/gift-guides. The stored component data (categories-sub-navigation/main,nav-dropdown-button/homepage) is doubled, and the oldprefixes.remove+prefixes.addproduce exactly that for any site mounted under a path:It is not limited to pages: both functions rewrite every
"url"/"customUrl"key in a dispatch, so component fields (nav, trending links) were hit. #241 addedurlto the rewrite and stripped the whole site prefix, which was right; #239 ("Add clay vite") then changedremoveto strip only the origin, and that is in every6.0.0-rc.*. The same functions also moved external urls onto the site (https://pyxis.nymag.com/v1/imgs/a.jpgbecamehttps://nymag.com/strategist/v1/imgs/a.jpg; in staging data theurlfields ofarticle/product/imageare overwhelmingly pyxis image urls) and prefixed#anchors,mailto:,//cdnurls and empty strings.Sites' feature-branch hydrate (
clay export <stg page> -p -l | clay import -k qa -p https://<branch>.dev.nymag.com/strategist/) runs exactly this shape. How the bad data got into prod is not established: Datadog's indexed logs only reach back ~7 days and PUT spans are sampled, so this PR fixes the tool, not the existing data (staging has the same doubled nav data).What changed
lib/prefixes.js(a url is rewritten only if it is on the site)removerelativizes a url on the site: the origin and the site path are dropped (http/https and case insensitive, only at a path boundary). What is left always starts at the site root, so the site itself,?a=band#topare exported as/,/?a=band/#top, whichaddcan put back. A query or fragment right after the host (https://thecut.com?a=b, no/before it) is read as the query, not as part of the origin, on both export and import. Any other url is left as it is.nymag.com/strategist): the path has to be inside the site's, on its own host too, sonymag.com/intelligencer,nymag.com/newslettersand the network rootnymag.com/are not on it;thecut.com): the host has to be the site's own, the domain or itswww, sopyxis.nymag.comandmediakit.nymag.comare not.co.uk,com.auand similar country domains count one label lower, and an ip address (ipv4, or an ipv6 literal in its brackets) is its own domain. That is a rule of thumb for the common cases, not the public suffix list (which would be a new dependency).https://stg.nymag.com:443andhttps://stg.nymag.comare the same site, as is the same hostname on another port).-p https://<branch>.dev.nymag.com/strategist/, asimport-featurebranch-pages.jspasses) is ignored when telling what is on the site.addonly prefixes paths on the site (a single leading/) and moves absolute urls that are on the site to the target origin.#anchors,mailto:,//cdnurls, empty strings and absolute urls that are not on the site are left alone. A page's ownurl/customUrlis always moved to the target origin, as before.lib/rest.js,lib/cmd/import.js(errors you can act on)400 Bad Request: Cannot publish: page has no layout(status code + the reason from amphora's response body), plus astatusfield on the result. An empty status text (http/2) no longer gives an empty message.fetch failed: getaddrinfo ENOTFOUND hostinstead offetch failed.Behavior changes to know about
404 Not Found). I updated the existing tests that asserted the old text.https://nymag.com/newsletters/in a thecut bootstrap, social links,mediakit). Links to the site itself are still moved.localhost:3001prefix, or an ip address) is left as it is, because there is nothing to tell it is the same site; with a domain prefix such aslocalhost.nymag.comor<branch>.dev.nymag.comit is moved as before. Emptyurlvalues stay empty instead of becoming the site root./strategist/about-us/on the sitehttps://nymag.com/strategist) is still read as relative to the site, so it still gets the prefix. That is the contract of the exported format, not something claycli can tell apart from a real/strategist/strategist/...page.Test plan
npm test(lint + jest): 645 tests pass, including new ones for a path-bearing prefix (pageurl/customUrl, nested component urls, http vs https, site root with a query or fragment, with or without a/before it, path boundary, sister sites on the same host,.co.uk/.com.au, ipv4 and ipv6 addresses, trailing slash, export-then-import round trips), other hosts of the site, external urls, non-path urls, and every new error messagewww.thecut.com220,www.curbed.com32,nymag.com/strategist/...); what differs is social links,mediakit,qualtrics, other sites' links and empty values that the old code moved onto the importing hostclay importwith bad JSON / bad YAML on stdin)