Skip to content

fix(matchers): resolve four silent matcher-resolution defects - #481

Open
aka-sacci-ccr wants to merge 1 commit into
mainfrom
fix-matcher-resolution-bugs
Open

fix(matchers): resolve four silent matcher-resolution defects#481
aka-sacci-ccr wants to merge 1 commit into
mainfrom
fix-matcher-resolution-bugs

Conversation

@aka-sacci-ccr

@aka-sacci-ccr aka-sacci-ccr commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Four defects in matcher resolution, diagnosed and measured in production on deco-sites/farmrio-storefront. All four fail the same way: the variant doesn't match, the fallback renders, HTTP is 200, the console is clean. On farmrio this cost a wrong topbar on every PDP of a sub-brand without anyone noticing — and the failure mode is indistinguishable from "the content is misconfigured", which is where the team looks first.

Symptom that led here: the Farm ETC PDP showed FARM's topbar. The Alerta block is a website/flags/multivariate/section.ts whose ETC variant is guarded by an ETC Segment block: pathname Includes /farm-etc OR (pathname Template /:slug/p AND queryString brand=farmetc). The /farm-etc category page worked (first branch); the PDPs never did (second branch).

1. pathnameMatcher didn't implement type: "Template"

The switch covered Equals, Not Includes, Starts With, Includes, and the default did path.includes(case.pathname). The CMS also emits type: "Template" with route syntax (/:slug/p), which hit the default and tested whether the pathname contained the literal string "/:slug/p" — never true.

Now compiled with URLPattern, which resolves this natively and is available in the Workers runtime. Compiled patterns are memoized in a 100-entry LRU (farmrio carries a getURLPattern with an LRU precisely because compiling per request cost ~6% of CPU); invalid patterns memoize as null, and isSafePattern still gates compilation.

URLPattern is a global in workerd and Node ≥ 23, but not on the Node/RSC target @decocms/nextjs builds against — so there's a :param→segment / *→anything regex compiler behind it. Returning false there would have reintroduced exactly this bug on that binding. The fallback is covered by a test that deletes the global.

2. queryStringMatcher didn't understand the CMS format

It read rule.key ?? rule.param and rule.value at the top of the rule, returning false when it found no key. The CMS emits conditions: [{ case: { type: "Equals", value: "farmetc" }, param: "brand" }] — an array, with the param name beside case and the value inside it. So key was always undefined and the matcher was unconditionally false.

Both shapes are accepted now. conditions[] entries AND together; Equals, Not Equals, Includes and Not Includes are handled, with Equals still the default so an unknown case.type behaves exactly as it did. A repeated param matches on any value for positive types and no value for negated ones, which also makes an absent param satisfy a negation. Parsing now passes a base URL so a path-relative ctx.url doesn't fail closed.

On the "confirm which case.type actually appear" note: the schemas in this repo are the stub schemas we emit, not admin's, so I couldn't confirm the full set from here. Implemented is the set reported for farmrio (Includes, Template, Equals for pathname; Equals for queryString) plus Not Equals/Not Includes for queryString symmetry. If admin offers more, the switch is the place to extend — the default is unchanged, so nothing regresses in the meantime.

3. resolveSiteGlobals() resolved with no matcher context

The signature took no context, so no site.global section could carry a URL-, date- or cookie-dependent variant — which is exactly where Alerta lives. farmrio carries this as a local patch (patches/@decocms+tanstack+7.20.2.patch) that was never upstreamed. Brought here: the context reaches resolvePageSections, wired at both cmsRoute.ts call sites.

The single cache became a keyed LRU (64 entries). The key includes the query string: keyed on path alone, /x/p?brand=farm and /x/p?brand=farmetc collide and the first request on a cold path decides both — reproduced, requesting ?brand=farmetc first on a cold path makes both URLs serve ETC's alert. Params are sorted for a stable key and tracking params are dropped (via the existing registerTrackingParams registry, exposed as a new isTrackingParam) so utm_* doesn't fragment the cache.

I went past the query string you specified: device and the deco_segment cookie are in the key too. Device and A/B matchers are as common in site.global as URL ones, and now that the context actually arrives, a path+query-only key would have reintroduced the identical wrong-variant-from-cache failure on those axes. The residual limit is documented in the JSDoc — matchers reading other cookies, geo, or wall-clock time are still not represented in the key.

The "does this site declare globals at all" check moved to its own memo, so a globals-free site doesn't burn a cache slot per URL.

4. Matcher override contract was ambiguous

Two registration paths: one guarded by G.__deco._builtinMatchersRegistered at resolve.ts module load, and one unguarded — registerBuiltinMatchers() inside createSiteSetup, overwriting unconditionally. A site registering an override after createSiteSetup() might or might not win depending on module evaluation order in the bundle; observed working in vite dev and lost in the same site's production build.

Chose the explicit extension point. registerMatcher(key, fn) is the site API and always wins, before or after createSiteSetup(). Both builtin paths pass { builtin: true }, which skips a key a site already owns and overwrites only another builtin — so registerBuiltinMatchers() can still replace resolve.ts's inline inclusive date matcher with the strict one, but can't touch a site's. Ownership lives in a globalThis set beside customMatchers, so it holds across module instances. Adds unregisterMatcher(key) to drop an override and let the framework reclaim the key. Same behavior in dev and build.

