Skip to content

feat(lists): add Queries + Parameters count columns to list tables#436

Open
SoundMindsAI wants to merge 5 commits into
mainfrom
feat_list_count_columns
Open

feat(lists): add Queries + Parameters count columns to list tables#436
SoundMindsAI wants to merge 5 commits into
mainfrom
feat_list_count_columns

Conversation

@SoundMindsAI
Copy link
Copy Markdown
Owner

Summary

Adds a count column to two list tables, surfacing at-a-glance metadata operators previously had to click into each row to see:

  • /query-sets gains a Queries column (query_count) — how many queries are in each benchmark set.
  • /templates gains a Parameters column (param_count) — how many declared params each template exposes, i.e. its tuning surface (0 = non-tunable; 6–8 = rich search space).

Both are additive int fields on the existing list-summary schemas. No migration, no new product surface.

Why two different implementations

  • query_count counts child queries rows, so it uses a new batched GROUP BY aggregate (repo.count_queries_for_sets) — one query per page, NOT per-row. This is the same no-N+1 pattern repo.count_trials_for_studies established for the studies-list trial_count field (feat_studies_convergence_visibility, PR docs(research): complementary-architecture one-pager (three-layer handoff) #421). QuerySetSummary had previously omitted the count specifically "to avoid N+1 counts at list time"; the batched aggregate removes that objection.
  • param_count is len(declared_params) — free, because declared_params is a JSONB column already loaded on the template row (not a child relationship). No extra query, no N+1.

A subtle bug caught by mypy during implementation: the aggregate column is labeled query_count, NOT count — SQLAlchemy Row objects are tuple-like and expose a built-in .count() method, so row.count would resolve to the bound method rather than the labeled column. Commented inline.

Generated artifacts + guides

  • Regenerated ui/openapi.json + ui/src/lib/types.ts via scripts/regen-generated-artifacts.sh, so the frontend types pick up both new fields. The generated-artifacts-fresh CI gate (shipped PR infra(freshness-gate): CI gates for generated artifacts (copy-docs + openapi.json + types.ts) #433) validates the snapshot on this PR.
  • Regenerated in-app walkthrough guides 03 (create_query_template) + 04 (create_query_set) against a populated stack — the list-view screenshots now show the new PARAMETERS (values 1–3 across 7 templates) and QUERIES (5 per set across 5 sets) columns with real data. Walkthrough .webm videos promoted to match.

Test coverage

Layer File Cases
Backend integration backend/tests/integration/test_list_count_fields.py 5 (query_count: 3/0/per-set; param_count: 3/0 — real Postgres)
Backend contract backend/tests/contract/test_list_count_fields_contract.py 2 (both fields integer + required in OpenAPI)
UI vitest ui/src/__tests__/components/common/list-count-columns.test.tsx 7 (both columns: exists, value, zero, thousands-separator)

74 related existing backend tests + 1032 vitest still green; no exact-shape assertion broke.

Test plan

  • New backend tests (7) pass against real Postgres in the one-shot container
  • pnpm --dir ui test → 1032/1032; pnpm typecheck clean; pnpm build OK
  • make lint && make typecheck clean; ruff format --check clean
  • Freshness gate: canonical regen leaves git status clean (openapi.json + types.ts current)
  • Guides 03 + 04 regenerated on a populated stack (screenshots show new columns)
  • CI green (generated-artifacts-fresh + full suite)

🤖 Generated with Claude Code

SoundMindsAI and others added 5 commits June 3, 2026 06:15
Two cohesive list-summary additions (feat_list_count_columns):

1. /query-sets gains a "Queries" column showing query_count.
   - QuerySetSummary gains query_count: int (the schema previously
     omitted it "to avoid N+1 counts at list time").
   - New batched repo.count_queries_for_sets(ids) — one GROUP BY
     aggregate per page, NOT a per-row count. Mirrors the no-N+1
     pattern repo.count_trials_for_studies established for the
     studies-list trial_count field (feat_studies_convergence_visibility,
     PR #421). The list endpoint stays at a fixed 2 queries regardless
     of page size.
   - The aggregate column is labeled `query_count` (NOT `count`):
     SQLAlchemy Row is tuple-like and exposes a built-in `.count()`
     method, so `row.count` would resolve to the bound method, not the
     labeled column (caught by mypy).

2. /templates gains a "Parameters" column showing param_count.
   - QueryTemplateSummary gains param_count: int = len(declared_params).
   - Free to compute — declared_params is a JSONB column already loaded
     on the row (not a child relationship), so no extra query, no N+1.
     The full dict stays on QueryTemplateDetail.
   - Surfaces each template's tuning surface at a glance (0 =
     non-tunable; 6-8 = rich search space).

Regenerated ui/openapi.json + ui/src/lib/types.ts via the canonical
scripts/regen-generated-artifacts.sh, so the frontend types pick up
both new fields automatically — and the generated-artifacts-fresh CI
gate (shipped in the prior PR) validates the snapshot on this PR.

Tests (14 new): 2 contract (schema shape: both fields integer +
required), 5 integration (query_count: 3/0/per-set; param_count: 3/0
— verified against real Postgres in the one-shot container), 7 vitest
(both columns: exists, value render, zero, thousands-separator
formatting). 74 related existing backend tests + 1032 vitest still green.

No migration (Alembic head unchanged). No new product surface.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: SoundMindsAI <eric.starr@soundminds.ai>
Tangential discovery during feat_list_count_columns:
ui/public/guides/03_create_query_template/01-templates-list.png +
ui/public/guides/04_create_query_set/01-query-sets-list.png screenshot
the affected list views and will show old 4-column / 3-column tables
once the new query_count / param_count columns ship.

In-PR regen attempted on an empty DB produced empty-state screenshots
(net negative vs. previous populated-list illustrations). Reverted the
regen + filed this chore for a populated-stack regen pass. PR ships
unblocked; screenshots catch up later (3-5 min on a populated stack).

CLAUDE.md tangential rubric row justifying deferral: "Fix requires an
operator-environment change you can't make" — needs a populated demo DB
via `make seed-demo`, which is an operator decision.

Includes regenerated MVP2/roadmap dashboards (pre-commit hook).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: SoundMindsAI <eric.starr@soundminds.ai>
The operator brought up a populated stack (make up + make seed-demo:
5 query-sets × 5 queries each, 7 templates with 1-3 declared params),
so the in-PR guide regen the earlier empty-DB attempt couldn't do is
now possible.

Regenerated guides 03 (create_query_template) + 04 (create_query_set)
against the populated stack:
- 03/01-templates-list.png: shows the new PARAMETERS column with real
  values (3,2,2,1,1,2,2) across 7 templates.
- 04/01-query-sets-list.png: shows the new QUERIES column with 5 per
  set across 5 sets.
- The create-modal + detail screenshots in both guides regenerated in
  lockstep (the list shows behind the modal); all populated + current.

This obsoletes chore_guide_regen_after_list_count_columns (filed in
027ad78 when the DB was empty) — removed the idea folder since the
work it deferred is done here. The dashboard-regen hook drops it from
the MVP2/roadmap dashboards in lockstep.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: SoundMindsAI <eric.starr@soundminds.ai>
The prior commit (4bc7c3f) removed the obsolete
chore_guide_regen_after_list_count_columns folder but its dashboard-
regen hook didn't fire on the deletion, leaving DASHBOARD.md /
MVP2_DASHBOARD.md / *.html / website/docs/roadmap.md still referencing
the deleted folder. Regenerated all of them via `make dashboard` +
`scripts/build_public_roadmap.py` so the generated docs no longer point
at a folder that doesn't exist.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: SoundMindsAI <eric.starr@soundminds.ai>
The regenerated 03 + 04 guides (4bc7c3f) updated the screenshots but
the .webm walkthroughs were left stale (playwright test was run
directly, not via `pnpm capture-guides` which chains promote-videos.mjs).
Promoted the fresh recordings from this run's test-results so the
walkthrough videos match the populated-stack screenshots.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: SoundMindsAI <eric.starr@soundminds.ai>
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces list-summary count fields to optimize performance and surface metrics at a glance. Specifically, it adds a query_count field to QuerySetSummary (resolved via a single batched GROUP BY query to avoid N+1 queries) and a param_count field to QueryTemplateSummary (derived from the already-loaded declared_params JSONB column). Corresponding columns ("Queries" and "Parameters") are added to the frontend tables, accompanied by comprehensive contract, integration, and frontend tests. A review comment suggests using a fallback default dictionary when calculating param_count to prevent a potential TypeError if declared_params is None.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

version=row.version,
# declared_params is a JSONB column already loaded on the row, so
# len() is free — no extra query, no N+1 (see QueryTemplateSummary).
param_count=len(row.declared_params),
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If row.declared_params is None (which can happen if the JSONB column contains a NULL value in legacy database rows), calling len() directly will raise a TypeError: object of type 'NoneType' has no len(). To prevent potential 500 Internal Server Errors on the list endpoint, it is safer to use a fallback default dictionary.

Suggested change
param_count=len(row.declared_params),
param_count=len(row.declared_params or {}),

@SoundMindsAI
Copy link
Copy Markdown
Owner Author

Review adjudication (Gemini Code Assist)

Gemini Code Assist (1 finding)

# Sev Location Verdict Notes
1 Medium backend/app/api/v1/query_templates.py:116 Rejected len(row.declared_params or {}) guards against a NULL declared_params — but that state is unreachable. The column is nullable=False in both the ORM model (backend/app/db/models/query_template.py:38mapped_column(JSONB, nullable=False)) and the creating migration (migrations/versions/0003_study_lifecycle_schema.py:56sa.Column("declared_params", JSONB(), nullable=False)), and no later migration relaxes it (the column appears in exactly one migration). Postgres rejects a NULL at insert time, so row.declared_params is None cannot occur for a persisted row. Adding or {} would be a defensive fallback against an impossible state (contra the Bug Fix Protocol's "don't add defensive fallbacks that mask the real problem") and inconsistent with the sibling NOT-NULL _summary fields (row.name, row.engine_type, row.version), all accessed without guards. If a future migration ever relaxes the constraint, that migration is the right place to revisit this — not a speculative guard now.

Outcomes

All 17 CI checks green (including generated-artifacts-fresh, which validated the regenerated ui/openapi.json + ui/src/lib/types.ts). Ready for human review + merge.

@SoundMindsAI
Copy link
Copy Markdown
Owner Author

Final cross-model review (GPT-5.5) — adjudication

GPT-5.5 returned 2 findings, both rejected as false positives caused by the review input excluding the (large, bot-generated) ui/src/lib/types.ts from the diff for brevity:

# Sev Location Verdict Notes
1 High query-sets-table.column-config.tsx:43 Rejected (false positive) "column reads query_count but types.ts not updated." It IS updated — ui/src/lib/types.ts carries QuerySetSummary.query_count (committed in 1a817813, +23 lines). Proof: the CI frontend (… tsc …) job passed green, which is impossible if the field were missing (row.original.query_count would be a hard tsc error). generated-artifacts-fresh also green (types.ts matches the openapi.json snapshot). GPT-5.5 couldn't see types.ts because it was excluded from the diff input.
2 High templates-table.column-config.tsx:59 Rejected (false positive) Same cause + same counter-evidence for QueryTemplateSummary.param_count. tsc + generated-artifacts-fresh both green.

Outcomes (all reviews)

All 17 CI checks green. Ready for human review + merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant