Lewis/policies ssr#159
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request introduces two new React chart components—PolicyAssigneeChart (bar chart) and PolicyStatusChart (pie chart)—to visualize policy data. The dashboard page is refactored to integrate these components using a caching data-fetching mechanism with Suspense. Additionally, a new asynchronous Loading component displays localized spinner cards during data retrieval. Minor formatting updates in the dashboard layout and modifications to global CSS color variables for charts have also been applied. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant PO as PoliciesOverview Page
participant GO as getPoliciesOverview (cache)
participant PSC as PolicyStatusChart
participant PAC as PolicyAssigneeChart
participant L as Loading Component
User->>PO: Request Policy Overview
PO->>GO: Fetch policies data
GO-->>PO: Return data
alt Data available
PO->>PSC: Render PolicyStatusChart with data
PO->>PAC: Render PolicyAssigneeChart with data
else No Data
PO->>L: Show Loading spinner (fallback)
end
Possibly related PRs
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/(overview)/components/policy-status-chart.tsx (3)
41-54: Consider typing the tooltip props properlyThe StatusTooltip component uses
anyfor its props type, which should be avoided in TypeScript for better type safety.-const StatusTooltip = ({ active, payload }: any) => { +interface TooltipProps { + active?: boolean; + payload?: Array<{ + payload: { + name: string; + value: number; + }; + }>; +} + +const StatusTooltip = ({ active, payload }: TooltipProps) => {
77-102: Add handling for empty chart dataThe filter at line 101 might remove all items if all values are 0, potentially leading to an empty chart. Consider handling this edge case.
const chartData = React.useMemo(() => { const items = [ // ...existing items ]; - return items.filter(item => item.value); + const filteredItems = items.filter(item => item.value); + + // Handle case where all values are 0 + if (filteredItems.length === 0) { + return items; // Return all items or at least one item to show something + } + + return filteredItems; }, [data, t]);
147-172: Be consistent with null checksThere's an additional check for the presence of
viewBox.cyon line 164 that isn't present in other places, which could lead to inconsistency.<tspan x={viewBox.cx} - y={(viewBox.cy || 0) + 24} + y={viewBox.cy + 24} className="fill-muted-foreground" >Since you've already checked that
viewBox.cyexists in the if condition, the fallback to 0 is unnecessary.apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/(overview)/components/policy-assignee-chart.tsx (2)
61-64: Consider adding indication of data limitationThe chart only displays the top 4 assignees, but there's no indication to users that data might be truncated if there are more assignees.
Consider adding a note in the card footer indicating when data has been limited:
<CardFooter> <div className="flex flex-wrap gap-4 py-2"> {Object.entries(chartConfig).map(([key, config]) => ( <div key={key} className="flex items-center gap-2"> <div className="h-3 w-3" style={{ backgroundColor: config.color }} /> <span className="text-xs">{config.label}</span> </div> ))} + {data.length > 4 && ( + <span className="text-xs text-muted-foreground ml-auto"> + Showing top 4 of {data.length} assignees + </span> + )} </div> </CardFooter>
117-125: Add horizontal grid lines for better readabilityConsider adding horizontal grid lines to make it easier for users to interpret the values in the bar chart, especially since the XAxis is hidden.
<YAxis dataKey="name" type="category" tickLine={false} tickMargin={10} axisLine={false} tickFormatter={(value) => value.includes(' ') ? value.split(' ')[0] : value} /> +<CartesianGrid horizontal={true} vertical={false} stroke="rgba(0, 0, 0, 0.1)" />Don't forget to add the import:
-import { Bar, BarChart, XAxis, YAxis, Legend, ResponsiveContainer, LabelList } from "recharts" +import { Bar, BarChart, XAxis, YAxis, Legend, ResponsiveContainer, LabelList, CartesianGrid } from "recharts"packages/ui/src/globals.css (1)
28-32: Consider verifying the naming vs. color usage.
Using “warning” for a grayish hue and “neutral” for a bright yellow might be confusing. Verify these labels with your design guidelines to avoid semantic inconsistencies.apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/(overview)/page.tsx (1)
6-8: Add a fallback to Suspense.
Currently, there’s no fallback provided for<Suspense>, which can momentarily leave a blank screen. Consider adding a loading indicator or fallback component for better user experience.import { cache, Suspense } from "react"; import { PolicyStatusChart } from "./components/policy-status-chart"; import { PolicyAssigneeChart } from "./components/policy-assignee-chart"; -export default async function PoliciesOverview({...}) { +export default async function PoliciesOverview({...}) { + // ... + return ( + <Suspense fallback={<div>Loading...</div>}> + {/* existing content */} + </Suspense> + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/(overview)/components/policy-assignee-chart.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/(overview)/components/policy-status-chart.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/(overview)/layout.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/(overview)/loading.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/(overview)/page.tsx(1 hunks)packages/ui/src/globals.css(2 hunks)
🔇 Additional comments (9)
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/(overview)/loading.tsx (1)
5-31: Well-structured loading component for policy charts!The async Loading component is well implemented, showing loading spinners in a layout that mirrors the actual chart components. This provides a smooth user experience while data is being fetched.
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/(overview)/layout.tsx (1)
6-14: Approved variable renaming for consistencyThe renaming from
organizationIdtoorgIdbrings consistency with the URL path structure, which is a good improvement.apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/(overview)/components/policy-status-chart.tsx (1)
33-38: LGTM: Good use of CSS variables for chart colorsUsing CSS variables for chart colors allows for consistent theming and easier maintenance.
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/(overview)/components/policy-assignee-chart.tsx (2)
30-35: Consistent chart color schemeGreat job maintaining consistency with the color scheme used in the PolicyStatusChart component.
124-124:Details
✅ Verification successful
Handle names without spaces
The tickFormatter function assumes that all names contain spaces, which might not always be true.
-tickFormatter={(value) => value.split(' ')[0]} +tickFormatter={(value) => value.includes(' ') ? value.split(' ')[0] : value}Let's verify if the users or assignees in your system might have single-word names:
🏁 Script executed:
#!/bin/bash # Let's check for any user records that might have single-word names fd -e tsx -e ts -e js | xargs grep -l "user.*name" | xargs cat | grep -E "name.*:" | head -n 20Length of output: 708
Update tickFormatter to handle single-word names correctly
The current implementation always splits the string assuming a space exists. Our verification confirms that some assignee names (e.g., "asc") do not contain spaces, so the condition check ensures the formatter returns the original name when no space is present.
- File:
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/(overview)/components/policy-assignee-chart.tsx- Line: 124
- Diff Snippet:
-tickFormatter={(value) => value.split(' ')[0]} +tickFormatter={(value) => value.includes(' ') ? value.split(' ')[0] : value}packages/ui/src/globals.css (1)
73-77: Ensure consistency between light and dark modes.
These color definitions mirror those in light mode, which is good for continuity. Confirm that the same reasoning applies so users don’t misinterpret the color states in dark mode.apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/(overview)/page.tsx (3)
32-148: Good data aggregation approach with parallel queries.
Combining groupBy and findMany in a transaction is efficient and straightforward. No obvious concurrency or PII concerns here.
150-163: Metadata generation looks fine.
The code properly localizes the page title. No issues found.
10-30:Details
✅ Verification successful
Check the promise-based params usage.
It’s somewhat unusual thatparamsis declared as aPromise<{ locale: string }>rather than a plain object. Verify that this is intentional and consistent with your Next.js configuration. Otherwise, it may be clearer to pass an already-resolved object.
🏁 Script executed:
#!/bin/bash # Description: Search for references of the `params` object usage across the codebase # to confirm it is consistently treated as a Promise. rg -A 5 "function PoliciesOverview" rg -A 5 "function generateMetadata"Length of output: 16227
Promise-based params usage confirmed and consistent across the repository.
The search results indicate that the pattern of declaring
paramsas aPromise<{ locale: string }>is consistently applied not only inPoliciesOverviewbut also in multiple instances (e.g., in variousgenerateMetadatafunctions). This confirms that the use of asynchronous params is intentional and aligns with your Next.js configuration. No changes are necessary here.
1 participant
Summary by CodeRabbit
New Features
Refactor/Style