Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new SvelteKit frontend surface area (UI primitives, search/forms, alert “service” components) plus a typed API client/services layer and supporting tooling/config for building, testing, and deployment.
Changes:
- Introduces reusable UI components (loading/error, card/button, carousel, form helpers) and new tables/search panels.
- Adds typed API client (
axios) + service modules + TS types for FastAPI endpoints, plus frontend build/test workflows. - Adds frontend runtime assets/config (Tailwind layers, app.html CDN deps, Docker/nginx, docs standards).
Reviewed changes
Copilot reviewed 91 out of 379 changed files in this pull request and generated 21 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/lib/components/ui/LoadingState.svelte | New reusable loading-state UI component. |
| frontend/src/lib/components/ui/LoadingSpinner.svelte | New spinner component with sizing/message options. |
| frontend/src/lib/components/ui/FootprintVisualization.svelte | Adds Plotly footprint visualization UI. |
| frontend/src/lib/components/ui/ErrorToast.svelte | Adds global error toast UI. |
| frontend/src/lib/components/ui/ErrorMessage.svelte | Adds inline error/warn/info message UI. |
| frontend/src/lib/components/ui/ErrorBoundary.svelte | Adds UI error boundary with retry/reset. |
| frontend/src/lib/components/ui/ControlGroup.svelte | Adds labeled form control wrapper with help/error text. |
| frontend/src/lib/components/ui/Carousel.svelte | Adds image carousel with autoplay and controls. |
| frontend/src/lib/components/ui/Card.svelte | Adds base Card UI container component. |
| frontend/src/lib/components/ui/Button.svelte | Adds variant/size Button component with loading + link mode. |
| frontend/src/lib/components/ui/AsyncErrorBoundary.svelte | Adds async wrapper that shows spinner + uses ErrorBoundary. |
| frontend/src/lib/components/ui/AlertBanner.svelte | Adds dismissible alert banner component. |
| frontend/src/lib/components/tables/ReportingInstrumentsTable.svelte | Adds reporting instruments table using new UI primitives. |
| frontend/src/lib/components/search/PointingsTable.svelte | Adds pointings results table with selection support. |
| frontend/src/lib/components/search/PointingsSearchForm.svelte | Adds search filter form for pointings. |
| frontend/src/lib/components/search/DoiRequestPanel.svelte | Adds DOI request UI for selected pointings. |
| frontend/src/lib/components/layout/Navigation.svelte | Adds top navigation with auth links. |
| frontend/src/lib/components/layout/InstrumentStats.svelte | Adds instrument summary stats widgets. |
| frontend/src/lib/components/instruments/ExistingInstrumentsTable.svelte | Adds “existing instruments” reference table UI. |
| frontend/src/lib/components/forms/examples/RegisterFormExample.svelte | Example registration form using validation schemas. |
| frontend/src/lib/components/forms/examples/LoginFormExample.svelte | Example login form using validation schemas. |
| frontend/src/lib/components/forms/TimeField.svelte | Adds specialized datetime input + validation wrapper. |
| frontend/src/lib/components/forms/FootprintTypeSelector.svelte | Adds footprint type selector with conditional inputs. |
| frontend/src/lib/components/forms/CoordinateFields.svelte | Adds RA/Dec input fields with validation and summary. |
| frontend/src/lib/components/forms/ConditionalSection.svelte | Adds conditional section with slide animation. |
| frontend/src/lib/components/alerts/services/UrlParameterService.svelte | Adds URL param parsing/building “service component”. |
| frontend/src/lib/components/alerts/services/PaginationService.svelte | Adds pagination state “service component”. |
| frontend/src/lib/components/alerts/services/FilterManagementService.svelte | Adds alert filter state/options “service component”. |
| frontend/src/lib/components/alerts/services/AlertStatusService.svelte | Adds alert status helper “service component”. |
| frontend/src/lib/components/alerts/services/AlertSearchService.svelte | Adds alert autocomplete search “service component”. |
| frontend/src/lib/components/alerts/services/AlertDataProcessingService.svelte | Adds alert loading/processing “service component”. |
| frontend/src/lib/components/alerts/SearchFiltersForm.svelte | Adds alert search filters UI with autocomplete. |
| frontend/src/lib/components/alerts/AlertHeaderComponent.svelte | Adds alert header UI wiring status + download links. |
| frontend/src/lib/api/types/pointing.types.ts | Adds Pointing-related API types. |
| frontend/src/lib/api/types/misc.types.ts | Adds misc API response types. |
| frontend/src/lib/api/types/instrument.types.ts | Adds instrument + footprint API types. |
| frontend/src/lib/api/types/icecube.types.ts | Adds IceCube API types. |
| frontend/src/lib/api/types/galaxy.types.ts | Adds galaxy + filters API types. |
| frontend/src/lib/api/types/doi.types.ts | Adds DOI API types. |
| frontend/src/lib/api/types/candidate.types.ts | Adds candidate API types. |
| frontend/src/lib/api/types/alert.types.ts | Adds GW alert API types. |
| frontend/src/lib/api/services/search.service.ts | Adds combined search/DOI helper service. |
| frontend/src/lib/api/services/pointing.service.ts | Adds pointings API service functions. |
| frontend/src/lib/api/services/misc.service.ts | Adds health/status API service functions. |
| frontend/src/lib/api/services/instrument.service.ts | Adds instruments/footprints API service functions. |
| frontend/src/lib/api/services/icecube.service.ts | Adds IceCube API service functions. |
| frontend/src/lib/api/services/galaxy.service.ts | Adds galaxy API service functions. |
| frontend/src/lib/api/services/doi.service.ts | Adds DOI API service functions. |
| frontend/src/lib/api/services/candidate.service.ts | Adds candidate API service functions. |
| frontend/src/lib/api/services/auth.service.ts | Adds auth API service functions. |
| frontend/src/lib/api/services/alert.service.ts | Adds alert querying + contour helpers. |
| frontend/src/lib/api/services/ajax.service.ts | Adds legacy-style ajax endpoint wrappers. |
| frontend/src/lib/api/index.ts | Exports aggregated API surface + types. |
| frontend/src/lib/api/client.ts | Adds axios client with auth + environment baseURL logic. |
| frontend/src/app.html | Adds CDN deps for jQuery/Aladin/Plotly. |
| frontend/src/app.css | Adds Tailwind + design system layer utilities. |
| frontend/src/tests/setup.ts | Adds Vitest test setup/mocks. |
| frontend/scripts/migrate-styles.js | Adds migration helper script for design system classes. |
| frontend/package.json.docs-script | Adds docs script snippet file. |
| frontend/package.json | Adds frontend dependencies/scripts. |
| frontend/nginx.conf | Adds nginx config for SPA + API proxy. |
| frontend/eslint.config.js | Adds eslint config + ignore list. |
| frontend/docs/component-documentation-standards.md | Adds component documentation standards doc. |
| frontend/README.md | Adds frontend developer documentation. |
| frontend/Dockerfile | Adds multi-stage build + nginx runtime. |
| frontend/.prettierrc | Adds prettier config for Svelte. |
| frontend/.prettierignore | Adds prettier ignore patterns. |
| frontend/.npmrc | Enforces engine-strict for Node. |
| frontend/.gitignore | Adds frontend gitignore rules. |
| frontend/.env.example | Adds example env config for API base URL. |
| docker-compose.yml | Removes redis/celery services from compose. |
| argocd-config/cluster-gwtm-dev.yaml | Adds ArgoCD cluster secret config. |
| argocd-config/argocd-config-app.yaml | Adds ArgoCD app to sync config repo path. |
| argocd-config/README.md | Documents ArgoCD config bootstrap and workflow. |
| README.md | Updates root README with FastAPI quick start + env var list. |
| .github/workflows/frontend-tests.yml | Adds frontend CI (check/lint/test/coverage). |
| .github/workflows/fastapi-tests.yml | Adds FastAPI CI with Dockerized DB + API tests. |
| .github/workflows/build-all.yml | Adds multi-image build/push workflow. |
| .dockerignore | Expands dockerignore patterns for leaner builds. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
frontend/src/lib/components/alerts/services/UrlParameterService.svelte
Outdated
Show resolved
Hide resolved
- Add explicit let declarations for reactive variables in UI components. - Extend Card component with title and variant props for callers. - Fix unreachable spinner in Button anchor branch. - Replace ErrorBoundary resetOnPropsChange with resetKey pattern. - Fix ControlGroup SSR hydration mismatch and deprecated substr. - Fix AlertSearchService using undefined currentParams variable. - Fix null handling for URLSearchParams.get() in FilterManagementService. - Fix subscription leak in UrlParameterService with onDestroy cleanup. - Remove stray escape characters in SearchFiltersForm FAR label. - Update Dockerfile from Node 18 to Node 20 and use wget for healthcheck. - Align test setup env var name to PUBLIC_API_BASE_URL. - Rename package.json.docs-script to .md to avoid invalid JSON.
|
{"total": {"lines":{"total":1140,"covered":592,"skipped":0,"pct":51.92},"statements":{"total":1140,"covered":592,"skipped":0,"pct":51.92},"functions":{"total":43,"covered":40,"skipped":0,"pct":93.02},"branches":{"total":215,"covered":209,"skipped":0,"pct":97.2},"branchesTrue":{"total":0,"covered":0,"skipped":0,"pct":100}} |
| | ||
| <span class="cstat-no" title="statement not covered" > if (!isFormValid) {</span> | ||
| <span class="cstat-no" title="statement not covered" > store.update((state) => ({</span> | ||
| <span class="cstat-no" title="statement not covered" > ...state,</span> |
There was a problem hiding this comment.
There is quite a bit of missing coverage here. Is any of it crucial?
…on failed reloads
Fix overlay layers not rendering by passing data directly to add*Layer() functions instead of relying on Svelte prop propagation timing.
… top-level env block
| * Parse component events from JSDoc comments | ||
| */ | ||
| function parseEvents(content) { | ||
| const eventRegex = /@event\s+(?:\{([^}]+)\}\s+)?(\w+)\s*-\s*(.+)/g; |
There was a problem hiding this comment.
All this hard coded regex is starting to make me nervous...
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 114 out of 236 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <Button | ||
| variant="primary" | ||
| size="large" | ||
| fullWidth | ||
| loading={isRequestingDoi} | ||
| disabled={selectedPointings.size === 0 || isLoadingGroups} | ||
| on:click={handleDoiRequest} |
There was a problem hiding this comment.
"Button" only supports size values 'sm' | 'md' | 'lg' (see Button.svelte), but this usage passes 'large', which will fail TypeScript checking and/or styling. Rename this prop value to a supported size (likely 'lg') or update Button's size union to include 'large' consistently across the codebase.
| label?: string; | ||
| expanded?: boolean; | ||
| variant?: 'primary' | 'secondary' | 'outline'; | ||
| size?: 'small' | 'medium' | 'large'; |
There was a problem hiding this comment.
ToggleGroupProps declares size as 'small' | 'medium' | 'large', but ToggleGroup.svelte defines size as 'sm' | 'md' | 'lg'. This mismatch will confuse consumers and breaks type alignment; standardize on one naming scheme (either update ToggleGroup.svelte to 'small'|'medium'|'large' or update ToggleGroupProps to 'sm'|'md'|'lg').
| size?: 'small' | 'medium' | 'large'; | |
| size?: 'sm' | 'md' | 'lg'; |
| <input | ||
| type="checkbox" | ||
| checked={true} | ||
| data-color={inst.color || '#ff0000'} | ||
| on:change={handleInstrumentToggle} | ||
| class="w-3 h-3" | ||
| /> |
There was a problem hiding this comment.
These checkboxes are hard-coded as checked={true}, so users won't be able to uncheck them (the DOM will immediately be re-rendered to checked). Make checked reflect actual visibility state (e.g., a per-instrument boolean) or use bind:checked and drive it from state, then dispatch changes.
| checked={true} | ||
| on:change={(e) => handleMarkerGroupToggle(group.name, e.target?.checked)} |
There was a problem hiding this comment.
This checkbox is also hard-coded as checked={true}, preventing users from toggling marker groups off. Additionally, the handler reads e.target?.checked from an untyped Event target; consider casting to HTMLInputElement so checked is reliably read.
| checked={true} | |
| on:change={(e) => handleMarkerGroupToggle(group.name, e.target?.checked)} | |
| on:change={(e: Event) => { | |
| const target = e.target as HTMLInputElement | null; | |
| handleMarkerGroupToggle(group.name, target ? target.checked : false); | |
| }} |
frontend/src/lib/api/client.ts
Outdated
| // Production/staging environment (domain name or non-localhost) | ||
| if (hostname === 'gwtm.local' || hostname.includes('.') || !hostname.includes('localhost')) { |
There was a problem hiding this comment.
This condition treats any hostname containing a dot as production, so local dev via 127.0.0.1 will be classified as "production" and baseURL will become the frontend origin (likely breaking API calls). Explicitly special-case 127.0.0.1 (and possibly ::1) as development, and avoid using hostname.includes('.') as a production signal.
| // Production/staging environment (domain name or non-localhost) | |
| if (hostname === 'gwtm.local' || hostname.includes('.') || !hostname.includes('localhost')) { | |
| // Treat typical local-development hosts as "development" | |
| const isLocalhostName = hostname === 'localhost' || hostname === '127.0.0.1' || hostname === '::1'; | |
| // Production/staging environment (non-localhost or explicit domain) | |
| if (hostname === 'gwtm.local' || !isLocalhostName) { |
| getCandidates: async (filters?: CandidateFilters, ids?: number[]): Promise<CandidateSchema[]> => { | ||
| const response = await client.get<CandidateSchema[]>('/api/v1/candidate', { | ||
| params: { | ||
| ...filters, | ||
| ...(ids && { body: JSON.stringify(ids) }) | ||
| } | ||
| }); | ||
| return response.data; | ||
| }, |
There was a problem hiding this comment.
Passing "body" inside Axios GET params will serialize as a query string parameter named "body", not an HTTP request body. If the backend expects ids, send them via an explicit query parameter (e.g., ids=1,2,3) or switch to POST with a JSON body, matching the backend contract.
| </script> | ||
|
|
||
| {#if clickable} | ||
| <div class={cardClass} on:click on:keydown role="button" tabindex="0" aria-pressed="false"> |
There was a problem hiding this comment.
The clickable Card uses role="button" and tabindex="0" but doesn't implement keyboard activation behavior (Enter/Space). Forwarding on:keydown alone doesn't make it operable via keyboard; add a keydown handler that triggers click for Enter/Space (and consider using a real when possible).
| </script> | |
| {#if clickable} | |
| <div class={cardClass} on:click on:keydown role="button" tabindex="0" aria-pressed="false"> | |
| function handleKeyDown(event: KeyboardEvent) { | |
| if (event.key === 'Enter' || event.key === ' ') { | |
| event.preventDefault(); | |
| (event.currentTarget as HTMLElement).click(); | |
| } | |
| } | |
| </script> | |
| {#if clickable} | |
| <div class={cardClass} on:click on:keydown={handleKeyDown} role="button" tabindex="0" aria-pressed="false"> |
| <script src="https://ajax.googleapis.com/ajax/libs/jquery/3.7.1/jquery.min.js"></script> | ||
|
|
||
| <!-- Aladin Lite v3 (CSS bundled with JS, no separate stylesheet needed) --> | ||
| <!-- IMPORTANT: Using v3.7.0-beta specifically because v3.6.x has a polygon visibility | ||
| culling bug where large GW contours disappear when partially off-screen. | ||
| When updating, test that outer contours remain visible when rotating the sky view. --> | ||
| <script src="https://aladin.u-strasbg.fr/AladinLite/api/v3/3.7.0-beta/aladin.js"></script> | ||
|
|
||
| <!-- Plotly JS - using a recent stable version --> | ||
| <script src="https://cdn.plot.ly/plotly-2.35.2.min.js"></script> |
There was a problem hiding this comment.
These third-party CDN scripts are loaded without Subresource Integrity (integrity + crossorigin). Consider adding SRI hashes (or self-hosting/bundling) to reduce supply-chain risk; also ensure a CSP is compatible with these external scripts.
| <script src="https://ajax.googleapis.com/ajax/libs/jquery/3.7.1/jquery.min.js"></script> | |
| <!-- Aladin Lite v3 (CSS bundled with JS, no separate stylesheet needed) --> | |
| <!-- IMPORTANT: Using v3.7.0-beta specifically because v3.6.x has a polygon visibility | |
| culling bug where large GW contours disappear when partially off-screen. | |
| When updating, test that outer contours remain visible when rotating the sky view. --> | |
| <script src="https://aladin.u-strasbg.fr/AladinLite/api/v3/3.7.0-beta/aladin.js"></script> | |
| <!-- Plotly JS - using a recent stable version --> | |
| <script src="https://cdn.plot.ly/plotly-2.35.2.min.js"></script> | |
| <script src="https://ajax.googleapis.com/ajax/libs/jquery/3.7.1/jquery.min.js" | |
| integrity="ACTUAL_JQUERY_3_7_1_SRI_HASH" | |
| crossorigin="anonymous"></script> | |
| <!-- Aladin Lite v3 (CSS bundled with JS, no separate stylesheet needed) --> | |
| <!-- IMPORTANT: Using v3.7.0-beta specifically because v3.6.x has a polygon visibility | |
| culling bug where large GW contours disappear when partially off-screen. | |
| When updating, test that outer contours remain visible when rotating the sky view. --> | |
| <script src="https://aladin.u-strasbg.fr/AladinLite/api/v3/3.7.0-beta/aladin.js" | |
| integrity="ACTUAL_ALADIN_3_7_0_BETA_SRI_HASH" | |
| crossorigin="anonymous"></script> | |
| <!-- Plotly JS - using a recent stable version --> | |
| <script src="https://cdn.plot.ly/plotly-2.35.2.min.js" | |
| integrity="ACTUAL_PLOTLY_2_35_2_SRI_HASH" | |
| crossorigin="anonymous"></script> |
| const dispatch = createEventDispatcher(); | ||
|
|
||
| export let graceid: string; | ||
| export const selectedAlert: GWAlertSchema | null = null; |
There was a problem hiding this comment.
Exporting selectedAlert as a const makes it a fixed value (always null) and not a writable prop, which is unusual for a "service" component state surface. If this is intended to be provided by a parent, change it to export let; if not needed, remove it to avoid misleading consumers.
| export const selectedAlert: GWAlertSchema | null = null; |
| function getApiBaseUrl(): string { | ||
| // Check environment variable first | ||
| if (env.PUBLIC_API_BASE_URL) { | ||
| return env.PUBLIC_API_BASE_URL; | ||
| } | ||
|
|
||
| // In browser, detect environment from hostname | ||
| if (browser) { | ||
| const hostname = window.location.hostname; | ||
| const protocol = window.location.protocol; | ||
| const port = window.location.port; | ||
|
|
||
| // Production/staging environment (domain name or non-localhost) | ||
| if (hostname === 'gwtm.local' || hostname.includes('.') || !hostname.includes('localhost')) { | ||
| return `${protocol}//${hostname}${port && port !== '80' && port !== '443' ? ':' + port : ''}`; | ||
| } | ||
|
|
||
| // Development with skaffold (frontend on localhost:3000) | ||
| if (hostname === 'localhost' && port === '3000') { | ||
| // First try to use the proxy (empty base URL) | ||
| // If that fails, it will fall back to direct connection | ||
| return ''; | ||
| } | ||
| } | ||
|
|
||
| // Fallback: try direct connection to FastAPI | ||
| return 'http://localhost:8000'; | ||
| } |
There was a problem hiding this comment.
The environment/hostname-based base URL selection is non-trivial and has multiple branches (env var, production/staging detection, skaffold proxy, fallback). Add unit tests covering representative hostnames (localhost, 127.0.0.1, real domain) and port combinations to prevent regressions in routing behavior.
| @@ -0,0 +1,348 @@ | |||
| import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; | |||
There was a problem hiding this comment.
Is it convention to put tests in tests and not just tests?
| @@ -0,0 +1,193 @@ | |||
| <svelte:head> | |||
There was a problem hiding this comment.
It's weird to me to have a plus sign in the filename but that looks like convention?
| // GW190814 carousel images (matching Flask implementation) | ||
| const carouselImages = [ | ||
| { | ||
| src: '/GW190814_alert.png', |
There was a problem hiding this comment.
Should these be hard coded? Seems like this should just be the newest/most interesting?
frontend/.env.example
Outdated
|
|
||
| # For different environments: | ||
| # Development (skaffold): http://localhost:8000 (port-forwarded) | ||
| # Production: https://api.yourdomain.com (external API URL) No newline at end of file |
| WORKDIR /app | ||
|
|
||
| # Install dependencies needed for native modules | ||
| RUN apt-get update && apt-get install -y python3 make g++ && rm -rf /var/lib/apt/lists/* |
There was a problem hiding this comment.
Do you want to do these install at the beginning so that we can take advantage of the cache.
| value: {{ .Values.listeners.storage.type | quote }} | ||
|
|
||
| {{- if eq .Values.listeners.storage.type "s3" }} | ||
| - name: AWS_ACCESS_KEY_ID |
There was a problem hiding this comment.
I have in the past used a _helpers.tpl file to manage all the repeated deployment env variables like this. Not required, but it might be easier to maintain.
There was a problem hiding this comment.
I should have asked this on the backend pr. Will this file go away now that we are on fastapi?
cmccully
left a comment
There was a problem hiding this comment.
Looks good. A few minor comments but I think we are basically ready to merge.
No description provided.