Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…tion, UserCellRenderer primitives, registry correctness - LookupCellRenderer: accept field prop, resolve primitive IDs via field.options (fixes customer/account showing as raw IDs e.g. 2, 3 in grid views) - UserCellRenderer: handle primitive user values gracefully - getCellRenderer standardMap: fix lookup/master_detail → LookupCellRenderer - fieldRegistry: register status, user, owner explicitly - 30 new unit tests covering all fixed renderer scenarios - ROADMAP.md updated with bug fix entry Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
|
@copilot 评估是否已经完成了任务中要求的所有开发内容仔细检查 |
…xtCellRenderer tests Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Here is the complete assessment of what has been done and what remains. ✅ All renderer-level bugs fixed (commits
36 new unit tests added across all fixed paths. All 313 The screenshot shows The renderer-side fix is the correct foundation. Fully resolving the screenshot requires either:
This is a data-layer concern logged in the ROADMAP and is the correct next step after this PR. |
There was a problem hiding this comment.
Pull request overview
This PR fixes multiple field renderer display bugs that caused lookup, user, status, and select fields to show raw IDs instead of human-readable labels across grid and detail views. The root cause was a combination of missing primitive ID resolution logic, incorrect null checks, wrong renderer mappings, and missing field type registrations.
Changes:
- Fixed
LookupCellRendererto resolve primitive IDs viafield.optionsand handle null/empty values correctly - Fixed
UserCellRendererto handle primitive user IDs gracefully instead of attempting to access object properties - Fixed
TextCellRendererto correctly display numeric zero instead of treating it as a falsy empty value - Corrected
getCellRendererstandardMap to useLookupCellRendererfor lookup/master_detail types - Added explicit field registry entries for status, user, and owner types
- Added comprehensive test coverage with 36 new unit tests
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/fields/src/index.tsx | Core renderer fixes: LookupCellRenderer now resolves primitive IDs via options; UserCellRenderer handles primitives; TextCellRenderer null check fixed; standardMap corrected; explicit registrations added for status/user/owner |
| packages/fields/src/tests/cell-renderers.test.tsx | Added 36 tests covering registry assertions, TextCellRenderer edge cases (null/zero/false), LookupCellRenderer ID resolution, and UserCellRenderer primitive handling |
| ROADMAP.md | Documented the 5 defects, fixes, and test additions with technical details |
| expect(renderer).toBe(LookupCellRenderer); | ||
| }); | ||
|
|
||
| it('should return SelectCellRenderer for status type', () => { |
There was a problem hiding this comment.
Duplicate test case detected. There's already a test case with the same name "should return SelectCellRenderer for status type" at lines 40-51 (outside this diff but in the same file). The earlier test performs a more comprehensive check by rendering the component and verifying the output, while this one only checks the renderer identity. Consider either removing this duplicate or renaming it to clarify the specific aspect being tested (e.g., "should return correct renderer identity for status type").
| it('should return SelectCellRenderer for status type', () => { | |
| it('should return correct renderer identity for status type', () => { |
| if (typeof value === 'object' && value !== null) { | ||
| const label = value.name || value.label || value._id || '[Object]'; | ||
| return <span className="truncate">{label}</span>; |
There was a problem hiding this comment.
Inconsistent fallback for objects without name/label/_id properties. In the array case (line 557), the code uses String(item) as the final fallback, but in the single object case (line 575), it uses '[Object]' as the literal string. This inconsistency could lead to different display behaviors for the same type of data. Consider using String(value) instead of '[Object]' at line 575 to maintain consistency with the array handling logic, or use '[Object]' in both places if that's the preferred behavior.
| @@ -578,11 +596,24 @@ | |||
| */ | |||
| export function UserCellRenderer({ value }: CellRendererProps): React.ReactElement { | |||
| if (!value) return <span>-</span>; | |||
There was a problem hiding this comment.
Potential issue with falsy value check. The condition if (!value) at line 598 will return "-" for a user ID of 0, which could be a valid user ID in some systems. This is inconsistent with the LookupCellRenderer (line 537) which was updated to use if (value == null || value === '') to handle numeric zero correctly. Consider updating this to if (value == null || value === '') for consistency, or at minimum if (value == null) to only catch null/undefined values.
| if (!value) return <span>-</span>; | |
| if (value == null || value === '') return <span>-</span>; |
Lookup (
customer,account) and user fields were rendering raw integer IDs instead of resolved labels across all grid views. Five distinct defects inpackages/fields/src/index.tsx:Defects fixed
LookupCellRenderer— primitive ID not resolved (root cause of screenshot)Only destructured
value, never touchedfield. When the API returnscustomer: 2(unexpanded FK), fell through toString(value). Now readsfield.optionsand resolves viaString(opt.value) === String(val), covering both single primitives and arrays of IDs.UserCellRenderer— silent fallback for primitive IDsAccessing
.nameon a number isundefined; fell through to generic"User"avatar. Now returns a plain<span>for any non-object value. Array items that are primitives are also guarded.getCellRendererstandardMap — wrong fallback forlookup/master_detailBoth were mapped to
SelectCellRendererinstead ofLookupCellRenderer. Dead code (shadowed byfieldRegistry), but semantically wrong.fieldRegistry— missing explicit registrationsstatus,user, andownerwere never pre-registered; only reachable via thestandardMapfallback. Now explicitly registered alongsidelookup/master_detail/select.TextCellRenderer— falsy null guard swallowed numeric zeroUsed
value || '-'which incorrectly rendered'-'for a value of0. Updated to(value != null && value !== '') ? String(value) : '-'for consistent null-only suppression, matching the pattern used by all other renderers.Tests
36 new unit tests in
cell-renderers.test.tsxcovering: registry identity assertions for all 5 types;TextCellRendererwith null, undefined, empty string, numeric zero (0renders"0"not"-"), and boolean false;LookupCellRendererwith null, empty string, primitive number/string ID, unresolved primitive, object (name/label/_idfallback chain), array of objects, array of primitive IDs resolved via options;UserCellRendererwith null, primitive number, primitive string, object with name/username, array of user objects.Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.