Documented as constraints 9 and 10 in CLAUDE.md.

Tests

44 added, in the two existing files. The matcher tests drive the raw CMS shapes (case / conditions), not a normalized form — the divergence between the two was the bug. Includes the full ETC Segment rule end-to-end (Includes OR (Template AND queryString)), asserting the category page matched before and the PDP branch matches now, plus regression coverage pinning every pre-existing rule shape.

Verified the new tests fail against the old behavior rather than passing vacuously:

  • reverting the builtin-ownership guard → 2 override tests fail
  • reverting to a path-only cache key → 5 globals tests fail

bun run typecheck clean on blocks, tanstack, nextjs. Full suite: 2394 tests, the only failures are 4–5 pre-existing ones in draftShell.test.ts / workerEntry.test.ts / upgrade-6-to-7.test.ts that fail identically on clean HEAD.

Note

persistFlags(matcherCtx) still runs before globals resolve in loadCmsPageInternal, so a sticky flag first recorded by a global section won't reach the deco_segment cookie on that request. Moving the call after the Promise.all fixes it but also drops the cookie whenever section loaders reject, so I left it alone — worth a follow-up if sticky A/B variants land in site.global.

🤖 Generated with Claude Code


Summary by cubic

Fixes four silent matcher-resolution failures that made variants fall back without errors. Pathname templates now match, query-string rules in CMS format evaluate, site globals resolve with matcher context, and site matcher overrides are deterministic.

  • Pathname: implement type: "Template" using URLPattern with a regex fallback for the @decocms/nextjs target; memoize compiled patterns (LRU 100).
  • Query string: accept CMS conditions[] shape alongside { key, value }; AND semantics; support Equals/Not Equals/Includes/Not Includes; handle repeated params; parse with a base URL.
  • Site globals: resolveSiteGlobals(matcherCtx) now forwards context and caches by path + sorted non-tracking query + device + deco_segment cookie (LRU 64); avoid per-URL growth and drop tracking params from the key.
  • Overrides: registerMatcher(key, fn) is the site extension point and always wins; built-ins register with { builtin: true } and never clobber a site-owned key; add unregisterMatcher(key).

Review notes

  • Validate pathname template matching and Node fallback in pathnameMatcher; confirm default cases unchanged.
  • Check queryStringMatcher normalization and negation handling; defaults preserve prior behavior on unknown types.
  • Inspect resolveSiteGlobals(matcherCtx) and siteGlobalsCacheKey for key construction and LRU eviction; confirm cmsRoute passes context at both call sites.
  • Verify the override contract in registerMatcher and registerBuiltinMatchers; tests cover before/after ordering.

Rollout

  • No migration required. Sites overriding matchers should keep using registerMatcher; do not pass { builtin: true }.
  • Expect correct site.global variants per query/device/segment where they previously collapsed; cache impact is bounded by the LRUs.
  • Optional: if you call resolveSiteGlobals directly, pass a matcherCtx to enable variant-aware caching; existing calls work but remain contextless.

Written for commit 07d3d60. Summary will update on new commits.

Review in cubic

All four made a content variant lose silently — no error, no log, HTTP 200,
just the fallback block rendering. Diagnosed in production on
deco-sites/farmrio-storefront, where an ETC-segment topbar lost to FARM's on
every sub-brand PDP.

1. pathname `type: "Template"` was unimplemented. The CMS emits route syntax
   (`/:slug/p`); it fell into the `Includes` default and tested whether the
   pathname contained the literal string — never true. Now compiled via
   URLPattern (workerd, Node >= 23) with a `:param`/`*` regex fallback for the
   Node/RSC target, memoized in a 100-entry LRU.

2. queryString read only the flat `{ key, value }` shape. The CMS emits
   `conditions: [{ param, case: { type, value } }]`, so `key` was always
   undefined and the matcher was unconditionally false. Both shapes are now
   accepted; conditions AND together; Equals / Not Equals / Includes /
   Not Includes are handled, with Equals still the default so unknown types
   behave exactly as before.

3. resolveSiteGlobals() resolved with no matcher context, so no `site.global`
   section could carry a URL-, date- or cookie-dependent variant — which is
   where the multivariate block lives. The context now reaches
   resolvePageSections, and the single cache became a keyed LRU. The key
   includes the sorted, tracking-param-free query string: keyed on path alone,
   /x/p?brand=farm and /x/p?brand=farmetc collide and the first request on a
   cold path picks the variant for both. Device and the deco_segment cookie
   are in the key too, since device and A/B matchers are as common in
   site.global as URL ones and would otherwise fail the same way.

4. The matcher override contract was order-dependent: registerBuiltinMatchers()
   overwrote unconditionally, so a site override registered after
   createSiteSetup() could win in vite dev and be silently lost in the same
   site's production build. Contract is now explicit — registerMatcher(key, fn)
   is the site extension point and always wins. Builtin paths pass
   `{ builtin: true }`, which yields to a site-owned key and overwrites only
   another builtin, so registerBuiltinMatchers() can still replace resolve.ts's
   inline date matcher. Adds unregisterMatcher(key).

Every fix is additive: no decofile that resolves today changes result. The
matcher tests drive the raw CMS shapes (case / conditions), not a normalized
form — the divergence between the two was the bug.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@aka-sacci-ccr
aka-sacci-ccr requested a review from a team August 19, 2026 21:07
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.

1 participant