-
Notifications
You must be signed in to change notification settings - Fork 630
fix: Format pagination numbers in log footers #3969
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 20 commits
adc1ada
a9b6ccf
47e0d42
088f9a3
3ed115d
a1150db
529fcdb
89f2bfb
f392003
4203d27
bd0bf96
ecf5a04
c8a35f0
0769a91
4fe2d24
a01a4c2
b8ebdd8
e2d9de7
9e3b50e
433b53c
7cb8de3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| "use client"; | ||
| import { VirtualTable } from "@/components/virtual-table/index"; | ||
| import type { Column } from "@/components/virtual-table/types"; | ||
| import { formatNumberFull } from "@/lib/fmt"; | ||
| import { shortenId } from "@/lib/shorten-id"; | ||
| import type { KeyDetails } from "@/lib/trpc/routers/api/keys/query-api-keys/schema"; | ||
| import { BookBookmark, Dots, Focus, Key } from "@unkey/icons"; | ||
|
|
@@ -25,10 +26,20 @@ import { StatusDisplay } from "./components/status-cell"; | |
| import { useKeysListQuery } from "./hooks/use-keys-list-query"; | ||
| import { getRowClassName } from "./utils/get-row-class"; | ||
|
|
||
| const SKELETON_COLUMN_KEYS = [ | ||
| "select", | ||
| "key", | ||
| "value", | ||
| "usage", | ||
| "last_used", | ||
| "status", | ||
| "action", | ||
| ] as const; | ||
|
|
||
| const KeysTableActionPopover = dynamic( | ||
| () => | ||
| import("./components/actions/keys-table-action.popover.constants").then( | ||
| (mod) => mod.KeysTableActions, | ||
| (mod) => mod.KeysTableActions | ||
| ), | ||
| { | ||
| ssr: false, | ||
|
|
@@ -37,13 +48,16 @@ const KeysTableActionPopover = dynamic( | |
| type="button" | ||
| className={cn( | ||
| "group-data-[state=open]:bg-gray-6 group-hover:bg-gray-6 group size-5 p-0 rounded m-0 items-center flex justify-center", | ||
| "border border-gray-6 group-hover:border-gray-8 ring-2 ring-transparent focus-visible:ring-gray-7 focus-visible:border-gray-7", | ||
| "border border-gray-6 group-hover:border-gray-8 ring-2 ring-transparent focus-visible:ring-gray-7 focus-visible:border-gray-7" | ||
| )} | ||
| > | ||
| <Dots className="group-hover:text-gray-12 text-gray-11" size="sm-regular" /> | ||
| <Dots | ||
| className="group-hover:text-gray-12 text-gray-11" | ||
| size="sm-regular" | ||
| /> | ||
| </button> | ||
| ), | ||
| }, | ||
| } | ||
| ); | ||
|
|
||
| export const KeysList = ({ | ||
|
|
@@ -53,9 +67,10 @@ export const KeysList = ({ | |
| keyspaceId: string; | ||
| apiId: string; | ||
| }) => { | ||
| const { keys, isLoading, isLoadingMore, loadMore, totalCount, hasMore } = useKeysListQuery({ | ||
| keyAuthId: keyspaceId, | ||
| }); | ||
| const { keys, isLoading, isLoadingMore, loadMore, totalCount, hasMore } = | ||
| useKeysListQuery({ | ||
| keyAuthId: keyspaceId, | ||
| }); | ||
| const [selectedKey, setSelectedKey] = useState<KeyDetails | null>(null); | ||
| const [navigatingKeyId, setNavigatingKeyId] = useState<string | null>(null); | ||
| // Add state for selected keys | ||
|
|
@@ -98,13 +113,17 @@ export const KeysList = ({ | |
| className={cn( | ||
| "size-5 rounded flex items-center justify-center cursor-pointer", | ||
| identity ? "bg-successA-3" : "bg-grayA-3", | ||
| isSelected && "bg-brand-5", | ||
| isSelected && "bg-brand-5" | ||
| )} | ||
| onMouseEnter={() => setHoveredKeyId(key.id)} | ||
| onMouseLeave={() => setHoveredKeyId(null)} | ||
| > | ||
| {isNavigating ? ( | ||
| <div className={cn(identity ? "text-successA-11" : "text-grayA-11")}> | ||
| <div | ||
| className={cn( | ||
| identity ? "text-successA-11" : "text-grayA-11" | ||
| )} | ||
| > | ||
| <Loading size={18} /> | ||
| </div> | ||
| ) : ( | ||
|
|
@@ -155,17 +174,24 @@ export const KeysList = ({ | |
| rel="noopener noreferrer" | ||
| aria-disabled={isNavigating} | ||
| > | ||
| <span className="font-mono bg-gray-4 p-1 rounded">{identity}</span> | ||
| <span className="font-mono bg-gray-4 p-1 rounded"> | ||
| {identity} | ||
| </span> | ||
| </Link> | ||
| ) : ( | ||
| <span className="font-mono bg-gray-4 p-1 rounded">{identity}</span> | ||
| <span className="font-mono bg-gray-4 p-1 rounded"> | ||
| {identity} | ||
| </span> | ||
| )} | ||
|
Comment on lines
+177
to
185
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Make the tooltip trigger focusable for keyboard users iconContainer is a div; with asChild, the trigger won’t be focusable. Add tabIndex and an aria-label for a11y. - {React.cloneElement(iconContainer, {
- className: cn(
- iconContainer.props.className,
- "cursor-pointer"
- ),
- })}
+ {React.cloneElement(iconContainer, {
+ className: cn(iconContainer.props.className, "cursor-pointer"),
+ tabIndex: 0,
+ role: "button",
+ "aria-label": "Key identity info",
+ })}Also applies to: 179-184 🤖 Prompt for AI Agents |
||
| </> | ||
| } | ||
| asChild | ||
| > | ||
| {React.cloneElement(iconContainer, { | ||
| className: cn(iconContainer.props.className, "cursor-pointer"), | ||
| className: cn( | ||
| iconContainer.props.className, | ||
| "cursor-pointer" | ||
| ), | ||
| })} | ||
| </InfoTooltip> | ||
| ) : ( | ||
|
|
@@ -205,7 +231,11 @@ export const KeysList = ({ | |
| header: "Value", | ||
| width: "15%", | ||
| render: (key) => ( | ||
| <HiddenValueCell value={key.start} title="Value" selected={selectedKey?.id === key.id} /> | ||
| <HiddenValueCell | ||
| value={key.start} | ||
| title="Value" | ||
| selected={selectedKey?.id === key.id} | ||
| /> | ||
| ), | ||
| }, | ||
| { | ||
|
|
@@ -266,7 +296,7 @@ export const KeysList = ({ | |
| selectedKeys, | ||
| toggleSelection, | ||
| hoveredKeyId, | ||
| ], | ||
| ] | ||
| ); | ||
|
|
||
| const getSelectedKeysState = useCallback(() => { | ||
|
|
@@ -291,17 +321,15 @@ export const KeysList = ({ | |
|
|
||
| // Early exit if we already found a mix | ||
| if (!allEnabled && !allDisabled) { | ||
| break; | ||
| return "mixed"; | ||
| } | ||
| } | ||
|
|
||
| if (allEnabled) { | ||
| return "all-enabled"; | ||
| } | ||
| if (allDisabled) { | ||
| return "all-disabled"; | ||
| } | ||
| return "mixed"; | ||
|
|
||
| return "all-disabled"; | ||
| }, [selectedKeys, keys]); | ||
|
|
||
| return ( | ||
|
|
@@ -330,9 +358,12 @@ export const KeysList = ({ | |
| ), | ||
| countInfoText: ( | ||
| <div className="flex gap-2"> | ||
| <span>Showing</span> <span className="text-accent-12">{keys.length}</span> | ||
| <span>Showing</span>{" "} | ||
| <span className="text-accent-12"> | ||
| {formatNumberFull(keys.length)} | ||
| </span> | ||
| <span>of</span> | ||
| {totalCount} | ||
| {formatNumberFull(totalCount)} | ||
| <span>keys</span> | ||
| </div> | ||
|
Comment on lines
+361
to
368
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Reuse a memoized number formatter Apply the shared formatter to keep consistency with the rest of the PR. - <span className="text-accent-12">
- {new Intl.NumberFormat().format(keys.length)}
- </span>
+ <span className="text-accent-12">{nf.format(keys.length)}</span>
...
- {new Intl.NumberFormat().format(totalCount)}
+ {nf.format(totalCount)}Support (outside selected lines): import React, { useCallback, useMemo, useState } from "react";
const nf = useMemo(() => new Intl.NumberFormat(), []);🤖 Prompt for AI Agents |
||
| ), | ||
|
|
@@ -343,8 +374,8 @@ export const KeysList = ({ | |
| <Empty.Icon className="w-auto" /> | ||
| <Empty.Title>No API Keys Found</Empty.Title> | ||
| <Empty.Description className="text-left"> | ||
| There are no API keys associated with this service yet. Create your first API key to | ||
| get started. | ||
| There are no API keys associated with this service yet. Create | ||
| your first API key to get started. | ||
| </Empty.Description> | ||
| <Empty.Actions className="mt-4 justify-start"> | ||
| <a | ||
|
|
@@ -374,7 +405,7 @@ export const KeysList = ({ | |
| className={cn( | ||
| "text-xs align-middle whitespace-nowrap pr-4", | ||
| idx === 0 ? "pl-[18px]" : "", | ||
| column.key === "key" ? "py-[6px]" : "py-1", | ||
| column.key === "key" ? "py-[6px]" : "py-1" | ||
| )} | ||
| style={{ height: `${rowHeight}px` }} | ||
| > | ||
|
|
@@ -384,9 +415,11 @@ export const KeysList = ({ | |
| {column.key === "last_used" && <LastUsedColumnSkeleton />} | ||
| {column.key === "status" && <StatusColumnSkeleton />} | ||
| {column.key === "action" && <ActionColumnSkeleton />} | ||
| {!["select", "key", "value", "usage", "last_used", "status", "action"].includes( | ||
| column.key, | ||
| ) && <div className="h-4 w-full bg-grayA-3 rounded animate-pulse" />} | ||
| {!SKELETON_COLUMN_KEYS.includes( | ||
| column.key as (typeof SKELETON_COLUMN_KEYS)[number] | ||
| ) && ( | ||
| <div className="h-4 w-full bg-grayA-3 rounded animate-pulse" /> | ||
| )} | ||
| </td> | ||
| )) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2,7 +2,7 @@ | |||||
| import { VirtualTable } from "@/components/virtual-table/index"; | ||||||
| import type { Column } from "@/components/virtual-table/types"; | ||||||
| import { useIsMobile } from "@/hooks/use-mobile"; | ||||||
| import type { Deployment, Environment } from "@/lib/collections"; | ||||||
|
|
||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing type imports cause compilation error: View Details📝 Patch Detailsdiff --git a/apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/deployments-list.tsx b/apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/deployments-list.tsx
index afe222b25..9876d5843 100644
--- a/apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/deployments-list.tsx
+++ b/apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/deployments-list.tsx
@@ -2,6 +2,7 @@
import { VirtualTable } from "@/components/virtual-table/index";
import type { Column } from "@/components/virtual-table/types";
import { useIsMobile } from "@/hooks/use-mobile";
+import type { Deployment, Environment } from "@/lib/collections";
import { shortenId } from "@/lib/shorten-id";
import { BookBookmark, Cloud, CodeBranch, Cube } from "@unkey/icons";
AnalysisMissing type imports cause TypeScript compilation errors in deployments-list.tsxWhat fails: TypeScript compilation fails with "Cannot find name 'Deployment'" and "Cannot find name 'Environment'" errors in How to reproduce: cd apps/dashboard
pnpm exec tsc --noEmit --isolatedModules --jsx react-jsx app/\(app\)/projects/\[projectId\]/deployments/components/table/deployments-list.tsxResult: TypeScript errors: Expected: Code should compile without type errors since the types are defined and exported from Fix: Added |
||||||
| import { shortenId } from "@/lib/shorten-id"; | ||||||
| import { BookBookmark, Cloud, CodeBranch, Cube } from "@unkey/icons"; | ||||||
| import { Button, Empty, TimestampInfo } from "@unkey/ui"; | ||||||
|
|
@@ -307,7 +307,7 @@ export const DeploymentsList = () => { | |||||
| onRowClick={setSelectedDeployment} | ||||||
| selectedItem={selectedDeployment} | ||||||
| keyExtractor={(deployment) => deployment.id} | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix keyExtractor to use row.deployment.id VirtualTable rows appear to be shaped as { deployment, environment }. The current extractor uses the row itself as a Deployment, which likely produces undefined/duplicate keys. - keyExtractor={(deployment) => deployment.id}
+ keyExtractor={(row) => row.deployment.id}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||
| rowClassName={(deployment) => getRowClassName(deployment, selectedDeployment?.deployment.id)} | ||||||
|
|
||||||
| emptyState={ | ||||||
| <div className="w-full flex justify-center items-center h-full"> | ||||||
| <Empty className="w-[400px] flex items-start"> | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,4 @@ | ||
| import { collection } from "@/lib/collections"; | ||
| import { ilike, useLiveQuery } from "@tanstack/react-db"; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing imports cause compilation error: View Details📝 Patch Detailsdiff --git a/apps/dashboard/app/(app)/projects/_components/list/index.tsx b/apps/dashboard/app/(app)/projects/_components/list/index.tsx
index b0d569ece..e2d6fe4ea 100644
--- a/apps/dashboard/app/(app)/projects/_components/list/index.tsx
+++ b/apps/dashboard/app/(app)/projects/_components/list/index.tsx
@@ -1,6 +1,8 @@
import { BookBookmark, Dots } from "@unkey/icons";
import { Button, Empty } from "@unkey/ui";
+import { ilike, useLiveQuery } from "@tanstack/react-db";
+import { collection } from "@/lib/collections";
import { useProjectsFilters } from "../hooks/use-projects-filters";
import { ProjectActions } from "./project-actions";
import { ProjectCard } from "./projects-card";
@@ -101,6 +103,6 @@ export const ProjectsList = () => {
/>
))}
</div>
-
+ </div>
);
};
AnalysisMissing imports cause TypeScript compilation errors in ProjectsList componentWhat fails: ProjectsList component in How to reproduce: cd apps/dashboard && pnpm run typecheckResult: TypeScript compiler reports: Expected: Clean compilation with proper imports from |
||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Critical syntax errors: Missing required imports for View Details📝 Patch Detailsdiff --git a/apps/dashboard/app/(app)/projects/_components/list/index.tsx b/apps/dashboard/app/(app)/projects/_components/list/index.tsx
index b0d569ece..e2d6fe4ea 100644
--- a/apps/dashboard/app/(app)/projects/_components/list/index.tsx
+++ b/apps/dashboard/app/(app)/projects/_components/list/index.tsx
@@ -1,6 +1,8 @@
import { BookBookmark, Dots } from "@unkey/icons";
import { Button, Empty } from "@unkey/ui";
+import { ilike, useLiveQuery } from "@tanstack/react-db";
+import { collection } from "@/lib/collections";
import { useProjectsFilters } from "../hooks/use-projects-filters";
import { ProjectActions } from "./project-actions";
import { ProjectCard } from "./projects-card";
@@ -101,6 +103,6 @@ export const ProjectsList = () => {
/>
))}
</div>
-
+ </div>
);
};
AnalysisCritical syntax errors in ProjectsList component: Missing imports and unclosed JSX tagWhat fails: File How to reproduce: cd apps/dashboard && pnpm run typecheckResult: TypeScript compiler fails with errors:
Expected: File should compile without TypeScript errors Fix applied:
|
||
| import { BookBookmark, Dots } from "@unkey/icons"; | ||
| import { Button, Empty } from "@unkey/ui"; | ||
| import { useProjectsFilters } from "../hooks/use-projects-filters"; | ||
|
|
@@ -102,6 +101,6 @@ export const ProjectsList = () => { | |
| /> | ||
| ))} | ||
| </div> | ||
| </div> | ||
|
|
||
| ); | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| }; | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3,7 +3,7 @@ | |||||||||||||||||||||||
| import { useSort } from "@/components/logs/hooks/use-sort"; | ||||||||||||||||||||||||
| import { VirtualTable } from "@/components/virtual-table/index"; | ||||||||||||||||||||||||
| import type { Column } from "@/components/virtual-table/types"; | ||||||||||||||||||||||||
| import { formatNumber } from "@/lib/fmt"; | ||||||||||||||||||||||||
| import { formatNumber, formatNumberFull } from "@/lib/fmt"; | ||||||||||||||||||||||||
| import { cn } from "@/lib/utils"; | ||||||||||||||||||||||||
| import type { RatelimitOverviewLog } from "@unkey/clickhouse/src/ratelimits"; | ||||||||||||||||||||||||
| import { Ban, BookBookmark } from "@unkey/icons"; | ||||||||||||||||||||||||
|
|
@@ -218,8 +218,9 @@ export const RatelimitOverviewLogsTable = ({ | |||||||||||||||||||||||
| hide: isLoading, | ||||||||||||||||||||||||
| countInfoText: ( | ||||||||||||||||||||||||
| <div className="flex gap-2"> | ||||||||||||||||||||||||
| <span>Showing</span> <span className="text-accent-12">{historicalLogs.length}</span> | ||||||||||||||||||||||||
| <span>of {totalCount}</span> | ||||||||||||||||||||||||
| <span>Showing</span>{" "} | ||||||||||||||||||||||||
| <span className="text-accent-12">{formatNumberFull(historicalLogs.length)}</span> | ||||||||||||||||||||||||
| <span>of {formatNumberFull(totalCount)}</span> | ||||||||||||||||||||||||
| <span>rate limit identifiers</span> | ||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||
|
Comment on lines
+221
to
225
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Use shared formatter for footer counts You already import - <span>Showing</span>{" "}
- <span className="text-accent-12">
- {new Intl.NumberFormat().format(historicalLogs.length)}
- </span>
- <span>of {new Intl.NumberFormat().format(totalCount)}</span>
+ <span>Showing</span> <span className="text-accent-12">{formatNumber(historicalLogs.length)}</span>
+ <span>of {formatNumber(totalCount)}</span>📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |||||||||||||||||||||||||||
| import { safeParseJson } from "@/app/(app)/logs/utils"; | ||||||||||||||||||||||||||||
| import { VirtualTable } from "@/components/virtual-table/index"; | ||||||||||||||||||||||||||||
| import type { Column } from "@/components/virtual-table/types"; | ||||||||||||||||||||||||||||
| import { formatNumberFull } from "@/lib/fmt"; | ||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't force en-US; honor user locale and cache formatter Import looks good, but current formatNumberFull uses Intl.NumberFormat("en-US"), which conflicts with the PR goal of “locale-appropriate” formatting and allocates a formatter per call. Switch the helper to default-locale (or param) and cache per locale. Support (outside selected lines, apps/dashboard/lib/fmt.ts): -export function formatNumberFull(n: number): string {
- return Intl.NumberFormat("en-US").format(n);
-}
+const nfCache = new Map<string | undefined, Intl.NumberFormat>();
+function getNF(locale?: string) {
+ if (!nfCache.has(locale)) nfCache.set(locale, new Intl.NumberFormat(locale));
+ return nfCache.get(locale)!;
+}
+export function formatNumberFull(n: number, locale?: string): string {
+ return getNF(locale).format(n);
+}📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||
| import { cn } from "@/lib/utils"; | ||||||||||||||||||||||||||||
| import type { RatelimitLog } from "@unkey/clickhouse/src/ratelimits"; | ||||||||||||||||||||||||||||
| import { BookBookmark } from "@unkey/icons"; | ||||||||||||||||||||||||||||
|
|
@@ -220,9 +221,10 @@ export const RatelimitLogsTable = () => { | |||||||||||||||||||||||||||
| hide: isLoading, | ||||||||||||||||||||||||||||
| countInfoText: ( | ||||||||||||||||||||||||||||
| <div className="flex gap-2"> | ||||||||||||||||||||||||||||
| <span>Showing</span> <span className="text-accent-12">{historicalLogs.length}</span> | ||||||||||||||||||||||||||||
| <span>Showing</span>{" "} | ||||||||||||||||||||||||||||
| <span className="text-accent-12">{formatNumberFull(historicalLogs.length)}</span> | ||||||||||||||||||||||||||||
| <span>of</span> | ||||||||||||||||||||||||||||
| {totalCount} | ||||||||||||||||||||||||||||
| {formatNumberFull(totalCount)} | ||||||||||||||||||||||||||||
| <span>ratelimit requests</span> | ||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||
|
Comment on lines
+224
to
229
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Reuse a memoized number formatter Apply the shared formatter here too. - <span className="text-accent-12">
- {new Intl.NumberFormat().format(historicalLogs.length)}
- </span>
+ <span className="text-accent-12">{nf.format(historicalLogs.length)}</span>
...
- {new Intl.NumberFormat().format(totalCount)}
+ {nf.format(totalCount)}Support (outside selected lines): import { useMemo } from "react";
const nf = useMemo(() => new Intl.NumberFormat(), []);🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.