V2.4.1 Improve code quality, add list thumbnail, improve UI, share to others, add generic import, add Playwright tests - #468
Merged
Merged
Conversation
┌────────────────────────────────────────┬────────────────────────────────────────────────────────────────────────────────────────────┐ │ Fix │ Verification │ ├────────────────────────────────────────┼────────────────────────────────────────────────────────────────────────────────────────────┤ │ S2365 — PlaylistDetail.razor rename │ PlaylistSmokeTest passed against a real browser + real Blazor Server circuit │ ├────────────────────────────────────────┼────────────────────────────────────────────────────────────────────────────────────────────┤ │ ASP0025 ×2 — AddAuthorizationBuilder │ ReferenceDataAdminResourceTest (AdminOnly) + PlaylistResourceTest/BookResourceTest │ │ │ (MemberOnly) — 9/9 passed over real HTTP with real Firebase auth │ ├────────────────────────────────────────┼────────────────────────────────────────────────────────────────────────────────────────────┤ │ CA1862 ×2 — │ AlbumReferenceRepositoryTest/BookReferenceRepositoryTest — 7/7 passed against real MongoDB │ │ StringComparison.OrdinalIgnoreCase │ │ ├────────────────────────────────────────┼────────────────────────────────────────────────────────────────────────────────────────────┤ │ CA1859 ×3 — concrete return types │ 235/235 unit tests passed │ ├────────────────────────────────────────┼────────────────────────────────────────────────────────────────────────────────────────────┤ │ JS S2486 — logged exception │ No test infra exists for this file — still an honest gap, noted in the docs │ └────────────────────────────────────────┴────────────────────────────────────────────────────────────────────────────────────────────┘ docker itself isn't on PATH in either shell here (confirmed again), but Test-NetConnection -Port 27017 proved Mongo was reachable, which was enough to run everything through it directly. Updated docs/code-quality-findings.md with the real test results (replacing the earlier "couldn't verify" caveat), and saved a memory note on the working recipe for WebApi.IntegrationTests (not just Playwright) via Local.runsettings env-var loading + --filter-class, since that'll save time next session.
Every image-bearing list page (Movies, TV Shows, Books, Albums, Video Games, Cars, Houses, Health Profiles, Gear, Collectibles) gains a list/thumbnail toggle in the search bar. The thumbnail view is a responsive poster grid that leverages each type's cover art (portrait posters, square album art, wide game imagery) with title + meta captions. View mode lives in the URL as ?view=grid, following the existing search/sort/filter URL-state convention: bookmarkable and restored on back-nav. It is deliberately kept out of the query signature so switching views never refetches, and unlike a filter it does not reset the page. Playlists (no cover art) stays list-only. MobileScreenshotTest gains a cover-art showcase seed and grid captures at both phone (390x844) and desktop (1280x900) viewports, verified visually. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WvLYeTtX526Y5QZqrrz1o6
Close the top UI-coverage gap from the new testing assessment: the TV Time, Amazon, and generic video-game import pages had API-level coverage but were never driven through the browser. - Support/*FixtureCsvBuilder + TvTimeImportFixtureZipBuilder: minimal GUID-suffixed in-memory fixtures so every run imports a genuinely new item it then deletes (no dedup-hidden "already imported" rows, no accumulation). - Pages/ImportPage + Amazon/GenericVideoGame page objects, plus a PageBase.OpenImportAsync nav helper. - End2EndFixture.GetItemIdsAsync: one reusable list-query helper all three tests use for API cleanup (books, video games, tv-shows, episodes). - docs/testing-assessment.md: the assessment itself, with these gaps marked closed. All three pass in self-hosted mutating mode. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PxyaxubjRzm2PSLMz7AE24
Covers the admin page's provider-free, deterministic surfaces (recommendation #2 from the testing assessment): page load via the Admin nav link, the System status panel resolving, unresolved-queue type switching, and the export -> import round-trip (idempotent upsert-by-id, so it changes no data). The provider search/link flow is deliberately left to the per-type detail-page smoke tests (same endpoints, real providers), and a full sync-now poll is left out as known-flaky on provider latency. Passes in self-hosted mutating mode. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PxyaxubjRzm2PSLMz7AE24
Close the last genuine coverage gaps from the assessment (#4 account page, #5 user-preferences UI) - Manage.razor previously had no test at any level. ManageAccountSmokeTest asserts the signed-in identity renders, then round-trips a preference toggle through the real UI (toggle -> persisted -> a fresh load reflects it) and restores the original value via the API so the shared account is left unchanged. The toggle re-clicks through the prerender->interactive gap the same way ClickUntilAsync does for buttons. Also records in the assessment doc why per-type Quick Add scenarios (original recommendation #3) were withdrawn: they would duplicate coverage the suite already provides, which the quality bar rejects. Passes in self-hosted mutating mode. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PxyaxubjRzm2PSLMz7AE24
- Inset the grid from the panel edges (was flush): 1rem desktop, 0.75rem mobile, matching the list rows' horizontal padding. - Size grid columns per cover shape so wide (16:9) tiles are no longer tiny strips: wide uses a 260px min (3 large columns on desktop, 2 on mobile) for video games/cars/houses/health/gear/collectibles; square (albums) 160px; portrait unchanged. The grid container now carries its ItemImageShape class so the CSS can target it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WvLYeTtX526Y5QZqrrz1o6
Space the last row of grid items off the card's bottom border (1.5rem desktop, 1.25rem mobile); the grid previously had no bottom padding so items sat flush against the panel edge. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WvLYeTtX526Y5QZqrrz1o6
Fixed the problem was in the shared DateTimeFields.razor time input (used by both the health record form and car-history form). It's a free-text HH:mm field with inputmode="numeric", so the phone's numeric keypad shows digits but no : key, making the colon impossible to type.
The owner deliberately chose free-text over the native <input type="time"> picker to guarantee 24h display (documented in the component and CLAUDE.md), so rather than override that decision, I made the colon optional on input:
- SetTimeTextAsync now also accepts bare digits — 1430 → 14:30, 930 → 09:30 — via a new TryParseTime helper, while still accepting 14:30 typed on a desktop keyboard.
- The field reformats to the canonical HH:mm on blur, so the stored/displayed value is unchanged.
- Loosened the pattern to allow the optional colon and added a title hint ("Type HH:mm, or just the digits").
- An unparseable entry is still ignored, leaving the previous value untouched — same behavior as before.
Because it's the shared component, car-history time entry on mobile benefits from the same fix.
The view choice is now remembered instead of defaulting to list on every page. It is a global user preference (like a theme), not list state: it never changes which items show or their order, only their presentation. Stored per-device in localStorage and mirrored in a new circuit-scoped ListViewPreference so every list page in the session shares one choice. The app renders InteractiveServer over a WebSocket circuit, so in-app navigation carries no fresh HttpContext to read a cookie from - hence localStorage (read once per circuit on first interactive render, since it is not reachable during prerender) rather than a server cookie. Chose per-device deliberately: thumbnails suit a desktop while a phone may prefer the compact list. The ?view= URL parameter is removed; toggling now re-renders in place (no navigation, no refetch) and writes the preference. MobileScreenshotTest drives the captures via the toggle + localStorage instead of ?view=, which also verifies the preference carries across pages and full reloads. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WvLYeTtX526Y5QZqrrz1o6
These two pages build their own layout (not InventoryList) but render the same media rows, so they gained the same view toggle and poster grid. To keep one copy of the logic, the toggle's seed/persist plumbing moved into a shared ListViewToggle component (used by InventoryList too, which no longer inlines it or the seeding in InventoryPageBase), and the poster card moved into a shared ItemGridCard used by all three grids. The card's caption render-fragment is named MetaContent, not Meta, because <Meta> collides with the HTML <meta> void element in Razor. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WvLYeTtX526Y5QZqrrz1o6
Fixed. The Reference field in the owned-copy editor (OwnedVersionFields.razor) was misaligned because when an Amazon ASIN is detected, the "Open on Amazon" link renders as a .kt-icon-btn that's 2.25rem (~36px) tall — taller than the ~30px input beside it. The flex row wrapping them used align-items-center, which vertically centered the shorter input inside the taller row, dropping it a few pixels below the Price/Acquired/Vendor inputs in the same row. Switching that row to align-items-start top-aligns the input so it lines up with its siblings again.
Root cause: In the thumbnail (grid) view, the delete button lived inside .kt-grid-cover. On hover, that element gets transform: translateY(-3px) (app.css:479), and a transform creates a new stacking context. That trapped the button's z-index: 2 within the cover's context, so it could no longer sit above the card-level Bootstrap stretched-link (z-index: 1). The link painted over the whole cover — button visible, but the click landed on the link. The list-row view avoids this because its delete button is a direct sibling of the stretched-link with no transformed ancestor between them. Fix: Moved @actions in ItemGridCard.razor out of .kt-grid-cover to be a direct child of .kt-grid-card — the same stacking context as the stretched-link — so its z-index: 2 genuinely wins. No CSS change needed: .kt-grid-delete is position: absolute; top/right, which now anchors to .kt-grid-card (still position-relative) and stays in the same top-right corner over the cover.
Before: three loose stacked rows (state buttons, a bare fully-completed toggle, playthroughs) with unlabelled date pickers appearing inline with no context. After: - A <hr> divider separates the shared owned-copy fields from the game-specific progress controls, so the two areas read as distinct groups. - State and Completion now sit side-by-side in a two-column row (col-md-6), each under an uppercase form-label matching the rest of the app's fields. On mobile they stack. - The bare date pickers get contextual muted hints — "Completed on" and "on" (kt-card-meta) — so a lone date box is no longer mysterious. Widened to 170px so full dates aren't cramped. - A second <hr> sets off Playthroughs, which now shows a "No playthroughs recorded yet." empty state instead of just a bare "+ Add" button, and its remove ✕ got an aria-label for parity with the other remove buttons. Everything reuses your existing design tokens (form-label, kt-card-meta, .kt-icon-btn, Bootstrap grid), so it's consistent with the other detail pages.
Personal (car/house/health) sharing — list and read-only detail (your key distinction: media = list only; personal = list + full detail with history/metrics/charts).
Backend (SharedWithMeController)
- GET /{shareId}/{cars|houses|health-profiles} (list) and GET /{shareId}/…/{itemId} (parent + full child history + computed metrics), via two generic helpers (ReadPersonalListAsync, LoadSharedParentAsync) — grant resolution stays the single security choke point. Metrics reuse the existing static Car/House/HealthMetricsService. No copy routes for personal categories (view-only, enforced by absence + the classifier).
- Contracts: one generic SharedDetailDto<TParent,TChild,TMetrics> (+ OwnerDisplayName for the breadcrumb).
Recipient UI — reuses the real detail pages (as you steered, "it takes an id to load data")
- CarDetail/HouseDetail/HealthProfileDetail gained a ShareId param → CanEdit => ShareId is null. When set, they load from the ownership-scoped shared endpoints (the one wrinkle: /api/cars/{id} is scoped to the caller's own user_id, so a recipient would 404), and every edit affordance is gated off. Three thin route wrappers own /account/manage/shared/{shareId}/…/{id} + MemberOnly auth, leaving the owner pages' own route/auth untouched.
- History rows gained ReadOnly (hides edit/delete). SharedCollectionPage gained Cars/Houses/Health tabs → new generic SharedPersonalList. Breadcrumb reads Shared with me › <owner> › <item> per your note.
Owner UI — SharingPage now has a Personal group; Health requires an explicit confirm before it can be enabled (never bundled).
The shared personal list (SharedPersonalList.razor) was a plain <a>-per-row list — no covers, no thumbnails, no grid, nothing resembling the media tabs. It now reuses the same InventoryList component the owner's own Cars/Houses/Health pages use, so it gets cover thumbnails, the list/grid toggle, search, sort, and per-type meta lines for free. Concretely: 1. InventoryList gained an optional DetailHref selector. Its read-only mode was built for the media shared list, where rows deliberately don't navigate anywhere. Personal items do have a full read-only detail page, so when DetailHref is set, read-only rows and grid cards link to it (media rows, with no DetailHref, are unchanged). 2. SharedPersonalList now wraps InventoryList in read-only mode with cover thumbnails (ImageUrl, wide shape, matching the owner pages), the per-type meta row, and detail links. Since a person has only a handful of cars/houses/profiles, the set is fetched once and search/sort run client-side over it (no server paging needed). 3. Extracted CarMetaRow / HouseMetaRow / HealthMetaRow into Components/Inventory/Meta/, mirroring the existing MovieMetaRow/etc. pattern, and pointed both the owner list pages and the shared list at them — so the meta line is defined once, not duplicated between the two views (per the no-duplication quality bar). 4. SharedCollectionPage passes the image/meta/detail params for the three personal tabs; removed the now-dead ItemSubtitle/CarSubtitle code. Both BlazorApp and the Playwright test project build clean with zero warnings. The existing SharedCollectionViewPage locators (.kt-item-row, row click) still work — the new rows keep that class and the stretched-link makes the whole row clickable, and the default view is the list view. One note: the grid/list toggle in this shared view respects the same circuit-wide ListViewPreference as every other list page, so if the recipient has switched to grid elsewhere, the shared personal tabs will show cover cards too — consistent with the media tabs' behavior.
Domain
- ShareCategory (Domain + Contracts): added Collectibles and Gears.
- ShareKind: added a third value Collection — a view-only list — alongside Media (copyable list) and Personal (view-only detail). ShareCategoryClassifier maps the two new categories to Collection, and IsCopyable stays the single rule KindOf == Media, so they're non-copyable.
Backend (SharedWithMeController)
- Injected the collectible/gear repos + DTO mappers.
- New generic helper ReadOwnedListAsync<TModel,TDto> — the media read path minus reference-image hydration and copy-dedup (constraint is plain IHasId, not IReferenceLinkedDto), still routing through the one ResolveGrantAsync security choke point.
- GET /{shareId}/collectibles and GET /{shareId}/gear. No copy routes. Search/sort/favourite/owned filters work via the existing repo GetFilters.
Recipient UI
- SharedCategoryList.Copy is now optional — when null, no per-row "add" action renders; it's the same full InventoryList (search/sort/filters/thumbnails/grid) but purely read-only.
- SharedCollectionPage gained Collectibles/Gear tabs.
- Extracted CollectibleMetaRow.razor / GearMetaRow.razor and reused them in the owner list pages too (same meta-extraction pattern as the media rows).
Owner UI
- SharingPage gained a third "Collections (view-only)" group; SharingLabels maps Gears → "Gear".
Tests
- ShareCategoryClassifierTest: added a Collection-kind/never-copyable theory.
- ShareResourceTest.CollectionShare_IsReadableAsAFilterableList_ButNeverCopyable: paged/searchable/favourite-filtered read, empty AlreadyInCollectionIds, out-of-scope 404, and copy route absent.
What was happening The shared collection's tabs were rendered straight from share.IncludedCategories, whose stored order reflects the order the owner clicked the category buttons when creating the share. That's because SharingPage.razor builds the grant from a HashSet<ShareCategory> (IncludedCategories = [.. _selected]), so click order leaks through to storage and then to the recipient's tabs. Fix Added a single canonical ordering helper, SharingLabels.Ordered(...), whose order matches the left nav menu (NavMenu.razor): Movies, TV shows, Books, Albums, Video games, Cars, Houses, Health, Collectibles, Gear. (Note the enum's own declaration order differs — it lists Collectibles/Gear before Cars/Houses/Health — so sorting by the enum wouldn't have matched the menu.) Applied it everywhere categories are displayed, so nothing depends on the stored click order anymore: - SharedCollectionPage.razor — the recipient's tab bar, and the default-selected tab fallback (so the landing tab is the first one shown). - SharingPage.razor — the owner's "Active shares" category summary. - SharedWithMeListPage.razor — the recipient's "shared with me" list summary. The stored data is untouched (categories are stored by name), this is purely a display-order change. One note: SharingLabels lives in BlazorApp, which has no unit-test project (tests are WebApi/integration/Playwright only), so the ordering helper isn't covered by an automated test — it's straightforward display logic.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The wishlist wasn't showing books' custom images because WishlistController.BuildWishlistAsync only hydrated the cover from the linked reference document, never applying the tenant-owned CustomImageUrl override that books (and video games) carry. The override logic (hydrate reference cover, then let a non-empty CustomImageUrl win) was already duplicated in four places — BookController, VideoGameController, AlbumController, and a private helper in SharedWithMeController — and simply missing from the wishlist. Rather than add a fifth copy, I: - Promoted the helper into the shared ReferenceImageHydrator.HydrateWithCustomOverrideAsync (the one place that already owns the reference-image batch-lookup logic). - Routed every call site through it: the three CRUD controllers' OnListMappedAsync, SharedWithMeController's three reads (removing its now-redundant private helper), and — the actual fix — the wishlist's Books and VideoGames. Movies and TV shows in the wishlist stay on the plain HydrateAsync path since neither has a CustomImageUrl concept. Net result: the algorithm now lives once, and custom book/game covers render in the wishlist (and shared wishlist) just as they do on the list pages.
The fix
Books' (and video games') tenant-owned CustomImageUrl override was applied on the per-type list endpoints but not in the wishlist, so custom covers silently fell back to the reference cover (or nothing).
- Promoted the "hydrate reference cover, then let CustomImageUrl win" logic into a shared ReferenceImageHydrator.HydrateWithCustomOverrideAsync — it previously existed as a private copy in SharedWithMeController plus three near-identical inline copies in Book/VideoGame/Album controllers.
- Routed all of them through it, plus the actual bug fix in WishlistController for Books and VideoGames (movies/TV shows stay on the plain path — they have no CustomImageUrl).
Tests
New WishlistResourceTest with two cases asserting a CustomImageUrl beats the linked reference cover in the wishlist payload, for books and video games. Both pass against local MongoDB.
Docs
- Fixed the now-inaccurate BookController.OnListMappedAsync doc comment (it claimed the override was "Book-specific, not shared via ReferenceImageHydrator" — the opposite of what it now is).
- CLAUDE.md doesn't document CustomImageUrl, so nothing needed changing there for the feature itself.
The test-filter gotcha you hit
Documented in CLAUDE.md's commands section: --settings Local.runsettings and --filter-method can't be combined — --settings forces legacy VSTest mode, which rejects the Microsoft.Testing.Platform filter flags and silently runs zero tests (exit 5, "error: 1"). Added the working recipe: load the runsettings' env vars into the shell, then filter without --settings. A full unfiltered run can still use --settings.
One thing I did not do: I left the AlbumResourceTest/BookResourceTest/VideoGameResourceTest existing override tests untouched since they still pass and cover the list-endpoint path. The shared-with-me endpoints already had override coverage via their own reads. If you'd like belt-and-suspenders coverage for the shared wishlist anonymous path (/api/wishlist/shared/{token}) specifically, I can add that too — but it exercises the identical BuildWishlistAsync code path, so I judged it redundant.
Summary of this round - Shorter messages (ReferenceRefreshMessage.cs): No match found → No match, Unlinked - no match → Unlinked, Already linked → No change. Linked! unchanged. - Narrower slot (app.css): reserved width 8.5rem → 6rem, which comfortably fits all four messages now. - Mobile: no reservation — inside the existing @media (max-width: 767px) block, .kt-title-toast-slot collapses to width: auto; flex: 0 1 auto, so on phones the (already small) title keeps its full width and the slot only takes room while a message is actually showing. Desktop now keeps the title stable with a modest 6rem reserved gap; mobile behaves as it did before, minus the permanent squeeze.
Root cause
The error is a DI scope mismatch, not a Blazor-lifecycle timing issue.
AuthenticationTokenHandler is wired into every API client with AddHttpMessageHandler<AuthenticationTokenHandler>(). IHttpClientFactory builds the message-handler pipeline in its own handler scope, separate from the component render scope. So the NavigationManager you constructor-injected into the handler was a fresh RemoteNavigationManager that the renderer never called .Initialize() on.
That instance is fine as long as nobody touches it — but RedirectToLogin() reads navigationManager.Uri. That path only fires when the Firebase token is stale/expired (past the ~1h token life, before the client-side refresh caught up) and the API returns 401, or when there's no token. Exactly the intermittent condition you saw. During the SSR/prerender pass (WaitForNonStreamingPendingTasks in your stack trace), .Uri on the uninitialized manager throws RemoteNavigationManager has not been initialized, the framework's clean-redirect path never runs, and you get the red error until you refresh (a refresh re-issues the token via the cookie, so the 401 doesn't recur).
Fix
src/BlazorApp/Components/Account/AuthenticationTokenHandler.cs: resolve NavigationManager from httpContext.RequestServices instead of the constructor. That's the request's own scope — the one the endpoint renderer already initialized before running OnInitializedAsync — so NavigateTo throws the proper NavigationException the framework converts into a real redirect to login. httpContext is already guaranteed non-null on this path (the guard at the top of SendAsync), and I made RedirectToLogin static since it no longer needs instance state.
Net effect: a stale token now cleanly bounces the user to the login page (re-running Firebase sign-in and re-issuing the cookie) instead of showing the red error.
Worth noting
I kept the change surgical. Two things I'd flag rather than silently change:
- This handler throws InvalidOperationException("HttpContext is not available") when HttpContext is null, which is the case inside a live interactive circuit. So these authenticated API calls effectively only succeed during the SSR/prerender pass. That's a broader design characteristic, not something this bug requires touching — but if you ever see failures on interactive re-fetches (paging/search after the circuit is live), that's the thing to look at.
- I didn't add a test: this is a rendering-scope/SSR behavior that a mocked unit test can't reproduce (it needs a real prerender host), and per your quality notes a mock here would prove nothing. If you want coverage, the right home is a Playwright smoke test that forces a stale token and asserts the login redirect
The fix
src/BlazorApp/Components/Account/AuthenticationTokenHandler.cs — resolve NavigationManager from httpContext.RequestServices (the request's render scope, where the endpoint renderer already initialized it) instead of constructor-injecting it (which came from IHttpClientFactory's separate handler scope, where the RemoteNavigationManager was never initialized). That uninitialized instance is what threw RemoteNavigationManager has not been initialized on .Uri during the SSR/prerender pass whenever a stale token produced a 401 — the red error you had to refresh past. Now the properly-initialized manager throws the NavigationException the framework converts into a clean redirect to login.
The test
test/BlazorApp.PlaywrightTests/Smoke/StaleTokenRedirectSmokeTest.cs (+ a ForgeStaleTokenMemberCookie() helper on End2EndFixture). It forges an auth cookie with a valid member principal but a Firebase token WebApi rejects, hits the exact reported page (/account/manage/shared/{id}), and asserts a redirect to login.
Verified both directions:
- With the fix: passes (~5–12s).
- With the fix reverted: fails, reproducing your exact stack trace (SharedCollectionPage.OnInitializedAsync → WaitForNonStreamingPendingTasks → uninitialized RemoteNavigationManager).
Recipe updated
I rewrote the e2e-run memory so this doesn't cost time again. The checks I ran (provider keys, Mongo port, browsers) were the recipe's gap — it now leads with "preconditions are already satisfied, don't re-verify," states plainly that no provider keys need setting (the host loads real ones from appsettings.Development.json; placeholder x values are pointless), and gives a copy-paste PowerShell block for a read-only single-class run.
One thing I'll flag again since it's adjacent: this handler throws HttpContext is not available when HttpContext is null (inside a live circuit), so these authenticated API calls effectively only succeed during SSR. Not in scope for this bug, but worth knowing if interactive re-fetches ever misbehave.
What was already there The sharing feature already had a committed SharingSmokeTest (from c89eae2 Share car/house/health) plus its page objects, covering two of the three sharing shapes: - Media (Movies) — copyable list with the "In collection" badge - Personal (Cars) — list plus a read-only detail page The actual gap The plan's progress log flagged one deferred Playwright leg: the collection categories (Collectibles + Gear), which are a genuinely third shape — a full read-only list, but view-only (no shared reference to copy, so no "add to my collection" action and no read-only detail page). Nothing exercised those two tabs. What I added 1. SharedCollectionViewPage.AddButton(itemTitle) — a page-object locator for the per-row data-testid="share-add" action (kept on the page object per your locator-discipline convention). 2. SharingSmokeTest.ShareCollections_RecipientSeesReadOnlyListWithNoAddAction — self-shares Collectibles + Gear through the owner UI, then as the recipient opens each tab and asserts the item is listed read-only with the add button absent (ToHaveCountAsync(0)) and no "In collection" badge. That absence is the whole point — it's what separates a view-only collection tab from a copyable media tab. 3. Updated the plan's progress log to mark the leg as written. test/BlazorApp.PlaywrightTests builds clean (0 warnings). Like every other smoke test it self-skips unless E2E_ENABLED=true, so I couldn't run it here — it needs your WSL E2E env (MongoDB + Firebase creds + browsers). Per your run recipe, that's where it should actually execute. I've left everything uncommitted for you to review first. One note on scope: the existing test covers Cars but not Houses/Health for the personal shape. Those three go through the identical SharedPersonalList + read-only-detail mechanism, so Cars is representative — I didn't add redundant House/Health legs. Say the word if you'd rather have explicit coverage for those too.
Done You were right — the WantToWatch flag was dead weight on TV shows. WatchNextService.ComputeInProgressShows drives shows purely from State == Current + a confirmed unseen episode and never looked at the flag; there was no "shows to watch" surface for it to feed either. It's a genuinely movie-only concept, which is left untouched. Removed across every layer (build clean, 288 unit tests green): - Model / Entity / DTO — TvShowModel, TvShow (want_to_watch BSON), TvShowDto. Mapperly stayed balanced, so the build itself is the proof nothing was missed. - Repository filter — the input.WantToWatch clause in TvShowRepository.GetFilter (it wasn't even exposed as a list filter button). - UI — the "To watch" badge in TvShowMetaRow and the "Watchlist" toggle button + ToggleWantToWatchAsync in TvShowDetail. - TV Time import — ShowEnrichment no longer maps the export's for_later status into shows; the now-unused ShowStatusCsvParser.ForLaterStatus constant went too (parser test keeps its for_later row coverage via the literal, since the parser still must read that column faithfully for favorite). - Index — dropped tvshow_want_to_watch from mongodb-create-index.js, with a note explaining why. Data cleanup: added scripts/unset-tvshow-want-to-watch.js (idempotent $unset, same run-once style as the poster-url migration) to strip the orphaned field from existing tvshow documents — run once per environment. Docs: updated the stale CLAUDE.md import line and added a short design note recording that WantToWatch is movie-only by design, so it doesn't get reintroduced as a flag later (if you ever want a "shows I want to start" view, it should be a real Watch Next section, not a flag with no reader).
Summary #1 — Killed the N+1 reference lookup (WatchNextController.cs) The old code awaited a separate FindByIdAsync per in-progress show (N serial round trips to tvshow_reference). Now it collects the candidate shows' reference ids and does one batched FindByIdsAsync, then maps references back per show — the same batched pattern the movie-poster hydration one line below already used. #2 (cheap) — Stopped fetching the whole episode history (EpisodeRepository + IEpisodeRepository) The old code pulled every episode the owner has (int.MaxValue), then WatchNextService discarded every non-Current show's episodes in memory — cost scaled with total lifetime watch history. Added a batched, owner-scoped FindByShowIdsAsync(ownerId, showIds); the controller now fetches episodes only for the Current+linked shows that can actually appear in the result. The owner_id + tv_show_id IN(...) filter rides the leading fields of the existing episode_last_watched index. Output is behaviorally identical — non-Current and unlinked shows were already excluded downstream. Tests - New EpisodeRepositoryTest (integration, real MongoDB): verifies FindByShowIdsAsync returns only the requested shows' episodes, is owner-scoped (a different owner tracking the same show id is excluded), and returns empty for no ids. Both pass. - Updated the FakeEpisodeRepository in TvTimeImportServiceIdempotencyTest to implement the new interface member. - Full unit suite (288) green; the two new integration tests green against your local Mongo. Not done (as agreed): the aggregation version of #2 (server-side max (season,episode) per show) and reference caching — hold those unless this isn't enough. If you want to sanity-check the win, the Watch Next page load should now issue a small constant number of queries regardless of how many finished shows/episodes are in the collection. One note: ITvShowReferenceRepository.FindByIdAsync is no longer called from this controller but is still used elsewhere, so I left it in place. the movies tab is already in good shape, unlike the show-reference path we just fixed. I checked all three layers: What's already correct 1. Server-side filter, with a matching index. WantToWatch = true is pushed into the Mongo query (MovieRepository.GetFilter:34), and there's a dedicated partial index movie_want_to_watch (owner_id, want_to_watch, filtered to want_to_watch: true) backing exactly that shape. So it fetches only the watchlist, never the whole movie collection. 2. Batched poster hydration, no N+1. ReferenceImageHydrator.HydrateAsync collects all reference ids and does a single FindByIdsAsync — the same batched pattern I just applied to the shows. No per-movie round trip. So the two things that mattered on the shows side (a server-side filter and avoiding N+1) are both already right here. Two marginal nits (I'd leave both) - The "already seen" exclusion runs in memory. FilterMoviesToWatch drops movies with FirstSeenAt != null in C#, even though GetFilter already supports pushing it down via IsUnseen = true (line 37). Pushing it to the DB would transfer slightly fewer docs — but the set is already just the watchlist (small), the FirstSeenAt condition wouldn't be index-covered anyway (only want_to_watch is in the partial index), and FilterMoviesToWatch is a documented, unit-tested method that exists specifically because the flag goes stale. Net payoff ≈ zero. Not worth the change. - A wasted count query — but this is not movie-specific and not really an inefficiency of the movies tab per se: FindAllAsync always runs a CountDocumentsAsync before the Find to populate PagedResult metadata that Watch Next discards. Both the shows fetch and the movies fetch pay it (2 round trips where 1 would do). It's indexed on owner_id and cheap; removing it would mean adding a non-paged read method to the repos — more API surface than the tiny saving justifies. Bottom line: the movies tab doesn't have the kind of scaling problem the episode/reference path had — its cost is bounded by watchlist size, not by total library size. I wouldn't change anything there. If you want me to push the seen-filter down anyway (turning FilterMoviesToWatch into a DB filter) I can, but I'd consider it cosmetic.
…istered provider instead of the configured default ReferenceClientRegistry.All is DI registration order (IGDB always registers before RAWG), and both admin picker components picked FirstOrDefault() as the initial selection, so IGDB stayed selected even when ReferenceData:VideoGameProvider was set to rawg. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RLXGPjBLcpCRU5h4cAXAXU
…md and docs/findings Adds a dedicated OmdbCallBudget unit test for the ceiling split, the asymmetric exhaustion rule, and the per-process short-circuit, none of which any existing test exercised since they all fake IOmdbCallBudget itself. Adds a Car to CarHistory cascade-delete test, matching the House and TvShow cascade tests already in place. Adds full CRUD coverage for TvShow and a new EpisodeResourceTest, closing the long-standing "Thin test coverage" finding for both. Adds MovieReferenceRepositoryTest, mirroring TvShowReferenceRepositoryTest's alias-matching, ambiguity-refusal, and null-Creator BSON coverage that Movie never had. Adds an in-circuit navigation case to NotFoundSmokeTest, proving the Blazor router's client-side not-found path separately from the server's status-code re-execute path already covered. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016u1Vw5pZJi8DdfGV9xaQmH
…e audit found IDataRepository/MongoDbRepositoryBase accept a CancellationToken and forward it into every Mongo call. DataCrudControllerBase's five actions bind it to HttpContext.RequestAborted and forward it through. InventoryApiClientBase forwards it into every HttpClient call. These are the three surfaces AGENTS.md's "No CancellationToken propagation" finding named. The four parent-cascade deletes and the Car/House/HealthProfile/TvShow controllers' own metrics, fuel-category and reference-link actions carry it too, since they run inline in the same request. OnCreatedAsync and the detached background enrichment it kicks off deliberately still take none, cancelling the HTTP request that started that work must never cancel the work itself. PagedRequest.Page now rejects a zero or negative value on purpose. It used to reach 400 only by accident, through a Mongo ArgumentOutOfRangeException that the exception filter happens to catch because it derives from ArgumentException. PageSize keeps no upper bound: a first attempt at capping it broke CarDetail/HealthProfileDetail/HouseDetail/TvShowDetail/AlbumDetail/PlaylistDetail, which request very large page sizes on purpose to fetch one parent's whole child collection in a single page. OwnershipIsolationResourceTest proves a user cannot read, update or delete another user's record over HTTP, seeding the other owner's document directly through the repository since only one Firebase test account exists. FirebaseClaimsBuilder is extracted out of AuthenticationController as a pure function and gets its own unit tests, covering the admin role claim copy that previously had none. docs/findings/by-design-and-gaps.md is updated to record what each finding closed and what is still open. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EP38ctzw44jwxmARLNec2Y
FirebaseClaimsBuilder's own unit tests (added in the previous commit) cover the claim-building logic. The doc still read as if the whole controller had none, which overstated the remaining gap: token verification, cookie issuance and the refresh-identity check are what is actually still untested. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EP38ctzw44jwxmARLNec2Y
…uthenticationController's own validation CancellationTokenPropagationTest passes an already-cancelled token into IMovieRepository.FindOneAsync/FindAllAsync against real MongoDB and asserts it throws OperationCanceledException. The previous commit wired the plumbing through; nothing had proven it actually reaches the driver rather than being forwarded into a parameter nobody reads. AuthSmokeTest now posts directly to /auth/callback and /auth/refresh, covering the 400 for a missing token, the 401 for an invalid one, and the 401 for a refresh attempted while signed out. Every other e2e test only ever exercised the callback's success path indirectly, through the fixture's own sign-in setup with a genuinely valid token. Still open: the "a refresh cannot swap the signed-in identity to a different Firebase user" check, which needs a second real identity the fixture does not hold. docs/findings/by-design-and-gaps.md is updated to record both. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EP38ctzw44jwxmARLNec2Y
… that made it pass for the wrong reason End2EndFixture.GetAnotherUsersIdTokenAsync mints a second ephemeral Firebase user on demand, the same self-hosted-only capability as ForgeStaleTokenMemberCookie, deleted alongside the run's primary ephemeral user through a shared DeleteEphemeralUserAsync helper. AuthSmokeTest.Refresh_WithAnotherUsersValidToken_Answers401 uses it to prove a token that verifies but belongs to a different Firebase user can never swap the signed-in identity. Writing that test surfaced a real bug in shared test infrastructure, not the app: AccountRepository.AuthenticateAsync cached a single sign-in behind one static field regardless of which username was passed. The second identity's sign-in silently returned the first identity's already-cached token, so the new test first passed with a 200 instead of a 401, both tokens being the same identity's. The cache is now keyed by username, so a repeated sign-in for the same identity still shares one call (the reason the cache exists, avoiding Google's abuse protection under WebApi.IntegrationTests' parallel runs) while a different identity signs in independently. docs/findings/by-design-and-gaps.md is updated to record the closure, and to keep the still-open WebApi.IntegrationTests-side AdminOnly gap clearly separate: that one needs a permanent second Firebase account, since that suite has no Admin SDK access to mint one on demand the way this one does. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EP38ctzw44jwxmARLNec2Y
scripts/firebase-user-role.js already grants a role to an existing user, but nothing created one, so a second test account had to be made by hand in the Firebase console. scripts/firebase-create-user.js calls the public accounts:signUp REST endpoint, the same one a real sign-up form uses, so it needs only the project's Web API key and never a service account. Email and password are both optional, a random test address and a strong random password are generated when omitted, matching the shape End2EndFixture already uses for its own ephemeral e2e users. Verified against the real Firebase project by creating a throwaway user and immediately deleting it with its own idToken (accounts:delete needs no admin privilege for a user deleting itself), and by checking the error path with an email known to already exist, both leaving nothing behind. CONTRIBUTING.md documents it next to the existing role-granting script. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EP38ctzw44jwxmARLNec2Y
ASP.NET Core and Microsoft.Extensions move to 10.0.12, MongoDB.Driver to 3.12.0, Playwright to 1.63.0 and xunit.v3.mtp-v2 to 4.0.1, among others. Verify.XunitV3 stays at 32.0.1, since 33.x fails the build with SC021 until a SponsorCheck license or exemption is declared. SharpCompress stays on 0.50.x, pinned transitively for MongoDB.Driver. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DoZUrcwA1U5Ch1XQCpxEKe
…de items of the backlog The five reference-linked detail pages share ReferenceLinkedDetailPageBase and ReferenceMatchControls, so each one watches for its reference link after creation. The watch replaces the page's model only once the item is linked, since replacing it earlier swapped the objects a pending removal held and a removed copy was saved back. Car, House and Health share JournalDetailPageBase and JournalEntryModal. Charts are Razor components over one ChartAxes, which removes the ASP0006 suppression, and SVG coordinates are formatted with the invariant culture. Checking for a reference match now reports the API's own error message. The wishlist sorts through the repository's title collation, replacing WishlistService. ReferenceMatchRules and RatingSourceCatalog move to Domain/Services, being pure logic. firebase-user-role.js runs on plain Node.js with no dependency, and gains a read-only --show mode. Comments lose their dashes and second person. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DoZUrcwA1U5Ch1XQCpxEKe
…f the sibling repositories AGENTS.md keeps every rule and its reason and drops the history, and states that a comment says why and is timeless. CONTRIBUTING.md holds only the setup steps, with stale ports, providers and test variables corrected. The --settings gotcha is corrected: the runner runs zero tests with it, filtered or not. operations.md, the findings and the backlog are brought up to date. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DoZUrcwA1U5Ch1XQCpxEKe
… images No file needs a byte order mark, markdownlint refuses a configuration file starting with one, and charset = utf-8 in .editorconfig keeps editors from adding one back. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01McrBgv5WFPJt215Qt8MXLn
task ci runs the CI pipeline of the last commit in containers with IstarCI, on the images of devpro/container-images that .istarci.yml names per job. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01McrBgv5WFPJt215Qt8MXLn
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01McrBgv5WFPJt215Qt8MXLn
…ctions Its tools install under RUNNER_TEMP with no sudo, and FOSSA and the Docker steps no longer go through third party actions. The pin names the branch commit, to be moved to the merge commit once it is merged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01McrBgv5WFPJt215Qt8MXLn
… a directory of their own Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01McrBgv5WFPJt215Qt8MXLn
Any other action fails the local run before a job starts, and a ref that can move is reported as a warning. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PrN6WEA4putGkQGnnfm9N5
…n development task ci:setup registers the repository with the installed daemon and gates git push, task ci and task ci:logs report on the runs. task ci:from-clone keeps the one-off run from a clone, for work on IstarCI itself. CONTRIBUTING.md and AGENTS.md say IstarCI is recommended but optional, the CI being the GitHub Actions pipeline. TODO.md describes the configuration in place, and docker/* joins the allowed actions, as AGENTS.md allows them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PrN6WEA4putGkQGnnfm9N5
…excluded Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PrN6WEA4putGkQGnnfm9N5
…e repository An id comes from the page route, so an unescaped "../admin" addressed another API resource (Sonar S7044). The runsettings loader reads only a .runsettings file inside the repository (Sonar S8707). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WALp7B6Pqzv995hfX8f618
…llision The runsettings loader reads only a file name at the repository root, since Sonar did not accept the startsWith guard. Explore's distinct reads await DistinctAsync instead of running the query synchronously. The album smoke test uses a title no other test adds, since a parallel "Kid A" link propagated onto its artist-less copy. The rest are small cleanups: an exception logged on the OMDb timeout, helpers no longer shadowing record members, a merged CSS rule, optional chains, generated regexes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WALp7B6Pqzv995hfX8f618
Creating an item, checking it for a match, adding it from Explore and the TV Time import link that one item, since another owner's copy is theirs to check. Only an admin's link reaches other records, every unlinked one matching the title and year it was made with. An admin's album link also requires the artist, and the admin queue lists albums by title, year and artist. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WALp7B6Pqzv995hfX8f618
|
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
2 participants
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.
No description provided.