feat: let a researcher choose and verify how ads carry attribution - #246
Closed
nandanrao wants to merge 18 commits into
Closed
feat: let a researcher choose and verify how ads carry attribution#246nandanrao wants to merge 18 commits into
nandanrao wants to merge 18 commits into
Conversation
Ref mode was reachable only by hand-authoring JSON through the API. The
Messenger, WhatsApp and multi destination forms now offer it, framed by what
each mode does to the researcher's data rather than by the word `ref_mode`:
encoded clean link, stratum comes from the ad-attributions export joined
on ref_token. The default, on every channel.
thick stratum values inline in every response, nothing to join. Offered
on pure-Messenger studies only, since its one cost -- a visible,
editable ref -- lands entirely on the WhatsApp arm.
Thin is not offered at all. It was only ever the WhatsApp/multi default and
the destination-type census found no production population on those channels,
so there is no stored conf to preserve; making the footgun unreachable beats
discouraging it. The `shortcode` literal stays in the model, because that is
what an untouched legacy conf still resolves to.
The load-bearing part is that the model keeps defaulting to legacy while the
UI defaults to encoded. Those only coexist if the UI default is strictly a
new-conf affordance:
- `displayedRefMode` reports what a conf actually does -- absent means thick
on Messenger, thin elsewhere -- and is never written back.
- The encoded default lives only in the two empty-state constructors.
- `GenericList` renders stored data as-is, and the forms spread `...data`,
so a field absent from a conf stays absent through an unrelated edit.
Without that, opening a legacy thick study to fix a typo in its welcome
message would re-serialise ref_mode from absent to "encoded" and silently
rewrite a running study's ads. Messenger.test.tsx exercises exactly that.
Changing a saved destination's mode warns about the real cost, which is the
ad rewrite -- the ref is part of the creative, so reconciliation sees every
ad as drifted -- not data loss, of which there is none.
Also folds Messenger onto the shared additionalMetadata helpers, as its
comment asked once the other branch landed, and teaches Select to render a
disabled option so a conf saved under a retired mode can be shown as what it
is without being selectable.
Plan: planning/ref-mode-dashboard-implementation.md
Design: planning/ref-mode-dashboard-ux.md §3, §4.1-4.4, §5.2
…clared
A stratified ad study attributes its respondents; that is the task, not a
preference. The researcher already named their stratum variables in Variables,
and those names are exactly what the ad's frozen ad_attributions row is keyed
by. Asking for them a second time, in a different form and a different
vocabulary -- location, mapping, key, name -- is the split that produces
silent half-configs: ads carrying a token nothing looks up, discovered hours
later in a swoosh log.
The Data Extraction step now offers to add one lookup conf per declared
variable. Three properties make it safe to press:
- Additive by name. A conf the researcher already wrote wins, whatever its
location -- a study can reasonably read `gender` from a survey answer
rather than from the ad, and a button that destroys work is a button
nobody presses twice.
- One token key across every generated conf, so it cannot create the
disagreement `disagreeing_token_keys` warns about.
- Idempotent, and disabled when it would add nothing, so "already done" is
distinguishable from "nothing to do" without clicking.
Offered on fly sources only, for the same reason the ad lookup mapping is:
Qualtrics and Typeform carry no ad token, so confs generated there would yield
nothing, forever and silently.
Generation writes ordinary editable rows saved through the normal Submit --
nothing is synthesised behind the researcher's back, and the stored conf stays
the whole truth about what swoosh will run. A study wanting to attribute on a
variable it did not stratify on still adds that by hand.
Plan: planning/ref-mode-dashboard-implementation.md §3
Design: planning/ref-mode-dashboard-ux.md §7
`ref_token` is a verification surface, not an input. A researcher does not
choose the codes their ads carry -- vlab mints them deterministically from the
stratum and creative -- so the honest answer to "where do I see the ref codes"
is: after the ads are built, here.
Until now there was no answer at all. The mapping existed only as a CSV
endpoint behind an API key, which in practice meant nobody looked. That
matters because the failure this mechanism guards against is a quiet one: a
study whose write and read sides do not line up raises nothing, it just counts
zero for every stratum while the optimizer reallocates away from strata that
are recruiting perfectly well. A glance at these rows turns "hope the two
halves line up" into "see that they do".
adopt gains GET /{org}/studies/{slug}/ad-attributions, returning the same rows
as the .csv route. Columns come from the CSV's own `metadata_columns` union
rather than a second derivation, so the table on screen cannot show a
different shape from the file downloaded beside it -- which is easy to get
wrong, because the columns are a union across rows in first-seen order and a
study whose conf changed mid-flight has rows frozen under two shapes.
The download is a fetch-and-object-URL rather than a link, because the
endpoint is bearer-authenticated and an <a href> cannot carry the header.
The step is registered after Current Data. Note that `confs` in shared.ts
doubles as the wizard's next-step chain, so this changes where Current Data
advances to.
Plan: planning/ref-mode-dashboard-implementation.md §4
Design: planning/ref-mode-dashboard-ux.md §4.4
The incoherent state surfaced hours later in a swoosh log; now it is refused
at the moment someone causes it, naming both sides. Being told only the half
you are looking at is how someone confidently fixes the wrong one.
Only one of the two possible incoherences is refused, and the asymmetry is the
design rather than an omission:
refused a thin write with no read -- the ads stop carrying the stratum and
nothing looks the token up, so every stratum counts zero and the
optimizer reallocates on empty data, silently, because a respondent
with no token is indistinguishable from an organic arrival.
allowed a read with no thin write -- lookup confs reading a token no ad
emits extract nothing, and swoosh skips a conf that finds nothing.
The respondent is still attributed inline. Nothing is lost.
That asymmetry prescribes the safe order for switching a live study to the
encoded ref: add the lookup confs first, where they lie dormant, then flip the
destination. Refusing both directions would make the flip unperformable in
either order, each conf waiting on the other; refusing this one turns the 422
into the instruction.
Checked only when the counterpart conf exists. The wizard saves Destinations
at step four and Data Extraction at step ten, so an unconditional check would
make an encoded study unsaveable before the researcher could reach the step
that satisfies it. The never-configured case stays with
`thins_its_ref_without_reading_the_mapping`, which warns every reconciliation
run and does not wait for a save.
Fixes a latent bug this feature would otherwise have shipped. Confs are stored
as `model_dump()`, which writes defaults, so a destination saved with
ref_mode="encoded" landed in the database as {"ref_mode": "encoded",
"include_metadata_in_ref": true} -- Messenger defaults that flag True. Reading
it back puts the flag in `model_fields_set`, which is exactly what
`ref_mode_must_not_contradict_the_legacy_flag` looks for, so the conf raised.
The save looked like it worked and the study was then permanently unparseable,
stopping its reconciliation. The model now drops the legacy flag once
ref_mode is stated, which is what the validator's own message asks for.
Splits `find_study_conf` out of `get_study_conf` for callers to whom a missing
conf is an ordinary answer rather than an error.
Plan: planning/ref-mode-dashboard-implementation.md §5
Design: planning/ref-mode-dashboard-ux.md §6
adAttributionOutcome classified by mechanism state -- is there a token, does it
resolve -- when the only thing an error surface should carry is outcome: could
this respondent be attributed. "organic", for an event with no token, was the
fossil of that. It is an expected, correct result, and the "must not alarm"
carve-out routing it in classifyExtractionError was the acknowledgement that it
did not belong on an error page.
It also actively misled. A study switching to the encoded ref keeps its inline
confs alongside the new lookup ones, so both eras attribute -- but every
pre-switch respondent carries no token, and swoosh recomputes a study's whole
history every run. So the branch reported the entire back-catalogue as
"arrived with no ref token and is not attributed to any stratum", every run,
forever, while those respondents sat there attributed by the raw confs. The
second half of that sentence was simply false.
That shape has bitten this repo before: planning/swoosh-config-reconciliation.md
records a warning re-emitted every hourly run over 52,090 historical rows,
never ageing out through the recency predicate, and calls it a permanent false
alarm.
Three outcomes collapse to two, and the thick-era respondent stops being
reported because it was never an error -- not because the code learned to
recognise it:
no token -> nothing. no ad provenance
token, mapping row found -> nothing. attributed
token, no mapping row -> UNMAPPED. vlab minted an ad and lost what
it meant. Always a bug.
Unmapped keeps severityError, and the documented rule that a row found but
missing the requested key is a conf problem rather than unmapped survives
untouched, since resolution is judged on the row and not on the value.
Given up deliberately: the share of respondents arriving with no ad provenance,
which is what would catch a leaked shortcode or an encoded study receiving no
tokens at all. Neither worked here -- a count with first_seen/last_seen cannot
show a jump, and the branch did not alarm. Both are one measurement, a rate,
which needs a denominator an error list does not have. Filed as VIR-32.
Design: planning/ref-mode-dashboard-ux.md §5.3, superseded by this
A dedicated pass after the code, per the repo's documentation-first protocol.
documentation/ad-attributions.md
- "The three-way split" is now wrong and becomes "What gets reported: only
the unmappable", with a section on why organic was removed rather than
scoped -- an expected outcome on an error surface, and for a flipped study
a false one.
- New section on choosing the ref mode: what the two options mean for the
researcher's data, why thick is pure-Messenger-only, why thin is absent
from the form but present in the model, and the three properties that stop
the UI default reaching a legacy conf.
- Records the model_dump round-trip trap, which is the kind of thing that is
invisible until it has already stopped a study's reconciliation.
- "Flipping a live study" gains the finding that a flip costs no data --
the eras partition by presence of the token, so both attribute -- and that
what the dashboard warns about is therefore the ad rewrite.
- "There is no dashboard download button yet" stops being true.
- Documents the save-time refusal and its two deliberate limits.
dashboard/README.md the ref-mode module and the one rule to preserve when
editing those forms; generateLookupConfs; the Ad
Attributions step and the getNextConf side effect.
adopt/README.md the save-time refusal, find_study_conf, and why the
ref_mode serialiser is load-bearing rather than tidiness.
inference/README.md the outcome table, and what removing organic gives up.
planning/ref-mode-dashboard-ux.md marks the three things that changed while
building -- §5.3 resolved by deletion, §6 one-directional, the round-trip bug
-- inline and in the header, since that document is what someone reads first.
✅ Deploy Preview for vlab-dashboard canceled.
|
nandanrao
commented
Aug 23, 2026
| export const REF_MODE_THICK = 'metadata'; | ||
|
|
||
| /** Thin. Never offered; see the module comment. */ | ||
| export const REF_MODE_THIN = 'shortcode'; |
Contributor
Author
There was a problem hiding this comment.
WHy is this here at all? There should be no mention of thin mode, it doesnt exist, remove all mention of it
Contributor
Author
There was a problem hiding this comment.
Agreed, removed entirely. REF_MODE_THIN, its label, its consequence string and the storedMode parameter that surfaced it as a disabled option are all gone, and so is the disabled support I had added to the shared Select — that existed only to render this, so Select.tsx is byte-identical to main again.
I had it defending a case that does not exist: the census found 0 whatsapp and 0 multi, so no conf resolves to thin. It stays in adopt's RefMode literal only because resolved_ref_mode still has to answer for a hypothetical API-authored conf; nothing in the dashboard mentions it now.
Contributor
Author
There was a problem hiding this comment.
Correcting my previous reply on this thread — I was wrong, and the fix is bigger than the dashboard.
You are right that there is no fall-through. A ref either carries the stratum or carries a token that resolves to it; "carry neither" attributes nobody, which nobody chooses. What thin actually described is a study with no stratification, whose ref is short because creative_metadata has nothing to put in it. Thick with nothing to say.
So it is out of adopt too, not just the dashboard:
RefMode = Literal["metadata", "encoded"]resolved_ref_modeisself.ref_mode or "metadata"include_metadata_in_refdeleted from all three destination classesmessenger_refandwhatsapp_reflose their fall-throughs, two branches each
It also deletes the round-trip bug I flagged in the PR description, rather than patching it. That bug existed only because two fields expressed one setting: model_dump() wrote the flag's default alongside an explicit ref_mode, and re-reading tripped the validator that rejected the pair. With one field there is nothing to contradict, so ref_mode_must_not_contradict_the_legacy_flag and the serialiser I had added to work around it are both gone.
Stored confs still hold the retired flag; pydantic ignores unknown keys so they parse unchanged, and there is a test pinning that — a model forbidding extras would stop every legacy destination loading and halt reconciliation.
One behaviour change to flag, since it is the only place this is observable: a WhatsApp or multi destination stating no mode now resolves to inline rather than thin, because the per-channel default went with the mode it selected. No such conf exists and the dashboard writes the mode for every new one, so it is unreachable in practice — and check_whatsapp_refs_are_deliverable now fires for that case, so it fails closed at config time rather than silently putting stratum values in someone's compose box.
adopt 721 passing, dashboard 184, inference clean.
| * points at a specific survey. Routing is not a job the ref does for them, so | ||
| * there is no mode to choose. | ||
| */ | ||
| export const REF_MODE_DESTINATION_TYPES = [MESSENGER, 'whatsapp', 'multi']; |
Contributor
Author
There was a problem hiding this comment.
Why is one a const and others strings?
Contributor
Author
There was a problem hiding this comment.
Inconsistent, fixed. MESSENGER, WHATSAPP and MULTI are all consts now and REF_MODE_DESTINATION_TYPES is built from them.
nandanrao
commented
Aug 23, 2026
| * one it does not have, and it gives the researcher a way to see and change it. | ||
| * Callers render that entry disabled: it is a current value, not a choice. | ||
| */ | ||
| export const refModeOptions = ( |
Contributor
Author
There was a problem hiding this comment.
This is a complex function. All its saying is "if messenger, either, otherwise only encoded"?
Contributor
Author
There was a problem hiding this comment.
Simpler than that, even — the destination type check was redundant, so it is now just:
const modes = isPureMessengerStudy(destinations)
? [REF_MODE_ENCODED, REF_MODE_THICK]
: [REF_MODE_ENCODED];Worth flagging the one thing your paraphrase drops, in case it was deliberate: the condition is not "is this destination Messenger" but "is the whole study Messenger". A study with a Messenger and a WhatsApp destination gets encoded on both, because thick on the Messenger arm would mean that study attributes two different ways and the researcher joins their data differently depending on which arm someone came through.
That is also why the per-destination check went: this destination is one of destinations, so a pure-Messenger study implies it is a Messenger one.
| ): string => { | ||
| if (storedMode) return storedMode; | ||
|
|
||
| return destinationType === MESSENGER ? REF_MODE_THICK : REF_MODE_THIN; |
Contributor
Author
There was a problem hiding this comment.
THIN shouldn't exist. All legacy is Messenger
Contributor
Author
There was a problem hiding this comment.
Right, and this was the load-bearing version of the same mistake. displayedRefMode no longer takes a destination type at all:
export const displayedRefMode = (storedMode: string | undefined): string =>
storedMode || REF_MODE_THICK;Absent means thick, full stop. refModeWouldChange, RefModeField and the three forms all lost the argument with it.
| * generating into an untouched form would otherwise leave an empty row that | ||
| * fails validation on save for a reason nobody could see. | ||
| */ | ||
| export const mergeLookupConfs = ( |
Contributor
Author
There was a problem hiding this comment.
What is this? Is this necessary? What is meaningful and claimed?? What is a generated conf? Generated should only be on the frontend, its just a ddefault, really, that gets overwritten by the user. We should never be storing both in memory anywhere...
There should be a simple mapping:
No user conf: then generate a conf.
Yes a user conf: show that user conf
That's it, no?
Contributor
Author
There was a problem hiding this comment.
You are right, and I built a merge where a default was called for.
Gone: mergeLookupConfs, wouldGenerateAnything, the blank-row filtering, and the button. To answer the questions directly, since they show how far off it was — "meaningful" meant a row the user had started filling in, "claimed" meant a variable already having a conf, and the two existed only so a button could be pressed repeatedly against a list already holding user edits. All of it was machinery for a problem that only existed because I chose a button.
It is now exactly your mapping. InferenceData.tsx already builds initialState per source, so the default lands there:
const dataSourceState = globalData.data_sources?.map(ds => [ds.name, {
extraction_confs: initialExtractionConfs(ds.source, variableNames)
}])and the existing localData ? localData : initialState does the rest — saved confs win, nothing merges, no second copy in memory. SourceExtraction.tsx is back to a three-line diff (it just imports blankExtractionConf for what Add appends).
One consequence worth naming, since it is a real behaviour change rather than only a simplification: a variable added to Variables after Data Extraction was saved no longer gets a row offered for it — the researcher adds it by hand. The button could top it up; a default cannot. I think that is the right trade for the simplicity, but say the word if you want it back in some form.
Also flagging a case I deliberately did not special-case, because it cuts against the same instinct you are pushing on: a legacy thick study that has never configured Data Extraction now gets lookup-conf defaults, which for that study read a token its ads do not emit. I considered gating the default on the study being encoded and decided against — thick studies are legacy by definition and already have their confs saved, so the combination is close to empty in practice, and gating it would be exactly the kind of branch we just deleted four of. Easy to add if you disagree.
…t is real Review on #246. Three things were built to defend cases that do not exist. **Thin is gone from the dashboard entirely**, rather than being offered as an unselectable current value. The destination-type census found no production population on the channels that defaulted to it, so no conf resolves to it -- which made REF_MODE_THIN, the disabled current-value option, its label and consequence strings, and the `storedMode` parameter threaded through refModeOptions all dead code guarding an empty set. The `disabled` support added to the shared Select existed only for that option and is reverted, so Select is byte-identical to main again. Following from the same census: every legacy conf is a Messenger one, so `displayedRefMode` no longer reasons per channel. Absent means thick, full stop, and the function drops its destinationType argument -- as do refModeWouldChange, RefModeField and the three forms that render it. **refModeOptions collapsed** to "pure-Messenger study -> both modes, otherwise encoded only". The `destinationType === MESSENGER` check alongside it was redundant: this destination is one of the destinations being judged, so a pure- Messenger study implies it is a Messenger one, and if it is not then the study is not pure. **Generation is a plain default, not a merge.** The button, mergeLookupConfs, wouldGenerateAnything and the blank-row filtering are gone. What is left is the simple rule: a source with saved confs shows those, a source without shows one lookup conf per declared variable. It is consumed in InferenceData.tsx where that file already builds initialState, so the defaults live in exactly one place and nothing holds a second copy of them. SourceExtraction.tsx is back to a three-line diff against main. Behaviour a researcher sees is unchanged except that the defaults now appear on arrival rather than after pressing a button.
Review on #246. A ref either carries the stratum inline or carries a token that resolves to it. "Carry neither" is a ref that attributes nobody -- not something anyone would choose, and not a third answer to the question. What it actually described is a study with no stratification, whose ref is short because creative_metadata has nothing to put in it. That is thick with nothing to say. So `RefMode` is `Literal["metadata", "encoded"]`, `resolved_ref_mode` is `self.ref_mode or "metadata"`, and `include_metadata_in_ref` -- the boolean that expressed the same setting a second way and could not express "encoded" -- is gone from all three destination classes. messenger_ref and whatsapp_ref lose their fallthroughs and become two branches each. I had argued for keeping the literal on the grounds that resolved_ref_mode still had to answer for an API-authored conf. That was defending an empty set, and it kept a concept alive in the type system that does not exist in the world. **This deletes the round-trip bug rather than patching it.** Confs are stored as model_dump(), which writes defaults, so an encoded Messenger destination was stored as {"ref_mode": "encoded", "include_metadata_in_ref": true} -- and re-reading it tripped ref_mode_must_not_contradict_the_legacy_flag, leaving the study permanently unparseable after a save that returned 201. With one field there is nothing to contradict: the validator and the serialiser added earlier in this PR to work around it are both removed. Stored confs still carry the retired flag. Pydantic ignores unknown keys, so they parse unchanged and resolve to the inline stratum exactly as before -- asserted directly, because a model forbidding extras would stop every legacy destination in the database from loading and halt reconciliation. One behaviour change worth naming: a WhatsApp or multi destination that states no mode now resolves to inline rather than thin, since the per-channel default went with the mode it selected. No such conf exists (0 whatsapp, 0 multi in the census) and the dashboard writes the mode explicitly for every new one, so this is unreachable in practice -- and check_whatsapp_refs_are_deliverable now fires for that case, so it fails closed at config time rather than silently disclosing. Docs follow the code: ad-attributions.md gains "Two modes, not three", multi-destination-ads.md and adopt/README.md drop the flag, and a stale note about Messenger.tsx keeping its own metadata-parsing copy is removed, since that was folded in earlier in this PR.
include_metadata_in_ref was committed on 2026-08-17 and never deployed, so nothing in production ever carried it and the migration-safety framing around it was moot. The test that pinned parsing tolerance keeps its value -- confs are stored as raw JSON and a model forbidding extras would break on any future field removal -- but it is no longer justified by a legacy population that does not exist.
nandanrao
commented
Aug 24, 2026
| return out.getvalue() | ||
|
|
||
|
|
||
| def ad_attributions_table(rows: Sequence[Dict[str, Any]]) -> Dict[str, Any]: |
Contributor
Author
There was a problem hiding this comment.
Repeated logic from ad_attributions_csv -- DRY it up please
| "initial_shortcode": "mnchweek", | ||
| "welcome_message": "Welcome!", | ||
| "button_text": "OK", | ||
| "include_metadata_in_ref": True, |
Contributor
Author
There was a problem hiding this comment.
Didn't we deprecate this? Is this backwards compatibility that we dont need?
We never had that before, no? We're not doing thin refs, so all we need now is ref_mode, thick or encoded, where thick is legacy fallback for none.
| /** | ||
| * The destination types that carry a ref mode at all. | ||
| * | ||
| * Web and app destinations are deliberately absent: neither has an |
Contributor
Author
There was a problem hiding this comment.
I dont follow. web pages also get metadata. So either they get the raw metadat as kv pairs or they get an encoded ref. Why not support both? That simplifies this logic AND it's a feature.
PR #246 has an unread merge review on it, and the context needed to act on that review without relitigating settled decisions is spread across a design doc, a build plan, three app READMEs and the cross-component feature doc. Collects it: where the branch and worktree are, the four load-bearing decisions and why each is the way it is, what two rounds of review already changed (so none of it gets reintroduced), the open items including the unread review itself, and how to verify plus the traps in doing so. The two existing planning docs now point at it.
csv_export: derive the columns and the cells once. ad_attributions_csv and ad_attributions_table each rebuilt the header list and each laid its own values out in that order, which is the one thing this module claims cannot happen -- the file and the table agreeing about columns held only as long as nobody edited one of them. Now `headers` and `cells` are the single order, the CSV writes them positionally and the table zips them. Drop include_metadata_in_ref from the two unknown-key tests. The field was never deployed, so pinning it read as backwards compatibility for something nothing carries. The property they exist for is the models' extra-key policy -- forbidding extras would stop every stored conf loading on any future field removal -- so the key is now fictional on purpose. Same claim corrected in resolved_ref_mode's docstring, which said stored confs still carry it. refMode.ts: correct two comments that no longer describe the code. The module header still credited adopt's RefMode with a third "shortcode" mode, deleted in 4b335b0. And the reason given for web and app having no ref mode was wrong: they do get a ref, the same full make_ref string interpolated into url_template / deeplink_template. What they lack is the read side -- swoosh's isAdTableLookup requires location "metadata", which only fly stamps, so an encoded web ref would mint a token no conf can resolve. Stated as the gap it is, with what has to move first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
"Shortcode-only Messenger refs" named the mode deleted in 4b335b0. The tests underneath it parameterise on ref_mode and exercise encoded; only the header still spoke the old vocabulary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round 2 deleted the "shortcode" mode and include_metadata_in_ref from adopt, and the outcome classifier lost two of its three answers. The docs recorded those changes in the sections about them and left the surrounding prose describing the old design as current. Corrected, in adopt/README.md and documentation/ad-attributions.md: - The section built around include_metadata_in_ref and the shortcode ref now describes ref_mode and its two modes. What the ref emits under "encoded" is r.<token>, not form.<shortcode>, and it routes because getMetadata decodes the token into md.form. - WhatsApp's inline ref is no longer "opt-in and rare": an unstated mode resolves to inline on every channel, so the section says what actually reaches that state and what fails closed on it. Same for check_whatsapp_refs_are_deliverable, which fires on the resolved mode rather than on a deleted flag, and for the claim that the dashboard exposes no ref control -- it exposes RefModeField, encoded-only there. - The half-migration guard counts one thin mode now, not two. - adAttributionOutcome reports the unmappable and nothing else. - dashboard/README.md said adopt resolves an absent mode per channel. It resolves to the inline ref on every channel. "Web and App stay on full refs" is rewritten in both. It gave the reason as these types having no initial_shortcode, so no routing job for the ref -- which is not true: they get the same full make_ref string interpolated into their template. The real constraint is the read side, and naming it makes the section a gap with a next step rather than a closed decision. Passages explicitly recording history keep the old names. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An extraction conf has two axes. Location says WHERE to read a value; mapping says WHAT that value means. They were tangled, and the tangle started in one place: adAttributionOutcome called tokenLookupKey, which walked a source's confs, took the FIRST ad_table_lookup conf's Key, and read User.Metadata[key] directly -- bypassing each conf's own location. That made "the token lives at metadata.<key>" a fact about the source rather than about the conf, and everything else followed from it. isAdTableLookup had to require location "metadata" (its comment said so: "the danger is specifically tokenLookupKey"), getRetrieveFunc had to error loudly on `variable` + lookup, and adopt had to enforce one token key per source. retrieveFromMetadata never needed any of it -- it already read conf.Key per conf. Now: - locationReader(location) reads a raw value. Nothing else. - resolveThroughAdTable(read, attributions) wraps ANY reader and turns what it read into a token, a row, and a stratum variable. - getRetrieveFunc composes the two. - isAdTableLookup is `conf.Mapping == MappingAdTableLookup`. - adAttributionOutcome asks each lookup conf through its own location, and stops at the first unresolved token, so an event still yields at most one outcome. token_location joins token_key in the details. - tokenLookupKey is gone, and with it the requirement that two lookup confs agree about anything. - metadataToken is refToken: it reads a token from wherever it came. A lookup on a survey field now works, which is the read side a web or app destination needs -- its respondent lands on the researcher's own page, so the token returns as a Typeform or Qualtrics field rather than as fly-stamped event metadata. The test that pinned `variable` + lookup as a hard error is replaced by three that pin the capability: a survey-field lookup resolves, an unresolvable one is still reported unmapped, and two lookup confs under one source need not agree on where the token is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
What a ref carries is a property of the ref, not of the channel that carries it. So WebDestination and AppDestination are RefModeDestinations like the rest, and four isinstance whitelists go: ad_ref_token, thins_its_ref_without_reading_the_mapping, and the two create_creative branches that hardcoded make_ref. messenger_ref is dotted_ref, since it was never about Messenger -- it is the dot-pair grammar, which Messenger, multi's Messenger arm, web and app all use. Only WhatsApp's autofill differs, because fly parses it under another grammar. An encoded web or app ref is the bare token, not `r.<payload>`. The packing exists so fly's decoder can recover the shortcode alongside the token; these destinations have no shortcode and nothing decodes their ref, and swoosh compares the extracted value to ref_token directly -- so a packed payload would resolve to nothing. Deleted, all of it coupling between the two sides: - ref_mode_incoherence and the server's 422. It had to be conditional on the counterpart conf existing and one-directional to avoid deadlocking the flip; both were the cost of treating two independent choices as one. _stored_conf and _DESTINATIONS existed only to feed it, and go too, so both endpoints are now a plain create_conf. - disagreeing_token_keys and its warning. It enforced one token key per source, which only mattered while swoosh took the first conf's key for the whole source. - ExtractionConf.a_lookup_reads_the_token_from_metadata. A lookup on a survey field is exactly how a web or app destination is read back. is_ad_table_lookup is now the mapping alone. thins_its_ref_without_reading_the_mapping stays: one unconditional warning, every reconciliation run, covering every destination type. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The forms held a map of the rest of the system, and it was wrong in both directions. Destinations. refModeOptions took the study's entire destination list so that thick could be withheld from anything but a pure-Messenger study -- a form reasoning about other destinations to decide what one destination may do. Gone, with carriesRefMode, REF_MODE_DESTINATION_TYPES, isPureMessengerStudy and the MESSENGER/WHATSAPP/MULTI constants: both modes, every destination, and refModeOptions takes no arguments. Web and App render RefModeField like the rest and their empty states carry a mode. refMode.ts is 214 lines to 126, most of it prose. Data Extraction. There were two form modules, and the only thing that differed was that Qualtrics/Typeform exported an empty mappingOptions so a lookup could not be declared there. Which data carries a token is a property of the platform, not something a form can know -- and a respondent who arrived through a web destination brings one back in the researcher's own survey. So qualtricsExtraction.ts and QualtricsExtraction.tsx are deleted, every source uses one form, and flyExtraction/FlyExtraction are renamed extraction/Extraction, which is what they always were. Also gone: showsMapping, which hid the dropdown on a variable read, and applyChange's reset of mapping to raw when leaving metadata. Both enforced `variable` + ad_table_lookup being invalid; it is now how a web or app destination is read back. displayedRefMode stays exactly as it was. An absent ref_mode still means a conf that predates the field, is still never written back, and Messenger.test.tsx still pins that editing a legacy study's welcome message does not flip its ads. generateLookupConfs still defaults lookups on fly sources only, but for the honest reason: the default has to guess the token's key, and vt is right only for fly. A convenience, not a claim about which source can carry a token. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two independent choices, and the docs said they were one. documentation/ad-attributions.md - The `mapping` concept: location and mapping are independent axes. A lookup is valid on either location, with the web/Typeform example. Why `variable` + lookup used to be rejected, and that the reason was self-inflicted rather than structural. - "The token's metadata key is conf-declared" is "the token's location is conf-declared", and nothing is source-wide. - The save-time 422 section becomes "the two sides save independently", naming what the 422's two exemptions were buying. - Config-time checks: one, not two. - The form: one module for every source, mapping always offered, no reset on a location change. - "Web and App stay on full refs" becomes "carry a mode like everything else", with why an encoded web ref is the bare token. - Read path, `refToken`, and the Where-things-live paths. adopt/README.md — the same three corrections, plus dotted_ref and the half-migration guard now covering every destination type. inference/README.md — a new section on location/mapping independence that says where the tangle started (adAttributionOutcome -> tokenLookupKey) and names the four things that went with it. dashboard/README.md — one extraction module, refModeOptions taking no arguments and why that signature is the claim, and the fly-only default restated as a guess about the token's key rather than a claim about which source can carry one. Passages recording history keep the old names. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Preserved during worktree cleanup. This is the specification PR #247 was built from; the implementation on this branch is its sibling. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Closing: superseded by #247. This implementation was deliberately rebuilt from scratch rather than merged. The The one behavioural gap this branch covered and #247 did not — Three design decisions here were deliberately overridden by the specification Branch tip recorded as 7c17b96 if anything is |
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
1 participant
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.
Builds the researcher-facing half of ad attribution. The write side (
ref_mode,encoded_ref,mint_ref_token) and the read side (ad_table_lookup,AdAttributions) already shipped — butref_modewas reachable only byhand-authoring JSON through the API, the two halves of one decision lived in two
unrelated forms, and the mapping the whole mechanism produces had no surface at
all.
Design:
planning/ref-mode-dashboard-ux.md. Build order and the decisions takenduring it:
planning/ref-mode-dashboard-implementation.md.What a researcher sees
Two choices on a destination, framed by consequence. The word
ref_modenever appears. Encoded ("clean link, stratum comes from the ad-attributions
export joined on
ref_token") is the default on every channel; thick ("stratumvalues inline, nothing to join") is offered on pure-Messenger studies only,
since its one cost — a visible, editable ref — lands entirely on the WhatsApp
arm. Thin is not offered at all: the destination-type census found no production
population on the channels that defaulted to it.
The read side generates itself. The Data Extraction step adds one
ad_table_lookupconf per variable already declared in Variables. Additive byname, idempotent, one token key throughout, disabled when it would add nothing.
Generated confs land as ordinary editable rows saved through the normal Submit.
A new Ad Attributions step shows each ad, its stratum and its
ref_token,with the CSV download beside it. Columns come from adopt's existing
metadata_columns, so the table cannot show a shape the download does not have.An incoherent pair is refused at save, with a 422 naming both sides.
Two things worth reviewing carefully
1. A latent bug that would have broken every study using this feature.
Confs are stored as
model_dump(), which writes defaults. So the moment the UIbegan sending
ref_mode: "encoded", a Messenger destination was stored as{"ref_mode": "encoded", "include_metadata_in_ref": true}— Messenger defaultsthat flag
True. Re-reading it puts the flag inmodel_fields_set, which isexactly what
ref_mode_must_not_contradict_the_legacy_flagrejects. The savereturned 201 and the study was then permanently unparseable, stopping its
reconciliation.
RefModeDestinationnow omits the legacy flag onserialisation once
ref_modeis stated — what that validator's own messagetells a human to do. Caught only by the endpoint tests; nothing else
round-tripped a conf through storage.
2. The organic branch is deleted, not scoped. The design filed monitoring
noise as a residual to suppress. It is a category error instead:
adAttributionOutcomeclassified by mechanism state — is there a token, doesit resolve — when the only thing an error surface should carry is outcome:
could this respondent be attributed.
ad=organicis an expected, correctresult, and the "must not alarm" carve-out it needed in
classifyExtractionErrorwas the admission that it did not belong there.It was also false for one whole class of study. A study switching to the encoded
ref keeps its inline confs alongside the new lookup ones, so both eras
attribute — but every pre-switch respondent carries no token, and swoosh
recomputes all history every run. The branch therefore reported the entire
back-catalogue as "arrived with no ref token and is not attributed to any
stratum", every run, forever, while those respondents sat there attributed.
planning/swoosh-config-reconciliation.mdrecords the identical shape over52,090 rows and calls it a permanent false alarm.
Given up deliberately: the share of arrivals with no ad provenance, which is
what would catch a leaked shortcode or an encoded study receiving no tokens at
all. Neither worked here — a count with
first_seen/last_seencannot show ajump, and the branch did not alarm. Both are a rate, needing a denominator an
error list has not got. Filed as VIR-32.
Design decisions that departed from the plan
read with no thin write is allowed, because those confs extract nothing and
swoosh skips them. That asymmetry is what makes switching a live study
possible — add the lookup confs first, where they lie dormant, then flip the
destination. Refusing both directions would deadlock it, each conf waiting on
the other, so the refusal becomes the instruction for the safe order.
Destinations at step four of ten; an unconditional check would make an encoded
study unsaveable before the researcher could reach Data Extraction.
thins_its_ref_without_reading_the_mappingstill covers the study that nevercomes back.
"shortcode"stays in theRefModeliteral, removed from the UI only.It is what an untouched legacy conf still resolves to; removing it would force
the stored-JSON rewrite the design exists to avoid.
The migration-safety property
The model keeps defaulting to legacy while the UI defaults to encoded, which
only works if the UI default is strictly a new-conf affordance. Three
properties hold it up:
displayedRefModereports what a conf actually does andis never written back; the default lives only in the two empty-state
constructors; and the forms spread
...data, so an absent field survives anunrelated edit. Break any one and editing a legacy study's welcome message
silently flips its ads.
Messenger.test.tsxexercises exactly that scenario.Testing
tsc --noEmitcleango vetcleanNote for reviewers
Registering the Ad Attributions step changes
shared.ts'sconfsarray, whichdoubles as the wizard's next-step chain — so Current Data now advances there.
Intended, but user-visible.
🤖 Generated with Claude Code