Skip to content

include unify script - #1279

Open
tmashara wants to merge 2 commits into
masterfrom
include-unify-script
Open

include unify script#1279
tmashara wants to merge 2 commits into
masterfrom
include-unify-script

Conversation

@tmashara

@tmashara tmashara commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

part of https://linear.app/metabase/issue/GRO-808/restore-unify-tracking-on-migrated-marketing-pages
website PR https://github.com/metabase/metabase.github.io/pull/6344

What

Docs pages opt into Unify through _data/unify-enabled-pages.yml. _includes/head.html stamps <meta name="enable-unify"> on any page whose URL contains a listed path; the marketing site's cookie-consent.js (website PR above) reads that tag and injects /js/unify.js, but only after the visitor grants marketing consent. Inert until that PR ships, so merge order doesn't matter.

Why

Docs pages load no Unify at all today. The {% if %} chain in _includes/footer-scripts.html only ran via footer.html, which docs layouts never include — so the four embedding URLs marketing tracks have been dark, and that block also loaded the script without consent. Both are gone.

js/unify.js is deleted rather than fixed: the marketing build owns /js/* on www.metabase.com and shadows this repo's copy, so a unify.js here is never served.

Docs paths live only in this list; the marketing list holds only non-docs pages, so the two are disjoint and there is nothing to keep in sync. Nothing verifies the list automatically, so the paths need a manual re-check when embedding docs move.

Verification

Full astro build: exactly 4 of 14,125 pages carry the tag (the four intended ones), versioned copies and controls are untagged, and no page references js/unify.js. The Liquid block renders identically under Jekyll's Ruby Liquid and the liquidjs renderer that now builds doc pages.

@tmashara
tmashara requested a review from a team August 12, 2026 13:48

@bpander bpander left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Main issue is I think the meta name value is wrong, but I also think we could maybe put more of this logic on the ts/astro side (for better devex and compile-time checking).

Comment thread _includes/head.html Outdated
{%- endcomment -%}
{%- for unify_path in site.data["unify-enabled-pages"] -%}
{%- if page.url contains unify_path %}
<meta name="enable-unify" content="true" />

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be name="unify" right?

That's what cookie-consent.js looks for:

    if (document.querySelector('meta[name="unify"]')) {

https://github.com/metabase/metabase.github.io/pull/6344/changes#diff-364e1f1ccdd08f5d41c5742e955c2bfc199508804d3eab3fcab1b9f21d9acd1bR176

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, thanks! updated

Comment thread _data/unify-enabled-pages.yml Outdated
@@ -0,0 +1,37 @@
# Docs pages that load Unify (visitor de-anonymization for outbound).

@bpander bpander Aug 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can/should put more of this logic on the ts/astro side. There's also a whole bunch of comments, some of which I don't think belong in the places they're at currently, and we're also probably moving _docs out of version control soon so they'll probably be outdated soon.

Maybe something like this?

Image

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, it looks much cleaner, I'll take it :)

@tmashara
tmashara force-pushed the include-unify-script branch from c7bd0f9 to 9d930fc Compare August 12, 2026 19:05
@tmashara
tmashara requested review from bpander and removed request for bpander August 12, 2026 19:06
@tmashara
tmashara force-pushed the include-unify-script branch from 9d930fc to 8bd617f Compare August 12, 2026 19:16
@tmashara
tmashara force-pushed the include-unify-script branch from 8bd617f to c13b113 Compare August 12, 2026 19:16
@tmashara
tmashara requested a review from bpander August 12, 2026 19:18

const lq = getLiquidRenderer({ page, dirname });

const unify = UNIFY_ENABLED_PAGES.includes(Astro.url.pathname);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I steered you slightly wrong. I forgot there was some dev server/full-build weirdness with Astro.url (inconsistent trailing slashes). I should've told you to use page.url which is the normalized url.

Suggested change
const unify = UNIFY_ENABLED_PAGES.includes(Astro.url.pathname);
const unify = UNIFY_ENABLED_PAGES.includes(page.url);

It'll also need url: string; added to this template's Props page type to make TS happy (it won't trigger any other TS errors). Sorry about that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants