Enhance book clippings UI with sorting, view modes, and pagination#179
Enhance book clippings UI with sorting, view modes, and pagination#179
Conversation
Redesign the book detail page's clippings section for clarity, elegance, and
performance on large libraries.
- Switch book query pagination from offset-based PaginationLegacy to
cursor-based Pagination ({limit, lastId}), aligning with clippingList
- New BookClippingCard drops the redundant book title; leads with content,
surfaces pageAt as a pill and createdAt as a subtle footer, adds a soft
left-edge quote accent
- New toolbar above the grid: live count summary, sort toggle (newest/oldest,
client-side on loaded items), and view switch (masonry / compact list)
- New InfiniteScrollFooter provides load-more fallback, loading spinner, and
end-of-list affordance; works alongside masonic's auto-loader
- Refreshed skeleton and empty state match the new layout; glassmorphism
+ blue-400 primary held consistent across light/dark and mobile/desktop
- i18n keys added for en, zhCN, ko
https://claude.ai/code/session_011YmdsaXCz6KAWRPrfeFWL1
Code ReviewOverall this is a well-structured feature addition. The component decomposition is clean, the accessibility attributes are thoughtful, and the localization coverage is solid. A few things worth addressing before merging: Bugs / Correctness1. Missing Japanese locale (
2. Sorting by // content.tsx
copy.sort((a, b) => {
const delta = (b.id as number) - (a.id as number)
return sort === 'newest' ? delta : -delta
})IDs don't reliably represent insertion order — bulk imports, migrations, or any out-of-order writes can make ID ordering diverge from actual creation time. Since the const aTime = new Date(a.createdAt as string).getTime()
const bTime = new Date(b.createdAt as string).getTime()
return sort === 'newest' ? bTime - aTime : aTime - bTime3. // book-clipping-card.tsx
<time dateTime={item.createdAt as unknown as string}>This double cast suggests the GraphQL scalar type for const iso = item.createdAt ? dayjs(item.createdAt).toISOString() : undefined
// ...
<time dateTime={iso}>Performance4. Masonry full remount on sort/view change <Masonry
key={`${view}-${sort}-${columnCount}`}
...
/>The comment acknowledges this forces a remount, but the side-effect is that every item re-animates from scratch on sort or view toggle, causing a visible flash. key={`${view}-${columnCount}`}UX5. Concurrent load triggers in list mode In list mode, Minor / Code Quality6. <div role="group" aria-label="Sort" ...>
<div role="group" aria-label="View" ...>These are hardcoded English strings. They could use 7. const { data: clippingsData, fetchMore } = useQuery<BookQuery>(BookDocument, {
...
notifyOnNetworkStatusChange: true,
})
What's good
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the book clippings page to implement infinite scrolling, sorting, and toggleable view modes. It introduces several new UI components, including a toolbar and specialized clipping cards, while updating the GraphQL schema and localizations. Feedback focuses on optimizing performance by hydrating server-side data to the client to avoid redundant requests, improving the loading experience by restoring the skeleton component, and refining the sorting logic and React keys to prevent unnecessary re-renders.
| pagination: { | ||
| limit: 10, | ||
| offset: 0, | ||
| limit: BOOK_CLIPPINGS_PAGE_SIZE, |
There was a problem hiding this comment.
The clippings data is fetched here on the server but is not passed to the BookPageContent component. Consequently, the client-side component performs the exact same query again upon mounting. This redundant network request increases page load time and server load. Consider passing the server-fetched data as props to BookPageContent to initialize the client-side state or hydrate the Apollo cache.
| import InfiniteScrollFooter from '@/components/clipping-item/infinite-scroll-footer' | ||
| import { usePageTrack } from '@/hooks/tracke' |
There was a problem hiding this comment.
The BookPageSkeleton import was removed, but it is still needed to provide a proper loading state while clippingsData is being fetched on the client. Re-adding this import allows for a better user experience than returning null.
| import InfiniteScrollFooter from '@/components/clipping-item/infinite-scroll-footer' | |
| import { usePageTrack } from '@/hooks/tracke' | |
| import InfiniteScrollFooter from '@/components/clipping-item/infinite-scroll-footer' | |
| import BookPageSkeleton from './skeleton' | |
| import { usePageTrack } from '@/hooks/tracke' |
| copy.sort((a, b) => { | ||
| const delta = (b.id as number) - (a.id as number) | ||
| return sort === 'newest' ? delta : -delta | ||
| }) |
There was a problem hiding this comment.
Sorting by id assumes that IDs are strictly chronological and numeric. It is more reliable and accurate to sort by createdAt, which is explicitly intended for time-based ordering. Additionally, using a direct subtraction on a type-asserted number can be fragile if the ID format is actually a string (as indicated by the uniqueById utility type).
copy.sort((a, b) => {
const timeA = a.createdAt ? new Date(a.createdAt).getTime() : 0
const timeB = b.createdAt ? new Date(b.createdAt).getTime() : 0
const delta = timeB - timeA
return sort === 'newest' ? delta : -delta
})
| if (!clippingsData) { | ||
| return null | ||
| } |
There was a problem hiding this comment.
Returning null while data is loading results in a blank UI area, which is a regression from the previous implementation that showed a loading skeleton. Since BookPageSkeleton was updated in this PR, it should be used here to improve perceived performance and maintain a consistent layout during the initial client-side fetch.
| if (!clippingsData) { | |
| return null | |
| } | |
| // Initial loading state | |
| if (!clippingsData) { | |
| return <BookPageSkeleton /> | |
| } |
| // Keying on view/sort forces Masonic to recalc layout cleanly. | ||
| key={`${view}-${sort}-${columnCount}`} |
There was a problem hiding this comment.
Including sort in the key prop of the Masonry component causes the entire grid to unmount and remount whenever the sort order changes. This is inefficient and can cause a flickering effect as the list grows. Masonic is designed to handle item reordering efficiently via the items prop and itemKey. The key should only include properties that fundamentally change the layout structure, such as view or columnCount.
| // Keying on view/sort forces Masonic to recalc layout cleanly. | |
| key={`${view}-${sort}-${columnCount}`} | |
| // Keying on view/columnCount forces Masonic to recalc layout when the grid structure changes. | |
| key={`${view}-${columnCount}`} |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| @@ -1,4 +1,4 @@ | |||
| query book($id: Int!, $pagination: PaginationLegacy!) { | |||
| query book($id: Int!, $pagination: Pagination!) { | |||
There was a problem hiding this comment.
🔴 GraphQL schema.json not updated: book query still expects PaginationLegacy, causing codegen/build failure
The book.graphql query was changed from $pagination: PaginationLegacy! to $pagination: Pagination!, but src/schema/schema.json was not updated and still declares the book query's pagination argument as type PaginationLegacy (src/schema/schema.json:4066-4068). The Pagination type has fields {lastId: Int, limit: Int!} while PaginationLegacy has {limit: Int!, offset: Int!} — they are entirely different input types.
Since codegen.yml uses schema: src/schema/schema.json as its source, running pnpm codegen (which is executed as part of pnpm build) will fail with a GraphQL validation error (VariablesInAllowedPosition rule) because the variable type Pagination! is not compatible with the expected argument type PaginationLegacy. This blocks the production build.
Schema mismatch details
schema.json says the book field expects:
"name": "pagination",
"type": { "kind": "INPUT_OBJECT", "name": "PaginationLegacy" }But book.graphql declares:
query book($id: Int!, $pagination: Pagination!) {The fix is to re-introspect the schema from the server (if it's been updated) and commit the new schema.json, or uncomment the live server URL in codegen.yml and run pnpm codegen.
Prompt for agents
The book.graphql query now declares $pagination: Pagination! but the local schema.json still defines the book query's pagination argument as PaginationLegacy. These are different GraphQL input types (Pagination has lastId+limit; PaginationLegacy has offset+limit). This causes graphql-codegen to fail when validating operations against the schema.
To fix: re-introspect the server schema to update src/schema/schema.json. This can be done by temporarily uncommenting the live server URL in codegen.yml and running pnpm codegen, or by downloading the latest introspection result from the GraphQL endpoint. The updated schema.json should be committed alongside this PR so that CI builds succeed.
Was this helpful? React with 👍 or 👎 to provide feedback.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34285e90c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } from '@/services/wenqu' | ||
|
|
||
| import BookPageContent from './content' | ||
| import BookPageContent, { BOOK_CLIPPINGS_PAGE_SIZE } from './content' |
There was a problem hiding this comment.
page.tsx is a Server Component, but this import pulls BOOK_CLIPPINGS_PAGE_SIZE from content.tsx, which is marked 'use client'. In App Router, server code can render a client component reference, but it cannot reliably consume runtime values exported by that client module; using this binding in pagination.limit can fail at runtime/build time. Put the constant in a shared non-client module (or duplicate locally) so the server query always receives a plain number.
Useful? React with 👍 / 👎.
| @@ -1,4 +1,4 @@ | |||
| query book($id: Int!, $pagination: PaginationLegacy!) { | |||
| query book($id: Int!, $pagination: Pagination!) { | |||
There was a problem hiding this comment.
The checked-in introspection schema still declares Query.book.pagination as PaginationLegacy (src/schema/schema.json line 4067), but this operation now declares $pagination: Pagination!. That makes the document invalid against the repository schema, so pnpm codegen/build validation will fail and requests against the same schema signature will be rejected. Keep this query on PaginationLegacy until the schema update is committed.
Useful? React with 👍 / 👎.
2 participants
Summary
Refactored the book clippings display to provide users with better control over how they browse clippings. Added client-side sorting (newest/oldest), view mode toggle (masonry/list), improved pagination with cursor-based loading, and a dedicated toolbar with visual feedback.
Key Changes
New Components:
BookClippingsToolbar: Segmented controls for sort order and view mode with clipping count displayBookClippingCard: Refactored clipping display with density modes (default/compact) and improved stylingInfiniteScrollFooter: Loading states and end-of-list indicator with manual load-more buttonPagination Improvements:
lastId)BOOK_CLIPPINGS_PAGE_SIZE = 12uniqueByIdutilityloadingRefandloadingMorestateSorting & Filtering:
View Modes:
UX Enhancements:
Localization:
Schema Updates:
Paginationinstead ofPaginationLegacyBOOK_CLIPPINGS_PAGE_SIZEfrom content component for server-side initial querySkeleton Updates:
https://claude.ai/code/session_011YmdsaXCz6KAWRPrfeFWL1