Skip to content

perf(webapp): slim calendar, home, assets-index and bookings loaders - #2738

Open
carlosvirreira wants to merge 6 commits into
mainfrom
perf/p95-under-300ms
Open

perf(webapp): slim calendar, home, assets-index and bookings loaders#2738
carlosvirreira wants to merge 6 commits into
mainfrom
perf/p95-under-300ms

Conversation

@carlosvirreira

@carlosvirreira carlosvirreira commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Makes four hot loaders fetch only what their routes actually render. No schema changes, no new dependencies, no caching layer.

Headline: /calendar server p95 drops 136ms → 28ms and /home 60ms → 33ms, measured back-to-back against this branch and main on the same seeded database. /bookings ships a 343KB → 109KB document.

Why these routes were slow

Profiled with a V8 sampling profiler inside the production server plus pg_stat_statements, against a locally seeded org (5,000 assets, 400 bookings, 120 kits, 15,000 notes). Every hotspot was the same shape: a loader fetching a subtree the page never reads.

Route What it fetched What it renders
/calendar full bookingAssets subtree: ~1,775 asset rows + QR/barcode/kit pivots per request (~1.5MB through Prisma) event bars: name, dates, status, custodian name, tags
/home the same asset subtree on 4 separate getBookings calls booking scalars + a count per widget
/bookings per-booking pivots for every row a table of scalars; the pivots only feed the assets drawer
/assets ranked all matching rows, then paginated one page of 20

What changed

1. getBookings gains includeAssets (default true) and takeCap. When includeAssets: false, the bookingAssets key is omitted from the Prisma include entirely. Every existing caller keeps today's behaviour by default.

2. /calendar opts out of assets, skips a count query it never read, slims the custodian/creator selects to the five fields the event mapping uses, and replaces an unbounded takeAll with takeCap: 1000 (warn-logged if a workspace ever hits it — previously this query had no ceiling at all).

3. /home opts all four calls out of assets. Two correctness fixes came out of the same pass:

  • the custodian-merge call passed perPage: 1000, which silently clamps to 20 — so "top custodians" was computed from the first 20 active bookings. It now uses takeCap and sees them all.
  • location distribution was a per-location count over the pivot (11.2ms/request); it is now groupBy-first (under 1ms).
  • the onboarding-checklist query joined the loader's existing Promise.all and a duplicate custody count was dropped.

4. /assets advanced-index CTE paginates before ranking on the default path, so Postgres does a top-N sort on the existing (organizationId, createdAt, id) index instead of sorting every matching row to feed ROW_NUMBER. It also materialises only the sort-key columns actually referenced by the active ORDER BY. Search and custom-field sorts route through the previous SQL byte-identically.

5. /bookings stops shipping the assets-drawer payload. A new authenticated resource route (GET /api/bookings/:bookingId/assets-sidebar) returns the identical shape on open. BookingAssetsSidebar is dual-mode: eager callers (booking overview, the asset/kit/user/me bookings tabs) are untouched and never fetch; the index fetches on open with a spinner, an error state with retry, and shouldRevalidate: false so closed drawers don't refetch after page actions.

Measurements

Open-loop 10 rps sustained per route, 25s, production build, same seeded DB, main and this branch measured minutes apart. Two samples each; the numbers below are the second (the first had one machine-noise outlier on an untouched route, which is why control routes are included).

Route main p50 / p95 this branch p50 / p95
/calendar 105.0 / 136.0 ms 23.9 / 28.0 ms
/home 48.7 / 60.5 ms 25.7 / 33.2 ms
/assets 34.8 / 47.2 ms 30.7 / 41.2 ms
/bookings 45.3 / 55.0 ms 51.8 / 60.9 ms
/kits (control, untouched) 26.0 / 31.8 ms 25.8 / 31.5 ms
/locations (control, untouched) 18.3 / 25.5 ms 17.9 / 25.0 ms

Document sizes, same session:

Route main this branch
/bookings 343,556 B 108,935 B
/home 263,501 B 95,308 B
/calendar 155,306 B 155,306 B (HTML unchanged; the win is server CPU and Prisma payload, not markup)

On /bookings being flat here: it trades the pivot payload for one bounded query that computes the "Includes unavailable assets" badge, so wall-clock is a wash at this data size while the document shrinks by 3x. The payload reduction is the durable win; it scales with assets-per-booking, which this seed keeps modest.

Rig caveats, stated plainly: this is a local single-process rig, not production. Absolute numbers are optimistic (no network RTT, warm cache, one Node process); the relative change and the payload reduction are the meaningful signals. The measurement harness is open-loop (arrivals independent of completions) because a closed loop measures queue collapse rather than service time.

Behaviour notes for review

  • Only one intentional UX change: on /bookings, drawer content now loads when you open a row (spinner on first open; reopening renders the cached payload immediately and refreshes in the background). Open-drawer rendering is byte-identical because the service and the resource route share one include constant.
  • The resource route mirrors the index's read gate: requirePermission (booking/read), org scoping, DRAFT-creator visibility, and the restricted-role custody scope — a SELF_SERVICE/BASE user cannot fetch a drawer for a booking the list wouldn't show them.
  • Calendar windows with >1,000 overlapping bookings now truncate (logged). Previously unbounded.
  • Known gap, unchanged by this PR: the index drawer still renders no InsufficientStockBadge/PendingReturnBadge, because availableUnitsByAsset is produced only by the booking-overview loader on main too — neither bookings-list surface has ever supplied it. Adding it to the index would be a new feature on a perf-sensitive surface; happy to do it separately if you want those badges there.

Merge history

This branch was merged with main twice, most recently to integrate the quantity-tracked availability wave. The interesting part of that resolution: ListBookingsContent (main's new shared row component) had bookingAssets: rawItem.bookingAssets ?? [], and an empty array would read as "eager with zero assets" and silently suppress the drawer fetch. The prop is now optional with undefined deliberately preserved, and the code says so, so the normalisation doesn't come back. Main's stock-conflict pill, disposition maps, and QT badge props are all preserved.

Testing

  • Full suite green: 3,847 tests, lint, typecheck.
  • New unit tests pin the drawer's dual-mode contract (eager renders without fetching, lazy fetches exactly once per open, reopen doesn't duplicate rows, error state with retry, book-by-model trigger states). Main's QT stock-badge suite is kept intact in its own file.
  • SQL identity for the assets-index rewrite verified against the seeded DB: md5 of page-1 and page-2 id lists identical across default and name sorts.
  • Browser-verified on the seeded rig at desktop (1247px) and mobile (375px): drawer opens with one fetch, all rows render, reopen doesn't duplicate, eager child pages make zero sidebar requests.
  • An adversarial multi-agent review pass over the diff; every confirmed finding was fixed on this branch.

🤖 Generated with Claude Code

Profiled with a V8 sampling profiler and pg_stat_statements against a seeded
local rig (5k assets, 400 bookings). The dominant costs were loader payloads
fetching data the routes never render:

- calendar: getBookings included the full bookingAssets subtree (~1,775 asset
  rows plus QR/kit pivots per request) for events that render scalars only.
  getBookings gains includeAssets (default true); the calendar opts out, skips
  its unused count, slims custodian/creator selects, and caps the window fetch
  at 1,000 rows (warn-logged when hit).
- home: all four getBookings calls opt out of assets; location distribution is
  groupBy-first on AssetLocation; the checklist query joins the loader's
  Promise.all and drops a duplicate custody count.
- assets index: the advanced-index CTE paginates before ranking on the default
  path and materializes only active sort-key columns. Page output verified
  identical (md5 of page ids, two sorts, two pages).
- bookings index: the row-expansion drawer payload (~99% never opened) moves to
  a new authenticated resource route fetched on open, with an in-drawer
  error/retry state and shouldRevalidate false so closed drawers skip action
  revalidation. The /bookings document shrinks from 343KB to 108KB.

Measured on the rig at 10 rps sustained per route: calendar p95 1473ms -> 29ms,
home 764ms -> 45ms, bookings index 1200ms -> 137ms; every core route now holds
p95 under 300ms at 10 and 15 rps sustained load.
@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown

🩺 React Doctor — webapp

Findings on the files changed by this PR:

  • 0 errors
  • 1 warning — advisory
⚠️ 1 warnings (click to expand)
  • react-doctor/no-giant-component (1)
    • apps/webapp/app/components/booking/booking-assets-sidebar.tsx:722

