feat: add usage-insight on the desktop-sidebar#2957
Conversation
revogabe
requested review from
MichaelUnkey,
chronark,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@revogabe is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis pull request updates multiple parts of the dashboard to support quota-based usage tracking. It renames the existing Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant UB as UsageBanner
participant TRPC as TRPC Billing Query
participant DB as ClickHouse DB
U->>UB: Loads dashboard
UB->>TRPC: Invoke queryUsage
TRPC->>DB: Fetch billableRatelimits & verifications concurrently
DB-->>TRPC: Return usage data
TRPC-->>UB: Return aggregated billing usage
UB->>UB: Compute progress and select color (red/green)
UB-->>U: Render usage progress circle & upgrade CTA
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🪧 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 (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
apps/dashboard/lib/create-context.tsx (1)
3-37: Well-designed context utility with a minor optimization opportunityThe
createContextutility is a well-structured wrapper around React's Context API that provides proper type safety, error handling, and debugging capabilities.Consider refining the
useMemodependency array in lines 16-18. UsingObject.values(context)as dependencies might cause unnecessary re-renders if the order of properties changes. A potentially more stable approach would be:const value = React.useMemo( () => context, - Object.values(context) + [context] ) as ContextValueTypeIf specific property tracking is needed, you could consider using a more explicit array of dependencies derived from the context object.
apps/dashboard/lib/mouse.tsx (1)
8-27: Well-implemented mouse position hook with performance optimization opportunityThe
useMousePositionhook is clean, focused, and follows React best practices with proper cleanup.Consider adding throttling or debouncing to reduce re-renders when this hook is used with animation components (like the Particles component mentioned in the PR summary):
import { useEffect, useState } from "react"; + import { throttle } from "lodash"; // or implement a simple throttle function export function useMousePosition(): MousePosition { const [mousePosition, setMousePosition] = useState<MousePosition>({ x: 0, y: 0, }); useEffect(() => { - const handleMouseMove = (event: MouseEvent) => { + const handleMouseMove = throttle((event: MouseEvent) => { setMousePosition({ x: event.clientX, y: event.clientY }); - }; + }, 16); // ~60fps window.addEventListener("mousemove", handleMouseMove); return () => { window.removeEventListener("mousemove", handleMouseMove); + handleMouseMove.cancel?.(); // Clean up throttled function if using lodash }; }, []); return mousePosition; }This would help maintain smooth performance, especially when used with animation components that could otherwise cause excessive re-renders.
apps/dashboard/components/ui/particles.tsx (2)
74-86: Consider optimizing mouse tracking.The current implementation recalculates mouse position on every mouse move, which could be performance-intensive. Consider using throttling or debouncing for the mouse position updates.
+import { useCallback, useEffect, useRef, useMemo } from "react"; +import debounce from "lodash/debounce"; // Add lodash as a dependency if not already present // Then replace the onMouseMove implementation: -const onMouseMove = useCallback(() => { +const onMouseMove = useMemo(() => debounce(() => { if (canvasRef.current) { const rect = canvasRef.current.getBoundingClientRect(); const { w, h } = canvasSize.current; const x = mousePosition.x - rect.left - w / 2; const y = mousePosition.y - rect.top - h / 2; const inside = x < w / 2 && x > -w / 2 && y < h / 2 && y > -h / 2; if (inside) { mouse.current.x = x; mouse.current.y = y; } } -}, [mousePosition.x, mousePosition.y]); +}, 10), [mousePosition.x, mousePosition.y]); // 10ms debounce time
145-159: Add error handling for canvas operations.The drawCircle function and other canvas operations don't have error handling, which could lead to silent failures.
const drawCircle = (circle: Circle, update = false) => { if (context.current) { - const { x, y, translateX, translateY, size, alpha } = circle; - context.current.translate(translateX, translateY); - context.current.beginPath(); - context.current.arc(x, y, size, 0, 2 * Math.PI); - context.current.fillStyle = `rgba(${rgb.join(", ")}, ${alpha})`; - context.current.fill(); - context.current.setTransform(dpr, 0, 0, dpr, 0, 0); + try { + const { x, y, translateX, translateY, size, alpha } = circle; + context.current.translate(translateX, translateY); + context.current.beginPath(); + context.current.arc(x, y, size, 0, 2 * Math.PI); + context.current.fillStyle = `rgba(${rgb.join(", ")}, ${alpha})`; + context.current.fill(); + context.current.setTransform(dpr, 0, 0, dpr, 0, 0); - if (!update) { - circles.current.push(circle); + if (!update) { + circles.current.push(circle); + } + } catch (error) { + console.error("Error drawing particle:", error); } } };apps/dashboard/components/billing/usage-insights.tsx (4)
97-100: Consider touch device compatibility for Details componentThe Details component visibility is controlled by the group-hover class, which works well for desktop but may not be ideal for touch devices. Consider adding a way to toggle visibility on touch/mobile devices.
+ const [isOpen, setIsOpen] = React.useState(false); + return ( <div {...detailsProps} ref={ref} className={cn( - "h-0 group-hover:h-12 duration-500 ease-out w-full overflow-hidden group-hover:opacity-100 opacity-0 flex flex-col gap-4 group-hover:mt-4 mt-1.5", + "h-0 duration-500 ease-out w-full overflow-hidden opacity-0 flex flex-col gap-4 mt-1.5", + "group-hover:h-12 group-hover:opacity-100 group-hover:mt-4", + isOpen && "h-12 opacity-100 mt-4", { className }, )} + onClick={() => setIsOpen(!isOpen)} >
139-143: Consider adding aria-label for accessibilityThe ProgressCircle component would benefit from an aria-label or aria-describedby attribute to improve accessibility for screen readers.
<ProgressCircle value={item?.current ?? current} max={item?.max ?? max} color={color ?? "#f76e19"} + aria-label={`${title}: ${item?.current ?? current} of ${item?.max ?? max}`} />
165-168: Consider adding default styling to Footer componentThe Footer component doesn't have any default styling unlike the other components. Consider adding some basic styling for consistency or documenting its intended use.
return ( - <div {...detailsProps} ref={ref} className={cn({ className })}> + <div {...detailsProps} ref={ref} className={cn("mt-2 px-2 text-sm text-gray-11", className)}> {children} </div>
28-35: Consider memoizing formatting function for performanceThe number formatting functions are recreated on every render. Consider memoizing these functions using useMemo to improve performance, especially if this component rerenders frequently.
+ import React, { useMemo } from "react"; export const Root = React.forwardRef<DivElement, UsageRootProps>((props, ref) => { const { plan, current, max, className, children, ...rootProps } = props; - const { format } = Intl.NumberFormat(undefined, { notation: "compact" }); + const format = useMemo(() => { + return new Intl.NumberFormat(undefined, { notation: "compact" }).format; + }, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
apps/dashboard/app/(app)/desktop-sidebar.tsx(4 hunks)apps/dashboard/app/(app)/layout.tsx(2 hunks)apps/dashboard/app/(app)/settings/billing/components/usage.tsx(2 hunks)apps/dashboard/app/(app)/user-button.tsx(3 hunks)apps/dashboard/components/billing/usage-insights.tsx(1 hunks)apps/dashboard/components/ui/particles.tsx(1 hunks)apps/dashboard/lib/create-context.tsx(1 hunks)apps/dashboard/lib/mouse.tsx(1 hunks)apps/dashboard/package.json(1 hunks)
🔇 Additional comments (17)
apps/dashboard/package.json (1)
26-26: Appropriate dependency additionThe addition of
@radix-ui/react-primitivealigns well with the project's existing usage of Radix UI components and is likely required for the new usage insights feature.apps/dashboard/app/(app)/user-button.tsx (3)
29-29: Improved UI sizing and spacingThe height reduction from
h-12toh-10and padding adjustment fromp-2topx-2makes the user button more compact, which aligns well with its placement in the usage insights sidebar.
58-58: Enhanced user experience with text selection preventionAdding the
select-noneclass prevents accidental text selection when clicking on the dropdown, which improves usability.
71-71: Improved dropdown alignmentAdding
align="start"ensures the dropdown content aligns properly with the trigger element, providing a more polished UI interaction.apps/dashboard/app/(app)/settings/billing/components/usage.tsx (2)
25-29: Added color customization to ProgressCircle component.This change enhances the reusability of the ProgressCircle by adding an optional color prop to customize the stroke color. The implementation correctly maintains backward compatibility since the color parameter is optional.
72-72: Proper implementation of the color prop.The color prop is correctly applied to the stroke style of the circle element, allowing for dynamic customization of the progress indicator.
apps/dashboard/app/(app)/layout.tsx (4)
2-2: Added clickhouse import for billing metrics.The import is necessary for the new billing metric functionality.
24-24: Included quota in workspace query.Adding the quota field to the workspace query is necessary to determine the maximum number of requests for usage calculations.
58-62: Passing billing data to DesktopSidebar.The implementation correctly passes the calculated billing metrics to the DesktopSidebar component with a sensible fallback value of 150,000 for maxRequests if quota is undefined.
31-49:Details
❓ Verification inconclusive
Added billing metrics calculation for usage insights.
The implementation correctly calculates the start of the current month and fetches billing metrics from ClickHouse using Promise.all for parallel execution. This approach is efficient and follows good practices.
However, since this is in a layout component that will be rendered frequently, consider potential performance implications.
Let's verify if these database queries might impact page load performance:
🏁 Script executed:
#!/bin/bash # Check if similar queries are already cached or memoized elsewhere in the codebase # Look for any caching mechanisms for clickhouse billing data rg -A 3 -B 3 "clickhouse.billing" --glob "!**/layout.tsx" # Check for any SWR or React Query implementations that might be reused rg -A 2 "useSWR|useQuery" --glob "**/*.tsx"Length of output: 4711
Performance Warning: Verify Potential Impact on Frequently Rendered Layout
The billing metrics code correctly computes the month start and retrieves data in parallel using
Promise.all, which is an efficient approach. However, since this code runs in a layout component that is rendered on every page load, it could lead to performance overhead if these heavy billing queries are executed too frequently.
- Existing patterns: Similar billing queries are executed in other parts of the codebase (e.g., invoicing and billing settings pages), and none appear to leverage caching or memoization strategies.
- Recommendation: Evaluate whether these queries should be cached or memoized (using SWR, React Query, etc.) to prevent potential performance degradation on repeated renders.
apps/dashboard/app/(app)/desktop-sidebar.tsx (4)
3-3: Added UsageInsight component import.This import is necessary for the new usage insights feature.
26-30: Added requests prop to DesktopSidebar.The interface is well-structured with clear type definitions for the request metrics.
46-46: Updated component signature with requests prop.The function signature correctly includes the new requests parameter.
105-118: Implemented usage insights in sidebar footer.The implementation uses the UsageInsight component hierarchy appropriately to display the usage metrics. The bottom-2 positioning change (from previous bottom-0) ensures proper spacing from the edge of the container.
The component successfully combines both the usage metrics display and the existing UserButton, maintaining all required functionality while adding the new feature.
apps/dashboard/components/billing/usage-insights.tsx (3)
44-50: Good implementation of Particles component with dynamic coloringThe Particles component implementation is well done, with dynamic color based on the plan type and good visual effects that only appear on hover. The opacity transition and z-indexing ensure it doesn't interfere with content.
68-72: Upgrade button logic is well implementedThe conditional rendering of the upgrade button when usage exceeds 94% is a good feature that aligns well with the PR objectives of providing insights and encouraging plan upgrades when needed.
1-7: Well-structured component with compound patternThe overall component structure follows the compound component pattern effectively, with a clean separation of concerns between Root, Details, Item, and Footer. The exports are also well organized using Object.assign for a nice developer experience.
Also applies to: 178-186
…the card always open
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/dashboard/app/(app)/desktop-sidebar.tsx (1)
82-84: Fix typo in tooltip textThere's a typo in the tooltip content for subscription ending notification.
- Your plan is schedueld to be downgraded to the{" "} + Your plan is scheduled to be downgraded to the{" "}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/dashboard/app/(app)/desktop-sidebar.tsx(5 hunks)apps/dashboard/components/billing/usage-insights.tsx(1 hunks)
🔇 Additional comments (9)
apps/dashboard/app/(app)/desktop-sidebar.tsx (1)
117-128: The usage insight integration looks good!The integration of the UsageInsight component provides a clear visual representation of token usage for users, which aligns perfectly with the PR objective. The implementation correctly calculates total usage by summing rate limits and verifications.
apps/dashboard/components/billing/usage-insights.tsx (8)
61-63: Fix className merging in cn functionThe className merging syntax is incorrect. The
cnfunction expects a list of class names or objects where keys are class names and values are booleans.- className={cn( - "relative flex flex-col bg-background border border-border rounded-xl pt-2.5 pb-2 px-2 group overflow-hidden w-full", - { className } - )} + className={cn( + "relative flex flex-col bg-background border border-border rounded-xl pt-2.5 pb-2 px-2 group overflow-hidden w-full", + className + )}
72-81: Good conditional rendering for upgrade buttonI like how you've implemented conditional rendering based on usage thresholds. Showing an "Upgrade" button when usage is ≥94% and a "Manage Plan" button otherwise provides users with contextual actions.
116-120: Same className merging issue as in Root componentFix the className merging here as well.
- className={cn( - "h-16 duration-500 ease-out w-full flex flex-col gap-4 items-start justify-center pt-2", - { - className, - } - )} + className={cn( + "h-16 duration-500 ease-out w-full flex flex-col gap-4 items-start justify-center pt-2", + className + )}
150-150: Fix the Intl.NumberFormat instantiationThe Intl.NumberFormat usage is incorrect. You need to instantiate it with
newand then call theformatmethod on the instance.- const { format } = Intl.NumberFormat(undefined, { notation: "compact" }); + const format = new Intl.NumberFormat(undefined, { notation: "compact" }).format;
157-159: Same className merging issueFix the className merging here as well.
- className={cn("flex items-start justify-start px-2 gap-3", { - className, - })} + className={cn("flex items-start justify-start px-2 gap-3", className)}
161-165: Well-designed usage visualization with ProgressCircleThe usage visualization with ProgressCircle provides users with an intuitive representation of their consumption relative to their plan limits. The ability to customize the color is a nice touch.
168-171: Good formatting of usage valuesUsing compact notation for the usage values makes the numbers more readable, especially for larger values. The description parameter allows for contextual labeling of the metrics.
184-191: Excellent component composition patternUsing
Object.assignto create a composite component with nested parts provides a clean API for consuming the component. This pattern makes the usage more intuitive and maintains proper component hierarchy.
Usage card still shows 150K like it's hardcoded.
|
|
Could you help me configure the Stripe environment variables so I can test switching plans? The documentation isn't very clear on how to set them up, and I couldn't find any mention of a .env.example file containing the variables. By configuring Stripe, I'll be able to test and understand what's happening since the PR from @chronark fixed this issue #3007 . The problem was that for personal workspaces, when creating an account, it didn't generate a quotas table, which forced the system to display a hardcoded value. |
|
The stripe variable keys are in apps/dashboard/lib/env.ts. You'll have to supply your own stripe variable values though. We can't give out Unkey's values. |
|
We also fixed it so that quotas table is generated for first-time users' personal workspaces, so after pulling from main, when you clear your database (bad data), it should include the quotas table now. |
|
yeah this should be fixed, |
|
Thanks @mcstepp, of course I'll set up my own stripe, I just hadn't noticed any mention of Stripe's env.example in the code but I checked now and will test it, don't worry 😄 |
This fixes the incorrect usage max issue
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
apps/dashboard/lib/trpc/routers/billing/query-usage/index.ts (1)
28-33: Consider enhancing error handling with specific error messagesWhile the error handling catches null responses correctly, it could be improved by providing more specific error messages that indicate which part of the query failed.
if (billableRatelimits === null || billableVerifications === null) { throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", - message: "Failed to fetch billing usage data. Please try again later.", + message: `Failed to fetch billing usage data: ${billableRatelimits === null ? 'rate limits' : ''} ${billableVerifications === null ? 'verifications' : ''}. Please try again later.`, }); }apps/dashboard/components/navigation/sidebar/usage-banner.tsx (1)
27-36: Extract hardcoded values into constantsThe color values and usage threshold are hardcoded within the component, making them difficult to update consistently.
+"use client"; +import { ProgressCircle } from "@/app/(app)/settings/billing/components/usage"; +import { trpc } from "@/lib/trpc/client"; +import type { Quotas } from "@unkey/db"; +import { Button } from "@unkey/ui"; +import Link from "next/link"; +import type React from "react"; +import { FlatNavItem } from "./app-sidebar/components/nav-items/flat-nav-item"; + +const USAGE_THRESHOLD = 0.9; // 90% +const COLORS = { + error: "#DD4527", // error-9 + success: "#0A9B8B", // success-9 +}; + type Props = { quotas: Quotas; }; export const UsageBanner: React.FC<Props> = ({ quotas }) => { const usage = trpc.billing.queryUsage.useQuery(); const current = usage.data?.billableTotal ?? 0; const max = quotas.requestsPerMonth; - const shouldUpgrade = current / max > 0.9; + const shouldUpgrade = max > 0 ? (current / max > USAGE_THRESHOLD) : false; return ( <FlatNavItem item={{ tooltip: "Usage", icon: () => ( <ProgressCircle value={current} max={max} color={ shouldUpgrade - ? "#DD4527" // error-9 - : "#0A9B8B" // success-9 + ? COLORS.error + : COLORS.success } />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/dashboard/app/(app)/layout.tsx(2 hunks)apps/dashboard/app/(app)/settings/billing/client.tsx(4 hunks)apps/dashboard/app/(app)/settings/billing/page.tsx(3 hunks)apps/dashboard/app/(app)/settings/team/page.tsx(1 hunks)apps/dashboard/components/app-sidebar.tsx(0 hunks)apps/dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/flat-nav-item.tsx(1 hunks)apps/dashboard/components/navigation/sidebar/app-sidebar/index.tsx(3 hunks)apps/dashboard/components/navigation/sidebar/usage-banner.tsx(1 hunks)apps/dashboard/components/navigation/sidebar/user-button.tsx(1 hunks)apps/dashboard/lib/trpc/routers/billing/query-usage/index.ts(1 hunks)apps/dashboard/lib/trpc/routers/index.ts(2 hunks)
💤 Files with no reviewable changes (1)
- apps/dashboard/components/app-sidebar.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/flat-nav-item.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/dashboard/lib/trpc/routers/index.ts
- apps/dashboard/app/(app)/layout.tsx
🧰 Additional context used
🧬 Code Definitions (2)
apps/dashboard/lib/trpc/routers/billing/query-usage/index.ts (3)
apps/dashboard/lib/trpc/trpc.ts (3)
t(8-8)requireWorkspace(23-33)withRatelimit(139-154)apps/dashboard/lib/trpc/routers/billing/query-usage/schemas.ts (1)
queryUsageResponse(3-7)apps/dashboard/lib/clickhouse.ts (1)
clickhouse(4-4)
apps/dashboard/components/navigation/sidebar/app-sidebar/index.tsx (1)
apps/dashboard/components/ui/sidebar.tsx (3)
Sidebar(585-585)SidebarContent(586-586)SidebarGroup(588-588)
🔇 Additional comments (18)
apps/dashboard/components/navigation/sidebar/user-button.tsx (1)
23-24: Improved layout stability with placeholder divThe change from returning
nullto returning a fixed-height div ensures consistent layout rendering when user data is unavailable or there's an error. This prevents layout shifts that could occur during loading states or error conditions.apps/dashboard/app/(app)/settings/team/page.tsx (2)
16-16: Property name change from 'quota' to 'quotas'This change correctly updates the database query to retrieve the renamed
quotasproperty. This aligns with the PR's objective of renaming the quota property throughout the application.
19-19: Updated reference to the renamed propertyThe reference has been properly updated to use the new
quotasproperty name while maintaining the same optional chaining pattern.apps/dashboard/app/(app)/settings/billing/page.tsx (3)
22-24: Property name change from 'quota' to 'quotas' in queryThis change correctly updates the database query to retrieve the renamed
quotasproperty. This maintains consistency with similar changes throughout the codebase.
139-143: Updated object structure for quotasThe object structure has been updated to use
quotasas the property name while maintaining the same internal structure. The propertyrequestsPerMonthis still populated from the same metadata field.
168-168: Updated reference to workspace quota propertyThe code correctly accesses the renamed property while maintaining the same fallback value of 150,000 requests per month.
apps/dashboard/lib/trpc/routers/billing/query-usage/index.ts (5)
1-5: Well-structured importsThe imports are well-organized, separating internal dependencies from external packages. The imports include authentication middleware, error handling from tRPC, and schema validation.
6-9: Appropriate middleware usageThe procedure correctly utilizes the
requireWorkspacemiddleware to ensure a workspace context is available and applies rate limiting withwithRatelimit. The output schema is properly defined using the imported schema.
10-14: Effective date handling for current billing periodThe code correctly calculates the current year and month for billing queries using UTC time, ensuring consistency regardless of the server's timezone.
15-27: Efficient parallel data fetchingUsing
Promise.allto fetch both billable rate limits and verifications in parallel is an efficient approach that minimizes response time.
35-39: Clean and well-structured responseThe response object is clean and well-structured, returning both individual metrics and a calculated total. This approach is flexible and allows consumers to use the data in various ways.
apps/dashboard/app/(app)/settings/billing/client.tsx (4)
36-38: Prop renamed fromquotatoquotas- Consistent naming changeThe property has been correctly renamed from singular
quotato pluralquotas. This change is consistent with the changes throughout the rest of the codebase and reflects what appears to be a shift to handle multiple quota types.
119-119: Renamed property reference updated correctlyThe reference has been properly updated from
p.quota.requestsPerMonthtop.quotas.requestsPerMonthto match the type definition change.
134-135: Renamed property reference updated correctlyReference updated from
p.quota.requestsPerMonthtop.quotas.requestsPerMonthto maintain consistency with the type definition change.
154-155: Renamed property reference updated correctlyReference updated from
p.quota.requestsPerMonthtop.quotas.requestsPerMonth, maintaining consistency with the type changes.apps/dashboard/components/navigation/sidebar/app-sidebar/index.tsx (3)
18-18: Correctly updated imports to include QuotasThe import statement has been properly updated to include the Quotas type alongside Workspace.
30-31: Handle optional quotas in component propsThe type declaration correctly marks
quotasas optional with?, but the implementation doesn't handle the case where it might be undefined.
97-97:Details
❓ Verification inconclusive
Sidebar layout modification could affect nested components
Adding
flex flex-col justify-betweento SidebarContent changes its layout behavior. Ensure this doesn't negatively impact other components within the sidebar.
🌐 Web query:
What's the impact of changing a container from regular block layout to flex with justify-between?💡 Result:
Changing a container from a regular block layout to Flexbox with
justify-content: space-betweenhas significant impacts on element positioning, spacing, and alignment:Layout Behavior
- Block → Flex transformation: Child elements switch from vertical stacking (block) to horizontal alignment by default (
flex-direction: row)[2][8].- Space distribution: Available horizontal space is automatically allocated between items, pushing the first item to the container's start edge and last item to the end edge[1][4].
Key Changes
Automatic spacing
No manual margin calculations needed - space between items adjusts dynamically with container width[1][6].Item positioning
- Single item: Aligns to start edge (unlike
space-around/space-evenly)[9]- Multiple items: Evenly spaced with no external padding[4][8]
Responsive behavior
Items shrink/grow based on content but maintain space-between distribution when resizing[6][9].Edge cases
- No effect if items occupy 100% width[3][6]
- Wrapping creates independent space-between per line[9]
Cross-Axis Implications
While
justify-contenthandles main-axis (horizontal in rows), vertical alignment defaults tostretch(items fill container height). Usealign-itemsto override[2][8]..container { display: flex; justify-content: space-between; /* Main axis control */ align-items: center; /* Cross axis override */ }This combination creates predictable horizontal spacing while allowing vertical alignment customization[4][8].
Citations:
- 1: https://www.youtube.com/watch?v=2CjQxFIUBlA
- 2: https://www.w3schools.com/cssref/css3_pr_justify-content.php
- 3: https://www.smashingmagazine.com/2018/08/flexbox-alignment/
- 4: https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_flexible_box_layout/Aligning_items_in_a_flex_container
- 5: https://blog.jim-nielsen.com/2023/justify-space-between-individual-items/
- 6: https://www.smashingmagazine.com/2018/08/flexbox-display-flex-container/
- 7: https://tailwindcss.com/docs/justify-content
- 8: https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_flexible_box_layout/Basic_concepts_of_flexbox
- 9: https://developer.mozilla.org/en-US/docs/Web/CSS/justify-content
- 10: https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_flexible_box_layout/Typical_use_cases_of_flexbox
Sidebar Layout Modification and Nested Component Verification
Adding
flex flex-col justify-betweentoSidebarContentshifts the container from a standard block layout to a flex layout with a vertical flow (due toflex-col). This makes the first child align at the top and the last child at the bottom with any intermediate children evenly spaced between them. Please verify that these layout changes do not adversely affect the spacing or alignment of nested sidebar components.
- Confirm that the vertical spacing and alignment of all child elements remain consistent with design expectations.
- Check for any unexpected shifts in the positioning of nested components following this flex layout update.
| return ( | ||
| <FlatNavItem | ||
| item={{ | ||
| tooltip: "Usage", | ||
| icon: () => ( | ||
| <ProgressCircle | ||
| value={current} | ||
| max={max} | ||
| color={ | ||
| shouldUpgrade | ||
| ? "#DD4527" // error-9 | ||
| : "#0A9B8B" // success-9 | ||
| } | ||
| /> | ||
| ), | ||
| href: "/settings/billing", | ||
| label: `Usage ${Math.round((current / max) * 100).toLocaleString()}%`, | ||
| tag: shouldUpgrade ? ( | ||
| <Link href="/settings/billing"> | ||
| <Button variant="primary" size="sm"> | ||
| Upgrade | ||
| </Button> | ||
| </Link> | ||
| ) : null, | ||
| }} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add loading state to improve user experience
The component doesn't show any indication while data is being loaded, which could confuse users.
return (
<FlatNavItem
item={{
tooltip: "Usage",
icon: () => (
+ usage.isLoading ? (
+ <div className="h-6 w-6 animate-pulse rounded-full bg-gray-5" />
+ ) : (
<ProgressCircle
value={current}
max={max}
color={
shouldUpgrade
? "#DD4527" // error-9
: "#0A9B8B" // success-9
}
/>
+ )
),
href: "/settings/billing",
- label: `Usage ${Math.round((current / max) * 100).toLocaleString()}%`,
+ label: usage.isLoading
+ ? "Loading usage..."
+ : `Usage ${max > 0 ? Math.round((current / max) * 100).toLocaleString() : 0}%`,
tag: shouldUpgrade ? (
<Link href="/settings/billing">
<Button variant="primary" size="sm">
Upgrade
</Button>
</Link>
) : null,
}}
/>
);📝 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.
| return ( | |
| <FlatNavItem | |
| item={{ | |
| tooltip: "Usage", | |
| icon: () => ( | |
| <ProgressCircle | |
| value={current} | |
| max={max} | |
| color={ | |
| shouldUpgrade | |
| ? "#DD4527" // error-9 | |
| : "#0A9B8B" // success-9 | |
| } | |
| /> | |
| ), | |
| href: "/settings/billing", | |
| label: `Usage ${Math.round((current / max) * 100).toLocaleString()}%`, | |
| tag: shouldUpgrade ? ( | |
| <Link href="/settings/billing"> | |
| <Button variant="primary" size="sm"> | |
| Upgrade | |
| </Button> | |
| </Link> | |
| ) : null, | |
| }} | |
| /> | |
| ); | |
| return ( | |
| <FlatNavItem | |
| item={{ | |
| tooltip: "Usage", | |
| icon: () => ( | |
| usage.isLoading ? ( | |
| <div className="h-6 w-6 animate-pulse rounded-full bg-gray-5" /> | |
| ) : ( | |
| <ProgressCircle | |
| value={current} | |
| max={max} | |
| color={ | |
| shouldUpgrade | |
| ? "#DD4527" // error-9 | |
| : "#0A9B8B" // success-9 | |
| } | |
| /> | |
| ) | |
| ), | |
| href: "/settings/billing", | |
| label: usage.isLoading | |
| ? "Loading usage..." | |
| : `Usage ${max > 0 ? Math.round((current / max) * 100).toLocaleString() : 0}%`, | |
| tag: shouldUpgrade ? ( | |
| <Link href="/settings/billing"> | |
| <Button variant="primary" size="sm"> | |
| Upgrade | |
| </Button> | |
| </Link> | |
| ) : null, | |
| }} | |
| /> | |
| ); |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
apps/dashboard/components/navigation/sidebar/usage-banner.tsx (4)
15-18: 🛠️ Refactor suggestionRefactor TRPC query configuration for better error handling
The TRPC query is configured with refetching options but lacks error handling. If the API call fails, the component will silently fail without informing the user.
const usage = trpc.billing.queryUsage.useQuery(undefined, { refetchOnMount: true, refetchInterval: 60 * 1000, + retry: 2, + onError: (error) => { + console.error("Failed to fetch usage data:", error); + } });
23-23:⚠️ Potential issueAdd safeguard against division by zero
The current calculation will throw an error if
maxis zero. Add a safeguard to handle this edge case.- const shouldUpgrade = current / max > 0.9; + const shouldUpgrade = max > 0 ? (current / max > 0.9) : false;
29-39: 🛠️ Refactor suggestionAdd loading state for better user experience
The component doesn't show any loading indicator while data is being fetched, which could confuse users.
icon: () => ( + usage.isLoading ? ( + <div className="h-6 w-6 animate-pulse rounded-full bg-gray-5" /> + ) : ( <ProgressCircle value={current} max={max} color={ shouldUpgrade ? "#DD4527" // error-9 : "#0A9B8B" // success-9 } /> + ) ),
41-41:⚠️ Potential issueFix potential division by zero in percentage calculation
Similar to the
shouldUpgradecalculation, the percentage calculation doesn't handle the case wheremaxis zero.- label: `Usage ${Math.round((current / max) * 100).toLocaleString()}%`, + label: usage.isLoading + ? "Loading usage..." + : `Usage ${max > 0 ? Math.round((current / max) * 100).toLocaleString() : 0}%`,
🧹 Nitpick comments (2)
apps/dashboard/components/navigation/sidebar/usage-banner.tsx (2)
14-52: Optimize component with memoization and add error stateThe component recalculates values on every render and doesn't handle error states. Consider using
useMemofor calculated values and display an error state when data fetching fails.export const UsageBanner: React.FC<Props> = ({ quotas }) => { const usage = trpc.billing.queryUsage.useQuery(undefined, { refetchOnMount: true, refetchInterval: 60 * 1000, + retry: 2, + onError: (error) => { + console.error("Failed to fetch usage data:", error); + } }); const current = usage.data?.billableTotal ?? 0; const max = quotas.requestsPerMonth; - const shouldUpgrade = current / max > 0.9; + const shouldUpgrade = React.useMemo(() => + max > 0 ? (current / max > 0.9) : false + , [current, max]); + + const usagePercentage = React.useMemo(() => + max > 0 ? Math.round((current / max) * 100).toLocaleString() : 0 + , [current, max]); + if (usage.error) { + return ( + <FlatNavItem + item={{ + tooltip: "Usage", + icon: () => <div className="h-6 w-6 rounded-full bg-error-3 flex items-center justify-center"><span className="text-error-9 text-xs">!</span></div>, + href: "/settings/billing", + label: "Error loading usage", + }} + /> + ); + } return ( <FlatNavItem item={{ tooltip: "Usage", icon: () => ( + usage.isLoading ? ( + <div className="h-6 w-6 animate-pulse rounded-full bg-gray-5" /> + ) : ( <ProgressCircle value={current} max={max} color={ shouldUpgrade ? "#DD4527" // error-9 : "#0A9B8B" // success-9 } /> + ) ), href: "/settings/billing", - label: `Usage ${Math.round((current / max) * 100).toLocaleString()}%`, + label: usage.isLoading ? "Loading usage..." : `Usage ${usagePercentage}%`, tag: shouldUpgrade ? ( <Link href="/settings/billing"> <Button variant="primary" size="sm"> Upgrade </Button> </Link> ) : null, }} /> ); };
16-17: Consider adding staleTime configuration for performance optimizationThe current configuration refreshes data frequently but doesn't set a staleTime. This might cause unnecessary re-renders. Adding staleTime would reduce server load and improve performance.
const usage = trpc.billing.queryUsage.useQuery(undefined, { refetchOnMount: true, refetchInterval: 60 * 1000, + staleTime: 30 * 1000, // Consider data fresh for 30 seconds });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/dashboard/components/navigation/sidebar/usage-banner.tsx(1 hunks)
🔇 Additional comments (1)
apps/dashboard/components/navigation/sidebar/usage-banner.tsx (1)
42-48:Details
❓ Verification inconclusive
Consider conditionally displaying the Upgrade button based on usage context
According to the PR comments from chronark, the Upgrade button should only be visible when users are near or above quota (>90%). Your implementation matches this requirement, but there could be additional considerations based on the user's subscription status.
Consider checking if this implementation aligns with the expected behavior discussed in the PR:
🏁 Script executed:
#!/bin/bash # Verify if there are any other conditions to show/hide the upgrade button # for different types of subscriptions or user roles rg -A 10 -B 10 "shouldUpgrade|upgrade button|show.*button" --type tsx --type tsLength of output: 109
Action: Verify subscription-status conditions for Upgrade display
Your implementation currently shows the Upgrade button only when
shouldUpgradeis true, which meets the >90% usage criteria. However, to fully align with the PR discussion (chronark), please verify that all subscription-related conditions (such as checking for specific user roles or subscription plans) are properly handled whereshouldUpgradeis determined.
- Confirm that
shouldUpgradealready incorporates any additional logic regarding the user's subscription status.- If not, consider extending its logic to conditionally display the Upgrade button only when the user's subscription also warrants it.
To help with this, please run the following shell script to search for related conditions in TS and TSX files:
#!/bin/bash rg -g "*.tsx" -g "*.ts" -A 10 -B 10 "shouldUpgrade" .
6 participants
What does this PR do?
Fixes #2956
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Upgrade Card Display
Usage Data Accuracy
Upgrade Flow
Edge Cases
Performance & UX
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Recording.2025-03-12.223019.mp4
Summary by CodeRabbit
ProgressCirclecomponent with customizable color options.