Repository navigation
Conversation
rc.12 exported a page url that is the site itself (https://host) as `/` and
imported it as https://host/. A page that was already published at
https://host (grubstreet and vulture have `url: https://host`) was then
published again at https://host/. Amphora keeps every url a page was
published at, and when the last two entries reduce to the same hostname and
path it writes a redirect from the url to itself, which postgres rejects
("new row for relation uris violates check constraint id_ne_data"): the
publish answered 500 and the page stayed on its old version. The sites
feature-branch hydrate hit it on every branch whose database already held
those homepages.
For a page's own url and customUrl, leave what follows the origin and the site
path exactly as it was: https://host is exported as an empty string and
https://host/ as `/`, and a ?query or #fragment right after the host stays
there. `add` puts each form back as it was. Everything else keeps the #279
behavior: in components the site itself is `/` and an empty value stays empty.
Exporting a public url (clay export https://site/some-article.html) has no api route to take a prefix from, so fromURL passes a null prefix to prefixes.remove after exportPublicURL has already taken off the site it found. Since 6.0.0-rc.12 (#279) remove asks the prefix for its host to tell urls on the site from the rest, so any dispatch still holding an absolute url (an instagram link, a CDN image) made the export throw "Cannot read properties of null (reading 'replace')". rc.11 did nothing for a null prefix, and remove now returns the dispatch as it is.
|
coverage: 85.61% (+0.06%) from 85.553% — jordan/keep-page-url-form into master |
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. |
![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: ed756c454b
ℹ️ 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".
Review feedback (Codex, P1): the rule "an empty string is the bare site" applied to every url and customUrl in a page dispatch, so an unset customUrl ('') became the site root on import, and a ?query or #fragment value anywhere in a page was moved onto the host. Sites reads a falsy customUrl as "no custom url", so that would have made one up.
Only the `url` of a page keeps the bare form now: https://host is exported as an empty string and an empty url comes back as https://host, which is what keeps grubstreet's and vulture's homepages at the form they are already published at. A customUrl that is the bare site is exported as /, an empty customUrl stays empty, and a ?query or #fragment right after the host is stripped like any other url (/?a=b) instead of getting a form of its own. No real page uses either: of the 37 staging pages the hydrate job imports, none has an empty url or customUrl.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3bd5b2e101
ℹ️ 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".
jjpaulino
changed the title
Why
The sites "Hydrate Feature Branch" job (
clay export <stg page> -p -l | clay import -k qa -p https://<branch>.dev.<domain>/) went red for grubstreet and vulture on branches whose database already held those homepages. The cause is a change of url form in 6.0.0-rc.12 (#279).rc.11 exported a page's bare-origin url (
https://host) as an empty string and imported it back ashttps://host. #279 made the site itself/on export, so it came back ashttps://host/. A page already published athttps://hosttherefore changed form when republished. Amphora keeps every url a page was published at and treatshttps://hostandhttps://host/as two entries; itsaddRedirectsthen writes a redirect from the url to itself (samehostname + pathname), postgres rejects it (id <> data, migration 005), and the publish answers 500 with the page left on its old version.Reviewing this change also turned up that #279 broke
clay export <public url>for any page that holds an external link or image (second item below).What changed
The bare site as the
urlof a page (lib/prefixes.js;49ffcf4, narrowed in3bd5b2eafter Codex's review)urlof a page that is the bare site,https://host, is exported as an empty string, as before rc.12, and an empty pageurlcomes back ashttps://host.https://host/stays/, as in 🍕 Stop doubling the site path in exported/imported urls, and say why imports fail #279. This is the form grubstreet's and vulture's homepages are published at, so republishing them no longer changes it.customUrlthat is the bare site is exported as/(an emptycustomUrlmeans there is none, and stays empty), the site root in component data is/, and a?queryor#fragmentright after the host is stripped like any other url (/?a=b).ed756c4no throw without a prefix (lib/prefixes.js)clay export https://site/some-article.htmlhas no api route to take a prefix from, sofromURLpassesnulltoprefixes.removeafterexportPublicURLalready took off the site it found. Since 🍕 Stop doubling the site path in exported/imported urls, and say why imports fail #279,removeasks the prefix for its host, so any dispatch still holding an absolute url (an external link, a CDN image) threwCannot read properties of null (reading 'replace'). rc.11 did nothing for a null prefix;removenow returns the dispatch as it is.Checked
jest: 31 suites, 674 tests pass; eslint is clean. New tests cover a page whoseurlis the bare site (round trip, re-import, a site mounted on a path), an emptycustomUrl, anchors inside a page, and a null, undefined or empty prefix.urlsent for grubstreet and vulture is identical to rc.11's, and so is thecustomUrlof curbed, thecut and nymag. The only other differences are the doubled/intelligencer/intelligencer/and/strategist/strategist/urls rc.11 sent, which 🍕 Stop doubling the site path in exported/imported urls, and say why imports fail #279 fixed, and vulture'scustomUrl, which now has a trailing slash. Amphora triesurl || customUrlfirst (_checkForUrlProperty, the first step of a sequential chain), so withurlset and unchanged thecustomUrldoes not decide the publish url or its history.urlorcustomUrl: unset is simply absent.Notes
https://host/flips the other way when republished with this change and hits the same amphora 500. The durable fix is in amphora: skip the redirect when the previous and new address are the same, and compare normalised urls instoreUrlHistory.