Feature 198 custom overlay themes#201
Conversation
- Backend: auto-discover routers from apps/backend/routes/ - Frontend: Dashboard.jsx dynamically loads feature components from features/ - Existing routes updated with prefixes for auto-discovery
- Backend: theme CRUD API with preset and custom theme support - Frontend: ThemeSelector, ThemeCustomizer, and theme utilities - Feature tab registered in Dashboard via dynamic feature discovery - Added integration tests for theme API
📝 WalkthroughWalkthroughThe backend now autodiscovers prefixed feature routers and exposes a theme management API. The dashboard adds theme presets, custom-theme persistence and editing, plus dynamically loaded feature tabs with an Overlay Themes view. ChangesTheme platform and dashboard integration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Dashboard
participant ViteModuleLoader
participant themes.jsx
participant ThemeSelector
Dashboard->>ViteModuleLoader: import.meta.glob("./features/*.jsx")
ViteModuleLoader->>themes.jsx: load feature module
themes.jsx->>ThemeSelector: export ThemeSelector and label
Dashboard->>ThemeSelector: render selected feature component
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 Warning |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
apps/dashboard/src/components/ThemeCustomizer.jsx (1)
55-55: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAvoid reading from
localStoragesynchronously during every render.Reading from
localStoragedirectly in the component body means a blocking I/O call is executed on every re-render (e.g., every keystroke in the theme name or drag of a slider).Consider reading it once into a state variable, or if you need it to stay synchronized across tabs, use React's
useSyncExternalStoreor an effect that syncs it to state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/dashboard/src/components/ThemeCustomizer.jsx` at line 55, Update ThemeCustomizer’s custom theme loading around getCustomThemes so localStorage is not read synchronously during every render. Initialize and maintain the themes through React state, using an appropriate effect or subscription only if cross-tab synchronization is required, while preserving the existing custom theme behavior.apps/backend/routes/themes.py (1)
144-151: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
model_dump()over manual dictionary construction.You can use Pydantic's built-in serialization to avoid manually mapping each field to a dictionary.
♻️ Proposed fix
- customs[body.name] = { - "name": body.name, - "boundingBoxColors": body.boundingBoxColors, - "label": body.label, - "confidence": body.confidence, - "trackingTrails": body.trackingTrails, - "accessibility": body.accessibility, - } + customs[body.name] = body.model_dump()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/routes/themes.py` around lines 144 - 151, Replace the manual dictionary construction assigned in the customs update with body.model_dump(), preserving the existing key and serialized field values while relying on Pydantic’s built-in serialization.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/backend/routes/themes.py`:
- Around line 104-118: Update _load_custom_themes and _save_custom_themes to
synchronize all custom-theme file access with a shared threading.Lock, including
the existence check, read, and write operations. Replace _load_custom_themes’
blanket exception handling with specific handling only for the intended
missing-file case, allowing JSON or I/O corruption errors to propagate instead
of returning {} and risking data loss.
In `@apps/dashboard/src/features/themes.jsx`:
- Around line 4-6: Update the default export in the themes feature module to be
a combined component that renders both ThemeSelector and ThemeCustomizer, while
preserving their existing named exports and the label value.
In `@apps/dashboard/src/pages/Dashboard.jsx`:
- Around line 31-34: Rename the activeComponent variable returned by the useMemo
in apps/dashboard/src/pages/Dashboard.jsx lines 31-34 to ActiveComponent, and
update its render usage at lines 92-92 to conditionally render <ActiveComponent
/>. No other changes are needed.
---
Nitpick comments:
In `@apps/backend/routes/themes.py`:
- Around line 144-151: Replace the manual dictionary construction assigned in
the customs update with body.model_dump(), preserving the existing key and
serialized field values while relying on Pydantic’s built-in serialization.
In `@apps/dashboard/src/components/ThemeCustomizer.jsx`:
- Line 55: Update ThemeCustomizer’s custom theme loading around getCustomThemes
so localStorage is not read synchronously during every render. Initialize and
maintain the themes through React state, using an appropriate effect or
subscription only if cross-tab synchronization is required, while preserving the
existing custom theme behavior.
🪄 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: 070180b0-01aa-4e0f-8ae3-971e3d3562d0
📒 Files selected for processing (13)
apps/backend/main.pyapps/backend/routes/__init__.pyapps/backend/routes/alerts.pyapps/backend/routes/feedback.pyapps/backend/routes/ingest.pyapps/backend/routes/snapshot.pyapps/backend/routes/themes.pyapps/dashboard/src/components/ThemeCustomizer.jsxapps/dashboard/src/components/ThemeSelector.jsxapps/dashboard/src/features/themes.jsxapps/dashboard/src/pages/Dashboard.jsxapps/dashboard/src/utils/themes.jstests/integration/test_themes.py
| def _load_custom_themes() -> dict: | ||
| if not os.path.exists(THEMES_FILE): | ||
| return {} | ||
| try: | ||
| with open(THEMES_FILE, "r", encoding="utf-8") as f: | ||
| return json.load(f) | ||
| except Exception: | ||
| return {} | ||
|
|
||
|
|
||
| def _save_custom_themes(themes: dict): | ||
| os.makedirs(os.path.dirname(THEMES_FILE), exist_ok=True) | ||
| with open(THEMES_FILE, "w", encoding="utf-8") as f: | ||
| json.dump(themes, f, indent=2) | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Race condition and data loss hazard during concurrent file operations.
The endpoints for reading and writing custom themes are standard synchronous def functions, which FastAPI runs in a threadpool. Concurrent requests to POST /themes or DELETE /themes will cause race conditions because the file reads and writes are not synchronized.
Furthermore, if a read occurs while a write is truncating the file, json.load(f) will raise an exception. Because _load_custom_themes catches all exceptions and silently returns {}, the subsequent save operation will overwrite the file with just the single new or remaining theme, permanently wiping out all other custom themes.
Please use a threading.Lock to synchronize file access and remove the blanket except Exception clause so that file corruption fails loudly rather than causing silent data loss.
🔒️ Proposed fix to synchronize file operations
import json
import os
+import threading
from fastapi import APIRouter, HTTPException
from pydantic import BaseModel, Field
logger = logging.getLogger(__name__)
router = APIRouter(prefix="/themes", tags=["themes"])
THEME_DIR = os.path.join(os.path.dirname(__file__), "..", "..", "config", "themes")
THEMES_FILE = os.path.join(THEME_DIR, "custom_themes.json")
+THEMES_LOCK = threading.Lock()
os.makedirs(THEME_DIR, exist_ok=True)
# ... (skip to _load_custom_themes) ...
def _load_custom_themes() -> dict:
- if not os.path.exists(THEMES_FILE):
- return {}
- try:
+ with THEMES_LOCK:
+ if not os.path.exists(THEMES_FILE):
+ return {}
with open(THEMES_FILE, "r", encoding="utf-8") as f:
return json.load(f)
- except Exception:
- return {}
def _save_custom_themes(themes: dict):
os.makedirs(os.path.dirname(THEMES_FILE), exist_ok=True)
- with open(THEMES_FILE, "w", encoding="utf-8") as f:
- json.dump(themes, f, indent=2)
+ with THEMES_LOCK:
+ with open(THEMES_FILE, "w", encoding="utf-8") as f:
+ json.dump(themes, f, indent=2)🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 107-107: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(THEMES_FILE, "r", encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 115-115: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(THEMES_FILE, "w", encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/routes/themes.py` around lines 104 - 118, Update
_load_custom_themes and _save_custom_themes to synchronize all custom-theme file
access with a shared threading.Lock, including the existence check, read, and
write operations. Replace _load_custom_themes’ blanket exception handling with
specific handling only for the intended missing-file case, allowing JSON or I/O
corruption errors to propagate instead of returning {} and risking data loss.
| export { ThemeSelector, ThemeCustomizer } | ||
| export default ThemeSelector | ||
| export const label = "Overlay Themes" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Render both ThemeSelector and ThemeCustomizer in the feature tab.
The Dashboard component renders the default export of this feature module. Currently, ThemeSelector is the default export, meaning ThemeCustomizer is orphaned and inaccessible in the UI.
To expose both the selector and the customizer, export a combined component as the default.
🐛 Proposed fix to render both components
-export { ThemeSelector, ThemeCustomizer }
-export default ThemeSelector
-export const label = "Overlay Themes"
+export { ThemeSelector, ThemeCustomizer };
+
+export default function ThemesFeature() {
+ return (
+ <div className="space-y-6">
+ <ThemeSelector />
+ <ThemeCustomizer />
+ </div>
+ );
+}
+
+export const label = "Overlay Themes";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export { ThemeSelector, ThemeCustomizer } | |
| export default ThemeSelector | |
| export const label = "Overlay Themes" | |
| export { ThemeSelector, ThemeCustomizer }; | |
| export default function ThemesFeature() { | |
| return ( | |
| <div className="space-y-6"> | |
| <ThemeSelector /> | |
| <ThemeCustomizer /> | |
| </div> | |
| ); | |
| } | |
| export const label = "Overlay Themes"; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/dashboard/src/features/themes.jsx` around lines 4 - 6, Update the
default export in the themes feature module to be a combined component that
renders both ThemeSelector and ThemeCustomizer, while preserving their existing
named exports and the label value.
| const activeComponent = useMemo(() => { | ||
| const tab = featureTabs.find((t) => t.id === view); | ||
| return tab?.component || null; | ||
| }, [view, featureTabs]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Capitalize activeComponent to fix JSX component rendering.
In React, JSX requires user-defined component variables to begin with an uppercase letter. Using <activeComponent /> causes React to output an invalid literal HTML tag (<activecomponent>) instead of rendering the feature module.
apps/dashboard/src/pages/Dashboard.jsx#L31-L34: rename the state variable toActiveComponent.apps/dashboard/src/pages/Dashboard.jsx#L92-L92: update the rendering syntax to{ActiveComponent && <ActiveComponent />}.
📍 Affects 1 file
apps/dashboard/src/pages/Dashboard.jsx#L31-L34(this comment)apps/dashboard/src/pages/Dashboard.jsx#L92-L92
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/dashboard/src/pages/Dashboard.jsx` around lines 31 - 34, Rename the
activeComponent variable returned by the useMemo in
apps/dashboard/src/pages/Dashboard.jsx lines 31-34 to ActiveComponent, and
update its render usage at lines 92-92 to conditionally render <ActiveComponent
/>. No other changes are needed.
1 participant
#198
Summary by CodeRabbit
New Features
Bug Fixes