Run locally with pnpm webapp:doctor for a full scan, or cd apps/webapp && pnpm exec react-doctor . --diff for the same diff-only view.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5188ea5a56

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread apps/webapp/app/components/booking/booking-assets-sidebar.test.tsx
@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The PR adds lazy booking-sidebar loading with authorized aggregation, narrows booking and dashboard queries, caps calendar results, optimizes asset sorting pagination, and adds sidebar, SQL-shape, and performance-seeding changes.

Changes

Booking assets sidebar

Layer / File(s) Summary
Sidebar contracts and asset include
apps/webapp/app/components/booking/booking-assets-sidebar.tsx, apps/webapp/app/modules/booking/constants.ts
The sidebar supports eager or count-based bookings. The shared include defines the drawer asset payload.
Sidebar loader and aggregates
apps/webapp/app/routes/api+/bookings.$bookingId.assets-sidebar.ts
The loader authorizes bookings, loads assets, computes disposition and checkout maps, and returns standardized errors.
Lazy sidebar integration
apps/webapp/app/routes/_layout+/bookings._index.tsx, apps/webapp/app/components/booking/list-bookings-content.tsx, apps/webapp/app/components/booking/booking-assets-sidebar.tsx
The bookings index uses counts and unavailable-booking IDs. The sidebar fetches details on open and renders loading, retry, progress, and reservation states.
Sidebar behavior tests
apps/webapp/app/components/booking/booking-assets-sidebar*.test.tsx
Tests cover eager rendering, lazy loading, refresh, retry handling, reservation-only bookings, and empty-booking triggers.

Booking and dashboard query loading

Layer / File(s) Summary
Booking query options and calendar cap
apps/webapp/app/modules/booking/service.server.ts
getBookings can omit assets and cap rows. Calendar loading uses the bounded configuration.
Dashboard loader data paths
apps/webapp/app/routes/_layout+/home.tsx, apps/webapp/app/utils/dashboard.server.ts
Dashboard booking queries omit assets. Location counts use grouped queries. Checklist values are fetched and composed separately.

Advanced asset query pagination

Layer / File(s) Summary
Sort-key projection and pagination
apps/webapp/app/modules/asset/query.server.ts, apps/webapp/app/modules/asset/query.server.test.ts
Direct sort columns are selected only when active. The default query paginates slim rows before ranking. Tests verify the projection changes.

Performance test seeding

Layer / File(s) Summary
Deterministic performance database seeder
apps/webapp/scripts/seed-perf.ts
The seeder creates batched organization, asset, booking, custody, note, and scan data with deterministic values and verifies row counts.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant BookingAssetsSidebar
  participant AssetsSidebarLoader
  participant Database
  User->>BookingAssetsSidebar: open assets sheet
  BookingAssetsSidebar->>AssetsSidebarLoader: request booking assets
  AssetsSidebarLoader->>Database: authorize booking and load aggregates
  Database-->>AssetsSidebarLoader: assets, consumption logs, checkout rows
  AssetsSidebarLoader-->>BookingAssetsSidebar: return sidebar payload
  BookingAssetsSidebar-->>User: render assets and progress
Loading

Possibly related issues

Possibly related PRs

Suggested reviewers: donkoko

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 61.54% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the primary performance changes to the calendar, home, assets-index, and bookings loaders.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch perf/p95-under-300ms

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
apps/webapp/app/modules/booking/service.server.ts (1)

9042-9060: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Use takeCap for the home dashboard merge query

