feat: add cart read queries #93

davide · 2026-07-15T11:33:04Z

davide commented

2026-07-15 11:33:04 +00:00

No description provided.

davide added 1 commit

2026-07-15 11:33:04 +00:00

feat: add cart read queries

forgejo-agent / LLM Bot response generation (pull_request) Successful in 2s

Details

683f9465fb

davide requested review from llm-bot

2026-07-15 11:33:25 +00:00

llm-bot reviewed

2026-07-15 12:04:55 +00:00

llm-bot left a comment

PR Review: feat: add cart read queries

Good work on this PR! The implementation follows the existing patterns in the codebase well. Here are my findings:

✅ Strengths

Consistent architecture — The new CartBatch, CartItemBatch, CartClient, and CartItemClient classes follow the established Batch/BatchRows/Serializable patterns used for products.
Proper security checks — Both CartBatch.forUser and CartItemBatch.forUser correctly validate that carts/items belong to the requesting user. The exists subquery in CartItemBatch.forUser is the right approach for cross-table ownership verification.
Bug fix — The cartItemValue table foreign key was corrected from product to productOption (cartItemValue.ts). This was a real bug — cart item values should reference product options, not products.
Good use of query.batch — All remote queries use query.batch for efficient batched server-side resolution.
CartBatch.getClients() is sync — This is a nice optimization since it only needs the ID already present in the batch, unlike other getClients() methods that require DB queries.

🔍 Observations & Suggestions

1. CartItemValue type placement (src/lib/entities/cart/cart-item/index.ts)
The CartItemValue type is defined here, but it's also used in the Cart.addItem method in src/lib/entities/cart/index.ts. Consider whether this should live in a shared location or if the current placement is fine. Currently it's a circular dependency concern — cart/index.ts imports from cart-item/index.ts which defines CartItemValue, and cart/index.ts is imported by cart-item/index.ts for the Product import. This works but feels slightly tangled.

2. ProductOptionClient.getSchema() return type (src/lib/entities/products/option/client.ts)

