Skip to content

Conversation

@oliveirara
Copy link
Contributor

@oliveirara oliveirara commented Sep 8, 2025

  • update python version to 3.12
  • update node version to 20
  • remove devcontainer.json
  • fix content security policy to allow websocket connections
  • fix mollweide projection to handle poles correctly
  • fix a bug in the mollweide projection that caused the y-axis to be inverted
  • refactor VirtualizedList to fix a type issue
  • set RAW_ENDPOINT to ngrok URL

Summary by CodeRabbit

  • Bug Fixes

    • Sky map rendering stabilized near poles and vertically flipped for correct orientation.
    • Favicon now loads reliably; dashboard supports WebSocket connections and web workers.
  • Chores

    • Removed development container setup.
    • CI updated to Python 3.12 and Node.js 20.
    • Streamlined data endpoint configuration.
  • Refactor

    • Internal rework of the virtualized list component for clarity without changing behavior.

- update python version to 3.12
- update node version to 20
- remove devcontainer.json
- fix content security policy to allow websocket connections
- fix mollweide projection to handle poles correctly
- fix a bug in the mollweide projection that caused the y-axis to be inverted
- refactor VirtualizedList to fix a type issue
- set RAW_ENDPOINT to ngrok URL
@oliveirara oliveirara self-assigned this Sep 8, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 8, 2025

Walkthrough

Removes the devcontainer configuration, updates CI to Python 3.12 and Node 20, adjusts dashboard HTML CSP and favicon path, hard-codes an API endpoint and drops two env typings, updates Mollweide projection math and orientation in SkyMap, and refactors VirtualizedList into an inner function with memoized export.

Changes

Cohort / File(s) Summary
Devcontainer removal
/.devcontainer/devcontainer.json
Deleted Codespaces/devcontainer config (Python 3.11 image, VS Code extensions, setup commands, Streamlit run, port forwarding).
CI workflow updates
/.github/workflows/deploy.yml
Bumped actions/setup-python to v5; Python version 3.11 → 3.12; Node.js 18 → 20.
Dashboard HTML policy and assets
/dashboard/index.html
CSP: connect-src now includes ws:/wss:, added worker-src 'self' blob:; favicon path made relative (telescope.png).
API endpoint and env typings
/dashboard/src/api.ts, /dashboard/src/env.d.ts
api.ts: replaced env-derived endpoint with hard-coded host; retains ngrok header logic. env.d.ts: removed VITE_MINIO_ACCESS_KEY and VITE_MINIO_SECRET_KEY typings.
SkyMap projection math
/dashboard/src/components/SkyMap.tsx
Enhanced solveTheta: pole handling, clamped initial guess, more iterations, tighter tolerance, derivative-guard; flipped y = -√2 sin(theta); comments updated.
VirtualizedList refactor
/dashboard/src/components/VirtualizedList.tsx
Introduced VirtualizedListInner; exported memo(VirtualizedListInner) with type assertion; no behavioral change intended.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

I twitch my nose at CSPs new,
With sockets humming, worker too.
The skies flip south in Mollweide light,
Lists stay swift, still scrolling right.
Devcontainer hops away—goodbye!
CI munches versions high.
A fixed endpoint? Carrot-locked, aye! 🥕🐇

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fixes

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@oliveirara oliveirara merged commit fdea387 into main Sep 8, 2025
2 of 3 checks passed
@oliveirara oliveirara deleted the fixes branch September 8, 2025 03:31
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
dashboard/src/api.ts (1)

64-67: Remove forbidden header: User-Agent cannot be set in browsers.

fetch will reject or ignore this header, potentially breaking image loads. Keep only ngrok-skip-browser-warning.

Apply:

         headers: {
           'ngrok-skip-browser-warning': 'true',
-          'User-Agent': 'LaStBeRu-Explorer/1.0 (Custom)'
         },
dashboard/src/components/SkyMap.tsx (2)

21-24: Fix RA→λ mapping: current function contradicts its own comment.

raToLon should map RA=180→λ=0 and make RA increase to the left. The current formula maps RA=180 to ±180.

Apply:

-// Convert RA (deg 0..360) to Mollweide longitude λ (deg -180..180) with RA=180 at λ=0 (center) and RA increasing to the left.
-// Formula: shift so 180 -> 0 then wrap, then negate for leftwards increase.
-const raToLon = (raDeg: number) => {
-  const wrapped = ((raDeg + 180) % 360) - 180; // RA=180 -> 0, RA=0 -> -180, RA=360-> -180
-  return -wrapped; // invert to make RA increase to the left
-};
+// Convert RA (0..360) to longitude λ (-180..180) with RA=180 at λ=0 and RA increasing to the left.
+const raToLon = (raDeg: number) => {
+  // RA normalized upstream to [0,360); this yields λ in [-180,180]
+  return 180 - raDeg;
+};

