fix: Apply addcustomfields to profile page route loader#4444
fix: Apply addcustomfields to profile page route loader#4444Ryrahul wants to merge 3 commits intovendurehq:masterfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughReplaces the profile route loader's direct query with an extended query built via extendDetailFormQuery(addCustomFields(...), pageId) and pre-fetched using getDetailQueryOptions(extendedQuery, { id: 'undefined' }) with context.queryClient.ensureQueryData. Adds imports for extendDetailFormQuery and addCustomFields, updates the useDetailPage import to include getDetailQueryOptions, introduces a new constant Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
biggamesmallworld
left a comment
biggamesmallworld
left a comment
There was a problem hiding this comment.
Review
Thanks for the fix! The addCustomFields wrapping is the right approach — this definitely addresses the crash when custom fields are defined on Administrator. A couple of suggestions to bring this in line with the established patterns in the codebase:
Suggestions
1. Use getDetailQueryOptions() in the loader to fix query key mismatch
The loader currently uses a hardcoded queryKey: ['DetailPage', 'profile'], but useDetailPage constructs its key as ['DetailPage', 'activeAdministrator', { id: 'undefined' }]. Since these don't match, the loader prefetches into a cache bucket the component never reads — so the data gets fetched twice.
The global-settings page (global-settings.tsx) is the closest analog and handles this correctly. The profile loader should follow the same pattern:
const pageId = 'profile';
// in loader:
const { extendedQuery } = extendDetailFormQuery(
addCustomFields(activeAdministratorDocument),
pageId,
);
await context.queryClient.ensureQueryData(
getDetailQueryOptions(extendedQuery, { id: 'undefined' }),
{},
);This also picks up extendDetailFormQuery(), which is needed since the component has pageId="profile" on its <Page> — without it, any extensions registered against the profile detail query won't be included in the loader's prefetch.
2. Remove the unrelated PageBlock formatting change
The multi-line → single-line reformatting of the PageBlock props is unrelated to the bug fix. Keeping the diff focused makes it easier to review and keeps git blame clean.
Notes
- The
addCustomFieldscall is correct — it usesWeakMapmemoization so calling it in the loader has no performance concern. - A regression test (e.g., a dashboard e2e test that configures an
Administratorcustom field and navigates to/profile) would be a nice addition to prevent this from coming back, but not a blocker.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/dashboard/src/app/routes/_authenticated/_profile/profile.tsx (1)
25-25: ReusepageIdfor the<Page>prop as well.The loader now depends on this constant, but Line 79 still hardcodes
'profile'. If those ever drift,extendDetailFormQuery()silently falls back to the original query, so wiring<Page pageId={pageId}>keeps both sides locked together.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dashboard/src/app/routes/_authenticated/_profile/profile.tsx` at line 25, The file defines const pageId = 'profile' but the <Page> component still hardcodes 'profile'; change the Page usage to <Page pageId={pageId}> so the loader and UI share the same identifier (update the JSX where Page is rendered), ensuring pageId and the loader/extendDetailFormQuery() calls remain in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/dashboard/src/app/routes/_authenticated/_profile/profile.tsx`:
- Around line 30-34: The hook call to useDetailPage is missing the pageId option
so extendDetailFormQuery is not applied; update the options object passed to
useDetailPage (the call that currently passes queryDocument:
activeAdministratorDocument, entityField: 'activeAdministrator', updateDocument:
updateAdministratorDocument, ...) to include pageId: pageId so that
useExtendedDetailQuery/extendDetailFormQuery and
addCustomFields(activeAdministratorDocument) run consistently with the loader.
---
Nitpick comments:
In `@packages/dashboard/src/app/routes/_authenticated/_profile/profile.tsx`:
- Line 25: The file defines const pageId = 'profile' but the <Page> component
still hardcodes 'profile'; change the Page usage to <Page pageId={pageId}> so
the loader and UI share the same identifier (update the JSX where Page is
rendered), ensuring pageId and the loader/extendDetailFormQuery() calls remain
in sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4d923e70-5243-40f5-b6e3-ee78a58bfb64
📒 Files selected for processing (1)
packages/dashboard/src/app/routes/_authenticated/_profile/profile.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/dashboard/src/app/routes/_authenticated/_profile/profile.tsx (1)
30-34: Use a named constant instead of the string literal'undefined'for queries without entity IDs.The
id: 'undefined'string is passed togetDetailQueryOptions, which executes an actual API query since'undefined'is not theNEW_ENTITY_IDsentinel ('__NEW__'). SinceactiveAdministratorDocumenthas noidvariable in its definition, this relies on GraphQL silently ignoring the extra variable. While this works due to standard GraphQL behavior, the pattern is confusing and appears in at least three places.Consider creating a dedicated constant (e.g.,
const NO_ENTITY_ID = 'undefined') to clarify the intent and make the code more maintainable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dashboard/src/app/routes/_authenticated/_profile/profile.tsx` around lines 30 - 34, Replace the literal string 'undefined' used as an absent-ID sentinel with a named constant to clarify intent: define a constant (e.g., NO_ENTITY_ID = 'undefined') and use it in the call to getDetailQueryOptions (and other occurrences) instead of the raw 'undefined' string; update references in the extendDetailFormQuery/addCustomFields/activeAdministratorDocument initialization and any other places that pass 'undefined' so the code reads getDetailQueryOptions(extendedQuery, { id: NO_ENTITY_ID }) and any similar calls, leaving behavior unchanged but making the sentinel explicit and easier to maintain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/dashboard/src/app/routes/_authenticated/_profile/profile.tsx`:
- Around line 30-34: Replace the literal string 'undefined' used as an absent-ID
sentinel with a named constant to clarify intent: define a constant (e.g.,
NO_ENTITY_ID = 'undefined') and use it in the call to getDetailQueryOptions (and
other occurrences) instead of the raw 'undefined' string; update references in
the extendDetailFormQuery/addCustomFields/activeAdministratorDocument
initialization and any other places that pass 'undefined' so the code reads
getDetailQueryOptions(extendedQuery, { id: NO_ENTITY_ID }) and any similar
calls, leaving behavior unchanged but making the sentinel explicit and easier to
maintain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c0660328-d62c-43e4-9a4a-51223be66e67
📒 Files selected for processing (1)
packages/dashboard/src/app/routes/_authenticated/_profile/profile.tsx
Thank you for the detailed review and helpful suggestions. Regarding the The Have also added the declaration off Thanks again for the careful review and for pointing these out I really appreciate it. |
2 participants
Description
The profile page (
/dashboard/profile) crashes when custom fields are defined on theAdministratorentity. The route loader was queryingactiveAdministratorDocumentwithout passing it throughaddCustomFields(), causing a GraphQL error — the server expectscustomFieldsto have sub-field selections, but the raw query sends it as a bare scalar field.The administrator detail page (
/administrators/:id) works correctly because it usesdetailPageRouteLoader(), which already appliesaddCustomFields()before executing the query.The fix wraps the query document with
addCustomFields()in the profile page's route loader, matching the behavior of all other detail pages.Breaking changes
None.
Checklist
📌 Always:
Fixes : #4443