fix(orders): remove broken fields param + show contact_name + REP label
# Regression: orders/quotes list lost customer/company/rep data
Last commit added a `fields` parameter to the orders and quotes
list queries to trim the wire payload. Measured on a stale test
result, looked like a 16x win. Actual: PB 0.26's `fields`
parameter is a HARD ALLOWLIST of MAIN record columns only \u2014
specifying it makes PB drop the entire `expand` payload from
the response. Result: customer, company, rep, quote names
disappeared from every order/quote card.
Verified against the live PB:
- With `fields`: expand is dropped, just relation IDs returned
- Without `fields`: expand works, full records (55-57 KB for
20 rows with deep expand)
Fix: remove the `fields` parameter from both list queries. The
slim-payload intent has to be paid at the schema level (cap the
unbounded text columns) rather than via PB query params.
The earlier '12x smaller' / '16x smaller' benchmarks were
measurements of the BROKEN response, not the working one. Real
wire sizes are unchanged from before the perf work \u2014 but the
other wins (server-side tab filter, combined status counts,
targeted users fetch, 60s counts cache) all still apply.
# Bonus: show contact_name on the order card
~76 of ~4.5k orders have `contact_name` set (the free-text
person the order is for, captured during email-intake when the
customer record is just the company). Added to the OrderCard
header between customer and rep, with a docstring explaining
the distinction from the customer relation and email_sender.
Added contact_name to the Order type and pinned the regression
with a test that asserts the orders list query does NOT pass
`fields` (so a future 'let's slim the payload' attempt
doesn't reintroduce the bug).
# REP label
The rep column on the order card now reads 'REP' (uppercase)
instead of 'rep' (lowercase). All 1312 tests pass.
Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
SJCoetz
parent
db1e7d6f76
commit
292cf2468e
5 changed files with 89 additions and 123 deletions
|
|
@ -787,11 +787,28 @@ function OrderCard({
|
|||
</span>
|
||||
</>
|
||||
)}
|
||||
{/*
|
||||
* `contact_name` is the free-text person the order is
|
||||
* for — sometimes the customer record is just the
|
||||
* company, and the actual contact is captured here
|
||||
* during the email-intake. ~76 of ~4.5k orders have
|
||||
* it set; render when present so the operator can
|
||||
* see "this goes to Dreyer, Mr. Petrus" without
|
||||
* opening the detail page.
|
||||
*/}
|
||||
{order.contact_name && (
|
||||
<>
|
||||
<span className="text-muted-foreground">·</span>
|
||||
<span className="text-muted-foreground truncate">
|
||||
{order.contact_name}
|
||||
</span>
|
||||
</>
|
||||
)}
|
||||
{repName && (
|
||||
<>
|
||||
<span className="text-muted-foreground">·</span>
|
||||
<span className="text-muted-foreground text-sm">
|
||||
rep: {repName}
|
||||
REP: {repName}
|
||||
</span>
|
||||
</>
|
||||
)}
|
||||
|
|
|
|||
|
|
@ -776,13 +776,15 @@ describe("listOrdersWithItems — tab filter", () => {
|
|||
expect(result.items.length).toBe(2);
|
||||
});
|
||||
|
||||
it("passes a `fields` parameter to the orders list (limits wire payload on the deep expand)", async () => {
|
||||
// The orders list query asks PB to limit the main record AND each
|
||||
// expanded record to just the fields the list view reads. Without
|
||||
// this, every order card drags the full `quote` record along
|
||||
// (quotes has unbounded `email_body` / `email_message` columns),
|
||||
// so 20 orders with deep expand = multi-MB JSON payload. PB
|
||||
// honours `expand.field` syntax in the `fields` query param.
|
||||
it("does not pass a `fields` parameter on the orders list (PB 0.26 breaks expand when fields is set)", async () => {
|
||||
// Regression guard: earlier I tried to limit the wire payload
|
||||
// by passing a `fields` list with per-expand syntax. PB 0.26
|
||||
// treats `fields` as a hard allowlist of MAIN record columns,
|
||||
// and the moment you set it the `expand` payload is dropped
|
||||
// from the response — so customer/company/rep/quote names
|
||||
// vanish from every order card. The orders list deliberately
|
||||
// omits `fields`; this test pins that behaviour so a future
|
||||
// "let's slim the payload" change doesn't reintroduce the bug.
|
||||
let capturedFields: string | undefined;
|
||||
let capturedExpand: string | undefined;
|
||||
const originalCollection = pbMock.collection;
|
||||
|
|
@ -807,15 +809,17 @@ describe("listOrdersWithItems — tab filter", () => {
|
|||
try {
|
||||
await listOrdersWithItems({ page: 1, perPage: 20 });
|
||||
|
||||
// Main record fields
|
||||
expect(capturedFields).toContain("order_number");
|
||||
expect(capturedFields).toContain("created");
|
||||
// Per-expand field limits — the actual perf win
|
||||
expect(capturedFields).toContain("quote.quote_number");
|
||||
expect(capturedFields).toContain("company.company");
|
||||
expect(capturedFields).toContain("customer.customer");
|
||||
expect(capturedFields).toContain("customer.rep.name");
|
||||
// Confirm we dropped potential_quote from the expand
|
||||
// `fields` must NOT be sent — PB 0.26 drops the expand payload
|
||||
// when it sees it.
|
||||
expect(capturedFields).toBeUndefined();
|
||||
// Expand IS sent, and includes the four relations the list view
|
||||
// reads (company, customer, customer.rep, quote). Importantly
|
||||
// it does NOT include `potential_quote` — the list view only
|
||||
// checks the field's existence, never the expanded record.
|
||||
expect(capturedExpand).toContain("company");
|
||||
expect(capturedExpand).toContain("customer");
|
||||
expect(capturedExpand).toContain("customer.rep");
|
||||
expect(capturedExpand).toContain("quote");
|
||||
expect(capturedExpand).not.toContain("potential_quote");
|
||||
} finally {
|
||||
pbMock.collection = originalCollection;
|
||||
|
|
|
|||
|
|
@ -144,63 +144,24 @@ export async function listOrdersWithItems(
|
|||
// the field's existence (the "Suggested" pill in the header), and
|
||||
// PB issues a separate IN query per expanded relation. Without it, we
|
||||
// save one round-trip per page load and per realtime refetch.
|
||||
//
|
||||
// NOTE on the `fields` parameter (and why it isn't here): earlier
|
||||
// I tried to limit wire payload by passing a `fields` list with
|
||||
// per-expand syntax like `company.company,customer.customer,...`.
|
||||
// PB 0.26 doesn't support that — the `fields` parameter is a hard
|
||||
// allowlist of MAIN record columns, and specifying it makes PB
|
||||
// drop the `expand` payload entirely. Result: customer/company/
|
||||
// rep/quote names disappeared from every order card. Restored by
|
||||
// removing the parameter; the page now works and the wire payload
|
||||
// is bigger than I'd like, but the deep expand works.
|
||||
//
|
||||
// Future: either upgrade PB (0.22+ supports per-expand field
|
||||
// limits properly) or move the heavy fields off the orders/quotes
|
||||
// collections (PB has a free `notes` text field with no max, and
|
||||
// unbounded `email_body` / `email_message` on quotes — these are
|
||||
// the dominant wire-payload cost). The schema-level fix would
|
||||
// permanently trim the payload.
|
||||
const expand = "company,customer,customer.rep,quote";
|
||||
// `fields` has two purposes here:
|
||||
//
|
||||
// 1. Limit the MAIN record's wire payload to only the columns the
|
||||
// list view actually reads. The orders schema is wide (notes,
|
||||
// email_body, ai_insights, migration_meta, etc.) and most rows
|
||||
// carry KBs of content we never render.
|
||||
//
|
||||
// 2. Limit each EXPANDED record's fields via the `expand.field`
|
||||
// syntax PB supports in the `fields` query param. This is the
|
||||
// big win: `quotes` has unbounded text columns (email_body,
|
||||
// email_message) that, without limiting, would drag the full
|
||||
// email body of every linked quote into the list response —
|
||||
// 20 orders with deep expand can easily become a 1-5 MB
|
||||
// JSON payload. We only need `quote.quote_number` for the
|
||||
// pill, so we ask for that and nothing else.
|
||||
//
|
||||
// - `company.company` — the company name on the header row
|
||||
// - `customer.customer` — the customer name on the header row
|
||||
// - `customer.rep.id/name/email` — rep label, via
|
||||
// `resolveRepDisplayName`
|
||||
// - `quote.quote_number` — the linked-quote pill
|
||||
// Top-level fields = main record (orders) columns. Dotted fields =
|
||||
// per-expand field limits using PB's `expandField.subfield` syntax.
|
||||
// The relation field ids (`company`, `customer`, `customer.rep`,
|
||||
// `quote`) must be present in the main list so PB knows which
|
||||
// relations to expand; once listed, the dotted forms below limit
|
||||
// what comes back from each expanded record.
|
||||
const fields =
|
||||
[
|
||||
// main record
|
||||
"id",
|
||||
"order_number",
|
||||
"company",
|
||||
"customer",
|
||||
"quote",
|
||||
"potential_quote",
|
||||
"potential_quote_confidence",
|
||||
"quote_match_confidence",
|
||||
"rep",
|
||||
"created",
|
||||
"updated",
|
||||
// expanded `company` — just the name
|
||||
"company.id",
|
||||
"company.company",
|
||||
// expanded `customer` — just the name + the rep relation id
|
||||
"customer.id",
|
||||
"customer.customer",
|
||||
"customer.rep",
|
||||
// expanded `customer.rep` (users) — just the label fields
|
||||
"customer.rep.id",
|
||||
"customer.rep.name",
|
||||
"customer.rep.email",
|
||||
// expanded `quote` — just the number for the pill
|
||||
"quote.id",
|
||||
"quote.quote_number",
|
||||
].join(",");
|
||||
|
||||
// Tab filter path — three sub-cases:
|
||||
// (a) no tab, no search → original PB-paginated path
|
||||
|
|
@ -225,14 +186,13 @@ export async function listOrdersWithItems(
|
|||
perPage,
|
||||
sort,
|
||||
expand,
|
||||
fields,
|
||||
});
|
||||
}
|
||||
} else if (search && !tabGroup) {
|
||||
ordersPage = await pb.collection(ORDER_COLLECTION).getList<Order>(
|
||||
1,
|
||||
ORDER_SEARCH_HARD_CAP,
|
||||
{ sort, expand, fields },
|
||||
{ sort, expand },
|
||||
);
|
||||
// search returns a single page; totalPages/totalItems are derived
|
||||
// from the filtered list size once we know how many survived the
|
||||
|
|
@ -243,7 +203,7 @@ export async function listOrdersWithItems(
|
|||
const rawPage = await pb.collection(ORDER_COLLECTION).getList<Order>(
|
||||
1,
|
||||
ORDER_SEARCH_HARD_CAP,
|
||||
{ sort, expand, fields },
|
||||
{ sort, expand },
|
||||
);
|
||||
ordersPage = {
|
||||
items: rawPage.items,
|
||||
|
|
@ -257,7 +217,7 @@ export async function listOrdersWithItems(
|
|||
ordersPage = await pb.collection(ORDER_COLLECTION).getList<Order>(
|
||||
page,
|
||||
perPage,
|
||||
{ sort, expand, fields },
|
||||
{ sort, expand },
|
||||
);
|
||||
}
|
||||
|
||||
|
|
@ -379,15 +339,15 @@ export async function getOrderIdsForTabGroup(
|
|||
async function fetchOrdersFilteredByIds(
|
||||
pb: AdminPocketBase,
|
||||
ids: string[],
|
||||
opts: { page: number; perPage: number; sort: string; expand: string; fields: string },
|
||||
opts: { page: number; perPage: number; sort: string; expand: string },
|
||||
): Promise<{ items: Order[]; totalItems: number; totalPages: number; page: number; perPage: number }> {
|
||||
const { page, perPage, sort, expand, fields } = opts;
|
||||
const { page, perPage, sort, expand } = opts;
|
||||
const filter = ids.map((id) => `id = "${id.replace(/"/g, '\\"')}"`).join(" || ");
|
||||
|
||||
if (ids.length <= ID_LIST_INLINE_LIMIT) {
|
||||
const result = await pb
|
||||
.collection(ORDER_COLLECTION)
|
||||
.getList<Order>(page, perPage, { filter, sort, expand, fields });
|
||||
.getList<Order>(page, perPage, { filter, sort, expand });
|
||||
return {
|
||||
items: result.items,
|
||||
totalItems: result.totalItems,
|
||||
|
|
@ -399,7 +359,7 @@ async function fetchOrdersFilteredByIds(
|
|||
|
||||
const all = await pb
|
||||
.collection(ORDER_COLLECTION)
|
||||
.getFullList<Order>({ filter, sort, expand, fields });
|
||||
.getFullList<Order>({ filter, sort, expand });
|
||||
const totalItems = all.length;
|
||||
const totalPages = Math.max(1, Math.ceil(totalItems / perPage));
|
||||
const start = (page - 1) * perPage;
|
||||
|
|
|
|||
|
|
@ -286,49 +286,18 @@ export async function getQuotesDashboardData(
|
|||
}
|
||||
|
||||
/**
|
||||
* Slim field list for the list-page response. The quotes collection
|
||||
* has 45+ fields including unbounded `email_body` and `email_message`
|
||||
* — without limiting, 20 quotes × full records = multi-MB JSON even
|
||||
* when most emails are empty (the wasted bandwidth and parse cost
|
||||
* shows up in page-load time). The list view reads:
|
||||
* quote.* : id, quote_number, status, claimed_by, is_template,
|
||||
* follow_up_due, last_reply_at, customer (rel id),
|
||||
* company (rel id), created, updated
|
||||
* expand.customer : customer (name), rep (rel id)
|
||||
* expand.customer.rep : id, name, email
|
||||
* expand.company : company (name)
|
||||
*
|
||||
* `customer.company` is intentionally NOT expanded — the page only
|
||||
* reads `expand.company.company` (the direct relation from quote),
|
||||
* not `expand.customer.expand.company`. Dropping the 3-level expand
|
||||
* Expand relation for the list-page response. `customer.company` is
|
||||
* intentionally NOT expanded — the page only reads
|
||||
* `expand.company.company` (the direct relation from quote), not
|
||||
* `expand.customer.expand.company`. Dropping the 3-level expand
|
||||
* saves one IN query per page load.
|
||||
*
|
||||
* (No `fields` projection here — see the comment at the call site.
|
||||
* PB 0.26 doesn't support per-expand field limiting without
|
||||
* dropping the entire `expand` payload, so we accept the larger
|
||||
* per-record wire size and rely on schema-level fixes to trim
|
||||
* the heavy text columns.)
|
||||
*/
|
||||
const QUOTE_LIST_FIELDS = [
|
||||
// main record
|
||||
"id",
|
||||
"quote_number",
|
||||
"status",
|
||||
"claimed_by",
|
||||
"is_template",
|
||||
"follow_up_due",
|
||||
"last_reply_at",
|
||||
"customer",
|
||||
"company",
|
||||
"created",
|
||||
"updated",
|
||||
// expanded company — just the name
|
||||
"company.id",
|
||||
"company.company",
|
||||
// expanded customer — name + rep relation
|
||||
"customer.id",
|
||||
"customer.customer",
|
||||
"customer.rep",
|
||||
// expanded customer.rep (users) — id, name, email
|
||||
"customer.rep.id",
|
||||
"customer.rep.name",
|
||||
"customer.rep.email",
|
||||
].join(",");
|
||||
|
||||
const QUOTE_LIST_EXPAND = "customer,customer.rep,company";
|
||||
|
||||
async function listQuotesForDashboard(
|
||||
|
|
@ -393,7 +362,13 @@ async function listQuotesForDashboard(
|
|||
filter: clauses,
|
||||
sort,
|
||||
expand: QUOTE_LIST_EXPAND,
|
||||
fields: QUOTE_LIST_FIELDS,
|
||||
// No `fields` here: PB 0.26 treats `fields` as a hard allowlist
|
||||
// of MAIN record columns, and specifying it makes PB drop the
|
||||
// `expand` payload entirely — so the customer/company/rep names
|
||||
// disappear from the list. The expand chain is what we actually
|
||||
// need; the per-record wire cost has to be paid until the
|
||||
// schema's unbounded text columns (notes, email_body, etc.)
|
||||
// are trimmed at the field level.
|
||||
});
|
||||
|
||||
if (search) {
|
||||
|
|
|
|||
|
|
@ -704,6 +704,16 @@ export interface Order extends BaseRecord {
|
|||
* monitoring pipeline; null for orders created without one.
|
||||
*/
|
||||
email_message_id?: string;
|
||||
/**
|
||||
* Free-text person the order is for. Sometimes the customer
|
||||
* record is just the company (legal entity), and the actual
|
||||
* contact captured during the email-intake lives here. Distinct
|
||||
* from `customer` (the relation) and `email_sender` (the email
|
||||
* From: address) — this is the operator-readable contact name
|
||||
* shown on the order card so the office can see "this goes to
|
||||
* Dreyer, Mr. Petrus" without opening the detail page.
|
||||
*/
|
||||
contact_name?: string;
|
||||
expand?: {
|
||||
company?: Company;
|
||||
customer?: Customer;
|
||||
|
|
|
|||
Loading…
Reference in a new issue