feat(Research page): Rebuild the Research page and set it as the homepage#49
Conversation
…page Closes jerry609#48 Signed-off-by: LIU BOYU <oor2020@163.com>
feat(Research page): Rebuild the Research page and set it as the homepage
Summary of ChangesHello @CJBshuosi, 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 delivers a significant user experience improvement by redesigning the Research page to be more intuitive and efficient. The new layout, inspired by modern AI interfaces, simplifies the process of finding and managing research papers. By making this redesigned page the new homepage and relocating the existing dashboard, the update enhances the application's primary user flow, focusing on immediate research tasks while maintaining access to broader system overviews. 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
|
📝 WalkthroughWalkthroughRedesigns the Research page into a Claude-style, single-column, search-first interface; adds backend support for PATCH /research/tracks/{id}; introduces multiple new React components for search, results, track management and modals; moves original Dashboard to /dashboard and makes Research the new homepage; adds tests and utility helpers. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant SearchBox as SearchBox (Client)
participant Page as ResearchPageNew (Client)
participant API as Backend API
participant Store as Research Store / DB
User->>SearchBox: enter query & submit
SearchBox->>Page: onSearch(query, activeTrack)
Page->>API: POST /research/context (query, track_id)
API->>Store: query papers, build context
Store-->>API: papers + reasoning
API-->>Page: ContextPack (papers, reasons)
Page->>Page: set results, isSearching=false
Page->>SearchBox: update UI (results visible)
User->>Page: open ManageTracks / CreateTrack
Page->>API: PATCH /research/tracks/{id} OR POST /research/tracks
API->>Store: update/create track, handle IntegrityError
Store-->>API: updated track
API-->>Page: TrackResponse
Page->>Page: refresh tracks, schedule embeddings (background)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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)
Tip Issue Planner is now in beta. Read the docs and try it out! 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 |
![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 rebuilds the Research page with a modern, search-centric UI, setting it as the new homepage. However, a security audit revealed a high-severity Insecure Direct Object Reference (IDOR) vulnerability in the backend API, stemming from user identification relying on a request parameter rather than a secure session. Furthermore, a medium-severity Cross-Site Scripting (XSS) vulnerability was identified in the frontend where external URLs are rendered without proper sanitization. Beyond these security concerns, there are also minor suggestions for improving maintainability and adhering to React best practices. Addressing these security issues is critical, and incorporating the suggested code improvements will further enhance this otherwise well-executed and extensive update.
| track_id: int, | ||
| req: TrackUpdateRequest, | ||
| background_tasks: BackgroundTasks, | ||
| user_id: str = "default", |
There was a problem hiding this comment.
The API endpoint accepts a user_id parameter (defaulting to "default") which is used to identify the user for data access and modification. Since this parameter is taken from the request (query string) and not from a secure session or token, any user can impersonate another user by providing their user_id in the request. This allows unauthorized access to and modification of research tracks belonging to other users.
To remediate this, remove user_id as a request parameter. Instead, implement a secure authentication mechanism and derive the authenticated user's ID from the secure session or token on the backend.
| </h3> | ||
| {paper.url && ( | ||
| <a | ||
| href={paper.url} |
There was a problem hiding this comment.
The paper.url property is rendered directly into the href attribute of an <a> tag without sanitization. If an attacker can control the URL (e.g., via a malicious entry in the external paper database), they can inject a javascript: URL to execute arbitrary code in the context of the user's session when the link is clicked.
To remediate this, sanitize the URL before rendering it. Ensure it uses an allowed protocol (e.g., http:, https:) and does not contain malicious scripts.
| update_data: Dict[str, Any] = {} | ||
| if req.name is not None: | ||
| update_data["name"] = req.name | ||
| if req.description is not None: | ||
| update_data["description"] = req.description | ||
| if req.keywords is not None: | ||
| update_data["keywords"] = req.keywords | ||
| if req.venues is not None: | ||
| update_data["venues"] = req.venues | ||
| if req.methods is not None: | ||
| update_data["methods"] = req.methods |
There was a problem hiding this comment.
The manual construction of update_data can be simplified and made more maintainable by using Pydantic's built-in methods. This will automatically include any new fields added to TrackUpdateRequest in the future.
Using model_dump(exclude_unset=True) (for Pydantic v2) or dict(exclude_unset=True) (for Pydantic v1) is a more robust approach.
| update_data: Dict[str, Any] = {} | |
| if req.name is not None: | |
| update_data["name"] = req.name | |
| if req.description is not None: | |
| update_data["description"] = req.description | |
| if req.keywords is not None: | |
| update_data["keywords"] = req.keywords | |
| if req.venues is not None: | |
| update_data["venues"] = req.venues | |
| if req.methods is not None: | |
| update_data["methods"] = req.methods | |
| update_data = req.model_dump(exclude_unset=True) |
| {trends.map((topic, i) => ( | ||
| <Badge | ||
| key={i} | ||
| variant="secondary" | ||
| className="text-xs cursor-pointer hover:bg-primary hover:text-primary-foreground transition-colors" | ||
| > | ||
| {topic.text} | ||
| </Badge> | ||
| ))} |
There was a problem hiding this comment.
Using the array index as a key is an anti-pattern in React, especially for dynamic lists that can be reordered or filtered. This can lead to unpredictable UI behavior and performance issues. It's recommended to use a unique and stable identifier from the data itself, such as a unique ID or string from the topic object.
| {trends.map((topic, i) => ( | |
| <Badge | |
| key={i} | |
| variant="secondary" | |
| className="text-xs cursor-pointer hover:bg-primary hover:text-primary-foreground transition-colors" | |
| > | |
| {topic.text} | |
| </Badge> | |
| ))} | |
| {trends.map((topic) => ( | |
| <Badge | |
| key={topic.text} // Assuming topic.text is unique. If not, a unique ID from the topic object is preferred. | |
| variant="secondary" | |
| className="text-xs cursor-pointer hover:bg-primary hover:text-primary-foreground transition-colors" | |
| > | |
| {topic.text} | |
| </Badge> | |
| ))} |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/paperbot/infrastructure/stores/research_store.py (1)
11-13:⚠️ Potential issue | 🟡 MinorDuplicate import.
normalize_arxiv_idandnormalize_doiare imported twice (lines 11 and 13).Fix
from paperbot.domain.paper_identity import normalize_arxiv_id, normalize_doi -from paperbot.domain.paper_identity import normalize_arxiv_id, normalize_doi -
🤖 Fix all issues with AI agents
In `@src/paperbot/api/routes/research.py`:
- Around line 135-161: Add tests covering the new PATCH endpoint function
update_track and the store method SqlAlchemyResearchStore.update_track: write
unit tests for (1) successful partial update (verify returned TrackResponse
contains updated fields and that _schedule_embedding_precompute/background task
is triggered with the correct track_id), (2) empty update payload returns HTTP
400, and (3) non-existent track returns HTTP 404; for the store method add tests
for updating individual fields and for returning None when track_id not found.
In tests, call the router path that hits update_track and mock/spy the
_research_store.update_track and _schedule_embedding_precompute (or inject a
test BackgroundTasks) to assert they are invoked with expected arguments; also
include input fixtures for TrackUpdateRequest variations to cover name,
description, keywords, venues, and methods updates.
In `@src/paperbot/infrastructure/stores/research_store.py`:
- Around line 182-218: The update_track method can raise an unhandled
sqlalchemy.exc.IntegrityError when changing a track name to one that already
exists; modify update_track to catch IntegrityError around session.commit(),
call session.rollback() on error, and then raise the same kind of domain-level
exception used in create_track (or a new DuplicateTrackNameError) so the API
layer can map it to a 409 Conflict instead of returning a 500; reference the
update_track function, the session.commit() call, and the create_track handling
as the model to follow.
In `@web/src/app/dashboard/page.tsx`:
- Around line 14-21: DashboardPage uses Promise.all to call fetchStats,
fetchTrendingTopics, fetchPipelineTasks, fetchReadingQueue, and fetchLLMUsage so
a single network error will reject the whole page; change to Promise.allSettled
(or individually try/catch) and map the settled results to either the .value or
a safe fallback for each corresponding variable (stats, trends, tasks,
readingQueue, llmUsage), ensuring the component renders with defaults instead of
throwing; update any downstream code that assumes non-null responses
accordingly.
In `@web/src/components/research/CreateTrackModal.tsx`:
- Around line 39-58: The form is being cleared unconditionally in handleCreate,
causing user input loss when onCreateTrack (parent
ResearchPageNew.handleCreateTrack) fails or swallows errors; change handleCreate
to only reset via setName/setDescription/setKeywords after a confirmed success —
either by letting onCreateTrack re-throw errors from
ResearchPageNew.handleCreateTrack and wrapping the await onCreateTrack(...) in
try/catch (reset in the try after await), or by validating a returned success
flag from onCreateTrack before clearing the fields.
In `@web/src/components/research/EditTrackModal.tsx`:
- Around line 55-72: handleUpdate currently closes the modal unconditionally
after calling onUpdateTrack so failures are hidden; change it to await
onUpdateTrack inside a try/catch and only call onOpenChange(false) on success
(or re-throw the caught error) so the modal remains open on failure, and/or
update ResearchPageNew.handleUpdateTrack to re-throw after setting its error
state so handleUpdate can detect failures; reference handleUpdate,
onUpdateTrack, ResearchPageNew.handleUpdateTrack, and onOpenChange when applying
this fix.
In `@web/src/components/research/ResearchPageNew.tsx`:
- Around line 38-45: The current fetchJson function attaches the full response
body to the thrown Error (via res.text()), which can leak server internals into
the UI; change fetchJson to throw a safe, generic message like `${res.status}
${res.statusText}` and avoid embedding the full response body in the Error that
flows to ResearchPageNew; if you still need the body for debugging, log it to
console or include it conditionally only in development (e.g., use
process.env.NODE_ENV === 'development') so that the UI-rendered error (the <pre>
in ResearchPageNew) never displays raw server stack traces or internal details.
- Around line 245-280: The handleFeedback function currently calls fetchJson
without error handling which can cause unhandled promise rejections; wrap the
await fetchJson(...) call in a try/catch inside handleFeedback, catch any error,
setError(...) with a user-friendly message (and optionally console.error the
error), and ensure you do not modify global loading state (PaperCard handles
loading) so that failures surface to the UI without crashing the parent click
handler.
In `@web/src/components/research/SearchBox.tsx`:
- Around line 103-114: The icon-only search Button in SearchBox.tsx lacks an
accessible label; update the Button (the element using onClick={handleSubmit}
and props disabled={disabled || isSearching || !query.trim()}) to include an
aria-label that reflects state (e.g., aria-label={isSearching ? "Searching…" :
"Search"}), so screen readers get a meaningful label for both the idle and
loading states; ensure the label is updated dynamically alongside isSearching
and do not remove the visual icon.
🧹 Nitpick comments (10)
web/src/app/dashboard/page.tsx (1)
77-85: Prefer a stable key over array index.
topic.textis unique and more semantically meaningful than the array index.Suggested fix
- {trends.map((topic, i) => ( - <Badge - key={i} + {trends.map((topic) => ( + <Badge + key={topic.text}web/src/components/research/PaperCard.tsx (1)
50-83: Action errors are silently swallowed — user gets no feedback on failure.All three handlers (
handleSave,handleLike,handleDislike) usetry/finallywithout acatch. If the callback throws, the loading state resets but the user sees no indication that the action failed. Consider adding acatchblock to surface a toast or temporary error state.web/src/components/research/SearchResults.tsx (1)
110-119: Consider computing stagger delay instead of enumerating indices.The explicit
idx === 0 && ...throughidx >= 5is functional but won't scale well if the number of visible cards changes. A computed approach is more maintainable:Suggested refactor
className={cn( "animate-in fade-in slide-in-from-bottom-2", - // Staggered animation delay - idx === 0 && "duration-300", - idx === 1 && "duration-300 delay-[50ms]", - idx === 2 && "duration-300 delay-[100ms]", - idx === 3 && "duration-300 delay-[150ms]", - idx === 4 && "duration-300 delay-[200ms]", - idx >= 5 && "duration-300 delay-[250ms]" + "duration-300", + `delay-[${Math.min(idx * 50, 250)}ms]` )}docs/research_page_redesign.md (1)
16-58: Add language identifiers to fenced code blocks.Multiple fenced code blocks throughout the document lack language specifiers, which triggers markdownlint MD040 warnings. For ASCII art diagrams, use
```textto satisfy the linter and improve rendering.web/src/app/research/page.tsx (1)
1-9: Duplicate page: identical toweb/src/app/page.tsx.Both
/(page.tsx) and/research(research/page.tsx) render the exact same markup — a<div>wrapper around<ResearchPageNew />. Consider having one redirect to the other (e.g.,redirect("/research")from the root page, or vice versa) to avoid maintaining two identical files.Option: redirect from root to /research
In
web/src/app/page.tsx:-import ResearchPageNew from "@/components/research/ResearchPageNew" +import { redirect } from "next/navigation" -export default function HomePage() { - return ( - <div className="flex-1 bg-stone-50/50 dark:bg-background"> - <ResearchPageNew /> - </div> - ) +export default function HomePage() { + redirect("/research") }web/src/components/research/TrackPills.tsx (1)
65-69: The "+N more" indicator is not actionable.Users can see that additional tracks exist but have no way to access them from this component. Consider making the indicator clickable — e.g., opening the Manage Tracks modal — to avoid a dead-end UX.
Make "+N more" open the manage modal
Add an
onManageTracksprop and wire it:interface TrackPillsProps { tracks: Track[] activeTrackId: number | null onSelectTrack: (trackId: number) => void onNewTrack: () => void + onManageTracks?: () => void disabled?: boolean maxVisible?: number }{hasMore && ( - <span className="text-sm sm:text-base text-muted-foreground"> + <button + className="text-sm sm:text-base text-muted-foreground hover:underline" + onClick={onManageTracks} + disabled={disabled} + > +{tracks.length - maxVisible} more - </span> + </button> )}web/src/components/research/ResearchPageNew.tsx (2)
54-76: Sharedloadingflag guards unrelated operations.A single
loadingboolean is used byactivateTrack,handleCreateTrack,handleUpdateTrack, andconfirmClearMemory. If any two overlap (e.g., user activates a track while an update is in flight), the firstfinallyblock clearsloadingprematurely. Consider per-action loading states or a counter.
56-56: HardcodeduserId— worth a TODO.
userIdis always"default". This is presumably intentional for the current phase, but a comment or TODO would make it clear this needs auth integration.Add a TODO
- const [userId] = useState("default") + // TODO: Replace with authenticated user ID once auth is integrated + const [userId] = useState("default")web/src/components/research/ManageTracksModal.tsx (1)
103-112: Consider adding a confirmation step before clearing track memory.This is a destructive, irreversible action that resets a track's context. A single click on the Trash2 icon button (which has no label text) could easily be triggered accidentally. Consider either an
AlertDialogconfirmation or an optimistic undo pattern.web/src/components/research/index.ts (1)
18-20: Legacy exports may discourage cleanup.Exporting
ResearchDashboardandSavedPapersListfrom the barrel keeps them discoverable and easy to import. If they're truly superseded by the new components, consider removing these re-exports (or adding a@deprecatedJSDoc) so consumers naturally migrate away.
| @router.patch("/research/tracks/{track_id}", response_model=TrackResponse) | ||
| def update_track( | ||
| track_id: int, | ||
| req: TrackUpdateRequest, | ||
| background_tasks: BackgroundTasks, | ||
| user_id: str = "default", | ||
| ): | ||
| update_data: Dict[str, Any] = {} | ||
| if req.name is not None: | ||
| update_data["name"] = req.name | ||
| if req.description is not None: | ||
| update_data["description"] = req.description | ||
| if req.keywords is not None: | ||
| update_data["keywords"] = req.keywords | ||
| if req.venues is not None: | ||
| update_data["venues"] = req.venues | ||
| if req.methods is not None: | ||
| update_data["methods"] = req.methods | ||
|
|
||
| if not update_data: | ||
| raise HTTPException(status_code=400, detail="No fields to update") | ||
|
|
||
| track = _research_store.update_track(user_id=user_id, track_id=track_id, **update_data) | ||
| if not track: | ||
| raise HTTPException(status_code=404, detail="Track not found") | ||
| _schedule_embedding_precompute(background_tasks, user_id=user_id, track_ids=[track_id]) | ||
| return TrackResponse(track=track) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing tests for new PATCH endpoint and store method.
The new update_track endpoint and SqlAlchemyResearchStore.update_track introduce behavior changes but no corresponding tests are included. As per coding guidelines, "If behavior changes, add or update tests".
🤖 Prompt for AI Agents
In `@src/paperbot/api/routes/research.py` around lines 135 - 161, Add tests
covering the new PATCH endpoint function update_track and the store method
SqlAlchemyResearchStore.update_track: write unit tests for (1) successful
partial update (verify returned TrackResponse contains updated fields and that
_schedule_embedding_precompute/background task is triggered with the correct
track_id), (2) empty update payload returns HTTP 400, and (3) non-existent track
returns HTTP 404; for the store method add tests for updating individual fields
and for returning None when track_id not found. In tests, call the router path
that hits update_track and mock/spy the _research_store.update_track and
_schedule_embedding_precompute (or inject a test BackgroundTasks) to assert they
are invoked with expected arguments; also include input fixtures for
TrackUpdateRequest variations to cover name, description, keywords, venues, and
methods updates.
| def update_track( | ||
| self, | ||
| *, | ||
| user_id: str, | ||
| track_id: int, | ||
| name: Optional[str] = None, | ||
| description: Optional[str] = None, | ||
| keywords: Optional[List[str]] = None, | ||
| venues: Optional[List[str]] = None, | ||
| methods: Optional[List[str]] = None, | ||
| ) -> Optional[Dict[str, Any]]: | ||
| now = _utcnow() | ||
| with self._provider.session() as session: | ||
| row = session.execute( | ||
| select(ResearchTrackModel).where( | ||
| ResearchTrackModel.user_id == user_id, ResearchTrackModel.id == track_id | ||
| ) | ||
| ).scalar_one_or_none() | ||
| if row is None: | ||
| return None | ||
|
|
||
| if name is not None: | ||
| row.name = name.strip() | ||
| if description is not None: | ||
| row.description = description.strip() | ||
| if keywords is not None: | ||
| row.keywords_json = _dump_list(keywords) | ||
| if venues is not None: | ||
| row.venues_json = _dump_list(venues) | ||
| if methods is not None: | ||
| row.methods_json = _dump_list(methods) | ||
|
|
||
| row.updated_at = now | ||
| session.add(row) | ||
| session.commit() | ||
| session.refresh(row) | ||
| return self._track_to_dict(row) |
There was a problem hiding this comment.
IntegrityError not handled when renaming to a duplicate name.
The research_tracks table has a UniqueConstraint("user_id", "name"). If a user updates a track's name to one that already exists, the session.commit() on line 216 will raise an unhandled IntegrityError, resulting in a 500 response. The existing create_track method handles this case explicitly.
Suggested fix
row.updated_at = now
session.add(row)
- session.commit()
- session.refresh(row)
- return self._track_to_dict(row)
+ try:
+ session.commit()
+ except IntegrityError:
+ session.rollback()
+ return None # or raise a more specific error
+ session.refresh(row)
+ return self._track_to_dict(row)Note: Returning None would map to a 404 at the API layer, which isn't ideal for a uniqueness conflict. You may want to raise a distinct exception that the route can catch and return as a 409 Conflict instead.
🤖 Prompt for AI Agents
In `@src/paperbot/infrastructure/stores/research_store.py` around lines 182 - 218,
The update_track method can raise an unhandled sqlalchemy.exc.IntegrityError
when changing a track name to one that already exists; modify update_track to
catch IntegrityError around session.commit(), call session.rollback() on error,
and then raise the same kind of domain-level exception used in create_track (or
a new DuplicateTrackNameError) so the API layer can map it to a 409 Conflict
instead of returning a 500; reference the update_track function, the
session.commit() call, and the create_track handling as the model to follow.
| async function fetchJson<T>(url: string, init?: RequestInit): Promise<T> { | ||
| const res = await fetch(url, init) | ||
| if (!res.ok) { | ||
| const text = await res.text().catch(() => "") | ||
| throw new Error(`${res.status} ${res.statusText} ${text}`.trim()) | ||
| } | ||
| return res.json() as Promise<T> | ||
| } |
There was a problem hiding this comment.
fetchJson error message may leak server internals to the UI.
The error includes the full response body (res.text()), which is rendered verbatim in a <pre> tag on Line 413. If the server returns stack traces or internal details in error responses, they'll be shown to the user.
🤖 Prompt for AI Agents
In `@web/src/components/research/ResearchPageNew.tsx` around lines 38 - 45, The
current fetchJson function attaches the full response body to the thrown Error
(via res.text()), which can leak server internals into the UI; change fetchJson
to throw a safe, generic message like `${res.status} ${res.statusText}` and
avoid embedding the full response body in the Error that flows to
ResearchPageNew; if you still need the body for debugging, log it to console or
include it conditionally only in development (e.g., use process.env.NODE_ENV ===
'development') so that the UI-rendered error (the <pre> in ResearchPageNew)
never displays raw server stack traces or internal details.
| async function handleFeedback( | ||
| paperId: string, | ||
| action: string, | ||
| rank?: number, | ||
| paper?: Paper | ||
| ): Promise<void> { | ||
| // Don't set global loading - PaperCard handles its own loading state | ||
| setError(null) | ||
| const body: Record<string, unknown> = { | ||
| user_id: userId, | ||
| track_id: activeTrackId, | ||
| paper_id: paperId, | ||
| action, | ||
| weight: 0.0, | ||
| context_run_id: contextPack?.context_run_id ?? null, | ||
| context_rank: typeof rank === "number" ? rank : undefined, | ||
| metadata: {}, | ||
| } | ||
|
|
||
| // Include paper metadata for save action | ||
| if (action === "save" && paper) { | ||
| body.paper_title = paper.title | ||
| body.paper_abstract = paper.abstract || "" | ||
| body.paper_authors = paper.authors || [] | ||
| body.paper_year = paper.year | ||
| body.paper_venue = paper.venue | ||
| body.paper_citation_count = paper.citation_count | ||
| body.paper_url = paper.url | ||
| } | ||
|
|
||
| await fetchJson(`/api/research/papers/feedback`, { | ||
| method: "POST", | ||
| body: JSON.stringify(body), | ||
| headers: { "Content-Type": "application/json" }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
handleFeedback has no error handling — unhandled promise rejection.
Every other API call in this component wraps fetchJson in try/catch, but handleFeedback does not. If the feedback request fails, the rejected promise will propagate to the child component's click handler and surface as an unhandled rejection, with no user-visible feedback.
Add try/catch
async function handleFeedback(
paperId: string,
action: string,
rank?: number,
paper?: Paper
): Promise<void> {
- // Don't set global loading - PaperCard handles its own loading state
- setError(null)
+ try {
+ setError(null)
const body: Record<string, unknown> = {
// ... existing body construction
}
await fetchJson(`/api/research/papers/feedback`, {
method: "POST",
body: JSON.stringify(body),
headers: { "Content-Type": "application/json" },
})
+ } catch (e) {
+ setError(String(e))
+ }
}🤖 Prompt for AI Agents
In `@web/src/components/research/ResearchPageNew.tsx` around lines 245 - 280, The
handleFeedback function currently calls fetchJson without error handling which
can cause unhandled promise rejections; wrap the await fetchJson(...) call in a
try/catch inside handleFeedback, catch any error, setError(...) with a
user-friendly message (and optionally console.error the error), and ensure you
do not modify global loading state (PaperCard handles loading) so that failures
surface to the UI without crashing the parent click handler.
Closes jerry609#48 Signed-off-by: LIU BOYU <oor2020@163.com>
Fix(Research page): add -- unit test and fix security issues
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@web/src/app/dashboard/page.tsx`:
- Around line 41-49: The Search input (Input component) and Import Button render
without handlers; add an onSubmit/onKeyDown handler for the Input (e.g.,
implement onSearchSubmit or handleSearchChange + onEnter to trigger search) and
wire the Button to an onClick handler (e.g., handleImportClick) that calls the
existing import/search logic or dispatches the relevant action; if they are
intentionally inactive, mark them disabled (disabled prop) and add a clear TODO
comment explaining future behavior so intent is explicit.
🧹 Nitpick comments (4)
web/src/components/research/ResearchPageNew.tsx (1)
38-45:fetchJsonis duplicated acrossResearchDashboard.tsxandResearchPageNew.tsx.Both files define identical
fetchJsonimplementations. Consider extracting it to a shared utility (e.g., in@/lib/utils.tsor a dedicated@/lib/api.ts).src/paperbot/infrastructure/stores/research_store.py (1)
11-13: Duplicate import on lines 11 and 13.
normalize_arxiv_idandnormalize_doiare imported twice from the same module.Remove duplicate
from paperbot.domain.paper_identity import normalize_arxiv_id, normalize_doi -from paperbot.domain.paper_identity import normalize_arxiv_id, normalize_doi -tests/unit/test_research_store_track_update.py (1)
9-12: Consider a shared pytest fixture to reduce boilerplate.Every test repeats the same 2-line store setup. A
@pytest.fixturewould reduce duplication.Example fixture
+@pytest.fixture +def store(tmp_path): + db_url = f"sqlite:///{tmp_path / 'test.db'}" + return SqlAlchemyResearchStore(db_url=db_url, auto_create_schema=True) + + -def test_update_track_name_success(tmp_path): +def test_update_track_name_success(store): """Test updating track name successfully.""" - db_url = f"sqlite:///{tmp_path / 'test.db'}" - store = SqlAlchemyResearchStore(db_url=db_url, auto_create_schema=True) - # Create a track track = store.create_track(user_id="test", name="Original Name", activate=False)tests/integration/test_research_track_routes.py (1)
79-154: Consider adding tests forvenuesandmethodsfields.The
TrackUpdateRequestmodel also supportsvenuesandmethods(per the route definition), but there are no tests exercising those fields. Adding at least one test for each would improve confidence that serialization (_dump_list/_load_list) works correctly for all list fields.
| <div className="flex w-full sm:max-w-sm items-center gap-1.5 bg-muted/50 p-1 rounded-md border"> | ||
| <Search className="ml-2 h-4 w-4 text-muted-foreground" /> | ||
| <Input | ||
| type="text" | ||
| placeholder="arXiv ID, URL or search..." | ||
| className="h-8 border-none bg-transparent shadow-none focus-visible:ring-0 text-sm" | ||
| /> | ||
| <Button size="sm" className="h-7 text-xs">Import</Button> | ||
| </div> |
There was a problem hiding this comment.
Search input and Import button are non-functional.
The search bar and Import button lack event handlers — they render as inert UI elements. If these are intentional placeholders for future work, consider adding a disabled attribute or a TODO comment to make that explicit.
🤖 Prompt for AI Agents
In `@web/src/app/dashboard/page.tsx` around lines 41 - 49, The Search input (Input
component) and Import Button render without handlers; add an onSubmit/onKeyDown
handler for the Input (e.g., implement onSearchSubmit or handleSearchChange +
onEnter to trigger search) and wire the Button to an onClick handler (e.g.,
handleImportClick) that calls the existing import/search logic or dispatches the
relevant action; if they are intentionally inactive, mark them disabled
(disabled prop) and add a clear TODO comment explaining future behavior so
intent is explicit.
1 participant
Closes #48
Signed-off-by: LIU BOYU oor2020@163.com
Summary by CodeRabbit
New Features
Documentation
Tests