175-189: Y-axis sign is inconsistent between meridians and parallels.

Meridians and plotted points use y = -√2·sinθ, but parallels use +√2·sinθ, flipping them vertically.

Apply:

-        const y = Math.SQRT2 * Math.sin(theta);
+        const y = -Math.SQRT2 * Math.sin(theta);
🧹 Nitpick comments (6)
.github/workflows/deploy.yml (1)

36-40: Consider pinning and caching Python deps for reproducible, faster builds.

Move these packages into a requirements file and enable setup-python caching. Optional, but improves reliability.

Example (if you add dashboard/requirements.txt):

       - name: Setup Python
         uses: actions/setup-python@v5
         with:
           python-version: '3.12'
+          cache: 'pip'
+          cache-dependency-path: 'dashboard/requirements.txt'

       - name: Install Python dependencies
-        run: |
-          cd dashboard
-          pip install pandas numpy minio pyarrow fastparquet
+        run: |
+          cd dashboard
+          pip install -r requirements.txt
dashboard/index.html (1)

8-8: Harden CSP against clickjacking and URL injection (optional).

Add frame-ancestors 'none' and base-uri 'self'. These don’t affect current behavior and increase protection.

Proposed meta content additions:

-    <meta http-equiv="Content-Security-Policy" content="default-src 'self'; img-src 'self' data: blob: https: http:; style-src 'self' 'unsafe-inline'; script-src 'self'; connect-src 'self' https: http: ws: wss:; worker-src 'self' blob:; object-src 'none';" />
+    <meta http-equiv="Content-Security-Policy" content="default-src 'self'; img-src 'self' data: blob: https: http:; style-src 'self' 'unsafe-inline'; script-src 'self'; connect-src 'self' https: http: ws: wss:; worker-src 'self' blob:; object-src 'none'; base-uri 'self'; frame-ancestors 'none';" />
dashboard/src/api.ts (2)

59-80: Avoid leaking blob URLs (revoke when no longer needed).

Returning a blob URL without revocation can leak memory during long sessions. Consider returning the Response/blob to the caller to manage lifecycle, or provide a revoker.

Example pattern:

// return { url, revoke: () => URL.revokeObjectURL(url) }

103-104: Gate debug logs behind an env flag.

Noisy logs in production; align with the existing VITE_DEBUG_CUTOUTS flag.

if ((import.meta as any).env?.VITE_DEBUG_CUTOUTS) console.log('[DEBUG] Using direct URL:', url);
dashboard/src/components/VirtualizedList.tsx (1)

84-84: Preserve generics through memo with an explicit call signature.

Casting to typeof VirtualizedListInner isn’t accurate; define the generic call signature to keep type inference at call sites.

Apply:

-export const VirtualizedList = memo(VirtualizedListInner) as typeof VirtualizedListInner;
+export const VirtualizedList = memo(VirtualizedListInner) as <T>(p: VirtualizedListProps<T>) => JSX.Element;
dashboard/src/components/SkyMap.tsx (1)

169-170: Minor: add missing semicolon for consistency.

-        const y = -Math.SQRT2 * Math.sin(theta)
+        const y = -Math.SQRT2 * Math.sin(theta);
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 699d1a3 and 4b9c9b4.

📒 Files selected for processing (7)
  • .devcontainer/devcontainer.json (0 hunks)
  • .github/workflows/deploy.yml (1 hunks)
  • dashboard/index.html (1 hunks)
  • dashboard/src/api.ts (1 hunks)
  • dashboard/src/components/SkyMap.tsx (2 hunks)
  • dashboard/src/components/VirtualizedList.tsx (2 hunks)
  • dashboard/src/env.d.ts (0 hunks)
💤 Files with no reviewable changes (2)
  • .devcontainer/devcontainer.json
  • dashboard/src/env.d.ts
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/deploy.yml

[error] 28-28: trailing spaces

(trailing-spaces)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build
🔇 Additional comments (5)
.github/workflows/deploy.yml (1)

25-27: Python 3.12 and Node 20 upgrade — LGTM

Action versions and runtimes look correct for ubuntu-latest.

Also applies to: 32-32

dashboard/index.html (2)

8-8: CSP update to allow ws/wss and workers — LGTM

This unblocks dev HMR/WebSockets and web workers while keeping a restrictive policy.


10-10: Relative favicon path for GitHub Pages — LGTM

Avoids root-absolute path issues under /slcomp/.

dashboard/src/components/VirtualizedList.tsx (1)

47-54: Non-virtualized path — LGTM

Simple small-list fallback avoids virtualization overhead.

dashboard/src/components/SkyMap.tsx (1)

30-45: Newton solver improvements — solid.

Pole handling, tighter tolerance, and derivative guard look good for stability near |φ|≈π/2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants