fix(dashboard): "No bucket found" message position on Audit Logs page #2413
Conversation
…atting Replace `toLocaleDateString()` with `date-fns` to ensure consistent date formatting between server and client. This prevents hydration mismatches caused by differing time zones or locale settings, ensuring stable rendering across environments.
unrenamed
requested review from
perkinsjr,
chronark and
mcstepp
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@unrenamed is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request implements changes across several components related to the glossary and audit logs. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
unrenamed
changed the title
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- apps/dashboard/app/(app)/audit/[bucket]/page.tsx (3 hunks)
- apps/dashboard/app/(app)/audit/[bucket]/row.tsx (2 hunks)
🧰 Additional context used
🔇 Additional comments (5)
apps/dashboard/app/(app)/audit/[bucket]/row.tsx (2)
8-8: LGTM: Good choice of date formatting library.The addition of the
formatfunction fromdate-fnsis a good choice. This library provides consistent date formatting across different environments, which aligns well with the PR objective of resolving hydration errors related to date formatting.
89-92:⚠️ Potential issueConsider preserving time information in the display.
The change to use
date-fnsfor formatting resolves the hydration error, which is great. However, the current implementation uses the same format ('dd/MM/yyyy') for both date and time, resulting in a loss of time information.Consider modifying the time format to include hours and minutes:
- {format(new Date(auditLog.time), 'dd/MM/yyyy')} + {format(new Date(auditLog.time), 'dd/MM/yyyy HH:mm')}This change would preserve the time information while maintaining consistency across server and client rendering.
To ensure this change doesn't conflict with any existing time display logic, let's verify the usage:
✅ Verification successful
Date Formatting Change Verified Successfully
The modification to include time information in the date format (
'dd/MM/yyyy HH:mm') inrow.tsxdoes not conflict with other date formatting usages in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other occurrences of date formatting in the codebase # Test: Search for date formatting patterns rg --type typescript -e "format\(.*?new Date" -e "toLocaleString" -e "toLocaleDateString" -e "toLocaleTimeString"Length of output: 144
Script:
#!/bin/bash # Description: Check for other occurrences of date formatting in TypeScript files # Test: Search for date formatting patterns in .ts and .tsx files rg --glob '*.{ts,tsx}' -e "format\(.*?new Date" -e "toLocaleString" -e "toLocaleDateString" -e "toLocaleTimeString"Length of output: 4492
apps/dashboard/app/(app)/audit/[bucket]/page.tsx (3)
46-46: Filtering out deleted rate limit namespacesThe addition of the
whereclause correctly filters out deleted rate limit namespaces, ensuring only active namespaces are included in the query.
150-159: Appropriate handling of missing buckets with user-friendly placeholderThe updated rendering logic provides a clear and informative message when a bucket is not found or inaccessible. This improves user experience by allowing users to understand the issue without navigating away from the page.
362-367: Correctly filtering out deleted and expired root keysThe
whereclause now properly excludes deleted and expired root keys, ensuring that only active keys are presented to the user for filtering.
…dability The mapping logic within the <AuditLogTable> component is complex due to nested structures. Extracting this logic into a separate function or utility enhances readability and maintainability.
Revert static datetime formatting using `date-fns` to `toLocaleDateString()` to better suit global users. Suppress the hydration mismatch warnings caused by differences between server and client-side rendered dates. Co-authored-by: @chronark PR: unkeyed#2413
|
@chronark Just wanted to quickly check in about my recent PRs. I noticed the review activity has slowed down a bit, and I totally understand if there are other priorities right now or personal things that have come up. I’m just a bit concerned about the upcoming hackathon deadline on October 31 and wanted to make sure the project is still actively participating. If you have time, I’d really appreciate a review before the deadline!
|
…-on-audit-logs-page
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (10)
apps/www/components/glossary/search.tsx (2)
Line range hint
16-20: Consider adding ARIA attributes for better accessibility.The component could benefit from additional accessibility attributes to improve screen reader support.
Consider adding these ARIA attributes:
export function FilterableCommand(props: { placeholder: string; className?: string; terms: Array<Glossary>; }) {// Add these props to the Command component aria-label="Search glossary terms" aria-expanded={open} role="combobox"
Line range hint
31-38: Document the blur behavior more clearly.The current comment explains the blur logic, but it could be more comprehensive to help future maintainers.
Consider expanding the comment:
- // The `onBlur` event checks if the new focus target (relatedTarget) is within the Command component: - // - If it's not (i.e., clicking outside), it closes the list. - // - If it is (i.e., selecting an item), it keeps the list open, allowing the `onSelect` to handle the navigation. + // Focus management for the command list: + // 1. When focus leaves the input field, we check where it's going using `relatedTarget` + // 2. If the new focus target is outside the Command component: + // - We close the list as the user is no longer interacting with the component + // 3. If the new focus target is inside the Command component: + // - We keep the list open to allow item selection + // - The `onSelect` handler will handle navigation when an item is chosen + // This prevents the list from closing prematurely when users try to select itemsapps/www/components/glossary/terms-rolodex-desktop.tsx (2)
Line range hint
63-67: Consider enhancing accessibility for navigation buttons.While the buttons have basic screen reader support, their accessibility could be improved with more descriptive labels.
Consider applying these changes to the scroll buttons:
<Button variant="ghost" size="icon" onClick={() => handleScroll("up")} className={cn("p-0 transition-colors duration-150", { "cursor-not-allowed": terms.length <= visibleTerms.length, })} disabled={terms.length <= visibleTerms.length} + aria-label={`Scroll up through glossary terms. ${terms.length <= visibleTerms.length ? 'Disabled when all terms are visible' : ''}`} > <ChevronUpIcon className="w-4 h-4" /> - <span className="sr-only">Scroll up</span> </Button>Similar changes should be applied to the "down" button as well.
Also applies to: 89-93
Line range hint
29-39: Consider optimizing the terms wrapping logic.The current implementation creates multiple copies of the terms array, which could be memory intensive for large lists. Consider using modulo arithmetic instead.
Here's a more efficient implementation:
const getVisibleTerms = () => { const totalVisible = Math.min(7, terms.length); const halfVisible = Math.floor(totalVisible / 2); - const start = currentIndex - halfVisible; - const end = currentIndex + halfVisible + 1; - - const wrappedTerms = [...terms, ...terms, ...terms]; - const centerOffset = terms.length; - - return wrappedTerms.slice(centerOffset + start, centerOffset + end); + return Array.from({ length: totalVisible }, (_, i) => { + const offset = i - halfVisible; + const index = ((currentIndex + offset) % terms.length + terms.length) % terms.length; + return terms[index]; + }); };This approach:
- Eliminates the need for array spreading
- Uses less memory
- Maintains the same visual behavior
apps/www/app/glossary/client.tsx (3)
Line range hint
89-95: Consider enhancing A-Z navigation accessibility.The A-Z navigation bar could benefit from improved accessibility:
- Add
aria-labelto indicate this is an alphabetical navigation- Consider adding keyboard navigation support
<div className="justify-between flex border-b border-white/10 pb-8 mb-8"> + <nav aria-label="Alphabetical glossary navigation"> {alphabet.map((letter) => groupedTerms[letter]?.length > 0 ? ( - <Link key={letter} href={`#${letter}`} className="rounded hover:underline"> + <Link + key={letter} + href={`#${letter}`} + className="rounded hover:underline" + role="link" + aria-label={`Jump to terms starting with ${letter}`} + > {letter} </Link> ) : ( - <span key={letter} className="rounded text-white/30"> + <span key={letter} className="rounded text-white/30" aria-hidden="true"> {letter} </span> ), )} + </nav> </div>
Line range hint
183-201: Remove commented-out code.The commented-out code at the bottom of the file should be removed as it:
- Adds unnecessary noise to the codebase
- Can be retrieved from version control if needed
- May become outdated and misleading over time
Line range hint
17-26: Consider performance optimizations.The
groupedTermscalculation could be optimized:
- Memoize the grouped terms to prevent unnecessary recalculations
- Consider moving the grouping logic outside the component
+import { useMemo } from "react"; export function GlossaryClient() { const alphabet = "ABCDEFGHIJKLMNOPQRSTUVWXYZ".split(""); - const groupedTerms = allGlossaries.reduce( + const groupedTerms = useMemo(() => allGlossaries.reduce( (acc, term) => { const firstLetter = term.title[0].toUpperCase(); if (!acc[firstLetter]) { acc[firstLetter] = []; } acc[firstLetter].push(term); return acc; }, {} as Record<string, Array<Glossary>>, - ); + ), []);apps/www/app/glossary/[slug]/page.tsx (3)
Line range hint
63-65: Consider initializing relatedTerms based on the current term.The
relatedTermsarray is currently empty but used in the UI. Consider implementing logic to populate it with terms that share similar categories or tags with the current term.Would you like me to help implement the related terms logic?
Line range hint
186-209: Replace hardcoded FAQ with dynamic content.The FAQ section currently contains hardcoded questions about mime types, which may not be relevant for all glossary terms. Consider making the FAQ content dynamic based on the current term's category or topic.
Example approach:
const getFAQForTerm = (term: string, category: string): FAQ[] => { // Return relevant FAQs based on term and category return term.faqs || defaultFAQs[category] || []; };
Line range hint
63-267: Consider adding error boundaries for component resilience.The component handles the
notFound()case but could benefit from error boundaries to gracefully handle runtime errors in the UI components.Example implementation:
import { ErrorBoundary } from '@/components/error-boundary'; // Wrap the main content <ErrorBoundary fallback={<ErrorFallback />}> <div className="prose sm:prose-sm md:prose-md sm:mx-6"> {/* ... existing content ... */} </div> </ErrorBoundary>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- apps/dashboard/app/(app)/audit/[bucket]/row.tsx (1 hunks)
- apps/www/app/glossary/[slug]/page.tsx (1 hunks)
- apps/www/app/glossary/client.tsx (1 hunks)
- apps/www/components/glossary/search.tsx (2 hunks)
- apps/www/components/glossary/terms-rolodex-desktop.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/dashboard/app/(app)/audit/[bucket]/row.tsx
🧰 Additional context used
📓 Learnings (1)
apps/www/components/glossary/search.tsx (1)
Learnt from: p6l-richard PR: unkeyed/unkey#2085 File: apps/www/components/glossary/search.tsx:16-20 Timestamp: 2024-10-23T16:21:47.395Z Learning: For the `FilterableCommand` component in `apps/www/components/glossary/search.tsx`, refactoring type definitions into an interface is not necessary at this time.
🔇 Additional comments (4)
apps/www/components/glossary/search.tsx (1)
Line range hint
3-14: LGTM! Clean import organization.The imports are well-organized, following a logical pattern: types first, followed by components and utilities.
apps/www/components/glossary/terms-rolodex-desktop.tsx (1)
3-9: LGTM! Import organization follows a logical pattern.The imports are now organized in a clear, logical order: types, components, utilities, icons, routing, and hooks.
Let's verify the integration with FilterableCommand as mentioned in the AI summary:
✅ Verification successful
Import organization is correct and components are properly integrated
The import organization in
terms-rolodex-desktop.tsxfollows a logical pattern, and the component is correctly integrated withFilterableCommandin the glossary page layout. Both components work together in the left sidebar where:
FilterableCommandprovides the search functionalityTermsRolodexDesktopdisplays the scrollable list of terms below it🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of TermsRolodexDesktop with FilterableCommand # Expected: Find where these components are used together in the glossary page # Search for files that import both components rg -l "TermsRolodexDesktop|FilterableCommand" | sort | uniq -d # Show the context of how they're used together ast-grep --pattern 'return ( $$$ <$_> $$$ <TermsRolodexDesktop $$$/> $$$ <FilterableCommand $$$/> $$$ </$_> )'Length of output: 203
Script:
#!/bin/bash # Let's try a different approach to verify the integration # First, find all files that mention TermsRolodexDesktop echo "=== Files containing TermsRolodexDesktop ===" rg --type tsx --type jsx "TermsRolodexDesktop" # Then, find all files that mention FilterableCommand echo -e "\n=== Files containing FilterableCommand ===" rg --type tsx --type jsx "FilterableCommand" # Look for the actual component definition to understand its usage echo -e "\n=== TermsRolodexDesktop component definition ===" rg -A 5 "export (default )?function TermsRolodexDesktop" --type tsx # Check if there are any recent changes related to glossary components echo -e "\n=== Recent changes in glossary components ===" fd . "apps/www/components/glossary" -t f -x ls -l {}Length of output: 1016
Script:
#!/bin/bash # Let's try without file type flags since tsx isn't recognized # Search for TermsRolodexDesktop usage echo "=== Files containing TermsRolodexDesktop ===" rg "TermsRolodexDesktop" # Search for FilterableCommand echo -e "\n=== Files containing FilterableCommand ===" rg "FilterableCommand" # Look at the content of the search component which might integrate these echo -e "\n=== Content of search.tsx ===" cat "apps/www/components/glossary/search.tsx"Length of output: 3121
Script:
#!/bin/bash # Let's look at the glossary page to see how these components are used together # Check the page where both components are imported echo "=== Content of glossary/[slug]/page.tsx ===" cat "apps/www/app/glossary/[slug]/page.tsx"Length of output: 11903
apps/www/app/glossary/client.tsx (1)
5-5: Import reordering looks good.Also applies to: 12-12
apps/www/app/glossary/[slug]/page.tsx (1)
4-5: LGTM: Import changes are well-organized.The new imports for
FilterableCommandandTermsRolodexDesktopare properly structured and follow Next.js conventions.
|
Awarding unrenamed: 150 points 🕹️ Well done! Check out your new contribution on oss.gg/unrenamed |
What does this PR do?
This PR includes several fixes to improve the Audit Logs section of the dashboard:
toLocaleDateString()withdate-fnsto ensure consistent date formatting between server and client, preventing rendering issues caused by time zone or locale differences.Fixes #2220
Type of change
How should this be tested?
devmode, open DevTools, enable console errors and go to Audit Logs page.Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
Summary by CodeRabbit