Fix empty breadcrumb on /[tenantId]/profile page and replace prop-drilled permission flags (readOnly, canEdit, canUse) with direct hook call useProjectPermissionsQuery()#2792
Conversation
🦋 Changeset detectedLatest commit: 14bc1ac The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
TL;DR — Replaces prop-drilled permission flags ( Key changes
Summary | 26 files | 18 commits | base:
|
| Component | Removed prop |
|---|---|
AgentItem |
canEdit |
ApiKeysTable |
canUse |
AppsTable |
canUse |
ArtifactComponentForm |
readOnly |
DataComponentForm |
readOnly |
SkillForm |
readOnly |
SkillEditModal |
readOnly |
ResourceMembersPage |
canManage |
ProjectMembersWrapper |
canManage, projectId, tenantId |
api-keys-table.tsx · apps-table.tsx · skill-form.tsx · resource-members-page.tsx
Server pages simplified to re-exports
Before: Several page files (members, skills edit, modal skills edit) were async server components that existed solely to fetch permissions and pass them as props to a single child component.
After: These pages are replaced with one-line re-exports likeexport { Component as default }, andProjectMembersWrappernow acceptsPagePropsdirectly (using React'suse()to unwrap params).
members/page.tsx · skills/edit/page.tsx · project-members-wrapper.tsx
Profile page header extracted into layout
Before: The profile page was a
'use client'component that rendered its ownPageHeaderinline, duplicating it in both the loading skeleton and the loaded state — and producing an empty breadcrumb.
After: A newprofile/layout.tsxserver component renders thePageHeaderonce, and the client page component only renders the form content beneath it.
This fixes breadcrumb rendering on the profile page by placing the header in the layout — where Next.js can render it as a server component — and adds 'profile' to STATIC_LABELS for consistent label reuse.
profile/layout.tsx · profile/page.tsx · theme.ts
There was a problem hiding this comment.
Clean refactor that centralizes permissions into the project layout + React Query hydration boundary so child pages/components consume useProjectPermissionsQuery() instead of threading props. One real issue with the double cache() wrapping in projects.ts.
| @@ -118,3 +118,5 @@ export const fetchProjectPermissions = cache( | |||
| return response.data; | |||
| } | |||
| ); | |||
|
|
|||
| export const fetchProjectPermissions = cache($fetchProjectPermissions); | |||
There was a problem hiding this comment.
fetchProjectPermissions is now double-wrapped in cache(). The inner $fetchProjectPermissions is already cache(async (...) => { ... }), and then the outer export wraps it again: cache($fetchProjectPermissions). This is a no-op at best (React dedupes on the same function reference) but misleading — every other function in this file uses the $fn / cache($fn) pattern with a plain async inner function. Should be:
const $fetchProjectPermissions = async (tenantId: string, projectId: string): Promise<ProjectPermissions> => {
// ...
};
export const fetchProjectPermissions = cache($fetchProjectPermissions);This matches the $fetchProjects / $fetchProject pattern above.
| const { user } = useAuthSession(); | ||
| const { | ||
| data: { canEdit }, | ||
| } = useProjectPermissionsQuery(); |
There was a problem hiding this comment.
Nit: ResourceMembersPage is documented as a generic component reusable for agents, MCP servers, etc. Hardcoding useProjectPermissionsQuery() here couples it to project-level permissions. If this is intentional (i.e., no other resource types are planned), the JSDoc on lines 67-69 should be updated. Otherwise, canManage/canEdit should stay as a prop for reusability.
|
@claude --review and rewrite my changeset based on changes in this PR |
Co-authored-by: Dimitri POSTOLOV <dmytropostolov@gmail.com>
There was a problem hiding this comment.
PR Review Summary
(2) Total Issues | Risk: Low
🟡 Minor (2) 🟡
Inline Comments:
- 🟡 Minor:
layout.tsx:10Layout function namedPageinstead ofLayout - 🟡 Minor:
projects.ts:110-113Arrow function style inconsistent with codebase pattern
💭 Consider (2) 💭
💭 1) project-members-wrapper.tsx:40-43 Component accepts PageProps instead of explicit props
Issue: ProjectMembersWrapper now receives PageProps<...> and uses use(params) to unwrap route params. Components in /components/ typically receive explicit props, while only page files in /app/ receive PageProps.
Why: This couples the component to a specific route structure and Next.js page patterns. The component's JSDoc mentions future reuse for AgentMembersWrapper and McpServerMembersWrapper — with PageProps typing, reuse requires matching route structures. However, this is a valid architectural choice that enables the clean single-line re-export pattern in page.tsx.
Fix: If maintaining strict separation of concerns matters for future reuse, keep explicit tenantId: string and projectId: string props. If the single-line re-export pattern is preferred, document this as the new standard for similar wrapper components.
Refs:
💭 2) resource-members-page.tsx:85-87 Generic component hardcodes project permissions
Issue: ResourceMembersPage is documented as generic ("can be used for any resource type") but calls useProjectPermissionsQuery() directly. This couples a supposedly generic component to project-specific logic.
Why: This may limit future reusability when implementing AgentMembersWrapper or McpServerMembersWrapper (as noted in JSDoc comments), which might need different permission models. However, the current approach simplifies the immediate refactoring and may be fine if all future member pages share project-level permissions.
Fix: Either rename to ProjectResourceMembersPage to reflect actual scope, or restore the canManage prop if true genericness is desired.
Refs:
✅ APPROVE
Summary: Clean refactoring that centralizes permission consumption via useProjectPermissionsQuery() with proper server-side hydration in the project layout. The cache() wrapping and React Query deduplication work correctly together. Two minor naming/style inconsistencies are easily fixable. The architectural "Consider" items are valid observations but represent reasonable tradeoffs — the pattern simplifies pages at the cost of some component-level abstraction purity. The prior feedback about double cache() wrapping has been addressed in commit 9517ddcb3. 🎉
| export const ProjectMembersWrapper: FC<PageProps<'/[tenantId]/projects/[projectId]/members'>> = ({ | ||
| params, | ||
| }) => { | ||
| const { tenantId, projectId } = use(params); |
There was a problem hiding this comment.
💭 Consider: Component receiving PageProps directly blurs page/component boundary
Issue: ProjectMembersWrapper now receives PageProps<'...'> and uses use(params) to extract route params. Components in /components/ typically receive explicit props, while only page files in /app/ receive PageProps.
Why: This couples the component to a specific route structure. The component's own comments mention future reuse for "AgentMembersWrapper" and "McpServerMembersWrapper" — with PageProps typing, reuse would require matching route structures.
Fix: Consider keeping explicit props for reusability:
interface ProjectMembersWrapperProps {
tenantId: string;
projectId: string;
}
export const ProjectMembersWrapper: FC<ProjectMembersWrapperProps> = ({
tenantId,
projectId,
}) => {
// ...
}Then the page.tsx can:
export default async function Page({ params }: PageProps<...>) {
const { tenantId, projectId } = await params;
return <ProjectMembersWrapper tenantId={tenantId} projectId={projectId} />;
}fetchProjectPermissions result/[tenantId]/profile page and replace prop-drilled permission flags (readOnly, canEdit, canUse) with direct hook call useProjectPermissionsQuery()
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This is a re-review scoped to the 7 commits since the prior review (commit 01e2abfae49c).
Prior Feedback Resolution ✅
All issues from the previous review have been addressed:
| Prior Issue | Status |
|---|---|
🟠 Double cache() wrapping in projects.ts |
✅ Fixed — now uses correct $fn/cache($fn) pattern |
🟡 Layout function named Page instead of Layout |
✅ Fixed — renamed to Layout |
| 🟡 Arrow function style inconsistent | ✅ Fixed — uses async function $fetchProjectPermissions |
💭 AgentItem N hook subscriptions concern |
✅ Addressed — reverted to prop-based pattern |
Delta Changes Summary
The 7 commits since last review made targeted fixes:
-
projects.ts— RefactoredfetchProjectPermissionsto use the standard$fn/cache($fn)pattern consistent with$fetchProjects,$fetchProject, and all other cached functions in the API layer. -
layout.tsx— Renamed the default export fromPagetoLayoutfor consistency with other layout files (login/layout.tsx,no-organization/layout.tsx). -
agent-item.tsx+agents/page.tsx— Reverted to prop-based permission passing for list items. The parent page now fetchescanEditonce and passes it to eachAgentItem, avoiding N hook subscriptions in list rendering. -
Changeset — Updated with accurate description of the PR's changes.
Code Quality
The refactoring is clean and consistent:
- Server-side
cache()wrapping deduplicates permission fetches within a single request - Client components consume
useProjectPermissionsQuery()for permission-aware UI - List components (
AgentItem) receive permissions via props to avoid subscription overhead - Profile page breadcrumb fix properly extracts
PageHeaderto layout
✅ APPROVE
Summary: All prior review feedback has been addressed. The permission centralization pattern is now consistent across the codebase — server pages fetch once via fetchProjectPermissions (with React cache() deduplication), and client components either receive permissions via props (for list items) or consume useProjectPermissionsQuery() (for standalone forms). Clean refactor, ready to ship! 🚀
…rilled permission flags (`readOnly`, `canEdit`, `canUse`) with direct hook call `useProjectPermissionsQuery()` (#2792) * upd * upd * format * format * format * format * format * format * format * format * format * fix review * fix breadcrumb on profile page * Apply suggestions from code review Co-authored-by: Dimitri POSTOLOV <dmytropostolov@gmail.com> * Update agents-manage-ui/src/lib/api/projects.ts Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> * Update agents-manage-ui/src/app/[tenantId]/profile/layout.tsx Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> * style: auto-format with biome * fix review --------- Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Ito Test Report ❌14 test cases ran. 1 failed, 13 passed. Across nine report shards, 14 includable test cases were executed with 13 passing and 1 failing, indicating the profile, project/apps/skills/components routes, permission transitions, deep-linking, modal/back-forward flows, listing pages, and mobile members/profile responsiveness were all stable and behaved as expected without crashes or 404s. The single critical finding was a high-severity, pre-existing bug in the skill creation flow at /default/projects/activities-planner/skills/new where rapid repeated Save clicks can fire duplicate create mutations before submit locking engages, while the security check confirmed injection payloads remained inert with no script execution. ❌ Failed (1)
|















No description provided.