page: 1, perPage: 1000 is clamped to take: 20, so this ONGOING/OVERDUE custodian merge only receives 20 bookings. Add a bounded takeCap here (or otherwise avoid silently capping the query), since the other home calls only fetch 5 rows and takeAll would make the query unbounded.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/webapp/app/modules/booking/service.server.ts` around lines 9042 - 9060,
Update the home dashboard’s ONGOING/OVERDUE custodian merge query to pass a
bounded takeCap so the requested 1000 bookings are not silently limited to the
default 20. Keep the query bounded rather than enabling takeAll, and preserve
the existing pagination behavior for other callers.
🧹 Nitpick comments (1)
apps/webapp/app/modules/asset/query.server.ts (1)

2736-2753: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a cheap-phase assertion for direct sort-key gating.

The current query.server.test.ts assertion searches the full SQL, where a.value and a.quantity also appear in the heavy lateral projection. It cannot detect regressions in this slim CTE. Slice before sorted_asset_query and assert default sorting omits value/quantity while value:asc or quantity:asc includes the respective key.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/webapp/app/modules/asset/query.server.ts` around lines 2736 - 2753, The
test assertion for direct sort-key gating must inspect only the slim CTE, not
the full generated SQL. In query.server.test.ts, slice the SQL before
sorted_asset_query and assert default sorting excludes a.value and a.quantity,
while value:asc and quantity:asc include their corresponding direct sort keys;
update the existing assertions around DIRECT_SORT_KEY_SELECTS behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@apps/webapp/app/routes/_layout`+/home.tsx:
- Around line 405-411: Exclude soft-deleted members from onboarding completion:
update the directCustodians source query in
apps/webapp/app/routes/_layout+/home.tsx lines 405-411 to filter active team
members with deletedAt: null before deriving hasCustodies; also update the
team-member count in apps/webapp/app/utils/dashboard.server.ts lines 275-277 to
include deletedAt: null.

---

Outside diff comments:
In `@apps/webapp/app/modules/booking/service.server.ts`:
- Around line 9042-9060: Update the home dashboard’s ONGOING/OVERDUE custodian
merge query to pass a bounded takeCap so the requested 1000 bookings are not
silently limited to the default 20. Keep the query bounded rather than enabling
takeAll, and preserve the existing pagination behavior for other callers.

---

Nitpick comments:
In `@apps/webapp/app/modules/asset/query.server.ts`:
- Around line 2736-2753: The test assertion for direct sort-key gating must
inspect only the slim CTE, not the full generated SQL. In query.server.test.ts,
slice the SQL before sorted_asset_query and assert default sorting excludes
a.value and a.quantity, while value:asc and quantity:asc include their
corresponding direct sort keys; update the existing assertions around
DIRECT_SORT_KEY_SELECTS behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: b641f99a-70e0-4fb6-a15a-abfc17311163

📥 Commits

Reviewing files that changed from the base of the PR and between 4a3be9a and 5188ea5.

📒 Files selected for processing (9)
  • apps/webapp/app/components/booking/booking-assets-sidebar.tsx
  • apps/webapp/app/modules/asset/query.server.ts
  • apps/webapp/app/modules/booking/constants.ts
  • apps/webapp/app/modules/booking/service.server.ts
  • apps/webapp/app/routes/_layout+/bookings._index.tsx
  • apps/webapp/app/routes/_layout+/home.tsx
  • apps/webapp/app/routes/api+/bookings.$bookingId.assets-sidebar.ts
  • apps/webapp/app/utils/dashboard.server.ts
  • apps/webapp/test/components/booking/booking-assets-sidebar.test.tsx

Comment thread apps/webapp/app/routes/_layout+/home.tsx
- co-locate the sidebar dual-mode test with its component (repo test convention;
  flagged by Codex review)
- home custodian merge: pass takeCap 1000 so the ONGOING/OVERDUE merge sees every
  booking instead of the perPage-clamped first 20 (flagged by CodeRabbit; uses the
  takeCap primitive this PR introduces)
- assets-index query test: assert direct sort-key gating against the sliced cheap
  phase instead of the full SQL, where the heavy lateral also matches (CodeRabbit
  nitpick); default sort now provably omits value/quantity from the slim CTE
@carlosvirreira

Copy link
Copy Markdown
Contributor Author

Replies to the two CodeRabbit comments that could not be posted inline:

Outside-diff: use takeCap for the home dashboard ONGOING/OVERDUE custodian merge (service.server.ts 9042-9060). Agreed and fixed in cf8d051: the call now passes takeCap: 1000 instead of the silently clamped perPage: 1000, so the top-custodians widget counts every active/overdue booking rather than the first 20. This was a pre-existing clamp on main; the takeCap primitive introduced by this PR is exactly the bounded escape hatch for it.

Nitpick: cheap-phase assertion for direct sort-key gating (query.server.ts 2736-2753). Agreed and fixed in cf8d051: the test now slices the SQL before sorted_asset_query and asserts the default sort omits a.value/a.quantity from the slim CTE while valuation:asc / quantity:asc pull exactly their key back in. The old full-SQL assertion could not catch a cheap-phase regression because the heavy lateral emits the same aliases.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@apps/webapp/app/modules/asset/query.server.test.ts`:
- Around line 1129-1135: Strengthen the slim SELECT assertions around cheap so
each active-sort case also verifies that the other sort key is absent: the
valuation sort must exclude assetQuantity, and the quantity sort must exclude
assetValue. Keep the existing inclusion checks for the requested sort column.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 8f706659-f7bc-4e07-bb14-f840297d85d8

