feat(paper): switch saved view to dense list#379
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactors SavedPapersList to extract a new SavedPaperListItem component and formatReadingStatusLabel helper, switches from table to card-based per-item rendering, changes toggleReadStatus signature to (paperId, nextStatus), updates per-item loading state handling, and adds unit tests for status labels and item shape. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant List as "SavedPapersList"
participant Item as "SavedPaperListItem"
participant API as "Server API"
participant DB as "Database"
Note over List,Item: UI rendering and per-item actions
User->>List: view saved papers
List->>Item: render per-item card (props: item, status, handlers)
User->>Item: clicks status dropdown / Unsave
Item->>List: calls onToggleReadStatus(paperId, nextStatus) / onUnsave(paperId)
List->>API: POST/PUT status change or unsave (status: nextStatus)
API->>DB: update paper reading_status / saved flag
DB-->>API: ack
API-->>List: response success
List->>Item: update item state (reading_status, toggling flags)
Item-->>User: updated UI (badge, loading cleared)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Summary of ChangesHello, 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 significantly enhances the user experience for viewing saved papers by transitioning from a tabular display to a dense, card-like list. This change aims to make the list more visually appealing and easier to scan, while also improving the overall layout and interaction for individual paper items. 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
Activity
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
|
![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 successfully refactors the saved papers view from a table to a more compact, dense list. The introduction of the SavedPaperListItem component is a great improvement, encapsulating the item rendering logic and making the main component cleaner and more maintainable. The changes are well-executed and align with the goal of improving the UI. I have one suggestion to enhance the clarity of the button labels for updating a paper's reading status.
![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
🧹 Nitpick comments (1)
web/src/components/research/SavedPapersList.tsx (1)
156-165: Rename callback arg to match new contract.
onToggleReadStatusnow receives the target status, but the prop name still sayscurrentStatus. Please rename it to avoid misuse in future edits.✏️ Suggested rename
type SavedPaperListItemProps = { item: SavedPaperItem status: ReadingStatus selected: boolean togglingRead: boolean unsaving: boolean onToggleSelect: (paperId: number) => void - onToggleReadStatus: (paperId: number, currentStatus: ReadingStatus) => void + onToggleReadStatus: (paperId: number, nextStatus: ReadingStatus) => void onUnsave: (paperId: number, externalId: string | null) => void }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/research/SavedPapersList.tsx` around lines 156 - 165, The prop type SavedPaperListItemProps declares onToggleReadStatus with parameter named currentStatus but the implementation contract now passes the target status; update the parameter name to targetStatus in the SavedPaperListItemProps type (and any related local typings) and then update all call sites and implementations that reference that parameter name (e.g., components using onToggleReadStatus, any handlers that accept currentStatus) to use targetStatus to prevent confusion and match the new contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/components/research/SavedPapersList.tsx`:
- Around line 997-1034: The refactor changed the rendered list structure and how
callbacks are wired (SavedPapersList.tsx: pagedItems mapping to
SavedPaperListItem and handlers toggleSelectAll, toggleSelect, toggleReadStatus,
unsavePaper), but there are no updated tests; add/modify unit/RTL tests to cover
the new dense-list behavior and ensure callbacks are invoked: test rendering of
pagedItems count and "Select all" checkbox (toggleSelectAll), individual
selection (toggleSelect), read-status toggle wiring (toggleReadStatus) including
the updatingAction states (togglingRead/unsaving), and unsave flow
(unsavePaper), plus selectedIds state changes and UI feedback for
toggling/unsaving.
- Around line 240-287: The dropdown is missing the "archived" action so users
cannot set archived status; add another DropdownMenuItem inside
DropdownMenuContent mirroring the others that calls onToggleReadStatus(paper.id,
"archived"), shows the Check icon when status === "archived", and displays the
"Archived" label so formatReadingStatusLabel and the status model remain
consistent; update the list of items adjacent to the existing "To
read"/"Reading"/"Read" DropdownMenuItem entries to include this new "Archived"
entry.
---
Nitpick comments:
In `@web/src/components/research/SavedPapersList.tsx`:
- Around line 156-165: The prop type SavedPaperListItemProps declares
onToggleReadStatus with parameter named currentStatus but the implementation
contract now passes the target status; update the parameter name to targetStatus
in the SavedPaperListItemProps type (and any related local typings) and then
update all call sites and implementations that reference that parameter name
(e.g., components using onToggleReadStatus, any handlers that accept
currentStatus) to use targetStatus to prevent confusion and match the new
contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8a3f69b2-2c97-41ee-8926-cd699ca22449
📒 Files selected for processing (1)
web/src/components/research/SavedPapersList.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/components/research/SavedPapersList.tsx (1)
37-64:⚠️ Potential issue | 🔴 CriticalExport types and helper for test access.
The test file imports
ReadingStatus,SavedPaperItem, andformatReadingStatusLabel, but these are not exported from this module. This will cause compilation errors.🔧 Proposed fix
-type SavedPaperSort = "saved_at" | "judge_score" | "published_at" +export type SavedPaperSort = "saved_at" | "judge_score" | "published_at" -type ReadingStatus = "unread" | "reading" | "read" | "archived" +export type ReadingStatus = "unread" | "reading" | "read" | "archived" -type SavedPaperItem = { +export type SavedPaperItem = {And for the helper function:
-function formatReadingStatusLabel(status: ReadingStatus): string { +export function formatReadingStatusLabel(status: ReadingStatus): string {Also applies to: 149-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/research/SavedPapersList.tsx` around lines 37 - 64, The tests import ReadingStatus, SavedPaperItem, and formatReadingStatusLabel from this module but they are not exported; update the module to export the ReadingStatus type, the SavedPaperItem type, and the helper function formatReadingStatusLabel (and any other helper referenced around the later block at lines ~149-154) by adding explicit exports for those symbols so tests can import them (e.g., export type ReadingStatus = ...; export type SavedPaperItem = ...; export function formatReadingStatusLabel(...) { ... }).
🧹 Nitpick comments (2)
web/src/components/research/SavedPapersList.test.tsx (2)
1-1: Remove unusedviimport.The
vimock utility is imported but never used in this file.🧹 Proposed fix
-import { describe, expect, it, vi } from "vitest" +import { describe, expect, it } from "vitest"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/research/SavedPapersList.test.tsx` at line 1, The import statement in SavedPapersList.test.tsx includes an unused symbol `vi`; remove `vi` from the import so it only imports the used test helpers (`describe`, `expect`, `it`) to eliminate the unused-import warning and keep imports minimal.
9-44: Tests provide baseline coverage for the helper logic.The
formatReadingStatusLabeltests verify all four status mappings, and the type-shape test confirms structural compatibility. Consider adding RTL-based tests for component rendering and callback wiring (selection, status changes, unsave) to fully cover the behavioral changes from this refactor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/research/SavedPapersList.test.tsx` around lines 9 - 44, Current tests cover formatReadingStatusLabel and SavedPaperItem shape but lack integration/behavioral tests for the SavedPapersList component; add React Testing Library tests that render the SavedPapersList (using a mock SavedPaperItem), verify the UI displays the paper title and reading status label (formatReadingStatusLabel), and simulate user interactions to assert callbacks are invoked: select item (selection handler), change reading status (status update handler), and unsave/remove (unsave handler). Use the SavedPapersList component and SavedPaperItem test data, mock the callback props (onSelect, onUpdateReadingStatus, onUnsave), fire events to trigger selection/status change/unsave, and assert the mocks were called with the expected arguments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/components/research/SavedPapersList.test.tsx`:
- Around line 3-7: The tests import SavedPaperItem, ReadingStatus, and
formatReadingStatusLabel from SavedPapersList but those are not exported yet;
open SavedPapersList.tsx and add exports for the types SavedPaperItem and
ReadingStatus (export type ...) and export the helper function
formatReadingStatusLabel (export function formatReadingStatusLabel) while
keeping the default export SavedPapersList intact so the test imports succeed.
In `@web/src/components/research/SavedPapersList.tsx`:
- Around line 156-165: The prop type SavedPaperListItemProps currently names the
second parameter of onToggleReadStatus as currentStatus but callers pass the
target status to transition to; update the parameter name in the
SavedPaperListItemProps declaration (onToggleReadStatus) from currentStatus to
targetStatus (or similar) so the type reflects actual semantics and avoid
confusion when reading functions that call onToggleReadStatus in this component.
---
Outside diff comments:
In `@web/src/components/research/SavedPapersList.tsx`:
- Around line 37-64: The tests import ReadingStatus, SavedPaperItem, and
formatReadingStatusLabel from this module but they are not exported; update the
module to export the ReadingStatus type, the SavedPaperItem type, and the helper
function formatReadingStatusLabel (and any other helper referenced around the
later block at lines ~149-154) by adding explicit exports for those symbols so
tests can import them (e.g., export type ReadingStatus = ...; export type
SavedPaperItem = ...; export function formatReadingStatusLabel(...) { ... }).
---
Nitpick comments:
In `@web/src/components/research/SavedPapersList.test.tsx`:
- Line 1: The import statement in SavedPapersList.test.tsx includes an unused
symbol `vi`; remove `vi` from the import so it only imports the used test
helpers (`describe`, `expect`, `it`) to eliminate the unused-import warning and
keep imports minimal.
- Around line 9-44: Current tests cover formatReadingStatusLabel and
SavedPaperItem shape but lack integration/behavioral tests for the
SavedPapersList component; add React Testing Library tests that render the
SavedPapersList (using a mock SavedPaperItem), verify the UI displays the paper
title and reading status label (formatReadingStatusLabel), and simulate user
interactions to assert callbacks are invoked: select item (selection handler),
change reading status (status update handler), and unsave/remove (unsave
handler). Use the SavedPapersList component and SavedPaperItem test data, mock
the callback props (onSelect, onUpdateReadingStatus, onUnsave), fire events to
trigger selection/status change/unsave, and assert the mocks were called with
the expected arguments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 15ed4b90-0c8c-4814-aad6-abf8fe8de556
📒 Files selected for processing (2)
web/src/components/research/SavedPapersList.test.tsxweb/src/components/research/SavedPapersList.tsx
| import type { SavedPaperItem, ReadingStatus } from "./SavedPapersList" | ||
|
|
||
| // We import the default component only to ensure the module compiles, | ||
| // but these tests focus on the pure helper logic. | ||
| import SavedPapersList, { formatReadingStatusLabel } from "./SavedPapersList" |
There was a problem hiding this comment.
Imports depend on exports being added to the main file.
These imports will fail until SavedPaperItem, ReadingStatus, and formatReadingStatusLabel are exported from SavedPapersList.tsx (see comment on that file).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/research/SavedPapersList.test.tsx` around lines 3 - 7, The
tests import SavedPaperItem, ReadingStatus, and formatReadingStatusLabel from
SavedPapersList but those are not exported yet; open SavedPapersList.tsx and add
exports for the types SavedPaperItem and ReadingStatus (export type ...) and
export the helper function formatReadingStatusLabel (export function
formatReadingStatusLabel) while keeping the default export SavedPapersList
intact so the test imports succeed.
1 participant
Summary by CodeRabbit
Refactor
Tests