refactor: improve error handling and split large components#3817
Merged
MarkusNeusinger merged 2 commits intomainfrom Jan 12, 2026
Merged
refactor: improve error handling and split large components#3817MarkusNeusinger merged 2 commits intomainfrom
MarkusNeusinger merged 2 commits intomainfrom
Conversation
Backend: - Add DatabaseQueryError and CacheError exceptions for specific error handling - Wrap cache operations with try/except (non-fatal, logged as warnings) - Wrap database queries with SQLAlchemyError handling Frontend: - Split SpecPage.tsx (807→500 lines) into SpecOverview and SpecDetailView components - Extract Breadcrumb as internal sub-component - Split useFilterState.ts (375→258 lines) into useUrlSync and useFilterFetch hooks - Update component and hook exports Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors both backend and frontend code to improve maintainability through better error handling and component/hook separation.
Changes:
- Backend: Added
DatabaseQueryErrorexception type and improved error handling in the plots router with specific try-catch blocks for database and cache operations - Frontend: Split large
SpecPage.tsxcomponent intoSpecOverviewandSpecDetailViewsub-components, and extractedBreadcrumbas an internal component - Frontend: Split
useFilterStatehook by extractinguseUrlSyncanduseFilterFetchhooks for better separation of concerns
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| api/exceptions.py | Added DatabaseQueryError and CacheError exception classes with helper function |
| api/routers/plots.py | Added try-catch blocks for database queries and cache operations with logging |
| app/src/pages/SpecPage.tsx | Refactored to use SpecOverview and SpecDetailView components, extracted Breadcrumb sub-component |
| app/src/components/SpecOverview.tsx | New component for grid view of implementations |
| app/src/components/SpecDetailView.tsx | New component for single implementation detail view |
| app/src/hooks/useFilterState.ts | Refactored to compose useUrlSync and useFilterFetch hooks |
| app/src/hooks/useUrlSync.ts | New hook for URL synchronization with filter state |
| app/src/hooks/useFilterFetch.ts | New hook for fetching filtered plot images |
| app/src/hooks/index.ts | Exports new hooks |
| app/src/components/index.ts | Exports new components |
Comment on lines
+385
to
+500
| // Breadcrumb sub-component | ||
| interface BreadcrumbProps { | ||
| specId: string; | ||
| selectedLibrary: string | null; | ||
| isOverviewMode: boolean; | ||
| reportUrl: string; | ||
| onTrackEvent: (event: string, props?: Record<string, string | undefined>) => void; | ||
| } | ||
|
|
||
| function Breadcrumb({ specId, selectedLibrary, isOverviewMode, reportUrl, onTrackEvent }: BreadcrumbProps) { | ||
| return ( | ||
| <Box | ||
| sx={{ | ||
| display: 'flex', | ||
| justifyContent: 'space-between', | ||
| alignItems: 'center', | ||
| mx: { xs: -2, sm: -4, md: -8, lg: -12 }, | ||
| mt: -5, | ||
| px: 2, | ||
| py: 1, | ||
| mb: 2, | ||
| bgcolor: '#f3f4f6', | ||
| borderBottom: '1px solid #e5e7eb', | ||
| fontFamily: '"MonoLisa", monospace', | ||
| fontSize: '0.85rem', | ||
| }} | ||
| > | ||
| {/* Breadcrumb links */} | ||
| <Box sx={{ display: 'flex', alignItems: 'center' }}> | ||
| <Box | ||
| component={Link} | ||
| to="/" | ||
| sx={{ | ||
| color: '#3776AB', | ||
| textDecoration: 'none', | ||
| '&:hover': { textDecoration: 'underline' }, | ||
| }} | ||
| > | ||
| pyplots.ai | ||
| </Box> | ||
| <Box component="span" sx={{ mx: 1, color: '#9ca3af' }}> | ||
| › | ||
| </Box> | ||
| <Box | ||
| component={Link} | ||
| to="/catalog" | ||
| sx={{ | ||
| color: '#3776AB', | ||
| textDecoration: 'none', | ||
| '&:hover': { textDecoration: 'underline' }, | ||
| }} | ||
| > | ||
| catalog | ||
| </Box> | ||
| <Box component="span" sx={{ mx: 1, color: '#9ca3af' }}> | ||
| › | ||
| </Box> | ||
| {isOverviewMode ? ( | ||
| <Box component="span" sx={{ color: '#4b5563' }}> | ||
| {specId} | ||
| </Box> | ||
| ) : ( | ||
| <> | ||
| <Box | ||
| component={Link} | ||
| to={`/${specId}`} | ||
| sx={{ | ||
| color: '#3776AB', | ||
| textDecoration: 'none', | ||
| '&:hover': { textDecoration: 'underline' }, | ||
| }} | ||
| > | ||
| {specId} | ||
| </Box> | ||
| <Box component="span" sx={{ mx: 1, color: '#9ca3af' }}> | ||
| › | ||
| </Box> | ||
| <Box component="span" sx={{ color: '#4b5563' }}> | ||
| {selectedLibrary} | ||
| </Box> | ||
| </> | ||
| )} | ||
| </Box> | ||
|
|
||
| {/* Report issue link */} | ||
| <Tooltip title="report issue"> | ||
| <Box | ||
| component="a" | ||
| href={reportUrl} | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| onClick={() => onTrackEvent('report_issue', { spec: specId, library: selectedLibrary || undefined })} | ||
| sx={{ | ||
| color: '#9ca3af', | ||
| textDecoration: 'none', | ||
| display: 'flex', | ||
| alignItems: 'center', | ||
| '&:hover': { color: '#3776AB' }, | ||
| }} | ||
| > | ||
| <BugReportIcon sx={{ fontSize: '1.1rem', display: { xs: 'block', md: 'none' } }} /> | ||
| <Box | ||
| component="span" | ||
| sx={{ | ||
| display: { xs: 'none', md: 'block' }, | ||
| fontFamily: '"MonoLisa", monospace', | ||
| fontSize: '0.85rem', | ||
| }} | ||
| > | ||
| report issue | ||
| </Box> | ||
| </Box> | ||
| </Tooltip> | ||
| </Box> | ||
| ); | ||
| } |
There was a problem hiding this comment.
The Breadcrumb sub-component is defined at the bottom of SpecPage.tsx but is not exported. While this is acceptable for internal components, consider whether this component might be reusable in other pages. If it's only used in SpecPage, the current approach is fine. If it might be reused, consider moving it to a separate file in the components directory and exporting it from components/index.ts.
Comment on lines
+20
to
+28
| interface Implementation { | ||
| library_id: string; | ||
| library_name: string; | ||
| preview_url: string; | ||
| preview_thumb?: string; | ||
| preview_html?: string; | ||
| quality_score: number | null; | ||
| code: string | null; | ||
| } |
There was a problem hiding this comment.
The Implementation interface is duplicated across SpecPage.tsx, SpecOverview.tsx, SpecDetailView.tsx, and LibraryPills.tsx with slightly different field requirements. While this provides component-level type safety, consider creating a shared base Implementation type in app/src/types/index.ts and using Pick or intersection types to define component-specific versions. This would reduce duplication and make it easier to maintain consistent field names across components.
Comment on lines
+86
to
+94
| class CacheError(PyplotsException): | ||
| """Cache operation failed (non-fatal, logged only).""" | ||
|
|
||
| def __init__(self, operation: str, detail: str): | ||
| message = f"Cache {operation} failed: {detail}" | ||
| super().__init__(message, status_code=500) | ||
| self.operation = operation | ||
|
|
||
|
|
There was a problem hiding this comment.
The CacheError exception class is defined but never used in the codebase. In the plots router, cache failures are handled with generic Exception catching and logging, but don't raise CacheError. Consider either using this exception type consistently in the cache error handlers, or removing it if it's not needed.
Suggested change
| class CacheError(PyplotsException): | |
| """Cache operation failed (non-fatal, logged only).""" | |
| def __init__(self, operation: str, detail: str): | |
| message = f"Cache {operation} failed: {detail}" | |
| super().__init__(message, status_code=500) | |
| self.operation = operation |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
- Create shared Breadcrumb component used across 4 pages - Add shared Implementation type to types/index.ts - Remove unused CacheError exception from api/exceptions.py - Add tests for DatabaseQueryError (100% coverage on exceptions.py) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
DatabaseQueryError,CacheError) and improve error handling in plots routerSpecPage.tsx(807→500 lines) intoSpecOverviewandSpecDetailViewcomponentsuseFilterState.ts(375→258 lines) intouseUrlSyncanduseFilterFetchhooksTest plan
🤖 Generated with Claude Code