feat: add pagination to item endpoint by using fastapi-pagination #89
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideIntegrates fastapi-pagination into the backend item endpoint and extends the Next.js frontend with dynamic page & size parameters, new pagination UI components, updated API client types/SDK, enhanced tests, and version/dependency bumps to support paginated item listings. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @carvalhocaio - I've reviewed your changes - here's some feedback:
- In DashboardPage,
searchParamsis not a promise in Next.js Server Components—change its type to a plain object and remove theawait searchParamsto simplify the code. - When rendering table rows, avoid using the array index as the key—use a stable unique identifier like
item.idto prevent UI rerender issues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In DashboardPage, `searchParams` is not a promise in Next.js Server Components—change its type to a plain object and remove the `await searchParams` to simplify the code.
- When rendering table rows, avoid using the array index as the key—use a stable unique identifier like `item.id` to prevent UI rerender issues.
## Individual Comments
### Comment 1
<location> `nextjs-frontend/app/dashboard/page.tsx:23` </location>
<code_context>
-export default async function DashboardPage() {
- const items = (await fetchItems()) as ReadItemResponse;
+interface DashboardPageProps {
+ searchParams: Promise<{
+ page?: string;
</code_context>
<issue_to_address>
The type of searchParams as Promise<{ page?: string; size?: string; }> is unconventional for Next.js route handlers.
In the Next.js app directory, searchParams should be a plain object, not a Promise. Using a Promise here may cause confusion or require unnecessary awaiting. Use searchParams: { page?: string; size?: string; } unless a Promise is specifically needed.
Suggested implementation:
```typescript
interface DashboardPageProps {
searchParams: {
page?: string;
size?: string;
};
}
```
```typescript
export default async function DashboardPage({
searchParams,
}: DashboardPageProps) {
const page = Number(searchParams.page) || 1;
const size = Number(searchParams.size) || 10;
```
</issue_to_address>
### Comment 2
<location> `nextjs-frontend/components/page-pagination.tsx:34` </location>
<code_context>
+ return (
+ <div className="flex items-center justify-between my-4">
+ <div className="text-sm text-gray-600">
+ Showing {(currentPage - 1) * pageSize + 1} to{" "}
+ {Math.min(currentPage * pageSize, totalItems)} of {totalItems} results
+ </div>
+
</code_context>
<issue_to_address>
The calculation for the 'Showing X to Y' range does not handle the case when there are zero results.
When totalItems is 0, the range displays 'Showing 1 to 0 of 0 results', which is unclear. Suggest displaying 'Showing 0 of 0 results' or hiding the range when there are no items.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
return (
<div className="flex items-center justify-between my-4">
<div className="text-sm text-gray-600">
Showing {(currentPage - 1) * pageSize + 1} to{" "}
{Math.min(currentPage * pageSize, totalItems)} of {totalItems} results
</div>
=======
return (
<div className="flex items-center justify-between my-4">
<div className="text-sm text-gray-600">
{totalItems === 0 ? (
<>Showing 0 of 0 results</>
) : (
<>
Showing {(currentPage - 1) * pageSize + 1} to{" "}
{Math.min(currentPage * pageSize, totalItems)} of {totalItems} results
</>
)}
</div>
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `nextjs-frontend/components/page-pagination.tsx:60` </location>
<code_context>
+ </Link>
+
+ {/* Page Info */}
+ <span className="text-sm font-medium">
+ Page {currentPage} of {totalPages}
+ </span>
+
</code_context>
<issue_to_address>
Displaying 'Page X of Y' when totalPages is 0 may be misleading.
When totalPages is 0, the UI displays 'Page 1 of 0'. Consider hiding this info or ensuring totalPages is at least 1 to avoid confusion.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
{/* Page Info */}
+ <span className="text-sm font-medium">
+ Page {currentPage} of {totalPages}
+ </span>
=======
{/* Page Info */}
{totalPages > 0 && (
<span className="text-sm font-medium">
Page {currentPage} of {totalPages}
</span>
)}
>>>>>>> REPLACE
</suggested_fix>
### Comment 4
<location> `fastapi_backend/tests/routes/test_items.py:68` </location>
<code_context>
+
+ items = response_data["items"]
+
+ # Filter items created in this test (to avoid interference from other tests)
+ test_items = [
+ item for item in items if item["name"] in ["First Item", "Second Item"]
+ ]
- assert len(items) == 2
- assert any(item["name"] == "First Item" for item in items)
- assert any(item["name"] == "Second Item" for item in items)
+ assert len(test_items) == 2
+ assert any(item["name"] == "First Item" for item in test_items)
+ assert any(item["name"] == "Second Item" for item in test_items)
@pytest.mark.asyncio(loop_scope="function")
</code_context>
<issue_to_address>
Missing tests for pagination edge cases (e.g., page boundaries, empty pages, custom page sizes).
Please add tests for: exceeding page numbers (should return empty items), custom page sizes (verify item count and metadata), last page correctness, and invalid page/size values (ensure proper error handling). This will improve coverage of pagination edge cases.
</issue_to_address>
### Comment 5
<location> `nextjs-frontend/app/openapi-client/types.gen.ts:60` </location>
<code_context>
client_secret?: string | null;
};
+export type Page_ItemRead_ = {
+ items: Array<ItemRead>;
+ total?: number | null;
</code_context>
<issue_to_address>
Consider introducing a generic paginated response type and reusing it to simplify and unify paginated data structures.
```suggestion
// Introduce a reusable generic for all paginated responses
export type Paginated<T> = {
items: T[];
total?: number | null;
page?: number | null;
size?: number | null;
pages?: number | null;
};
// Collapse Page_ItemRead_ into a single well-named alias
export type ReadItemResponse = Paginated<ItemRead>;
// Rename ReadItemData to reflect its intent, and inline query params
export type ReadItemParams = {
query?: {
/** Page number */
page?: number;
/** Page size */
size?: number;
};
};
// Remove these now-unnecessary types:
// - export type Page_ItemRead_ = { … }
// - export type ReadItemData = { … }
```
</issue_to_address>
### Comment 6
<location> `nextjs-frontend/components/ui/select.tsx:3` </location>
<code_context>
+"use client";
+
+import * as React from "react";
+import * as SelectPrimitive from "@radix-ui/react-select";
+import { cn } from "@/lib/utils";
</code_context>
<issue_to_address>
Consider refactoring the component by using a factory function for forwardRef wrappers, merging scroll buttons, and only exporting used primitives to reduce boilerplate.
```markdown
I think we can keep full Radix-select functionality but cut down on boilerplate by:
1. Generating the identical `forwardRef` wrappers with a small factory
2. Merging ScrollUp/ScrollDown into one component
3. Only exporting the pieces you actually use in your page-size selector
## 1) Component factory
```ts
import * as SelectPrimitive from "@radix-ui/react-select";
import { cn } from "@/lib/utils";
import React from "react";
type Prim = typeof SelectPrimitive;
type CompName = keyof Prim;
function createSelect<K extends CompName>(
key: K,
baseClass: string,
defaultProps: Partial<React.ComponentPropsWithoutRef<Prim[K]>> = {}
) {
const Component = React.forwardRef<any, React.ComponentPropsWithoutRef<Prim[K]>>(
({ className, ...props }, ref) => {
const Pr = (SelectPrimitive as any)[key];
return (
<Pr
ref={ref}
className={cn(baseClass, className)}
{...defaultProps}
{...props}
/>
);
}
);
Component.displayName = `Select${key}`;
return Component as React.FC<React.ComponentPropsWithoutRef<Prim[K]>>;
}
```
## 2) ScrollButton
```ts
import { ChevronUpIcon, ChevronDownIcon } from "@radix-ui/react-icons";
const scrollBase = "flex cursor-default items-center justify-center py-1";
export const SelectScrollButton = React.forwardRef<
React.ElementRef<typeof SelectPrimitive.ScrollUpButton>,
React.ComponentPropsWithoutRef<typeof SelectPrimitive.ScrollUpButton> & {
direction?: "up" | "down";
}
>(({ direction = "up", className, ...props }, ref) => {
const Icon = direction === "up" ? ChevronUpIcon : ChevronDownIcon;
const Prim = direction === "up"
? SelectPrimitive.ScrollUpButton
: SelectPrimitive.ScrollDownButton;
return (
<Prim ref={ref} className={cn(scrollBase, className)} {...props}>
<Icon className="h-4 w-4" />
</Prim>
);
});
SelectScrollButton.displayName = "SelectScrollButton";
```
## 3) Your exports
```ts
export const SelectTrigger = createSelect(
"Trigger",
"flex h-9 w-full items-center justify-between …"
);
export const SelectContent = createSelect(
"Content",
"relative z-50 max-h-… shadow-md …",
{ asChild: false } // whatever defaults you need
);
// only if you use these:
export const SelectValue = SelectPrimitive.Value;
export const SelectItem = createSelect("Item", "relative flex w-full …");
export const SelectSeparator = createSelect("Separator", "-mx-1 my-1 h-px bg-muted");
```
This shrinks ~160 lines of near-duplicate `forwardRef` code down to ~30 lines, preserves all functionality, and makes it trivial to add/remove primitives later.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Thanks @carvalhocaio! I'll check it soon! ;) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Endpoint Pagination #73
Description
using fastapi-pagination I added pagination to the item listing endpoint. I made the adjustment to the endpoint for dynamic pagination.
Motivation and Context
Because there was no list, the item rendering had a very large scroll, which wasn't visually appealing, and it also returned all the items at once. With fast pagination, we return a specific number of items and control the number of items displayed on the frontend, which makes the experience much better.
Screenshots (if appropriate):
Steps to reproduce (if appropriate):
Types of changes
Checklist:
Summary by Sourcery
Implement end-to-end pagination for the items API: enable paginated queries on the backend, update OpenAPI types and SDK, and update the frontend dashboard to fetch, display, and navigate through paginated item lists.
New Features:
Enhancements:
Build:
CI:
Documentation:
Tests: