feat: Only displaying the quick connect option - #3315
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds a persisted ChangesQuick Connect-only authentication
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SettingsUsers
participant Settings
participant SettingsContext
participant JellyfinLogin
participant AuthRoute
SettingsUsers->>Settings: Save quickConnectOnly
Settings->>SettingsContext: Expose quickConnectOnly
SettingsContext->>JellyfinLogin: Provide quickConnectOnly
JellyfinLogin->>JellyfinLogin: Hide username/password form when enabled
JellyfinLogin->>AuthRoute: Submit Jellyfin password login
AuthRoute-->>JellyfinLogin: Return HTTP 403 restriction error
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
Billos
changed the title
There was a problem hiding this comment.
Pull request overview
Adds a new quickConnectOnly setting intended to allow administrators to hide Jellyfin’s classic username/password login UI and encourage (or enforce) Quick Connect-based sign-in, supporting Jellyfin configurations that rely on OIDC-only authentication.
Changes:
- Introduces
quickConnectOnlyas a main/public setting across server + client settings models. - Adds an admin UI toggle (Jellyfin-only) under Settings → Users to enable Quick Connect-only mode.
- Updates the Jellyfin login screen to hide the username/password form when
quickConnectOnlyis enabled.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/pages/_app.tsx | Adds quickConnectOnly to initial settings defaults. |
| src/context/SettingsContext.tsx | Adds quickConnectOnly to client-side default settings context. |
| src/components/Settings/SettingsUsers/index.tsx | Adds the new Settings UI toggle and persists it via /api/v1/settings/main. |
| src/components/Login/JellyfinLogin.tsx | Hides username/password form when quickConnectOnly is enabled; keeps Quick Connect button available. |
| server/lib/settings/index.ts | Adds the setting to MainSettings and includes it in the full public settings payload. |
| server/interfaces/api/settingsInterfaces.ts | Exposes the setting in PublicSettingsResponse typing. |
| src/i18n/locale/en.json | Adds translations for the new Settings toggle. |
| src/i18n/locale/fr.json | Adds translations for the new Settings toggle and the Quick Connect label. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/components/Settings/SettingsUsers/index.tsx:217
- This nested
LabeledCheckboxinherits a known accessibility/UX issue fromsrc/components/Common/LabeledCheckbox/index.tsxwhere the internal<label>uses a hard-codedhtmlFor="localLogin". As a result, the label text for this newquickConnectOnlycheckbox won’t correctly toggle/focus its checkbox (and may toggle the wrong checkbox), which is especially confusing with nested checkboxes. TheLabeledCheckboxcomponent should usehtmlFor={id}instead.
<LabeledCheckbox
id="quickConnectOnly"
label={intl.formatMessage(
messages.quickConnectOnly
)}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/components/Login/JellyfinLogin.tsx:136
quickConnectOnlycurrently only hides the username/password<Form>in the UI. Users can still authenticate with credentials by calling the existing/api/v1/auth/jellyfinendpoint directly, which means this setting does not actually “only allow” Quick Connect sign-in.
Consider enforcing this server-side (e.g., reject credential-based Jellyfin login when settings.main.quickConnectOnly is true), or adjust the setting label/tooltip to clearly indicate it only affects the UI.
{!quickConnectOnly && (
<Form data-form-type="login">
<div>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/components/Login/JellyfinLogin.tsx:49
quickConnectOnlyis applied regardless of the current media server type. SinceJellyfinLoginis also used for Emby (seeLogin/index.tsxwhereisJellyfinincludesMediaServerType.EMBY), a stalequickConnectOnly=truesetting (e.g. after switching servers) would hide the username/password form for Emby and effectively force a Jellyfin-only Quick Connect flow.
const quickConnectOnly = settings.currentSettings.quickConnectOnly;
server/lib/settings/index.ts:721
quickConnectOnlyis exposed infullPublicSettingseven when the configuredmediaServerTypeis not Jellyfin. If the setting remains true after switching away from Jellyfin, clients could still treat Quick Connect-only mode as enabled and hide credential login, potentially causing a lockout.
localLogin: this.data.main.localLogin,
mediaServerLogin: this.data.main.mediaServerLogin,
quickConnectOnly: this.data.main.quickConnectOnly,
jellyfinExternalHost: this.data.jellyfin.externalHostname,
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/components/Login/JellyfinLogin.tsx:49
quickConnectOnlyis applied even when this component is being used for an Emby server (serverType === MediaServerType.EMBY). Since the Quick Connect flow is implemented against Jellyfin-specific endpoints (/QuickConnect/*inserver/api/jellyfin.ts), enabling this setting while configured for Emby can hide the only working sign-in method and effectively lock users out. Gate the flag so it only hides the username/password form for Jellyfin.
const quickConnectOnly = settings.currentSettings.quickConnectOnly;
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
server/routes/auth.ts:265
- The error message returned when
quickConnectOnlyis enabled is misleading: this endpoint is the username/password Jellyfin login, so telling the caller it "can only be used with Quick Connect enabled" reads like Quick Connect must also be enabled to use password login. Consider changing the message to explicitly state that username/password login is disabled and users must use Quick Connect.
if (settings.main.quickConnectOnly) {
return res.status(500).json({
error: 'Jellyfin login can only be used with Quick Connect enabled.',
});
}
server/routes/auth.ts:265
quickConnectOnlyintroduces a new login-blocking branch inPOST /auth/jellyfin, but the existing auth route tests only cover the Quick Connect endpoints (seeserver/routes/auth.test.ts). Adding a test that asserts this endpoint rejects username/password login whensettings.main.quickConnectOnlyis true would prevent regressions.
if (settings.main.quickConnectOnly) {
return res.status(500).json({
error: 'Jellyfin login can only be used with Quick Connect enabled.',
});
}
600b942 to
2491ed7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
src/components/Settings/SettingsUsers/index.tsx:86
isJellyfinis also true forMediaServerType.EMBY, so the name is misleading. Renaming it will make the condition’s intent clearer (and avoid confusion if Jellyfin/Emby behavior diverges later).
const isJellyfin =
settings.currentSettings.mediaServerType === MediaServerType.JELLYFIN ||
settings.currentSettings.mediaServerType === MediaServerType.EMBY;
src/components/Settings/SettingsUsers/index.tsx:214
- Follow-up to the rename: update the usage site to match the new
isJellyfinOrEmbyvariable name.
{values.mediaServerLogin && isJellyfin && (
server/routes/auth.ts:265
- This branch rejects password-based Jellyfin/Emby sign-in when
quickConnectOnlyis enabled, but it responds with HTTP 500 (server error). Since this is an intentional policy restriction, a 4xx (e.g., 403 Forbidden) is more accurate and avoids triggering server-error monitoring/clients treating it as a backend failure.
if (settings.main.quickConnectOnly) {
return res.status(500).json({
error: 'Jellyfin login can only be used with Quick Connect enabled.',
});
}
✅ Action performedReview finished.
|
|
@coderabbitai full review |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (2)
server/routes/auth.ts:265
quickConnectOnlyblocksPOST /auth/jellyfinunconditionally, including during initial setup (mediaServerType === NOT_CONFIGURED). Quick Connect authentication is explicitly rejected during initial setup (/auth/jellyfin/quickconnect/authenticatereturns 403 in that case), so a migrated/manual config withquickConnectOnly: truecan lock the instance out of completing setup.
if (settings.main.quickConnectOnly) {
return res.status(403).json({
error: 'Jellyfin login can only be used with Quick Connect.',
});
}
server/routes/auth.ts:265
- The new Quick Connect-only enforcement on
POST /auth/jellyfinis not covered by route tests. Since Quick Connect behavior is already tested inserver/routes/auth.test.ts, add tests for the password-login route to ensure it returns 403 whenmain.quickConnectOnlyis enabled (and still allows setup whenmediaServerType === NOT_CONFIGURED).
if (settings.main.quickConnectOnly) {
return res.status(403).json({
error: 'Jellyfin login can only be used with Quick Connect.',
});
}
54d4b0c to
6cd87e3
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/components/Settings/SettingsUsers/index.tsx:133
quickConnectOnlyis posted back even when the checkbox is hidden/irrelevant (e.g. media-server sign-in is off or the configured media server isn’t Jellyfin/Emby). That can persist inconsistent settings values and makes the server-side behavior depend on a hidden form field. Consider coercing it tofalseunless Jellyfin/Emby sign-in is enabled.
await axios.post('/api/v1/settings/main', {
localLogin: values.localLogin,
mediaServerLogin: values.mediaServerLogin,
quickConnectOnly: values.quickConnectOnly,
newPlexLogin: values.newPlexLogin,
server/routes/auth.ts:265
- This new Quick Connect-only rejection returns
{ error: ... }, but the Jellyfin login UI (and other callers) primarily key offresponse.data.message(e.g.ApiErrorCode.*). Returning a different error shape makes it harder to surface a meaningful message and is inconsistent with the rest of this route’s error handling. Consider usingnext({ status, message })here as well.
if (settings.main.quickConnectOnly) {
return res.status(403).json({
error: 'Jellyfin login can only be used with Quick Connect.',
});
}
6cd87e3 to
f91dcd9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (2)
server/routes/auth.ts:265
quickConnectOnlyblocks POST /auth/jellyfin even during initial setup (mediaServerType === NOT_CONFIGURED). Quick Connect auth is explicitly unavailable during initial setup (see/jellyfin/quickconnect/authenticate), so if this flag is ever enabled via config/restore/API before configuration, the instance can get locked out of completing setup. Gate this restriction to configured instances only.
if (settings.main.quickConnectOnly) {
return res.status(403).json({
error: 'Jellyfin login can only be used with Quick Connect.',
});
}
server/routes/auth.ts:265
- The new
quickConnectOnlybehavior onPOST /auth/jellyfinis currently untested. Since this file already has route tests (server/routes/auth.test.ts), add coverage to assert (1) a configured instance returns 403 whenquickConnectOnlyis enabled, and (2) initial setup (mediaServerType === NOT_CONFIGURED) is not blocked by the flag.
This issue also appears on line 261 of the same file.
if (settings.main.quickConnectOnly) {
return res.status(403).json({
error: 'Jellyfin login can only be used with Quick Connect.',
});
}
f91dcd9 to
f39df2d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Suppressed comments (1)
server/routes/auth.ts:265
- This new Quick Connect-only enforcement is not covered by existing auth route tests. Adding coverage would help prevent regressions (e.g., verify
/auth/jellyfinreturns 403 whenquickConnectOnlyis enabled for Jellyfin, and that Emby login is unaffected).
if (settings.main.quickConnectOnly) {
return res.status(403).json({
error: 'Jellyfin login can only be used with Quick Connect.',
});
}
| const intl = useIntl(); | ||
| const settings = useSettings(); | ||
| const [showQuickConnect, setShowQuickConnect] = useState(false); | ||
| const quickConnectOnly = settings.currentSettings.quickConnectOnly; |
| const isJellyfin = | ||
| settings.currentSettings.mediaServerType === MediaServerType.JELLYFIN || | ||
| settings.currentSettings.mediaServerType === MediaServerType.EMBY; |
| if (settings.main.quickConnectOnly) { | ||
| return res.status(403).json({ | ||
| error: 'Jellyfin login can only be used with Quick Connect.', | ||
| }); | ||
| } |
f39df2d to
b49e465
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Suppressed comments (2)
src/components/Settings/SettingsUsers/index.tsx:86
isJellyfinincludesMediaServerType.EMBY, but the Quick Connect UI is only rendered for Jellyfin (JellyfinLoginshows the Quick Connect button only whenserverType === JELLYFIN). Allowing this setting on Emby can hide the username/password form without providing Quick Connect, effectively blocking sign-in.
const isJellyfin =
settings.currentSettings.mediaServerType === MediaServerType.JELLYFIN ||
settings.currentSettings.mediaServerType === MediaServerType.EMBY;
src/components/Login/JellyfinLogin.tsx:49
quickConnectOnlyis applied unconditionally from global settings. If it ever ends up enabled while this component is used for Emby, it will hide the password form but the Quick Connect button is only rendered for Jellyfin (serverType === JELLYFIN), leaving no usable login path.
const quickConnectOnly = settings.currentSettings.quickConnectOnly;
| } | ||
| /> | ||
| ); | ||
| setFieldValue('quickConnectOnly', false); |
| if (settings.main.quickConnectOnly) { | ||
| return res.status(403).json({ | ||
| error: 'Jellyfin login can only be used with Quick Connect.', | ||
| }); | ||
| } |
| if (settings.main.quickConnectOnly) { | ||
| return res.status(403).json({ | ||
| error: 'Jellyfin login can only be used with Quick Connect.', | ||
| }); | ||
| } |
b49e465 to
66aca33
Compare
Description
This PR adds a new setting called
quickConnectOnly.If ticked, the JellyfinLogin will hide the classic username / password form and only display the Quick Connect button
Allowing the users to log in with username and password forces the Jellyfin server to keep the local or LDAP Authentication service. Forcing them to use the Quick Connect allows the Jellyfin server to use the OIDC only.
How Has This Been Tested?
Tested in the webUI.
Screenshots / Logs (if applicable)
Enregistrement.d.ecran_20260730_121729.webm
AI Usage
Claude was used to understand some elements of the codebase and what should be changed.
Code was hand written with the goal to be sure of what I'm editing and why.
Checklist:
pnpm buildpnpm i18n:extractSummary by CodeRabbit
New Features
Bug Fixes