feat: do not offer filter sections that cannot filter anything (#910) - #935
Open
Deeds67 wants to merge 40 commits into
Open
feat: do not offer filter sections that cannot filter anything (#910)#935Deeds67 wants to merge 40 commits into
Deeds67 wants to merge 40 commits into
Conversation
The /search page built its request from the URL terms alone, so a URL-driven search covered only the caller's own (and partner) assets. "Show similar photos" on a photo shared through a Space navigates here with just a queryAssetId, which meant the space owner got a full list of matches and every other member got none for the same reference asset. Send `withSharedSpaces: true` from the page, matching what the command palette already does for global search (#894) and what mobile already sends on its smart-search path. The flag is spread before the URL terms so an explicit value still wins, and is skipped when the terms carry a `spaceId` — the server rejects the two together. The server already resolves the reference asset through space membership and carries the space scope onto the queryAssetId path; added a unit test covering that combination so the half the fix relies on stays pinned.
…acet-failure sentinel, {#key} invariant)
Eight slice plans plus a spec correction: feedback_no_dynamic_rating_media_hiding records a standing decision against per-star and per-button dimming, which the first draft would have reversed. #910 is satisfied at section level instead.
Corrects three design-level defects and a set of plan defects found reviewing the plans against the codebase: - media availability was `mediaTypes.length < 2`, but the server returns raw distinct asset.type and AssetType includes AUDIO/OTHER, so a photo library with one OTHER asset passed with a dead Videos button. Now requires both IMAGE and VIDEO. - the baseline was to be a second suggestionsProvider(createFilterState()) call. The three query-mode pages keep a single smartFacetInFlight slot and abort on key mismatch, and their .then writes smartFacets, which feeds the timeline and the result count -- so that call would abort the real request and then render unfiltered data under active filters. Replaced with a config.baselineProvider hook that returns undefined in query mode. - mobile had no baseline, so cross-section narrowing would pop sections in and out; it now shares the web model via the same family provider under SearchFilter.empty(). Its own null-response sentinel becomes a throw. Also: e2e scope was three suites short (map seeds nothing; recently-added asserts all ten sections) and People cannot be seeded without ML, so those assertions invert; every pnpm test command dropped its path filter via a stray `--`; make sql/open-api/lint-server and pnpm sync:open-api do not exist; and slice 5 carried an assertion that cannot pass (objectContaining with an absent key) plus a tautological baseline-rejection test.
Restore the albumIds suppression guard on the smart-search path's isNotInAlbum/isInAlbum re-application (both sibling implementations keep it), pin that the favourites fix did not over-correct total, reword a test comment that pointed at a scratch review workspace, add tag-filter coverage for the hasFavorites "other dimensions honoured" case on both the browse and smart-search paths per spec §8.1, and correct the plan's query count for the Task 6 sortQueries hints.
…decisions (#910) The no-dimming guard passes on first write by design: it locks feedback_no_dynamic_rating_media_hiding through the gating work in the next commit. The guard test needed one addition beyond the brief's literal snippet: an explicit `vi.advanceTimersByTimeAsync(0)` after render, matching every other test in this file. @testing-library's waitFor only self-advances Jest fake timers (it probes a global `jest`), not Vitest's vi.useFakeTimers(), so without it the test hangs for real until its own 5s timeout. currentSuggestions/baseline/baselineRequested are captured but genuinely unread until the next commit wires them into getSectionAvailability, so each declaration carries a scoped eslint-disable for no-unused-vars rather than being deferred to that commit.
…910) The photos filter panel suite assumed every section always renders. Slice 5 correctly hides sections with nothing to filter, so seed a video, GPS/camera EXIF, a tag, a favourite, and a partial album membership alongside the existing dated images and rating — one asset per section the suite exercises. People is the one exception: it needs a detected face, and the web e2e project runs no ML, so a bare API-created person never satisfies the facet. The "all sections" assertion now asserts People's absence instead, which is positive #910 coverage (a library with no faces genuinely cannot filter by person), not a workaround. Also adds direct coverage for the hide/reveal behavior itself: a fresh, unseeded library hides every gated section but keeps timeline/text, then favouriting an asset reveals Favorites and uploading a video reveals Media.
…910) createPopulatedSpace only ever seeded four plain images, so slice 5 hides almost every section (~25 rating/media control clicks in this suite target elements that no longer render). Extends the shared helper with the same recipe as the photos suite — video, GPS/camera EXIF, a tag, a rating, and a partial album membership — added to the *space*, not just the library, since that's what the space-scoped facets are computed over. Rates both the plain image and the video (not just one): rating only an image means a rating filter narrows the space to a single media type, which correctly (per #910) empties the Media facet mid-interaction and detaches media-type-image/-video while a combined rating+media test is still clicking it. Rating a video too keeps at least one of each type in every rating-filtered view. People needs a real face, which this ML-less e2e project cannot seed (same reasoning as the photos suite). Fixes all four People assertions that relied on a visible people-section/list in this file — the "all sections" list, plus three individual "People filter" tests with unconditional visibility checks the earlier plan didn't enumerate — to assert the section's absence instead. One more assertion changes for the same underlying reason but a different section: "should show empty messages in location and camera sections" relied on the space having zero EXIF data, which is no longer true once createPopulatedSpace is enriched. Gives it its own bare, un-enriched fixture and asserts the sections are absent (slice 5 hides them outright) rather than showing an "-empty" placeholder inside them. Also fixes "should grey out years with zero photos after filtering": filtering to zero results correctly collapses the temporal picker's content (the same greyed/collapsed/disabled treatment every emptied-by-current-filter section gets), not just "zero height" as the pre-#910 test assumed. Asserts the section wrapper stays visible (Timeline is never hidden outright) and the content is correctly absent.
Recently Added already seeds images, videos, and ratings, so media and rating were unaffected. Swaps two of the already-seeded images into real GPS/camera EXIF uploads (rather than adding new assets, which would have drifted the TOTAL/RATED/VIDEOS constants several other assertions in the file pin exactly), and tags/favourites/partially-albums three more of them. People gets the same carve-out as the other suites: the ten-section assertion now asserts nine sections plus People's absence. Map seeded nothing at all, so every gated section (including favorites and location, the two the suite actually tested) was unreachable. Adds a favourited asset and a GPS upload to the existing beforeAll, and adds a new "empty library" describe ahead of it — its own reset, no seeding — asserting the empty-library panel renders only timeline/text and hides the other eight sections, since an empty map is the cheapest place in this repo to prove the hide behavior end to end.
Test 9 waited unconditionally for filter-section-camera while its own assertion two lines below was already guarded on exifInfo?.make. When make is absent, slice 5 correctly hides the (now-empty) camera section and the unconditional wait timed out. Applies the same guard to the wait.
…910) Slice 2 made hasFavorites/hasAssetsInAlbum/hasAssetsNotInAlbum required DTO fields; 15 mobile test files still constructed the DTO without them. Mechanical fixture repair, no assertions changed.
sectionAvailabilityProvider is the mobile analogue of web's
filter-availability.ts: a deep-sheet section is hidden only when its
facet is empty in BOTH the current filter and a same-family baseline
request taken with no filters, and it holds no active filter of its
own. The baseline is what stops cross-section narrowing (e.g. a
person with no rated photos) from popping the Rating section in and
out mid-session, and costs no extra request in the common case since
SearchFilter.empty() == an already-empty filter serves the same
cached future.
The media rule stays contains('IMAGE') && contains('VIDEO'), never a
length check, since AssetType also includes AUDIO/OTHER.
Review found the "People carve-out" premise false: utils.createFace and utils.createSpacePerson seed a face without ML, so People is seedable like every other filter section. Restores the three "all sections" lists to include people/favorites/albums/text, and restores the three space People tests that had been weakened into duplicate absence assertions back to real coverage of space-vs-global scoping, person-select-updates-timeline, and OR-logic selection. Also fixes: a landmine assertion that asserted the temporal picker stays attached after a filter combination that actually empties it (contradicting the sibling "collapses" test); a greying assertion the "collapses" test had dropped; two rating tests left vacuous once a 3-star asset was seeded (cameraAsset's own embedded EXIF rating means 3 assets clear >=3, not the 2 the seed explicitly rates — corrected the comments and asserted concrete counts); and two pre-existing flaky force-clicks in the full 86-test run, root-caused by diffing against the pre-fix file (both reproduce identically there) and fixed by awaiting the triggering bucket refetch before the click. utils.createFace/createSpacePerson now insert their asset_face row with sourceType 'manual' instead of leaving it at the schema default of 'machine-learning' — on any stack that runs facial recognition (e.g. a `make dev` stack, unlike the e2e project's own ML-less compose), the detect-faces job otherwise treats the manually-seeded face as a stale ML detection and deletes it a few seconds after creation.
The plan claimed People could not be seeded in the web e2e project and sanctioned an absence assertion instead. That was wrong: utils.createFace and utils.createSpacePerson seed a face/space-person without ML. Replaces "The People carve-out" section with "People is seedable too", updates the recipe table and the Task 1-3 People steps to seed-and-assert, and corrects the "Done when" bullet that claimed only four assertions changed meaning.
…es a baseline (#910) Two spec-prescribed defects from the slice 7 review: the facets lookup for section availability and the two gated toggle switches keyed on the raw filter instead of the debounced one, bypassing the 250 ms debounce every other deep-sheet consumer shares. And the two gated switches (Favourites, Not-in-album) consulted only the current facets with no baseline, so a cross-section filter (e.g. selecting a person with no favourites) could pop a switch out mid-session — the exact behaviour the section-level baseline exists to prevent. Both call sites now watch photosFilterDebouncedProvider for the facets key while keeping the immediate photosFilterProvider for active-filter reads. A new baselineFacetsProvider centralises the whole-scope baseline fetch so sectionAvailabilityProvider and TogglesSection share one request instead of each computing their own, and a toggleAvailable() helper generalises the section-level "empty in both current and baseline, and no active filter" rule down to a single boolean facet.
…e timeline rule The carve-out premise was wrong: utils.createFace inserts a face row in SQL with no ML, so People is seedable like every other section. The plan was corrected during slice 6; the spec still carried the false rationale and it ships in this branch. Also footnotes the 4.3 timeline row, which read as an unconditional 'greys when empty' and contradicted 4.4 rule 1 (an active filter always wins). The code takes rule 1; the table now says so.
github-actions
Bot
added
documentation
…ed fields (#910) baselineFacetsProvider used SearchFilter.isEmpty to decide whether the baseline key can reuse the main facets key, but isEmpty also counts fields the facets request never forwards (search-bar context chief among them). Whenever only one of those was set, the two keys diverged even though the outgoing request would be byte-identical, firing a wasted second request. Narrow the check to the fields filter_suggestions.provider.dart actually sends.
…facets request (#910) toSuggestionRequest() dropped both album-membership filters, so setting the Albums filter in the asset picker (which renders it, unlike album detail) computed every other section's availability against an unfiltered asset set instead of the actually-filtered one. Forward them the same way the photos page does. Both predicates are inert inside album detail (asset.repository.ts guards them with !options.albumId), so this is safe for both callers.
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
1 participant
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.
Closes #910.
A filter section is no longer offered when using it cannot change what you see. A section that is empty for the whole scope regardless of filters is hidden; one that is empty only because of the filters currently applied keeps today's treatment — 50% opacity, a
(0)suffix, collapsed and disabled.All eight enumerable sections take the rule (People, Location, Camera, Tags, Rating, Media, Favorites, Albums), on web and mobile. Timeline and Text are exempt.
Heads-up for reviewers
A fresh library will show fewer sections than before. With no faces named, no GPS, no tags, no albums and no favourites, the panel shows Timeline, People (the unnamed-faces hint), Media and Text only. That is the intended behaviour — the hidden sections genuinely cannot filter anything — but it is a visible change and worth knowing before you open the panel on a clean install.
Base branch. This targets
fix/797-filter-section-ledger(#926), notmain. #926 rewrites the{ selected, known }section ledger in the same files slice 5 edits heavily, so developing both againstmainwould have meant a 162-line conflict. Retarget tomainonce #926 merges.What changed
Server — three new facets on both filter-suggestion paths, each computed with its own filter excluded:
hasFavorites,hasAssetsInAlbum,hasAssetsNotInAlbum. Two pre-existing defects fixed along the way:isFavoritewas applied unconditionally in the smart-facet builder while every other dimension was guarded, and the album predicates were baked into the smart-search candidate temp table so no facet could exclude them.Web — a pure rule module (
filter-availability.ts) turns facet data into one of three verdicts, and the panel gates both the section body and its toggle icon. Availability is derived and never written to the persisted section ledger, so a section going unavailable is not recorded as user-hidden and returns on its own when the library gains its first video.Mobile — the same model, minus the greyed state (mobile has no
(0)treatment, so it hides or shows).Two failure sentinels removed. Both clients fabricated an all-empty facet response when the request failed. Harmless before; under this feature it is indistinguishable from an empty library and would hide every section at once. Both now reject, so the panel keeps its last good facets.
Notes on two decisions
Rating stars and media buttons are untouched. Their facets are read to decide whether the section is offered, and never reach the controls. All five stars and all three buttons always render. Filtering them made the stars positional liars — with
[1,2,3,5]available, clicking the fourth visible star selected rating 5 (#261).The baseline is a separate
baselineProviderhook, not a secondsuggestionsProvidercall. The three query-mode pages keep a single in-flight facet slot and abort on key mismatch, and their resolve handler writes the state that drives the timeline and the result count — so a concurrent unfiltered request would both kill the real one and render unfiltered data under active filters. Query mode returns no baseline, which means it greys rather than hides; that is the safe direction.Incidental fix
asset_face.sourceTypedefaults tomachine-learning, so on any stack with facial recognition enabledhandleDetectFacestreated a manually-seeded face as a prior detection, re-ran detection, found nothing in a synthetic fixture, and deleted it. Faces seeded byutils.createFace/utils.createSpacePersonvanished seconds after creation. Both helpers now insertsourceType: 'manual', matching what the app's own manual-face endpoint writes. This affects 22 call sites across the e2e suites.Verification
dart analyze --fatal-infos0 · dart formatE2E ran against
:3000rather thanmake e2e-web-dev's hardcoded:2283, which serves zero-length bodies for non-API routes on a dev stack. Same application source; CI serves a production bundle, so treat that as "seeds and assertions are correct" rather than "CI is green".Known follow-ups (not blocking)
isInAlbum/isNotInAlbumto its facets request. The other five web surfaces do.SearchFilter.isEmptycountscontext, but the facets request never forwards it.withSharedSpaces: trueunconditionally, so the per-user-favourites scope reasoning in the design does not transfer there.Design:
docs/superpowers/specs/2026-08-04-interdependent-filter-sections-910-design.md