-
Notifications
You must be signed in to change notification settings - Fork 326
perf(webapp): slim calendar, home, assets-index and bookings loaders #2738
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
carlosvirreira
wants to merge
6
commits into
main
Choose a base branch
from
perf/p95-under-300ms
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
5188ea5
perf(webapp): slim core loader payloads to cut route p95s
cf8d051
fix(webapp): address review feedback on loader-slimming PR
119415d
test(webapp): assert inactive sort keys stay out of the slim CTE
83b1bdf
Merge branch 'main' into perf/p95-under-300ms
DonKoko 485209b
merge: main into perf/p95-under-300ms
0022368
chore: drop the perf seeder from the PR
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
365 changes: 365 additions & 0 deletions
365
apps/webapp/app/components/booking/booking-assets-sidebar.test.tsx
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,365 @@ | ||
| /** | ||
| * BookingAssetsSidebar — dual-mode (eager vs lazy) unit tests | ||
| * | ||
| * The sidebar sources its rows two ways (see the component's file-level | ||
| * doc): callers that ship `booking.bookingAssets` inline render eagerly | ||
| * with zero fetching, while the bookings index omits the payload and the | ||
| * sheet lazily fetches `/api/bookings/:bookingId/assets-sidebar` on open. | ||
| * These tests pin the observable contract of both modes: | ||
| * | ||
| * - Eager: rows render straight from the prop, no fetcher traffic. | ||
| * - Lazy: exactly one fetch per open, spinner while in flight, rows | ||
| * once the payload lands. | ||
| * - Lazy reopen: rows never duplicate; the previous payload renders | ||
| * immediately (no spinner flash) while the deliberate freshness | ||
| * re-fetch fires in the background. | ||
| * - Trigger state: a booking with zero concrete assets but outstanding | ||
| * model reservations is still openable (Book-by-Model), while a | ||
| * booking with nothing to show keeps an inert trigger. | ||
| * | ||
| * @see {@link file://./booking-assets-sidebar.tsx} | ||
| * @see {@link file://./../../routes/api+/bookings.$bookingId.assets-sidebar.ts} | ||
| */ | ||
|
|
||
| import type { ComponentProps } from "react"; | ||
| import { render, screen } from "@testing-library/react"; | ||
| import userEvent from "@testing-library/user-event"; | ||
| import { beforeEach, describe, expect, it, vi } from "vitest"; | ||
|
|
||
| import { | ||
| BookingAssetsSidebar, | ||
| type DispositionBreakdown, | ||
| type SidebarBookingAssets, | ||
| type SidebarModelRequest, | ||
| } from "~/components/booking/booking-assets-sidebar"; | ||
|
|
||
| /** | ||
| * Shape of a successful `/api/bookings/:bookingId/assets-sidebar` response | ||
| * as seen through `fetcher.data`. Mirrors `payload()` in the resource | ||
| * route (`{ error: null, ...data }`), typed off the component's own | ||
| * exports so the mock payload stays structurally in sync with what the | ||
| * sidebar actually consumes. | ||
| */ | ||
| type AssetsSidebarPayload = { | ||
| error: null; | ||
| bookingAssets: SidebarBookingAssets; | ||
| dispositionedByAsset: Record<string, number>; | ||
| dispositionBreakdownByAsset: Record<string, DispositionBreakdown>; | ||
| checkedOutByAsset: Record<string, number>; | ||
| }; | ||
|
|
||
| /** | ||
| * Settled error payload, as produced by the resource route's catch | ||
| * (`data(error(reason), { status })`) — `error` is the only key the | ||
| * component reads on this branch. | ||
| */ | ||
| type AssetsSidebarErrorPayload = { error: { message: string } }; | ||
|
|
||
| /** The slice of the fetcher API the sidebar actually touches. */ | ||
| type FetcherStub = { | ||
| state: "idle" | "loading" | "submitting"; | ||
| data: AssetsSidebarPayload | AssetsSidebarErrorPayload | undefined; | ||
| load: ReturnType<typeof vi.fn>; | ||
| submit: ReturnType<typeof vi.fn>; | ||
| }; | ||
|
|
||
| function createFetcherStub(): FetcherStub { | ||
| return { | ||
| state: "idle", | ||
| data: undefined, | ||
| // The component `void`s the returned promise, so resolve immediately. | ||
| load: vi.fn(() => Promise.resolve()), | ||
| submit: vi.fn(), | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Mutable per-test fetcher stub. Tests mutate `fetcherStub.data` to | ||
| * simulate the fetch resolving, then re-render (the real fetcher triggers | ||
| * that re-render itself when data lands). | ||
| */ | ||
| let fetcherStub: FetcherStub = createFetcherStub(); | ||
|
|
||
| // why: the sidebar's lazy mode is driven by `useFetcher` — mocking it lets | ||
| // tests assert "no request fired" / "exactly one request" and hand-feed | ||
| // the resolved payload without spinning up a data router. `Link` is | ||
| // swapped for a plain anchor for the same reason (the "Scan to assign" | ||
| // link and the asset-title `Button to=` both need a router at runtime). | ||
| vi.mock("react-router", async () => { | ||
| // Typed as a plain record: the module type can't be named here without a | ||
| // namespace import of react-router, which trips no-restricted-imports. | ||
| const actual = await vi.importActual<Record<string, unknown>>("react-router"); | ||
|
|
||
| return { | ||
| ...actual, | ||
| useFetcher: () => fetcherStub, | ||
| Link: ({ to, children, ...rest }: ComponentProps<"a"> & { to: string }) => ( | ||
| <a {...rest} href={to}> | ||
| {children} | ||
| </a> | ||
| ), | ||
| }; | ||
| }); | ||
|
|
||
| // why: useCurrentOrganization reads the `_layout` route's loader data via | ||
| // useRouteLoaderData, which requires a data-router context these tests | ||
| // don't mount. Returning undefined exercises the component's documented | ||
| // no-org branch (display-code chips are skipped) — irrelevant to the | ||
| // dual-mode behavior under test. | ||
| vi.mock("~/hooks/use-current-organization", () => ({ | ||
| useCurrentOrganization: () => undefined, | ||
| })); | ||
|
|
||
| // why: the real AssetImage wires its own useFetcher for signed-URL refresh, | ||
| // which would collide with the sidebar's mocked fetcher instance (both | ||
| // callers would receive the same stub and AssetImage would misread the | ||
| // sidebar payload). A bare <img> keeps each row's image slot inert. | ||
| vi.mock("~/components/assets/asset-image", () => ({ | ||
| AssetImage: ({ alt }: { alt: string }) => <img alt={alt} />, | ||
| })); | ||
|
|
||
| type SidebarBooking = ComponentProps<typeof BookingAssetsSidebar>["booking"]; | ||
|
|
||
| /** | ||
| * Minimal, type-correct `BookingAsset` pivot row: a standalone (no kit) | ||
| * INDIVIDUAL asset — the simplest shape `groupAssets` renders as one | ||
| * individual row. | ||
| */ | ||
| function buildBookingAsset({ | ||
| id, | ||
| title, | ||
| }: { | ||
| id: string; | ||
| title: string; | ||
| }): SidebarBookingAssets[number] { | ||
| return { | ||
| id: `ba-${id}`, | ||
| quantity: 1, | ||
| assetKitId: null, | ||
| asset: { | ||
| id, | ||
| title, | ||
| type: "INDIVIDUAL", | ||
| availableToBook: true, | ||
| custody: [], | ||
| status: "AVAILABLE", | ||
| mainImage: null, | ||
| thumbnailImage: null, | ||
| mainImageExpiration: null, | ||
| sequentialId: null, | ||
| preferredBarcodeId: null, | ||
| qrCodes: [], | ||
| barcodes: [], | ||
| category: null, | ||
| assetKits: [], | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| /** Outstanding (unfulfilled) Book-by-Model reservation: 2 of 3 remaining. */ | ||
| function buildModelRequest( | ||
| overrides?: Partial<SidebarModelRequest> | ||
| ): SidebarModelRequest { | ||
| return { | ||
| id: "mreq-1", | ||
| assetModelId: "model-1", | ||
| quantity: 3, | ||
| fulfilledQuantity: 1, | ||
| fulfilledAt: null, | ||
| assetModel: { id: "model-1", name: "Sony A7 IV" }, | ||
| ...overrides, | ||
| }; | ||
| } | ||
|
|
||
| function buildBooking(overrides?: Partial<SidebarBooking>): SidebarBooking { | ||
| return { | ||
| id: "booking-1", | ||
| name: "Studio session", | ||
| status: "RESERVED", | ||
| ...overrides, | ||
| }; | ||
| } | ||
|
|
||
| /** Successful lazy-fetch payload with empty qty-progress maps. */ | ||
| function buildPayload( | ||
| bookingAssets: SidebarBookingAssets | ||
| ): AssetsSidebarPayload { | ||
| return { | ||
| error: null, | ||
| bookingAssets, | ||
| dispositionedByAsset: {}, | ||
| dispositionBreakdownByAsset: {}, | ||
| checkedOutByAsset: {}, | ||
| }; | ||
| } | ||
|
|
||
| /** The lazy path's loading indicator (shared `Spinner`, class-based). */ | ||
| function querySpinner() { | ||
| return document.querySelector(".spinner"); | ||
| } | ||
|
|
||
| describe("BookingAssetsSidebar", () => { | ||
| beforeEach(() => { | ||
| fetcherStub = createFetcherStub(); | ||
| }); | ||
|
|
||
| it("eager mode: renders rows from the inline payload without firing a fetch", async () => { | ||
| const user = userEvent.setup(); | ||
| render( | ||
| <BookingAssetsSidebar | ||
| booking={buildBooking({ | ||
| bookingAssets: [ | ||
| buildBookingAsset({ id: "asset-1", title: "Camera A" }), | ||
| buildBookingAsset({ id: "asset-2", title: "Tripod B" }), | ||
| ], | ||
| })} | ||
| /> | ||
| ); | ||
|
|
||
| await user.click(screen.getByRole("button", { name: "2 assets" })); | ||
|
|
||
| expect(screen.getByText(/Assets in "Studio session"/)).toBeInTheDocument(); | ||
| expect(screen.getByText("Camera A")).toBeInTheDocument(); | ||
| expect(screen.getByText("Tripod B")).toBeInTheDocument(); | ||
| expect(screen.getByText("2 items")).toBeInTheDocument(); | ||
| // Eager data is instant: no spinner, and the lazy endpoint is never hit. | ||
| expect(querySpinner()).not.toBeInTheDocument(); | ||
| expect(fetcherStub.load).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("lazy mode: fetches exactly once on open, shows a spinner, then renders rows", async () => { | ||
| const user = userEvent.setup(); | ||
| // Bookings-index shape: `_count` for the trigger label, no pivots. | ||
| const booking = buildBooking({ _count: { bookingAssets: 2 } }); | ||
| const { rerender } = render(<BookingAssetsSidebar booking={booking} />); | ||
|
|
||
| await user.click(screen.getByRole("button", { name: "2 assets" })); | ||
|
|
||
| expect(fetcherStub.load).toHaveBeenCalledTimes(1); | ||
| expect(fetcherStub.load).toHaveBeenCalledWith( | ||
| "/api/bookings/booking-1/assets-sidebar" | ||
| ); | ||
| // In flight: spinner shows, no rows yet. | ||
| expect(querySpinner()).toBeInTheDocument(); | ||
| expect(screen.queryByText("Camera A")).not.toBeInTheDocument(); | ||
|
|
||
| // Resolve the fetch. The real fetcher re-renders the component when | ||
| // data lands; with the stub we mutate + re-render explicitly. | ||
| fetcherStub.data = buildPayload([ | ||
| buildBookingAsset({ id: "asset-1", title: "Camera A" }), | ||
| buildBookingAsset({ id: "asset-2", title: "Tripod B" }), | ||
| ]); | ||
| rerender(<BookingAssetsSidebar booking={booking} />); | ||
|
|
||
| expect(querySpinner()).not.toBeInTheDocument(); | ||
| expect(screen.getByText("Camera A")).toBeInTheDocument(); | ||
| expect(screen.getByText("Tripod B")).toBeInTheDocument(); | ||
| expect(screen.getByText("2 items")).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("lazy mode: closing and reopening renders each row once (refresh, not append)", async () => { | ||
| const user = userEvent.setup(); | ||
| const booking = buildBooking({ _count: { bookingAssets: 1 } }); | ||
| const { rerender } = render(<BookingAssetsSidebar booking={booking} />); | ||
|
|
||
| // First open + resolved fetch. | ||
| await user.click(screen.getByRole("button", { name: "1 assets" })); | ||
| fetcherStub.data = buildPayload([ | ||
| buildBookingAsset({ id: "asset-1", title: "Camera A" }), | ||
| ]); | ||
| rerender(<BookingAssetsSidebar booking={booking} />); | ||
| expect(screen.getAllByText("Camera A")).toHaveLength(1); | ||
|
|
||
| // Close via the sheet's X — content unmounts. | ||
| await user.click(screen.getByRole("button", { name: /close/i })); | ||
| expect(screen.queryByText("Camera A")).not.toBeInTheDocument(); | ||
|
|
||
| // Reopen: the retained payload renders immediately (no spinner flash) | ||
| // and rows are NOT duplicated. The component deliberately re-fetches | ||
| // on each open for freshness — pin that too. | ||
| await user.click(screen.getByRole("button", { name: "1 assets" })); | ||
| expect(screen.getAllByText("Camera A")).toHaveLength(1); | ||
| expect(querySpinner()).not.toBeInTheDocument(); | ||
| expect(fetcherStub.load).toHaveBeenCalledTimes(2); | ||
| }); | ||
|
|
||
| it("lazy mode: a settled error shows the error state with retry instead of an endless spinner", async () => { | ||
| const user = userEvent.setup(); | ||
| const booking = buildBooking({ _count: { bookingAssets: 2 } }); | ||
| const { rerender } = render(<BookingAssetsSidebar booking={booking} />); | ||
|
|
||
| await user.click(screen.getByRole("button", { name: "2 assets" })); | ||
| // Fetch settles with an error payload (booking deleted / permission | ||
| // lost between page load and drawer open). | ||
| fetcherStub.data = { error: { message: "Booking not found" } }; | ||
| rerender(<BookingAssetsSidebar booking={booking} />); | ||
|
|
||
| expect(querySpinner()).not.toBeInTheDocument(); | ||
| expect( | ||
| screen.getByText("Failed to load the booking's assets.") | ||
| ).toBeInTheDocument(); | ||
|
|
||
| // Retry re-fires the fetch: once on open, once from the button. | ||
| await user.click(screen.getByRole("button", { name: "Try again" })); | ||
| expect(fetcherStub.load).toHaveBeenCalledTimes(2); | ||
| expect(fetcherStub.load).toHaveBeenLastCalledWith( | ||
| "/api/bookings/booking-1/assets-sidebar" | ||
| ); | ||
| }); | ||
|
|
||
| it("zero concrete assets + outstanding model requests: trigger stays openable and renders the reservations section", async () => { | ||
| const user = userEvent.setup(); | ||
| render( | ||
| <BookingAssetsSidebar | ||
| booking={buildBooking({ | ||
| // Eager-empty payload (pure Book-by-Model booking). | ||
| bookingAssets: [], | ||
| modelRequests: [buildModelRequest()], | ||
| })} | ||
| /> | ||
| ); | ||
|
|
||
| // 0 concrete assets, but the outstanding reservation keeps the | ||
| // trigger clickable (`hasItems` counts unfulfilled model requests). | ||
| await user.click(screen.getByRole("button", { name: "0 assets" })); | ||
|
|
||
| // quantity 3 − fulfilled 1 = 2 remaining across 1 model. | ||
| expect( | ||
| screen.getByText("Unassigned model reservations (2)") | ||
| ).toBeInTheDocument(); | ||
| expect(screen.getByText("Sony A7 IV")).toBeInTheDocument(); | ||
| expect(screen.getByText("2 remaining")).toBeInTheDocument(); | ||
| // RESERVED is scan-to-assign eligible. | ||
| expect( | ||
| screen.getByRole("link", { name: "Scan to assign" }) | ||
| ).toHaveAttribute("href", "/bookings/booking-1/overview/scan-assets"); | ||
| // The empty-but-present eager payload means: no lazy fetch, no | ||
| // spinner, and an empty assets table below the reservations. | ||
| expect(screen.getByText("0 items")).toBeInTheDocument(); | ||
| expect(querySpinner()).not.toBeInTheDocument(); | ||
| expect(fetcherStub.load).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("zero assets + only fulfilled model requests: trigger is inert and the sheet never opens", async () => { | ||
| const user = userEvent.setup(); | ||
| render( | ||
| <BookingAssetsSidebar | ||
| booking={buildBooking({ | ||
| _count: { bookingAssets: 0 }, | ||
| // Fully-fulfilled requests don't count toward `hasItems`. | ||
| modelRequests: [ | ||
| buildModelRequest({ | ||
| fulfilledQuantity: 3, | ||
| fulfilledAt: new Date("2026-07-01T00:00:00.000Z"), | ||
| }), | ||
| ], | ||
| })} | ||
| /> | ||
| ); | ||
|
|
||
| await user.click(screen.getByRole("button", { name: "0 assets" })); | ||
|
|
||
| expect(screen.queryByText(/Assets in/)).not.toBeInTheDocument(); | ||
| expect(fetcherStub.load).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.