📥 Commits

Reviewing files that changed from the base of the PR and between 5188ea5 and cf8d051.

📒 Files selected for processing (3)
  • apps/webapp/app/components/booking/booking-assets-sidebar.test.tsx
  • apps/webapp/app/modules/asset/query.server.test.ts
  • apps/webapp/app/routes/_layout+/home.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/webapp/app/routes/_layout+/home.tsx

Comment thread apps/webapp/app/modules/asset/query.server.test.ts Outdated
Carlos Virreira and others added 3 commits July 23, 2026 14:57
Follow-up to CodeRabbit's re-review: inclusion-only assertions would pass even if
gating regressed to all-or-nothing. Valuation sort legitimately selects BOTH value
and quantity (it orders by total value = assetValue * assetQuantity), so the
exclusion is asserted against an unrelated key there, and against value on the
single-key quantity sort.
Integrates the quantity-tracked availability wave with the lazy assets drawer.

- ListBookingsContent (new shared row component on main) goes dual-mode:
  bookingAssets is optional and deliberately NOT normalised to [] — an empty
  array would read as 'eager with zero assets' and suppress the drawer fetch.
  The 'Includes unavailable assets' badge derives from the pivots when they
  are eager (child bookings tabs) and from the index loader's bounded
  bookingsWithUnavailableAssets query when they are not.
- BookingAssetsSidebar keeps main's qty-progress + stock-badge props and
  falls back to the lazily fetched payload when a caller omits them.
- getBookings keeps main's include shape, hoisted to a module const so the
  includeAssets gate can omit it without changing the type for other callers.
- The two independently-added test suites for the sidebar are split by
  concern: QT stock badges stay in booking-assets-sidebar.test.tsx (plus a
  useFetcher stub the dual-mode component now requires), and the eager/lazy
  contract moves to booking-assets-sidebar-dual-mode.test.tsx.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
apps/webapp/app/components/booking/booking-assets-sidebar.tsx (1)

725-749: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Add availableUnitsByAsset to the lazy sidebar payload.

The bookings-index renderer passes booking without availableUnitsByAsset, and assets-sidebar only returns bookingAssets, dispositionedByAsset, dispositionBreakdownByAsset, and checkedOutByAsset. The component then reads undefined for availability, so resolveQtyStockBadgeVariant returns null and the bookings-index asset drawer cannot render InsufficientStockBadge or PendingReturnBadge. Either compute the workspace-availability map in assets-sidebar and merge it like the other maps, or remove the stock-badge support for the index drawer if this is intentional.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/webapp/app/components/booking/booking-assets-sidebar.tsx` around lines
725 - 749, Update the lazy assets-sidebar loader and its consumer to provide and
merge an availableUnitsByAsset map alongside the other quantity maps, using the
existing availability calculation used by eager callers. Ensure the resolved
availability reaches resolveQtyStockBadgeVariant so the bookings-index drawer
can render InsufficientStockBadge and PendingReturnBadge.
🧹 Nitpick comments (3)
apps/webapp/scripts/seed-perf.ts (1)

51-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Derive TARGETS.customFields from the plan list.

TARGETS.customFields is 8 and buildCustomFieldPlans() returns 8 hardcoded entries. The summary log at line 533 reports the constant, not the inserted row count. If a plan entry is added or removed, the log becomes wrong. Log customFieldPlans.length instead, or derive the target from the plan.

Also applies to: 274-285

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/webapp/scripts/seed-perf.ts` around lines 51 - 63, Update the
custom-field target reporting to derive its value from buildCustomFieldPlans()
rather than the hardcoded TARGETS.customFields constant. Ensure the summary log
uses customFieldPlans.length so it always reflects the number of plans inserted,
including when plan entries change.
apps/webapp/app/routes/_layout+/bookings._index.tsx (1)

212-260: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Run the stock-conflict decoration and unavailable-assets lookup concurrently.

