perf(dashboard): bundle menor, corrige bugs e restringe CSP#15
Conversation
- removed @mui/x-data-grid and recoil from dependencies - added package-lock.json for dependency management
- refine content security policy for improved security - remove unused dependencies from Vite configuration
- deleted Gallery and PerformanceMonitor components to streamline the codebase - removed unused columnWidth utility file
- set BASE_PATH and VITE_MINIO_ENDPOINT in build step for better configuration
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThis PR refactors the dashboard by removing Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
- update assignment method to use `.at[]` for scalar access - ensure compatibility with pandas >= 2.x to avoid errors
There was a problem hiding this comment.
Pull request overview
This PR optimizes the dashboard bundle/runtime performance and tightens security defaults (CSP + configurable MinIO endpoint), while also removing unused/dead code and dependencies.
Changes:
- Removes
@mui/x-data-gridandrecoil, replacing grids with lighter-weight MUI Table + a customVirtualizedList. - Refactors SkyMap rendering (2-canvas layering) and moves projection work to a Vite
?workermodule worker with a main-thread fallback. - Tightens CSP and updates deployment to inject
VITE_MINIO_ENDPOINTat build time; adjusts cutout fetching/caching behavior.
Reviewed changes
Copilot reviewed 17 out of 20 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| package-lock.json | Adds a root lockfile (new). |
| dashboard/vite.config.ts | Removes DataGrid/recoil-related chunking and dep optimization. |
| dashboard/src/workers/skyProjectionWorker.ts | Refines worker implementation and typing for sky projection. |
| dashboard/src/theme.ts | Updates theme to remove backdrop-filter/translucency-heavy styling. |
| dashboard/src/components/VirtualizedList.tsx | Adds scrollToIndex + stable item keys for virtualization. |
| dashboard/src/components/SkyMap.tsx | Refactors worker usage + rendering pipeline with base/overlay canvases. |
| dashboard/src/components/ObjectsTable.tsx | Replaces DataGrid with VirtualizedList-backed list UI. |
| dashboard/src/components/FiltersDrawer.tsx | Removes internal debounce and simplifies drawer paper styling. |
| dashboard/src/components/DataTables.tsx | Replaces DataGrid with MUI Table + pagination. |
| dashboard/src/components/CutoutGrid.tsx | Removes query invalidation + blob revocation; adds referrerPolicy. |
| dashboard/src/api.ts | Makes MinIO endpoint configurable; introduces blob URL caching policy changes. |
| dashboard/src/App.tsx | Consolidates indexing/domain computation; unifies debounce timing; integrates new tables. |
| dashboard/package.json | Drops @mui/x-data-grid and recoil dependencies. |
| dashboard/package-lock.json | Removes DataGrid/recoil transitive deps from lockfile. |
| dashboard/index.html | Tightens CSP directives (removes http/ws/wss allowances; adds base-uri/frame-ancestors etc.). |
| .github/workflows/deploy.yml | Passes VITE_MINIO_ENDPOINT into the build environment. |
| dashboard/src/components/PerformanceMonitor.tsx | Removes dead code. |
| dashboard/src/components/Gallery.tsx | Removes dead code. |
| dashboard/src/utils/columnWidth.ts | Removes dead code. |
Files not reviewed (1)
- dashboard/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
dashboard/src/theme.ts (1)
42-60: LGTM — note minor first-paint mismatch withindex.htmlinline style.
dashboard/index.html(lines 12–16) still applies a radial gradient (radial-gradient(circle at 20% 20%,#13242d,#091015,#060b0f)) tobody, while thisMuiCssBaselineoverride replaces it with a different linear gradient on#03060a. There's a brief perceptible color/shape change on first paint while the JS bundle hydrates and CssBaseline takes over.Optional: align the inline
<style>inindex.htmlwith this theme's gradient to avoid the flash, or drop the inline gradient and keep onlybackground-color:#03060a`` until CssBaseline runs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dashboard/src/theme.ts` around lines 42 - 60, The inline style in index.html is causing a first-paint flash because it uses a different radial gradient than the theme's MuiCssBaseline override; update index.html's inline <style> (body rule) to either match the MuiCssBaseline backgroundImage ('linear-gradient(135deg, `#04080d` 0%, `#061018` 50%, `#04080d` 100%)') or remove the inline gradient entirely and leave only background-color: `#03060a` so the initial paint matches the theme provided by MuiCssBaseline and avoids a perceptible color/shape change during hydration..github/workflows/deploy.yml (1)
58-64: Fail-fast on missingVITE_MINIO_ENDPOINTto avoid a silently-broken deploy.If the GH secret is not set (the PR notes this is required pre-merge),
${{ secrets.VITE_MINIO_ENDPOINT }}resolves to an empty string, the build still succeeds, and prod ships with cutouts pointing at the GH Pages base URL → 404 with no obvious CI signal. A one-line guard catches this at build time:🛡️ Suggested guard
- name: Build env: BASE_PATH: /slcomp/ VITE_MINIO_ENDPOINT: ${{ secrets.VITE_MINIO_ENDPOINT }} run: | cd dashboard + if [ -z "${VITE_MINIO_ENDPOINT}" ]; then + echo "::error::VITE_MINIO_ENDPOINT secret is not set; cutouts will 404 in production." + exit 1 + fi + case "${VITE_MINIO_ENDPOINT}" in + https://*) ;; + *) echo "::error::VITE_MINIO_ENDPOINT must be https:// (CSP forbids http)"; exit 1 ;; + esac npm run buildThe second check pairs with the new CSP
connect-src 'self' https:indashboard/index.htmlso anhttp://endpoint is rejected at build time rather than failing silently in the browser at runtime.Also: please make sure the team is aware that
VITE_*values are inlined into the public JS bundle — putting the endpoint behind GH Secrets does not keep it confidential, only out of source control. If hiding the endpoint matters, it must be fronted by a same-origin proxy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/deploy.yml around lines 58 - 64, Add a fail-fast guard in the Build step of the GitHub Actions workflow to abort the job when the required VITE_MINIO_ENDPOINT secret is missing or insecure: in the Build step (the job named "Build" that sets env VITE_MINIO_ENDPOINT and runs cd dashboard && npm run build) insert a short shell check before running the build that exits non‑zero if $VITE_MINIO_ENDPOINT is empty or begins with "http://". This ensures the workflow fails early on a missing or non‑TLS endpoint and prevents a silently broken prod build.dashboard/src/workers/skyProjectionWorker.ts (1)
63-75: Optional: replaceself as unknown as Workerwith proper worker typing.Inside a worker,
selfisDedicatedWorkerGlobalScope, notWorker. The double-cast works but obscures the type. Cleaner alternatives:♻️ Suggested cleanup
+declare const self: DedicatedWorkerGlobalScope; self.onmessage = (e: MessageEvent) => { const { objects, requestId } = e.data as { objects: SkyObject[]; requestId: number }; try { const projectedPoints = projectObjects(objects); - (self as unknown as Worker).postMessage({ requestId, projectedPoints, success: true }); + self.postMessage({ requestId, projectedPoints, success: true }); } catch (error) { - (self as unknown as Worker).postMessage({ + self.postMessage({ requestId, error: error instanceof Error ? error.message : 'Unknown error', success: false }); } };Or add
"webworker"to thelibarray indashboard/tsconfig.json:"lib": ["DOM", "DOM.Iterable", "ES2020", "webworker"]The typed
MessageEventhandler andtoNumextraction are correct—NaNcoercions are filtered at line 49 before any trig math, so untrusted input stays safe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dashboard/src/workers/skyProjectionWorker.ts` around lines 63 - 75, The worker currently uses a double-cast "(self as unknown as Worker).postMessage" which obscures proper typing; replace that with the correct DedicatedWorkerGlobalScope typing (e.g., const workerScope = self as DedicatedWorkerGlobalScope; use workerScope.postMessage(...) in the self.onmessage handler) or add "webworker" to the dashboard/tsconfig.json "lib" array so TypeScript recognizes self as a worker global; update both the success and error postMessage calls in the onmessage handler (the block that calls projectObjects and posts results) to use the properly typed scope instead of the double-cast.dashboard/src/App.tsx (1)
24-29: Optional: drop the unsafeas unknown ascast onNUMERIC_FIELDS.
NUMERIC_FIELDSisas const(readonly tuple), so the cast widens it throughunknown. IfFiltersDrawer's prop is widened toreadonly { key: string; label: string }[](or a generic), the cast becomes unnecessary and you keep compile-time guarantees that field keys match the domain map.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dashboard/src/App.tsx` around lines 24 - 29, NUMERIC_FIELDS is declared with "as const" but an unsafe "as unknown as" cast was used to satisfy FiltersDrawer; remove that cast and instead widen the FiltersDrawer prop type to accept readonly tuples or a ReadonlyArray of the field shape (e.g. change the prop from mutable array type to readonly { key: string; label: string }[] or make FiltersDrawer generic over the tuple type), then keep NUMERIC_FIELDS as "as const" so callers retain the literal/readonly types without the unsafe cast.dashboard/src/components/SkyMap.tsx (1)
402-462: Pan churns the interactionuseEffect— listeners are torn down/re-attached every mousemove.
transformis recomputed each timepanchanges (and pan changes per pointermove viasetPan). Becausetransformis in the dep array, this effect detaches and re-adds all 7 DOM listeners on every frame of a drag. On a busy SkyMap this is measurable.Move the click hit-test's coordinate inputs through refs so the effect can run once with a stable handler.
Sketch
+ const transformRef = useRef(transform); + const ptsRef = useRef(pts); + const zoomRef = useRef(zoom); + useEffect(() => { transformRef.current = transform; }, [transform]); + useEffect(() => { ptsRef.current = pts; }, [pts]); + useEffect(() => { zoomRef.current = zoom; }, [zoom]); @@ - useEffect(() => { + useEffect(() => { const canvas = overlayCanvasRef.current; if (!canvas) return; ... - const { baseScaleX, baseScaleY, centerX, centerY } = transform; + const { baseScaleX, baseScaleY, centerX, centerY } = transformRef.current; + const z = zoomRef.current; + const points = ptsRef.current; ... - }, [pts, transform, zoom, onSelect]); + }, [onSelect]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dashboard/src/components/SkyMap.tsx` around lines 402 - 462, The effect attaches DOM listeners on overlayCanvasRef in useEffect but includes changing values (transform, pts, zoom, onSelect) in its dependency array so listeners are torn down/re-attached on every pan/mousemove; fix it by stabilizing the handler inputs via refs: create refs like ptsRef, transformRef (or separate baseScaleXRef/baseScaleYRef/centerXRef), zoomRef and onSelectRef and update them wherever pts/transform/zoom/onSelect change, then inside the useEffect handlers (onClick, onWheel, onMove, etc.) read from those refs instead of closing over the state; finally remove transform/pts/zoom/onSelect from the useEffect dependency array so the listeners are added once for overlayCanvasRef and only cleaned up on unmount.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dashboard/index.html`:
- Line 8: The CSP prevents plain http: connections which breaks cutout image
fetches when MinIO is configured with http; update the deployment/docs and
runtime checks: ensure the production secret VITE_MINIO_ENDPOINT uses https://
and document the HTTPS requirement in the README/deployment guide, and/or add a
runtime validation in buildCutoutUrl (or wherever VITE_MINIO_SCHEME /
VITE_MINIO_ENDPOINT are read) to reject or warn when the scheme is http (or
automatically upgrade to https), and if http support is required, note that you
must relax the CSP meta tag (connect-src/img-src) or front MinIO with an HTTPS
reverse proxy so browsers can load images.
In `@dashboard/src/api.ts`:
- Around line 25-32: The catch currently turns any AbortError into a "Request
timeout for ${url}" message even when the abort came from elsewhere; update the
logic around the fetch timeout (the timeout callback that references timeoutId)
to set a boolean flag (e.g., timedOut) when the 30s timer fires, then in the
catch check err instanceof Error && err.name === 'AbortError' && timedOut to
throw the "Request timeout for ${url}" error; otherwise rethrow or throw a new
Error that includes the original err.message (preserve the original error
details). Ensure references: timeoutId, timedOut (new flag), AbortError checks,
and url are used to locate and apply the change.
- Around line 88-101: The fetch call that sets the custom header
skip_zrok_interstitial (and Accept: 'image/*') causes a CORS preflight in
production which prevents blobCache from being populated when MinIO's CORS
policy doesn't allow that header; update the MinIO bucket CORS to include
Access-Control-Allow-Headers: skip_zrok_interstitial, Accept and
Access-Control-Allow-Methods: GET, HEAD, OPTIONS and ensure
Access-Control-Allow-Origin matches the dashboard origin so the fetch in the
function that creates blobUrl and sets blobCache (the fetch(...) ->
response.blob() / blobCache.set(...) flow) succeeds in production.
In `@dashboard/src/components/DataTables.tsx`:
- Around line 27-31: The current useMemo for computing columns only inspects
Object.keys(rows[0]) so any keys missing from the first row are dropped; change
the logic in the useMemo (the columns constant) to compute the union of keys
across all rows (e.g., iterate rows and collect every key except 'id') and then
filter that union to remove keys where every row has an empty value via
isEmptyVal, preserving the existing filtering behavior but ensuring
heterogeneous rows contribute columns.
In `@dashboard/src/components/SkyMap.tsx`:
- Around line 80-112: The onmessage handler in the SkyMap constructor currently
ignores messages where success is false causing the pending resolve to wait for
the 15s timeout; update the handler to handle error replies by retrieving the
pending entry from this.pending (which should store both resolve and the
original objects if you choose the suggested approach), and immediately call the
resolve with projectMainThread(objects) (or reject/log the worker error) and
delete the pending entry; modify where you set this.pending in project(...) (and
the pending Map shape) so entries include the original objects (or an error
handler) and ensure SkyProjectionWorker, constructor, onmessage, project,
this.pending, this.nextId and projectMainThread are used to locate and implement
the fix.
---
Nitpick comments:
In @.github/workflows/deploy.yml:
- Around line 58-64: Add a fail-fast guard in the Build step of the GitHub
Actions workflow to abort the job when the required VITE_MINIO_ENDPOINT secret
is missing or insecure: in the Build step (the job named "Build" that sets env
VITE_MINIO_ENDPOINT and runs cd dashboard && npm run build) insert a short shell
check before running the build that exits non‑zero if $VITE_MINIO_ENDPOINT is
empty or begins with "http://". This ensures the workflow fails early on a
missing or non‑TLS endpoint and prevents a silently broken prod build.
In `@dashboard/src/App.tsx`:
- Around line 24-29: NUMERIC_FIELDS is declared with "as const" but an unsafe
"as unknown as" cast was used to satisfy FiltersDrawer; remove that cast and
instead widen the FiltersDrawer prop type to accept readonly tuples or a
ReadonlyArray of the field shape (e.g. change the prop from mutable array type
to readonly { key: string; label: string }[] or make FiltersDrawer generic over
the tuple type), then keep NUMERIC_FIELDS as "as const" so callers retain the
literal/readonly types without the unsafe cast.
In `@dashboard/src/components/SkyMap.tsx`:
- Around line 402-462: The effect attaches DOM listeners on overlayCanvasRef in
useEffect but includes changing values (transform, pts, zoom, onSelect) in its
dependency array so listeners are torn down/re-attached on every pan/mousemove;
fix it by stabilizing the handler inputs via refs: create refs like ptsRef,
transformRef (or separate baseScaleXRef/baseScaleYRef/centerXRef), zoomRef and
onSelectRef and update them wherever pts/transform/zoom/onSelect change, then
inside the useEffect handlers (onClick, onWheel, onMove, etc.) read from those
refs instead of closing over the state; finally remove
transform/pts/zoom/onSelect from the useEffect dependency array so the listeners
are added once for overlayCanvasRef and only cleaned up on unmount.
In `@dashboard/src/theme.ts`:
- Around line 42-60: The inline style in index.html is causing a first-paint
flash because it uses a different radial gradient than the theme's
MuiCssBaseline override; update index.html's inline <style> (body rule) to
either match the MuiCssBaseline backgroundImage ('linear-gradient(135deg,
`#04080d` 0%, `#061018` 50%, `#04080d` 100%)') or remove the inline gradient entirely
and leave only background-color: `#03060a` so the initial paint matches the theme
provided by MuiCssBaseline and avoids a perceptible color/shape change during
hydration.
In `@dashboard/src/workers/skyProjectionWorker.ts`:
- Around line 63-75: The worker currently uses a double-cast "(self as unknown
as Worker).postMessage" which obscures proper typing; replace that with the
correct DedicatedWorkerGlobalScope typing (e.g., const workerScope = self as
DedicatedWorkerGlobalScope; use workerScope.postMessage(...) in the
self.onmessage handler) or add "webworker" to the dashboard/tsconfig.json "lib"
array so TypeScript recognizes self as a worker global; update both the success
and error postMessage calls in the onmessage handler (the block that calls
projectObjects and posts results) to use the properly typed scope instead of the
double-cast.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 55ae10b9-e914-4d86-b725-615e3eb6e6a9
⛔ Files ignored due to path filters (2)
dashboard/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
.github/workflows/deploy.ymldashboard/index.htmldashboard/package.jsondashboard/src/App.tsxdashboard/src/api.tsdashboard/src/components/CutoutGrid.tsxdashboard/src/components/DataTables.tsxdashboard/src/components/FiltersDrawer.tsxdashboard/src/components/Gallery.tsxdashboard/src/components/ObjectsTable.tsxdashboard/src/components/PerformanceMonitor.tsxdashboard/src/components/SkyMap.tsxdashboard/src/components/VirtualizedList.tsxdashboard/src/theme.tsdashboard/src/utils/columnWidth.tsdashboard/src/workers/skyProjectionWorker.tsdashboard/vite.config.ts
💤 Files with no reviewable changes (2)
- dashboard/src/components/PerformanceMonitor.tsx
- dashboard/src/components/Gallery.tsx
- SkyMap WorkerPool: handle worker error replies (success:false) by falling back to main-thread immediately, and clear per-request setTimeout on success - SkyMap: stable interaction listeners via refs (transform/pts/zoom/ onSelect) so they aren't reattached on every pan - SkyMap: cache selected point via Map<jname, ProjectedPoint> so the overlay rAF loop is O(1) per frame instead of pts.find - SkyMap: HMR-safe beforeunload via import.meta.hot.dispose - DataTables: derive columns from union of keys across rows, clamp page on row-count shrink - VirtualizedList: ignore scrollToIndex when >= items.length - api.ts: timedOut flag distinguishes 30s timeout from external aborts; DEBUG_CUTOUTS only accepts explicit "true"/"1" - skyProjectionWorker: proper DedicatedWorkerGlobalScope typing (tsconfig lib += "WebWorker") - App.tsx: drop unsafe `as unknown as` cast on NUMERIC_FIELDS via `satisfies readonly NumericFilterConfig[]` - FiltersDrawer: numericFields prop is now readonly Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 21 changed files in this pull request and generated 6 comments.
Files not reviewed (1)
- dashboard/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- index.html: restore ws/wss in connect-src so Vite HMR keeps working in dev (the meta CSP applies in both dev and prod; for stricter prod hardening, drop them via server headers on deploy) - api.ts: rewrite blobCache comment to honestly describe the session-lifetime policy and when an LRU would be warranted - CutoutGrid: surface broken images via <img onError> + local state. getCutoutObject still returns the raw URL on fetch failure (so the browser gets a second chance), but if <img> itself fails the card now shows the Error placeholder instead of a silent broken image - VirtualizedList: explicitly setScrollTop(el.scrollTop) after a programmatic scroll, instead of relying on the scroll event firing - SkyMap: stop reading isPanningRef.current from JSX (cursor stayed stuck at 'grabbing' since refs don't trigger re-renders). Drive cursor via canvas.style.cursor in onDown/onUp, and guard releasePointerCapture against not-captured pointers Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 21 changed files in this pull request and generated 4 comments.
Files not reviewed (1)
- dashboard/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Resumo
backdrop-filter(principal causa de jank de scroll), trocando por gradientes opacos com a mesma estética dark/aqua.@mui/x-data-grid(~108 KB gz) erecoil(não usado): bundle inicial servido cai de ~285 KB gz para ~162 KB gz.ObjectsTableporVirtualizedListe emDataTablesporTabledo@mui/material(já no bundle).SkyMapem dois canvases (estático + overlay animado) e worker carregado via?workerimport.database/consolidated/cutoutspor JNAME → seleção O(1).VITE_MINIO_ENDPOINT.Bugs corrigidos
revokeBlobUrlno unmount e oinvalidateQueriesno mount doCutoutGrid(que anulava o cache a cada navegação).step=10perdia mínimos/máximos reais — substituído por uma única passada em loop.Math.min(...arr)para arrays grandes — trocado por loop.api.tsignorandoVITE_MINIO_ENDPOINT.Gallery(componente removido — era dead code).Segurança
http:/ws:/wss:; adicionadosbase-uri 'self',form-action 'self',frame-ancestors 'none'.VITE_MINIO_ENDPOINTno build;<img>comreferrerPolicy="no-referrer".CI
.github/workflows/deploy.ymlpassaVITE_MINIO_ENDPOINT(secret) no step de build.VITE_*é inlined no bundle (visível no JS público).Bundle (gzip)
mui-coremui-datagridrecoilLimpeza
Arquivos removidos por estarem mortos:
Gallery.tsx,PerformanceMonitor.tsx,utils/columnWidth.ts.Test plan
npm run build(0 erros)npm run lint(0 erros; só warnings deanypré-existentes)npm run previewcomBASE_PATH=/slcomp/eVITE_MINIO_ENDPOINTsetadoVITE_MINIO_ENDPOINTno GitHub antes do mergeworkflow_dispatche validar o deploySummary by CodeRabbit
New Features
Bug Fixes
Improvements
Dependencies