feat(Research): Rebuild -- Research page, Papers library and dashboard#115
Conversation
Closes jerry609#114 Signed-off-by: LIU BOYU <oor2020@163.com>
|
@CJBshuosi is attempting to deploy a commit to the Jerry's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 Walkthrough🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 @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 introduces a comprehensive rebuild of the research page, papers library, and dashboard components. The changes aim to enhance the user experience by providing more dynamic and interactive elements, such as on-demand track data loading, improved paper management with export and related work generation, and a more streamlined research interface. The underlying database schema is also updated to support new paper metadata. 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@web/src/components/dashboard/TrackSpotlightSection.tsx`:
- Around line 63-68: The activation POST is executed after setting activeTrack
(selectedTrack) so a failed activation can leave the client state inconsistent;
modify the logic in TrackSpotlightSection (where activeTrack and selectedTrack
are used) to either include the activation fetch call in the same Promise.all
that fetches feed/anchors so any failure goes to the existing catch, or if you
prefer to keep it separate, catch errors from the activation fetch and revert
activeTrack back to the previous value (or clear it) and surface an error to the
user; locate the activation call to `/api/research/tracks/${trackId}/activate`
and the state setter activeTrack (and any place selectedTrack is assigned) and
update accordingly.
In `@web/src/components/research/ResearchPageNew.tsx`:
- Around line 203-214: The effect is suppressing exhaustive-deps while
referencing handleSearch, risking stale closures and re-entrant searches;
memoize handleSearch with useCallback (including its real deps like query,
searchSources, setResults, etc.), add isSearchingRef (useRef) and mirror
isSearching into it so the debounce callback checks the current flag before
invoking handleSearch, and implement cancellation using an AbortController
inside handleSearch (or a shared ref) so any pending request is aborted when a
new debounced call runs or on cleanup; finally include handleSearch and
isSearching (or the ref) in the effect dependencies instead of disabling eslint.
In `@web/src/components/research/SavedPapersList.tsx`:
- Around line 278-281: The arrow callback in handleExport that builds the query
string uses an expression arrow (selectedIds.forEach((id) =>
qs.append("paper_id", String(id)))) which implicitly returns a value and
triggers a lint warning; change the forEach callback to a block-bodied arrow
function that explicitly performs the append without returning a value (e.g.,
selectedIds.forEach((id) => { qs.append("paper_id", String(id)); })), keeping
the rest of handleExport, qs, and selectedIds unchanged.
- Around line 122-128: The fetch in the useEffect for tracks calls res.json()
without checking res.ok, which can cause parse errors on non-2xx responses;
update the useEffect fetch handler (the anonymous fetch in the useEffect that
currently calls res.json() then setTracks) to first check if res.ok, and if not
handle the error path (e.g., throw or call setTracks([]) and optionally log the
response status/message) before calling res.json(), and ensure the catch block
still sets setTracks([]) for failures.
- Around line 193-201: The current logic compares selectedIds.size to
pagedItems.length which can wrongly mark a page as fully selected; update the
membership checks to verify that every visible item is actually selected.
Replace the isAllSelected expression with a true membership test (e.g.,
pagedItems.every(item => selectedIds.has(item.paper.id))) and update
toggleSelectAll’s condition to use the same membership test when deciding to
clear or select the page; ensure you still call setSelectedIds(new Set(...))
when selecting and setSelectedIds(new Set()) when clearing, and keep appropriate
dependencies for the useCallback on selectedIds and pagedItems.
- Around line 322-327: The handler handleCopyRw currently awaits
navigator.clipboard.writeText(rwMarkdown) with no error handling; wrap the
clipboard call in a try/catch inside handleCopyRw (which already depends on
rwMarkdown) so failures (insecure context or permission denied) are caught, log
or report the error (e.g., console.error or trigger existing UI notification),
and only setRwCopied(true) on success; ensure the catch resets any transient
state or shows a user-facing error instead of leaving the promise unhandled.
🧹 Nitpick comments (4)
web/src/components/research/ResearchPageNew.tsx (1)
73-75: MoveALL_SOURCESoutside the component.This constant is redefined on every render but never depends on props or state. Hoist it to module scope for clarity and to avoid creating a new array reference each render.
♻️ Proposed fix
+const ALL_SOURCES = ["semantic_scholar", "arxiv", "openalex", "papers_cool", "hf_daily"] + export default function ResearchPageNew() { const searchParams = useSearchParams() // User state const [userId] = useState("default") // Track state const [tracks, setTracks] = useState<Track[]>([]) const [activeTrackId, setActiveTrackId] = useState<number | null>(null) - // All available sources - const ALL_SOURCES = ["semantic_scholar", "arxiv", "openalex", "papers_cool", "hf_daily"] - // Search stateweb/src/components/dashboard/TrackSpotlightSection.tsx (1)
25-25:tracksstate is initialized but never updated.
const [tracks] = useState(initialTracks)— without a setter, the track list is frozen at SSR time. If this is intentional (server-driven list, no client mutations), adding a brief comment would help future maintainers.alembic/versions/0012_reconcile_papers_schema.py (2)
100-108: Newfirst_seen_atcolumn added and backfilled — consider adding an index if it will be queried.The column addition and backfill from
created_atare correctly guarded with idempotent helpers. However, unlike other columns added in this migration (e.g.,year,citation_count), no index is created forfirst_seen_at. If downstream queries filter or sort by this column, an index would be beneficial.Add index if needed
_create_index_if_possible("ix_papers_primary_source", "papers", ["primary_source"]) + _create_index_if_possible("ix_papers_first_seen_at", "papers", ["first_seen_at"])And add it to the downgrade as well:
for idx in [ "ix_papers_semantic_scholar_id", ... "ix_papers_primary_source", + "ix_papers_first_seen_at", ]:
136-148: Downgrade doesn't dropfirst_seen_atcolumn nor revert its backfill.The existing pattern intentionally keeps columns for backward compatibility, which is fine for columns that pre-existed. But
first_seen_atis introduced by this migration — a downgrade that leaves it behind could be confusing. Consider documenting this decision in the downgrade docstring or adding a comment.
| // Optionally activate the track on the backend | ||
| await fetch(`/api/research/tracks/${trackId}/activate?user_id=${encodeURIComponent(userId)}`, { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: "{}", | ||
| }) |
There was a problem hiding this comment.
Backend activation failure silently leaves stale activeTrack state.
If the activation POST fails, activeTrack has already been set to selectedTrack (line 38) and feed/anchors are populated. The failure is only logged, not surfaced to the user. More importantly, if the feed/anchor fetches succeed but activation fails, the user sees the correct data but the backend's "active" flag is out of sync — which could cause a different track to appear active on page reload.
Consider either moving the activation call into the Promise.all so its failure triggers the catch block, or reverting activeTrack on activation failure.
🤖 Prompt for AI Agents
In `@web/src/components/dashboard/TrackSpotlightSection.tsx` around lines 63 - 68,
The activation POST is executed after setting activeTrack (selectedTrack) so a
failed activation can leave the client state inconsistent; modify the logic in
TrackSpotlightSection (where activeTrack and selectedTrack are used) to either
include the activation fetch call in the same Promise.all that fetches
feed/anchors so any failure goes to the existing catch, or if you prefer to keep
it separate, catch errors from the activation fetch and revert activeTrack back
to the previous value (or clear it) and surface an error to the user; locate the
activation call to `/api/research/tracks/${trackId}/activate` and the state
setter activeTrack (and any place selectedTrack is assigned) and update
accordingly.
| // Auto-refresh search when sources change (after initial search) | ||
| useEffect(() => { | ||
| if (!hasSearched || !query.trim() || isSearching) return | ||
|
|
||
| // Debounce to avoid rapid re-fetching | ||
| const timer = setTimeout(() => { | ||
| handleSearch() | ||
| }, 300) | ||
|
|
||
| return () => clearTimeout(timer) | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [searchSources]) |
There was a problem hiding this comment.
Auto-refresh effect references handleSearch but suppresses the exhaustive-deps warning — risk of stale closures and re-entrant calls.
handleSearch is not memoized, so the eslint-disable hides a real dependency. Additionally, if the user manually triggers a search while the debounce timer is pending (e.g., toggling a source then immediately pressing Enter), both calls can race and the UI may show results from whichever resolves last, potentially for a different source set.
Consider wrapping handleSearch in useCallback (with its deps) and adding an AbortController so a new search cancels the in-flight one. At minimum, add isSearching as a guard inside the timeout callback by reading it from a ref, since the closure captures a potentially stale value.
Sketch of a safer pattern
+ const abortRef = useRef<AbortController | null>(null)
+
+ const handleSearch = useCallback(async () => {
+ if (!query.trim()) return
+ abortRef.current?.abort()
+ const controller = new AbortController()
+ abortRef.current = controller
+
+ setIsSearching(true)
+ setHasSearched(true)
+ setError(null)
+ try {
+ const body = { /* ... */ }
+ const data = await fetchJson<{ context_pack: ContextPack }>(
+ `/api/research/context`,
+ { method: "POST", body: JSON.stringify(body), headers: { "Content-Type": "application/json" }, signal: controller.signal }
+ )
+ if (!controller.signal.aborted) setContextPack(data.context_pack)
+ } catch (e) {
+ if (!controller.signal.aborted) setError(String(e))
+ } finally {
+ if (!controller.signal.aborted) setIsSearching(false)
+ }
+ }, [query, searchSources, /* other deps */])
useEffect(() => {
if (!hasSearched || !query.trim() || isSearching) return
const timer = setTimeout(() => {
handleSearch()
}, 300)
return () => clearTimeout(timer)
- // eslint-disable-next-line react-hooks/exhaustive-deps
- }, [searchSources])
+ }, [searchSources, handleSearch])🤖 Prompt for AI Agents
In `@web/src/components/research/ResearchPageNew.tsx` around lines 203 - 214, The
effect is suppressing exhaustive-deps while referencing handleSearch, risking
stale closures and re-entrant searches; memoize handleSearch with useCallback
(including its real deps like query, searchSources, setResults, etc.), add
isSearchingRef (useRef) and mirror isSearching into it so the debounce callback
checks the current flag before invoking handleSearch, and implement cancellation
using an AbortController inside handleSearch (or a shared ref) so any pending
request is aborted when a new debounced call runs or on cleanup; finally include
handleSearch and isSearching (or the ref) in the effect dependencies instead of
disabling eslint.
| // Fetch tracks on mount | ||
| useEffect(() => { | ||
| fetch("/api/research/tracks?user_id=default") | ||
| .then((res) => res.json()) | ||
| .then((data) => setTracks(data.tracks || [])) | ||
| .catch(() => setTracks([])) | ||
| }, []) |
There was a problem hiding this comment.
Track fetch doesn't check res.ok before parsing JSON.
If the API returns a non-2xx status, res.json() may throw a confusing parse error instead of a handled failure.
Proposed fix
useEffect(() => {
fetch("/api/research/tracks?user_id=default")
- .then((res) => res.json())
+ .then((res) => (res.ok ? res.json() : Promise.reject(new Error(`${res.status}`))))
.then((data) => setTracks(data.tracks || []))
.catch(() => setTracks([]))
}, [])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Fetch tracks on mount | |
| useEffect(() => { | |
| fetch("/api/research/tracks?user_id=default") | |
| .then((res) => res.json()) | |
| .then((data) => setTracks(data.tracks || [])) | |
| .catch(() => setTracks([])) | |
| }, []) | |
| // Fetch tracks on mount | |
| useEffect(() => { | |
| fetch("/api/research/tracks?user_id=default") | |
| .then((res) => (res.ok ? res.json() : Promise.reject(new Error(`${res.status}`)))) | |
| .then((data) => setTracks(data.tracks || [])) | |
| .catch(() => setTracks([])) | |
| }, []) |
🤖 Prompt for AI Agents
In `@web/src/components/research/SavedPapersList.tsx` around lines 122 - 128, The
fetch in the useEffect for tracks calls res.json() without checking res.ok,
which can cause parse errors on non-2xx responses; update the useEffect fetch
handler (the anonymous fetch in the useEffect that currently calls res.json()
then setTracks) to first check if res.ok, and if not handle the error path
(e.g., throw or call setTracks([]) and optionally log the response
status/message) before calling res.json(), and ensure the catch block still sets
setTracks([]) for failures.
| const toggleSelectAll = useCallback(() => { | ||
| if (selectedIds.size === pagedItems.length && pagedItems.length > 0) { | ||
| setSelectedIds(new Set()) | ||
| } else { | ||
| setSelectedIds(new Set(pagedItems.map((item) => item.paper.id))) | ||
| } | ||
| }, [selectedIds.size, pagedItems]) | ||
|
|
||
| const isAllSelected = pagedItems.length > 0 && selectedIds.size === pagedItems.length |
There was a problem hiding this comment.
isAllSelected uses size comparison instead of membership check — can show a false "all selected" state.
If the user selects N items on page 1, then navigates to page 2 which happens to also have N items, selectedIds.size === pagedItems.length evaluates to true even though none of page 2's items are in selectedIds. The "select all" checkbox would appear checked when no visible rows are actually selected.
Check actual membership instead of comparing counts:
🐛 Proposed fix
- const isAllSelected = pagedItems.length > 0 && selectedIds.size === pagedItems.length
+ const isAllSelected = pagedItems.length > 0 && pagedItems.every((item) => selectedIds.has(item.paper.id))🤖 Prompt for AI Agents
In `@web/src/components/research/SavedPapersList.tsx` around lines 193 - 201, The
current logic compares selectedIds.size to pagedItems.length which can wrongly
mark a page as fully selected; update the membership checks to verify that every
visible item is actually selected. Replace the isAllSelected expression with a
true membership test (e.g., pagedItems.every(item =>
selectedIds.has(item.paper.id))) and update toggleSelectAll’s condition to use
the same membership test when deciding to clear or select the page; ensure you
still call setSelectedIds(new Set(...)) when selecting and setSelectedIds(new
Set()) when clearing, and keep appropriate dependencies for the useCallback on
selectedIds and pagedItems.
| const handleExport = useCallback(async (format: "bibtex" | "ris" | "markdown" | "csl_json") => { | ||
| const qs = new URLSearchParams({ format, user_id: "default" }) | ||
| // Add selected paper IDs | ||
| selectedIds.forEach((id) => qs.append("paper_id", String(id))) |
There was a problem hiding this comment.
Arrow function in forEach implicitly returns a value (static analysis flag).
(id) => qs.append(...) implicitly returns the result of qs.append(). While harmless, wrapping in braces silences the lint error cleanly.
Proposed fix
- selectedIds.forEach((id) => qs.append("paper_id", String(id)))
+ selectedIds.forEach((id) => { qs.append("paper_id", String(id)) })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleExport = useCallback(async (format: "bibtex" | "ris" | "markdown" | "csl_json") => { | |
| const qs = new URLSearchParams({ format, user_id: "default" }) | |
| // Add selected paper IDs | |
| selectedIds.forEach((id) => qs.append("paper_id", String(id))) | |
| const handleExport = useCallback(async (format: "bibtex" | "ris" | "markdown" | "csl_json") => { | |
| const qs = new URLSearchParams({ format, user_id: "default" }) | |
| // Add selected paper IDs | |
| selectedIds.forEach((id) => { qs.append("paper_id", String(id)) }) |
🧰 Tools
🪛 Biome (2.3.14)
[error] 281-281: This callback passed to forEach() iterable method should not return a value.
Either remove this return or remove the returned value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
In `@web/src/components/research/SavedPapersList.tsx` around lines 278 - 281, The
arrow callback in handleExport that builds the query string uses an expression
arrow (selectedIds.forEach((id) => qs.append("paper_id", String(id)))) which
implicitly returns a value and triggers a lint warning; change the forEach
callback to a block-bodied arrow function that explicitly performs the append
without returning a value (e.g., selectedIds.forEach((id) => {
qs.append("paper_id", String(id)); })), keeping the rest of handleExport, qs,
and selectedIds unchanged.
| const handleCopyRw = useCallback(async () => { | ||
| if (!rwMarkdown) return | ||
| await navigator.clipboard.writeText(rwMarkdown) | ||
| setRwCopied(true) | ||
| setTimeout(() => setRwCopied(false), 2000) | ||
| }, [rwMarkdown]) |
There was a problem hiding this comment.
navigator.clipboard.writeText can reject — unhandled promise.
handleCopyRw awaits navigator.clipboard.writeText but has no try/catch. This will throw in insecure contexts (non-HTTPS) or if clipboard permission is denied, leaving the user with no feedback.
Proposed fix
const handleCopyRw = useCallback(async () => {
if (!rwMarkdown) return
- await navigator.clipboard.writeText(rwMarkdown)
- setRwCopied(true)
- setTimeout(() => setRwCopied(false), 2000)
+ try {
+ await navigator.clipboard.writeText(rwMarkdown)
+ setRwCopied(true)
+ setTimeout(() => setRwCopied(false), 2000)
+ } catch {
+ setError("Failed to copy to clipboard")
+ }
}, [rwMarkdown])🤖 Prompt for AI Agents
In `@web/src/components/research/SavedPapersList.tsx` around lines 322 - 327, The
handler handleCopyRw currently awaits navigator.clipboard.writeText(rwMarkdown)
with no error handling; wrap the clipboard call in a try/catch inside
handleCopyRw (which already depends on rwMarkdown) so failures (insecure context
or permission denied) are caught, log or report the error (e.g., console.error
or trigger existing UI notification), and only setRwCopied(true) on success;
ensure the catch resets any transient state or shows a user-facing error instead
of leaving the promise unhandled.
![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 significant refactoring and new features across several frontend components and a database migration. The database migration adds a first_seen_at column to the papers table and backfills it from created_at. On the frontend, the TrackSpotlight component has been converted to a client component, renamed to TrackSpotlightSection, and refactored to manage its own state for tracks, feed items, and anchors, including a new dropdown for selecting active tracks. The ResearchPageNew component has undergone a major overhaul, removing the tab-based navigation for 'Feed' and 'Saved' sections, eliminating the dedicated 'Anchor Authors' card, and replacing the 'Memory' tab with a Sheet (drawer) component. The SearchBox component now includes controls for toggling 'Personalized'/'Global' anchor mode and opening the 'Track Memory' drawer. The SavedPapersList component has been enhanced with new features such as paper selection via checkboxes, track filtering, and the ability to export selected papers in various formats (BibTeX, RIS, Markdown, CSL-JSON). It also introduces a new 'Related Work' dialog that allows users to generate a related work section based on a topic from their saved papers. Review comments highlight the need to add cache: "no-store" to all client-side API fetch calls to prevent stale data, address a hardcoded userId value, and fix an exhaustive-deps warning for the useEffect that debounces search. Additionally, a suggestion was made to improve error handling in the 'Related Work' dialog by using a dedicated error state instead of displaying error messages within the markdown output area.
| const [feedRes, anchorsRes] = await Promise.all([ | ||
| fetch(`/api/research/tracks/${trackId}/feed?user_id=${encodeURIComponent(userId)}&limit=6`), | ||
| fetch(`/api/research/tracks/${trackId}/anchors/discover?user_id=${encodeURIComponent(userId)}&limit=4`), | ||
| ]) |
There was a problem hiding this comment.
The fetch calls to the API are missing the cache: "no-store" option. This can lead to the browser serving stale data from its cache, which is undesirable for a dynamic dashboard. Other parts of the application, like in web/src/lib/api.ts, consistently use this option to ensure data freshness. I recommend adding it to all fetch calls in this component, including the one to activate the track.
| const [feedRes, anchorsRes] = await Promise.all([ | |
| fetch(`/api/research/tracks/${trackId}/feed?user_id=${encodeURIComponent(userId)}&limit=6`), | |
| fetch(`/api/research/tracks/${trackId}/anchors/discover?user_id=${encodeURIComponent(userId)}&limit=4`), | |
| ]) | |
| const [feedRes, anchorsRes] = await Promise.all([ | |
| fetch(`/api/research/tracks/${trackId}/feed?user_id=${encodeURIComponent(userId)}&limit=6`, { cache: "no-store" }), | |
| fetch(`/api/research/tracks/${trackId}/anchors/discover?user_id=${encodeURIComponent(userId)}&limit=4`, { cache: "no-store" }), | |
| ]) |
| useEffect(() => { | ||
| fetch("/api/research/tracks?user_id=default") | ||
| .then((res) => res.json()) | ||
| .then((data) => setTracks(data.tracks || [])) | ||
| .catch(() => setTracks([])) | ||
| }, []) |
There was a problem hiding this comment.
This fetch call is missing the cache: "no-store" option, which can lead to stale data being displayed. This is a recurring issue across multiple new fetch calls in this file (e.g., in loadSavedPapers, handleExport, handleGenerateRelatedWork). To ensure data freshness, please add { cache: "no-store" } to all client-side API fetches.
| useEffect(() => { | |
| fetch("/api/research/tracks?user_id=default") | |
| .then((res) => res.json()) | |
| .then((data) => setTracks(data.tracks || [])) | |
| .catch(() => setTracks([])) | |
| }, []) | |
| useEffect(() => { | |
| fetch("/api/research/tracks?user_id=default", { cache: "no-store" }) | |
| .then((res) => res.json()) | |
| .then((data) => setTracks(data.tracks || [])) | |
| .catch(() => setTracks([])) | |
| }, []) |
| initialFeedItems={feedItems} | ||
| initialFeedTotal={feedTotal} | ||
| initialAnchors={anchors} | ||
| userId="default" |
There was a problem hiding this comment.
The userId is hardcoded as "default". While this might be acceptable for initial development or a single-user context, it will need to be replaced with a dynamic value from an authentication context or session when multi-user support is implemented. This hardcoded value appears in several places in this PR.
| useEffect(() => { | ||
| if (!hasSearched || !query.trim() || isSearching) return | ||
|
|
||
| // Debounce to avoid rapid re-fetching | ||
| const timer = setTimeout(() => { | ||
| handleSearch() | ||
| }, 300) | ||
|
|
||
| return () => clearTimeout(timer) | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [searchSources]) |
There was a problem hiding this comment.
This useEffect has a disabled exhaustive-deps warning because handleSearch is not in its dependency array. This can lead to bugs with stale closures and makes the code brittle.
To fix this and improve the code's robustness, you should:
- Wrap the
handleSearchfunction inuseCallbackwith its dependencies (e.g.,query,userId,searchSources). - Add
handleSearchto thisuseEffect's dependency array and remove theeslint-disablecomment.
This change will also improve the user experience by triggering a debounced search as the user types, which is a common and helpful pattern.
useEffect(() => {
if (!hasSearched || !query.trim() || isSearching) return
// Debounce to avoid rapid re-fetching
const timer = setTimeout(() => {
handleSearch()
}, 300)
return () => clearTimeout(timer)
}, [searchSources, handleSearch])
| } catch { | ||
| setRwMarkdown("Failed to generate related work. Please try again.") | ||
| } finally { |
There was a problem hiding this comment.
In the catch block for handleGenerateRelatedWork, the error message is being set into rwMarkdown. This will render the error text inside the <pre> block intended for markdown content, which is not ideal for user experience. It would be better to use a dedicated state variable for errors within the dialog and display it as a proper error message (e.g., using an Alert component).
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Closes jerry609#114 Signed-off-by: LIU BOYU <oor2020@163.com>
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 140-142: The current guard in SavedPapersList.tsx uses a falsy
check (if (selectedTrackId)) which incorrectly skips valid ID 0; change the
condition around qs.set("track_id", String(selectedTrackId)) to explicitly check
for null/undefined (e.g., selectedTrackId != null or selectedTrackId !== null &&
selectedTrackId !== undefined) so a track with id === 0 is included while still
excluding missing values.
- Around line 553-558: The Enter key handler on the Input triggers
handleGenerateRelatedWork even while IME composition is active; update the
onKeyDown handler to ignore Enter when composing by checking e.isComposing or
e.nativeEvent.isComposing before calling handleGenerateRelatedWork. Locate the
Input with props value={rwTopic}, onChange={(e) => setRwTopic(e.target.value)},
onKeyDown={(e) => { if (e.key === "Enter") handleGenerateRelatedWork() }} and
change onKeyDown to first return if e.isComposing || (e.nativeEvent &&
(e.nativeEvent as any).isComposing), otherwise proceed to call
handleGenerateRelatedWork when e.key === "Enter" and not rwLoading.
🧹 Nitpick comments (1)
web/src/components/research/SavedPapersList.tsx (1)
130-163: Loading all papers client-side (limit=500) to paginate in the browser.The query hard-codes
limit: "500"and pages entirely on the client. This is fine for small libraries, but if the paper count grows significantly, this will degrade both network and rendering performance. Consider noting this as a known scaling limitation or switching to server-side pagination in a follow-up.
| if (selectedTrackId) { | ||
| qs.set("track_id", String(selectedTrackId)) | ||
| } |
There was a problem hiding this comment.
if (selectedTrackId) is falsy — a track with id === 0 would be silently ignored.
While database IDs are typically ≥ 1, the safer check is an explicit null comparison.
Proposed fix
- if (selectedTrackId) {
+ if (selectedTrackId !== null) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (selectedTrackId) { | |
| qs.set("track_id", String(selectedTrackId)) | |
| } | |
| if (selectedTrackId !== null) { | |
| qs.set("track_id", String(selectedTrackId)) | |
| } |
🤖 Prompt for AI Agents
In `@web/src/components/research/SavedPapersList.tsx` around lines 140 - 142, The
current guard in SavedPapersList.tsx uses a falsy check (if (selectedTrackId))
which incorrectly skips valid ID 0; change the condition around
qs.set("track_id", String(selectedTrackId)) to explicitly check for
null/undefined (e.g., selectedTrackId != null or selectedTrackId !== null &&
selectedTrackId !== undefined) so a track with id === 0 is included while still
excluding missing values.
| <Input | ||
| placeholder="Enter research topic..." | ||
| value={rwTopic} | ||
| onChange={(e) => setRwTopic(e.target.value)} | ||
| onKeyDown={(e) => { if (e.key === "Enter") handleGenerateRelatedWork() }} | ||
| disabled={rwLoading} |
There was a problem hiding this comment.
Enter key handler doesn't guard against IME composition events.
When using CJK (Chinese/Japanese/Korean) input methods, pressing Enter to confirm a composition candidate will also fire keyDown with key === "Enter", inadvertently triggering handleGenerateRelatedWork mid-composition. Guard with e.isComposing (or e.nativeEvent.isComposing).
Proposed fix
- onKeyDown={(e) => { if (e.key === "Enter") handleGenerateRelatedWork() }}
+ onKeyDown={(e) => { if (e.key === "Enter" && !e.nativeEvent.isComposing) handleGenerateRelatedWork() }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Input | |
| placeholder="Enter research topic..." | |
| value={rwTopic} | |
| onChange={(e) => setRwTopic(e.target.value)} | |
| onKeyDown={(e) => { if (e.key === "Enter") handleGenerateRelatedWork() }} | |
| disabled={rwLoading} | |
| <Input | |
| placeholder="Enter research topic..." | |
| value={rwTopic} | |
| onChange={(e) => setRwTopic(e.target.value)} | |
| onKeyDown={(e) => { if (e.key === "Enter" && !e.nativeEvent.isComposing) handleGenerateRelatedWork() }} | |
| disabled={rwLoading} |
🤖 Prompt for AI Agents
In `@web/src/components/research/SavedPapersList.tsx` around lines 553 - 558, The
Enter key handler on the Input triggers handleGenerateRelatedWork even while IME
composition is active; update the onKeyDown handler to ignore Enter when
composing by checking e.isComposing or e.nativeEvent.isComposing before calling
handleGenerateRelatedWork. Locate the Input with props value={rwTopic},
onChange={(e) => setRwTopic(e.target.value)}, onKeyDown={(e) => { if (e.key ===
"Enter") handleGenerateRelatedWork() }} and change onKeyDown to first return if
e.isComposing || (e.nativeEvent && (e.nativeEvent as any).isComposing),
otherwise proceed to call handleGenerateRelatedWork when e.key === "Enter" and
not rwLoading.
1 participant
Closes #114
Summary by CodeRabbit