feat: add spec pages, catalog, and interactive views#3167
feat: add spec pages, catalog, and interactive views#3167MarkusNeusinger merged 14 commits intomainfrom
Conversation
…tion - Add workflow_dispatch input to force run tests - Include test configuration file changes in test detection logic - Update .gitignore to ensure JetBrains IDE files are ignored
…tion - Add workflow_dispatch input to force run tests - Include test configuration file changes in test detection logic - Update .gitignore to ensure JetBrains IDE files are ignored
…tion - Add workflow_dispatch input to force run tests - Include test configuration file changes in test detection logic - Update .gitignore to ensure JetBrains IDE files are ignored
- Introduced HomeState context for persistent state across navigation - Updated Layout component to provide home state and scroll position management - Refactored useFilterState to utilize persistent home state - Improved HomePage to restore scroll position from home state - Enhanced SpecPage layout responsiveness
- Added id and name attributes to filter input for better accessibility - Adjusted layout responsiveness in SpecTabs component - Updated score color logic in SpecTabs for clearer status indication - Prevented double fetching of images in CatalogPage with useRef - Implemented copy code functionality in SpecPage with user feedback
- Implement InteractivePage component for displaying interactive plots - Update routing to include interactive view - Modify SpecPage to link to the new interactive page
…porting - Introduced a new proxy endpoint to fetch HTML and inject size reporting script - Updated InteractivePage to use dynamic dimensions based on content size - Modified initialization of dimensions for better responsiveness - Included proxy router in the application
- Refactor image fetching logic to remove unnecessary double-fetch prevention - Update styles for image containers and interactive elements - Adjust scale calculation to account for padding - Improve loading behavior with opacity transition for iframe
- Add sticky detection and scroll percentage calculation to FilterBar - Refactor Layout component to serve as a global provider - Implement breadcrumb navigation in InteractivePage and SpecPage
- Added catalog link to FilterBar and SpecPage for easier navigation - Implemented breadcrumb navigation in CatalogPage - Enhanced layout responsiveness and styling across components
…mprovements - Added a floating scroll-to-top button on the catalog page - Improved filter bar layout with a catalog icon
- Simplify HTTPException handling in proxy.py - Update sitemap generation to include catalog page and new spec URLs in seo.py - Refactor analytics tracking in CatalogPage and InteractivePage components - Remove unused FullscreenModal import in HomePage - Clean up unused imports in SpecPage
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR implements a complete routing overhaul, transitioning from a modal-based single-page application to a multi-page architecture using react-router-dom. It adds three main pages (spec details, catalog, and interactive viewer), removes the fullscreen modal component, introduces persistent state management for navigation preservation, and adds a backend HTML proxy endpoint for interactive plot rendering with auto-scaling support.
Key Changes:
- Replaced modal-based spec viewing with dedicated spec detail pages (
/:specId/:library) - Added catalog page (
/catalog) with alphabetically sorted specs and thumbnail rotation - Implemented fullscreen interactive HTML viewer (
/interactive/:specId/:library) with auto-scaling - Created shared Layout component with global data providers and persistent home state
Reviewed changes
Copilot reviewed 24 out of 26 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| app/src/router.tsx | Defines new route structure with Layout wrapper and separate interactive route |
| app/src/main.tsx | Updates entry point to use AppRouter instead of App component |
| app/src/pages/SpecPage.tsx | New spec detail page with tabs for code/spec/implementation/quality |
| app/src/pages/CatalogPage.tsx | New alphabetical spec listing with image rotation and navigation |
| app/src/pages/InteractivePage.tsx | New fullscreen interactive plot viewer with iframe scaling |
| app/src/pages/HomePage.tsx | Refactored to use router navigation and persistent state context |
| app/src/components/Layout.tsx | Shared layout with AppDataProvider and HomeState context for persistence |
| app/src/components/SpecTabs.tsx | Tabbed interface for spec details with syntax highlighting |
| app/src/components/LibraryPills.tsx | Horizontal library selector carousel with quality scores |
| app/src/components/FilterBar.tsx | Added catalog link and sticky header styling |
| app/src/components/SpecAccordions.tsx | Accordion-based spec viewer (appears unused) |
| app/src/hooks/useFilterState.ts | Enhanced with persistent state and deterministic shuffling |
| app/src/hooks/useAnalytics.ts | Added urlOverride parameter for custom pageview tracking |
| app/src/components/FullscreenModal.tsx | Removed - replaced by dedicated pages |
| api/routers/proxy.py | New HTML proxy endpoint that injects size reporting script |
| api/routers/specs.py | Added review fields to spec detail response |
| api/routers/seo.py | Updated sitemap to new URL structure (/, /catalog, /{spec_id}) |
| api/schemas.py | Extended schemas with review and timestamp fields |
| tests/unit/api/test_routers.py | Updated tests for new URL format and mock data types |
Comments suppressed due to low confidence (1)
app/src/pages/HomePage.tsx:72
- The scroll restoration effect lacks proper cleanup and has a potential memory leak. If the component unmounts before
requestAnimationFrameexecutes, the callback will still attempt to execute with stale data. Add a cleanup function to cancel the animation frame and add a mounted ref to prevent state updates after unmount.
| // Send to parent | ||
| if (width > 0 && height > 0) { | ||
| window.parent.postMessage({ | ||
| type: 'pyplots-size', | ||
| width: Math.ceil(width), | ||
| height: Math.ceil(height) | ||
| }, '*'); |
There was a problem hiding this comment.
The postMessage uses '*' as the target origin, which allows any website to receive the size data if it embeds this iframe. This is a security risk as it could leak information about plot dimensions to malicious sites. Consider using a specific origin (e.g., window.location.origin or a configured allowed origin) instead of the wildcard.
| // Send to parent | |
| if (width > 0 && height > 0) { | |
| window.parent.postMessage({ | |
| type: 'pyplots-size', | |
| width: Math.ceil(width), | |
| height: Math.ceil(height) | |
| }, '*'); | |
| // Determine the target origin for postMessage: prefer parent page origin from document.referrer | |
| var targetOrigin = window.location.origin; | |
| if (document.referrer) { | |
| try { | |
| var refUrl = new URL(document.referrer); | |
| targetOrigin = refUrl.origin; | |
| } catch (e) { | |
| // If parsing fails, keep the safer default of window.location.origin | |
| } | |
| } | |
| // Send to parent | |
| if (width > 0 && height > 0) { | |
| window.parent.postMessage({ | |
| type: 'pyplots-size', | |
| width: Math.ceil(width), | |
| height: Math.ceil(height) | |
| }, targetOrigin); |
| const handleMessage = (event: MessageEvent) => { | ||
| if (event.data?.type === 'pyplots-size') { | ||
| const { width, height } = event.data; | ||
| if (width > 0 && height > 0) { | ||
| setContentWidth(width); | ||
| setContentHeight(height); | ||
| setSizeReady(true); | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing validation for the message origin in the event handler. An attacker could send spoofed size messages from a malicious website. Add origin validation by checking that event.origin matches the expected API URL before processing the message data.
| import { useState, useCallback } from 'react'; | ||
| import Box from '@mui/material/Box'; | ||
| import Accordion from '@mui/material/Accordion'; | ||
| import AccordionSummary from '@mui/material/AccordionSummary'; | ||
| import AccordionDetails from '@mui/material/AccordionDetails'; | ||
| import Typography from '@mui/material/Typography'; | ||
| import IconButton from '@mui/material/IconButton'; | ||
| import Tooltip from '@mui/material/Tooltip'; | ||
| import ExpandMoreIcon from '@mui/icons-material/ExpandMore'; | ||
| import CodeIcon from '@mui/icons-material/Code'; | ||
| import DescriptionIcon from '@mui/icons-material/Description'; | ||
| import StarIcon from '@mui/icons-material/Star'; | ||
| import ContentCopyIcon from '@mui/icons-material/ContentCopy'; | ||
| import CheckIcon from '@mui/icons-material/Check'; | ||
|
|
||
| // Import Prism for syntax highlighting (same as FullscreenModal) | ||
| import Prism from 'prismjs'; | ||
| import 'prismjs/components/prism-python'; | ||
| import 'prismjs/themes/prism.css'; | ||
|
|
||
| interface SpecAccordionsProps { | ||
| code: string | null; | ||
| description: string; | ||
| applications?: string[]; | ||
| notes?: string[]; | ||
| qualityScore: number | null; | ||
| libraryId: string; | ||
| onTrackEvent?: (name: string, props?: Record<string, string | undefined>) => void; | ||
| } | ||
|
|
||
| export function SpecAccordions({ | ||
| code, | ||
| description, | ||
| applications, | ||
| notes, | ||
| qualityScore, | ||
| libraryId, | ||
| onTrackEvent, | ||
| }: SpecAccordionsProps) { | ||
| const [copied, setCopied] = useState(false); | ||
| const [expanded, setExpanded] = useState<string | false>(false); | ||
|
|
||
| const handleCopy = useCallback(async () => { | ||
| if (!code) return; | ||
| try { | ||
| await navigator.clipboard.writeText(code); | ||
| setCopied(true); | ||
| onTrackEvent?.('copy_code', { library: libraryId, method: 'accordion' }); | ||
| setTimeout(() => setCopied(false), 2000); | ||
| } catch (err) { | ||
| console.error('Copy failed:', err); | ||
| } | ||
| }, [code, libraryId, onTrackEvent]); | ||
|
|
||
| const handleChange = (panel: string) => (_: React.SyntheticEvent, isExpanded: boolean) => { | ||
| setExpanded(isExpanded ? panel : false); | ||
| if (isExpanded) { | ||
| onTrackEvent?.(`expand_${panel}`, { library: libraryId }); | ||
| } | ||
| }; | ||
|
|
||
| // Highlight code with Prism | ||
| const highlightedCode = code | ||
| ? Prism.highlight(code, Prism.languages.python, 'python') | ||
| : ''; | ||
|
|
||
| return ( | ||
| <Box sx={{ mt: 3, maxWidth: 900, mx: 'auto' }}> | ||
| {/* Code Accordion */} | ||
| <Accordion | ||
| expanded={expanded === 'code'} | ||
| onChange={handleChange('code')} | ||
| sx={{ | ||
| bgcolor: '#fafafa', | ||
| boxShadow: 'none', | ||
| '&:before': { display: 'none' }, | ||
| borderRadius: '8px !important', | ||
| mb: 1, | ||
| }} | ||
| > | ||
| <AccordionSummary | ||
| expandIcon={<ExpandMoreIcon />} | ||
| sx={{ | ||
| '& .MuiAccordionSummary-content': { | ||
| alignItems: 'center', | ||
| gap: 1, | ||
| }, | ||
| }} | ||
| > | ||
| <CodeIcon sx={{ color: '#3776AB', fontSize: '1.25rem' }} /> | ||
| <Typography sx={{ fontFamily: '"MonoLisa", monospace', fontWeight: 500 }}> | ||
| Code | ||
| </Typography> | ||
| </AccordionSummary> | ||
| <AccordionDetails sx={{ pt: 0 }}> | ||
| <Box sx={{ position: 'relative' }}> | ||
| <Tooltip title={copied ? 'Copied!' : 'Copy code'}> | ||
| <IconButton | ||
| onClick={handleCopy} | ||
| sx={{ | ||
| position: 'absolute', | ||
| top: 8, | ||
| right: 8, | ||
| bgcolor: '#fff', | ||
| boxShadow: 1, | ||
| '&:hover': { bgcolor: '#f3f4f6' }, | ||
| }} | ||
| size="small" | ||
| > | ||
| {copied ? <CheckIcon color="success" /> : <ContentCopyIcon fontSize="small" />} | ||
| </IconButton> | ||
| </Tooltip> | ||
| <Box | ||
| component="pre" | ||
| sx={{ | ||
| bgcolor: '#1e1e1e', | ||
| color: '#d4d4d4', | ||
| p: 2, | ||
| borderRadius: 1, | ||
| overflow: 'auto', | ||
| maxHeight: 400, | ||
| fontSize: '0.8rem', | ||
| fontFamily: '"MonoLisa", monospace', | ||
| lineHeight: 1.5, | ||
| '& .token.comment': { color: '#6a9955' }, | ||
| '& .token.string': { color: '#ce9178' }, | ||
| '& .token.keyword': { color: '#569cd6' }, | ||
| '& .token.function': { color: '#dcdcaa' }, | ||
| '& .token.number': { color: '#b5cea8' }, | ||
| '& .token.operator': { color: '#d4d4d4' }, | ||
| '& .token.builtin': { color: '#4ec9b0' }, | ||
| }} | ||
| > | ||
| <code dangerouslySetInnerHTML={{ __html: highlightedCode }} /> | ||
| </Box> | ||
| </Box> | ||
| </AccordionDetails> | ||
| </Accordion> | ||
|
|
||
| {/* Description Accordion */} | ||
| <Accordion | ||
| expanded={expanded === 'description'} | ||
| onChange={handleChange('description')} | ||
| sx={{ | ||
| bgcolor: '#fafafa', | ||
| boxShadow: 'none', | ||
| '&:before': { display: 'none' }, | ||
| borderRadius: '8px !important', | ||
| mb: 1, | ||
| }} | ||
| > | ||
| <AccordionSummary | ||
| expandIcon={<ExpandMoreIcon />} | ||
| sx={{ | ||
| '& .MuiAccordionSummary-content': { | ||
| alignItems: 'center', | ||
| gap: 1, | ||
| }, | ||
| }} | ||
| > | ||
| <DescriptionIcon sx={{ color: '#FFD43B', fontSize: '1.25rem' }} /> | ||
| <Typography sx={{ fontFamily: '"MonoLisa", monospace', fontWeight: 500 }}> | ||
| Description | ||
| </Typography> | ||
| </AccordionSummary> | ||
| <AccordionDetails> | ||
| <Typography | ||
| sx={{ | ||
| fontFamily: '"MonoLisa", monospace', | ||
| fontSize: '0.9rem', | ||
| lineHeight: 1.7, | ||
| color: '#374151', | ||
| mb: 2, | ||
| }} | ||
| > | ||
| {description} | ||
| </Typography> | ||
|
|
||
| {applications && applications.length > 0 && ( | ||
| <Box sx={{ mb: 2 }}> | ||
| <Typography | ||
| sx={{ | ||
| fontFamily: '"MonoLisa", monospace', | ||
| fontSize: '0.8rem', | ||
| fontWeight: 600, | ||
| color: '#6b7280', | ||
| mb: 1, | ||
| }} | ||
| > | ||
| Applications | ||
| </Typography> | ||
| <Box component="ul" sx={{ m: 0, pl: 2 }}> | ||
| {applications.map((app, i) => ( | ||
| <Typography | ||
| key={i} | ||
| component="li" | ||
| sx={{ | ||
| fontFamily: '"MonoLisa", monospace', | ||
| fontSize: '0.85rem', | ||
| color: '#4b5563', | ||
| mb: 0.5, | ||
| }} | ||
| > | ||
| {app} | ||
| </Typography> | ||
| ))} | ||
| </Box> | ||
| </Box> | ||
| )} | ||
|
|
||
| {notes && notes.length > 0 && ( | ||
| <Box> | ||
| <Typography | ||
| sx={{ | ||
| fontFamily: '"MonoLisa", monospace', | ||
| fontSize: '0.8rem', | ||
| fontWeight: 600, | ||
| color: '#6b7280', | ||
| mb: 1, | ||
| }} | ||
| > | ||
| Notes | ||
| </Typography> | ||
| <Box component="ul" sx={{ m: 0, pl: 2 }}> | ||
| {notes.map((note, i) => ( | ||
| <Typography | ||
| key={i} | ||
| component="li" | ||
| sx={{ | ||
| fontFamily: '"MonoLisa", monospace', | ||
| fontSize: '0.85rem', | ||
| color: '#4b5563', | ||
| mb: 0.5, | ||
| }} | ||
| > | ||
| {note} | ||
| </Typography> | ||
| ))} | ||
| </Box> | ||
| </Box> | ||
| )} | ||
| </AccordionDetails> | ||
| </Accordion> | ||
|
|
||
| {/* Quality Accordion */} | ||
| <Accordion | ||
| expanded={expanded === 'quality'} | ||
| onChange={handleChange('quality')} | ||
| sx={{ | ||
| bgcolor: '#fafafa', | ||
| boxShadow: 'none', | ||
| '&:before': { display: 'none' }, | ||
| borderRadius: '8px !important', | ||
| }} | ||
| > | ||
| <AccordionSummary | ||
| expandIcon={<ExpandMoreIcon />} | ||
| sx={{ | ||
| '& .MuiAccordionSummary-content': { | ||
| alignItems: 'center', | ||
| gap: 1, | ||
| }, | ||
| }} | ||
| > | ||
| <StarIcon sx={{ color: '#f59e0b', fontSize: '1.25rem' }} /> | ||
| <Typography sx={{ fontFamily: '"MonoLisa", monospace', fontWeight: 500 }}> | ||
| Quality: {qualityScore ? `${Math.round(qualityScore)}/100` : 'N/A'} | ||
| </Typography> | ||
| </AccordionSummary> | ||
| <AccordionDetails> | ||
| <Typography | ||
| sx={{ | ||
| fontFamily: '"MonoLisa", monospace', | ||
| fontSize: '0.9rem', | ||
| color: '#6b7280', | ||
| }} | ||
| > | ||
| {qualityScore && qualityScore >= 90 ? ( | ||
| <>This implementation scored <strong>{Math.round(qualityScore)}/100</strong> in AI quality review.</> | ||
| ) : qualityScore ? ( | ||
| <>Quality score: <strong>{Math.round(qualityScore)}/100</strong></> | ||
| ) : ( | ||
| 'Quality score not available.' | ||
| )} | ||
| </Typography> | ||
| </AccordionDetails> | ||
| </Accordion> | ||
| </Box> | ||
| ); | ||
| } |
There was a problem hiding this comment.
The SpecAccordions.tsx component is defined but never appears to be used in the codebase. This is dead code that should either be removed or integrated into the application. The newer SpecTabs.tsx component seems to serve a similar purpose and is actually being used in the SpecPage.
| onClick={(e) => { | ||
| if (!expandedDescs[spec.id]) { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); |
There was a problem hiding this comment.
The image click handler in the catalog stops event propagation when the description is not expanded, but this prevents the navigation to the spec page that's defined on the parent Link element. Users who click on a collapsed description expecting to navigate to the spec page will instead just expand the description, which breaks the expected navigation behavior. Consider using separate clickable areas for expand vs navigate, or allow both actions.
| onClick={(e) => { | |
| if (!expandedDescs[spec.id]) { | |
| e.preventDefault(); | |
| e.stopPropagation(); | |
| onClick={() => { | |
| if (!expandedDescs[spec.id]) { |
| raise HTTPException(status_code=400, detail=f"Only URLs from {ALLOWED_HOST}/{ALLOWED_BUCKET} are allowed") | ||
|
|
||
| # Fetch the HTML | ||
| async with httpx.AsyncClient(timeout=30.0) as client: |
There was a problem hiding this comment.
The timeout value of 30 seconds for fetching HTML from GCS is quite long and could lead to poor user experience if the storage is slow or unavailable. Most interactive HTML files should load much faster. Consider reducing this to 10 seconds or making it configurable. Also consider adding retry logic for transient failures.
| const [activeFilters, setActiveFilters] = useState<ActiveFilters>(() => | ||
| homeStateRef.current.initialized ? homeStateRef.current.activeFilters : parseUrlFilters() | ||
| ); | ||
| const [filterCounts, setFilterCounts] = useState<FilterCounts | null>(() => | ||
| homeStateRef.current.initialized ? homeStateRef.current.filterCounts : null | ||
| ); | ||
| const [globalCounts, setGlobalCounts] = useState<FilterCounts | null>(() => | ||
| homeStateRef.current.initialized ? homeStateRef.current.globalCounts : null | ||
| ); | ||
| const [orCounts, setOrCounts] = useState<Record<string, number>[]>(() => | ||
| homeStateRef.current.initialized ? homeStateRef.current.orCounts : [] | ||
| ); | ||
|
|
||
| // Image state | ||
| const [allImages, setAllImages] = useState<PlotImage[]>([]); | ||
| const [displayedImages, setDisplayedImages] = useState<PlotImage[]>([]); | ||
| const [hasMore, setHasMore] = useState(false); | ||
| // Image state - restore from persistent state if available | ||
| const [allImages, setAllImages] = useState<PlotImage[]>(() => | ||
| homeStateRef.current.initialized ? homeStateRef.current.allImages : [] | ||
| ); | ||
| const [displayedImages, setDisplayedImages] = useState<PlotImage[]>(() => | ||
| homeStateRef.current.initialized ? homeStateRef.current.displayedImages : [] | ||
| ); | ||
| const [hasMore, setHasMore] = useState(() => | ||
| homeStateRef.current.initialized ? homeStateRef.current.hasMore : false | ||
| ); | ||
|
|
||
| // UI state | ||
| const [loading, setLoading] = useState(true); | ||
| const [loading, setLoading] = useState(() => !homeStateRef.current.initialized); |
There was a problem hiding this comment.
The persistent state restoration logic has a race condition. The state is initialized from homeStateRef.current.initialized, but this ref is updated asynchronously via useEffect in the Layout component. If the HomePage mounts before the Layout's effect runs, the state won't be properly restored. Consider using a synchronous initialization pattern or adding guards to ensure the ref is updated before child components attempt to read from it.
| # Security: Only allow URLs from our GCS bucket | ||
| if not url.startswith(f"https://{ALLOWED_HOST}/{ALLOWED_BUCKET}/"): | ||
| raise HTTPException(status_code=400, detail=f"Only URLs from {ALLOWED_HOST}/{ALLOWED_BUCKET} are allowed") |
There was a problem hiding this comment.
The HTML proxy endpoint only validates that URLs start with the allowed bucket path, but this doesn't prevent path traversal attacks. An attacker could craft a URL like https://storage.googleapis.com/pyplots-images/../other-bucket/malicious.html which would pass the validation but potentially access files outside the intended bucket. Consider using URL parsing to validate the bucket name more strictly.
| import { useState, useEffect, createContext, useContext, useRef, useCallback, type ReactNode } from 'react'; | ||
| import { Outlet } from 'react-router-dom'; | ||
| import Box from '@mui/material/Box'; | ||
| import Container from '@mui/material/Container'; | ||
|
|
||
| import { API_URL } from '../constants'; | ||
| import type { LibraryInfo, SpecInfo, PlotImage, ActiveFilters, FilterCounts } from '../types'; | ||
|
|
||
| interface AppData { | ||
| specsData: SpecInfo[]; | ||
| librariesData: LibraryInfo[]; | ||
| stats: { specs: number; plots: number; libraries: number } | null; | ||
| } | ||
|
|
||
| // Persistent home state that survives navigation | ||
| interface HomeState { | ||
| allImages: PlotImage[]; | ||
| displayedImages: PlotImage[]; | ||
| activeFilters: ActiveFilters; | ||
| filterCounts: FilterCounts | null; | ||
| globalCounts: FilterCounts | null; | ||
| orCounts: Record<string, number>[]; | ||
| hasMore: boolean; | ||
| scrollY: number; | ||
| initialized: boolean; | ||
| } | ||
|
|
||
| interface HomeStateContext { | ||
| homeState: HomeState; | ||
| homeStateRef: React.MutableRefObject<HomeState>; | ||
| setHomeState: React.Dispatch<React.SetStateAction<HomeState>>; | ||
| saveScrollPosition: () => void; | ||
| } | ||
|
|
||
| const initialHomeState: HomeState = { | ||
| allImages: [], | ||
| displayedImages: [], | ||
| activeFilters: [], | ||
| filterCounts: null, | ||
| globalCounts: null, | ||
| orCounts: [], | ||
| hasMore: false, | ||
| scrollY: 0, | ||
| initialized: false, | ||
| }; | ||
|
|
||
| const AppDataContext = createContext<AppData | null>(null); | ||
| const HomeStateContext = createContext<HomeStateContext | null>(null); | ||
|
|
||
| export function useAppData() { | ||
| const context = useContext(AppDataContext); | ||
| if (!context) { | ||
| throw new Error('useAppData must be used within AppDataProvider'); | ||
| } | ||
| return context; | ||
| } | ||
|
|
||
| export function useHomeState() { | ||
| const context = useContext(HomeStateContext); | ||
| if (!context) { | ||
| throw new Error('useHomeState must be used within AppDataProvider'); | ||
| } | ||
| return context; | ||
| } | ||
|
|
||
| // Global provider that wraps the entire router (persists across all pages including InteractivePage) | ||
| export function AppDataProvider({ children }: { children: ReactNode }) { | ||
| const [specsData, setSpecsData] = useState<SpecInfo[]>([]); | ||
| const [librariesData, setLibrariesData] = useState<LibraryInfo[]>([]); | ||
| const [stats, setStats] = useState<{ specs: number; plots: number; libraries: number } | null>(null); | ||
|
|
||
| // Persistent home state (both ref for sync access and state for reactivity) | ||
| const [homeState, setHomeState] = useState<HomeState>(initialHomeState); | ||
| const homeStateRef = useRef<HomeState>(initialHomeState); | ||
|
|
||
| // Keep ref in sync with state | ||
| useEffect(() => { | ||
| homeStateRef.current = homeState; | ||
| }, [homeState]); | ||
|
|
||
| // Save scroll position synchronously to ref (called before navigation) | ||
| const saveScrollPosition = useCallback(() => { | ||
| homeStateRef.current = { ...homeStateRef.current, scrollY: window.scrollY }; | ||
| setHomeState((prev) => ({ ...prev, scrollY: window.scrollY })); | ||
| }, []); | ||
|
|
||
| // Load shared data on mount | ||
| useEffect(() => { | ||
| const fetchData = async () => { | ||
| try { | ||
| const [specsRes, libsRes, statsRes] = await Promise.all([ | ||
| fetch(`${API_URL}/specs`), | ||
| fetch(`${API_URL}/libraries`), | ||
| fetch(`${API_URL}/stats`), | ||
| ]); | ||
|
|
||
| if (specsRes.ok) { | ||
| const data = await specsRes.json(); | ||
| setSpecsData(Array.isArray(data) ? data : data.specs || []); | ||
| } | ||
|
|
||
| if (libsRes.ok) { | ||
| const data = await libsRes.json(); | ||
| setLibrariesData(data.libraries || []); | ||
| } | ||
|
|
||
| if (statsRes.ok) { | ||
| const data = await statsRes.json(); | ||
| setStats(data); | ||
| } | ||
| } catch (err) { | ||
| console.error('Error loading initial data:', err); | ||
| } | ||
| }; | ||
| fetchData(); | ||
| }, []); | ||
|
|
||
| return ( | ||
| <AppDataContext.Provider value={{ specsData, librariesData, stats }}> | ||
| <HomeStateContext.Provider value={{ homeState, homeStateRef, setHomeState, saveScrollPosition }}> | ||
| {children} | ||
| </HomeStateContext.Provider> | ||
| </AppDataContext.Provider> | ||
| ); | ||
| } | ||
|
|
||
| // Layout component for pages with standard layout (HomePage, SpecPage, CatalogPage) | ||
| export function Layout() { | ||
| return ( | ||
| <Box sx={{ minHeight: '100vh', bgcolor: '#fafafa', py: 5, position: 'relative' }}> | ||
| <Container maxWidth={false} sx={{ px: { xs: 2, sm: 4, md: 8, lg: 12 } }}> | ||
| <Outlet /> | ||
| </Container> | ||
| </Box> | ||
| ); | ||
| } |
There was a problem hiding this comment.
The Layout component combines both a data provider (AppDataProvider) and a layout component (Layout) with the same name prefix, which could be confusing. The AppDataProvider is actually wrapping the entire router (including InteractivePage which is outside Layout), while Layout is only for standard pages. Consider renaming for clarity, e.g., AppDataProvider → GlobalDataProvider and Layout → StandardPageLayout.
| function seededRandom(seed: number): () => number { | ||
| return () => { | ||
| let t = (seed += 0x6d2b79f5); | ||
| t = Math.imul(t ^ (t >>> 15), t | 1); | ||
| t ^= t + Math.imul(t ^ (t >>> 7), t | 61); | ||
| return ((t ^ (t >>> 14)) >>> 0) / 4294967296; | ||
| }; | ||
| } |
There was a problem hiding this comment.
The seeded random number generator implementation has a potential issue. The seed is modified in place with seed +=, which means the function returns a closure that maintains state between calls. However, if the same seed is used to create multiple generators, they will all share the same state due to the closure over the same seed variable. Consider using a local variable copy to avoid unintended state sharing.
| <Grid | ||
| container | ||
| spacing={3} | ||
| sx={{ | ||
| opacity: isTransitioning ? 0 : 1, | ||
| transition: 'opacity 0.15s ease-in-out', | ||
| }} | ||
| > |
There was a problem hiding this comment.
Inconsistent indentation detected. Lines 118-125 use 12-space indentation while the rest of the file uses consistent formatting. This appears to be a formatting error that should be fixed by running the linter.
- Fix SSRF/path traversal vulnerability in proxy.py with strict URL validation - Fix postMessage wildcard origin (now uses specific pyplots.ai origin) - Add origin validation for postMessage in InteractivePage - Add URL validation in useAnalytics to prevent injection - Add aria-label to FilterBar search for accessibility - Remove unused SpecAccordions.tsx (dead code) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
CodeQL was flagging the original approach because user-controlled URL was still passed to client.get(). Now we extract the path, validate it, and reconstruct a new URL from hardcoded scheme/host values. This breaks the taint flow and properly prevents SSRF attacks.
| <Box | ||
| sx={{ | ||
| display: 'flex', | ||
| alignItems: 'center', | ||
| mx: { xs: -2, sm: -4, md: -8, lg: -12 }, | ||
| mt: -5, // Compensate for Layout padding | ||
| px: 2, | ||
| py: 1, | ||
| mb: 2, | ||
| bgcolor: '#f3f4f6', | ||
| borderBottom: '1px solid #e5e7eb', | ||
| fontFamily: '"MonoLisa", monospace', | ||
| fontSize: '0.85rem', | ||
| }} | ||
| > | ||
| <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> | ||
| <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> |
There was a problem hiding this comment.
Missing accessibility attributes for the breadcrumb navigation. The breadcrumb should include ARIA attributes like aria-label="Breadcrumb" on the container and potentially aria-current="page" on the last item to help screen reader users understand the navigation hierarchy.
| <Box | ||
| onClick={() => handleImageClick(spec.id, spec.images.length)} | ||
| sx={{ | ||
| position: 'relative', | ||
| width: { xs: '100%', sm: 280 }, | ||
| height: { xs: 180, sm: 158 }, | ||
| borderRadius: 1.5, | ||
| overflow: 'hidden', | ||
| bgcolor: '#fff', | ||
| boxShadow: '0 2px 8px rgba(0,0,0,0.08)', | ||
| flexShrink: 0, | ||
| cursor: spec.images.length > 1 ? 'pointer' : 'default', | ||
| '&:hover .rotate-hint': { | ||
| opacity: spec.images.length > 1 ? 1 : 0, | ||
| }, | ||
| '&:hover .library-hint': { | ||
| opacity: 1, | ||
| }, | ||
| }} |
There was a problem hiding this comment.
The image click handler in the catalog lacks keyboard accessibility. Users navigating with keyboard cannot trigger the rotation functionality. Add keyboard event handlers (onKeyDown/onKeyPress) to support Enter and Space keys for rotation, and ensure the element has a proper role and tabindex for keyboard focus.
| onClick={(e) => { | ||
| if (!expandedDescs[spec.id]) { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| setExpandedDescs((prev) => ({ ...prev, [spec.id]: true })); | ||
| } | ||
| }} |
There was a problem hiding this comment.
The description expansion click handler prevents default behavior and stops propagation, but this element is nested inside a Link component. When the description is not expanded, clicking it should both expand the description AND prevent navigation. However, the current implementation may have unexpected behavior.
Consider restructuring so the clickable description area is not nested within the Link, or ensure the event handling logic is clear about the intended UX flow.
| // Pick random implementation | ||
| const randomIdx = Math.floor(Math.random() * data.implementations.length); | ||
| const randomLib = data.implementations[randomIdx].library_id; | ||
| setSelectedLibrary(randomLib); | ||
|
|
||
| // Update URL to include the library (without adding to history) | ||
| if (!urlLibrary) { | ||
| navigate(`/${specId}/${randomLib}`, { replace: true }); |
There was a problem hiding this comment.
Using Math.random() for selecting the initial library implementation creates non-deterministic behavior that makes the page non-cacheable and difficult to test. Consider using a deterministic selection method (e.g., first alphabetically sorted implementation) or deriving the selection from the spec_id hash for consistency across page loads.
| // Pick random implementation | |
| const randomIdx = Math.floor(Math.random() * data.implementations.length); | |
| const randomLib = data.implementations[randomIdx].library_id; | |
| setSelectedLibrary(randomLib); | |
| // Update URL to include the library (without adding to history) | |
| if (!urlLibrary) { | |
| navigate(`/${specId}/${randomLib}`, { replace: true }); | |
| // Pick deterministic implementation: first by alphabetical library_id | |
| const sortedImplementations = [...data.implementations].sort((a, b) => | |
| a.library_id.localeCompare(b.library_id), | |
| ); | |
| const defaultLib = sortedImplementations[0].library_id; | |
| setSelectedLibrary(defaultLib); | |
| // Update URL to include the library (without adding to history) | |
| if (!urlLibrary) { | |
| navigate(`/${specId}/${defaultLib}`, { replace: true }); |
| <InputBase | ||
| inputRef={inputRef} | ||
| id="filter-search" | ||
| name="filter-search" | ||
| aria-label={selectedCategory ? `Search ${FILTER_LABELS[selectedCategory]}` : 'Search filters'} |
There was a problem hiding this comment.
The search input is missing a label element. While it has a placeholder and aria-label, best practice for accessibility is to also include a visible or visually-hidden label element associated with the input via the htmlFor attribute pointing to the input's id.
| # Check for path traversal attempts | ||
| if ".." in parsed.path: | ||
| return None | ||
| # Validate path contains only safe characters (alphanumeric, hyphens, underscores, dots, slashes) |
There was a problem hiding this comment.
The safe character validation allows the "+" character but this may not be intended for GCS URLs. The comment says "alphanumeric, hyphens, underscores, dots, slashes" but the actual check includes "+". This could be a security issue if "+" has special meaning in URL contexts that could be exploited.
Either remove "+" from the allowed characters or update the comment to document why it's included.
| # Validate path contains only safe characters (alphanumeric, hyphens, underscores, dots, slashes) | |
| # Validate path contains only safe characters (alphanumeric, hyphens, underscores, dots, slashes, plus) |
| <Box | ||
| sx={{ | ||
| display: 'flex', | ||
| alignItems: 'center', | ||
| mx: { xs: -2, sm: -4, md: -8, lg: -12 }, | ||
| mt: -5, | ||
| px: 2, | ||
| py: 1, | ||
| mb: 3, | ||
| bgcolor: '#f3f4f6', | ||
| borderBottom: '1px solid #e5e7eb', | ||
| fontFamily: '"MonoLisa", monospace', | ||
| fontSize: '0.85rem', | ||
| position: 'sticky', | ||
| top: 0, | ||
| zIndex: 100, | ||
| }} | ||
| > | ||
| <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="span" sx={{ color: '#4b5563' }}> | ||
| catalog | ||
| </Box> | ||
| </Box> |
There was a problem hiding this comment.
Missing accessibility attributes for the breadcrumb navigation. The breadcrumb should include ARIA attributes like aria-label="Breadcrumb" on the container and potentially aria-current="page" on the last item to help screen reader users understand the navigation hierarchy.
| <Box | ||
| sx={{ | ||
| display: 'flex', | ||
| alignItems: 'center', | ||
| justifyContent: 'space-between', | ||
| px: 2, | ||
| py: 1, | ||
| bgcolor: '#f3f4f6', | ||
| borderBottom: '1px solid #e5e7eb', | ||
| fontFamily: '"MonoLisa", monospace', | ||
| fontSize: '0.85rem', | ||
| }} | ||
| > | ||
| <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> | ||
| <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={Link} | ||
| to={`/${specId}/${library}`} | ||
| sx={{ | ||
| color: '#3776AB', | ||
| textDecoration: 'none', | ||
| '&:hover': { textDecoration: 'underline' }, | ||
| }} | ||
| > | ||
| {library} | ||
| </Box> | ||
| <Box component="span" sx={{ mx: 1, color: '#9ca3af' }}>›</Box> | ||
| <Box component="span" sx={{ color: '#4b5563' }}> | ||
| interactive | ||
| </Box> | ||
| </Box> |
There was a problem hiding this comment.
Missing accessibility attributes for the breadcrumb navigation. The breadcrumb should include ARIA attributes like aria-label="Breadcrumb" on the container and potentially aria-current="page" on the last item to help screen reader users understand the navigation hierarchy.
| // Skip fetch on first mount if restored from persistent state with same filters | ||
| if (initializedRef.current && filtersMatchRef.current) { | ||
| initializedRef.current = false; | ||
| filtersMatchRef.current = false; | ||
| return; | ||
| } | ||
| initializedRef.current = false; | ||
| filtersMatchRef.current = false; |
There was a problem hiding this comment.
The persistent state restoration logic could lead to stale data being displayed. The state is only skipped on the first mount if filters match, but if the user navigates away and back, the data could be outdated. Consider adding a timestamp to the persisted state and invalidating it after a certain period, or refetch in the background to ensure data freshness.
| function hashFilters(filters: ActiveFilters): number { | ||
| const str = JSON.stringify(filters); | ||
| let hash = 0; | ||
| for (let i = 0; i < str.length; i++) { | ||
| const char = str.charCodeAt(i); | ||
| hash = (hash << 5) - hash + char; | ||
| hash = hash & hash; | ||
| } | ||
| return Math.abs(hash); | ||
| } |
There was a problem hiding this comment.
The deterministic shuffle seed is based on a hash of the filter state JSON, but object property order in JavaScript objects is not guaranteed in older implementations. While modern JS engines maintain insertion order, using JSON.stringify for hashing could produce different results for semantically identical filter states with different property orders.
Consider sorting the object keys before stringifying or using a more robust serialization method to ensure consistent hashing across different scenarios.
Summary
/:specId/:library): Individual pages for each spec/library with code, specification, implementation, and quality tabs/catalog): Alphabetically sorted list of all specs with thumbnail rotation/interactive/:specId/:library): Fullscreen interactive HTML view with auto-scaling/proxy/html): Backend endpoint to inject size reporting script into HTML plots/,/catalog,/{spec_id})New Components
SpecPage.tsx- Main spec detail view with image, tabs, and library pillsCatalogPage.tsx- Alphabetical spec listing with rotating thumbnailsInteractivePage.tsx- Fullscreen interactive plot viewerSpecTabs.tsx- Code/Spec/Impl/Quality tabbed contentLibraryPills.tsx- Horizontal scrollable library selectorLayout.tsx- Shared layout with data providersTest plan
🤖 Generated with Claude Code