refactor: reuse RoleCard and KpiCard in member page #367
Conversation
Replace custom MemberRoleCard with existing RoleCard and KpiCard components. Each role now displays as a paired layout with role info on left and KPI on right.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR removes the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 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
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 |
Code Review - PR #367SummaryThis is an excellent refactoring that eliminates duplicate code by reusing existing ✅ Strengths
🔍 Minor Observations
🎯 Architecture & Patterns✅ Follows CLAUDE.md Guidelines:
✅ Component Design:
✅ Integration:
🔒 Security & Performance
📝 Recommendations
✅ VerdictLGTM - Approve with minor optional suggestions This is a well-executed refactoring that:
The suggestions above are optional improvements and should not block merging. Great work! 🚀 |
![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
🤖 Fix all issues with AI agents
In @src/app/member/[id]/_components/role-kpi-pair.tsx:
- Around line 61-65: The current placeholder shown when hasMetricButNoChart is
true is ambiguous; update the UI to distinguish between "no chart configured"
and "still loading" by either changing the message in the hasMetricButNoChart
branch to a clearer statement like "No KPI data available" (or "KPI chart not
configured") and/or wire in an explicit loading flag from where dashboardChart
is populated; specifically, update the JSX branch that checks
hasMetricButNoChart (and references role.metricId and the dashboardChart lookup)
to show the clearer static message or conditionalize on a new loading boolean so
genuine loading uses "Loading KPI data…" while missing chart shows "No KPI data
available."
🧹 Nitpick comments (1)
src/app/member/[id]/_components/role-kpi-pair.tsx (1)
22-36: Consider using a theme constant for the color fallback.The hardcoded color
"#3b82f6"could be extracted to a theme constant for better maintainability.♻️ Example: Using a theme constant
+import { DEFAULT_ROLE_COLOR } from "@/lib/constants/theme"; const roleCardData: RoleCardData = { id: role.id, title: role.title, purpose: role.purpose, - color: role.color ?? "#3b82f6", + color: role.color ?? DEFAULT_ROLE_COLOR, effortPoints: role.effortPoints,
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app/member/[id]/_components/member-role-card.tsxsrc/app/member/[id]/_components/role-kpi-pair.tsxsrc/app/member/[id]/_components/team-section.tsx
💤 Files with no reviewable changes (1)
- src/app/member/[id]/_components/member-role-card.tsx
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (GEMINI.md)
Use TypeScript 5.9 with strict type checking for all frontend and backend code
Files:
src/app/member/[id]/_components/team-section.tsxsrc/app/member/[id]/_components/role-kpi-pair.tsx
src/**/*.tsx
📄 CodeRabbit inference engine (GEMINI.md)
Prefer Server Components for initial data fetching; use Client Components ('use client') only for interactivity
Files:
src/app/member/[id]/_components/team-section.tsxsrc/app/member/[id]/_components/role-kpi-pair.tsx
src/**/*/*.tsx
📄 CodeRabbit inference engine (GEMINI.md)
Client Components must use
import { api } from '@/trpc/react'for standard HTTP/Hooks wrapper
Files:
src/app/member/[id]/_components/team-section.tsxsrc/app/member/[id]/_components/role-kpi-pair.tsx
**/*.tsx
📄 CodeRabbit inference engine (GEMINI.md)
Use Tailwind CSS 4 for styling with shadcn/ui and Radix UI primitive components
Files:
src/app/member/[id]/_components/team-section.tsxsrc/app/member/[id]/_components/role-kpi-pair.tsx
src/app/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
src/app/**/*.tsx: Use the dual tRPC API pattern: direct calls in Server Components (api.team.getById) for 10x faster performance, and React hooks in Client Components (api.team.getById.useQuery)
Use getUserDisplayName(userId, members) utility (client-side sync) from @/lib/helpers/get-user-name for displaying user names in components
Files:
src/app/member/[id]/_components/team-section.tsxsrc/app/member/[id]/_components/role-kpi-pair.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use @trivago/prettier-plugin-sort-imports with inline type imports for import organization
Files:
src/app/member/[id]/_components/team-section.tsxsrc/app/member/[id]/_components/role-kpi-pair.tsx
🧠 Learnings (15)
📓 Common learnings
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T12:52:42.935Z
Learning: Applies to src/app/teams/[teamId]/_components/role-node.tsx,src/app/teams/[teamId]/_components/public-role-node.tsx : These role node components are 75% identical and should be consolidated. Extract shared `RoleNodeTemplate` component with `isEditable` prop to DRY up the code.
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T21:27:04.176Z
Learning: Applies to src/app/(teams|org)/**/*.tsx : Use shared MembersList component at src/components/member/member-list.tsx for displaying members in canvas sidebar and org page; it includes getMemberDisplayInfo() utility for initials/name logic
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T12:52:42.935Z
Learning: Applies to src/components/dashboard-metric-card.tsx,src/app/dashboard/[teamId]/_components/public-dashboard-metric-card.tsx : Dashboard metric cards are duplicated with public variant. Consolidate into single component with `readOnly` mode prop instead of maintaining separate components.
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T21:27:04.176Z
Learning: Applies to src/app/teams/[teamId]/_components/**/*.tsx : Implement cache-first node pattern for role nodes: store only roleId in node data, fetch display data from TanStack Query cache using useRoleData hook
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T21:27:04.176Z
Learning: Applies to src/hooks/**/*.ts : For role-metric cache updates, optimistically update both role cache (role.getByTeamId) and dashboard cache (dashboard.getDashboardCharts) to maintain UI consistency
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-12-20T22:12:00.576Z
Learning: Applies to src/app/teams/[teamId]/**/*.tsx : React Flow nodes must store minimal data (e.g., just `roleId`); fetch full Role data from TanStack Query cache in the Node component to keep canvas and sidebars in sync
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T12:52:42.935Z
Learning: Applies to src/app/dashboard/[teamId]/**/*.{ts,tsx} : Dashboard cache updates for role-metric assignments must update both `role.getByTeamId` and `dashboard.getDashboardCharts` caches during mutations. Use onMutate for optimistic updates on both caches, then invalidate both on success.
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T12:52:42.935Z
Learning: Applies to src/app/teams/[teamId]/**/*.{ts,tsx} : Cache-first node system: Role nodes store only `roleId` in node.data. Display data must be fetched from TanStack Query cache using the `useRoleData()` hook which queries the `role.getByTeamId` procedure.
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T21:27:04.176Z
Learning: Applies to src/server/api/**/*.ts : Use enrichRolesWithUserNames() for flat role arrays and enrichChartRolesWithUserNames() for nested chart roles from src/server/api/utils/organization-members.ts
📚 Learning: 2025-12-29T12:52:42.935Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T12:52:42.935Z
Learning: Applies to src/app/teams/[teamId]/_components/role-node.tsx,src/app/teams/[teamId]/_components/public-role-node.tsx : These role node components are 75% identical and should be consolidated. Extract shared `RoleNodeTemplate` component with `isEditable` prop to DRY up the code.
Applied to files:
src/app/member/[id]/_components/team-section.tsxsrc/app/member/[id]/_components/role-kpi-pair.tsx
📚 Learning: 2025-12-29T21:27:04.176Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T21:27:04.176Z
Learning: Applies to src/app/teams/[teamId]/_components/**/*.tsx : Implement cache-first node pattern for role nodes: store only roleId in node data, fetch display data from TanStack Query cache using useRoleData hook
Applied to files:
src/app/member/[id]/_components/team-section.tsxsrc/app/member/[id]/_components/role-kpi-pair.tsx
📚 Learning: 2025-12-20T22:12:00.576Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-12-20T22:12:00.576Z
Learning: Applies to src/app/teams/[teamId]/**/*.tsx : React Flow nodes must store minimal data (e.g., just `roleId`); fetch full Role data from TanStack Query cache in the Node component to keep canvas and sidebars in sync
Applied to files:
src/app/member/[id]/_components/team-section.tsxsrc/app/member/[id]/_components/role-kpi-pair.tsx
📚 Learning: 2025-12-29T12:52:42.935Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T12:52:42.935Z
Learning: Applies to src/components/dashboard-metric-card.tsx,src/app/dashboard/[teamId]/_components/public-dashboard-metric-card.tsx : Dashboard metric cards are duplicated with public variant. Consolidate into single component with `readOnly` mode prop instead of maintaining separate components.
Applied to files:
src/app/member/[id]/_components/team-section.tsxsrc/app/member/[id]/_components/role-kpi-pair.tsx
📚 Learning: 2025-12-29T21:27:04.176Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T21:27:04.176Z
Learning: Applies to src/app/(teams|org)/**/*.tsx : Use shared MembersList component at src/components/member/member-list.tsx for displaying members in canvas sidebar and org page; it includes getMemberDisplayInfo() utility for initials/name logic
Applied to files:
src/app/member/[id]/_components/team-section.tsxsrc/app/member/[id]/_components/role-kpi-pair.tsx
📚 Learning: 2025-12-29T12:52:42.935Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T12:52:42.935Z
Learning: Applies to src/app/dashboard/[teamId]/**/*.{ts,tsx} : Dashboard cache updates for role-metric assignments must update both `role.getByTeamId` and `dashboard.getDashboardCharts` caches during mutations. Use onMutate for optimistic updates on both caches, then invalidate both on success.
Applied to files:
src/app/member/[id]/_components/team-section.tsxsrc/app/member/[id]/_components/role-kpi-pair.tsx
📚 Learning: 2025-12-29T21:27:04.176Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T21:27:04.176Z
Learning: Applies to src/hooks/**/*.ts : For role-metric cache updates, optimistically update both role cache (role.getByTeamId) and dashboard cache (dashboard.getDashboardCharts) to maintain UI consistency
Applied to files:
src/app/member/[id]/_components/team-section.tsxsrc/app/member/[id]/_components/role-kpi-pair.tsx
📚 Learning: 2025-12-29T12:52:42.935Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T12:52:42.935Z
Learning: Applies to src/app/teams/[teamId]/**/*.{ts,tsx} : Cache-first node system: Role nodes store only `roleId` in node.data. Display data must be fetched from TanStack Query cache using the `useRoleData()` hook which queries the `role.getByTeamId` procedure.
Applied to files:
src/app/member/[id]/_components/team-section.tsxsrc/app/member/[id]/_components/role-kpi-pair.tsx
📚 Learning: 2025-12-20T22:12:00.576Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-12-20T22:12:00.576Z
Learning: Applies to src/app/teams/[teamId]/**/*.tsx : Avoid modifying `enrichNodesWithRoleData` flow without understanding the complete canvas serialization logic for saving/loading React Flow nodes to the database
Applied to files:
src/app/member/[id]/_components/team-section.tsxsrc/app/member/[id]/_components/role-kpi-pair.tsx
📚 Learning: 2025-12-29T21:27:04.176Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T21:27:04.176Z
Learning: Applies to src/server/api/**/*.ts : Use enrichRolesWithUserNames() for flat role arrays and enrichChartRolesWithUserNames() for nested chart roles from src/server/api/utils/organization-members.ts
Applied to files:
src/app/member/[id]/_components/team-section.tsx
📚 Learning: 2025-12-29T21:27:04.176Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T21:27:04.176Z
Learning: Applies to src/app/(metric|dashboard)/**/*.tsx : Use three-stage metrics transformation: API → DataPoints (DataIngestionTransformer), DataPoints → ChartConfig (ChartTransformer), ChartConfig → UI (DashboardMetricChart)
Applied to files:
src/app/member/[id]/_components/team-section.tsxsrc/app/member/[id]/_components/role-kpi-pair.tsx
📚 Learning: 2025-12-29T12:52:42.935Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T12:52:42.935Z
Learning: Applies to src/components/member/member-list.tsx : The MembersList component is consolidated and reused by both canvas sidebar (canvas-side-panels.tsx) and org page (MembersListClient.tsx). Use this component for all member list displays to maintain consistency.
Applied to files:
src/app/member/[id]/_components/team-section.tsx
📚 Learning: 2025-12-29T21:27:04.176Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T21:27:04.176Z
Learning: Applies to src/app/metric/_components/**/*.tsx : Use shared MetricDialogBase component from base/ for all metric dialog implementations
Applied to files:
src/app/member/[id]/_components/team-section.tsxsrc/app/member/[id]/_components/role-kpi-pair.tsx
📚 Learning: 2025-12-29T21:27:04.176Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T21:27:04.176Z
Learning: Applies to src/app/metric/_components/**/*.tsx : Create metric dialogs with pattern: [Provider]MetricDialog.tsx (dialog wrapper) + [Provider]MetricContent.tsx (form content) in src/app/metric/_components/[provider]/, then register in index.ts
Applied to files:
src/app/member/[id]/_components/role-kpi-pair.tsx
🧬 Code graph analysis (2)
src/app/member/[id]/_components/team-section.tsx (1)
src/app/member/[id]/_components/role-kpi-pair.tsx (1)
RoleKpiPair(17-68)
src/app/member/[id]/_components/role-kpi-pair.tsx (4)
src/trpc/react.tsx (1)
RouterOutputs(42-42)src/components/role/role-card.tsx (2)
RoleCardData(28-42)RoleCard(329-329)src/lib/utils.ts (1)
cn(4-6)src/app/member/[id]/_components/member-role-card.tsx (1)
MemberRoleCard(17-85)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
🔇 Additional comments (3)
src/app/member/[id]/_components/role-kpi-pair.tsx (1)
1-15: LGTM: Clean setup with correct Next.js and tRPC patterns.The component follows the client component pattern with proper type inference from RouterOutputs, which aligns with the coding guidelines for tRPC usage in client components.
src/app/member/[id]/_components/team-section.tsx (2)
16-16: LGTM: Successfully refactored to use the new RoleKpiPair component.This change eliminates the duplicate MemberRoleCard component in favor of composing RoleCard and KpiCard, which aligns with the codebase pattern of consolidating duplicate components.
77-89: LGTM: Correct integration of RoleKpiPair with proper prop passing.The layout change to
space-y-4is appropriate since eachRoleKpiPairmanages its own internal grid layout. TheteamIdprop is correctly passed, and the conditionaldashboardChartlookup preserves the original logic.
| {hasMetricButNoChart && ( | ||
| <div className="border-border/60 text-muted-foreground flex items-center justify-center rounded-lg border border-dashed p-6"> | ||
| <span className="text-sm">KPI data loading...</span> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Clarify the loading state message.
The message "KPI data loading..." might be misleading. This condition fires when role.metricId exists but no corresponding dashboardChart was found in the map. This could indicate:
- The chart data hasn't been fetched yet
- The metric exists but has no chart configured
- A data inconsistency
Consider a more accurate message like "No KPI data available" or adding actual loading state tracking if the data is genuinely loading.
💬 Suggested alternative
{hasMetricButNoChart && (
<div className="border-border/60 text-muted-foreground flex items-center justify-center rounded-lg border border-dashed p-6">
- <span className="text-sm">KPI data loading...</span>
+ <span className="text-sm">No KPI data available</span>
</div>
)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {hasMetricButNoChart && ( | |
| <div className="border-border/60 text-muted-foreground flex items-center justify-center rounded-lg border border-dashed p-6"> | |
| <span className="text-sm">KPI data loading...</span> | |
| </div> | |
| )} | |
| {hasMetricButNoChart && ( | |
| <div className="border-border/60 text-muted-foreground flex items-center justify-center rounded-lg border border-dashed p-6"> | |
| <span className="text-sm">No KPI data available</span> | |
| </div> | |
| )} |
🤖 Prompt for AI Agents
In @src/app/member/[id]/_components/role-kpi-pair.tsx around lines 61 - 65, The
current placeholder shown when hasMetricButNoChart is true is ambiguous; update
the UI to distinguish between "no chart configured" and "still loading" by
either changing the message in the hasMetricButNoChart branch to a clearer
statement like "No KPI data available" (or "KPI chart not configured") and/or
wire in an explicit loading flag from where dashboardChart is populated;
specifically, update the JSX branch that checks hasMetricButNoChart (and
references role.metricId and the dashboardChart lookup) to show the clearer
static message or conditionalize on a new loading boolean so genuine loading
uses "Loading KPI data…" while missing chart shows "No KPI data available."
Replace custom MemberRoleCard with existing RoleCard and KpiCard components. Each role now displays as a paired layout with role info on left and KPI on right.
Summary
Refactors the member profile page to eliminate duplicate components by reusing existing RoleCard and KpiCard components.
Changes
RoleKpiPaircomponent with paired layout (role info left, KPI right)TeamSectionto use the new componentMemberRoleCardcomponentSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.