Skip to content

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

Merged
DonKoko merged 2 commits into
mainfrom
fix-base-and-self-service-not-seeing-custom-fields
Aug 7, 2026
Merged

fix(assets): restore custom fields on the asset overview for BASE and SELF_SERVICE#2824
DonKoko merged 2 commits into
mainfrom
fix-base-and-self-service-not-seeing-custom-fields

Conversation

@DonKoko

@DonKoko DonKoko commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

The bug

BASE and SELF_SERVICE users see no custom fields at all on an asset's overview page, even when values are set. Reported by a customer.

Root cause

The overview loader skips three "editor-only" queries for users who can't update the asset — a perf optimization from #2486, live since 2026-05-07:

const [allCustomFieldDefs, categoriesData, locationsData] = canEditAsset
  ? await Promise.all([getActiveCustomFields(...), ...])
  : [[], { categories: [] }, { locations: [] }];   // ← view-only gets []

Categories and locations genuinely are editor-only (dropdown sources). But the component built the entire custom-fields list from allCustomFieldDefs:

const allCustomFields = (allCustomFieldDefs ?? []).sort(...).map((def) => ({ def, storedValue: ... }));

BASE holds asset: [read], SELF_SERVICE holds asset: [read, custody] — neither has update, so the array is empty and the whole card disappears.

The surrounding code shows this was never intended: it already had a "hide Not set rows from view-only users" branch and a card condition keyed on some(storedValue) — logic that could never fire.

Why nobody caught it for three months: an empty array typechecks and passes every test, and ADMIN/OWNER short-circuit to allow-all in hasPermission, so no one who could have noticed ever saw it.

The fix

Stored values already carry their own definition via getAssetOverviewFields (id, name, type, options, …), so no extra query is needed and the perf optimization stays intact. New pure helper buildAssetOverviewCustomFields seeds the list from the asset's own values and only tops up with editable definitions for the "Not set" placeholder rows.

Second bug fixed along the way

Affects 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 (getActiveCustomFieldsForAssetif (!fieldDef) throw 400), so offering an editor there would dead-end.

No change for admins/owners

Both validators short-circuit those roles, so canEditAsset is true, allCustomFieldDefs is actually fetched, every definition in it gets isEditable: true, and canEditField is true. Every row that had a pencil still has one. The only non-editable rows are ones that previously didn't render at all.

Blast radius checked

  • All other canEditAsset uses on the page (category, location, description, tags, value, total value, asset model, placements) render their display path for view-only users — only pencils and editors are gated.
  • This is the only loader in _layout+ that gates data on a permission.
  • Asset index (simple + advanced) and the mobile assets.$assetId API fetch custom fields ungated — unaffected.
  • lastScan is deliberately withheld from these roles (scan: read — scanner name, email, GPS). Intentional privacy gate, left as-is.

Testing

  • 6 new cases in app/utils/custom-fields.test.ts, written failing first — all 6 failed before the fix, 15/15 pass after
  • pnpm webapp:validate: 4349 passed, lint + typecheck clean (the two activity[.csv] suites are the known 10s hookTimeout flake; both pass at --hookTimeout=30000)
  • Manually verified in the browser as BASE/SELF_SERVICE and as admin

Also included

Two .claude/rules/ entries:

  • permission-gated-loader-data-must-not-gate-display.md — the bug class: gating a loader fetch is safe only if nothing displayed derives from it
  • label-fix-prs-on-github.md — fix PRs get the fix label at creation time

Summary by CodeRabbit

  • New Features

    • View-only users can now see stored custom fields on asset overview pages.
    • Custom fields are clearly marked as editable or read-only based on permissions and category scope.
    • Custom fields are sorted consistently and displayed without duplicates.
  • Bug Fixes

    • Prevented missing stored fields when editing permissions are unavailable.
    • Empty, unrelated, and orphaned custom-field entries are handled more accurately.
  • Documentation

    • Added guidance for applying the fix label to bug-fix pull requests.
    • Documented permission-safe handling of supplementary loader data.

… SELF_SERVICE

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.
@DonKoko DonKoko added the fix label Aug 7, 2026
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown

🩺 React Doctor — webapp

Findings on the files changed by this PR:

  • 0 errors
  • 3 warnings — advisory
⚠️ 3 warnings (click to expand)
  • react-doctor/no-derived-useState (2)
    • apps/webapp/app/routes/_layout+/assets.$assetId.overview.tsx:1852
    • apps/webapp/app/routes/_layout+/assets.$assetId.overview.tsx:1853
  • react-doctor/no-giant-component (1)
    • apps/webapp/app/routes/_layout+/assets.$assetId.overview.tsx:738

Run locally with pnpm webapp:doctor for a full scan, or cd apps/webapp && pnpm exec react-doctor . --diff for the same diff-only view.

@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: e18757a7-1752-4133-af3f-cd2d355721d6

📥 Commits

Reviewing files that changed from the base of the PR and between ce6bf2d and e9210ab.

📒 Files selected for processing (5)
  • .claude/rules/label-fix-prs-on-github.md
  • .claude/rules/permission-gated-loader-data-must-not-gate-display.md
  • apps/webapp/app/routes/_layout+/assets.$assetId.overview.tsx
  • apps/webapp/app/utils/custom-fields.test.ts
  • apps/webapp/app/utils/custom-fields.ts

Walkthrough

The PR adds a reusable custom-field builder, preserves stored fields for view-only users, applies field-level editability in the asset overview, adds regression tests, and documents rules for permission-gated display data and bug-fix PR labels.

Changes

Asset custom-field display

Layer / File(s) Summary
Custom-field builder and validation
apps/webapp/app/utils/custom-fields.ts, apps/webapp/app/utils/custom-fields.test.ts
The builder merges stored values with editable definitions, removes duplicates, marks editability, sorts rows, ignores empty values, and preserves input arrays. Tests cover these behaviors.
Permission-safe overview rendering
.claude/rules/permission-gated-loader-data-must-not-gate-display.md, apps/webapp/app/routes/_layout+/assets.$assetId.overview.tsx
The overview displays stored fields without requiring edit permissions. Fields outside the permitted category scope remain visible and read-only. Unset non-editable fields remain hidden.

Bug-fix PR labeling

Layer / File(s) Summary
Bug-fix PR label guidance
.claude/rules/label-fix-prs-on-github.md
The rule documents applying and validating the GitHub fix label for bug-fix pull requests.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: restoring custom fields on the asset overview for BASE and SELF_SERVICE users.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-base-and-self-service-not-seeing-custom-fields

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@DonKoko
DonKoko merged commit bde09e3 into main Aug 7, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant