Feat/datainfra refactor tracker#100
Conversation
- PaperCard: collapsible evidence quotes section below judge scores - SavedTab: export dropdown (BibTeX/RIS/Markdown) triggering browser download - TopicWorkflowDashboard: Download .md button for daily paper reports - SearchResults: add papers_cool and hf_daily to SOURCE_OPTIONS Closes #96
…ion test
- Add GET /api/research/metrics/evidence-coverage endpoint that returns
coverage_rate, total_claims, total_with_evidence, and daily trend data
- Add unit test verifying saved papers rank higher than skipped papers
in list_track_feed
- Enhance POST /api/model-endpoints/{id}/test to return latency_ms,
success, and error fields
- Add unit tests for all new functionality
Closes #92
Closes #95
Closes #97
- Add utils/secret.py with encrypt/decrypt using Fernet (AES-128-CBC + HMAC-SHA256) - Encryption key from PAPERBOT_SECRET_KEY env var, fallback key with warning - model_endpoint_store: encrypt on write, decrypt on read - Graceful migration: non-Fernet values (legacy plaintext) pass through on decrypt - Fix pre-existing test bugs (unquoted f-string path expressions) Closes #77
…gine - Remove paper_searcher parameter from ContextEngine.__init__ - Remove legacy SemanticScholarSearch import and direct API call path - ContextEngine now exclusively uses PaperSearchService for paper search - Fallback: logs warning and returns empty results if no search_service provided - Remove unused PaperMeta import Closes #78
- Replace direct harvester calls (ArxivHarvester, SemanticScholarHarvester, OpenAlexHarvester) with PaperSearchService.search() - PaperSearchService handles dedup and persistence internally, removing the need for separate dedup and store phases - Remove unused imports (HarvestedPaper, HarvestResult, HarvestRunResult, PaperDeduplicator, HarvesterPort, individual harvesters) - Keep pipeline orchestration: keyword expansion, venue recommendation, harvest run records, progress events Closes #79
Delete PapersCoolTopicSearchWorkflow, TopicSearchSourceRegistry, SemanticScholarSearch and their test files. Update all test mocks to patch _run_topic_search / run_unified_topic_search instead of the removed classes. Fix pre-existing SSE event assertions that expected per-paper judge events no longer emitted by EnrichmentPipeline. Closes #80
Rewrite dashboard with clean 2/3+1/3 grid (StatsBar, ActivityTimeline, LLMUsageChart, SavedPapers, DeadlineRadar), delete 9 deprecated components. Rebuild settings page with ccswitch-style provider cards and Dialog form. Add KeychainStore for macOS Keychain API key storage with DB encryption fallback. Add usage proxy route.
refactor: dashboard/settings redesign + keychain API key storage
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis PR consolidates the search architecture by removing topic search workflows and semantic scholar utilities, integrating a unified PaperSearchService into harvest and context pipelines. It adds system keychain support for API key storage, introduces evidence quote capture in paper judgments, expands the research API with metrics and export endpoints, and redesigns the frontend dashboard with updated data structures and component composition. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ 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 @jerry609, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a comprehensive set of improvements focused on strengthening the application's data infrastructure, enhancing security, and refining the user experience. It streamlines paper discovery and management through a unified search service, introduces robust API key handling, and provides new tools for analyzing and exporting research data. The user interface has also been modernized for better clarity and functionality. 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
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the web dashboard/activity feed and the backend search/harvest + provider configuration plumbing, adding export + evidence/metrics capabilities and removing legacy topic-search/search utilities.
Changes:
- Replace the dashboard “feed” cards with a simplified ActivityTimeline + SavedPapers + StatsBar layout and update related types/API helpers.
- Add paper export (BibTeX/RIS/Markdown) and evidence coverage metrics endpoints; extend paper judge output with evidence quotes.
- Add encrypted/keychain-backed model provider API key storage and update settings UI; migrate harvest pipeline/context engine to unified
PaperSearchService.
Reviewed changes
Copilot reviewed 50 out of 50 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/lib/types.ts | Extend Paper.status; replace legacy Activity with TimelineItem; add SavedPaper type; remove pipeline/queue types. |
| web/src/lib/api.ts | Update dashboard data fetchers (fetchActivities, fetchSavedPapers); make LLM usage fetch SSR-compatible via Next proxy. |
| web/src/components/research/TopicWorkflowDashboard.tsx | Add markdown report download button. |
| web/src/components/research/SearchResults.tsx | Add new search source options. |
| web/src/components/research/SavedTab.tsx | Add export dropdown (BibTeX/RIS/Markdown) for saved papers. |
| web/src/components/research/PaperCard.tsx | Render judge evidence quotes section in paper cards. |
| web/src/components/dashboard/feed/NewPaperCard.tsx | Remove legacy feed card. |
| web/src/components/dashboard/feed/MilestoneCard.tsx | Remove legacy feed card. |
| web/src/components/dashboard/feed/ConferenceCard.tsx | Remove legacy feed card. |
| web/src/components/dashboard/StatsCard.tsx | Remove legacy stats card component. |
| web/src/components/dashboard/StatsBar.tsx | Add compact stats bar component for dashboard header. |
| web/src/components/dashboard/SavedPapers.tsx | Add “Saved Papers” dashboard card. |
| web/src/components/dashboard/ReadingQueue.tsx | Remove legacy reading queue card. |
| web/src/components/dashboard/QuickFilters.tsx | Remove legacy quick filters card. |
| web/src/components/dashboard/QuickActions.tsx | Remove legacy quick actions card. |
| web/src/components/dashboard/PipelineStatus.tsx | Remove legacy pipeline status card. |
| web/src/components/dashboard/LLMUsageChart.tsx | Compact chart header and add “no data” empty state. |
| web/src/components/dashboard/DeadlineRadar.tsx | Compact layout, limit items shown, tweak track badges/links. |
| web/src/components/dashboard/ActivityTimeline.tsx | Add “Recent Activity” timeline card. |
| web/src/components/dashboard/ActivityFeed.tsx | Remove legacy activity feed container. |
| web/src/app/settings/page.tsx | Redesign provider settings UI around cards + add/edit dialog; surface key presence/source. |
| web/src/app/dashboard/page.tsx | Rebuild dashboard page using new components and new fetchers. |
| web/src/app/api/model-endpoints/usage/route.ts | Add Next.js proxy route for usage endpoint. |
| tests/unit/test_topic_search_sources.py | Remove tests for deleted legacy topic source registry. |
| tests/unit/test_paperscool_topic_search.py | Remove tests for deleted legacy papers.cool workflow. |
| tests/unit/test_paperscool_route.py | Update route tests to monkeypatch async unified search runner; adjust SSE expectations. |
| tests/unit/test_paperscool_cli.py | Update CLI tests to mock unified topic search entrypoint. |
| tests/unit/test_paper_judge.py | Add coverage for parsing/serializing evidence_quotes. |
| tests/unit/test_model_endpoint_store.py | Fix sqlite tmp_path quoting. |
| tests/unit/test_feed_ranking.py | Add test asserting saved papers rank above skipped papers. |
| tests/unit/test_export.py | Add unit tests for BibTeX/RIS/Markdown export helpers. |
| tests/unit/test_evidence_coverage_route.py | Add test for evidence coverage metrics endpoint. |
| tests/unit/test_connection_test_endpoint.py | Add tests for provider connection test endpoint behavior. |
| tests/unit/test_arq_daily_papers.py | Update job test to mock unified topic search. |
| tests/test_framework.py | Remove tests for deleted tools.search module. |
| src/paperbot/utils/secret.py | Add Fernet-based encryption helpers for secrets at rest. |
| src/paperbot/utils/search.py | Remove legacy Semantic Scholar search utility module. |
| src/paperbot/infrastructure/stores/paper_store.py | Fix library sorting key to use timestamp directly. |
| src/paperbot/infrastructure/stores/model_endpoint_store.py | Store API keys in keychain when available, else encrypted in DB; expose key source. |
| src/paperbot/infrastructure/stores/keychain.py | Add keyring-based keychain abstraction. |
| src/paperbot/context_engine/engine.py | Remove legacy S2 search fallback; rely on injected search_service. |
| src/paperbot/application/workflows/topic_search_sources.py | Remove legacy topic search source registry. |
| src/paperbot/application/workflows/paperscool_topic_search.py | Remove legacy papers.cool topic search workflow. |
| src/paperbot/application/workflows/harvest_pipeline.py | Switch harvesting to PaperSearchService unified search path. |
| src/paperbot/application/workflows/analysis/paper_judge.py | Add evidence_quotes parsing and serialization. |
| src/paperbot/application/workflows/analysis/judge_prompts.py | Update judge prompt schema to request evidence quotes. |
| src/paperbot/api/routes/research.py | Add evidence coverage endpoint + paper export endpoint and helpers. |
| src/paperbot/api/routes/paperscool.py | Count evidence quotes as evidence in metrics. |
| src/paperbot/api/routes/model_endpoints.py | Add latency/success fields to connection test response. |
| pyproject.toml | Add cryptography and keyring dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const blob = new Blob([dailyResult.markdown || ""], { type: "text/markdown" }) | ||
| const url = URL.createObjectURL(blob) | ||
| const a = document.createElement("a") | ||
| a.href = url | ||
| a.download = `dailypaper-${dailyResult.report.date || "report"}.md` | ||
| a.click() | ||
| URL.revokeObjectURL(url) | ||
| }} |
There was a problem hiding this comment.
The blob download revokes the object URL immediately after a.click(). This can intermittently cancel downloads in some browsers; revoke the URL asynchronously (e.g., in setTimeout) after the click is triggered.
| elif not api_key_text.startswith("***"): | ||
| row.api_key_value = api_key_text | ||
| if KeychainStore.store_key(name, api_key_text): | ||
| row.api_key_value = _KEYCHAIN_MARKER | ||
| else: | ||
| row.api_key_value = _encrypt_secret(api_key_text) |
There was a problem hiding this comment.
When storing a new API key, KeychainStore.store_key(name, …) uses the updated name. If the endpoint was renamed, the old keychain secret under the previous name will remain orphaned (and could be used unintentionally if a provider is recreated with the old name). Consider deleting/migrating the old keychain entry when row.name changes.
| Logger.warning( | ||
| "No search_service provided — skipping paper search. " | ||
| "Pass a PaperSearchService instance to ContextEngine.", | ||
| file=LogFiles.HARVEST, | ||
| ) |
There was a problem hiding this comment.
If search_service is None, the engine now silently skips paper search and returns no recommendations. However, the API layer still logs that it will “fallback to legacy S2 path” when PaperSearchService init fails. Either restore a real fallback here (e.g., legacy Semantic Scholar search) or propagate an explicit error so the caller/UI can handle degraded mode intentionally.
| search_result = await self.search_service.search( | ||
| search_query, | ||
| sources=sources, | ||
| max_results=config.max_results_per_source * len(sources), |
There was a problem hiding this comment.
PaperSearchService.search() forwards max_results to each adapter. Passing config.max_results_per_source * len(sources) means each source may now fetch far more than max_results_per_source, increasing latency and load. To preserve the config semantics, pass config.max_results_per_source to the service (or extend the service API to accept per-source limits).
| max_results=config.max_results_per_source * len(sources), | |
| max_results=config.max_results_per_source, |
| papers_found = search_result.total_raw | ||
| papers_new = len(search_result.papers) | ||
| deduplicated_count = search_result.duplicates_removed |
There was a problem hiding this comment.
papers_new is set to len(search_result.papers), which is “unique returned” (and also capped by max_results), not “newly created rows” as in the previous implementation. If HarvestRunModel.papers_new is used as “created count”, this will misreport; consider tracking created/updated counts during the persist loop and storing created as papers_new (or rename the field/variable to reflect the new meaning).
| from datetime import datetime, timezone | ||
| from typing import Any, Dict, List, Optional | ||
|
|
||
| from loguru import logger |
There was a problem hiding this comment.
logger from loguru is imported but not used in this module; please remove it to avoid lint/typecheck failures from unused imports.
| from loguru import logger |
| api_key_text = str(payload.get("api_key") or "").strip() | ||
| if not api_key_text: | ||
| row.api_key_value = None | ||
| KeychainStore.delete_key(name) | ||
| elif not api_key_text.startswith("***"): |
There was a problem hiding this comment.
When clearing an API key (api_key provided but empty), KeychainStore.delete_key(name) uses the new name value. If the endpoint is being renamed in the same update, the old keychain entry under the previous name will be left behind. Capture the previous row.name before overwriting it and delete both old/new names (or rename the keychain entry).
| logger.warning( | ||
| f"{_ENV_KEY} is not set — using deterministic fallback key. " | ||
| "Set this variable in production to secure API keys at rest." | ||
| ) | ||
| key = base64.urlsafe_b64encode( | ||
| hashlib.sha256(b"paperbot-default-insecure-key").digest() | ||
| ) |
There was a problem hiding this comment.
The “deterministic fallback key” means secrets are effectively encrypted with a hard-coded constant when PAPERBOT_SECRET_KEY is unset, which provides little real protection at rest (an attacker with DB access can decrypt across installs). For production safety, consider failing fast when the env var is missing (or generating a random key persisted outside the DB) rather than using a deterministic default.
| papers={stats.new_papers} | ||
| tokens={stats.llm_usage} | ||
| saved={stats.read_later} | ||
| tracks={deadlines.length} |
There was a problem hiding this comment.
StatsBar displays the label “tracks”, but the value passed is deadlines.length (number of upcoming deadlines, not number of research tracks). This will show misleading UI (e.g. “5 tracks” when it’s 5 deadlines); either pass an actual track count or rename the prop/label to reflect what’s being shown.
| tracks={deadlines.length} | |
| tracks={stats.tracked_scholars} |
| const handleExport = async (format: "bibtex" | "ris" | "markdown") => { | ||
| const qs = new URLSearchParams({ format, user_id: userId }) | ||
| if (trackId) qs.set("track_id", String(trackId)) | ||
| try { | ||
| const res = await fetch(`/api/papers/export?${qs.toString()}`) | ||
| if (!res.ok) throw new Error(`${res.status}`) | ||
| const blob = await res.blob() |
There was a problem hiding this comment.
handleExport fetches /api/papers/export, but this PR only adds the FastAPI route at /api/research/papers/export and there is no Next.js proxy route for /api/papers/export (only /api/papers/library etc.). This will 404 and make Export unusable; either add a Next.js route that proxies to the backend export endpoint, or update the client to call the existing /api/research/papers/export proxy (and add that proxy route if missing).
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant refactoring of the data infrastructure, focusing on tracker functionality. Key changes include:
- Secret Management: API keys are now securely stored using
keyringfor system keychain integration andcryptographyfor fallback database encryption. This is a great security enhancement. - Search Service Refactoring: The paper harvesting and search logic has been centralized into a
PaperSearchService, simplifying components likeHarvestPipelineandContextEngine. Several old workflow and search modules have been removed, leading to a cleaner architecture. - New Features:
- Paper export functionality (BibTeX, RIS, Markdown) has been added.
- An evidence coverage metric endpoint is introduced.
- The paper judging mechanism now supports
evidence_quotes.
- UI Overhaul: The dashboard and settings pages have been completely redesigned for a much-improved user experience. The settings page now uses a more intuitive card-based layout with dialogs for editing, and the dashboard is more streamlined.
My feedback focuses on improving API consistency and fixing a minor bug in the frontend's handling of masked keys. Overall, this is a high-quality pull request with substantial improvements across the board.
| ok: bool | ||
| success: bool |
There was a problem hiding this comment.
The EndpointTestResponse model has both ok and success boolean fields, which are redundant. To improve API clarity and consistency, it's best to use a single field to indicate the outcome. I recommend removing the ok field and relying solely on success.
| ok: bool | |
| success: bool | |
| success: bool |
| except Exception as exc: | ||
| latency_ms = int((time.monotonic() - t0) * 1000) | ||
| raise HTTPException(status_code=400, detail=f"test failed: {exc}") from exc |
There was a problem hiding this comment.
The error handling in this except block can be improved for better API consistency. Currently, it calculates latency_ms but this value is lost as an HTTPException is raised. The EndpointTestResponse model is already designed to handle error states with its success and error fields.
I suggest refactoring this to return an EndpointTestResponse with success=False on failure. This provides the client with a consistent response structure, including the latency and a detailed error message, rather than an abrupt HTTP error.
| except Exception as exc: | |
| latency_ms = int((time.monotonic() - t0) * 1000) | |
| raise HTTPException(status_code=400, detail=f"test failed: {exc}") from exc | |
| except Exception as exc: | |
| latency_ms = int((time.monotonic() - t0) * 1000) | |
| return EndpointTestResponse( | |
| ok=False, | |
| success=False, | |
| endpoint_id=endpoint_id, | |
| provider={}, | |
| api_key_present=api_key_present, | |
| latency_ms=latency_ms, | |
| message=f"Test failed: {exc}", | |
| error=str(exc), | |
| ) |
| function maskKey(key?: string): string { | ||
| if (!key) return "" | ||
| if (key.length <= 8) return "****" | ||
| return "****" + key.slice(-4) | ||
| } |
There was a problem hiding this comment.
The maskKey function is redundant and its logic is inconsistent with the backend's masking function. The backend already provides a masked API key for display. Applying another mask on the frontend can lead to confusing output or unintentionally reveal parts of the key.
I recommend removing this maskKey function and directly using the item.api_key value from the backend in the UI. The display logic on line 312 should be simplified to Key: {item.api_key_present ? item.api_key : "missing"}.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Removals