Conversation
Add desktop client detection and a fallback UI when running inside Jellyfin Desktop/Media Player: isJellyfinDesktopClient() and getPluginServerAddress() were added to services/jellyfin/core.ts, and main.tsx now renders a simple "Open in Browser" prompt (renderDesktopFallback) instead of the full app when the embedded client is detected.
Reviewer's GuideImplements Jellyfin desktop client detection and a dedicated DesktopFallback UI, refactors Jellyfin plugin client accessors for compatibility, centralizes the app base route, and includes minor robustness and tooling tweaks. Sequence diagram for Jellyfin desktop detection and fallback renderingsequenceDiagram
actor JellyfinDesktopUser
participant BrowserRuntime
participant main_tsx
participant JellyfinCore
participant DesktopFallback
participant RouterProvider
JellyfinDesktopUser->>BrowserRuntime: Open SegmentEditor plugin
BrowserRuntime->>main_tsx: Load entrypoint
main_tsx->>JellyfinCore: isPluginMode()
JellyfinCore-->>main_tsx: pluginMode = true
main_tsx->>JellyfinCore: isJellyfinDesktopClient()
JellyfinCore-->>main_tsx: true (UA matches DESKTOP_UA_PATTERNS)
alt pluginMode and Jellyfin desktop client
main_tsx->>DesktopFallback: Render DesktopFallback
DesktopFallback->>JellyfinCore: getStandaloneEditorUrl()
JellyfinCore->>JellyfinCore: getPluginServerAddress()
JellyfinCore-->>DesktopFallback: standalone editor URL
DesktopFallback-->>JellyfinDesktopUser: Show "Open in Browser" UI
else other environments
main_tsx->>RouterProvider: Render router-based app
RouterProvider-->>JellyfinDesktopUser: Full SegmentEditor UI
end
Class diagram for Jellyfin core utilities, main bootstrap, and DesktopFallbackclassDiagram
class JellyfinCore {
<<module>>
+string APP_BASE_ROUTE
+string PLUGIN_ROUTER_BASE_PATH
+string PLUGIN_ROUTER_ENTRY
+RegExp DESKTOP_UA_PATTERNS
+Credentials getPluginCredentials()
+boolean isPluginMode()
+boolean isJellyfinDesktopClient()
+string getPluginServerAddress()
+string getStandaloneEditorUrl()
-string readServerAddress(JellyfinApiClient client)
-string readAccessToken(JellyfinApiClient client)
-JellyfinApiClient getPluginApiClient()
}
class DesktopFallback {
<<react_component>>
+DesktopFallback()
}
class MainAppBootstrap {
<<module>>
-boolean pluginMode
-boolean pluginBuild
-any history
-any router
-string basePath
+initialize(rootElement)
-renderDesktopFallback(root)
-renderFullApp(root)
}
class JellyfinApiClient {
+string serverAddress()
+string accessToken()
-string _serverAddress
-any _serverInfo
}
class Credentials {
+string serverAddress
+string accessToken
}
MainAppBootstrap ..> JellyfinCore : uses
MainAppBootstrap ..> DesktopFallback : renders
DesktopFallback ..> JellyfinCore : uses
JellyfinCore ..> JellyfinApiClient : reads fields
JellyfinCore ..> Credentials : returns
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 security issues, 2 other issues, and left some high level feedback:
Security issues:
- User controlled data in methods like
innerHTML,outerHTMLordocument.writeis an anti-pattern that can lead to XSS vulnerabilities (link) - User controlled data in a
container.innerHTMLis an anti-pattern that can lead to XSS vulnerabilities (link)
General comments:
- The desktop fallback UI is currently built via a large
innerHTMLstring with extensive inline styles; consider extracting this into a small React component (or DOM-building helper) so you avoid raw HTML injection and keep styling/layout easier to maintain and reuse. - In
getPluginServerAddressyou fall back toclient._serverAddress, which relies on an internal/private field; if possible, prefer a public API or a typed extension point so this doesn’t break if the client implementation changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The desktop fallback UI is currently built via a large `innerHTML` string with extensive inline styles; consider extracting this into a small React component (or DOM-building helper) so you avoid raw HTML injection and keep styling/layout easier to maintain and reuse.
- In `getPluginServerAddress` you fall back to `client._serverAddress`, which relies on an internal/private field; if possible, prefer a public API or a typed extension point so this doesn’t break if the client implementation changes.
## Individual Comments
### Comment 1
<location path="src/services/jellyfin/core.ts" line_range="107-112" />
<code_context>
+}
+
+/** Returns the server address from the plugin API client, or empty string if unavailable. */
+export function getPluginServerAddress(): string {
+ const client = getPluginApiClient()
+ if (!client) return ''
+ return client.serverAddress?.() ?? client._serverAddress ?? ''
+}
+
</code_context>
<issue_to_address>
**suggestion:** Relying on `client._serverAddress` couples this helper to a private/implementation detail.
This can fail silently if the client implementation changes. Prefer relying solely on `serverAddress()` and letting callers handle the empty string, or add a public accessor on the client instead of using `_serverAddress` directly.
```suggestion
/** Returns the server address from the plugin API client, or empty string if unavailable. */
export function getPluginServerAddress(): string {
const client = getPluginApiClient()
return client?.serverAddress?.() ?? ''
}
```
</issue_to_address>
### Comment 2
<location path="src/main.tsx" line_range="28" />
<code_context>
+// When running as a plugin inside Jellyfin Desktop or Jellyfin Media Player,
+// the embedded browser lacks features needed by this app. Show a link to the
+// standalone browser version instead.
+function renderDesktopFallback(container: HTMLElement): void {
+ const serverAddress = getPluginServerAddress()
+ const editorUrl = serverAddress
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the desktop fallback into a React component with a shared URL helper and a single React render path instead of an imperative DOM helper and special-case branch in main.tsx.
The imperative `renderDesktopFallback` and the special-case branch at the entry point do increase complexity. You can keep the behavior while simplifying by:
1. Computing `editorUrl` as a small pure helper.
2. Moving the fallback UI into a React component (in its own file).
3. Keeping the entry point render path uniform (always use `ReactDOM.createRoot`).
### 1. Extract a helper for `editorUrl`
In a shared module (or near the fallback component), keep the URL logic but decouple it from DOM work:
```ts
// services/jellyfin/desktopFallback.ts (or similar)
import { getPluginServerAddress } from './core'
export function getEditorUrl(): string {
const serverAddress = getPluginServerAddress()
return serverAddress
? `${serverAddress.replace(/\/+$/, '')}/SegmentEditor`
: '/SegmentEditor'
}
```
### 2. Create a React `DesktopFallback` component
Move the inline HTML into JSX with classes (or minimal inline styles) in its own file:
```tsx
// components/DesktopFallback.tsx
import { getEditorUrl } from '../services/jellyfin/desktopFallback'
export function DesktopFallback() {
const editorUrl = getEditorUrl()
return (
<div className="desktop-fallback">
<div className="desktop-fallback__card">
<h1 className="desktop-fallback__title">Segment Editor</h1>
<p className="desktop-fallback__text">
This app is not supported in the Jellyfin desktop client.
Please open it in your browser instead.
</p>
<a
href={editorUrl}
target="_blank"
rel="noopener noreferrer"
className="desktop-fallback__button"
>
Open in Browser
</a>
<div className="desktop-fallback__url">{editorUrl}</div>
</div>
</div>
)
}
```
Add minimal CSS instead of deeply nested inline styles:
```css
/* styles.css (or a dedicated css module) */
.desktop-fallback {
display: flex;
align-items: center;
justify-content: center;
min-height: 100vh;
padding: 2rem;
font-family: -apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, Oxygen,
Ubuntu, Cantarell, sans-serif;
background: #09090b;
color: #fafafa;
}
.desktop-fallback__card {
max-width: 28rem;
width: 100%;
text-align: center;
border: 1px solid #27272a;
border-radius: 0.75rem;
padding: 2.5rem 2rem;
background: #18181b;
}
.desktop-fallback__title {
margin: 0 0 0.75rem;
font-size: 1.25rem;
font-weight: 600;
}
/* add the remaining styles from your inline rules as needed */
```
### 3. Keep a single React render path in the entry file
Replace the imperative `renderDesktopFallback` + conditional DOM branch with a component-level decision:
```tsx
// main.tsx (entry)
import { DesktopFallback } from './components/DesktopFallback'
import { isJellyfinDesktopClient, isPluginMode } from './services/jellyfin/core'
// ...other imports...
// existing router and providers setup...
const rootElement = document.getElementById('app')
if (rootElement) {
const root = ReactDOM.createRoot(rootElement)
const isDesktopPlugin = pluginMode && isJellyfinDesktopClient()
root.render(
<StrictMode>
{isDesktopPlugin ? (
<DesktopFallback />
) : (
<TanStackQueryProvider.Provider {...TanStackQueryProviderContext}>
<RouterProvider router={router} />
</TanStackQueryProvider.Provider>
)}
</StrictMode>,
)
}
```
This keeps:
- All existing behavior (same URL logic, same UI content).
- A single, uniform bootstrapping flow (always `ReactDOM.createRoot`).
- UI expressed as React components with JSX and CSS instead of a large template string and `innerHTML`.
</issue_to_address>
### Comment 3
<location path="src/main.tsx" line_range="34-94" />
<code_context>
container.innerHTML = `
<div style="
display: flex;
align-items: center;
justify-content: center;
min-height: 100vh;
padding: 2rem;
font-family: -apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, Oxygen, Ubuntu, Cantarell, sans-serif;
background: #09090b;
color: #fafafa;
">
<div style="
max-width: 28rem;
width: 100%;
text-align: center;
border: 1px solid #27272a;
border-radius: 0.75rem;
padding: 2.5rem 2rem;
background: #18181b;
">
<h1 style="margin: 0 0 0.75rem; font-size: 1.25rem; font-weight: 600;">
Segment Editor
</h1>
<p style="margin: 0 0 1.5rem; font-size: 0.875rem; color: #a1a1aa; line-height: 1.5;">
This app is not supported in the Jellyfin desktop client.
Please open it in your browser instead.
</p>
<a
href="${editorUrl}"
target="_blank"
rel="noopener noreferrer"
style="
display: inline-block;
padding: 0.5rem 1.25rem;
font-size: 0.875rem;
font-weight: 500;
color: #fafafa;
background: #3b82f6;
border: none;
border-radius: 0.375rem;
text-decoration: none;
cursor: pointer;
margin-bottom: 1rem;
"
>Open in Browser</a>
<div style="
margin-top: 0.25rem;
padding: 0.625rem 1rem;
background: #09090b;
border: 1px solid #27272a;
border-radius: 0.375rem;
font-family: ui-monospace, SFMono-Regular, 'SF Mono', Menlo, Consolas, monospace;
font-size: 0.8125rem;
color: #a1a1aa;
word-break: break-all;
user-select: all;
cursor: text;
">${editorUrl}</div>
</div>
</div>
`
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-document-method):** User controlled data in methods like `innerHTML`, `outerHTML` or `document.write` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>
### Comment 4
<location path="src/main.tsx" line_range="34-94" />
<code_context>
container.innerHTML = `
<div style="
display: flex;
align-items: center;
justify-content: center;
min-height: 100vh;
padding: 2rem;
font-family: -apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, Oxygen, Ubuntu, Cantarell, sans-serif;
background: #09090b;
color: #fafafa;
">
<div style="
max-width: 28rem;
width: 100%;
text-align: center;
border: 1px solid #27272a;
border-radius: 0.75rem;
padding: 2.5rem 2rem;
background: #18181b;
">
<h1 style="margin: 0 0 0.75rem; font-size: 1.25rem; font-weight: 600;">
Segment Editor
</h1>
<p style="margin: 0 0 1.5rem; font-size: 0.875rem; color: #a1a1aa; line-height: 1.5;">
This app is not supported in the Jellyfin desktop client.
Please open it in your browser instead.
</p>
<a
href="${editorUrl}"
target="_blank"
rel="noopener noreferrer"
style="
display: inline-block;
padding: 0.5rem 1.25rem;
font-size: 0.875rem;
font-weight: 500;
color: #fafafa;
background: #3b82f6;
border: none;
border-radius: 0.375rem;
text-decoration: none;
cursor: pointer;
margin-bottom: 1rem;
"
>Open in Browser</a>
<div style="
margin-top: 0.25rem;
padding: 0.625rem 1rem;
background: #09090b;
border: 1px solid #27272a;
border-radius: 0.375rem;
font-family: ui-monospace, SFMono-Regular, 'SF Mono', Menlo, Consolas, monospace;
font-size: 0.8125rem;
color: #a1a1aa;
word-break: break-all;
user-select: all;
cursor: text;
">${editorUrl}</div>
</div>
</div>
`
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-innerhtml):** User controlled data in a `container.innerHTML` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+107
to
+112
| /** Returns the server address from the plugin API client, or empty string if unavailable. */ | ||
| export function getPluginServerAddress(): string { | ||
| const client = getPluginApiClient() | ||
| if (!client) return '' | ||
| return client.serverAddress?.() ?? client._serverAddress ?? '' | ||
| } |
There was a problem hiding this comment.
suggestion: Relying on client._serverAddress couples this helper to a private/implementation detail.
This can fail silently if the client implementation changes. Prefer relying solely on serverAddress() and letting callers handle the empty string, or add a public accessor on the client instead of using _serverAddress directly.
Suggested change
| /** Returns the server address from the plugin API client, or empty string if unavailable. */ | |
| export function getPluginServerAddress(): string { | |
| const client = getPluginApiClient() | |
| if (!client) return '' | |
| return client.serverAddress?.() ?? client._serverAddress ?? '' | |
| } | |
| /** Returns the server address from the plugin API client, or empty string if unavailable. */ | |
| export function getPluginServerAddress(): string { | |
| const client = getPluginApiClient() | |
| return client?.serverAddress?.() ?? '' | |
| } |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds detection for Jellyfin Desktop/Media Player embedded clients and renders a simple "Open in Browser" fallback UI instead of the full app, since the embedded browser lacks features the app requires. It also includes minor formatting and config changes.
Changes:
- Added
isJellyfinDesktopClient()(user-agent check) andgetPluginServerAddress()toservices/jellyfin/core.ts - Added
renderDesktopFallback()inmain.tsxthat renders a static HTML prompt with a link to open the editor in a real browser - Minor formatting/config cleanups: import reorder in
vitest.config.ts, addedvitest.config.tstotsconfig.json, reformattedvitest.setup.ts, removed non-null assertion insegment-form.ts
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/services/jellyfin/core.ts |
New isJellyfinDesktopClient() and getPluginServerAddress() utilities |
src/main.tsx |
Desktop client detection at render time; fallback HTML UI |
vitest.config.ts |
Import reorder (formatting) |
tsconfig.json |
Added vitest.config.ts to the include list |
src/vitest.setup.ts |
Reformatted chained array method calls |
src/lib/forms/segment-form.ts |
Removed non-null assertion operator |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
src/main.tsx
Outdated
Comment on lines
+29
to
+94
| const serverAddress = getPluginServerAddress() | ||
| const editorUrl = serverAddress | ||
| ? `${serverAddress.replace(/\/+$/, '')}/SegmentEditor` | ||
| : '/SegmentEditor' | ||
|
|
||
| container.innerHTML = ` | ||
| <div style=" | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| min-height: 100vh; | ||
| padding: 2rem; | ||
| font-family: -apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, Oxygen, Ubuntu, Cantarell, sans-serif; | ||
| background: #09090b; | ||
| color: #fafafa; | ||
| "> | ||
| <div style=" | ||
| max-width: 28rem; | ||
| width: 100%; | ||
| text-align: center; | ||
| border: 1px solid #27272a; | ||
| border-radius: 0.75rem; | ||
| padding: 2.5rem 2rem; | ||
| background: #18181b; | ||
| "> | ||
| <h1 style="margin: 0 0 0.75rem; font-size: 1.25rem; font-weight: 600;"> | ||
| Segment Editor | ||
| </h1> | ||
| <p style="margin: 0 0 1.5rem; font-size: 0.875rem; color: #a1a1aa; line-height: 1.5;"> | ||
| This app is not supported in the Jellyfin desktop client. | ||
| Please open it in your browser instead. | ||
| </p> | ||
| <a | ||
| href="${editorUrl}" | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| style=" | ||
| display: inline-block; | ||
| padding: 0.5rem 1.25rem; | ||
| font-size: 0.875rem; | ||
| font-weight: 500; | ||
| color: #fafafa; | ||
| background: #3b82f6; | ||
| border: none; | ||
| border-radius: 0.375rem; | ||
| text-decoration: none; | ||
| cursor: pointer; | ||
| margin-bottom: 1rem; | ||
| " | ||
| >Open in Browser</a> | ||
| <div style=" | ||
| margin-top: 0.25rem; | ||
| padding: 0.625rem 1rem; | ||
| background: #09090b; | ||
| border: 1px solid #27272a; | ||
| border-radius: 0.375rem; | ||
| font-family: ui-monospace, SFMono-Regular, 'SF Mono', Menlo, Consolas, monospace; | ||
| font-size: 0.8125rem; | ||
| color: #a1a1aa; | ||
| word-break: break-all; | ||
| user-select: all; | ||
| cursor: text; | ||
| ">${editorUrl}</div> | ||
| </div> | ||
| </div> | ||
| ` |
Comment on lines
+108
to
+112
| export function getPluginServerAddress(): string { | ||
| const client = getPluginApiClient() | ||
| if (!client) return '' | ||
| return client.serverAddress?.() ?? client._serverAddress ?? '' | ||
| } |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
src/main.tsx
Outdated
| : '/SegmentEditor' | ||
|
|
||
| container.innerHTML = ` | ||
| <div style=" |
Contributor
There was a problem hiding this comment.
WARNING: Potential XSS risk - serverAddress from getPluginServerAddress() is used directly in HTML without sanitization.
The editorUrl is interpolated into innerHTML at lines 68 and 97 without validation. While the risk is LOW (value comes from Jellyfin plugin API), consider adding URL validation as defense-in-depth:
Suggested change
| <div style=" | |
| // Validate URL before using in HTML | |
| try { | |
| new URL(editorUrl, 'http://localhost') | |
| } catch { | |
| editorUrl = '/SegmentEditor' | |
| } |
src/lib/forms/segment-form.ts
Outdated
| const parsed = Number(part) | ||
| if (!Number.isFinite(parsed)) return Number.NaN | ||
| return sum + parsed * TIME_MULTIPLIERS[index]! | ||
| return sum + parsed * TIME_MULTIPLIERS[index] |
Contributor
There was a problem hiding this comment.
SUGGESTION: Safe to remove non-null assertion.
The ! was safely removable because TIME_MULTIPLIERS is a const tuple (as const) and the index is bounds-checked at line 67 (parts.length > TIME_MULTIPLIERS.length). TypeScript correctly infers this is safe.
Contributor
Code Review SummaryStatus: No New Issues | Recommendation: Merge Overview
NotesThis PR addresses previously identified issues:
All previously identified issues in the diff have been addressed. The remaining inline comments about Files Reviewed (7 files)
|
Replace the large innerHTML template in renderDesktopFallback with programmatically created DOM nodes. The change creates wrapper, card, heading, description, link and urlDisplay elements, applies styles via Object.assign, sets textContent/href/rel/target and appends the nodes to the container. This improves safety (avoids HTML injection), CSP compatibility, readability, and maintainability while preserving the original UI and behavior.
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
renderDesktopFallback, theSegmentEditorpath is hardcoded in the URL construction; consider reusingPLUGIN_ROUTER_ENTRYor another shared constant so the fallback stays in sync if the base route changes. getPluginServerAddresscurrently relies on the_serverAddressprivate field as a fallback; if possible, prefer a stable public API from the plugin client or at least isolate this access behind a clearly labeled compatibility shim to make future breakage easier to track.- The desktop client detection is solely based on a user agent regex (
DESKTOP_UA_PATTERNS); it may be worth centralizing this pattern and documenting expected values or making it configurable so it can be updated without touching core logic if Jellyfin changes its UA strings.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `renderDesktopFallback`, the `SegmentEditor` path is hardcoded in the URL construction; consider reusing `PLUGIN_ROUTER_ENTRY` or another shared constant so the fallback stays in sync if the base route changes.
- `getPluginServerAddress` currently relies on the `_serverAddress` private field as a fallback; if possible, prefer a stable public API from the plugin client or at least isolate this access behind a clearly labeled compatibility shim to make future breakage easier to track.
- The desktop client detection is solely based on a user agent regex (`DESKTOP_UA_PATTERNS`); it may be worth centralizing this pattern and documenting expected values or making it configurable so it can be updated without touching core logic if Jellyfin changes its UA strings.
## Individual Comments
### Comment 1
<location path="src/main.tsx" line_range="24" />
<code_context>
import './styles.css'
-// Create a new router instance
+// Desktop client fallback
+// When running as a plugin inside Jellyfin Desktop or Jellyfin Media Player,
+// the embedded browser lacks features needed by this app. Show a link to the
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the desktop fallback UI and URL logic into a dedicated React component with CSS-based styling instead of imperative DOM code in main.tsx to keep bootstrapping simple and the view easier to maintain.
You can keep the functionality but reduce complexity by moving the UI construction into a small React component (and optional CSS) instead of imperative DOM in `main.tsx`. That keeps `main.tsx` focused on bootstrapping and makes the fallback easier to test and tweak.
**1. Extract URL logic and fallback UI into a component**
Create a `DesktopFallback.tsx`:
```tsx
// DesktopFallback.tsx
import React from 'react'
import { getPluginServerAddress } from './services/jellyfin/core'
function getEditorUrl(): string {
const serverAddress = getPluginServerAddress()
return serverAddress
? `${serverAddress.replace(/\/+$/, '')}/SegmentEditor`
: '/SegmentEditor'
}
export function DesktopFallback() {
const editorUrl = getEditorUrl()
return (
<div className="desktop-fallback-wrapper">
<div className="desktop-fallback-card">
<h1 className="desktop-fallback-heading">Segment Editor</h1>
<p className="desktop-fallback-description">
This app is not supported in the Jellyfin desktop client. Please open it in your browser instead.
</p>
<a
href={editorUrl}
target="_blank"
rel="noopener noreferrer"
className="desktop-fallback-link"
>
Open in Browser
</a>
<div className="desktop-fallback-url">
{editorUrl}
</div>
</div>
</div>
)
}
```
**2. Use normal React rendering in `main.tsx`**
Replace the imperative `renderDesktopFallback` + DOM calls with a standard React render path:
```ts
// main.tsx
import { DesktopFallback } from './DesktopFallback'
// ...
const rootElement = document.getElementById('app')
if (rootElement) {
const root = ReactDOM.createRoot(rootElement)
if (pluginMode && isJellyfinDesktopClient()) {
root.render(
<StrictMode>
<DesktopFallback />
</StrictMode>,
)
} else {
root.render(
<StrictMode>
<TanStackQueryProvider.Provider {...TanStackQueryProviderContext}>
<RouterProvider router={router} />
</TanStackQueryProvider.Provider>
</StrictMode>,
)
}
}
```
**3. Move inline styles to CSS (optional but simplifies further)**
Add to `styles.css` (or a dedicated CSS module):
```css
.desktop-fallback-wrapper {
display: flex;
align-items: center;
justify-content: center;
min-height: 100vh;
padding: 2rem;
font-family: -apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, Oxygen,
Ubuntu, Cantarell, sans-serif;
background: #09090b;
color: #fafafa;
}
.desktop-fallback-card {
max-width: 28rem;
width: 100%;
text-align: center;
border: 1px solid #27272a;
border-radius: 0.75rem;
padding: 2.5rem 2rem;
background: #18181b;
}
.desktop-fallback-heading {
margin: 0 0 0.75rem;
font-size: 1.25rem;
font-weight: 600;
}
.desktop-fallback-description {
margin: 0 0 1.5rem;
font-size: 0.875rem;
color: #a1a1aa;
line-height: 1.5;
}
.desktop-fallback-link {
display: inline-block;
padding: 0.5rem 1.25rem;
font-size: 0.875rem;
font-weight: 500;
color: #fafafa;
background: #3b82f6;
border: none;
border-radius: 0.375rem;
text-decoration: none;
cursor: pointer;
margin-bottom: 1rem;
}
.desktop-fallback-url {
margin-top: 0.25rem;
padding: 0.625rem 1rem;
background: #09090b;
border: 1px solid #27272a;
border-radius: 0.375rem;
font-family: ui-monospace, SFMono-Regular, 'SF Mono', Menlo, Consolas,
monospace;
font-size: 0.8125rem;
color: #a1a1aa;
word-break: break-all;
user-select: all;
cursor: text;
}
```
This keeps all behavior intact (same `editorUrl`, same layout) while:
- Removing imperative DOM manipulation from `main.tsx`.
- Making the feature a self-contained React component.
- Making styles easier to maintain and reuse.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
parseUncheckedTimeText, removing the non-null assertion onTIME_MULTIPLIERS[index]meansundefinedis now allowed at runtime; consider either constrainingindexto the multiplier array length or explicitly handling theundefinedcase to avoid implicitNaNfromparsed * undefined. - The construction of the standalone editor URL is now duplicated conceptually between
APP_BASE_ROUTEusage inmain.tsxandgetEditorUrlinDesktopFallback; consider extracting a single helper (e.g.getStandaloneEditorUrl) in the Jellyfin core module to avoid configuration drift and make future changes to the base path safer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `parseUncheckedTimeText`, removing the non-null assertion on `TIME_MULTIPLIERS[index]` means `undefined` is now allowed at runtime; consider either constraining `index` to the multiplier array length or explicitly handling the `undefined` case to avoid implicit `NaN` from `parsed * undefined`.
- The construction of the standalone editor URL is now duplicated conceptually between `APP_BASE_ROUTE` usage in `main.tsx` and `getEditorUrl` in `DesktopFallback`; consider extracting a single helper (e.g. `getStandaloneEditorUrl`) in the Jellyfin core module to avoid configuration drift and make future changes to the base path safer.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
parseUncheckedTimeText, usingTIME_MULTIPLIERS[index] ?? 0silently ignores unexpected extra segments; consider explicitly failing (e.g., returningNaN) whenindexexceeds the known multipliers to avoid masking malformed input. - The
DESKTOP_UA_PATTERNSuser-agent check is currently hard-coded to two values; if possible, centralizing or documenting how to update this for new Jellyfin desktop variants (or allowing injection/override in tests/config) would make the detection more resilient to future client changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `parseUncheckedTimeText`, using `TIME_MULTIPLIERS[index] ?? 0` silently ignores unexpected extra segments; consider explicitly failing (e.g., returning `NaN`) when `index` exceeds the known multipliers to avoid masking malformed input.
- The `DESKTOP_UA_PATTERNS` user-agent check is currently hard-coded to two values; if possible, centralizing or documenting how to update this for new Jellyfin desktop variants (or allowing injection/override in tests/config) would make the detection more resilient to future client changes.
## Individual Comments
### Comment 1
<location path="src/lib/forms/segment-form.ts" line_range="71-72" />
<code_context>
const parsed = Number(part)
if (!Number.isFinite(parsed)) return Number.NaN
- return sum + parsed * TIME_MULTIPLIERS[index]!
+ const multiplier = TIME_MULTIPLIERS[index] ?? 0
+ return sum + parsed * multiplier
}, 0)
}
</code_context>
<issue_to_address>
**issue:** Double-check behavior change for unexpected extra time segments.
Previously, out-of-range `index` produced `parsed * undefined → NaN`, so malformed inputs (extra `:` segments) caused the whole result to be `NaN`. With `?? 0`, those extra segments are now treated as 0 and silently ignored, yielding a finite value instead. If rejecting malformed input is desired, consider returning `NaN` when `TIME_MULTIPLIERS[index]` is `undefined`. Otherwise, please confirm all call sites are OK with this more permissive parsing.
</issue_to_address>
### Comment 2
<location path="src/main.tsx" line_range="93-96" />
<code_context>
- </StrictMode>,
- )
+
+ if (pluginMode && isJellyfinDesktopClient()) {
+ root.render(
+ <StrictMode>
+ <DesktopFallback />
+ </StrictMode>,
+ )
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider keeping the query provider mounted even when showing the desktop fallback.
In this branch `DesktopFallback` renders without `TanStackQueryProvider.Provider`, unlike the main app path. That works today, but any future use of query hooks or provider-dependent state in `DesktopFallback` would break only in this environment. To avoid that subtle divergence, consider always mounting the provider and conditionally rendering either `<DesktopFallback />` or `<RouterProvider router={router} />` inside it.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Add desktop client detection and a fallback UI when running inside Jellyfin Desktop/Media Player: isJellyfinDesktopClient() and getPluginServerAddress() were added to services/jellyfin/core.ts, and main.tsx now renders a simple "Open in Browser" prompt (renderDesktopFallback) instead of the full app when the embedded client is detected.
Summary by Sourcery
Detect Jellyfin desktop clients and render a simplified fallback UI instead of the full app when embedded, while centralizing app base route configuration and URL generation.
New Features:
Bug Fixes:
Enhancements:
Build:
Tests: