fix(matchers): resolve four silent matcher-resolution defects - #481
Open
aka-sacci-ccr wants to merge 1 commit into
Open
fix(matchers): resolve four silent matcher-resolution defects#481aka-sacci-ccr wants to merge 1 commit into
aka-sacci-ccr wants to merge 1 commit into
Conversation
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.tswhose ETC variant is guarded by an ETC Segment block: pathnameIncludes/farm-etcOR (pathnameTemplate/:slug/pAND queryStringbrand=farmetc). The/farm-etccategory page worked (first branch); the PDPs never did (second branch).1.
pathnameMatcherdidn't implementtype: "Template"The switch covered
Equals,Not Includes,Starts With,Includes, and the default didpath.includes(case.pathname). The CMS also emitstype: "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 agetURLPatternwith an LRU precisely because compiling per request cost ~6% of CPU); invalid patterns memoize asnull, andisSafePatternstill gates compilation.URLPatternis a global in workerd and Node ≥ 23, but not on the Node/RSC target@decocms/nextjsbuilds against — so there's a:param→segment /*→anything regex compiler behind it. Returningfalsethere would have reintroduced exactly this bug on that binding. The fallback is covered by a test that deletes the global.2.
queryStringMatcherdidn't understand the CMS formatIt read
rule.key ?? rule.paramandrule.valueat the top of the rule, returningfalsewhen it found no key. The CMS emitsconditions: [{ case: { type: "Equals", value: "farmetc" }, param: "brand" }]— an array, with the param name besidecaseand the value inside it. Sokeywas alwaysundefinedand the matcher was unconditionally false.Both shapes are accepted now.
conditions[]entries AND together;Equals,Not Equals,IncludesandNot Includesare handled, withEqualsstill the default so an unknowncase.typebehaves 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-relativectx.urldoesn't fail closed.3.
resolveSiteGlobals()resolved with no matcher contextThe signature took no context, so no
site.globalsection 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 reachesresolvePageSections, wired at bothcmsRoute.tscall sites.The single cache became a keyed LRU (64 entries). The key includes the query string: keyed on path alone,
/x/p?brand=farmand/x/p?brand=farmetccollide and the first request on a cold path decides both — reproduced, requesting?brand=farmetcfirst 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 existingregisterTrackingParamsregistry, exposed as a newisTrackingParam) soutm_*doesn't fragment the cache.I went past the query string you specified: device and the
deco_segmentcookie are in the key too. Device and A/B matchers are as common insite.globalas 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._builtinMatchersRegisteredatresolve.tsmodule load, and one unguarded —registerBuiltinMatchers()insidecreateSiteSetup, overwriting unconditionally. A site registering an override aftercreateSiteSetup()might or might not win depending on module evaluation order in the bundle; observed working invite devand 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 aftercreateSiteSetup(). Both builtin paths pass{ builtin: true }, which skips a key a site already owns and overwrites only another builtin — soregisterBuiltinMatchers()can still replaceresolve.ts's inline inclusivedatematcher with the strict one, but can't touch a site's. Ownership lives in aglobalThisset besidecustomMatchers, so it holds across module instances. AddsunregisterMatcher(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 (IncludesOR (TemplateANDqueryString)), 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:
bun run typecheckclean onblocks,tanstack,nextjs. Full suite: 2394 tests, the only failures are 4–5 pre-existing ones indraftShell.test.ts/workerEntry.test.ts/upgrade-6-to-7.test.tsthat fail identically on clean HEAD.Note
persistFlags(matcherCtx)still runs before globals resolve inloadCmsPageInternal, so a sticky flag first recorded by a global section won't reach thedeco_segmentcookie on that request. Moving the call after thePromise.allfixes 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 insite.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.
type: "Template"usingURLPatternwith a regex fallback for the@decocms/nextjstarget; memoize compiled patterns (LRU 100).conditions[]shape alongside{ key, value }; AND semantics; support Equals/Not Equals/Includes/Not Includes; handle repeated params; parse with a base URL.resolveSiteGlobals(matcherCtx)now forwards context and caches by path + sorted non-tracking query + device +deco_segmentcookie (LRU 64); avoid per-URL growth and drop tracking params from the key.registerMatcher(key, fn)is the site extension point and always wins; built-ins register with{ builtin: true }and never clobber a site-owned key; addunregisterMatcher(key).Review notes
pathnameMatcher; confirm default cases unchanged.queryStringMatchernormalization and negation handling; defaults preserve prior behavior on unknown types.resolveSiteGlobals(matcherCtx)andsiteGlobalsCacheKeyfor key construction and LRU eviction; confirmcmsRoutepasses context at both call sites.registerMatcherandregisterBuiltinMatchers; tests cover before/after ordering.Rollout
registerMatcher; do not pass{ builtin: true }.site.globalvariants per query/device/segment where they previously collapsed; cache impact is bounded by the LRUs.resolveSiteGlobalsdirectly, pass amatcherCtxto enable variant-aware caching; existing calls work but remain contextless.Written for commit 07d3d60. Summary will update on new commits.