perf: optimize loaders for assets, bookings, and settings routes#2483
Merged
perf: optimize loaders for assets, bookings, and settings routes#2483
Conversation
Add a Hono middleware that measures total request duration and exposes it via the Server-Timing HTTP header. Only active when NODE_ENV is not production to avoid leaking timing info.
- /assets: parallelize filter/asset queries in Promise.all, include booking custodian data in initial query to eliminate N+1 re-fetch, increase image URL expiration to 72h - /bookings: remove N+1 asset.bookings select from getBookings() - /settings/team/users: reuse currentOrganization from requirePermission instead of duplicate query, limit teamMembers fetch to take:1 - /bookings (shared): combine getTeamMembersForNotify into a single Prisma query instead of two sequential calls - schema: add status index on Booking table for faster status filtering in asset→booking joins
…imizations # Conflicts: # apps/webapp/server/index.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7153531c21
ℹ️ 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".
WalkthroughBooking-based custodian enrichment now uses included booking data synchronously; several independent asset-related queries are parallelized; main-image signed URLs extended to 72h; server-timing Hono middleware added and registered early; a non-unique index on Booking.status was added. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Server as Hono Server
participant Timing as serverTiming()
participant HostHdr as ensureHostHeaders()
participant Handler as Route Handler
participant DB as Database
rect rgba(200,200,255,0.5)
Client->>Server: HTTP request
Server->>Timing: invoke serverTiming middleware
end
rect rgba(200,255,200,0.5)
Timing->>HostHdr: await next()
HostHdr->>Handler: await next()
Handler->>DB: perform queries (Promise.all / findMany)
DB-->>Handler: query results (includes bookings/custodians)
Handler->>Handler: updateAssetsWithBookingCustodians (synchronous mapper)
Handler-->>HostHdr: response
HostHdr-->>Timing: response
end
rect rgba(255,200,200,0.5)
Timing-->>Server: append Server-Timing header (non-prod)
Server-->>Client: HTTP response (with Server-Timing)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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/asset/fields.ts (1)
218-243:⚠️ Potential issue | 🟠 MajorPreserve custodian fields in the date-range
bookingsoverride.This branch replaces the default
bookingsselect and dropscustodianTeamMember/custodianUser.updateAssetsWithBookingCustodians()still runs on simple-mode results, so checked-out assets lose their syntheticcustody.custodianas soon asbookingFrom/bookingToare present.🛠️ Proposed fix
bookings: { where: { status: { in: unavailableBookingStatuses }, OR: [ { from: { lte: bookingTo }, to: { gte: bookingFrom }, }, { from: { gte: bookingFrom }, to: { lte: bookingTo }, }, ], }, select: { from: true, to: true, status: true, id: true, name: true, + custodianTeamMember: true, + custodianUser: { + select: { + firstName: true, + lastName: true, + displayName: true, + profilePicture: true, + }, + }, }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/modules/asset/fields.ts` around lines 218 - 243, The override of the bookings include (inside the branch checking bookingFrom, bookingTo, unavailableBookingStatuses) replaces the default select and omits custodian fields, causing updateAssetsWithBookingCustodians to lose custody info; fix by adding the custodian fields back into the bookings.select (include custodianTeamMember and custodianUser or the specific nested custody fields your default include uses) so that bookings still return custody data when bookingFrom/bookingTo are present, leaving the rest of the where/OR logic unchanged.
🧹 Nitpick comments (2)
apps/webapp/app/routes/_layout+/settings.team.users.tsx (1)
56-70: Clarifyorganizationnullability assumptions in the loader.Line 56 treats
organizationas optional (organization?.type), while Line 70 assumes it is always present (organization.name). Consider adding an explicit guard (or making both accesses non-optional) to keep this path robust against future contract changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/routes/_layout`+/settings.team.users.tsx around lines 56 - 70, The code assumes `organization` may be undefined in one place (organization?.type) but unconditionally accesses `organization.name` later; add an explicit guard right after loading `organization` to ensure it's present and handle the missing case consistently (e.g., redirect to "/settings/general" or return a 404 Response) before any use of `organization` (so both the `if (organization?.type === "PERSONAL")` check and the HeaderData creation use a non-null `organization`); ensure the same contract is applied for any downstream calls like getPaginatedAndFilterableSettingUsers that expect organizationId.apps/webapp/app/utils/one-week-from-now.ts (1)
11-14: Add JSDoc for this exported helper.This new utility is exported, so it should carry the same short JSDoc as the other shared helpers in this layer.
As per coding guidelines, "All code must include inline documentation and JSDoc comments... Every exported function, component, and type must have a JSDoc comment describing parameters, return values, and thrown errors."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/utils/one-week-from-now.ts` around lines 11 - 14, Add a short JSDoc block above the exported helper threeDaysFromNow describing that it returns a Date three days from now; document that there are no parameters, that it returns a Date, and note that it does not throw errors (or may throw if Date construction fails). Ensure the JSDoc follows the project's style used by other shared helpers and is placed immediately above the export for the threeDaysFromNow function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/webapp/app/modules/asset/service.server.ts`:
- Around line 3022-3035: The custodian name is synthesized only from
firstName/lastName and ignores custodianUser.displayName, causing blank names
for users who only have displayName; in the branch that returns
custody.custodian (where custodianUser is available) update the name
construction to use custodianUser.displayName as a fallback (e.g., name =
custodianUser.firstName/lastName combined or, if empty,
custodianUser.displayName), and ensure profile fields still map (firstName,
lastName, profilePicture) so consumers get displayName when first/last are
missing; look for the return block that spreads ...a and builds
custody.custodian to change the name logic accordingly.
- Around line 3175-3177: The mainImageExpiration value (set via
threeDaysFromNow()) can outlive the actual signed URL lifetime from
createSignedUrl(), causing broken images until refresh; update the logic in the
asset URL refresh flow to derive mainImageExpiration from the real signed URL
TTL returned/used by createSignedUrl() (or a shared signedUrlTTL constant)
instead of hardcoding threeDaysFromNow(), and ensure any signer configuration
that mints signed URLs is updated consistently so the app gate
(mainImageExpiration) and the signer use the same lifetime.
In `@apps/webapp/server/index.ts`:
- Around line 85-87: The serverTiming middleware is registered after
ensureHostHeaders() and runWithRequestCache(), so its metric misses time spent
in those earlier middlewares; move the server.use("*", serverTiming()) call to
be the very first middleware registration (before ensureHostHeaders() and
runWithRequestCache()) so serverTiming() wraps the full request lifecycle and
emits an end-to-end duration metric for each request.
---
Outside diff comments:
In `@apps/webapp/app/modules/asset/fields.ts`:
- Around line 218-243: The override of the bookings include (inside the branch
checking bookingFrom, bookingTo, unavailableBookingStatuses) replaces the
default select and omits custodian fields, causing
updateAssetsWithBookingCustodians to lose custody info; fix by adding the
custodian fields back into the bookings.select (include custodianTeamMember and
custodianUser or the specific nested custody fields your default include uses)
so that bookings still return custody data when bookingFrom/bookingTo are
present, leaving the rest of the where/OR logic unchanged.
---
Nitpick comments:
In `@apps/webapp/app/routes/_layout`+/settings.team.users.tsx:
- Around line 56-70: The code assumes `organization` may be undefined in one
place (organization?.type) but unconditionally accesses `organization.name`
later; add an explicit guard right after loading `organization` to ensure it's
present and handle the missing case consistently (e.g., redirect to
"/settings/general" or return a 404 Response) before any use of `organization`
(so both the `if (organization?.type === "PERSONAL")` check and the HeaderData
creation use a non-null `organization`); ensure the same contract is applied for
any downstream calls like getPaginatedAndFilterableSettingUsers that expect
organizationId.
In `@apps/webapp/app/utils/one-week-from-now.ts`:
- Around line 11-14: Add a short JSDoc block above the exported helper
threeDaysFromNow describing that it returns a Date three days from now; document
that there are no parameters, that it returns a Date, and note that it does not
throw errors (or may throw if Date construction fails). Ensure the JSDoc follows
the project's style used by other shared helpers and is placed immediately above
the export for the threeDaysFromNow function.
🪄 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
Run ID: b63846f0-e23a-4adb-bd02-1005a206941f
📒 Files selected for processing (13)
apps/webapp/app/modules/asset/data.server.tsapps/webapp/app/modules/asset/fields.tsapps/webapp/app/modules/asset/service.server.tsapps/webapp/app/modules/booking/service.server.tsapps/webapp/app/modules/settings/service.server.tsapps/webapp/app/modules/team-member/service.server.tsapps/webapp/app/routes/_layout+/admin-dashboard+/org.$organizationId.assets.tsxapps/webapp/app/routes/_layout+/settings.team.users.tsxapps/webapp/app/utils/one-week-from-now.tsapps/webapp/server/index.tsapps/webapp/server/timing.server.tspackages/database/prisma/migrations/20260407140322_add_status_index_to_booking_table/migration.sqlpackages/database/prisma/schema.prisma
💤 Files with no reviewable changes (1)
- apps/webapp/app/modules/booking/service.server.ts
DonKoko
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Performance-focused server-side optimizations for the Remix webapp’s worst-performing routes (/assets, /bookings, /settings/team/users), plus shared query and observability improvements.
Changes:
- Optimizes asset index loaders by parallelizing independent queries, inlining booking custodian data to avoid N+1 re-fetches, and extending signed URL expirations.
- Fixes
/bookingstimeout risk by removing an unused nestedasset.bookingsselection that could load excessive related rows. - Reduces duplicate work in team settings/users and notification-related queries; adds Booking(status) index and dev-only Server-Timing middleware.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/database/prisma/schema.prisma | Adds Prisma-level index on Booking.status to speed status filtering. |
| packages/database/prisma/migrations/20260407140322_add_status_index_to_booking_table/migration.sql | Creates the DB index for Booking.status. |
| apps/webapp/server/timing.server.ts | Adds dev-only Hono middleware emitting Server-Timing header. |
| apps/webapp/server/index.ts | Registers the new Server-Timing middleware in the server pipeline. |
| apps/webapp/app/utils/one-week-from-now.ts | Adds threeDaysFromNow() helper for longer signed URL expirations. |
| apps/webapp/app/routes/_layout+/settings.team.users.tsx | Reuses currentOrganization from requirePermission() to avoid a duplicate org query. |
| apps/webapp/app/routes/_layout+/admin-dashboard+/org.$organizationId.assets.tsx | Updates call site for now-synchronous booking custodian reshaping. |
| apps/webapp/app/modules/team-member/service.server.ts | Collapses a 2-step “admins then teamMembers” lookup into a single Prisma query with nested relation filtering. |
| apps/webapp/app/modules/settings/service.server.ts | Limits included teamMembers to take: 1 to avoid overfetching. |
| apps/webapp/app/modules/booking/service.server.ts | Removes nested asset.bookings selection from getBookings() results. |
| apps/webapp/app/modules/asset/service.server.ts | Parallelizes independent loader queries; refactors booking custodian enrichment to be synchronous; increases signed URL expiry to 72h. |
| apps/webapp/app/modules/asset/fields.ts | Inlines booking custodian data in assetIndexFields() booking include. |
| apps/webapp/app/modules/asset/data.server.ts | Updates call sites/comments for synchronous custodian reshaping; clarifies signed URL refresh behavior. |
Comments suppressed due to low confidence (1)
apps/webapp/app/modules/asset/fields.ts:242
- When
bookingFrom/bookingToare provided,assetIndexFields()overwritesbookingsto select only{from,to,status,id,name}. That drops thecustodianUser/custodianTeamMemberfields thatupdateAssetsWithBookingCustodians()relies on, so checked-out assets can lose custodian display data under date-range/hideUnavailable flows. Consider extending this conditionalbookings.selectto also include custodian fields (and, if multiple statuses are included, ensure the checked-out custodian comes from an ONGOING/OVERDUE booking rather than assumingbookings[0]).
// Conditionally add bookings if date range is provided
if (bookingTo && bookingFrom && unavailableBookingStatuses) {
return {
...fields,
bookings: {
where: {
status: { in: unavailableBookingStatuses },
OR: [
{
from: { lte: bookingTo },
to: { gte: bookingFrom },
},
{
from: { gte: bookingFrom },
to: { lte: bookingTo },
},
],
},
select: {
from: true,
to: true,
status: true,
id: true,
name: true,
},
},
- Align signed URL TTL with 72h expiration tracking (createSignedUrl was 24h, mainImageExpiration was 72h) - Add custodian fields to date-range bookings override in assetIndexFields so custodian display works with date filters active - Use resolveUserDisplayName for custodian name to prioritize displayName over firstName/lastName - Move Server-Timing middleware to first position to capture full request lifecycle - Use Set for checkedOutAssetIds lookup (O(1) vs O(n)) - Add JSDoc to threeDaysFromNow helper
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/webapp/app/utils/one-week-from-now.ts (1)
11-15: Move the 72h signed-URL TTL into shared app config.
threeDaysFromNow()hardcodes a policy value (3 * 24 * 60 * 60 * 1000). Please source this fromapps/webapp/app/config/shelf.config.tsso expiration behavior is centralized and easier to tune consistently across routes.As per coding guidelines, "
apps/webapp/app/**/*.{ts,tsx}: Referenceapps/webapp/app/config/shelf.config.tsfor application configuration and constants. Do not hardcode configuration values".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/utils/one-week-from-now.ts` around lines 11 - 15, threeDaysFromNow() currently hardcodes the 72h TTL; change it to read the TTL from the shared shelf config instead: import the exported TTL constant (e.g. SIGNED_URL_TTL_MS or similarly named constant) from apps/webapp/app/config/shelf.config.ts and replace the literal expression (3 * 24 * 60 * 60 * 1000) with that constant in the threeDaysFromNow function; if the TTL constant doesn't yet exist in shelf.config.ts, add a clearly named export (TTL in milliseconds) there so all code uses the centralized value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/webapp/app/utils/one-week-from-now.ts`:
- Around line 11-15: threeDaysFromNow() currently hardcodes the 72h TTL; change
it to read the TTL from the shared shelf config instead: import the exported TTL
constant (e.g. SIGNED_URL_TTL_MS or similarly named constant) from
apps/webapp/app/config/shelf.config.ts and replace the literal expression (3 *
24 * 60 * 60 * 1000) with that constant in the threeDaysFromNow function; if the
TTL constant doesn't yet exist in shelf.config.ts, add a clearly named export
(TTL in milliseconds) there so all code uses the centralized value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d1705849-9418-4d86-8a55-75195ec410da
📒 Files selected for processing (6)
apps/webapp/app/modules/asset/data.server.tsapps/webapp/app/modules/asset/fields.tsapps/webapp/app/modules/asset/service.server.tsapps/webapp/app/utils/one-week-from-now.tsapps/webapp/app/utils/storage.server.tsapps/webapp/server/index.ts
✅ Files skipped from review due to trivial changes (2)
- apps/webapp/app/utils/storage.server.ts
- apps/webapp/app/modules/asset/data.server.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/webapp/server/index.ts
- apps/webapp/app/modules/asset/fields.ts
- apps/webapp/app/modules/asset/service.server.ts
Contributor
Author
|
@CodeRabbit comments have been addressed and resolved in commit d2cc574. Do a new review |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/webapp/app/modules/asset/service.server.ts`:
- Around line 3174-3176: Multiple places set mainImageExpiration with different
hardcoded helpers (oneDayFromNow/threeDaysFromNow); unify to a single config
constant. Add a signed URL TTL constant (e.g., SIGNED_URL_EXPIRATION) into
apps/webapp/app/config/shelf.config.ts, make it equal to the 72h value, then
replace all uses of oneDayFromNow() and threeDaysFromNow() in this module
(references around newExpiration, mainImageExpiration assignments) with that
config constant by importing it; remove hardcoded helpers from writers (fresh
uploads, duplicates, imports, and refresh path) so every writer uses
config.SIGNED_URL_EXPIRATION.
- Around line 3018-3020: The code wrongly assumes bookings[0] is the active
checkout; instead, update the helper to pick the current booking explicitly (do
not use bookings[0]) by selecting the booking with an active status (include
status in the booking select via assetIndexFields()) and then choose
deterministically — e.g., prefer status 'ONGOING' or 'OVERDUE', otherwise the
earliest upcoming 'RESERVED' — and use that booking to derive
custodianUser/custodianTeamMember (replace references to bookings[0] with this
selectedBooking). Also modify assetIndexFields() to include booking.status so
this logic can run deterministically.
🪄 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
Run ID: a431d663-c930-4371-a1a3-2ebe03b880ac
📒 Files selected for processing (14)
apps/webapp/app/modules/asset/data.server.tsapps/webapp/app/modules/asset/fields.tsapps/webapp/app/modules/asset/service.server.tsapps/webapp/app/modules/booking/service.server.tsapps/webapp/app/modules/settings/service.server.tsapps/webapp/app/modules/team-member/service.server.tsapps/webapp/app/routes/_layout+/admin-dashboard+/org.$organizationId.assets.tsxapps/webapp/app/routes/_layout+/settings.team.users.tsxapps/webapp/app/utils/one-week-from-now.tsapps/webapp/app/utils/storage.server.tsapps/webapp/server/index.tsapps/webapp/server/timing.server.tspackages/database/prisma/migrations/20260407140322_add_status_index_to_booking_table/migration.sqlpackages/database/prisma/schema.prisma
💤 Files with no reviewable changes (1)
- apps/webapp/app/modules/booking/service.server.ts
- Pick active ONGOING/OVERDUE booking explicitly instead of assuming bookings[0] is the checkout (availability view can include RESERVED bookings) - Unify all mainImageExpiration writers to threeDaysFromNow (fresh uploads, duplicates, imports were still using 24h) - Add status to default booking select in assetIndexFields so the active booking can be identified deterministically
Contributor
Author
|
@CodeRabbit latest comments have been resolved in 2b9e5f5. Please make another review |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/webapp/app/modules/asset/service.server.ts (1)
2986-2999: Consider explicitly includingstatusin the booking type.The type relies on
[key: string]: unknownindex signature forstatus, and the code defensively checks"status" in bat line 3023. For better type safety and IDE support, consider addingstatusto the explicit type definition:bookings?: Array<{ id: string; + status?: string; custodianTeamMember?: { name: string } | null; custodianUser?: {This makes the contract clearer and enables better type checking on the status comparison.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/modules/asset/service.server.ts` around lines 2986 - 2999, The booking type used in updateAssetsWithBookingCustodians should explicitly include a status field instead of relying on the index signature; update the generic constraint for T so the bookings array items declare status?: string (or a tighter union of possible statuses) alongside id, custodianTeamMember and custodianUser, which will let you remove the weak `"status" in b` defensive check and allow direct type-safe comparisons of b.status within the function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/webapp/app/modules/asset/service.server.ts`:
- Around line 2986-2999: The booking type used in
updateAssetsWithBookingCustodians should explicitly include a status field
instead of relying on the index signature; update the generic constraint for T
so the bookings array items declare status?: string (or a tighter union of
possible statuses) alongside id, custodianTeamMember and custodianUser, which
will let you remove the weak `"status" in b` defensive check and allow direct
type-safe comparisons of b.status within the function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4418034e-4b40-405f-8f81-41997f67796b
📒 Files selected for processing (2)
apps/webapp/app/modules/asset/fields.tsapps/webapp/app/modules/asset/service.server.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/webapp/server/timing.server.ts (1)
40-47: Capture timing on error paths too (usetry/finally).If downstream middleware throws at Line 40, the header append at Line 44 is skipped. Using
try/finallykeeps timing visible for failed requests as well.Suggested refactor
export function serverTiming(): MiddlewareHandler { return async (c, next) => { // Skip in production to avoid leaking timing information if (process.env.NODE_ENV === "production") { return next(); } const start = performance.now(); - - await next(); - - const duration = performance.now() - start; - - c.res.headers.append( - "Server-Timing", - `total;dur=${duration.toFixed(1)};desc="Total Request"` - ); + try { + await next(); + } finally { + const duration = performance.now() - start; + c.res.headers.append( + "Server-Timing", + `total;dur=${duration.toFixed(1)};desc="Total Request"` + ); + } }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/server/timing.server.ts` around lines 40 - 47, The Server-Timing header append is skipped if downstream middleware throws; wrap the await next() call in a try/finally so timing is always recorded. In the middleware function that uses start and calls await next(), move the duration calculation and c.res.headers.append(...) into a finally block (keeping the same formatting call to c.res.headers.append with `total;dur=${duration.toFixed(1)};desc="Total Request"`), so the header is appended on both success and error paths.apps/webapp/app/modules/settings/service.server.ts (1)
77-77: Excellent optimization – limits the query to match actual usage.This change improves performance by fetching only the first
teamMemberrecord instead of loading all team members for each user, which aligns perfectly with line 115 where onlyteamMembers?.[0]is accessed.Optional: Consider adding orderBy for deterministic results
If multiple
teamMemberrecords per user/organization are possible (though unlikely based on the domain model), adding anorderByclause would ensure consistent results:teamMembers: { where: { organizationId }, take: 1, + orderBy: { createdAt: 'asc' }, include: { _count: { select: { custodies: true },This is likely unnecessary if the database schema enforces uniqueness, but it would make the query behavior explicit and deterministic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/modules/settings/service.server.ts` at line 77, The query was limited with take: 1 which is good; to make the returned teamMember deterministic when multiple records could exist, add an orderBy clause to the same query (the one that selects teamMembers with take: 1) so the DB always returns a consistent record; locate the query where teamMembers are fetched in service.server.ts (the query using take: 1 and later accessed with teamMembers?.[0]) and add a suitable orderBy (e.g., createdAt or id) to guarantee deterministic results.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/webapp/app/modules/asset/service.server.ts`:
- Around line 3058-3064: The warning currently logs the full asset object via
additionalData: { asset: a } (in the Logger.warn call that constructs a new
ShelfError), which exposes signed URLs and nested data; change the
additionalData to include only stable identifiers by replacing the asset payload
with a minimal object like { assetId: a.id, status: a.status, bookingIds:
(a.bookings || []).map(b => b.id) } (keep label and cause as-is) so the
ShelfError contains only non-sensitive identifiers.
---
Nitpick comments:
In `@apps/webapp/app/modules/settings/service.server.ts`:
- Line 77: The query was limited with take: 1 which is good; to make the
returned teamMember deterministic when multiple records could exist, add an
orderBy clause to the same query (the one that selects teamMembers with take: 1)
so the DB always returns a consistent record; locate the query where teamMembers
are fetched in service.server.ts (the query using take: 1 and later accessed
with teamMembers?.[0]) and add a suitable orderBy (e.g., createdAt or id) to
guarantee deterministic results.
In `@apps/webapp/server/timing.server.ts`:
- Around line 40-47: The Server-Timing header append is skipped if downstream
middleware throws; wrap the await next() call in a try/finally so timing is
always recorded. In the middleware function that uses start and calls await
next(), move the duration calculation and c.res.headers.append(...) into a
finally block (keeping the same formatting call to c.res.headers.append with
`total;dur=${duration.toFixed(1)};desc="Total Request"`), so the header is
appended on both success and error paths.
🪄 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
Run ID: 3a8f82d4-73ec-458e-baa0-50e1f66da351
📒 Files selected for processing (14)
apps/webapp/app/modules/asset/data.server.tsapps/webapp/app/modules/asset/fields.tsapps/webapp/app/modules/asset/service.server.tsapps/webapp/app/modules/booking/service.server.tsapps/webapp/app/modules/settings/service.server.tsapps/webapp/app/modules/team-member/service.server.tsapps/webapp/app/routes/_layout+/admin-dashboard+/org.$organizationId.assets.tsxapps/webapp/app/routes/_layout+/settings.team.users.tsxapps/webapp/app/utils/one-week-from-now.tsapps/webapp/app/utils/storage.server.tsapps/webapp/server/index.tsapps/webapp/server/timing.server.tspackages/database/prisma/migrations/20260407140322_add_status_index_to_booking_table/migration.sqlpackages/database/prisma/schema.prisma
💤 Files with no reviewable changes (1)
- apps/webapp/app/modules/booking/service.server.ts
Only log assetId and status instead of the entire asset object which includes signed URLs and nested user data.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/webapp/app/modules/asset/service.server.ts (1)
1716-1716: TTL consistency achieved across all signed URL writers.All
mainImageExpirationassignments now usethreeDaysFromNow():
- Line 1716:
updateAssetMainImage- Line 1985:
duplicateAsset- Line 2554:
createAssetsFromContentImport- Line 2723:
createAssetsFromBackupImport- Line 3182:
refreshExpiredAssetImagesThis aligns with the 72h window in
storage.server.ts(context snippet 4) and eliminates the previous inconsistency where fresh uploads used 24h while refresh used 72h.As per coding guidelines, consider extracting the signed URL TTL into
apps/webapp/app/config/shelf.config.tsas a constant (e.g.,SIGNED_URL_TTL_HOURS = 72). This would centralize the configuration and make future TTL adjustments easier. The current approach usingthreeDaysFromNow()is consistent and functional, so this is a nice-to-have improvement.Also applies to: 1985-1985, 2554-2554, 2723-2723, 3180-3182
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/modules/asset/service.server.ts` at line 1716, Extract the 72-hour signed URL TTL into a central constant and replace direct calls to threeDaysFromNow() across the asset service; create SIGNED_URL_TTL_HOURS (or SIGNED_URL_TTL) in apps/webapp/app/config/shelf.config.ts and export a helper (e.g., signedUrlExpirationFromHours) or a precomputed expiration function, then update usages in updateAssetMainImage, duplicateAsset, createAssetsFromContentImport, createAssetsFromBackupImport, and refreshExpiredAssetImages to use the new config constant/helper instead of calling threeDaysFromNow() directly so TTL is centralized and configurable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/webapp/app/modules/asset/service.server.ts`:
- Line 1716: Extract the 72-hour signed URL TTL into a central constant and
replace direct calls to threeDaysFromNow() across the asset service; create
SIGNED_URL_TTL_HOURS (or SIGNED_URL_TTL) in
apps/webapp/app/config/shelf.config.ts and export a helper (e.g.,
signedUrlExpirationFromHours) or a precomputed expiration function, then update
usages in updateAssetMainImage, duplicateAsset, createAssetsFromContentImport,
createAssetsFromBackupImport, and refreshExpiredAssetImages to use the new
config constant/helper instead of calling threeDaysFromNow() directly so TTL is
centralized and configurable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 66e55499-f59b-406b-b6d7-2e478bcc4f6f
📒 Files selected for processing (1)
apps/webapp/app/modules/asset/service.server.ts
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
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.
Summary
Server-side performance optimizations targeting the three worst routes identified via Cloudflare RUM analytics (app.shelf.nu, last 7 days):
/assets/settings/team/users/bookingsChanges
/assetslist (700 views/week — highest traffic)Promise.all(were sequential)/bookingslistasset.bookingsselect fromgetBookings()— was loading ALL bookings for every asset in every booking result (N+1), likely causing the 32s timeout. No caller uses this data./settings/team/userscurrentOrganizationfromrequirePermission()instead of making a duplicatedb.organization.findFirst()calltake: 1to teamMembers include — onlyteamMembers[0]._count.custodiesis used downstreamShared
getTeamMembersForNotify()from two sequential DB queries into a single Prisma query with nested relation filter@@index([status])on Booking table for faster status filtering in asset→booking joinsPerformance baseline (Cloudflare RUM)
Captured before changes for post-deploy comparison:
Target: LCP P75 < 2,500ms, fix /bookings timeout, reduce /settings/team/users by 50%+
Test plan
pnpm webapp:validatepasses (lint, typecheck, all tests)/assets— table loads correctly, custodian column shows for checked-out assets/bookings— page loads without timeout/settings/team/users— users list loads correctlySummary by CodeRabbit
Bug Fixes
Performance
UI Changes
Infrastructure