getSchema(): v.GenericSchema<ProductOptionValue | undefined, ProductOptionValue> {

The v.GenericSchema type is quite broad. Consider if a more specific type would be better, or if this is intentionally loose to accommodate the different schema types (boolean, text, number).

3. CartItemBatch.forUser subquery (src/lib/entities/cart/cart-item/batch.ts)
The exists subquery is correct but could be slightly simplified. Consider whether a JOIN with inArray on cart.userId would be more performant for large datasets:

// Alternative approach:
.where(
  and(
    inArray(s.cartItem.id, ids),
    inArray(
      s.cartItem.cartId,
      db.select({ id: s.cart.id }).from(s.cart).where(eq(s.cart.userId, user.id))
    )
  )
)

Both approaches are valid — the exists is more explicit about intent.

4. ProductOptionBatch import style (src/lib/entities/products/batch.ts)

import { ProductOptionBatch } from "./option/batch.ts";

This is a new import that wasn't needed before. The change from returning raw ProductOptionData[] to returning a ProductOptionBatch is a good architectural improvement for consistency, but it does add a new dependency.

5. Minor: order.ts cartId change
Changing cartId from .notNull() to .primaryKey() is correct and makes semantic sense — an order IS a cart. Just want to confirm this doesn't affect any existing migration or data.

✅ No blocking issues found

The code is well-structured, follows existing patterns, and the security model is sound. The bug fix in cartItemValue.ts is particularly valuable.

Recommendation: Approve 🚀

## PR Review: feat: add cart read queries Good work on this PR! The implementation follows the existing patterns in the codebase well. Here are my findings: ### ✅ Strengths 1. **Consistent architecture** — The new `CartBatch`, `CartItemBatch`, `CartClient`, and `CartItemClient` classes follow the established `Batch`/`BatchRows`/`Serializable` patterns used for products. 2. **Proper security checks** — Both `CartBatch.forUser` and `CartItemBatch.forUser` correctly validate that carts/items belong to the requesting user. The `exists` subquery in `CartItemBatch.forUser` is the right approach for cross-table ownership verification. 3. **Bug fix** — The `cartItemValue` table foreign key was corrected from `product` to `productOption` (`cartItemValue.ts`). This was a real bug — cart item values should reference product options, not products. 4. **Good use of `query.batch`** — All remote queries use `query.batch` for efficient batched server-side resolution. 5. **`CartBatch.getClients()` is sync** — This is a nice optimization since it only needs the ID already present in the batch, unlike other `getClients()` methods that require DB queries. ### 🔍 Observations & Suggestions **1. `CartItemValue` type placement** (`src/lib/entities/cart/cart-item/index.ts`) The `CartItemValue` type is defined here, but it's also used in the `Cart.addItem` method in `src/lib/entities/cart/index.ts`. Consider whether this should live in a shared location or if the current placement is fine. Currently it's a circular dependency concern — `cart/index.ts` imports from `cart-item/index.ts` which defines `CartItemValue`, and `cart/index.ts` is imported by `cart-item/index.ts` for the `Product` import. This works but feels slightly tangled. **2. `ProductOptionClient.getSchema()` return type** (`src/lib/entities/products/option/client.ts`) ```typescript getSchema(): v.GenericSchema<ProductOptionValue | undefined, ProductOptionValue> { ``` The `v.GenericSchema` type is quite broad. Consider if a more specific type would be better, or if this is intentionally loose to accommodate the different schema types (boolean, text, number). **3. `CartItemBatch.forUser` subquery** (`src/lib/entities/cart/cart-item/batch.ts`) The `exists` subquery is correct but could be slightly simplified. Consider whether a `JOIN` with `inArray` on `cart.userId` would be more performant for large datasets: ```typescript // Alternative approach: .where( and( inArray(s.cartItem.id, ids), inArray( s.cartItem.cartId, db.select({ id: s.cart.id }).from(s.cart).where(eq(s.cart.userId, user.id)) ) ) ) ``` Both approaches are valid — the `exists` is more explicit about intent. **4. `ProductOptionBatch` import style** (`src/lib/entities/products/batch.ts`) ```typescript import { ProductOptionBatch } from "./option/batch.ts"; ``` This is a new import that wasn't needed before. The change from returning raw `ProductOptionData[]` to returning a `ProductOptionBatch` is a good architectural improvement for consistency, but it does add a new dependency. **5. Minor: `order.ts` cartId change** Changing `cartId` from `.notNull()` to `.primaryKey()` is correct and makes semantic sense — an order IS a cart. Just want to confirm this doesn't affect any existing migration or data. ### ✅ No blocking issues found The code is well-structured, follows existing patterns, and the security model is sound. The bug fix in `cartItemValue.ts` is particularly valuable. **Recommendation: Approve** 🚀

src/lib/entities/cart/cart-item/batch.ts

					
				@ -0,0 +12,4 @@

				    const rows = await db

				      .select({ id: s.cartItem.id })

				      .from(s.cartItem)

				      .where(

The exists subquery here is correct but consider whether a JOIN with inArray on cart.userId might be more performant for large datasets.

src/lib/entities/cart/cart-item/index.ts

					
				@ -0,0 +2,4 @@

				import type { ProductOptionValue } from "#lib/entities/products/option/index.ts";

				import type { CartItemId } from "./id.ts";

				export type CartItemValue = { productOptionId: ProductOptionId; value: ProductOptionValue };

The CartItemValue type is defined in cart-item/index.ts but also used in Cart.addItem in cart/index.ts. This creates a slight circular dependency feel. Consider if this type belongs in a shared location.

src/lib/server/database/tables/cartItemValue.ts

					
				@ -9,4 +9,4 @@

				const columns = {

				  productOptionId: text()

				    .$type<ProductOptionId>()

				    .notNull()

Good catch fixing this foreign key — cart item values should reference product options, not products.

davide merged commit f67eafb56f into feat/order-management

2026-07-15 12:13:48 +00:00

davide referenced this pull request from a commit

2026-07-15 12:13:48 +00:00

feat: add cart read queries (#93)

davide deleted branch feat/cart-queries

2026-07-15 12:13:48 +00:00

Sign in to join this conversation.

No reviewers

No labels

No milestone

No assignees

2 participants

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

projects/events-cash-register!93

Rows
Columns