🎉 Metadata Diff: review the metadata texts a branch changes - #6753
Open
paarriagadap wants to merge 49 commits into
Open
🎉 Metadata Diff: review the metadata texts a branch changes#6753paarriagadap wants to merge 49 commits into
paarriagadap wants to merge 49 commits into
Conversation
A file that sits in a step folder without naming a step of its own resolved to a path in no DAG, and the expansion then returned nothing at all: - get_all_changed_catalog_paths returns an empty list for a branch that edits only a shared.meta.yml — verified: zero paths, where a normal step's .meta.yml gives 118. It returns an empty list for a package step's own files too, verified for both .../vdem/vdem.meta.yml and .../vdem/__init__.py, which implement an active step holding 200 kB of reader-facing text. Three callers depend on this — chart-diff filters its metadata changes by it, etl diff --changed picks its datasets from it, and etl run --modified picks what to build — so such an edit was invisible to all three, and --modified selected nothing to rebuild. - A nested file now resolves to the nearest ancestor folder that names a step, and falls back to crediting the steps beside it when none does. Adds get_directly_changed_export_uris for the export side, which has the same problem: a recipe's helper module (explorers/un/latest/utils.py) is not a step, and a retired recipe still on disk answers to a live explorer's name. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A wizard page that compares the *rendered* user-facing texts — title, subtitle, footnote, description_short and WYSK / description_key — between a staging server and the baseline every other diff uses. Chart Diff compares chart configs, so an edit authored in a garden step (Jinja templates and shared definitions included) changes what readers see while leaving the config byte-identical: this is the diff that catches those. Four sections. Blast radius leads: everywhere the branch's edits land, by edit (edit → the texts it renders into → the pages each lands on), by affected page, or on one MDim's dimension grid. Then Charts, MDims and Explorers, each listing its own surface's changes with a Reviewed tick bound to the exact text — edit that text again and the tick goes stale. Nothing here syncs on merge; metadata ships through ETL, so the ticks are progress tracking and the badges count them down. The care is in what it does not claim: - A change is this branch's only if the dataset is in its git scope AND this server rebuilt it. Scope alone put 118 datasets in play where 9 had been rebuilt. - Each change is compared against master's own staging server too, so text master edited that the baseline has not rebuilt is not reported as ours. - A dataset this server is serving from a stale build is named, with the etlr command to fix it, because every difference involving it is inverted. - Reach counts published charts; drafts and unpublished MDims are listed apart. - Counts are distinct texts, deduped across surfaces, and the headline separates the one edit somebody authored from the several texts it renders into. Cold load is 9.6s against a 5,182-chart server, and the reading is held for 30 minutes with a Re-read button in the section bar. 92 unit tests on the pure logic, and the three sections are also run headlessly against a staging server with Streamlit's AppTest. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A metadata-diff section beside chart-diff and data-diff, linking to the tool:
metadata-diff: ✏️
• Charts: 15 (from 45 indicators)
• MDims: 1
• Unpublished MDims changed: 1 — no reader sees them yet
• Fields: WYSK (11)
• 13 further MDim/explorer difference(s) are baseline lag, not this branch
Counts come from the same discovery pass the page uses, so the comment and the
page cannot disagree. An unpublished MDim is reported on its own line rather than
in the count: it shows readers nothing today, and it is exactly the text that goes
live when the PR publishing it merges.
No GitHub check run — metadata gates nothing on merge, so a red check would be
noise. 🚧 outranks every other status when a dataset on the server is stale,
because the rebuild has to happen before any of the numbers mean anything.
Needs `--services metadata-diff` in the staging pipeline (owid/ops#621) before the
section appears; cli.py only warns on unknown services, so neither side breaks
while they are out of step.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
Quick links (staging server):
Login: chart-diff: ✅No charts for review.data-diff: ❌ 3 changed · 2 identical — full report~ garden/growth/2025-01-16/gdppc_vs_living_conditions
~ gdppc_vs_living_conditions.mean_income (changed metadata)
~ gdppc_vs_living_conditions.median_income (changed metadata)
~ garden/lis/2026-06-12/luxembourg_income_study
~ poverty.headcount (changed metadata)
~ poverty.headcount_ratio (changed metadata)
~ poverty.total_shortfall (changed metadata)
~ incomes.avg (changed metadata)
~ incomes.mean (changed metadata)
~ incomes.median (changed metadata)
~ incomes.thr (changed metadata)
~ garden/wb/2026-06-26/world_bank_pip
~ poverty.avg_shortfall (changed metadata)
~ poverty.headcount (changed metadata)
~ poverty.headcount_above (changed metadata)
~ poverty.headcount_between (changed metadata)
~ poverty.headcount_ratio (changed metadata)
~ poverty.headcount_ratio_above (changed metadata)
~ poverty.headcount_ratio_between (changed metadata)
~ poverty.income_gap_ratio (changed metadata)
~ poverty.poverty_gap_index (changed metadata)
~ poverty.poverty_severity (changed metadata)
~ poverty.total_shortfall (changed metadata)
~ poverty.watts (changed metadata)
~ other_indicators.pg (changed metadata)
~ other_indicators.spl (changed metadata)
~ other_indicators.spr (changed metadata)
~ incomes.avg (changed metadata)
~ incomes.mean (changed metadata)
~ incomes.median (changed metadata)
~ incomes.thr (changed metadata)
~ incomes.top1_avg (changed metadata)
~ incomes.top1_thr (changed metadata)
~ percentiles.avg (changed metadata)
~ percentiles.thr (changed metadata)= 2 compared dataset(s) turned out identical; details in the full report Automatically updated datasets matching excess_mortality|covid|fluid|flunet|country_profile|garden/ihme_gbd/2019/gbd_risk are not included. Run locally with Edited: 2026-08-26 15:36:24 UTC |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40ad622087
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…FORE MERGE
Five edits in five different places, chosen to hit different paths through the
tool rather than to be five of the same thing:
1. definitions.description_key_ppp shared definition, WYSK — one authored
edit rendering into many texts
2. definitions.ppp_adjustment_subtitle shared definition reaching chart
subtitles (chart config, not indicator)
3. definitions.ppp_adjustment_note shared definition reaching footnotes
4. headcount_ratio title_public per-variable, inside a Jinja branch
5. headcount_ratio per-variable literal, and a data-page-
description_from_producer only field, so it splits charts with a
data page from charts that keep the text
behind "Learn more about this data"
This commit exists so a staging server has something for the tool to find. It
must not merge: it is the tip of the branch on purpose, so `git revert` takes it
out in one step.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A `presentation.grapher_config.subtitle` or `.note` edit in a garden step is resolved into each chart's own config and never touches the `variables` row. The Charts section built its comparison with chart_config=None, so those edits were not under-reported — they were invisible, while the MDim and explorer sections diffed chart text all along. Measured on this branch's staging server, editing two shared definitions moved the subtitle of 15 published charts and the footnote of 14, and `variables` matched the new wording zero times. Each chart is compared as a view keyed by its slug, so group_changes collapses charts that say the same thing — what a shared definitions.* edit produces — and the reach model, the badges, the PR brief and Blast radius need no chart-specific branch. Restricted by the same two signals as the indicator layer: the chart must render a dataset in the branch's git scope that this server rebuilt, or a chart master rebuilt would read as ours. A chart absent from the baseline is skipped rather than reported as a text change: that is a new chart, which is chart-diff's subject. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…he baseline "Other differences" pooled two unlike reasons a difference is not attributable, and captioned both "its recipe is untouched — see Chart Diff", which reads as master having done it: - the MDim's view configs differ because master rebuilt it (genuinely not ours); - the view's first y-indicator is a *different variant* here than in the baseline (`survey_comparability_4` against `_6` on this branch), so its texts differ for two reasons at once and no rewording can be attributed to the branch. Measured on this branch's staging server, the second case is the majority: poverty_pip splits 18 repointed against 4 lagging, incomes_pip 21 against 16. So the old caption was wrong about most of what it described. `ViewDiff.indicator_replaced` records it where `_same_indicator` already decided it, the group carries it, and the MDim card gives it its own line. A version bump is still the same indicator and is not marked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e charts beside it The dimension tree goes first and is the default — except on a branch that changes no MDim, where it would open on "no dimension grid to draw" while the real changes sat one click away; the default then falls back to by-edit. Both options keep their names, so existing ?blast-group= links still resolve. The tree view showed one MDim's views and nothing else, so the charts the same edits reach — which are never MDim views — appeared nowhere in it. They are listed above the grid now, split by whether the grid accounts for them. That split is the point. The grid's "↗ N charts" badge is per view: the charts using the indicator of a view whose change is in the shared indicator layer. So it covers part of the reach and is silent about the rest, and measuring it showed how much: with incomes_pip selected the badges cover 19 of 66 affected charts, with poverty_pip 43 of 66. Labelling the whole list "outside these MDims" (as the first attempt did) was wrong in both directions. The list sits above the component because that component resizes its own iframe and overlaps whatever follows it, and is computed after the picker because the split depends on which MDim is selected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The grid could only ever mention a chart indirectly, through a view's "↗ N charts" badge — the charts sharing that view's indicator — and was silent about the rest: on this branch the badges cover 43 of 66 affected charts for one MDim and 19 of 66 for the other. The remainder lived in a separate expander above the grid, in a different visual language, half-repeating what the badges said. They are now a root sibling of the grid: "📈 Charts", collapsed, grouped by how a reader meets the text (data page, sources drawer, unpublished draft). A leaf links to the chart on this staging server and shows the word-level diff on hover, and a leaf the grid already accounts for is marked with the same ↗ rather than hidden, so the branch's total and the badges' totals reconcile by eye instead of reading as two different answers. Chart previews extend the same PREVIEWS array the view leaves index into, so the component's hover and filter code needed no branch of its own — a chart leaf is a leaf with a higher index. The expander survives only where there is no grid to hang charts off: a branch that changes no MDim. Verified live: Charts 75 = 17 data pages + 49 via "Learn more" + 9 drafts, 19 of them marked, hover showing the diff. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The dimension-tree view asked which MDim to draw. Nobody reviewing a branch wants one of them: the picker existed because two components cannot be stacked — each sizes its own iframe and overlaps whatever Streamlit renders after it — not because one at a time was right. So it is one component now. render_multi_tree_html takes a section per MDim, each a collapsible root branch cut by its own dimensions and labelled with its own changed/total (an unpublished MDim says so on the branch), leaf indices shifted into one combined view list so the hover previews, badges and show-all filter work unchanged. The charts branch sits beside them as before, its badged set now the union across every MDim drawn. A single-MDim call keeps its old flat shape, and render_tree_html stays as that wrapper. A "Jump to:" index sits in the toolbar — one entry per MDim plus the charts — living inside the component, since only script in the iframe can scroll its sections into view and expand a collapsed target before jumping. It only renders when there is more than one place to go. Cost is one view diff per MDim, so the grid stops at MAX_TREE_MDIMS = 6 and names what it dropped rather than drawing forever or truncating silently. Verified live: 87 of 101 views changed across 2 MDims, no picker, three index entries, and the Charts entry expanding its branch on click. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…the jump land
Three fixes from using the multi-MDim grid:
- Every branch and leaf pill carries the name of the dimension it belongs to as a
muted prefix ("Poverty line · $8.30 a day", "Breaks in data · Don't show"),
replacing the dimension order that was crammed into the section pill. A header
row over the columns is not available: each branch's boxes have their own
widths, so the columns' x positions vary per branch and a fixed header could
not stay aligned with them.
- Sections get a rule and margin between them; they ran together before.
- The index links scrolled nothing. The component's iframe is sized to its
content, so the page scrolls rather than the frame's own document, and
scrollIntoView inside the frame is a no-op. The frame is same-origin, so the
handler scrolls the parent's main container to the target's position (clearing
the 130px of app header plus sticky bar), lining the target up inside the
tree's internal scroll first where that is active.
Verified live: 184 pills labelled across 5 dimensions, 3 separated sections, and
the Charts jump moving the page 0 -> 1557px with the section landing at 130px.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three usability fixes to the multi-MDim grid: - The jump index is a panel — "Jump to a section", one row per section with its changed/total on the right — instead of an inline "a · b · c" line that vanished into the toolbar text around it. - Column titles sit at the top of each section (MDIM · INDICATOR · GROUP · PERIOD · BREAKS IN DATA), pointing at the pill columns. What makes them possible is making the columns real: inside an MDim section every pill takes a fixed border-box width (long names wrap), so each depth starts at depth × pitch and a title lands exactly over it — measured 0px misalignment across all five columns. This replaces the per-pill dimension prefix, which was the wrong reading of the request and noisy besides; the charts branch keeps auto-width pills, having no dimensions to title. The section pill's dimension-order note is gone with it. - One vertical scrollbar. Three could nest: the page, the component iframe (scrolling=True), and past 4000px the tree's own inner scrollbar. The frame now always grows to its content, the inner cap is gone (the constant is a first-paint estimate now, and says so), and the iframe never scrolls; wide trees still scroll sideways, which is the other axis and beats clipping. Measured 0px of inner vertical scroll on both the tree and the iframe. Also repairs the index-entry annotation, whose 3-tuple rename was lost when an edit script failed before its single write while the appends went through. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…titles
Each MDim section opens with the MDim's own human-readable title from its config
("Incomes across the distribution", "Poverty indicators · unpublished"), its
changed/total beside it, the catalogPath as a monospace subtitle, and its own
"Show all views" checkbox + legend. The filter is genuinely per section — its
checkbox hides only that section's unchanged views, defaulting to hidden only
where the section has changes — verified by toggling one and watching 15 pills
appear in it and none in its neighbour.
The catalogPath no longer occupies the tree's first column: the root pill is
gone, sections start at the first dimension, and the MDIM column title went with
it. The component's top is down to the summary (x of y views changed across z
MDims) and the Jump panel, which places both directly under the Dimension tree /
By edit / By surface toggle; the index rows use the human titles too.
The global checkbox and legend are gone with their JS, and the filter CSS is
scoped to sections. mdim_view_diffs returns (title, dimensions, view_diffs) now,
with the title extracted from the config and falling back to the catalogPath.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rts beside it
The grid reads as two top-level hierarchies now:
MDims (2 MDims)
Incomes across the distribution 51/57 views changed
Poverty indicators · unpublished 36/44 views changed
Charts (75 affected)
Data pages · Via Learn more about this data · Unpublished drafts
The MDim sections nest under an "MDims" header, indented behind a left rule, and
Charts gets the same header treatment instead of its root pill — the pill was
compensating for sections having no headers, and its groups are the section's top
level now, collapsed by default to keep the closed footprint the pill provided.
The jump index mirrors it (an unclickable "MDims" group label with its rows
indented, Charts at the top level), and expands a section's top-level collapsed
groups so it never lands on closed boxes.
Verified live: both super-titles render, the index carries the group label with
two indented rows, the three chart groups start collapsed and the jump opens them.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…by-edit diff Five changes from a round of using the grid: - The page opens on Blast radius. How far a branch's edits go is the first question; the three review sections are where you go once you know. - "By surface" is gone. The grid and the by-edit tree answer the same questions better, and a third reading of the same data was one more thing to explain. reach_by_surface stays in discovery: the headline counts distinct pages with it, which is what stopped a page rendering two texts counting twice. A stale ?blast-group=surface link is sanitized before the widget reads it — dropping an option from a url_persist'ed control is exactly what made the section switcher raise on every load earlier this week. - The by-edit card states the edit as a diff: a rewording is one line, old struck through and new highlighted, instead of an "added" statement and a "removed" statement to pair up by eye. Below it, the same edit in context — windowed on the change inside the first text carrying it, labelled "in context, in the first of N texts" rather than passed off as the whole story. - The jump index bullets and indents its MDim rows under the "MDims" label. - The toolbar's "87 of 101 views changed across 2 MDims" is gone: the index rows say it per section. The total moved onto the MDims header, where it is said once. - Each MDim name folds: click the title to collapse that MDim, same caret idiom as the branches inside it, with the title and catalogPath staying visible. The jump unfolds its target, or an index click would land on a closed section. Verified live: two toggle options, a stale surface link landing on the grid with no exception, four context lines with their labels, bulleted index rows, no standalone summary, and folding one MDim while its neighbour stays open. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The highlight rules were scoped to `.mdd-text`, the bordered before/after box, so an <ins>/<del> inside an inline `.mdd-diff` line matched nothing and fell back to the browser's underline and strikethrough. The by-edit cards' diffs were in the markup and invisible as diffs — which is what "I don't clearly see the diff" was: not a missing diff, an unstyled one. Both containers are covered now. `.mdd-text` keeps its box; `.mdd-diff` gets only the colours, since it is a line inside prose. Verified by computed style: every ins/del in a by-edit card reports rgb(211, 249, 216) / the red equivalent rather than a transparent background. This predates today's cards — the first Blast radius commit used `.mdd-diff`, so those diffs have never been highlighted on the Streamlit side. The component's tooltips were never affected: the iframe carries its own copy of the rules. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…t blocks
"Text 1 of 12" made the reader hold a distinction the card had just told them not
to care about — one edit, and the texts are its renderings. The detail now answers
the questions directly, aggregated across the edit's texts and deduped:
30 views in 1 MDim
Incomes across the distribution — _1 · day · avg · no_spells, …
16 views in 1 MDim · unpublished MDims
Poverty indicators — headcount · _100 · no_spells, …
63 charts a reader can open
On their data page (12) — …
Via Learn more about this data (51) — …
Unpublished drafts (8), which no reader can open — …
Named as the dimension grid names them, and every view and chart is a link: a
view to itself on this staging server, a chart to its page, a draft to the admin
editor. The reach model carries what a link needs now — the MDim's title, its
grapher slug, and each affected view's dimensions rather than only a count.
`mdim_list` picks up the title, and sorts in pandas. Touching `config` at all,
even to extract the title scalar with `->>`, puts the row's JSON in MySQL's sort
set, and `order by updatedAt desc` over that fails outright with "Out of sort
memory" — measured against a real server, and swallowed by the MDim block's
try/except into a page warning, which is why the first attempt silently rendered
charts and no MDims.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ey can be seen
The by-edit expander now counts and names instead of enumerating. One edit reaches
a hundred views; the dimension grid already lists every one of them, so repeating
that list here buried the two numbers the expander exists to give. Published first
and unpublished kept apart, because "can a reader see this today" is what decides
how much care an edit needs:
A reader can see this
30 views in the MDim Incomes across the distribution
63 charts — 12 on their data page, 51 via Learn more about this data
Not published, so no reader can see it yet
16 views in the MDim Poverty indicators
8 draft charts
The surface is named on every line: "30 views in Incomes across the distribution"
left the reader to infer that the italicised thing was an MDim, and beside a
bullet counting charts that inference is the one not to leave to them.
Where links do remain — the grid's leaves, the chart lists — an unpublished thing
now points where it can actually be seen. Two dead ends, both verified against a
staging server before and after:
- a draft MDim's views pointed at `/grapher/<slug>`, which 404s until the MDim is
published even though the slug is already set. They now open the admin preview,
which renders the page and applies the dimensions from the query string.
- every draft-chart link read `{admin_site}/admin/charts/<id>/edit`, doubling a
segment `admin_site` already ends in. It answered 200, which is why this stood:
the admin serves its shell for any `/admin/**` path, and the doubled one renders
the nav chrome with no editor in it. Both sites now use `chart_admin_site`.
`view_url` was already in the tool, dead, doing exactly the published/unpublished
split by hand-rolled duplicates of it. Now used, and covered.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The caption spent four clauses and a semicolon chain explaining a header the reader can already see, and closed by pointing at sections that are buttons a few pixels above it. What it needed to say is why one edit, several texts and many pages are three different counts: Why three numbers: one edit can render into several texts, and each text lands on many pages. A sentence added to a shared `definitions.*` entry is one thing to judge, even when it turns up in eighty places. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`definitions.*` is a filename fragment. It means something to whoever wrote the garden step and nothing to anyone else opening Blast radius, so the caption now says what it is: A sentence added to a shared definition across multiple indicators is one thing to judge, even when it turns up in eighty places. Left alone deliberately: the strings that tell an author what to go and edit — the Charts section's guidance and the PR brief's checklist — still name `definitions.*` and `shared.meta.yml`. Those are instructions for someone about to open the YAML, and the key is the actionable half. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three things a reviewer has to read, all of which were hard to read or wrong. **The line explaining why a card groups what it groups** was set in caption grey and explained itself in YAML. It now says what is true, in the theme's own text colour: 🔗 Grouped because these 4 indicators (headcount, headcount_above, headcount_between, headcount_ratio) have exactly the same WYSK — the whole text, word for word. It is written once and shared between them, so this is one edit. `definitions.*`, `shared.meta.yml` and `tables.<t>.variables.<v>` are gone from it: they named the machinery instead of the fact, and pointing at a variable key was actively wrong for a dimensional indicator, whose own field holds a template reference. The exact field to edit is the PR brief's job, and it still does it. The section's scope note and its new-indicator warning read dark for the same reason — one states a coverage gap, and a gap in caption grey is a gap nobody sees. `st_note` is now the one helper for all of them. **Empty sections go grey instead of disappearing.** Dropping them was the first cut and it threw away a finding: "Explorers (0)" is the tool reporting that it looked, and a bar that silently shrinks cannot be told apart from a tool that never checked. Greyed keeps the zero on screen and still stops anyone opening a page with nothing on it — verified: opacity .4, pointer-events none, the click lands nowhere and the section does not change. A zero is only greyed when it is a finding rather than a silence. Any warning at all keeps every section live, and so does an unresolved MDim count or a set of new indicators — a surface whose lookup failed also counts zero, which is exactly how a broken MDim query looked an hour ago. **The scope note drew the line in the wrong place.** "A chart's own title, subtitle and footnote live in the chart itself" stopped being true in this PR: a garden step can set chart FAUST through `presentation.grapher_config`, that text reaches readers, and this section now compares it — the header has been counting it all along. The line is written-in-ETL vs typed-into-the-chart, not indicator vs chart. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…orth reading Blast radius has been counting explorer views and showing them nowhere. `reach_by_surface` files an explorer view as a page a reader can reach, so the header's "on N pages" included them — while the by-edit detail listed only MDim views and charts, and the grid drew only charts beside the MDims. Two numbers on one screen that could not be reconciled by eye, which is the exact failure this section exists to catch. Explorers now appear in both readings: - by edit: "12 views in the explorer energy", in the reader-visible block, with the caveat that decides what it means — for a WYSK edit the line adds "but not in the view itself, which has no data page". - the grid: an Explorers branch beside Charts, one leaf per explorer carrying its view count, linked to the explorer. - and in both no-grid fallbacks, which listed charts and dropped explorers. Only when there are any. An "Explorers — 0 affected" heading is a heading about nothing, so an empty branch is not drawn at all. The tree's charts branch was already a generic "labelled groups of linked leaves" renderer, so it becomes one of a list of branches rather than being copied: `chart_branch` → `branches`, groups carry `leaves`. Verified the charts branch still draws unchanged — 75 leaves, index row, anchor. The empty state said the sentence the page had already said two lines above. It now says what the view is for and why the three sections are empty, so "why is this blank" is answered on screen. Verified by comparing a server against itself: all three sections grey with (0), and this message. Also drops the per-chart-review buttons from the Charts cards. They fire — the click does fill the slug and open the review, measured before deciding — but the popover closes on the click and the review renders inside an expander at the foot of a paginated page with seventy-odd cards above it. Nothing changes anywhere near the button, so it reads as dead, and making it visible would mean teleporting the reviewer away from the list they were working through. The card already shows the edit and links every chart it lands on. The lookup box stays: typing a slug to check a chart you expected to change is its own job. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
paarriagadap
force-pushed
the
metadata-diff
branch
from
August 25, 2026 17:01
fc5cd76 to
922992b
Compare
…he explorer path⚠️ REVERT BEFORE MERGE. Throwaway metadata edit, like 440faf3, kept in its own commit so it can be dropped in one step. `ppp_adjustment_subtitle` is used 14 times across the LIS subtitles and reaches ~612 published explorer views plus the charts, so one reworded sentence is the "one edit → many texts → many pages" case the Blast radius exists to show — and the first one on this branch that touches an explorer at all. How an explorer edit reaches the tool, since it took a wrong turn to find out: etlr export://explorers/… --export → upserts explorers.tsv / .config grapher, asynchronously → refreshes explorer_views + chart_configs the tool → compares those refreshed configs So `--export` does write the DB — the `explorers` row updates immediately and carries the new text. What lags is the refresh grapher runs off it, tracked in `explorers.viewsRefreshStatus` (queued → refreshing → clean) and `lastViewsRefreshAt`. Checking the views straight after the export finds the old text and looks like nothing happened: 466 views take a while, and mid-refresh the count climbs (6 new / 396 old when first measured). Read the status column, not the clock. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
paarriagadap
force-pushed
the
metadata-diff
branch
from
August 25, 2026 17:12
44f1ee2 to
13bc41d
Compare
Found by the LIS edit, which was the point of making it: the tool reported **zero explorers** while 402 views of `incomes-across-distribution-lis` on staging carried this branch's new sentence. It had detected all 402 — and filed every one under "master's lag, not this branch". Attribution asked one question: did the indicator's `variables` row change? A garden step's `presentation.grapher_config` title / subtitle / note is not in that row — it is a config of its own, reached through `variables.patchConfigIdETL` — so the surface that moved and the evidence used to attribute it were looking in two different places. An explorer view's text *is* a chart config, which makes this the exact edit most likely to move one. `fetch_indicator_config_texts` reads that config, and `changed_indicator_configs` compares it between environments; explorer attribution now unions both answers. The LIS edit lands as one edit, 348 rendered texts, 402 explorer views — and the Explorers section goes from (0) to (0/348). Two counting fixes fell out of having live data at last: - an explorer's affected views are counted by union, not by the largest single text. One edit renders into a text per view here (each subtitle names its own period and decile), so the max reported "6 views" where 402 had changed. The dimensions are in the change group, exactly as for MDims, so the union is available and is the true number. - the grid's MDims heading counts MDims — "2 of 78 MDims changed by this branch", the MDims section's own phrasing — instead of pairing a per-MDim count with a per-view one. Every section header and index row below it already carries its own view count, and two denominators side by side read as a discrepancy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… reach **Explorer views get a dimension grid**, a hierarchy beside the MDims rather than one leaf saying "402 views changed" with no way to see which. An explorer view is addressed by dimensions exactly as an MDim view is, so the same grid answers the same question, drawn the same way: expanded, titled with the explorer's own name (`explorers.config.explorerTitle`) and its slug beneath. Its columns are inferred from the views themselves — an explorer publishes no dimension list the tool can read — narrowest dimension first, because a leaf is named by the last dimension's value and leaving a two-choice toggle there labels four hundred leaves "true". The tree now takes a list of hierarchies (grids under a heading) and a list of branches (flat linked leaves, for charts). The charts branch was already a generic renderer, so it became one of a list rather than being copied. **The section buttons carry a review marker, not a count.** `Charts (2/10)` counted distinct text changes, and beside the word "Charts" that reads as ten charts, which it is not — one text change can reach eight hundred. `Charts 🟡` until everything there is reviewed, `Charts ✅` after, the same two marks the change cards use, nothing when there is nothing to review (the button is already greyed for that). **A chart's own review is a page, reachable by clicking its name.** `?chart=<slug>` now routes: that chart's review, alone, at the top, with a way back. Three separate reasons it did not work before, each found by measuring rather than reasoning: - the link was absolute, built from `SOURCE.wizard_url` — the *staging* wizard — so from a local instance every one of them left the app you were using. Now relative: same host, whichever host that is. - Streamlit renders every markdown link with `target="_blank"`, so the review opened in a new tab and the tab you clicked from did not change, which from the reader's side is a link that does nothing. The lists are HTML now, so the anchor can say `target="_self"`. The ↗ to the live chart keeps `_blank`. - following a link inside an already-open session undid itself: `url_persist` seeds a widget from the query string only when its session value is `None`, and the lookup box leaves an empty string once rendered, so the blank was written straight back over `?chart=`. `requested_chart` gives the URL precedence over a blank widget, and the box declares `value=""` so clearing it removes the param instead of leaving `chart=` dangling. Also fixes a crash this branch shipped for one page load: `_explorer_dimensions` sorted with a key calling `order.index`, and `list.sort` empties the list while it runs, so the first comparison raised `ValueError: list.index(x): x not in list` and took the whole Blast radius down. Positions are captured first, and the test pins it. A second, quieter one beside it: tidying dashes into spaces turned a dimension value of "-" into a single space, leaving those leaves unlabelled. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6668c04df
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| description_key_ppp: |- | ||
| <% if poverty_line is not defined or poverty_line not in ["40% of the median", "50% of the median", "60% of the median"] %> | ||
| This data is expressed in constant international dollars to adjust for inflation and differences in living costs between countries. Read more in our article, [What are international dollars?](https://ourworldindata.org/international-dollars) | ||
| This data is expressed in constant international dollars to adjust for inflation and differences in living costs between countries. Comparisons across countries and over time are only meaningful after this adjustment. Read more in our article, [What are international dollars?](https://ourworldindata.org/international-dollars) |
There was a problem hiding this comment.
The commit description explicitly identifies these World Bank PIP edits, together with the LIS edit, as throwaway staging data that must be reverted. Leaving them in the commit will ship unrelated reader-facing wording changes across poverty and income indicators when this feature merges; remove all of the demo metadata changes from both .meta.yml files.
Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment.
Written by Claude Opus 5 — @paarriagadap at the wheel.
You have read the commit correctly, and the conclusion is right — these edits must not merge. Leaving them for now is deliberate, though, so no change here.
They are the only real input this tool has. Metadata Diff resolves the text a chart, an MDim view and an explorer view actually serve, and the interesting cases — a shared definitions.* entry rendering into many indicators, a subtitle inherited through grapher_config rather than stored on the variable — cannot be reproduced with fixtures. Reverting them now would leave the branch with nothing to compare against on staging, and every count on the page at zero.
They come out in their own revert before this merges, and both commits are named so it is obvious what to drop. Marking this thread resolved would be wrong while they are still here, so I am leaving it open as the reminder.
…e link opens the tree The inline caps on the MDim and explorer cards cut the remaining changes' controls away rather than folding them: `n reviewed / N` counted every change, so a card past its cap could never read complete however much the reviewer read. The remainder now renders in an expander, toggles included. The MDim cards' dimension-tree link still carried the removed deep page's route, which the MDims list drops on load — so it opened the list the reader was already on. It now carries the Blast radius section's own state, MDim included, and that section draws the requested MDim first so its own cap cannot be what hides it. The PR brief's reach lines asked for usage of each group's first indicator only, while `group_usage` reads the whole of `indicator_ids` back out — for an edit to a shared definition, most of the charts and MDims it reaches went unmentioned. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…shed MDim in Chart-text comparison selected indicators with `catalogPath LIKE '<dataset>%'`, which also matches a sibling whose name starts with the same string — `surface_temperature` pulled in `surface_temperature_anomalies`, so a difference of the sibling's was reported as this branch's work. The predicate now stops at the `/` or `#` that follows a dataset's name. Unpublished MDims recorded their reach but not their review marks, so a branch whose only change is a draft MDim met a greyed-out MDims section it could not open — the section its own cards, toggles and all, are rendered in. Both kinds now go through one recording path, which is also what stops them drifting apart again. And that drafts list paginates, because nothing in the section can look an MDim up: a card off the first page was unreachable. A metadata value that cannot be dumped as YAML no longer comes back as a `repr` labelled as a pastable snippet — it may be invalid YAML, or valid and wrong. The failure is now stated in the snippet, with every line commented out so there is nothing to paste by accident. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51bb1801df
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+704
to
+705
| source_rows = chart_text_rows(source_engine, in_play) | ||
| target_rows = chart_text_rows_by_slug(target_engine, list(source_rows)) |
There was a problem hiding this comment.
When a scoped dataset was rebuilt but one of its charts differs for an unrelated reason—such as a production/admin chart edit made after the staging server forked—this compares the chart's entire resolved config and attributes every title, subtitle, or note difference to the branch. The dataset scope and rebuild checks establish only that the chart uses branch data, not that ETL authored its config change, so Metadata Diff duplicates Chart Diff and reports baseline lag as branch work; compare the ETL patch layer or cross-check the resolved value against master before accepting it.
Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment.
Written by Claude Opus 5 — @paarriagadap at the wheel.
Real gap, and I am not fixing it in this round — deliberately, because the fix is a change to what the tool claims rather than a patch. Leaving the thread open.
The diagnosis holds. Dataset scope plus "this server rebuilt it" establishes that the chart renders this branch's data, not that ETL authored the text difference. A chart edited in the production admin after this server forked differs for that reason alone, and the comparison would report it — backwards, as text this branch wrote.
Where I disagree is only about the remedy. The other two surfaces answer this by cross-checking staging-site-master: if master's server says the same thing this one does, the difference is master's or the baseline's lag, not ours. summarize already receives that engine, and chart_text_rows_by_slug already reads chart text by slug, so the mechanism exists — but it needs somewhere to put the answer. The MDim and explorer sections both have a "these differ, but not because of this branch" bucket; the Charts section has none, and silently dropping the charts instead would be the failure this tool exists to prevent. So it is a bucket, its copy, the owidbot line, and the plumbing — the author's call, not a review-loop patch.
Worth recording for whoever picks it up: comparing the ETL patch layer alone is not sufficient either. Chart text can be inherited from the indicator's grapher_config (that is what commit ee62989 was about), so the resolved value is the only place some of these edits are visible. The master cross-check is the part that generalizes.
…-only edit still reports Two ways the MDim count can overflow the view-by-view budget — too many published MDims, or too many unpublished ones — and either leaves the section with no review keys while its cards are still rendered. Only the published overflow was allowed for, so a branch changing a lot of draft MDims met a greyed-out section holding all of them. The two flags are read together now, through one `keep_sections`, which also puts the "which zeros are silences" rule next to the summary it reads rather than in the page body. A garden `presentation.grapher_config` edit moves a chart's own title, subtitle or footnote without touching any indicator row, so a branch that only does that had every indicator-layer count at zero — and owidbot's comment named the field and no charts at all. It now reports those charts on their own line, deliberately not folded into the chart total: the two sets overlap and nothing here dedupes them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c669aed6cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+1477
to
+1483
| def _reach_slot(reach: dict[tuple[str, str], ChangeReach], g: ChangeGroup) -> ChangeReach: | ||
| """The blast-radius row for this change, creating it on first sight of the text.""" | ||
| key = change_identity(g) | ||
| slot = reach.get(key) | ||
| if slot is None: | ||
| slot = ChangeReach(field=g.field, old=g.old, new=g.new) | ||
| reach[key] = slot |
There was a problem hiding this comment.
If two unrelated garden datasets happen to make the same field transition, change_identity is identical for both, so this slot merges them and discards their catalog-path origins before group_by_edit runs. The Blast radius UI then states that the combined reach came from “1 edit,” although applying or reverting it requires edits in two separate files; the Charts section already recognizes this case as multiple edits via distinct_garden_datasets. Include the authored dataset/location in the identity while continuing to merge sightings of the same edit across charts, MDims, and explorers.
Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment.
Written by Claude Opus 5 — @paarriagadap at the wheel.
The ambiguity is real; the remedy as stated would trade this wrong number for the opposite one, so I am leaving this open for the author rather than applying it.
Two texts can share an identical old → new transition for two different reasons, and the row cannot currently tell them apart:
- one edit to a shared
definitions.*orshared.meta.ymlentry that renders into indicators of several datasets — one file to change, and merging is right; - two datasets edited separately that happen to land on the same transition — two files to change, and merging is wrong.
Putting the dataset into change_identity splits both cases, so the first would report "2 edits" for one sentence typed once, across two rows a reviewer has to compare by hand. That is the failure this grouping was built to fix, and it is not hypothetical on this branch: the shared-definition case is what its staging data exercises. distinct_garden_datasets in the Charts section is not quite the precedent it looks like — it counts where the text lives for a single group, rather than deciding what counts as one edit.
What would settle it is the authoring location, which nothing here reads: parse_catalog_path resolves an indicator to its dataset's garden folder, never to the file the text was typed in, so a shared entry and a per-dataset one look identical. Until something reads that, the honest fix is to keep merging and say so on the row — carry the distinct garden datasets on ChangeReach and have the Blast radius line read "1 edit, authored in 2 datasets" — which is a copy and model decision for whoever owns the section, not a review-loop patch.
…les live in The chart-text comparison matched the branch's scope against `variables.catalogPath`, which is always in the grapher channel, while the scope holds whichever channel the changed file sits in. Usually harmless — a garden step's grapher sibling is in scope too — but an edit to a `shared.meta.yml` resolves to the garden steps that own the file and to nothing else, so its charts were looked for under a `garden/...` path that matches no variable at all. That is the worst place for the mismatch to land: chart text is the only comparison that can see a shared `presentation.grapher_config` edit, because such an edit never touches the indicator row. The change disappeared rather than being reported from elsewhere. Every dataset in scope is now asked for once, by identity, in the grapher channel. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rked Three changes, all about what a section shows you first. **Per-item views are back, from #6615, with the navigation taken out of the way.** That branch had a View diff page (one MDim, one view at a time, reached by assembling a dimension selection) and a chart flow you typed a slug into. Both answered a question the change-grouped lists cannot — "what happened to *this* view / *this* chart" — and both made you navigate before showing you anything. So: - **MDims → View by view**: every changed view of an MDim, already on the page, each with its own diffs and a link that opens it (admin preview when the MDim is unpublished). A card shows three and folds the rest; `🔍 Open view by view` gives one MDim its own paginated page, addressable as `?mdim-views=<path>`. - **Charts → Chart by chart**: every changed chart, ordered by how much changed, naming the fields and linking to its own review. Plus a searchable picker — "Open one of the 67 changed charts" — which is the thing that was missing: you could reach a chart's review only by typing its slug. - **Explorers → View by view**: the same for explorer views. **Item view is the default in all three**, with `🧬 By change` one click away under a shared `?layout=` key. Grouping by change is still the right shape for judging an edit — one reworded sentence listed once, with the eight hundred pages it reaches underneath — it is just not what someone arriving at the page is usually asking. **The review sign-off is out of the UI for now**: no Reviewed toggles, no "k/n reviewed" counters, no 🟡/✅ per change, and the section badges are back to plain names. Parked, not deleted — `review_state.py` and the table stay, and the summary still identifies every distinct change, because the badges and the greyed-when-empty rule count changes rather than ticks. Two tests the last review round added pinned "every change past the cap keeps its Reviewed toggle". The controls are gone, so they are replaced by one test pinning the half that survives and is what the finding was really about: a card that says it holds N changes renders N changes. It asserts on a single inserted token per change, because the diff renders word by word and a rewritten sentence comes back split across <ins>/<del> tags. The stale-label serde test is deleted outright — it failed on its own precondition ("the labels must differ, or there is nothing to defend against"), which is exactly right now that a label is just a name. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…w page #6615's View diff had two controls this page was missing: a ⚡ jump to a changed view, and the MDim's own dimension menu. Restored — and restored the way that branch wired them, which is the part worth copying: the jump renders nothing itself, it *writes into* the menu, so there is only ever one answer on screen to "which view am I looking at". ⚡ Changes detected — jump to a changed view (51) Indicator Group Period Breaks in data Mean income… nan Per day Don't show Two differences, both following from the page being a list first: - every dimension may be left unset, so the menu **filters** as well as focuses. Verified: `?dim-indicator=mean` alone reports "6 of 51 changed views match the menu above" and lists them. #6615 forced a complete selection, which is how it could land you on "No view exists for this combination". - with nothing set you still see every changed view. The menu narrows; it is not the way in. A complete selection shows that view **whether or not it changed** — "did the view I was worried about move?" is a question only an unchanged view can answer, and it says so plainly rather than rendering an empty diff. The jump writes the dimension widgets' session state from a callback, which is the only order that works: a widget reads its session value when it is created, so setting it afterwards is a change nobody sees until the next rerun. Stale values are dropped before the widget is built — widening a dimension above can strand the one below, and `url_persist` raises on a value that is no longer an option. Every choice is URL-visible, so one view of one MDim is a link somebody else can open: `?mdim-views=<path>&dim-decile=nan&dim-period=day&dim-indicator=mean&…` Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four asks, one shape. Each item section now holds exactly this:
[ picker: every changed item, searchable ] [ Next ▶ ]
<that item's diff>
**MDims.** The "Open view by view" button is gone — it sat inside View by view and
led to View by view. In its place the tab opens straight onto an MDim, with a
picker above it: "MDim 1 of 2 changed by this branch", plus **Next MDim ▶**. That
is also the answer for a branch touching several MDims, and **unpublished MDims are
in the same picker**, badged, rather than filed in an expander somewhere else —
"which of mine changed" is one question, and an unpublished MDim is the one most
likely to be yours. Published first, then drafts. Below the picker, the ⚡ jump and
the dimension menu are unchanged.
Switching MDim clears the previous one's ⚡ jump and dimension selection. Left in
place they would either filter the new MDim to nothing or, worse, quietly match a
same-named dimension and show a view nobody asked for.
**Charts.** A picker of the changed charts ("Chart 1 of 67 changed by this branch")
and **Next change ▶**, with that chart's full review inline — and nothing else: the
list of the same charts underneath is gone, and so is "Look up any chart". The
review used to sit behind a text box asking for a slug the caller already knew, so
`chart_flow` becomes `render_chart_by_ref(source, target, ref)`.
Both browsers open on the most-changed item rather than an empty page waiting to be
told what to show, and both write `?chart=` / `?mdim-views=` on every move, so the
address always names what is on screen. The By-change cards keep their own way in:
their "View by view" button now switches layout *and* selects that MDim.
Also: the MDims header counted only published MDims while the picker counted drafts
too, so it read "1 MDim changed" above "MDim 1 of 2". It now says "1 MDim changed by
this branch, plus 1 not published yet".
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The ⚡ jump could take you to any changed view, but only by naming it — with fifty views that is fifty decisions to read them all. Next change ▶ sits beside it and does what its Charts counterpart does: move to the next one. Both controls drive the dimension menu, so there is still one answer on screen to "which view am I looking at", and stepping leaves the jump showing the view it moved to (verified: after one press the jump reads "Threshold income or consumption · Poorest 10%, median, richest 10% · Per month · Don't show", which is the view rendered below it). Next is relative to where you are, not to a counter of its own: the current position is recovered by matching the menu's selection against the changed views. So stepping continues from a view you reached with the menu, or from a link somebody pasted you, and wraps at the end rather than dead-ending. Verified three presses in a row, each landing on a distinct view with its diff. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ValueError: Please review the URL query.
Value 🔍 Chart by chart not in options ['items', 'changes'].
Same failure as the first crash on this branch, reintroduced by me in the layout
switcher. `st.segmented_control` sends and receives the *formatted label*, and
hands the raw label back when it matches no current option. The three sections
word the item option differently — "🔍 View by view" on MDims and Explorers,
"🔍 Chart by chart" on Charts — under one shared `?layout=` key, so moving between
sections produced exactly that, `url_persist` wrote the label into the URL, and the
next load rejected it. Inside a single section the labels never change, which is
why it only fired on a section hop.
Two guards, because either alone leaves a hole:
- `coerce_layout` reads a label back to the option it denotes, by marker rather
than by an exact table — the wording is per-section and will keep changing,
while which of the two a label means is stable.
- the switcher sanitizes the URL *and* the held session value before the widget is
built, so `url_persist`'s strict check never sees a label.
The per-section wording stays; it is the useful half and not what was broken.
Verified against the failing path rather than the fix: the section-hop test fails
with the sanitizer removed, and the reported URL — `?layout=🔍%20Chart%20by%20chart`
— now loads, rewrites itself to `layout=items`, and renders. Six section hops with
the layout set produce no exception and carry `layout=changes` across all three.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…s links stated once **MDims show exactly one view.** The paginated list is gone: the page opens on the first changed view, says "Changed view 1 of 51 — Next change ▶ steps through them", and Next walks the rest. The ⚡ jump and the dimension menu still choose directly, and a complete selection naming an *unchanged* view still shows it, since that is the only way to answer "did the view I was worried about move". Stepping had to learn where it was. Position is read from the URL before the nav renders — the nav's callbacks write the menu's state, so no later order works — and it now uses the same precedence the page uses to choose what to show. Without that the first press re-selected the view already on screen and appeared to do nothing. **A Reviewed toggle per item**: per chart, per MDim view, per explorer view. That is a different question from the parked per-edit sign-off, so it gets its own identity rather than borrowing one: `item_identity` keys the slot to the item and hashes every changed text on it, so editing any one of them makes the mark stale and the item reopens. Separate `item:` surfaces, so the two kinds of mark can never overwrite each other's rows, and the same promises hold — content-bound, stored on this staging server, never synced on merge. Verified a tick survives a reload. **The chart review states its two data pages once**, under the title, instead of repeating the same pair on every field's two columns — four changed fields meant the same two links five times over. The "N fields changed" banner is a plain line now rather than a warning: it is the reason you opened the page, not a problem. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…appearing **By change was unreachable in MDims.** The section had an early route: with `?mdim-views=` set it rendered that MDim's page and returned — before the layout switcher was drawn. The picker and Next set that key on every move, so one click into the section removed the switcher entirely and there was no way back to the grouped list except editing the URL. The route is gone; the browser reads the key as its selection, which is what it was for. **Explorers now gets everything MDims got**: an explorer picker with **Next explorer ▶**, one changed view at a time with the ⚡ jump and **Next change ▶**, a cascading dimension menu, both servers' links on the view, and its own Reviewed tick. Explorers publish no dimension list, so the menu's columns are inferred from the views themselves — narrowest first, so a view's label ends in the thing that distinguishes it rather than in a two-choice toggle. Rather than copy four controls into a second section, they moved into `view_nav`: the jump, Next, the menu, and the rule for which view is on screen. The prefix is a parameter, so the two sections cannot read each other's dimension selections (`dim-` vs `edim-`), and the dimension list is a parameter, because one surface publishes one and the other does not. The first press of Next was already wrong once; that is now one place to be right. **The tick sits at the top of every item**, beside its name, instead of under the diff — on a chart with four changed fields it was below the fold, so you scrolled past the answer to reach the control. Verified by geometry, not by eye: the toggle's top is above the first diff slot's in both sections. **An MDim view offers both servers' links**, as the chart review does: reading a diff and then opening only one side leaves you comparing text against memory. A new view shows only this server, having nothing to compare against, and an unpublished MDim's links are admin previews on both sides. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…d By edit **Notes.** Every item — a chart, an MDim view, an explorer view — gets a note box beside its tick, collapsed until something is written. The row already had a `comment` column, so this needed no schema change, but it did need the status to mean something: `reviewed` used to be "a row exists", so writing a note would have marked the item reviewed. Now a note-only row carries `noted`, unticking keeps the note rather than deleting it, and `reviewed` checks the status. **A fourth tab, Review**, collects what the pass produced: every tick and note, grouped by surface, plus the same thing as markdown to paste into the PR. It computes nothing of its own — one query over the rows the sections wrote. It also states the denominator, because a list of what you ticked reads as a finished job: "1 of 555 items ticked · 1 with a note", then "**Review unfinished** — 554 items still to look at", or "Nothing ticked yet" when only notes exist, or "Nothing reviewed yet" on arrival. The total is changed charts plus changed MDim views plus changed explorer views, read from caches the sections already filled; an MDim it cannot read is skipped rather than guessed at. **Every picker says what you have already done to each item** — ✅ ticked, 📝 note — which is what makes stepping through fifty views survivable. It reads the slot only, never the text, so it costs no diff on a list of sixty-seven charts; the consequence, stated in the code, is that it cannot see staleness, and the item's own page is what reports that. **"By change" is now "By edit"**, the name Blast radius already uses for the same grouping. The old label still resolves, so a link written before the rename lands where it meant to. **The active layout stops looking unselected.** `st.segmented_control` is deselectable: clicking the option already active clears it and returns None, so the page kept showing the item view while the control showed nothing. A callback puts the value back before the next run, and a last-good key covers the run in which the click happened. **The section bar says which way to work**: left to right, Blast radius → the three surfaces → Review. One bug found by checking rather than assuming: the Review tab first came up empty over a table with rows in it. `surface_key` namespaces every surface with `list:`, so the query for `item:%` matched nothing — it asks for `list:item:%`, taken from the same helper that writes the rows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…the rest
The tab was listing twelve characters of hex per row. A stored row carries a hash,
not a name — the slot has to survive an edit to the text — but the names are
recoverable: enumerate the items the sections show, hash each one the way its tick
was hashed, and match. Everything that enumeration needs is already cached for the
sections, so the index costs nothing new.
So a recorded item now reads as itself, and links to itself:
Charts · 1 of 67 reviewed · 0 with a note
✅ share-in-poverty-relative-to-different-poverty-thresholds (6 changes)
MDim wb/latest/incomes_pip#incomes_pip · 1 of 51 reviewed · 1 with a note
📝 Incomes across the distribution — Threshold income or consumption · …
> Checking the PPP wording with the data team before merge.
**Ticks with nothing written on them are folded**, not listed: one says "done" and
four hundred say it four hundred times. Each surface shows its count and hides the
bare ones behind "N ticked with no note" — folded, never dropped, since "which ones
did I tick" is a fair question, just not the first one. The markdown export does the
same, because pasted into a PR a list of every ticked view buries the two sentences
somebody actually wrote.
**The MDim and explorer pickers show progress too** — `Incomes across the
distribution · ✅ 1 · 📝 1` — so the level above a view says whether you have been
there at all. No denominator there on purpose: it would mean diffing every view of
every MDim to label a dropdown.
One thing this shook out: the Charts section and the Review tab were each
enumerating the changed charts, and two enumerations can disagree about how many
there are. Now `cached.changed_charts` does it once for both.
Also: an item whose row survives but whose change does not — text reverted, chart
unpublished — is reported as "an item that is no longer in this diff" rather than
rendered as a bare hash.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nnot miss They were two halves of one decision in two places: the tick tucked into the header's right column, the note collapsed inside an expander below it. So the note read as a detail and the tick was easy to walk straight past. Now one review strip per item, directly under its name and above the diff — tick on the left, note open on the right, both inside a tinted box. The tint matters more than it sounds: on a page whose body is two columns of prose, an untinted row of controls disappears. Keyed containers render as `st-key-<key>`, so the CSS matches by prefix rather than needing a class of its own. Verified by geometry rather than by eye: one strip holds both controls, the note is to the right of the tick and on the same row (112,971 vs 376,949), the strip sits above the first diff slot (932 < 1102), the box is `rgb(241, 243, 249)`, and a note typed into its new home survives a reload. The note stopped being an expander, so `st_item_note` becomes `st_review_strip` and the three item renderers each lost a column split. Applied by line range rather than by matching source text: the formatter had already reflowed two of these anchors between edits, and a silent no-match is worse than a noisy one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
It was a text area, where Enter inserts a newline and the value is only committed on
blur or Ctrl+Enter — so a note typed and confirmed with Enter looked saved and was
not, which is the worst way for this to behave. A single-line input commits on
Enter, and a review note is a sentence, so that is the right trade.
Verified end to end rather than by reading the docs: typed into the box, pressed
Enter, clicked nothing, and the row is in the table —
list:item:mdim:wb/latest/incomes_pip#incomes_pip | noted |
Enter-key save check: subtitle wording queried. | 2026-08-26 11:20:55
The placeholder says so ("Press Enter to save"), and the strip's tint now styles an
input rather than a textarea.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Streamlit offers one or the other: a text input commits on Enter but is a single
line, a text area is multi-line but commits on blur or Ctrl+Enter, where Enter only
inserts a newline. So the box is a text area again and a small script binds the
keys — Enter blurs it, which is exactly what makes Streamlit commit the value and
fire `on_change`, so saving goes down the same path clicking away already used.
Shift+Enter is left alone and does what it always did.
The script lives in a zero-height component reaching the parent document, the way
the dimension grid does, and rebinds on mutation because Streamlit rebuilds the DOM
on every rerun. If it never runs, the note still saves when you click away — the
behaviour degrades to Streamlit's own rather than breaking.
Verified in the browser and then in the table: typed a line, Shift+Enter, typed a
second, pressed Enter, clicked nothing —
'Line one of the multiline check.\\nLine two after Shift+Enter.'
list:item:mdim:wb/latest/incomes_pip#incomes_pip | noted | 2026-08-26 11:24:36
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Markdown carries no newline inside a blockquote or a list item on its own, so a
two-line note broke in both places the report renders it — and the note box now
invites two lines, since Shift+Enter starts one.
On the page, `> one\ntwo` quoted the first line and dropped the second out of the
quote as a floating paragraph. Every line carries the marker now, with a bare `>`
for blank ones so the quote stays a single block:
> Line one of the multiline check.
> Line two after Shift+Enter.
In the export, ` - one\ntwo` ended the list item and left the rest as a stray
paragraph between bullets. Continuations are indented four spaces, which keeps them
inside the item — checked against the real row rather than a made-up one:
- [Incomes across the distribution — Threshold income or consumption · …](…)
- Line one of the multiline check.
Line two after Shift+Enter.
Both helpers are pure, so both are pinned by a test — including the empty note,
which used to be an IndexError waiting to happen.
Verified on the page too: one blockquote, both lines inside it, nothing stray.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
**The explorer comparison read every published view's text from both servers** —
8,976 rows a side, four JSON extractions each — to find the handful that changed.
The hash was already there: `chart_configs.configMd5` covers the whole resolved
config, so two views whose hashes match cannot differ in their text. A cheap hash
join names the candidates and only those are read.
Measured against the same server, and the answer is identical (402 changed views
either way — checked, not assumed):
candidates after the hash join 497 of 8,938 views
explorer_view_rows × 2 (all) 0.46s
explorer_view_rows × 2 (candidates) 0.13s
changed_explorer_views, cold 3.75s -> 3.10s
summarize, cold 4.85s -> 4.11s
Conservative in the right direction: a view whose config changed in some non-text
way becomes a candidate and is then compared textually, costing a row rather than
producing a false positive. Same trick the MDim comparison already uses.
**Each item view was reading the same review rows twice** — once to mark the
picker's options, then again to resolve the item's own tick. Two identical queries
on every toggle, note and step, which is the one path where latency is felt. The
loaded rows are passed down instead.
Profiled and deliberately left alone, so nobody repeats it: `mdim_changes_df`
(~2.9s warm) spends it on 78 MDim configs and 974 candidate indicator paths, and
`summarize` already fetches its three surfaces in parallel. The cold figures above
were inflated by the branch scope being recomputed per call (~1.4s of git), which
the app pays once.
**Also in here: the review notes say where they came from, and a button copies
them.** Pasted into a PR comment or handed back to an assistant, the notes arrived
context-free, so the text now leads with its provenance:
- Branch: `metadata-diff`
- Pull request: #6753
- Staging server: http://staging-site-metadata-diff
- Compared against: `production`
The PR is asked of `gh` once per session and best-effort — `gh` may be missing or
unauthenticated and the branch may have no PR yet, none of which should stop the
notes rendering. Nothing is guessed from the branch name; a missing line beats a
wrong one. The copy button is a component rather than a widget, because the
clipboard is a browser API: the text is embedded as JSON so quotes and newlines
survive, with an `execCommand` fallback since `navigator.clipboard` is unavailable
over plain HTTP, which a staging server is. Verified: it reports "Copied".
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
paarriagadap
force-pushed
the
metadata-diff
branch
from
August 26, 2026 11:48
c177321 to
dc34e59
Compare
👀 nothing recorded, ⏳ started, ✅ every item ticked — on Charts, MDims and
Explorers, live on this branch's data:
Blast radius · Charts ⏳ · MDims ⏳ · Explorers 👀 · Review
Only those three carry a mark. Blast radius reports reach and Review collects the
record, so a mark on either would be a claim about nothing — enforced in
`section_label` rather than left to the caller to remember, which is what the test
for it caught.
Asked for lazily, because ✅ needs a denominator and the denominator means diffing
every changed view of every changed MDim: with nothing ticked there is no ✅ to
establish, so a fresh page shows 👀 without enumerating anything. Once something is
recorded, the totals come from caches the reading has already warmed.
That enumeration was the Review tab's, so it moves into `cached.item_index` and both
read it — one list of the items, one set of totals, no way for the bar and the tab to
disagree about how many there are.
`view_label` and `view_url` move from `render` to `core` to make that possible:
`render` imports `cached` to clear its caches, so the cache importing the renderer
would have been a cycle. Both are pure — a label from dimensions, a URL from an env
and a slug — and belong with the rest of the logic that needs no Streamlit.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…hanges themselves
The notes say what a reviewer thought; they never said what the PR does. Pasted into
a GitHub issue or a channel, "2 of 556 ticked" plus three sentences describes a
review of something the reader cannot see.
So the tab holds two documents, each named for what it is and each copyable on its
own — they go to different places, and often to different people:
**What this branch changed** — the edits themselves, grouped as Blast radius groups
them, so one reworded sentence is one entry however many texts it renders into:
**5 edits** authored here, rendering **371 distinct texts**, on **69 pages** a
reader can reach, plus 10 unpublished
### WYSK — 12 texts · 63 charts, 1 MDim, 8 unpublished charts
- added: “Comparisons across countries and over time are only meaningful after
this adjustment.”
### Chart subtitle — 348 texts · 1 chart, 1 explorer
- before: “…living costs between countries. Income here is measured after…”
- after: “…living costs between countries, so amounts are comparable across
countries and over time. Income here is measured after…”
**The review notes** — unchanged, still named, linked and counted.
Both are plain markdown, no HTML: the inline <ins>/<del> the page uses renders as
tags in Slack. Both go through `markdown_output`, which already renders copyable
markdown *and* carries a copy button with the non-secure-context fallback — so the
copy button I had started writing was deleted rather than kept beside it.
Writing the digest immediately caught a real defect in it: two edits came out with
before/after lines that were character-for-character identical, because the words
that moved sit past the trim. Trimming from the front is the same bug the
blast-radius preview had — a diff that shows no difference reads as "nothing
happened here". The pair is now trimmed around the divergence, measured from the
common prefix and suffix, and a test pins it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Each one has its own home now: view by view is the section's default layout, the dimension tree is Blast radius, and a pasteable account of the changes is what the Review tab's two documents are. Three exits from a card whose job is to show one edit. Verified: the only buttons left on that page belong to the wizard's chrome and the layout switcher. `usage_for` moves to `brief.py` rather than being deleted with the button that called it. It fetches every indicator of a shared edit rather than the group's first — a Codex finding from this PR's review, and deleting the caller would have quietly taken the fix and its test with it. Note for whoever picks this up: **the PR brief is now unreachable from the UI.** `brief.py` still builds it and its tests still pass, but nothing calls it. Either surface it in Review beside the other two documents, or retire the module — it should not sit in the tree as neither. Also: I rewrote `usage_for`'s body from memory while moving it and changed its cache key, which its test caught immediately (`KeyError: 'configMd5_target'`). Restored verbatim from git. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… which explorers
"63 charts, 1 MDim, 1 explorer" says how far an edit reaches and nothing about where
to look. Read in an issue or a channel, the next two questions are always the same,
so each edit now answers them:
### WYSK — 12 texts · 63 charts, 1 MDim, 8 unpublished charts
- added: “Comparisons across countries and over time are only meaningful…”
- datasets: `etl/steps/data/garden/wb/2026-06-26/world_bank_pip`
- MDims: [Incomes across the distribution](…/grapher/incomes-across-distribution-wb)
— `wb/latest/incomes_pip#incomes_pip`;
[Poverty indicators](…/admin/grapher/wb%2Flatest%2Fpoverty_pip…) (unpublished)
— `wb/latest/poverty_pip#poverty_pip`
### Chart subtitle — 348 texts · 1 chart, 1 explorer
- datasets: `etl/steps/data/garden/lis/2026-06-12/luxembourg_income_study`
- explorers: [incomes-across-distribution-lis](…/explorers/…)
An unpublished MDim is labelled and linked to its admin preview, since that is where
it can be seen at all. A shared definition edited in two datasets names both —
naming one would send somebody to fix half the change.
Three things had to change for the dataset line to be true rather than mostly true:
- the blast-radius rows never carried the indicators behind a text, so they do now.
- a chart-config edit knew no indicator at all: `chart_text_rows` already joins
`variables` to find the charts, so it returns that catalogPath (grouped per chart
rather than `distinct`, and every column aggregated because the group key is the
chart while the rest live on other tables).
- `group_changes` collected paths only for indicator-layer changes. That gate is
right for `catalog_paths`, which licenses the shared-definition claim, so the
paths a change was merely *seen* on go in a separate `authored_in` that only the
reporting reads.
Also: `view_url` no longer leaves a bare `?` on a link with no dimensions — an MDim
URL pasted into an issue looked truncated.
Co-Authored-By: Claude Opus 5 <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
2 participants
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.
440faf38eaand13bc41d172must be reverted before merge. Between them they are six throwaway metadata edits — five inworld_bank_pip.meta.yml, one reworded shared subtitle definition inluxembourg_income_study.meta.yml— kept so the branch's staging server has something for the tool to find. They are also whyowidbot/chart-diffgoes red: the approval gate doing its job on charts whose text this branch changes.Metadata Diff reviews how a branch changes the metadata texts readers see — chart titles, subtitles and footnotes,
description_short, and What you should know /description_key— across charts, MDims and explorers.Chart Diff compares chart configs. An edit authored in a garden step — including Jinja templates and shared
definitions— changes what a reader sees while leaving the config byte-identical, so no config diff shows it. This is the diff that catches those.Open it at Wizard → Metadata Diff (
/etl/wizard/metadata-diff).This supersedes #6615, which stays open as a record of the tool's first version. Everything here is the tool itself, apart from the two demo commits flagged above.
What it does
💥 Blast radius leads, and is the only view that crosses surfaces on purpose — three levels, because reporting any one of them alone misleads:
One sentence added to a shared
definitions.*entry is one edit to judge; it splices into every description referencing it, so the site renders several texts; each is read on many pages. Readable by edit (edit → text → page), by affected page, or on one MDim's dimension grid.Charts · MDims · Explorers then list their own surface's changes, each with a Reviewed tick bound to the exact text — edit that text again and the tick goes stale and the change reopens. The section badges count them down (
Charts (5/10)). Nothing syncs on merge: metadata ships through ETL, so the ticks are progress tracking, unlike chart-diff approvals which gateetl chart-sync.The care is in what it doesn't claim
world_bank_pip.meta.ymlput 118 datasets in scope where 9 had been rebuilt here.etlr grapher://… --graphercommand to fix it. Every difference involving one is inverted — the branch appears to have written text it removed — so🚧outranks every other status.Cost
Cold load 9.6s against a 5,182-chart staging server with ~9,000 explorer views (the wizard's own floor is 1.5s); a warm page is 0.9s. The reading is held for 30 minutes, with a ↻ Re-read button in the section bar — a timer expiring mid-review costs a load and explains nothing.
Tests
116 unit tests on the pure logic — comparison keying, branch attribution, export-scope identity, stale-build detection, the edit/text distinction, data-page slot order, review-key staleness. The three sections are also run headlessly against a staging server with Streamlit's
AppTest, which is how the duplicate-key and review-key bugs surfaced.Reviewing this
Review everything except the two demo commits named at the top, which come out before merge.
staging-site-metadata-diffexercises the tool against those edits — a shared WYSK definition, a shared chart subtitle, a shared footnote, a per-variable Jinja title, and a per-variable data-page-only field — which between them cover both layers the tool reads and both prominence groups it reports.chart_configs.full→config: this follows master's migration (12c4a15208,ed3d6e4a4d) rather than resolving the column per environment, so like chart-diff it needs a staging server rebuilt after that migration.Still open
--services metadata-diffin the staging pipeline) must merge before the owidbot section appears.cli.pyonly warns on unknown services, so neither side breaks meanwhile.mdim_pages.pyis misnamed — it holds only the single-chart lookup now.summarizealready takes master's engine,chart_text_rows_by_slugalready reads text by slug) but the Charts section has no "differs, but not from this branch" bucket to put the answer in, and dropping those charts silently would be worse than reporting them. Thread left open on the PR.change_identitykeys on the text transition, which is what makes a shareddefinitions.*edit one row instead of eleven — and also merges two datasets that happen to land on the same transition, where two files need changing. Telling them apart needs the authoring file, which nothing here reads. Thread left open on the PR..meta.ymlfiles, package steps, helper modules imported by a recipe, a retired recipe still on disk — but they are all the same bug: step ownership derived from naming conventions in several places. The DAG already knows which step a file belongs to.🤖 Generated with Claude Code