Merged
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @Connorbelez, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 34 minutes and 58 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (30)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Owner
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
This was referenced Apr 13, 2026
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/components/admin/shell/FieldRenderer.tsx (2)
113-136: Consider defensive type checking before casting date values.The
"date"and"datetime"cases castargs.valuetostring | numberwithout runtime validation. If an unexpected value type reaches this code path, it could cause runtime issues inDateCell.🛡️ Suggested defensive check
case "date": - return ( - <DateCell format="absolute" value={args.value as string | number} /> - ); + return typeof args.value === "string" || typeof args.value === "number" ? ( + <DateCell format="absolute" value={args.value} /> + ) : ( + <TextCell value={String(args.value)} /> + ); case "datetime": - return <DateCell format="both" value={args.value as string | number} />; + return typeof args.value === "string" || typeof args.value === "number" ? ( + <DateCell format="both" value={args.value} /> + ) : ( + <TextCell value={String(args.value)} /> + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/admin/shell/FieldRenderer.tsx` around lines 113 - 136, The date/datetime branches in FieldRenderer.tsx cast args.value to string|number without runtime validation, which can send invalid types into DateCell; update the "date" and "datetime" cases to first check typeof args.value === "string" || typeof args.value === "number" (and optionally validate with Date.parse or new Date(...) to ensure a valid date) and only then render DateCell, otherwise fallback to rendering TextCell (e.g., String(args.value)) or a safe placeholder to avoid runtime errors.
114-124: Hardcoded currency/locale may limit internationalization.The
CurrencyCelluses hardcodedcurrency="CAD"andlocale="en-CA". If the application needs to support multiple currencies or locales, consider deriving these from the field metadata or a context provider.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/admin/shell/FieldRenderer.tsx` around lines 114 - 124, FieldRenderer currently renders CurrencyCell with hardcoded currency="CAD" and locale="en-CA"; change it to derive currency and locale from field metadata or a locale context: read currency and locale from args.field (e.g. args.field?.meta?.currency or args.field?.currency) or from a React context/provider (e.g. useLocale()/useSettings()) and pass those values to CurrencyCell, falling back to sensible defaults (e.g. Intl.DateTimeFormat/navigator language or app-wide default) while preserving the existing isCents and value behavior.convex/crm/entityViewFields.ts (1)
45-47: Synthetic ID pattern note.The
computed:${fieldName}pattern for synthetic field IDs is unconventional but acceptable here since computed fields don't have persistedfieldDefdocuments. Consider adding a brief comment explaining this convention for future maintainers.📝 Suggested documentation
+/** + * Generates a synthetic fieldDefId for adapter-computed fields. + * These IDs are prefixed with "computed:" to distinguish them from + * real document IDs and should never be used for database lookups. + */ function toSyntheticFieldDefId(fieldName: string): Id<"fieldDefs"> { return `computed:${fieldName}` as Id<"fieldDefs">; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/crm/entityViewFields.ts` around lines 45 - 47, Add a brief comment above the toSyntheticFieldDefId function explaining that computed fields are not persisted as fieldDef documents and therefore use a synthetic ID pattern prefixed with "computed:" (returned as Id<"fieldDefs">) so future maintainers understand the convention and its rationale.src/components/admin/shell/entity-view-adapters.tsx (1)
103-112: Field duplication in borrowers layout configuration.
verificationSummaryappears in bothhighlightFieldNamesand the "Verification" section'sfieldNames. WhileSectionedRecordDetailshandles this via deduplication (consumed names), the field will only render in highlights, making the section potentially empty if that's the only field.Was this intentional? If "Verification" section should always show, consider different fields or removing the duplicate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/admin/shell/entity-view-adapters.tsx` around lines 103 - 112, The borrowers layout currently lists "verificationSummary" in both the borrowers.highlightFieldNames array and the "Verification" section's fieldNames, which causes SectionedRecordDetails to dedupe and render the field only in highlights leaving the section empty; update the borrowers configuration by removing "verificationSummary" from either highlightFieldNames or the "Verification" section (or replace it with another field) so the section always renders as intended and references remain consistent with SectionedRecordDetails behavior (look for borrowers, highlightFieldNames, verificationSummary, and the "Verification" section in the entity-view-adapters).src/components/admin/shell/RecordSidebar.tsx (1)
621-623: Consider extracting shared utility to avoid duplication.
hasRenderableFieldValueis duplicated here and indetail-sections.tsx(lines 25-27). Consider extracting this to a shared utility module.♻️ Suggested extraction
Create a shared utility, e.g.,
src/components/admin/shell/field-utils.ts:export function hasRenderableFieldValue(value: unknown): boolean { return value !== undefined && value !== null && value !== ""; }Then import it in both files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/admin/shell/RecordSidebar.tsx` around lines 621 - 623, Extract the duplicated function hasRenderableFieldValue into a shared utility module and import it where needed: create a new file exporting function hasRenderableFieldValue(value: unknown): boolean { return value !== undefined && value !== null && value !== ""; } then replace the local implementations in RecordSidebar (the function in this diff) and in detail-sections.tsx to import and use the shared helper to eliminate duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/admin/shell/detail-sections.tsx`:
- Around line 135-152: The current mapping over renderedSections uses
section.title as the React key which can collide if titles repeat; update the
key to a stable unique identifier (preferably an explicit id on the section
object) or fall back to a combination like `${section.title}-${index}` or the
array index when no id exists; locate the map over renderedSections in the
component (where section.title is used as key and DetailFieldGrid is rendered)
and change the key prop to use section.id if present, otherwise use a safe
fallback such as title+index to ensure unique keys.
In `@src/hooks/useAdminDetailSheet.tsx`:
- Around line 18-19: The function resolveAdminDetailSheetState uses the
SidebarRecordRef type but that type is not imported; update the import from
RecordSidebarProvider to include SidebarRecordRef (i.e., add SidebarRecordRef to
the existing import from "#/components/admin/shell/RecordSidebarProvider") so
the resolveAdminDetailSheetState signature compiles and type-checks.
---
Nitpick comments:
In `@convex/crm/entityViewFields.ts`:
- Around line 45-47: Add a brief comment above the toSyntheticFieldDefId
function explaining that computed fields are not persisted as fieldDef documents
and therefore use a synthetic ID pattern prefixed with "computed:" (returned as
Id<"fieldDefs">) so future maintainers understand the convention and its
rationale.
In `@src/components/admin/shell/entity-view-adapters.tsx`:
- Around line 103-112: The borrowers layout currently lists
"verificationSummary" in both the borrowers.highlightFieldNames array and the
"Verification" section's fieldNames, which causes SectionedRecordDetails to
dedupe and render the field only in highlights leaving the section empty; update
the borrowers configuration by removing "verificationSummary" from either
highlightFieldNames or the "Verification" section (or replace it with another
field) so the section always renders as intended and references remain
consistent with SectionedRecordDetails behavior (look for borrowers,
highlightFieldNames, verificationSummary, and the "Verification" section in the
entity-view-adapters).
In `@src/components/admin/shell/FieldRenderer.tsx`:
- Around line 113-136: The date/datetime branches in FieldRenderer.tsx cast
args.value to string|number without runtime validation, which can send invalid
types into DateCell; update the "date" and "datetime" cases to first check
typeof args.value === "string" || typeof args.value === "number" (and optionally
validate with Date.parse or new Date(...) to ensure a valid date) and only then
render DateCell, otherwise fallback to rendering TextCell (e.g.,
String(args.value)) or a safe placeholder to avoid runtime errors.
- Around line 114-124: FieldRenderer currently renders CurrencyCell with
hardcoded currency="CAD" and locale="en-CA"; change it to derive currency and
locale from field metadata or a locale context: read currency and locale from
args.field (e.g. args.field?.meta?.currency or args.field?.currency) or from a
React context/provider (e.g. useLocale()/useSettings()) and pass those values to
CurrencyCell, falling back to sensible defaults (e.g.
Intl.DateTimeFormat/navigator language or app-wide default) while preserving the
existing isCents and value behavior.
In `@src/components/admin/shell/RecordSidebar.tsx`:
- Around line 621-623: Extract the duplicated function hasRenderableFieldValue
into a shared utility module and import it where needed: create a new file
exporting function hasRenderableFieldValue(value: unknown): boolean { return
value !== undefined && value !== null && value !== ""; } then replace the local
implementations in RecordSidebar (the function in this diff) and in
detail-sections.tsx to import and use the shared helper to eliminate
duplication.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72abaa7a-202e-4e23-8678-8ec51f6396e8
⛔ Files ignored due to path filters (1)
convex/_generated/api.d.tsis excluded by!**/_generated/**
📒 Files selected for processing (29)
convex/crm/__tests__/records.test.tsconvex/crm/entityViewFields.tsconvex/crm/recordQueries.tsconvex/crm/types.tsconvex/crm/viewState.tsspecs/ENG-279/chunks/chunk-01-backend-detail-contract/context.mdspecs/ENG-279/chunks/chunk-01-backend-detail-contract/status.mdspecs/ENG-279/chunks/chunk-01-backend-detail-contract/tasks.mdspecs/ENG-279/chunks/chunk-02-frontend-registry/context.mdspecs/ENG-279/chunks/chunk-02-frontend-registry/status.mdspecs/ENG-279/chunks/chunk-02-frontend-registry/tasks.mdspecs/ENG-279/chunks/chunk-03-dedicated-sections/context.mdspecs/ENG-279/chunks/chunk-03-dedicated-sections/status.mdspecs/ENG-279/chunks/chunk-03-dedicated-sections/tasks.mdspecs/ENG-279/chunks/chunk-04-tests-and-validation/context.mdspecs/ENG-279/chunks/chunk-04-tests-and-validation/status.mdspecs/ENG-279/chunks/chunk-04-tests-and-validation/tasks.mdspecs/ENG-279/chunks/manifest.mdspecs/ENG-279/execution-checklist.mdspecs/ENG-279/summary.mdspecs/ENG-279/tasks.mdsrc/components/admin/shell/AdminDetailSheet.tsxsrc/components/admin/shell/FieldRenderer.tsxsrc/components/admin/shell/RecordSidebar.tsxsrc/components/admin/shell/detail-sections.tsxsrc/components/admin/shell/entity-view-adapters.tsxsrc/hooks/useAdminDetailSheet.tsxsrc/test/admin/admin-shell.test.tssrc/test/admin/useAdminDetailSheet.test.ts
There was a problem hiding this comment.
Pull request overview
Implements ENG-279’s “detail surface” contract and section-based renderer registry so the admin record detail UI (sheet + page) consumes normalized fields (including computed/editability metadata) from a single backend query and renders dedicated, domain-shaped details where available.
Changes:
- Added a backend
getRecordDetailSurfacequery returning{ objectDef, record, adapterContract, normalized fields }, plus computed-field materialization. - Refactored the shared admin detail surface (
RecordSidebar+ adapters +FieldRenderer) to use normalized field definitions and sectioned layouts. - Updated admin-shell/unit tests and added ENG-279 specs/checklists documenting scope and validation.
Reviewed changes
Copilot reviewed 29 out of 30 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/admin/useAdminDetailSheet.test.ts | Adds unit tests for route-independent detail sheet state resolution. |
| src/test/admin/admin-shell.test.ts | Updates adapter tests to validate dedicated renderer output ordering. |
| src/hooks/useAdminDetailSheet.tsx | Extracts a pure resolver for detail sheet state and rewires hook logic. |
| src/components/admin/shell/entity-view-adapters.tsx | Replaces prioritized-field rendering with section-based dedicated layouts and new adapter args. |
| src/components/admin/shell/detail-sections.tsx | Introduces a reusable SectionedRecordDetails component for sectioned/highlighted rendering. |
| src/components/admin/shell/RecordSidebar.tsx | Refactors detail surface to use getRecordDetailSurface, adapter contracts, and normalized fields. |
| src/components/admin/shell/FieldRenderer.tsx | Enhances rendering using normalized metadata (badges, richer cell renderers, link-like fields). |
| src/components/admin/shell/AdminDetailSheet.tsx | Delegates the legacy sheet wrapper to the canonical shared AdminRecordDetailSurface. |
| specs/ENG-279/tasks.md | Adds ENG-279 task breakdown and validation notes. |
| specs/ENG-279/summary.md | Adds ENG-279 scope/constraints summary and links to planning docs. |
| specs/ENG-279/execution-checklist.md | Adds execution checklist capturing requirements and validation status. |
| specs/ENG-279/chunks/manifest.md | Adds chunk manifest tracking implementation phases. |
| specs/ENG-279/chunks/chunk-04-tests-and-validation/tasks.md | Adds chunk task list for tests/validation. |
| specs/ENG-279/chunks/chunk-04-tests-and-validation/status.md | Records validation commands run and blockers. |
| specs/ENG-279/chunks/chunk-04-tests-and-validation/context.md | Adds context for tests/validation chunk. |
| specs/ENG-279/chunks/chunk-03-dedicated-sections/tasks.md | Adds dedicated-sections chunk task list. |
| specs/ENG-279/chunks/chunk-03-dedicated-sections/status.md | Records completion/validation notes for dedicated sections. |
| specs/ENG-279/chunks/chunk-03-dedicated-sections/context.md | Adds context for dedicated sections chunk. |
| specs/ENG-279/chunks/chunk-02-frontend-registry/tasks.md | Adds frontend-registry chunk task list. |
| specs/ENG-279/chunks/chunk-02-frontend-registry/status.md | Records completion/validation notes for frontend registry. |
| specs/ENG-279/chunks/chunk-02-frontend-registry/context.md | Adds context for frontend registry chunk. |
| specs/ENG-279/chunks/chunk-01-backend-detail-contract/tasks.md | Adds backend-contract chunk task list. |
| specs/ENG-279/chunks/chunk-01-backend-detail-contract/status.md | Records completion/validation notes for backend contract. |
| specs/ENG-279/chunks/chunk-01-backend-detail-contract/context.md | Adds context for backend contract chunk. |
| convex/crm/viewState.ts | Rewires view state assembly to shared normalized-field/adaptor builders. |
| convex/crm/types.ts | Adds GetRecordDetailSurfaceResult interface. |
| convex/crm/recordQueries.ts | Refactors record reference loading and adds getRecordDetailSurface. |
| convex/crm/entityViewFields.ts | Adds shared adapter/field normalization + computed-field evaluation/materialization helpers. |
| convex/crm/tests/records.test.ts | Adds tests for getRecordDetailSurface (CRM + native/dedicated/computed). |
| convex/_generated/api.d.ts | Updates generated API types to include the new module import. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Merged
Connorbelez
changed the base branch from
graphite-base/395
to
rbac-permission-reconciliation
Owner
Author
Merge activity
|
Connorbelez
changed the base branch from
rbac-permission-reconciliation
to
graphite-base/395
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Tests