fix: Improve the arrangement of metrics in mobile view in ratelimit overview page #2732
Conversation
…unkey into responsiveness
BadriVishalPadhy
requested review from
perkinsjr,
chronark,
mcstepp and
ogzhanolguncu
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@BadriVishalPadhy is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
bot
added
Feature
|
Thank you for following the naming conventions for pull request titles! 🙏 |
|
@ogzhanolguncu check plz |
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request primarily focus on the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 1
🧹 Outside diff range and nitpick comments (3)
apps/dashboard/components/timestamp-info 2.tsx (1)
50-65: Consider debouncing the scroll and resize event listenersThe current implementation might cause performance issues on mobile devices as scroll and resize events can fire frequently.
useEffect(() => { + const debounce = (fn: Function, ms = 100) => { + let timeoutId: ReturnType<typeof setTimeout>; + return function (this: any, ...args: any[]) { + clearTimeout(timeoutId); + timeoutId = setTimeout(() => fn.apply(this, args), ms); + }; + }; + + const debouncedUpdateAlignment = debounce(updateAlignment); const updateAlignment = () => { if (triggerRef.current) { const rect = triggerRef.current.getBoundingClientRect(); const windowHeight = window.innerHeight; setAlign(rect.top < windowHeight / 2 ? "start" : "end"); } }; updateAlignment(); - window.addEventListener("scroll", updateAlignment); - window.addEventListener("resize", updateAlignment); + window.addEventListener("scroll", debouncedUpdateAlignment); + window.addEventListener("resize", debouncedUpdateAlignment); return () => { - window.removeEventListener("scroll", updateAlignment); - window.removeEventListener("resize", updateAlignment); + window.removeEventListener("scroll", debouncedUpdateAlignment); + window.removeEventListener("resize", debouncedUpdateAlignment); }; }, []);apps/dashboard/app/(app)/ratelimits/[namespaceId]/page.tsx (2)
128-128: Remove unnecessary double spaces in className stringsThere are extra spaces in the className strings which should be removed for consistency.
- <div className="flex flex-col gap-4"> + <div className="flex flex-col gap-4"> <Card> - <CardContent className="grid sm:grid-cols-3 sm:divide-x"> + <CardContent className="grid sm:grid-cols-3 sm:divide-x">Also applies to: 130-130
Line range hint
130-143: Consider adding explicit mobile layout classesWhile the layout switches to a single column on mobile by default, consider adding explicit mobile-first classes for better clarity.
- <CardContent className="grid sm:grid-cols-3 sm:divide-x"> + <CardContent className="grid grid-cols-1 gap-4 sm:gap-0 sm:grid-cols-3 sm:divide-x">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/page.tsx(1 hunks)apps/dashboard/components/timestamp-info 2.tsx(1 hunks)
🔇 Additional comments (2)
apps/dashboard/components/timestamp-info 2.tsx (1)
1-1: Consider adding proper attribution and license information
The comment indicates this file is based on Supabase's implementation. Ensure proper attribution and license compliance are maintained.
apps/dashboard/app/(app)/ratelimits/[namespaceId]/page.tsx (1)
Line range hint 128-143: Verify mobile responsiveness across different screen sizes
The layout changes look good, but we should verify the responsiveness across various mobile devices and screen sizes.
✅ Verification successful
Let me analyze the code further to verify the responsive design implementation.
Responsive design implementation verified and follows best practices
The implementation demonstrates proper responsive design patterns:
- Uses
sm:grid-cols-3with a proper fallback for mobile (single column) - Implements responsive text sizing (
text-xs sm:text-xxs) - Handles spacing responsively (
gap-0 sm:gap-8) - Includes mobile-specific display logic (
hidden sm:block) - Follows dashboard-wide grid pattern conventions
- Uses flexible layouts with
flexandflex-colfor adaptable content
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any responsive design test configurations
fd -e spec.tsx -e spec.ts -e test.tsx -e test.ts | xargs rg -l "viewport|screen size|mobile|responsive"
# Look for existing responsive design patterns
rg -l "sm:|md:|lg:|xl:|2xl:"
Length of output: 6046
Script:
#!/bin/bash
# Check the specific file for responsive design patterns and grid implementation
rg -A 5 "sm:|md:|lg:|xl:|2xl:" "apps/dashboard/app/(app)/ratelimits/[namespaceId]/page.tsx"
# Look for similar grid patterns in other dashboard pages for consistency
rg "grid sm:grid-cols-[23]" "apps/dashboard/app/(app)/"
# Check for any responsive design utilities or components used
rg "className=\".*flex.*\"" "apps/dashboard/app/(app)/ratelimits/[namespaceId]/page.tsx"
Length of output: 1702
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
apps/dashboard/components/timestamp-info 2.tsx (1)
50-65: Optimize scroll and resize event listenersThe current implementation might cause unnecessary re-renders on frequent scroll/resize events.
Consider debouncing the event handlers:
+ import debounce from 'lodash/debounce'; useEffect(() => { - const updateAlignment = () => { + const updateAlignment = debounce(() => { if (triggerRef.current) { const rect = triggerRef.current.getBoundingClientRect(); const windowHeight = window.innerHeight; setAlign(rect.top < windowHeight / 2 ? "start" : "end"); } - }; + }, 150); updateAlignment(); window.addEventListener("scroll", updateAlignment); window.addEventListener("resize", updateAlignment); return () => { window.removeEventListener("scroll", updateAlignment); window.removeEventListener("resize", updateAlignment); + updateAlignment.cancel(); }; }, []);apps/dashboard/app/(app)/ratelimits/[namespaceId]/page.tsx (1)
130-130: Remove extra whitespace in classNameThere are multiple consecutive spaces in the className string.
- <CardContent className="grid sm:grid-cols-3 sm:divide-x"> + <CardContent className="grid sm:grid-cols-3 sm:divide-x">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/page.tsx(1 hunks)apps/dashboard/components/timestamp-info 2.tsx(1 hunks)
🔇 Additional comments (1)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/page.tsx (1)
128-130: Verify mobile layout improvements
The grid layout changes look good for improving mobile responsiveness. The sm:grid-cols-3 ensures single-column layout on mobile and three columns on larger screens.
✅ Verification successful
Let me gather more context about the grid layout patterns to ensure consistency.
Mobile layout implementation is consistent with app patterns
The grid layout implementation in the ratelimits page follows the established responsive design patterns across the app:
- Uses
flex flex-col gap-4for vertical stacking of components - Implements
grid sm:grid-cols-3withsm:divide-xfor metric cards, matching the pattern used in other dashboard pages like the API keys view - Properly handles the mobile-first approach by defaulting to single column and expanding to 3 columns on larger screens
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent responsive design patterns across the app
rg -l 'sm:grid-cols-[0-9]' | xargs rg 'flex flex-col.*gap-4'
Length of output: 375
Script:
#!/bin/bash
# Check the grid layout implementation in the files that use sm:grid-cols
rg -l 'sm:grid-cols-[0-9]' | xargs rg 'grid.*sm:grid-cols|sm:grid-cols.*grid' -B 2 -A 2
Length of output: 2952
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
apps/dashboard/components/timestamp-info 2.tsx (1)
9-17: Consider adding input validation and error handlingThe timestamp conversion functions could benefit from additional error handling:
- Validate input ranges
- Handle invalid inputs gracefully
const unixMicroToDate = (unix: string | number): Date => { + if (!isUnixMicro(unix)) { + throw new Error('Invalid Unix microsecond timestamp'); + } return fromUnixTime(Number(unix) / 1000 / 1000); }; const isUnixMicro = (unix: string | number): boolean => { + if (unix === null || unix === undefined) { + return false; + } const digitLength = String(unix).length === 16; const isNum = !Number.isNaN(Number(unix)); return isNum && digitLength; };apps/dashboard/app/(app)/ratelimits/[namespaceId]/page.tsx (2)
128-130: Clean up unnecessary whitespace in className stringsThere are extra spaces in the className strings that should be removed.
- <div className="flex flex-col gap-4"> + <div className="flex flex-col gap-4"> <Card> - <CardContent className="grid sm:grid-cols-3 sm:divide-x"> + <CardContent className="grid sm:grid-cols-3 sm:divide-x">
130-130: Enhance mobile responsivenessWhile the changes improve mobile layout, consider these additional improvements:
- Add padding for better touch targets on mobile
- Consider adjusting text sizes for better readability
- <CardContent className="grid sm:grid-cols-3 sm:divide-x"> + <CardContent className="grid gap-4 sm:gap-0 sm:grid-cols-3 sm:divide-x p-4"> - <Code className="flex items-start gap-0 sm:gap-8 p-4 my-8 text-xs sm:text-xxs text-start overflow-x-auto max-w-full"> + <Code className="flex items-start gap-0 sm:gap-8 p-4 my-8 text-sm sm:text-xs text-start overflow-x-auto max-w-full">Also applies to: 191-192
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/page.tsx(1 hunks)apps/dashboard/components/timestamp-info 2.tsx(1 hunks)
|
Also, let's add horizontal dividers similar to what we use in vertical design |
ogzhanolguncu
added
Improvement
|
@ogzhanolguncu hey can i ask u something |
|
Sure |
|
@ogzhanolguncu can i work as intern at unkey |
I Appreciate your interest in interning at Unkey. We currently don't offer internships as we are a small team and don't have the infrastructure to offer it at this time. |
|
thnks @perkinsjr if there will be any job roles or intern open please do consider me |
What does this PR do?
Vertically arranged the metrics in mobile view in ratelimit->overview page
Fixes #2731
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
TimestampInfocomponent for displaying formatted timestamps (local, UTC, and relative).RatelimitNamespacePagecomponent.Bug Fixes