Banner: absolutize root-relative hrefs in wp-admin (Odyssey) - #113168
Conversation
Root-relative hrefs on Banner/UpsellNudge are Calypso routes, but wp-admin resolves them against the site's own domain - every upgrade nudge rendered in Odyssey Stats linked to a 404 (the SEO preview nudge was patched at its call site in #113144; stats-no-content-banner's /post link is still broken today). Absolutize at the Banner chokepoint - computed defaults, caller-provided hrefs, and the secondary CTA all pass through it - via getCalypsoUrl(), a no-op outside wp-admin. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
WordPress.com
Automattic for Agencies
|
chihsuan
left a comment
There was a problem hiding this comment.
Thanks for the follow-up! @dognose24
The Banner chokepoint is the right call, and the /-only trigger keeps Calypso and Jetpack Cloud untouched.
A couple of things worth addressing before merge:
- The wiring itself isn't covered by tests. I removed
toCalypsoHreffromgetHref()and fromsecondaryHreflocally, and the wholecomponents/banner/test/suite still passed.
The rest are optional cleanups and shouldn't block.
| return getCalypsoUrl() + href; | ||
| } | ||
| return href; | ||
| } |
There was a problem hiding this comment.
Would client/lib/url/ be a better home for this? It's not really a Banner concept, and pulling it in from here drags the whole component.
There was a problem hiding this comment.
Worth noting there are already ~4 versions of this in the tree (post-likes, mini-carousel, stats-upsell, plus getUpgradeNudgeHref) and they've genuinely drifted. Not asking you to unify them here, just thinking a shared home makes that possible later. What do you think?
| ! href.startsWith( '//' ) && | ||
| config.isEnabled( 'is_odyssey' ) | ||
| ) { | ||
| return getCalypsoUrl() + href; |
There was a problem hiding this comment.
nit: getCalypsoUrl() already takes a path and strips the leading slash, so could this just be getCalypsoUrl( href )?
| if ( ! href && siteSlug && canUserUpgrade ) { | ||
| if ( customerType ) { | ||
| return `/plans/${ siteSlug }?customerType=${ customerType }`; | ||
| return toCalypsoHref( `/plans/${ siteSlug }?customerType=${ customerType }` ); |
There was a problem hiding this comment.
Would it be worth wrapping once at the boundary rather than at each return?
| // wp-admin. Everywhere else (Calypso proper, Jetpack Cloud) this is a no-op. | ||
| export function toCalypsoHref( href ) { | ||
| if ( | ||
| href && |
There was a problem hiding this comment.
Tiny robustness thought, could we make this typeof href === 'string' &&?
kangzj
left a comment
There was a problem hiding this comment.
Tested locally and reviewed — LGTM. ![]()
Testing
- Unit tests:
yarn test-client client/components/banner/test/ client/blocks/upsell-nudge/test/— 36/36 pass as described. - Live check, Calypso proper: on
/stats/day/<simple-site>from the local dev server, the "Free domain available"UpsellNudgeCTA rendershref="/domains/add/<site>"— relative, unchanged. No regression. - Live check, Odyssey branch: reloading the same page with
?flags=is_odyssey(flips the real config flag in the dev bundle), the same banner rendershref="https://wordpress.com/domains/add/<site>". Both branches verified in the actual bundle, not just Jest. - Odyssey bundle:
apps/odyssey-statsyarn build && yarn test:sizepasses — 526.97 kB gzipped against the 583.68 kB limit, so the new@automattic/calypso-urlimport costs nothing meaningful. - Lint clean on both changed files; all CI checks green.
Review notes
- Trigger condition is exactly right (
/but not//, Odyssey-only), and it composes safely with #113144 already on trunk:getUpgradeNudgeHrefproduces an already-absolute URL, which passes throughtoCalypsoHrefuntouched — no double-prefixing after merge. Note this branch forked just before #113144 landed, so a rebase before merge would make CI reflect the trunk it will actually land in. - Audited stats surfaces for hrefs that should not leave wp-admin: the only Banner/UpsellNudge call site with an explicit href is
stats-no-content-banner's/post/<slug>, a genuine Calypso destination. No internal Odyssey route flows throughBanner, so nothing that relied on hashbang navigation changes behavior. - One imprecision in the description: per
apps/odyssey-stats/README.md, the stats-admin package intercepts anchor clicks whose href doesn't start withhttpand rewrites them to hashbang routes — so the pre-fix failure mode is likely a mangled#!/post/…route inside wp-admin rather than a domain 404. Either way the link is broken and the fix is correct; if anything the interceptor strengthens it, since absolutizedhttps://links are exactly what escapes interception. Worth one sanity click in a real Simple-site wp-admin before merge since local simulation can't reproduce the interceptor. - One non-blocking suggestion inline about the helper's location.
| // Calypso absolutely there; getCalypsoUrl() falls back to | ||
| // https://wordpress.com when the current origin isn't a Calypso one, as in | ||
| // wp-admin. Everywhere else (Calypso proper, Jetpack Cloud) this is a no-op. | ||
| export function toCalypsoHref( href ) { |
There was a problem hiding this comment.
Non-blocking: a component file is a slightly odd home for a URL helper, and the same bug exists on Odyssey-rendered links that don't go through Banner — e.g. client/my-sites/stats/promo-cards/index.jsx (/advertising/…) and client/my-sites/stats/stats-email-detail/index.jsx (/post/… via CoreButton). Those call sites can't reasonably import from calypso/components/banner, so moving this to calypso/lib/url (or @automattic/calypso-url) would let the planned #113144 cleanup and those other surfaces share it. Fine as a follow-up.
Review feedback on #113168: - toCalypsoHref isn't a Banner concept, and call sites that can't import from a component (promo-cards, stats-email-detail, and #113144's getUpgradeNudgeHref) need it too. Move it to calypso/lib/url so the ~4 drifted copies in the tree have somewhere to converge. - The wiring itself was untested: cutting toCalypsoHref out of getHref() and secondaryHref left the suite green. Add render tests covering every href Banner emits — computed plans hrefs, caller-provided hrefs, the CTA button, and the secondary CTA — which now fail if the wiring is removed. - getCalypsoUrl() already strips the leading slash, so pass the href straight through instead of concatenating. - Guard on typeof href === 'string' rather than truthiness. - Wrap once where getHref() returns instead of at each branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ke-upsellnudge-banner-default-hrefs-absolute
|
Thanks both — addressed the blocker and took the Wiring coverage. You were right that the suite passed with Moved to Nits. |
kangzj
left a comment
There was a problem hiding this comment.
Reviewed the chokepoint approach and the new toCalypsoHref() helper — branch precedence in getHref() is preserved by the refactor, the guard correctly skips absolute and protocol-relative hrefs, and there's no double-absolutization with the getUpgradeNudgeHref fix from #113144. The new banner/test/odyssey-href.jsx covers the wiring gap I flagged earlier.
One optional nit: the JSDoc in client/lib/url/to-calypso-href.ts says getCalypsoUrl() "falls back to https://wordpress.com when the current origin isn't a Calypso one", but it never reads window.location.origin — it always defaults to wordpress.com and only overrides via an allow-listed calypso_origin query arg. The no-op-outside-Odyssey behavior comes from the is_odyssey guard instead. Not a blocker.
LGTM.
Fixes STATS-397 — generalizes #113144's fix, as suggested in its review.
Proposed Changes
Banner/UpsellNudgeare Calypso routes, but wp-admin (Odyssey Stats) resolves them against the site's own domain — every upgrade nudge rendered in Odyssey linked to a 404. SEO preview nudge: link to wordpress.com absolutely in wp-admin #113144 patched one call site (the SEO preview nudge);stats-no-content-banner's/post/<slug>link is still broken today, and any future nudge would be too.Bannerchokepoint, where every variant converges: computed default hrefs (/plans/…withfeature/plan/customerType), caller-provided hrefs (coversUpsellNudge's computed href and explicit ones likestats-no-content-banner), and the secondary CTA all pass through a new exportedtoCalypsoHref()— which prefixesgetCalypsoUrl()whenis_odysseyis enabled and the href is root-relative./(not//, not absolute, notadmin.php?…) and only in Odyssey. Calypso proper and Jetpack Cloud are byte-for-byte unaffected (the helper is a no-op there). No Odyssey surface passes site-relative/wp-admin/…paths to Banner (wp-admin links are built absolute viagetSiteAdminUrlby convention — audited).getUpgradeNudgeHref) becomes redundant and can be removed in a small cleanup.Why are these changes being made?
Testing Instructions
yarn test-client client/components/banner/test/ client/blocks/upsell-nudge/test/— 36 tests green, including newtoCalypsoHrefcases: absolutizes root-relative routes in Odyssey; leaves absolute / protocol-relative / non-rooted URLs untouched; passes empty values through; no-op outside wp-admin.https://wordpress.com/plans/…(previously via SEO preview nudge: link to wordpress.com absolutely in wp-admin #113144's call-site fix; now via the chokepoint).https://wordpress.com/post/<site>instead of a 404.Pre-merge Checklist
🤖 Generated with Claude Code