decoratedBookings (lines 218-221) and bookingsWithUnavailableAssets (lines 234-260) both depend only on bookings, not on each other's result. They currently run as two sequential awaits, adding one extra round trip of latency to every /bookings request. Since this PR's goal is reducing loader latency, run them with Promise.all instead.

⚡ Proposed fix to parallelize the two independent queries
-    const decoratedBookings = await decorateBookingsWithStockConflicts({
-      bookings,
-      organizationId,
-    });
-
-    const bookingIdsOnPage = bookings.map((b) => b.id);
-    const bookingsWithUnavailableAssets =
-      bookingIdsOnPage.length > 0
-        ? (
-            await db.booking.findMany({
-              where: {
-                id: { in: bookingIdsOnPage },
-                organizationId,
-                bookingAssets: {
-                  some: {
-                    asset: {
-                      OR: [
-                        { availableToBook: false },
-                        { custody: { some: {} } },
-                      ],
-                    },
-                  },
-                },
-              },
-              select: { id: true },
-            })
-          ).map((b) => b.id)
-        : [];
+    const bookingIdsOnPage = bookings.map((b) => b.id);
+    const [decoratedBookings, bookingsWithUnavailableAssets] = await Promise.all([
+      decorateBookingsWithStockConflicts({ bookings, organizationId }),
+      bookingIdsOnPage.length > 0
+        ? db.booking
+            .findMany({
+              where: {
+                id: { in: bookingIdsOnPage },
+                organizationId,
+                bookingAssets: {
+                  some: {
+                    asset: {
+                      OR: [
+                        { availableToBook: false },
+                        { custody: { some: {} } },
+                      ],
+                    },
+                  },
+                },
+              },
+              select: { id: true },
+            })
+            .then((rows) => rows.map((b) => b.id))
+        : Promise.resolve<string[]>([]),
+    ]);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/webapp/app/routes/_layout`+/bookings._index.tsx around lines 212 - 260,
Run decorateBookingsWithStockConflicts and the unavailable-assets
db.booking.findMany lookup concurrently with Promise.all, since both depend only
on bookings and organizationId. Preserve the existing result values and
empty-page behavior, then destructure the two resolved results for subsequent
loader use.
apps/webapp/app/components/booking/booking-assets-sidebar.tsx (1)

68-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the duplicated booking-asset Prisma select shared by both files. Both files independently declare the same nested asset select for the booking-asset payload. This duplication has already caused drift: booking-assets-sidebar.tsx's select omits consumptionType while list-bookings-content.tsx's select still includes it.

  • apps/webapp/app/components/booking/booking-assets-sidebar.tsx#L68-L133: extract the nested asset select from BookingWithAssets into a shared, exported select/type (for example alongside the shared include already referenced in apps/webapp/app/modules/booking/constants.ts) and import it here.
  • apps/webapp/app/components/booking/list-bookings-content.tsx#L102-L208: replace the inline duplicate asset select in ListBookingsContentProps with the same shared select/type used by booking-assets-sidebar.tsx.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/webapp/app/components/booking/booking-assets-sidebar.tsx` around lines
