fix(685): suppress both empty-states on error; name the addable-kind derivation
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 11s
PR Gates / Docs update reminder (pull_request) Successful in 18s
Review verdict / Set review-verdict status (pull_request_target) Successful in 8s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 1m33s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 1m21s
review-verdict/h10 Review-verdict: MERGEABLE @ 7be4265 (base: main)
PR Gates / decisions lifecycle (pull_request) Successful in 2m7s
PR Gates / Script tests (pytest) (pull_request) Successful in 2m13s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 9m7s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 17m2s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 18m58s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 11s
PR Gates / Docs update reminder (pull_request) Successful in 18s
Review verdict / Set review-verdict status (pull_request_target) Successful in 8s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 1m33s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 1m21s
review-verdict/h10 Review-verdict: MERGEABLE @ 7be4265 (base: main)
PR Gates / decisions lifecycle (pull_request) Successful in 2m7s
PR Gates / Script tests (pytest) (pull_request) Successful in 2m13s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 9m7s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 17m2s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 18m58s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Round-4 verification returned MERGEABLE with no new defects and no BLOCKER/HIGH/MEDIUM. These are its remaining LOW and nits. - "No results — try a search above." asserts a search that COMPLETED and found nothing, so it was false beside a failed request. Both empty-state messages are now suppressed on error and the role="alert" banner is the whole message, as that test's comment already claimed. Pinned positively and negatively so a refactor cannot satisfy the assertion by rendering nothing at all. - The (typeof ADDABLE_TYPE_LIST)[number] derivation is now the named AddableKind, spelled once instead of twice: a third ingress into the searched kinds is most likely to be written by copying one of the existing two, and the "one list, all ingresses" property should be visible at a glance rather than reassembled. - The §3b lesson cited two measured test counts, which go stale against the very suite they describe — a count taken before the helper had unit tests no longer holds now that it does. Scoped the observation to the sha it was measured on and replaced the counts with the invariant they were evidence for: every gate needs at least one test that reddens when that gate ALONE is removed. - Documented why the error banner stays conditionally mounted while the hint's live region does not: role="alert" is the one live-region role screen readers reliably announce on insertion, so the two are correct for opposite reasons. The review flagged the divergence as unexplained, not as wrong. refs #740
This commit is contained in:
@@ -203,11 +203,12 @@ the #644 follow-up got Class A right and Class B only half right):
|
||||
not the screen — however single-sink the screen's own function looks.** #685 got this wrong
|
||||
twice in a row, and the second time is the instructive one: the check sat inside `runSearch`,
|
||||
which genuinely IS the one sink both entry paths route through, so it read as correct. It was
|
||||
still a duplicate of the helper's gate, and the two masked each other — with both present,
|
||||
deleting EITHER left the whole suite green, so the min-query boundary test pinned nothing.
|
||||
Measured after removing the screen's copy: deleting the helper's gate alone reddens that test
|
||||
and three more. A guard you cannot redden is not a guard, and "it's the single sink" is not
|
||||
evidence that it is the only one. **Outstanding on this screen**: `AddItemsDialog` still lacks the monotonic `seqRef`
|
||||
still a duplicate of the helper's gate, and the two masked each other: as of `4be3f247d` —
|
||||
which had no unit tests on the helper — deleting EITHER copy left the whole suite green, so the
|
||||
min-query boundary test pinned nothing. Removing the screen's copy is what made the helper's
|
||||
gate load-bearing. **The invariant, not the count: every gate must have at least one test that
|
||||
reddens when that gate ALONE is removed.** A guard you cannot redden is not a guard, and "it's
|
||||
the single sink" is not evidence that it is the only one. **Outstanding on this screen**: `AddItemsDialog` still lacks the monotonic `seqRef`
|
||||
stale-response guard and `useIsMountedRef()` — the same class of guard "Debounced typeaheads"
|
||||
below mandates there, applied to a debounced-while-typing fetch; `AddItemsDialog` is an explicit
|
||||
Search-button submission, not a typeahead, so that mandate doesn't reach it directly, but the
|
||||
|
||||
@@ -649,6 +649,12 @@ describe('CollectionsScreen', () => {
|
||||
expect(
|
||||
within(dialog).queryByText(`Type at least ${LIBRARY_PICKER_MIN_QUERY} characters to search.`)
|
||||
).not.toBeInTheDocument();
|
||||
// Nor "No results", which asserts a search that COMPLETED and found nothing. Both empty-state
|
||||
// messages are suppressed on error, leaving the alert banner as the whole message. Pinned
|
||||
// positively as well as negatively so a refactor cannot satisfy this by rendering nothing at
|
||||
// all (#685 review round 4).
|
||||
expect(within(dialog).queryByText('No results — try a search above.')).not.toBeInTheDocument();
|
||||
expect(within(dialog).getByRole('alert')).toHaveTextContent(/500/);
|
||||
});
|
||||
|
||||
/* ---------- add-items search bounds (#685) ---------- */
|
||||
|
||||
@@ -81,9 +81,15 @@ const ADDABLE_TYPES = new Set<LibraryBrowseItem['mediaType']>(ADDABLE_TYPE_LIST)
|
||||
// totalCounts against post-ADDABLE_TYPES rows. A non-addable kind here would overstate the hint
|
||||
// with every one of its rows dropped, and typing it as the whole mediaType union let that through
|
||||
// (#685 review round 3 — one of two ingress paths was enforced, which is not an invariant).
|
||||
const DEFAULT_SEARCH_KINDS: (typeof ADDABLE_TYPE_LIST)[number][] = ['Movie', 'TelevisionShow', 'Artist'];
|
||||
const DEFAULT_SEARCH_KINDS: AddableKind[] = ['Movie', 'TelevisionShow', 'Artist'];
|
||||
|
||||
type MediaKindFilter = 'all' | (typeof ADDABLE_TYPE_LIST)[number];
|
||||
// The single derivation every ingress into the searched kinds goes through. Named rather than
|
||||
// spelled inline at each use so the "one list, all ingresses" property is visible at a glance —
|
||||
// a fourth ingress is most likely to be written by copying one of the existing three (#685
|
||||
// review round 4).
|
||||
type AddableKind = (typeof ADDABLE_TYPE_LIST)[number];
|
||||
|
||||
type MediaKindFilter = 'all' | AddableKind;
|
||||
|
||||
const MEDIA_KIND_FILTERS: { label: string; value: MediaKindFilter }[] = [
|
||||
{ label: 'All', value: 'all' },
|
||||
@@ -382,6 +388,10 @@ function AddItemsDialog({
|
||||
“All” searches movies, shows and artists; pick a specific kind to add seasons, episodes,
|
||||
music videos, songs, other videos, images or remote streams.
|
||||
</p>
|
||||
{/* Conditionally mounted, unlike the aria-live hint below — deliberately, not by oversight:
|
||||
`role="alert"` is the one live-region role screen readers reliably announce on INSERTION,
|
||||
so mounting it together with its content is right here and would be wrong there (#685
|
||||
review round 4 flagged the divergence as unexplained). */}
|
||||
{error && (
|
||||
<span className="ctv-field-error" role="alert">
|
||||
{error}
|
||||
@@ -412,7 +422,10 @@ function AddItemsDialog({
|
||||
<div className="ctv-collections-picker-empty">
|
||||
Type at least {LIBRARY_PICKER_MIN_QUERY} characters to search.
|
||||
</div>
|
||||
) : results.length === 0 && !searching ? (
|
||||
) : results.length === 0 && !searching && !error ? (
|
||||
// Also suppressed on `error`: "No results" asserts a search that COMPLETED and found
|
||||
// nothing, which is false when the request failed. The role="alert" banner above is the
|
||||
// whole message in that state (#685 review round 4).
|
||||
<div className="ctv-collections-picker-empty">No results — try a search above.</div>
|
||||
) : (
|
||||
results.map((item) => {
|
||||
|
||||
Reference in New Issue
Block a user