From e9210ab6f3a4eb412c2147c9a2397782b781feed Mon Sep 17 00:00:00 2001 From: Donkoko Date: Fri, 7 Aug 2026 16:52:47 +0300 Subject: [PATCH] fix(assets): restore custom fields on the asset overview for BASE and SELF_SERVICE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The overview loader skips `getActiveCustomFields` for users without `asset: update` (a perf optimization from #2486), but the page built its ENTIRE custom-fields list from those definitions. BASE holds `asset: [read]` and SELF_SERVICE holds `asset: [read, custody]`, so both received an empty array and saw no custom fields at all — even on assets with values set. Live since 2026-05-07. Invisible to CI (an empty array typechecks and passes every test) and to anyone who could have caught it, since ADMIN and OWNER short-circuit to allow-all in `hasPermission`. The stored values already carry their own definition via `getAssetOverviewFields`, so no extra query is needed and the optimization stays intact. The new `buildAssetOverviewCustomFields` helper seeds the list from the asset's values and only tops it up with editable definitions for the "Not set" placeholder rows. This also fixes a second bug of the same class affecting every role including owners: an uncategorized asset is offered only uncategorized definitions, so a value left behind by a category-scoped field rendered nowhere. Those rows are now visible but read-only — the action rejects writes for out-of-scope definitions with a 400, so offering an editor there would dead-end. Admins and owners keep inline editing on every row that had it before. Adds two rules: one for the loader-gating bug class, one requiring the GitHub `fix` label on fix PRs. --- .claude/rules/label-fix-prs-on-github.md | 35 ++++++ ...gated-loader-data-must-not-gate-display.md | 45 +++++++ .../_layout+/assets.$assetId.overview.tsx | 35 +++--- apps/webapp/app/utils/custom-fields.test.ts | 114 ++++++++++++++++++ apps/webapp/app/utils/custom-fields.ts | 95 +++++++++++++++ 5 files changed, 309 insertions(+), 15 deletions(-) create mode 100644 .claude/rules/label-fix-prs-on-github.md create mode 100644 .claude/rules/permission-gated-loader-data-must-not-gate-display.md diff --git a/.claude/rules/label-fix-prs-on-github.md b/.claude/rules/label-fix-prs-on-github.md new file mode 100644 index 000000000..5bc9d3057 --- /dev/null +++ b/.claude/rules/label-fix-prs-on-github.md @@ -0,0 +1,35 @@ +--- +description: Every PR opened for a bug fix must carry the GitHub "fix" label, applied at creation time +globs: ["**/*"] +--- + +# Label Fix PRs With `fix` + +When you open a PR whose purpose is to fix a bug — anything you'd commit as +`fix(scope): …` under Conventional Commits — apply the repo's **`fix`** label. +Releases and triage filter on labels, so an unlabelled fix PR is invisible to +whoever assembles the changelog. + +Apply it **at creation**, not as a follow-up — a PR that gets reviewed and +merged quickly may never come back around for the edit: + +```bash +# ✅ Good — label lands with the PR +gh pr create --title "fix(assets): …" --body "…" --label fix + +# ❌ Bad — unlabelled; relies on remembering a second command +gh pr create --title "fix(assets): …" --body "…" +``` + +Already opened it without the label? Fix it immediately: + +```bash +gh pr edit --add-label fix +``` + +The label must already exist on the repo — `gh pr create --label` fails the +whole command on an unknown label, which silently costs you the PR. Check with +`gh label list` before inventing a new one. + +Note you only reach this step **after the user has pushed the branch** — pushing +is theirs, per the repo's git conventions. diff --git a/.claude/rules/permission-gated-loader-data-must-not-gate-display.md b/.claude/rules/permission-gated-loader-data-must-not-gate-display.md new file mode 100644 index 000000000..cf7b19c66 --- /dev/null +++ b/.claude/rules/permission-gated-loader-data-must-not-gate-display.md @@ -0,0 +1,45 @@ +--- +description: Skipping a loader query behind an edit/manage permission is a perf win only if nothing DISPLAYED derives from it — otherwise view-only roles silently lose data +globs: ["apps/webapp/app/routes/**/*.tsx", "apps/webapp/app/routes/**/*.ts"] +--- + +# Permission-Gated Loader Data Must Not Feed Display + +Skipping a query for users who can't edit is a legitimate optimization. It +becomes a **silent data-loss bug** the moment a display path reads from the +skipped result — the page renders empty for BASE and SELF_SERVICE and looks +perfectly healthy for whoever wrote it (ADMIN/OWNER short-circuit to allow-all +in `hasPermission`). + +This shipped: the asset overview gated `getActiveCustomFields` on +`asset: update`, but the page built its ENTIRE custom-fields list from those +definitions. Every BASE and SELF_SERVICE user saw zero custom fields for three +months. Typecheck, unit tests and `validate` were green throughout — an empty +array is a valid array. + +**Before gating a loader fetch, name what the payload feeds.** Editor +affordances only (dropdown options, autocomplete sources) → safe to gate. +Anything that labels, orders, or decides the visibility of a row → not gateable. + +**Display must derive from the entity's own data, which the read gate already +authorized.** Stored values usually carry their definition already; use that as +the primary source and let the permission-gated fetch only ADD to it. + +```ts +// ❌ Bad — the whole list dies when the gated fetch is skipped +const defs = canEdit ? await getActiveCustomFields({ ... }) : []; +const rows = defs.map((def) => ({ def, value: valueMap.get(def.id) ?? null })); + +// ✅ Good — seeded from the asset's own values, topped up for editors +const rows = buildAssetOverviewCustomFields({ + storedValues: asset.customFields, // always present; read gate covered it + editableDefinitions: allCustomFieldDefs, // [] for view-only, adds "Not set" rows +}); +``` + +A row you surface but the action refuses to write must render read-only — +don't hand a user an editor that dead-ends on a 400. + +**Verify as the lowest role, not as an owner.** No automated check in this repo +catches this class; only loading the page as BASE or SELF_SERVICE does. See +[[org-scope-user-supplied-ids]] for the inverse failure (over-exposure). diff --git a/apps/webapp/app/routes/_layout+/assets.$assetId.overview.tsx b/apps/webapp/app/routes/_layout+/assets.$assetId.overview.tsx index 8c79380d9..0c308f357 100644 --- a/apps/webapp/app/routes/_layout+/assets.$assetId.overview.tsx +++ b/apps/webapp/app/routes/_layout+/assets.$assetId.overview.tsx @@ -92,6 +92,7 @@ import { getClientHint } from "~/utils/client-hints"; import { formatCurrency } from "~/utils/currency"; import { buildCustomFieldLinkHref } from "~/utils/custom-field-link"; import { + buildAssetOverviewCustomFields, buildCustomFieldValue, getCustomFieldDisplayValue, } from "~/utils/custom-fields"; @@ -762,18 +763,15 @@ export default function AssetOverview() { * Each entry pairs the field definition with its stored value (or null * if not set). This keeps fields in a stable position regardless of * whether they have values — no jumping when a user adds or clears data. + * + * The asset's own values are the primary source: `allCustomFieldDefs` is + * loaded ONLY for users who can update the asset, so building the list from + * it alone hid every custom field from BASE and SELF_SERVICE users. */ - const customFieldsValueMap = new Map( - (asset?.customFields ?? []) - .filter((f) => f.value) - .map((f) => [f.customField.id, f]) - ); - const allCustomFields = (allCustomFieldDefs ?? []) - .sort((a, b) => a.name.localeCompare(b.name)) - .map((def) => ({ - def, - storedValue: customFieldsValueMap.get(def.id) ?? null, - })); + const allCustomFields = buildAssetOverviewCustomFields({ + storedValues: asset?.customFields ?? [], + editableDefinitions: allCustomFieldDefs ?? [], + }); const location = asset ? getPrimaryLocation(asset) : null; usePosition(); @@ -1242,8 +1240,15 @@ export default function AssetOverview() { />
    - {allCustomFields.map(({ def, storedValue }) => { + {allCustomFields.map(({ def, storedValue, isEditable }) => { const hasValue = !!storedValue; + /** + * A field the action would refuse to write (its definition + * is outside the asset's category scope) stays visible but + * read-only — offering an editor there would dead-end on a + * 400. + */ + const canEditField = canEditAsset && isEditable; const fieldValue = hasValue ? (storedValue.value as unknown as ShelfAssetCustomFieldValueType["value"]) : null; @@ -1255,8 +1260,8 @@ export default function AssetOverview() { ? getCustomFieldDisplayValue(fieldValue!, prefs) : null; - /* Hide "Not set" rows from view-only users */ - if (!hasValue && !canEditAsset) return null; + /* Hide "Not set" rows from users who can't fill them in */ + if (!hasValue && !canEditField) return null; return ( { expect(getCustomFieldDisplayValue(value as never)).toBe("April 3rd, 2026"); }); }); + +/** + * Regression guard for the view-only blindness bug. + * + * The asset-overview loader only fetches the org's active custom-field + * DEFINITIONS for users who can update the asset (a perf optimization). The + * page then built its entire custom-fields list from that array, so BASE and + * SELF_SERVICE users — who hold `asset: [read]` and never `asset: update` — + * saw an empty definitions array and therefore NO custom fields at all, even + * on assets where values were set. + * + * The stored values already carry their own definition, so the list must be + * seeded from the values and only TOPPED UP with editable definitions. + */ +describe("buildAssetOverviewCustomFields", () => { + const def = (id: string, name: string) => ({ + id, + name, + type: "TEXT" as const, + options: [], + helpText: null, + required: false, + }); + + const storedValue = (id: string, name: string, raw: string) => ({ + value: { raw }, + customField: def(id, name), + }); + + it("shows fields that have values when there are no editable definitions", () => { + // why: this is exactly the BASE / SELF_SERVICE payload — the loader sends + // `allCustomFieldDefs: []` because they cannot update the asset. + const result = buildAssetOverviewCustomFields({ + storedValues: [storedValue("cf1", "Serial number", "ABC-123")], + editableDefinitions: [], + }); + + expect(result).toHaveLength(1); + expect(result[0].def.name).toBe("Serial number"); + expect(result[0].storedValue?.value).toEqual({ raw: "ABC-123" }); + // Visible, but not editable — they hold `asset: [read]`, not `update`. + expect(result[0].isEditable).toBe(false); + }); + + it("adds definitions with no stored value so editors get 'Not set' rows", () => { + const result = buildAssetOverviewCustomFields({ + storedValues: [storedValue("cf1", "Serial number", "ABC-123")], + editableDefinitions: [ + def("cf1", "Serial number"), + def("cf2", "Warranty"), + ], + }); + + expect(result.map((r) => r.def.name)).toEqual([ + "Serial number", + "Warranty", + ]); + expect(result[1].storedValue).toBeNull(); + }); + + it("does not duplicate a field present in both sources", () => { + const result = buildAssetOverviewCustomFields({ + storedValues: [storedValue("cf1", "Serial number", "ABC-123")], + editableDefinitions: [def("cf1", "Serial number")], + }); + + expect(result).toHaveLength(1); + expect(result[0].storedValue).not.toBeNull(); + }); + + it("keeps a stored value whose definition is missing from the editable set", () => { + // why: an uncategorized asset only gets UNCATEGORIZED definitions back, so + // a value left behind by a category-scoped field would otherwise vanish — + // for admins and owners too. + const result = buildAssetOverviewCustomFields({ + storedValues: [storedValue("cf-orphan", "Lens mount", "EF")], + editableDefinitions: [def("cf2", "Warranty")], + }); + + expect(result.map((r) => r.def.name)).toEqual(["Lens mount", "Warranty"]); + expect(result[0].storedValue).not.toBeNull(); + // The route's action refuses writes for out-of-scope definitions, so the + // row must render read-only rather than dead-end on a 400. + expect(result[0].isEditable).toBe(false); + expect(result[1].isEditable).toBe(true); + }); + + it("ignores stored rows with an empty value", () => { + const result = buildAssetOverviewCustomFields({ + storedValues: [{ value: null, customField: def("cf1", "Serial number") }], + editableDefinitions: [], + }); + + expect(result).toEqual([]); + }); + + it("sorts alphabetically without mutating the caller's arrays", () => { + const editableDefinitions = [def("cf-z", "Zoom"), def("cf-a", "Aperture")]; + + const result = buildAssetOverviewCustomFields({ + storedValues: [], + editableDefinitions, + }); + + expect(result.map((r) => r.def.name)).toEqual(["Aperture", "Zoom"]); + // The loader payload must stay untouched — `.sort()` in place would + // reorder data React may re-render from. + expect(editableDefinitions.map((d) => d.name)).toEqual([ + "Zoom", + "Aperture", + ]); + }); +}); diff --git a/apps/webapp/app/utils/custom-fields.ts b/apps/webapp/app/utils/custom-fields.ts index ebd6254b1..7569f90f8 100644 --- a/apps/webapp/app/utils/custom-fields.ts +++ b/apps/webapp/app/utils/custom-fields.ts @@ -491,6 +491,101 @@ export const getCustomFieldDisplayValue = ( return String(value.raw); }; +/** + * The subset of a custom-field definition the asset-overview list needs. + * + * Deliberately narrow so BOTH sources satisfy it: the full `CustomField` + * records returned by `getActiveCustomFields`, and the trimmed definition + * nested inside each stored `AssetCustomFieldValue` row (see + * `getAssetOverviewFields`). + */ +export type CustomFieldDefinitionForDisplay = { + id: string; + name: string; + type: CustomFieldType; + options: string[]; +}; + +/** A stored custom-field value paired with the definition it belongs to. */ +export type StoredCustomFieldValueForDisplay = { + value: unknown; + customField: CustomFieldDefinitionForDisplay; +}; + +/** One row of the asset-overview custom-fields list. */ +export type AssetOverviewCustomField< + TStored extends StoredCustomFieldValueForDisplay, +> = { + /** The definition used to label and render the row */ + def: CustomFieldDefinitionForDisplay; + /** The stored value, or `null` when the field has never been set */ + storedValue: TStored | null; + /** + * Whether this field appears in `editableDefinitions`. Always `false` for + * view-only users, who receive none. A row can carry a value the caller is + * not offered for editing (see the uncategorized-asset case below); the + * route's action rejects those writes with a 400, so the UI must not present + * an editor for them. + */ + isEditable: boolean; +}; + +/** + * Build the unified, alphabetically-sorted custom-fields list for the asset + * overview page. + * + * The list is seeded from the asset's STORED VALUES — each one already carries + * its own definition — and only then topped up with the org's editable + * definitions, which produce the "Not set" placeholder rows. + * + * Seeding from the values is what makes the list permission-independent. The + * loader only fetches `editableDefinitions` for users who can update the asset + * (skipping three queries for view-only users), so a list built from those + * alone renders empty for every BASE and SELF_SERVICE user. It also keeps a + * value visible when its definition falls outside the editable set — an + * uncategorized asset is offered only uncategorized definitions, yet may still + * hold a value written while it belonged to a category. + * + * @param params.storedValues - The asset's custom-field value rows; rows with + * an empty `value` are treated as unset + * @param params.editableDefinitions - Active definitions the user may fill in; + * pass an empty array for view-only users + * @returns One entry per distinct definition, sorted by name. Neither input + * array is mutated. + */ +export function buildAssetOverviewCustomFields< + TStored extends StoredCustomFieldValueForDisplay, +>({ + storedValues, + editableDefinitions, +}: { + storedValues: TStored[]; + editableDefinitions: CustomFieldDefinitionForDisplay[]; +}): AssetOverviewCustomField[] { + const editableIds = new Set(editableDefinitions.map((def) => def.id)); + const rows = new Map>(); + + for (const storedValue of storedValues) { + if (!storedValue.value) continue; + rows.set(storedValue.customField.id, { + def: storedValue.customField, + storedValue, + isEditable: editableIds.has(storedValue.customField.id), + }); + } + + for (const def of editableDefinitions) { + if (rows.has(def.id)) continue; + rows.set(def.id, { def, storedValue: null, isEditable: true }); + } + + // Spread before sorting: `.sort()` is in-place, and these entries reference + // arrays owned by the loader payload. + return [...rows.values()].sort((a, b) => + a.def.name.localeCompare(b.def.name) + ); +} + //header = "cf:name,type:text" export const getDefinitionFromCsvHeader = ( header: string