68 - 133, Extract the duplicated nested asset select from BookingWithAssets in
apps/webapp/app/components/booking/booking-assets-sidebar.tsx#L68-L133 into a
shared exported select/type, colocated with the existing booking include in
apps/webapp/app/modules/booking/constants.ts, and preserve all fields including
consumptionType. Replace the inline asset select in ListBookingsContentProps at
apps/webapp/app/components/booking/list-bookings-content.tsx#L102-L208 with the
shared select/type, importing it from the shared module so both payloads remain
synchronized.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@apps/webapp/scripts/seed-perf.ts`:
- Around line 42-45: Remove the unused ExtendedPrismaClient type import from the
seed-perf module, leaving the Prisma and createDatabaseClient imports unchanged.

---

Outside diff comments:
In `@apps/webapp/app/components/booking/booking-assets-sidebar.tsx`:
- Around line 725-749: Update the lazy assets-sidebar loader and its consumer to
provide and merge an availableUnitsByAsset map alongside the other quantity
maps, using the existing availability calculation used by eager callers. Ensure
the resolved availability reaches resolveQtyStockBadgeVariant so the
bookings-index drawer can render InsufficientStockBadge and PendingReturnBadge.

---

Nitpick comments:
In `@apps/webapp/app/components/booking/booking-assets-sidebar.tsx`:
- Around line 68-133: Extract the duplicated nested asset select from
BookingWithAssets in
apps/webapp/app/components/booking/booking-assets-sidebar.tsx#L68-L133 into a
shared exported select/type, colocated with the existing booking include in
apps/webapp/app/modules/booking/constants.ts, and preserve all fields including
consumptionType. Replace the inline asset select in ListBookingsContentProps at
apps/webapp/app/components/booking/list-bookings-content.tsx#L102-L208 with the
shared select/type, importing it from the shared module so both payloads remain
synchronized.

In `@apps/webapp/app/routes/_layout`+/bookings._index.tsx:
- Around line 212-260: Run decorateBookingsWithStockConflicts and the
unavailable-assets db.booking.findMany lookup concurrently with Promise.all,
since both depend only on bookings and organizationId. Preserve the existing
result values and empty-page behavior, then destructure the two resolved results
for subsequent loader use.

In `@apps/webapp/scripts/seed-perf.ts`:
- Around line 51-63: Update the custom-field target reporting to derive its
value from buildCustomFieldPlans() rather than the hardcoded
TARGETS.customFields constant. Ensure the summary log uses
customFieldPlans.length so it always reflects the number of plans inserted,
including when plan entries change.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 342425e0-141f-409c-9b6c-156d3ff41030

📥 Commits

Reviewing files that changed from the base of the PR and between 119415d and 485209b.

📒 Files selected for processing (9)
  • apps/webapp/app/components/booking/booking-assets-sidebar-dual-mode.test.tsx
  • apps/webapp/app/components/booking/booking-assets-sidebar.test.tsx
  • apps/webapp/app/components/booking/booking-assets-sidebar.tsx
  • apps/webapp/app/components/booking/list-bookings-content.tsx
  • apps/webapp/app/modules/asset/query.server.test.ts
  • apps/webapp/app/modules/asset/query.server.ts
  • apps/webapp/app/modules/booking/service.server.ts
  • apps/webapp/app/routes/_layout+/bookings._index.tsx
  • apps/webapp/scripts/seed-perf.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/webapp/app/modules/booking/service.server.ts

Comment thread apps/webapp/scripts/seed-perf.ts Outdated
seed-perf.ts is an operator-only script for the local perf rig, not product
code — it was swept in by a blanket stage on the merge commit. Removing it
also resolves the unused-import lint finding CodeRabbit raised against it.
@carlosvirreira

Copy link
Copy Markdown
Contributor Author

Reply to the outside-diff Major (booking-assets-sidebar.tsx 725-749, Add availableUnitsByAsset to the lazy sidebar payload):

The observation is accurate but it is pre-existing on main, not introduced here, so I have deliberately left it out of this PR.

Verified on origin/main: availableUnitsByAsset is produced by exactly one loader, the booking overview (bookings.$bookingId.overview.tsx:893, via buildAvailableUnitsByAsset). Neither bookings-list surface has ever supplied it: main's ListBookingsContent renders <BookingAssetsSidebar booking={item} dispositionedByAsset={…} dispositionBreakdownByAsset={…} checkedOutByAsset={…} /> with no availability prop, so resolveQtyStockBadgeVariant already returned null for every index-drawer row before this branch existed. The stock badges are an overview-drawer feature today.

What this PR does change is the other three maps, and those are preserved: the index used to compute them page-wide in the loader, and the resource route now returns all three (dispositionedByAsset, dispositionBreakdownByAsset, checkedOutByAsset) so the drawer keeps its qty-progress and partial-check-in rendering. The component prefers an explicitly-passed prop and falls back to the fetched payload, so eager callers (booking overview, child bookings tabs) are byte-identical.

Adding availability to the index drawer would be a new feature (it needs the per-asset workspace-availability computation run for the whole page or per drawer open), and it would land on the perf-sensitive surface this PR is slimming. Happy to do it as a follow-up if you want the badges there, @DonKoko — it is a product call, not a merge blocker.

@carlosvirreira carlosvirreira changed the title perf(webapp): slim core loader payloads to cut route p95s perf(webapp): slim calendar, home, assets-index and bookings loaders Jul 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants