design(434,435,438): RuleBuilder bundle spec — validation, relative-date operators, facet typeahead
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Claude Opus 4.8
@@ -0,0 +1,125 @@
|
||||
# RuleBuilder bundle — design (#438 + #435 + #434)
|
||||
|
||||
**Date:** 2026-07-23
|
||||
**Issues:** #438 (validation & polish), #435 (relative-date operators), #434 (facet-value typeahead)
|
||||
**Branch:** `feat/rulebuilder-bundle`
|
||||
**Left open:** #436 (deeper nesting — self-labeled YAGNI), #437 (inline RuleBuilder adoption)
|
||||
|
||||
The visual rule builder (`web/src/builder/rules/`, landed #176 / PR #433) compiles a visual rule tree
|
||||
to a Lucene query string and parses it back, with a lossless-round-trip contract. It has one consumer
|
||||
today: `CollectionsScreen.tsx` (SmartCollection create/edit, mounted at line ~347). Convention doc:
|
||||
`docs/spa-conventions.md` **§12** (not §11 — #437's citation is wrong and will be corrected when #437
|
||||
is worked).
|
||||
|
||||
## Slicing & workflow
|
||||
|
||||
- **One feature branch / one PR** closing all three (`fixes #438 #435 #434`). They share
|
||||
`RuleBuilder.tsx` (and #435/#438 share `types.ts`/`compile.ts`/`parse.ts`) too heavily to split
|
||||
without rebase churn, and read as one cohesive polish sweep.
|
||||
- **#434's backend endpoint is the only disjoint slice** — delegated to a parallel worktree agent
|
||||
(branched off the feature branch, merged back early) so its C# + OpenAPI regen runs while the
|
||||
frontend is built. **No parallel agents over the shared frontend module** (the one-file fan-out
|
||||
trap).
|
||||
- Everything else is **sequential, single committing agent** in the main worktree:
|
||||
**#438 → #435 → #434-combobox** (combobox last, after the endpoint's generated API types exist).
|
||||
- Independent adversarial review over the whole diff before push (new API endpoint + >~150 lines).
|
||||
Live-E2E on the SmartCollection screen for the round-trip.
|
||||
|
||||
## #438 — validation & polish (frontend only)
|
||||
|
||||
Confirmed current behavior (compile.ts):
|
||||
- `between` with empty `value2` emits `field:[v TO ]` (malformed). — compile.ts:45
|
||||
- empty `contains` → `field:**`; empty `startsWith` → `field:*`. — compile.ts:33–36
|
||||
- single-child group drops its connective (`join` over one element). — compile.ts:52–57
|
||||
- `isNot`/`notMatches` → `NOT field:"v"`, which matches nothing alone (no MatchAllDocs). — compile.ts:31
|
||||
|
||||
Fixes:
|
||||
1. **`between` requires both bounds.** A `between` rule with empty `value` or `value2` is *invalid* —
|
||||
surface a per-rule validation error and block compile/save. Never emit `field:[v TO ]`.
|
||||
2. **Empty text values are invalid.** `contains`/`startsWith`/`is`/`isNot` with an empty `value` are
|
||||
incomplete (not a wildcard) — same per-rule validation error; block compile/save. Never emit
|
||||
`field:*` or `field:**`.
|
||||
3. **Single-child group round-trips its connective.** Fix on the **parser** side: a compiled
|
||||
single-child group parses back to the builder's default `match` so a 1-child `{match:'any'}` no
|
||||
longer reopens as `'all'`. Output (compile) is unchanged; this is a parse-side alignment only.
|
||||
4. **Pure-negative top group → non-blocking warning.** When every child of the *top* group is negative
|
||||
(`isNot`/`notMatches`), show a warning (a `NOT`-only query matches nothing) and rely on the live
|
||||
preview count. **Do not block** — a positive branch nested deeper can legitimately rescue it, so a
|
||||
hard block would create false negatives. (Decision confirmed with user 2026-07-23.)
|
||||
|
||||
Validation surface: a per-rule "incomplete" flag rendered inline in `RuleBuilder.tsx`, and an
|
||||
aggregate "has-errors" boolean the consumer (`CollectionsScreen`) uses to disable Save. Compile of an
|
||||
invalid tree returns a sentinel (empty / null) rather than a malformed string.
|
||||
|
||||
Tests: extend `compile.test.ts` / `parse.test.ts` / `roundtrip.test.ts` for each case (cases 1–3 are
|
||||
hard assertions; case 4 asserts the warning predicate, not a block).
|
||||
|
||||
## #435 — relative-date operators (frontend only — backend macros already exist)
|
||||
|
||||
The Lucene macros exist in `ErsatzTV.Infrastructure/Search/CustomMultiFieldQueryParser.cs`:
|
||||
`released_inthelast` / `released_notinthelast` / `added_inthelast` / `added_notinthelast`, value form
|
||||
`"<n> day|week|month|year"` (unit substring-matched; negated from `DateTime.Today`). **No C# change** —
|
||||
they are absent from `SearchFieldCatalog` only because they are query-time synthetic fields. (There is
|
||||
no `inthenext` macro, so "in the next N" is out of scope — matches the issue.)
|
||||
|
||||
- **types.ts:** add operators `inLast` and `notInLast`, offered only on `date` fields (extend
|
||||
`OPERATORS_BY_TYPE.date`). Add optional `unit?: 'day' | 'week' | 'month' | 'year'` to `Rule` (cleaner
|
||||
than overloading `value2`, which is the `between` upper bound). `value` holds the integer N.
|
||||
- **RuleBuilder.tsx:** for `inLast`/`notInLast`, render a number input + a unit `<select>`.
|
||||
- **compile.ts:** map `{field, op, value:N, unit:U}` → `<synthetic>:"N U"` via a small mapping table
|
||||
`release_date → released`, `added_date → added`, and `inLast → inthelast` / `notInLast → notinthelast`.
|
||||
e.g. `{release_date, inLast, 7, day}` → `released_inthelast:"7 day"`.
|
||||
- **parse.ts:** recognize the four synthetic-field spellings `(released|added)_(inthelast|notinthelast):"<n> <unit>"`
|
||||
and map back to `{field: release_date|added_date, op: inLast|notInLast, value, unit}`.
|
||||
- The `release_date↔released` / `added_date↔added` + `inLast↔inthelast` mapping table is the single
|
||||
seam, shared by compile and parse.
|
||||
- Tests: extend `roundtrip.test.ts` + explicit compile/parse cases for each synthetic field and unit.
|
||||
|
||||
## #434 — facet-value typeahead
|
||||
|
||||
### Backend (parallel worktree agent)
|
||||
|
||||
- **New endpoint** `GET /api/v1/search/fields/{name}/values?q=&limit=` in
|
||||
`ErsatzTV/Controllers/Api/SearchController.cs`, thin `mediator.Send(...)` per the
|
||||
`SearchCollections`/`SearchArtists` pattern (SearchController.cs:85–99).
|
||||
- **New MediatR query + handler** under `ErsatzTV.Application/Search/Queries/`
|
||||
(`GetSearchFieldValues` / `GetSearchFieldValuesHandler`), following `GetSearchFieldCatalog*`.
|
||||
- **New `ISearchIndex` method** to enumerate distinct terms for a field with a case-insensitive prefix
|
||||
filter — implemented in `LuceneSearchIndex` (read the index reader's terms for the field). Cap at
|
||||
`limit` (default/max 50).
|
||||
- **Allow-list: `text`-type catalog fields only.** A field that is absent from `SearchFieldCatalog` or
|
||||
is not typed `text` returns **404** (bounds the surface; enum ships values inline, number/date/fulltext
|
||||
aren't typeahead targets). (Decision confirmed with user 2026-07-23.)
|
||||
- **Response DTO:** `SearchFieldValuesResponseModel { string[] Values }` in
|
||||
`ErsatzTV.Core/Api/Search/`, file-scoped `#nullable enable` (per `api.response-dtos`).
|
||||
- **OpenAPI:** regenerate `v1.json` / `v1.d.ts` / `endpoint-index.md` via `./scripts/update-openapi.sh`
|
||||
in the same diff (per `release.api-contract-ci-gate`).
|
||||
- **Tests:** handler test (allow-listed field returns filtered values; non-text/unknown field → the
|
||||
404 path); term-enumeration unit coverage.
|
||||
|
||||
### Frontend (sequential, after the endpoint's generated types exist)
|
||||
|
||||
- **`fieldCatalog.ts`:** add a `getSearchFieldValues(name, q)` client over the generated API.
|
||||
- **`RuleBuilder.tsx`:** for a `text` field's value input with a `contains`/`startsWith`/`is`/`isNot`
|
||||
operator, swap the `<input>` for an autocomplete combobox (debounced query to the endpoint). Purely
|
||||
additive — the emitted value and compile/parse are unchanged. Falls back to free-text when the
|
||||
endpoint returns nothing (so a typo still compiles, keeping the preview-count safety net).
|
||||
|
||||
## Docs updated in-PR
|
||||
|
||||
| Doc | Change |
|
||||
|---|---|
|
||||
| `api-conventions.md` | checklist entry for the new `search/fields/{name}/values` endpoint |
|
||||
| `v1.json` / `v1.d.ts` / `endpoint-index.md` | regenerated (#434 endpoint) |
|
||||
| `spa-conventions.md` §12 | relative-date operators (#435) + typeahead combobox (#434) behavior |
|
||||
| `docs/decisions.md` | record for the #434 distinct-values endpoint; record for the #435 relative-date field-mapping convention |
|
||||
| `docs/blazor-route-parity.md` | not required (no route change) |
|
||||
|
||||
## Out of scope / deferred
|
||||
|
||||
- **#436** (deeper group nesting): `compile.ts` already recurses arbitrarily deep; the cap is solely
|
||||
`parse.ts:121` passing `false`. Left open — self-labeled YAGNI until a real query needs it.
|
||||
- **#437** (inline RuleBuilder adoption in ChannelBuilder + Auto-Tune): larger integration + live-E2E,
|
||||
its own session. Note for then: cite `spa-conventions.md` **§12**, not §11.
|
||||
- **`does not contain` / date `is`**: these operators do not exist in the set and are not being added
|
||||
(only #435's `inLast`/`notInLast`).
|
||||
Reference in New Issue
Block a user