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() { />