feat(dashboard-wish): enhance sharing functionality and UI customization#245
Open
vishalkadam47 wants to merge 7 commits into
Open
feat(dashboard-wish): enhance sharing functionality and UI customization#245vishalkadam47 wants to merge 7 commits into
vishalkadam47 wants to merge 7 commits into
Conversation
vishalkadam47
commented
May 7, 2026
vishalkadam47
commented
Contributor
- Refactored Sharing component to compute renderable settings directly from server settings.
- Removed deprecated ShortcutsMenu component.
- Added video bitrate monitoring to SystemMonitoring component.
- Introduced PlayerGamepadButton for touch gamepad functionality in player modes.
- Updated TopMenu to conditionally render UI elements based on server settings.
- Improved Vite configuration for better build performance and environment handling.
- Added utility functions for computing renderable settings and route prefix.
- Refactored Sharing component to compute renderable settings directly from server settings. - Removed deprecated ShortcutsMenu component. - Added video bitrate monitoring to SystemMonitoring component. - Introduced PlayerGamepadButton for touch gamepad functionality in player modes. - Updated TopMenu to conditionally render UI elements based on server settings. - Improved Vite configuration for better build performance and environment handling. - Added utility functions for computing renderable settings and route prefix.
- Refactored Input, Label, Menubar, Progress, ScrollArea, Separator, Slider, Sonner, Switch, Tabs, Textarea, and Tooltip components to utilize radix-ui. - Enhanced styling for various components, including adjustments to class names and structure for better responsiveness and accessibility. - Removed unused Progress component. - Updated CSS imports and variables for improved theming and consistency. - Fixed import paths and configurations in main.jsx and vite.config.js for better module resolution.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces significant updates to the Selkies dashboard, including a new gamepad toggle button, improved streaming mode management (WebRTC vs. WebSockets), and a comprehensive refactor of UI components to use Radix UI. It also includes normalization logic for settings, improved server-side synchronization, and updated build configurations. My review highlights concerns regarding side effects in state initializers, missing dependencies in useEffect hooks, potential issues with response handling, and inconsistencies in fullscreen toggle logic and storage key generation.
| const parsed = parseInt(raw, 10); | ||
| if (!isNaN(parsed) && parsed > 1000) { | ||
| const normalized = Math.round(parsed / 1000); | ||
| localStorage.setItem(getPrefixedKey("video_bitrate"), normalized.toString()); |
Contributor
There was a problem hiding this comment.
The useState initializer should be a pure function. Performing a side effect like localStorage.setItem inside the initializer is discouraged in React as it can lead to inconsistent behavior, especially in Strict Mode where initializers may run multiple times. Consider moving the migration logic to a useEffect or performing the normalization without an immediate write-back during state initialization.
| setVideoBitRate(final); | ||
| localStorage.setItem(getPrefixedKey("video_bitrate"), final.toString()); | ||
| } | ||
| }, [serverSettings]); |
Contributor
There was a problem hiding this comment.
The useEffect hook uses streamMode to determine which encoder settings to synchronize (lines 291 and 300), but streamMode is missing from the dependency array. This means that if the streaming mode is changed, the encoder settings will not be re-synchronized with the server-provided constraints until a new serverSettings message is received.
Suggested change
| }, [serverSettings]); | |
| }, [serverSettings, streamMode]); |
Comment on lines
+583
to
+587
| if (!response.ok) { | ||
| throw new Error(`Request failed with status ${response.status}`); | ||
| } | ||
| await response.json(); | ||
| setStreamMode(mode); |
Contributor
There was a problem hiding this comment.
Awaiting response.json() without checking the content type or if the body is empty can cause the function to throw an error if the server returns a non-JSON response (e.g., 204 No Content). Since the result of the JSON parsing is not used, it is safer to remove this call.
Suggested change
| if (!response.ok) { | |
| throw new Error(`Request failed with status ${response.status}`); | |
| } | |
| await response.json(); | |
| setStreamMode(mode); | |
| if (!response.ok) { | |
| throw new Error(`Request failed with status ${response.status}`); | |
| } | |
| setStreamMode(mode); |
| const [latencyMs, setLatencyMs] = useState(0); | ||
| const [videoBitrateMbps, setVideoBitrateMbps] = useState(0); | ||
| const [isWebrtc, setIsWebrtc] = useState(() => { | ||
| const saved = localStorage.getItem(`${window.location.href.split('#')[0].replace(/[^a-zA-Z0-9.-_]/g, '_')}_stream_mode`); |
Contributor
There was a problem hiding this comment.
The logic for generating the storage key is duplicated here and manually implemented. This is error-prone and hard to maintain. It should ideally use a shared utility function to ensure consistency across the application, especially since the key format includes a regex-based sanitization of the URL.
Comment on lines
+873
to
+879
| onClick={() => { | ||
| if (document.fullscreenElement) { | ||
| document.exitFullscreen(); | ||
| } else { | ||
| document.documentElement.requestFullscreen(); | ||
| } | ||
| }} |
Contributor
There was a problem hiding this comment.
The fullscreen toggle logic here is inconsistent with the implementation in the Gaming Control Bar (lines 590-596). It calls requestFullscreen directly on the document element instead of using the requestFullscreen message type, which might bypass specific handling in the core library. Additionally, it lacks error handling for the returned Promise.
onClick={() => {
if (document.fullscreenElement) {
if (document.exitFullscreen) {
document.exitFullscreen().catch(err => console.error("Error exiting fullscreen:", err));
}
} else {
window.postMessage({ type: 'requestFullscreen' }, window.location.origin);
}
}}
There was a problem hiding this comment.
Pull request overview
This PR modernizes the selkies-dashboard-wish frontend by refactoring how UI feature visibility is derived from server settings, updating/removing several UI components, and adding player-mode touch gamepad support while improving build configuration.
Changes:
- Refactors settings-driven UI rendering via new utilities (
computeRenderableSettings,getRoutePrefix) and applies them across Sharing/TopMenu/Settings/Clipboard. - Updates the UI component layer (Radix/Shadcn styling + component API changes) and removes deprecated components (e.g., ShortcutsMenu; some UI primitives deleted).
- Adds WebRTC video bitrate monitoring and introduces a player-mode floating gamepad toggle button.
Reviewed changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| addons/selkies-dashboard-wish/vite.config.js | Adds env loading, minify plugin, build target, and injected define; updates React plugin options. |
| addons/selkies-dashboard-wish/tsconfig.app.json | Adjusts TS compiler options (deprecation handling) while extending base config. |
| addons/selkies-dashboard-wish/src/main.jsx | Defers selkies-core initialization, probes /status, and conditionally mounts dashboard vs player UI. |
| addons/selkies-dashboard-wish/src/lib/utils.ts | Adds route-prefix helper and shared logic for deciding which settings/UI controls should render. |
| addons/selkies-dashboard-wish/src/index.css | Adds shadcn + Inter font imports and updates theme tokens/typography. |
| addons/selkies-dashboard-wish/src/components/ui/tooltip.tsx | Updates tooltip primitive import/usage and styling. |
| addons/selkies-dashboard-wish/src/components/ui/textarea.tsx | Updates textarea styling classes. |
| addons/selkies-dashboard-wish/src/components/ui/tabs.tsx | Adds variants/orientation styling and exports variants helper. |
| addons/selkies-dashboard-wish/src/components/ui/switch.tsx | Updates switch primitive import/usage and adds size variant styling. |
| addons/selkies-dashboard-wish/src/components/ui/sonner.tsx | Adds toast icons and styling options for Sonner Toaster. |
| addons/selkies-dashboard-wish/src/components/ui/slider.tsx | Updates slider primitive import/usage and styling. |
| addons/selkies-dashboard-wish/src/components/ui/separator.tsx | Updates separator primitive import/usage and styling/data-slot. |
| addons/selkies-dashboard-wish/src/components/ui/scroll-area.tsx | Updates scroll-area primitive import/usage and scrollbar styling. |
| addons/selkies-dashboard-wish/src/components/ui/progress.tsx | Removes the Progress UI component. |
| addons/selkies-dashboard-wish/src/components/ui/menubar.tsx | Updates menubar primitive import/usage and restyles menu items/content. |
| addons/selkies-dashboard-wish/src/components/ui/label.tsx | Updates label primitive import/usage. |
| addons/selkies-dashboard-wish/src/components/ui/input.tsx | Updates input styling classes. |
| addons/selkies-dashboard-wish/src/components/ui/dropdown-menu.tsx | Updates dropdown primitives import/usage, adds defaults, and restyles items/content. |
| addons/selkies-dashboard-wish/src/components/ui/dialog.tsx | Updates dialog primitives, adds close button controls, and restyles overlay/content. |
| addons/selkies-dashboard-wish/src/components/ui/checkbox.tsx | Removes the Checkbox UI component. |
| addons/selkies-dashboard-wish/src/components/ui/card.tsx | Adds sizing support and updates card typography/layout styling. |
| addons/selkies-dashboard-wish/src/components/ui/button.tsx | Updates Slot usage and expands button variants/sizes with new styling/data attrs. |
| addons/selkies-dashboard-wish/src/components/ui/badge.tsx | Reworks badge component API (asChild) and styling variants. |
| addons/selkies-dashboard-wish/src/components/ui/alert.tsx | Removes the Alert UI component. |
| addons/selkies-dashboard-wish/src/components/DashboardOverlay.tsx | Adds viewer-role suppression for TopMenu and suppresses gamepad on secondary display. |
| addons/selkies-dashboard-wish/src/components/dashboard/top-menu.tsx | Applies renderable settings gating, removes shortcuts menu, and adds gaming control bar + panel logic updates. |
| addons/selkies-dashboard-wish/src/components/dashboard/system-monitoring.tsx | Adds WebRTC video bitrate display and adjusts stat polling behavior. |
| addons/selkies-dashboard-wish/src/components/dashboard/shortcuts-menu.tsx | Removes deprecated ShortcutsMenu implementation. |
| addons/selkies-dashboard-wish/src/components/dashboard/sharing.tsx | Refactors to compute renderable settings directly from server payload. |
| addons/selkies-dashboard-wish/src/components/dashboard/settings.tsx | Large refactor: key naming alignment, dual-mode switching, rate control/bitrate UI, and renderable settings gating. |
| addons/selkies-dashboard-wish/src/components/dashboard/PlayerGamepadButton.tsx | Adds a draggable floating button to toggle touch gamepad in player modes. |
| addons/selkies-dashboard-wish/src/components/dashboard/files.tsx | Cleans up unused icon import. |
| addons/selkies-dashboard-wish/src/components/dashboard/clipboard.tsx | Adds binary clipboard toggle, persistence, and renderable-settings gating. |
| addons/selkies-dashboard-wish/package.json | Updates dependencies (Radix/Shadcn/Tailwind/Vite/etc.) and adds new packages. |
| addons/selkies-dashboard-wish/index.html | Adds mobile/PWA-ish meta tags and changes script paths. |
| addons/selkies-dashboard-wish/eslint.config.js | Adds Node globals for config files. |
| addons/selkies-dashboard-wish/components.json | Updates shadcn style/theme settings and adds additional config fields. |
| addons/selkies-dashboard-wish/.gitignore | Ignores bun.lock. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+33
to
+39
| (async () => { | ||
| await detectInitialMode(); | ||
| // Prevent selkies-core from auto-initializing | ||
| window.__SELKIES_DEFER_INITIALIZATION = true; | ||
| await import('./selkies-core.js'); | ||
| // Initialize with the mode detected from server | ||
| window.selkiesCoreInitialize(); |
Comment on lines
+4
to
+17
| <meta charset="utf-8"> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0, maximum-scale=1.0, user-scalable=no"> | ||
| <meta name="apple-mobile-web-app-capable" content="yes"> | ||
| <meta name="apple-mobile-web-app-status-bar-style" content="black-translucent"> | ||
| <link rel="apple-touch-icon" href="icon.png"> | ||
| <link rel="manifest" href="manifest.json" crossorigin="use-credentials"> | ||
| <link rel="icon" type="image/png" href="icon.png" /> | ||
| </head> | ||
| <body> | ||
| <div id="app"></div> | ||
| <div id="root"></div> | ||
| <div id="touch-gamepad-host"></div> | ||
| <script vite-ignore src="/src/universalTouchGamepad.js"></script> | ||
| <script vite-ignore type="module" src="/src/selkies-core.js"></script> | ||
| <script type="module" src="/src/main.jsx"></script> | ||
| <script vite-ignore src="src/universalTouchGamepad.js"></script> | ||
| <script type="module" src="src/main.jsx"></script> |
Comment on lines
1
to
36
| import { useTheme } from "next-themes" | ||
| import { Toaster as Sonner, ToasterProps } from "sonner" | ||
| import { Toaster as Sonner, type ToasterProps } from "sonner" | ||
| import { CircleCheckIcon, InfoIcon, TriangleAlertIcon, OctagonXIcon, Loader2Icon } from "lucide-react" | ||
|
|
||
| const Toaster = ({ ...props }: ToasterProps) => { | ||
| const { theme = "system" } = useTheme() | ||
|
|
||
| return ( | ||
| <Sonner | ||
| theme={theme as ToasterProps["theme"]} | ||
| className="toaster group" | ||
| icons={{ | ||
| success: ( | ||
| <CircleCheckIcon className="size-4" /> | ||
| ), | ||
| info: ( | ||
| <InfoIcon className="size-4" /> | ||
| ), | ||
| warning: ( | ||
| <TriangleAlertIcon className="size-4" /> | ||
| ), | ||
| error: ( | ||
| <OctagonXIcon className="size-4" /> | ||
| ), | ||
| loading: ( | ||
| <Loader2Icon className="size-4 animate-spin" /> | ||
| ), | ||
| }} | ||
| style={ | ||
| { | ||
| "--normal-bg": "var(--popover)", | ||
| "--normal-text": "var(--popover-foreground)", | ||
| "--normal-border": "var(--border)", | ||
| "--border-radius": "var(--radius)", | ||
| } as React.CSSProperties | ||
| } |
Comment on lines
7
to
21
| function Tabs({ | ||
| className, | ||
| orientation = "horizontal", | ||
| ...props | ||
| }: React.ComponentProps<typeof TabsPrimitive.Root>) { | ||
| return ( | ||
| <TabsPrimitive.Root | ||
| data-slot="tabs" | ||
| className={cn("flex flex-col gap-2", className)} | ||
| data-orientation={orientation} | ||
| className={cn( | ||
| "group/tabs flex gap-2 data-horizontal:flex-col", | ||
| className | ||
| )} | ||
| {...props} | ||
| /> |
Comment on lines
+205
to
206
| const saved = localStorage.getItem(getPrefixedKey("use_browser_cursors")); | ||
| return saved !== null ? saved === "true" : false; |
Comment on lines
+14
to
+18
| base: '', | ||
| plugins: [ | ||
| react({ | ||
| exclude: 'src/selkies-core.js' | ||
| }), |
Member
|
I skimmed through the changes. I can't comment on the UI/UX aspect here, but most of the changes include the latest functionality modification. I'll try to do some functional testing later in the day to see if it passes the build and execution flow. |
…gs management - Extract storage key prefixing logic to shared utils module for reuse across components - Move computeRenderableSettings import to centralized utils file - Add TypeScript 6.0.3 to dev dependencies for improved type checking - Update clipboard component to use shared getPrefixedKey utility - Enhance files component to listen for serverSettings messages and conditionally render upload/download buttons based on settings - Refactor settings component to use centralized utility imports - Replace direct fullscreen API call with postMessage for better iframe compatibility - Consolidate per-display settings and storage key logic into reusable utilities - Improve type safety and reduce code duplication across dashboard components
Member
|
@PMohanJ @vishalkadam47 What's the status of this one? |
Member
|
The build and execution flow works as expected in selkies containers. The dashboard has minor issues with webrtc mode where the resizing and mouse cursor positions don't align upon session start; except that all good. |
Member
|
@vishalkadam47 Can we fix the issue? Then we can merge. |
- Remove localStorage.setItem calls for use_browser_cursors, as core persists this via setBoolParam - Remove localStorage.setItem calls for scaleLocallyManual, as core handles persistence - Remove localStorage.setItem calls for use_css_scaling, as core persists via setBoolParam - Remove localStorage.setItem calls for antiAliasingEnabled, as core handles persistence - Remove localStorage.setItem calls for useBrowserCursors, as core persists via setBoolParam - Add clarifying comments explaining that core handles persistence, not dashboard - Eliminates duplicate persistence logic between dashboard and core layer
Member
|
Integrated into #254. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.