V2.3 Google Books API, UX improvements#466
Merged
Merged
Conversation
…razor - _movie and _reference became public Movie/Reference auto-properties marked [PersistentState] (.NET 10's declarative prerendered-state persistence — public properties are a framework requirement). The data loaded during the prerender pass is now serialized into the page and restored when the interactive circuit attaches. - OnParametersSetAsync now skips the reload when Movie?.Id == Id — i.e. when the restored state already matches the route. That's what removes the flash: the first interactive render reuses the prerendered data, so there's no spinner reset and no duplicate API calls. The id check means an in-circuit navigation to a different movie still reloads normally, and the explicit refresh paths (RefreshReferenceAsync, the admin linker's OnLinked) still call LoadAsync unconditionally, so they force a real re-fetch as before.
Same shape as Movie, plus one extra: LoadAsync also fetches the watched-episodes list, so I persist three things — Show, Reference, and the raw Episodes list — and extracted the season/watched dictionary derivation into a local BuildDerivedState() (tuple-keyed dictionaries can't round-trip through the JSON-persisted state, so they're rebuilt cheaply on restore). OnParametersSetAsync skips the re-fetch when the restored Show.Id matches the route; all explicit LoadAsync callers (refresh reference, episode check/uncheck, manual add) still force a real reload.
- App.razor: data-bs-theme="dark" is set statically on <html> in server-rendered markup (no client JS), so there's no flash on load or between page navigations — this was the actual cause of the buttons flipping white/dark you saw. - app.css: kept only the dark token values in :root; removed the light token block, the [data-bs-theme="dark"] override selector, and the theme-toggle button CSS. - NavMenu.razor: removed the "Toggle theme" button. - Deleted wwwroot/theme.js (picked initial theme, exposed ktToggleTheme()) and wwwroot/Keeptrack.BlazorApp.lib.module.js (only existed to re-apply the theme after Blazor's enhanced-navigation DOM diff — no longer needed since the theme is static server markup now). - Updated CLAUDE.md's Theme section to document the dark-only design and why. Verified dotnet build src/BlazorApp succeeds with 0 warnings/errors, and confirmed no other files (Playwright tests, other Razor components) reference the removed toggle/JS.
Domain/storage/DTO layers — added Price (decimal?), Vendor (string?), AcquiredAt (DateOnly?), Reference (string?) to VideoGamePlatformModel, its Mongo entity VideoGamePlatform (Decimal128 price, acquired_at BSON element, DateTime storage via the existing CommonStorageMappings DateOnly↔DateTime conversion), and VideoGamePlatformDto. Mapperly's generated storage/DTO mappers picked these up automatically with no manual mapping code needed (verified by a clean build). UI — VideoGameDetail.razor's per-platform card and its handlers now show the identical Price(€)/Acquired on/Vendor/Reference fields (same labels, placeholders, input types, and data-testids) as OwnedVersionsEditor uses for movies/TV shows/books/albums, auto-saving on change like the existing State/CopyType controls. The "empty entry needs no remove-confirmation" check now also considers these new fields. The identity-only "Add a platform" draft form is left untouched, consistent with the app's existing convention that new-copy detail fields are edited after creation. Tests — extended VideoGameResourceTest's owned/wishlisted integration test to round-trip the new decimal/date fields through the real DTO→model→BSON path (mirroring MovieResourceTest's pattern), and updated a stale doc comment in VideoGamePlatformSmokeTest. Full solution builds with 0 warnings/errors; WebApi unit tests (162) pass. Integration tests need a local MongoDB + Firebase credentials I don't have configured here, so I didn't run those, but they compile cleanly.
Root cause: Blazor's ComponentBase.SetParametersAsync calls StateHasChanged() unconditionally right after the synchronous prefix of OnParametersSetAsync runs — before any awaited fetch resolves. My first pass added a 200ms delay before setting _loading = true inside the load method, but the _loading field itself still defaulted to true at declaration. Since a new detail-page component instance is created for each new item (different route Id → fresh field defaults), that forced render always caught _loading == true and painted the spinner for one frame, regardless of the delay logic inside LoadAsync. Fix: replaced the single _loading flag with two: - _loaded — false until a load attempt (fresh fetch or [PersistentState]-restored) has actually finished; both default to false now. - _loading — still delay-gated, only flips true if the fetch is genuinely still running after 200ms. Markup now reads @if (!_loaded) { @if (_loading) { spinner } } else if (X is null) { not found } else { content } — so that forced early render (before data or the delay have resolved) shows nothing instead of a spinner or a premature "not found" flash. Applied to all 9 detail pages, InventoryPageBase/InventoryList (covers all list pages), consistently.
Causes error
- Removed [PersistentState]; Data is now a plain private nullable property, with a comment explaining why this page must not persist its unbounded payload (dotnet/aspnetcore#65101, pointing at your docs/prerender-flash-fix.md). - OnInitializedAsync collapsed to just LoadAsync() — no restore branch. - Kept the flash fix intact: _loaded/_loading still default false so the pre-await render shows blank, and LoadingIndicator.RunAsync only shows the spinner if the fetch is still running after 200 ms. The interactive circuit will re-fetch on every visit now, but that call should stay under the 200 ms gate most of the time, so no spinner flash. - [PersistentState] removed; Data is now a private WishlistDto? with the same comment explaining the unbounded-payload / 32 KB hub limit rationale (dotnet/aspnetcore#65101, pointing at docs/prerender-flash-fix.md). - OnInitializedAsync collapsed to LoadAsync(); the restore branch is gone. The delay-gated spinner logic is untouched.
Confirmed then — that's the circuit-reuse bug I flagged, not a deployment issue and not something [PersistentState] can fix as currently applied. What's actually happening: the first page you load establishes one SignalR circuit for your whole session. Every later click on an item is an enhanced-nav within that same live circuit — the server prerenders the new page and embeds a fresh persisted-state blob, but attaching that new page's component to an already-running circuit doesn't go through the same restore handshake a fresh circuit boot does. So Movie comes back null, OnParametersSetAsync falls through to LoadAsync(), and you get the full double-fetch and flash again — every time, because "click from list" is the only way anyone actually uses this app. This means [PersistentState] was the wrong tool for this specific case. It only pays off for a page that's the very first thing loaded cold in a tab — real, but not your actual usage pattern. The real fix has to remove the second HTTP round-trip entirely rather than try to make it cheap. Concretely: keep the render-mode boundary on Routes.razor/a shared layout instead of on each individual page (@rendermode InteractiveServer per page is what forces every navigation through a fresh HTTP prerender). If the boundary is set once at the root, item clicks become a normal in-circuit Blazor route change — no second HTTP request, no re-prerender, no restore handshake to fail. That's a bigger, riskier change than what's on this branch (it affects every page, not just detail pages), so I don't want to just start rewriting it silently. Why it can't be scoped narrower: true in-circuit navigation (no HTTP round-trip at all between pages) requires the Router itself to be interactive, not just the pages. Right now Routes.razor's <Router> is static — that's exactly why every navigation, even between two pages that both say @rendermode InteractiveServer, still goes through a full server round-trip (the Router has to run server-side to resolve each new URL). Attaching the render mode to a shared layout instead of each page wouldn't help — the Router sits above the layout, so it'd still re-execute per click. The collateral effect: making the Router interactive is app-wide, not per-route — everything reachable through it becomes part of one continuous circuit-bound tree. That includes SharedWishlistPage.razor, which is deliberately static with no render mode today (comment right in the file: "no @rendermode... the recipient has no account and needs no interactivity"). An anonymous share-link visitor would start pulling a SignalR circuit too, which was an explicit design choice, not an oversight.
Extracts the Date/"HH:mm" time proxy pair duplicated in CarDetail.razor and HealthProfileDetail.razor into a shared Components/Shared/DateTimeFields.razor component, so Quick Add's upcoming record forms reuse it instead of copying it a third time. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01UxPGSjNnoa6Ltuk4pHdFaw
Extracts CarDetail.razor's history modal field body into a shared Components/Inventory/Shared/CarHistoryForm.razor component (Entry/ShowFuel/ShowElectric params, plus the NewEntry factory), so Quick Add's car-record form reuses it instead of duplicating the field layout. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01UxPGSjNnoa6Ltuk4pHdFaw
Extracts HouseDetail.razor's and HealthProfileDetail.razor's history/journal modal field bodies into shared Components/Inventory/Shared components (HouseHistoryForm, HealthRecordForm), each with its NewEntry factory moved in, so Quick Add's house-record and health-record forms reuse them. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01UxPGSjNnoa6Ltuk4pHdFaw
Adds IOwnedCopyDto (CopyType/Price/AcquiredAt/Vendor/Reference), implemented by OwnedVersionDto and VideoGamePlatformDto, and extracts their identical copy-field markup into a shared Components/Inventory/Shared/OwnedVersionFields component. Rewires OwnedVersionsEditor.razor and VideoGameDetail.razor's platform cards to use it; each caller keeps its own persistence timing via OnChanged and its own header chrome via the ButtonRowSuffix slot. Note: VideoGameDetail's platform-name heading now sits on its own line above the Physical/Digital toggle instead of beside it in the same flex row - a minor, deliberate layout simplification from sharing the component structure with OwnedVersionsEditor's simpler header (no title). Worth an eyeball during the UI/UX review pass. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01UxPGSjNnoa6Ltuk4pHdFaw
Adds the /add page: a WatchNext-style standalone page (not InventoryPageBase, so the record DTOs' required members cause no new() constraint problem) with a type picker driven by a ?type= query parameter (list-page URL-state convention), and one-shot add forms for the five media types (movie, TV show, book, album, video game). Each form reuses OwnedVersionFields/OwnedVersionDto for an optional "I own a copy" toggle and posts once via the shared SaveMediaAsync<TDto> helper, landing on the created item's detail page. Book/album/video game are gated behind MemberOnly like the rest of the app; movie/TV show stay available to preview accounts. Car/house/health record types are not wired up yet (step 6). Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01UxPGSjNnoa6Ltuk4pHdFaw
Adds the three record types (car/house/health) to the /add picker and form. Selecting a type fetches its possible parents (delay-gated spinner, shared LoadParentsAsync<TDto> helper): zero parents shows an empty-state link to create one first, exactly one preselects silently, more than one shows a segmented button row. The shared CarHistoryForm/HouseHistoryForm/HealthRecordForm components from steps 2-3 render the record fields; Save is disabled until a parent is selected and posts via the shared SaveRecordAsync<TDto> helper, landing on the parent's detail page. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01UxPGSjNnoa6Ltuk4pHdFaw
Adds "Quick add" as the first nav item below Home, and a "+ Quick add" button on the Home page (primary CTA above the stat grid when the user has data; primary CTA over "Go to my movies" on the empty-state variant). Uses plain ASCII "+" rather than a Unicode glyph for the icon, matching every other "add" affordance in the app (+ Add entry, + Add version) and sidestepping the emoji-presentation risk the plan flagged as unverified. Adds the minimal Quick Add CSS: a .kt-quickadd-back link style, and a mobile override forcing the type picker to 2 columns and Save buttons full-width under 767px. No sticky save bar in this first version - the forms are short enough not to need one. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01UxPGSjNnoa6Ltuk4pHdFaw
Adds QuickAddPage (page object) + PageBase.OpenQuickAddAsync(), and
QuickAddSmokeTest covering the two representative Quick Add scenarios: a
movie with an owned copy (exercises the shared OwnedVersionFields toggle,
landing on /movies/{id}) and a car refuel with a single existing car
(exercises the silent single-parent preselect path and CarHistoryForm,
landing on /cars/{id}). The car scenario runs in its own non-parallel
collection, same rationale as WatchNextSmokeTest - it depends on the shared
tenant having exactly one car at selection time.
Adds data-testid="mileage-input"/"cost-input" to CarHistoryForm - the first
test to ever drive those fields directly, and their label/input pairs have
no for/id association (the documented GetByLabel gotcha).
Adds /add and /add?type=movie to MobileScreenshotTest's capture list.
Per-step regression note: existing Car/House/Health/Ownership/VideoGamePlatform
smoke tests weren't re-run after each extraction step per this session's
build-only gating (tests deferred to the owner's final validation pass) -
they should be run once, together with everything else in the verification
section, before this branch is considered done.
Co-Authored-By: Claude Sonnet 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01UxPGSjNnoa6Ltuk4pHdFaw
Movie: drop the Rating field and the "Watched on" today-default (starts
empty, still editable). Book: drop the "Read on" today-default.
Fixes a real CSS bug behind the reported uneven/full-width button rows: the
mobile Save-button rule ".kt-quickadd .btn-primary { width: 100% }" matched
*any* primary button inside the page, not just Save - including whichever
event-type option or parent (car/house/health-profile) happened to be
selected, since selection is what turns a toggle button btn-primary. That's
why Refuel started full-width (selected by default, squeezing Maintenance/
Other into the remainder) and why picking a car stretched its button full-
width instead of just recoloring it. Save buttons now carry their own
kt-quickadd-save-btn class instead.
Also adds .kt-btn-row-even (flex:1 1 0 per button) to the event-type rows
in CarHistoryForm/HouseHistoryForm/HealthRecordForm and to Quick Add's own
car/house/health-profile selector rows, so options size evenly instead of
by label length - this also improves the original CarDetail/HouseDetail/
HealthProfileDetail modals, which share these same components.
Co-Authored-By: Claude Sonnet 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01UxPGSjNnoa6Ltuk4pHdFaw
Adds a "New <type>" heading (New movie, New TV show, New car record, etc.) above each Quick Add form, right below the "choose a different type" link, so it's clear at a glance which form is showing. Also reflects the owner's own edits: Home's empty-state CTA drops the "Go to my movies" secondary link (didn't display well next to the primary Quick Add button), plus minor reformatting in NavMenu.razor and the Playwright PageBase.cs doc comments. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01UxPGSjNnoa6Ltuk4pHdFaw
1. Mobile menu auto-close — recreated Keeptrack.BlazorApp.lib.module.js hooking enhancedload to uncheck #nav-toggle. 2. Unseen/Unread filters — added to Movie/Book lists, repos, and mappers. 3. New sorts — Movie "Last seen", Book "Last read", Video Game "Last completed" (backed by a genuinely new CompletedAt field on platform entries, auto-populated when state is set to "Completed"). 4. Modal safety — House/Car/Health history modals no longer close on outside click/drag; Cancel/✕ now warn via ConfirmModal if the draft has unsaved changes (new DirtyTracking helper). 5. Health yearly cost review — now newest year first. 6. Car — maintenance-overdue check removed; "Last record" per event type added (mirroring Health's "Last seen"). 7. House — same "Last record" per event type added. 8. Wishlist filter buttons — removed from Movie/Book/TvShow/VideoGame list pages (flag, badge, /wishlist page untouched). 9. Health/House/Car lists — now default-sort by name.
What was added - BookModel.Isbn (and Book/BookDto/BookReferenceModel/BookReference/BookReferenceDto) — edited only on BookDetail.razor, never the Add form. - Search input: threaded isbn through IBookReferenceClient.SearchBooksAsync and the whole admin search chain (ReferenceDataAdminController, ReferenceDataAdminPage.razor's ISBN field, InlineReferenceLinker's Isbn parameter bound from the book's own detail page). Only GoogleBooksClient actually uses it — as an exact isbn: lookup that supersedes title/author entirely when present; Open Library/BnF accept but ignore it. - Autofill on link/refresh: both Google Books (industryIdentifiers, preferring ISBN_13) and BnF (dc:identifier's "ISBN ..." value, confirmed against the real API) populate it; Open Library doesn't, same edition-vs-work-level reason as Language. - "Only fields actually used" for matched aliases: ReferenceMatchModel/ReferenceMatch gained an Isbn field, and the canonical alias (provider's own value) vs. the tenant-search alias (what was actually searched with) are now kept as genuinely separate entries — the search alias is never backfilled with the provider's ISBN when no ISBN was actually part of that search.
1. Homepage flash (empty → summary) — Home.razor had no prerender-flash protection at all: _stats was a plain field, so the interactive circuit reset it to null and re-fetched, causing the empty/CTA view to flash before the real stats popped in. I used [PersistentState] (the detail-page pattern), not the _loaded/re-fetch pattern from WatchNext/Wishlist — those two avoid [PersistentState] specifically because they embed unbounded lists that can exceed SignalR's 32KB message limit (see docs/prerender-flash-fix.md). CollectionStatsDto is just 8 counts, so it's safe and this halves the API call too, matching MovieDetail/InventoryPageBase's existing convention. 2. Background sync "not running daily" — the sync itself looks correctly wired (lease-based, 24h PeriodicTimer), but I found the real gap: ReferenceSyncBackgroundService never recorded anything in the background_job collection, so the admin page's "Recent jobs" table only ever showed manual "sync now" runs — the periodic pass was invisible there even when it ran fine (only the lease's expiry timestamp and logs proved it). Fixed by having the periodic loop create/complete/fail a job the same way the manual trigger does, so daily runs now show up in Recent Jobs. If it's still not appearing after this deploys, check Features:IsReferenceSyncEnabled in your live environment's actual config/env override — that's the one thing I couldn't verify from here. 3. Cross-year mismatch bug (the important one) — confirmed and fixed. All five TryLinkExisting*ReferenceAsync methods (TvShow/Movie/Book/Album/VideoGame) fell back to a title-only lookup whenever the exact title+year didn't match — even when the item had a specific year, just not yet a confirmed alias. Since the title-only fallback ignores year completely, linking "Road House" (2024) and then checking "Road House" (1990) would match the 2024 reference. Fixed so the title-only fallback only fires when the item has no year recorded at all (its original, correct purpose); a mismatched-but-known year is now left unresolved instead of guessed. Added a regression test using this exact scenario, plus a note in docs/code-quality-findings.md. Found the actual root cause. TryLinkExisting*ReferenceAsync (the "check for reference match" button) wasn't the whole story — the real culprit is Resolve*Async (what runs when an admin manually links a candidate, or when auto-resolve fires). It had the identical title-only fallback bug, but there it's worse: it reuses the wrongly-matched document's Id for the upsert, so it doesn't just link wrong — it overwrites the other reference document's data. That's the actual merge you saw with Road House. Fixed the same way in all five Resolve*Async methods (TvShow/Movie/Book/Album/VideoGame): the title-only fallback now only fires when no year is recorded at all. Added a regression test, ResolveMovieAsync_DoesNotMergeIntoAnUnrelatedSameTitledReference_WhenResolvingADifferentTmdbIdWithItsOwnKnownYear, which reproduces the exact scenario (2024 already resolved, then resolving 1990 under a different TMDB id) and asserts the 1990 resolve never reuses/overwrites reference-2024. Also added the equivalent TryLinkExistingMovieReferenceAsync test.
Home.razor now hides its content entirely until _loaded is true (populated either instantly from restored [PersistentState], or after a real fetch — with a spinner only appearing if that fetch takes over a second). Previously, Stats == null fell straight into the CTA branch for that one frame before the real data arrived — that's what you saw. This should eliminate the flash on redeploy.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| public string DisplayName => "BnF"; | ||
|
|
||
| private static readonly XNamespace s_srw = "http://www.loc.gov/zing/srw/"; |
| /// URL, an ISBN as plain text "ISBN 2841142787" - confirmed against the real API) - the ARK is already | ||
| /// captured separately via <c>srw:recordIdentifier</c>, so this only looks for the ISBN-prefixed one. | ||
| /// </summary> | ||
| private static readonly Regex s_isbnRegex = new(@"^ISBN\s+(.+)$", RegexOptions.IgnoreCase | RegexOptions.Compiled); |
| if (string.IsNullOrWhiteSpace(raw)) return null; | ||
|
|
||
| var namePart = raw.Split(". ", 2)[0]; | ||
| namePart = Regex.Replace(namePart, @"\s*\([^)]*\)", "").Trim(); |
| /// catalogue dates can carry uncertainty markers or ranges - a defensive last-4-digit-token extraction, | ||
| /// same shape as <see cref="OpenLibraryClient"/>'s own year parsing, rather than a bare <c>int.Parse</c>. | ||
| /// </summary> | ||
| private static readonly Regex s_yearRegex = new(@"\b\d{4}\b", RegexOptions.Compiled); |
|
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.