Repository navigation
fix(gh-389): label the city HUD's two object counts - #405
db-tycoon-stephen wants to merge 1 commit into
Conversation
PR size checkmode: Files changed: 7 counted / 8 allowed
Result: ✅ within limits. |
PR title checkmode:
Result: ✅ title matches the convention. |
The footer showed "90 objects" and the run replay header showed "109 nodes off this map" on the same screen (gh-389). They count different things: tables and views in the catalog, and dbt nodes in one run that have no building (tests, seeds, models the catalog does not have). Read bare, they look like two answers to one question. Each now says what it counts. Rebuilds the shipped web bundle so CI's freshness check stays green. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
54d077b to
c990bcd
Compare
3b218fe to
f6ab71b
Compare
There was a problem hiding this comment.
Independent review at f6ab71b. This replaces an earlier automated comment that was not based on the diff.
Verdict: approve, with two follow-ups.
- The footer now counts tables and views only, which matches how
object_countis built. - The run header counts dbt nodes with no building (tests, seeds, models outside this catalog), which matches
docs/city/run-json-v1.md. - Follow-up:
web/src/ui/tour_stops.ts:107anddocs/city/hud-design.md:58still say "objects". - Follow-up: on catalogs over
MAX_OBJECTS(500), the footer shows the capped count; the cap note in the notes popover is the only hint.
JesuFemi-O
left a comment
There was a problem hiding this comment.
Verdict: approve with fixes. The footer label is accurate and the bundle and tests hold up; the run header label says "no building" for seeds and snapshots that do have one, which should be reworded before it ships.
What it does
Footer N objects becomes N tables and views (web/src/boot/hud.ts:85). The run replay header N nodes off this map becomes N dbt nodes with no building (tests, seeds, models outside this catalog) (web/src/ui/run_panel.ts:206). Adds one e2e test, updates one assertion in refresh.spec.ts, adds a CHANGELOG entry, rebuilds web_dist.
Checked (at f6ab71b, with #401 to #404 below it)
- Footer count:
object_countislen(ctx.objects)(catalog/models.py:100), built only from DuckDB tables and views (catalog/loader.py:139-143), after the 500 object cap. Sources, tests, columns, roads and power lines are not in it. On a demo-tycoon project plus one added view and one added table the document reports 12 objects (11 table, 1 view) and the footer reads12 tables and views. The label is true. - Run header count:
unmapped_countis the run nodes whose unique_id is not a value ofctx.dbt_nodes_by_key(export/run_json.py:105-119).dbt_nodes_by_keyholds models only (catalog/dbt_manifest.py,nodes_by_key). CONFIRMED, see Should fix 1. - Bundle:
scripts/sync_web_bundle.pyproduces no diff on the branch. The minified diff against the base bundle contains only the two string changes (footer template, run header template) and the renamed asset reference inindex.html. No hidden source change. - Screenshots, shipped bundle served over the same exported project, before vs after. At 1280x800 the footer went from
12 objectsto12 tables and views. The run panel header went from 2 lines to 3 and the panel grew 13px, with no overflow or clipping (panel scroll size equals client size). At 420x800 the run panel fits and the header wraps to 3 lines. At 420 the footer#statusis not visible in either build (client width 0, ellipsis, the lens/exported/notes controls take the row), so the new label is not readable there before or after. - Footer clipping by width (measured
#statusclient vs scroll width): after the change the text needs 354px instead of 289px. At 900px viewport and above it fits. At 760px it shows 246 of 354px (before: 246 of 289), at 640px 126 of 354, at 520px 6 of 354. The new label is cut mid-phrase earlier than the old one, and the rows figure is the first thing lost. This sits on top of #403's clamping and is not a regression of it. - Tests:
npx tsc --noEmitclean.npx playwright test137 passed (afternpm run demo-data; withoutweb/publicevery test times out, which is an environment precondition, not a PR issue).uv run pytest -q1575 passed, 10 skipped, 1 deselected.ruff checkandruff format --checkclean. - Mutation check: reverting the footer string to
objectsfails the new test; reverting the run header string fails it too. Therefresh.spec.tsedit only swaps the expected string, so it is not weakened. - Repo rules: 7 files, under the cap of 8. CHANGELOG entry present with the link reference. No em dashes in added lines. The template box for docs is unticked and no docs changed.
Should fix
web/src/ui/run_panel.ts:206: the label says "no building (tests, seeds, models outside this catalog)", but a seed or snapshot whose table is in the catalog does have a building and is still counted. CONFIRMED: on a project whose warehouse containsraw.country_codes(seeded byseed.fx_dbt.country_codes) with a run containing that seed, a snapshot, a test and a model not in the catalog, the run document givesunmapped_count4 with kindsmodel,seed,snapshot,test, while the catalog has a building for the seed's table. The cause is that only models are joined to buildings (nodes_by_keyis models only), so seeds and snapshots are always unmapped. Snapshots are also missing from the parenthetical. Reword so it states what is true, for example "dbt nodes not matched to a building (tests, seeds, snapshots, models outside this catalog)". The same imprecision is indocs/city/run-json-v1.md:81; worth correcting there in the same PR or a follow-up.
Nits
web/src/ui/tour_stops.ts:107(tour "plant" stop) andweb/src/ui/stats.ts:67(Stats heading) still print the sameobject_countasN objects, so the app now names one number two ways. The PR body acknowledges the Stats heading and inspector; the tour stop is the one a first-time user reads next to the footer.docs/city/hud-design.md:58also still saysobjects. PLAUSIBLE that this confuses anyone; not reproduced as user confusion.- On catalogs over 500 objects the footer shows the capped count as
N tables and views, which reads as the whole database. The cap is only named in the notes popover. Not changed by this PR; reproduced from the code path only, so PLAUSIBLE. - The new test pins the strings against a fixture whose unmapped nodes are a test and a seed, so it would not catch the wording problem above. A fixture with a seed that has a table would.
Not checked
- A catalog larger than 500 objects, and a real dbt project with snapshots and sources in a run (the extra nodes above were added by hand to a generated demo project).
- Safari and Firefox rendering; screenshots were Chrome with software GL.
mkdocs build --strict(no docs changed),pre-commit, CI itself.- Upgrade from v0.2.3: the change is display text only and reads no new field, but no v0.2.3 project was run against this branch.
What this does
The footer said "90 objects" and the run replay header said "109 nodes off this map", with nothing to say they count different things. The footer now reads "90 tables and views" and the run header "109 dbt nodes with no building (tests, seeds, models outside this catalog)". Also rebuilds
web_dist.Mental model
Each number names its unit: catalog relations versus one run's dbt nodes that have no building.
Linked issue
Closes #389
Type of change
Checklist
pre-commit run --all-filespassescsv-importpipeline still runs end to end (this is the CI gate — if it breaks, the release is blocked)CHANGELOG.mdif this is user-facingNotes for reviewers
refresh.spec.ts's"99 objects"assertion was updated to match. The Stats modal heading and the plant inspector still say "objects"; they aren't next to the run count.Depends on
The shipped
web_distbundle is rebuilt here, so whichever city PR merges later needsscripts/sync_web_bundle.pyrerun.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.