fix(assets): restore custom fields on the asset overview for BASE and SELF_SERVICE

.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 <number> --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.

.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).

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() {
               />
               <Card className="my-3 px-[-4] py-[-5] md:border">
                 <ul className="item-information">
-                  {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,16 +1260,16 @@ 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 (
                       <InlineEditableField
                         key={def.id}
                         fieldName={`customField-${def.id}`}
                         formFieldName="customField"
                         label={def.name}
-                        canEdit={canEditAsset}
+                        canEdit={canEditField}
                         extraHiddenInputs={{
                           customFieldId: def.id,
                         }}

apps/webapp/app/utils/custom-fields.test.ts

@@ -2,6 +2,7 @@ import type { CustomField } from "@prisma/client";
 import { describe, expect, it } from "vitest";
 import type { ResolvedFormatPrefs } from "~/utils/date-format";
 import {
+  buildAssetOverviewCustomFields,
   buildCustomFieldValue,
   getCustomFieldDisplayValue,
 } from "./custom-fields";
@@ -91,3 +92,116 @@ describe("getCustomFieldDisplayValue — DATE with prefs", () => {
     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",
+    ]);
+  });
+});

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<TStored>[] {
+  const editableIds = new Set(editableDefinitions.map((def) => def.id));
+  const rows = new Map<string, AssetOverviewCustomField<TStored>>();
+
+  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