bugfix: fix searching network errors#10
Conversation
|
@copilot review |
|
@Dynamite2003 I've opened a new pull request, #11, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the academic search functionality by adding comprehensive error handling for network requests across multiple data sources. The changes introduce support for a new OpenAlex provider, improve timeout and SSL configuration handling, display source-specific errors to users, and refactor the UI to include floating filters.
- Added network error handling and reporting for arXiv, Semantic Scholar, and OpenAlex data sources
- Introduced configurable SSL, timeout, and retry settings for external API calls
- Enhanced UI to display source-specific errors and allow multi-source selection
- Refactored filter panel into a draggable floating component
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 22 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/lib/api/academic.ts | Added source_errors and sources_used fields to SearchResponse interface |
| frontend/src/components/academic/FloatingFilters.tsx | New draggable floating filter component with position persistence |
| frontend/src/components/academic/FilterPanel.tsx | Refactored filter panel with improved styling and NaN handling for year input |
| frontend/src/components/Recommendations.tsx | Integrated multi-source selection and source error display |
| frontend/src/app/academic/page.tsx | Added comprehensive error handling, source selection UI, and floating filters |
| backend/app/services/academic/search_service.py | Added per-source error tracking, OpenAlex provider integration, and post-query filtering |
| backend/app/services/academic/providers/semantic_scholar.py | Added timeout handling, SSL configuration, and improved error messages |
| backend/app/services/academic/providers/openalex.py | New OpenAlex API provider with inverted index abstract reconstruction |
| backend/app/services/academic/providers/arxiv.py | Enhanced with retry logic, exponential backoff, SSL configuration, and improved query tokenization |
| backend/app/schemas/academic.py | Added sources_used and source_errors fields to response schema |
| backend/app/core/config.py | Added SSL, timeout, and API configuration settings for all providers |
| backend/app/api/v1/arxiv.py | Reduced MAX_TOTAL_CANDIDATES from 200 to 150 |
Comments suppressed due to low confidence (1)
backend/app/services/academic/providers/arxiv.py:443
- This import of module re is redundant, as it was previously imported on line 9.
import re
| const handleToggleSource = (sourceValue: string) => { | ||
| setSelectedSources((prev) => { | ||
| if (prev.includes(sourceValue) && prev.length === 1) { | ||
| return prev; | ||
| } | ||
| const nextSources = prev.includes(sourceValue) | ||
| ? prev.filter((value) => value !== sourceValue) | ||
| : [...prev, sourceValue]; | ||
|
|
||
| if (mode === "standard" && lastQuery) { | ||
| void runStandardSearch(lastQuery, appliedFilters, nextSources); | ||
| } | ||
|
|
||
| return nextSources; | ||
| }); | ||
| }; |
There was a problem hiding this comment.
[nitpick] Similar race condition issue exists here. The handleToggleSource function (lines 151-166) immediately triggers a search when sources change. If multiple rapid clicks occur or if the search is slow, this could result in multiple concurrent searches with different source configurations, and the results displayed may not match the currently selected sources. Consider tracking the active search and canceling previous ones, or debouncing the source toggle.
| @staticmethod | ||
| def _apply_request_filters(papers: List[Dict], request: PaperSearchRequest) -> List[Dict]: | ||
| """Apply year/venue filters not handled directly by the provider query.""" | ||
| filtered = papers | ||
|
|
||
| if request.year: | ||
| filtered = [paper for paper in filtered if paper.get('year') == request.year] | ||
|
|
||
| if request.venue: | ||
| target = request.venue.lower().strip() | ||
| if target: | ||
| filtered = [ | ||
| paper for paper in filtered | ||
| if AcademicSearchService._paper_matches_venue(paper, target) | ||
| ] | ||
|
|
||
| return filtered |
There was a problem hiding this comment.
Missing documentation for the _apply_request_filters method. Since this is a new static method that performs post-processing filtering, it should have a docstring explaining its purpose, parameters, and return value, especially since other methods in the class have docstrings.
| OPENALEX_PER_PAGE: int = 25 | ||
| OPENALEX_MAX_PER_PAGE: int = 100 | ||
| OPENALEX_TRUST_ENV: bool = True | ||
| OPENALEX_MAILTO: str = "insightreading@example.com" |
There was a problem hiding this comment.
The email address "insightreading@example.com" in the OPENALEX_MAILTO configuration appears to be a placeholder using the example.com domain. OpenAlex uses this email to identify API users and may throttle or block example.com addresses. This should be replaced with a real contact email address before deployment.
| OPENALEX_MAILTO: str = "insightreading@example.com" | |
| OPENALEX_MAILTO: str = "tech@insightreading.local" # TODO: Set to a real contact email in production via env var |
| e.preventDefault(); | ||
| const form = e.currentTarget.form; | ||
| if (form) { | ||
| handleSubmit({ preventDefault: () => {}, currentTarget: form } as FormEvent<HTMLFormElement>); |
There was a problem hiding this comment.
[nitpick] The keyboard shortcut handling in line 363-368 creates a new form submission event by manually constructing an object with preventDefault and currentTarget. This is a workaround that bypasses normal form validation and event handling. A better approach would be to call form.requestSubmit() if available, or to extract the submission logic into a separate function that can be called from both the form's onSubmit and the keyboard handler.
| handleSubmit({ preventDefault: () => {}, currentTarget: form } as FormEvent<HTMLFormElement>); | |
| if (typeof form.requestSubmit === "function") { | |
| form.requestSubmit(); | |
| } else { | |
| form.submit(); | |
| } |
| total: int | ||
| query_interpretation: Optional[dict] = None | ||
| sources_used: Optional[List[str]] = None | ||
| source_errors: Optional[dict[str, str]] = None |
There was a problem hiding this comment.
[nitpick] Type annotation uses dict[str, str] (PEP 585 style) on line 41 but Dict from typing module is used elsewhere in the codebase (line 2, other files). While both work in Python 3.9+, mixing styles reduces consistency. Consider standardizing on one approach throughout the codebase.
| <div | ||
| ref={containerRef} | ||
| className={`fixed z-40 w-64 overflow-hidden rounded-2xl border border-white/40 bg-white/90 shadow-xl backdrop-blur-xl transition dark:border-slate-700/50 dark:bg-slate-900/80 ${ | ||
| isDragging ? 'cursor-grabbing' : '' |
There was a problem hiding this comment.
The cursor style classes are inconsistent. Line 175 sets cursor-grab and cursor-grabbing for the collapsed state, but line 191 only sets cursor-grabbing for the expanded state, missing cursor-grab when not dragging. This creates an inconsistent UX where the expanded filter doesn't show the grab cursor on hover. Consider adding cursor-grab to line 191 when !isDragging.
| isDragging ? 'cursor-grabbing' : '' | |
| isDragging ? 'cursor-grabbing' : 'cursor-grab' |
| > | ||
| <div | ||
| className="flex items-center justify-between border-b border-white/30 px-3 py-2.5 text-xs font-semibold text-slate-700 dark:border-slate-700/60 dark:text-slate-100" | ||
| onMouseDown={handleMouseDown} |
There was a problem hiding this comment.
[nitpick] The drag handle area on line 197 only covers the header, but users might expect to drag from anywhere in the filter panel. Consider making this more discoverable or extending the draggable area, especially since the entire collapsed button is draggable (line 177) but only the header is draggable when expanded.
| > | |
| <div | |
| className="flex items-center justify-between border-b border-white/30 px-3 py-2.5 text-xs font-semibold text-slate-700 dark:border-slate-700/60 dark:text-slate-100" | |
| onMouseDown={handleMouseDown} | |
| onMouseDown={handleMouseDown} | |
| > | |
| <div | |
| className="flex items-center justify-between border-b border-white/30 px-3 py-2.5 text-xs font-semibold text-slate-700 dark:border-slate-700/60 dark:text-slate-100" |
| @staticmethod | ||
| def _paper_matches_venue(paper: Dict, target: str) -> bool: | ||
| """Check whether any venue-related field roughly matches the user input.""" | ||
| candidate_fields = [ | ||
| paper.get('venue'), | ||
| paper.get('comment'), | ||
| paper.get('journal_ref'), | ||
| ] | ||
| for field in candidate_fields: | ||
| if field and target in field.lower(): | ||
| return True | ||
| return False |
There was a problem hiding this comment.
Missing documentation for the _paper_matches_venue method. A docstring would help explain what constitutes a "match" and which fields are checked.
| tokens = re.findall(r'"[^"]+"|\S+', query or '') | ||
| parsed: List[Tuple[str, bool]] = [] | ||
| for token in tokens: | ||
| stripped = token.strip() | ||
| if not stripped: | ||
| continue | ||
| if stripped.startswith('"') and stripped.endswith('"') and len(stripped) >= 2: | ||
| parsed.append((stripped[1:-1].strip(), True)) | ||
| else: | ||
| parsed.append((stripped, False)) |
There was a problem hiding this comment.
[nitpick] The method _tokenize_terms uses a regex to split quoted phrases but doesn't handle escaped quotes within phrases (e.g., "term with \" inside"). While this may be acceptable for the current use case, it could lead to unexpected behavior if users try to include literal quotes in their search terms. Consider documenting this limitation or handling escaped quotes.
| tokens = re.findall(r'"[^"]+"|\S+', query or '') | |
| parsed: List[Tuple[str, bool]] = [] | |
| for token in tokens: | |
| stripped = token.strip() | |
| if not stripped: | |
| continue | |
| if stripped.startswith('"') and stripped.endswith('"') and len(stripped) >= 2: | |
| parsed.append((stripped[1:-1].strip(), True)) | |
| else: | |
| parsed.append((stripped, False)) | |
| """ | |
| Tokenize the query string into (term, is_phrase) tuples. | |
| Handles quoted phrases and escaped quotes within phrases. | |
| """ | |
| parsed: List[Tuple[str, bool]] = [] | |
| if not query: | |
| return parsed | |
| i = 0 | |
| n = len(query) | |
| while i < n: | |
| # Skip whitespace | |
| while i < n and query[i].isspace(): | |
| i += 1 | |
| if i >= n: | |
| break | |
| if query[i] == '"': | |
| # Start of quoted phrase | |
| i += 1 | |
| phrase = [] | |
| in_escape = False | |
| while i < n: | |
| c = query[i] | |
| if in_escape: | |
| phrase.append(c) | |
| in_escape = False | |
| elif c == '\\': | |
| in_escape = True | |
| elif c == '"': | |
| i += 1 | |
| break | |
| else: | |
| phrase.append(c) | |
| i += 1 | |
| parsed.append((''.join(phrase).strip(), True)) | |
| else: | |
| # Unquoted token | |
| start = i | |
| while i < n and not query[i].isspace(): | |
| if query[i] == '"': | |
| break | |
| i += 1 | |
| token = query[start:i].strip() | |
| if token: | |
| parsed.append((token, False)) |
| SEMANTIC_SCHOLAR_VERIFY_SSL: bool = True | ||
| SEMANTIC_SCHOLAR_TRUST_ENV: bool = True | ||
| SEMANTIC_SCHOLAR_TIMEOUT: int = 30 | ||
| SEMANTIC_SCHOLAR_USER_AGENT: str = "InsightReading/1.0 (mailto:tech@insightreading.local)" |
There was a problem hiding this comment.
The config setting SEMANTIC_SCHOLAR_USER_AGENT uses "insightreading.local" as the domain in the mailto field. The .local TLD is reserved for local networks and won't work as a contact email. This should be changed to a valid domain (matching the OPENALEX_MAILTO issue) or made configurable via environment variables.
No description provided.