feat(web): add saved papers list with sorting and status actions #23
Conversation
Summary of ChangesHello @jerry609, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant new feature to the web application: a dedicated page for managing saved research papers. It centralizes the display, sorting, and status updates for these papers into a new, reusable component. This enhancement provides users with better control and organization over their saved research, improving the overall user experience for paper management. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughReplaces server-side PapersPage with a client-side wrapper that renders a header and a new SavedPapersList component. The page no longer fetches papers or renders a table; SavedPapersList handles fetching, pagination, sorting, status toggles, and unsave actions. Changes
Sequence DiagramsequenceDiagram
autonumber
actor User
participant Client as SavedPapersList (Client)
participant API as /api/research/papers/saved
participant DB as Database
User->>Client: Mount / interact (sort, page, actions)
activate Client
Client->>Client: set loading / state
Client->>API: GET ?sort=&page=&limit=
activate API
API->>DB: query saved papers
activate DB
DB-->>API: return papers + metadata
deactivate DB
API-->>Client: papers payload
deactivate API
Client->>Client: render list, pagination
User->>Client: Toggle reading status
activate Client
Client->>Client: optimistic update
Client->>API: PATCH /status
API->>DB: update status
DB-->>API: confirm
API-->>Client: success/failure
Client->>Client: confirm or revert
deactivate Client
User->>Client: Click Unsave
activate Client
Client->>Client: remove item locally
Client->>API: DELETE /:paperId
API->>DB: delete saved record
DB-->>API: confirm
API-->>Client: success/failure
Client->>Client: finalize UI
deactivate Client
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Tip We've launched Issue Planner and it is currently in beta. Please try it out and share your feedback on Discord! Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@web/src/components/research/SavedPapersList.tsx`:
- Around line 107-115: When pageSize changes the page state isn't reset causing
stale page value; update the component to call setPage(1) whenever pageSize
changes (e.g. in the same handler that updates pageSize or inside a useEffect
that watches pageSize) so that totalPages, pagedItems, and the Prev/Next
disabled states reflect the new pageSize immediately; refer to the page,
pageSize, setPage, totalPages and pagedItems logic to place this reset.
- Around line 278-300: The spinner logic must distinguish which action is in
progress, not just which paper; update the local state shape used by
SavedPapersList (replace or augment updatingPaperId with something like
updatingAction: { id: string, action: 'toggleRead' | 'unsave' } ), update
updateReadingStatus to accept and set that action (e.g., call
updateReadingStatus(paper.id, status, true, 'toggleRead') and
updateReadingStatus(paper.id, status, false, 'unsave')), and ensure the buttons
render their spinner only when updatingAction?.id === paper.id &&
updatingAction?.action === '<matching action>' (use 'toggleRead' for the Mark
Read/Reading button and 'unsave' for the Unsave button), and clear
updatingAction on completion or error. Reference: SavedPapersList.tsx,
updateReadingStatus, updatingPaperId/updatingAction.
🧹 Nitpick comments (3)
web/src/components/research/SavedPapersList.tsx (3)
78-101: Hardcodedlimit: "500"fetches the entire dataset client-side.This works for small collections but will degrade as users accumulate papers. Consider server-side pagination in the future, or at minimum document this as a known limitation. The hardcoded
user_id: "default"is presumably a placeholder until auth is wired up.
289-291: Button label "Reading" is ambiguous — reads like a current state, not an action.When
status === "read", the button text is"Reading". Users may not understand this means "mark as currently reading." Consider"Mark Reading"or"Resume Reading"for clarity.
292-299: Unsave action has no confirmation and is not easily reversible.A misclick permanently removes the paper from the saved list (the optimistic removal on line 142 is after server success, so the paper is truly unsaved). Consider adding a confirmation dialog or an undo toast.
![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 introduces a SavedPapersList component for displaying and managing saved papers, including sorting and status updates, and refactors the papers page. A critical security vulnerability exists due to hardcoded user identifiers (user_id: "default") in data fetching and update operations, leading to broken access control. There's also a high-severity scalability issue with client-side pagination and a medium-severity suggestion for optimistic UI updates to enhance data consistency.
| const qs = new URLSearchParams({ | ||
| sort_by: sortBy, | ||
| limit: "500", | ||
| user_id: "default", |
There was a problem hiding this comment.
The user_id is hardcoded to "default" in the loadSavedPapers function. This is a critical security risk as it bypasses proper user authentication and authorization, potentially leading to data leakage or unauthorized access. The user ID should be dynamically retrieved from the user's session or an authentication context.
| setError(null) | ||
| try { | ||
| const body: Record<string, unknown> = { | ||
| user_id: "default", |
There was a problem hiding this comment.
The user_id is hardcoded to "default" in the updateReadingStatus function. This is a critical security vulnerability, allowing any user to modify the reading status or unsave papers for the "default" user account, which is a form of Insecure Direct Object Reference (IDOR). This must be replaced with the ID of the currently authenticated user, retrieved from their session or an authentication context.
| try { | ||
| const qs = new URLSearchParams({ | ||
| sort_by: sortBy, | ||
| limit: "500", |
There was a problem hiding this comment.
Fetching a hardcoded limit of 500 items to perform client-side pagination is not scalable. This will lead to poor performance, high memory usage on the client, and slow initial load times as the number of saved papers grows. It is highly recommended to implement server-side pagination. The API should accept pagination parameters (e.g., page and limit), and this component should send them in the request, only fetching the data needed for the current view.
| if (markSaved === false) { | ||
| setItems((prev) => prev.filter((row) => row.paper.id !== paperId)) | ||
| return | ||
| } | ||
|
|
||
| setItems((prev) => | ||
| prev.map((row) => { | ||
| if (row.paper.id !== paperId) return row | ||
| return { | ||
| ...row, | ||
| reading_status: { | ||
| ...row.reading_status, | ||
| status, | ||
| updated_at: new Date().toISOString(), | ||
| }, | ||
| } | ||
| }), | ||
| ) |
There was a problem hiding this comment.
The UI is being updated optimistically with a client-generated timestamp (new Date().toISOString()). This can lead to data inconsistencies between the client and server. A better practice is to have the API endpoint return the updated object after a successful modification. The client should then use this authoritative response to update its state, ensuring the UI accurately reflects the state of the data in the database.
if (markSaved === false) {
setItems((prev) => prev.filter((row) => row.paper.id !== paperId))
return
}
const updatedItem = (await res.json()) as SavedPaperItem
setItems((prev) =>
prev.map((row) => (row.paper.id === paperId ? updatedItem : row))
)
There was a problem hiding this comment.
Pull request overview
Adds a new Saved Papers list UI to the web app and replaces the existing /papers page content to display saved papers with sorting and basic status actions, aligning with issue #7’s “saved list page + actions” scope.
Changes:
- Introduces
SavedPapersListclient component to load/render saved papers with sorting, client-side pagination, and actions (detail, mark read/reading, unsave). - Updates
/paperspage to use the newSavedPapersListcomponent instead of the previous papers table.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| web/src/components/research/SavedPapersList.tsx | Implements saved papers table UI, fetch + sort, pagination UI, and status/unsave actions. |
| web/src/app/papers/page.tsx | Replaces old papers table with the new saved papers list page content. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }) | ||
| const res = await fetch(`/api/research/papers/saved?${qs.toString()}`) | ||
| if (!res.ok) { | ||
| throw new Error(await res.text()) |
There was a problem hiding this comment.
Throwing await res.text() directly as the UI error can leak upstream/internal details (and may be HTML) to end users. Consider normalizing errors (status code + safe message, optionally parse JSON {detail}) before rendering error.
| throw new Error(await res.text()) | |
| let message = `Failed to load saved papers (status ${res.status})` | |
| const contentType = res.headers.get("content-type") || "" | |
| if (contentType.includes("application/json")) { | |
| try { | |
| const errorBody = await res.json() | |
| if (errorBody && typeof errorBody.detail === "string") { | |
| message = errorBody.detail | |
| } | |
| } catch { | |
| // ignore JSON parsing errors and fall back to the generic message | |
| } | |
| } | |
| throw new Error(message) |
| const res = await fetch(`/api/research/papers/${paperId}/status`, { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify(body), | ||
| }) |
There was a problem hiding this comment.
/api/research/papers/${paperId}/status is invoked here, but there is no matching Next.js API route handler under web/src/app/api/research/papers/[paperId]/status (and no rewrites). This will 404 at runtime unless you add a proxy route (and confirm the backend endpoint exists).
| }) | ||
|
|
||
| if (!res.ok) { | ||
| throw new Error(await res.text()) |
There was a problem hiding this comment.
Same as above: throwing await res.text() directly can leak upstream/internal details to end users. Prefer a normalized error message (status code + safe detail) rather than rendering raw response bodies.
| throw new Error(await res.text()) | |
| throw new Error( | |
| `Failed to update reading status (HTTP ${res.status}${res.statusText ? ` ${res.statusText}` : ""})`, | |
| ) |
| limit: "500", | ||
| user_id: "default", | ||
| }) | ||
| const res = await fetch(`/api/research/papers/saved?${qs.toString()}`) |
There was a problem hiding this comment.
This fetches /api/research/papers/saved, but there is no corresponding Next.js route handler under web/src/app/api/research/papers/saved (and no rewrites configured in next.config.ts). As-is, this call will 404 from the web app. Add the missing proxy route (and ensure the backend endpoint exists).
| const res = await fetch(`/api/research/papers/saved?${qs.toString()}`) | |
| const apiBase = process.env.NEXT_PUBLIC_BACKEND_API_URL ?? "" | |
| const res = await fetch(`${apiBase}/research/papers/saved?${qs.toString()}`) |
| sort_by: sortBy, | ||
| limit: "500", | ||
| user_id: "default", | ||
| }) |
There was a problem hiding this comment.
This uses a hard-coded limit: "500" and then paginates client-side. That doesn’t satisfy “pagination/limit” at the API level and will break for users with >500 saved papers and increases load time. Prefer requesting the current page from the API (e.g., limit=pageSize + offset/page) and returning total count from the endpoint.
| const [updatingPaperId, setUpdatingPaperId] = useState<number | null>(null) | ||
|
|
There was a problem hiding this comment.
updatingPaperId only tracks a single in-flight status update. If the user triggers actions on two different rows quickly, the second request will overwrite this state and re-enable the first row while its request is still running, leading to confusing UI and concurrent updates. Track updates per paper (e.g., Set<number> of updating IDs) or disable all actions while any update is pending.
1 participant
close #7
Summary by CodeRabbit