Make issues more informative & use plexapi to login in browser on settings page#59
Make issues more informative & use plexapi to login in browser on settings page#59primetime43 wants to merge 2 commits intobbrown430:mainfrom
Conversation
Replaced the generic "Configuration saved successfully!" message with context-aware status: - No credentials: orange "Config saved. No Plex URL/token configured." - Connection/auth error: red, shows the first error message (e.g. "Unable to connect to Plex server: ..." or "Invalid Plex token: ...") - Library not found: orange "Config saved. TV library named 'X' not found: ..." - Full success: green "Config saved. Connected — 2 TV, 1 Movie library loaded" (with correct pluralization)
There was a problem hiding this comment.
Pull request overview
This PR enhances user experience by making error messages more informative and implementing Plex OAuth authentication. The changes address issues #57 and #58, replacing manual token entry with a streamlined browser-based sign-in flow while improving status feedback during configuration.
Changes:
- Added OAuth authentication flow with browser-based Plex sign-in
- Enhanced error/warning tracking in PlexService with detailed status messages
- Improved configuration save feedback to show connection status and library counts
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 13 comments.
| File | Description |
|---|---|
| src/ui/gui/tabs/settings_tab.py | Added OAuth button UI with status label for Plex sign-in |
| src/ui/gui/app.py | Implemented OAuth flow methods (_start_plex_oauth, _poll_plex_oauth, _finish_plex_oauth) and enhanced _save_config with context-aware status messages |
| src/services/plex_service.py | Added errors and warnings lists to track setup issues, converted library-not-found from error to warning |
Comments suppressed due to low confidence (1)
src/services/plex_service.py:75
- The
setupmethod returnsNone, Nonein multiple error cases (lines 44, 50, 52, 56, 59), but the return type annotation isTuple[List, List]which doesn't includeNoneas a valid type. This creates a type mismatch. Consider updating the return type annotation toTuple[Optional[List], Optional[List]]orOptional[Tuple[List, List]]to accurately reflect that the method can return None values.
def setup(self, gui_mode: bool = False) -> Tuple[List, List]:
"""Setup Plex server connection and libraries.
Args:
gui_mode: Whether running in GUI mode for error handling.
Returns:
Tuple of (tv_libraries, movie_libraries).
"""
self.errors = []
self.warnings = []
if not self.config or not self.config.base_url or not self.config.token:
error_msg = "Invalid Plex token or base URL. Please provide valid values in config.json or via the GUI."
self._handle_error(error_msg, gui_mode)
return None, None
try:
self.plex = PlexServer(self.config.base_url, self.config.token)
except requests.exceptions.RequestException as e:
self._handle_error(f"Unable to connect to Plex server: {str(e)}", gui_mode)
return None, None
except plexapi.exceptions.Unauthorized as e:
self._handle_error(f"Invalid Plex token: {str(e)}", gui_mode)
return None, None
except xml.etree.ElementTree.ParseError as e:
self._handle_error(f"Received invalid XML from Plex server: {str(e)}", gui_mode)
return None, None
except Exception as e:
self._handle_error(f"Unexpected error: {str(e)}", gui_mode)
return None, None
# Setup TV libraries
self.tv_libraries = self._setup_libraries(
self.config.tv_library,
"TV library",
gui_mode
)
# Setup movie libraries
self.movie_libraries = self._setup_libraries(
self.config.movie_library,
"Movie library",
gui_mode
)
return self.tv_libraries, self.movie_libraries
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _finish_plex_oauth(self, base_url, token, username): | ||
| """Complete the OAuth flow by populating UI fields and saving config.""" | ||
| self.base_url_entry.delete(0, ctk.END) | ||
| self.base_url_entry.insert(0, base_url) | ||
| self.token_entry.delete(0, ctk.END) | ||
| self.token_entry.insert(0, token) | ||
| self._update_oauth_status(f"Signed in as {username}!", color="#4CAF50") | ||
| self._save_config() |
There was a problem hiding this comment.
If the OAuth flow completes but no server resources are found or the base_url remains empty, the method still proceeds to populate the UI with an empty base_url and automatically saves the config. This could lead to a confusing state where the user is told they're signed in, but the connection won't work because no server URL was found. Consider adding validation to ensure a valid base_url was obtained before proceeding, and provide a meaningful error message to the user if no server is found.
| parts.append(f"{tv_count} TV") | ||
| if movie_count: | ||
| parts.append(f"{movie_count} Movie") | ||
| lib_summary = ", ".join(parts) + " " + ("libraries" if tv_count + movie_count != 1 else "library") if parts else "no libraries" |
There was a problem hiding this comment.
The string concatenation logic for lib_summary on line 431 is complex and hard to read. The expression mixes multiple ternary operators and string operations in a single line. Consider breaking this into multiple lines or using more explicit conditionals for better maintainability and readability.
| lib_summary = ", ".join(parts) + " " + ("libraries" if tv_count + movie_count != 1 else "library") if parts else "no libraries" | |
| if parts: | |
| total_libs = tv_count + movie_count | |
| label = "library" if total_libs == 1 else "libraries" | |
| lib_summary = f"{', '.join(parts)} {label}" | |
| else: | |
| lib_summary = "no libraries" |
| # Show context-aware status message based on Plex setup results | ||
| if not self.config.base_url or not self.config.token: | ||
| self._update_status("Config saved. No Plex URL/token configured.", color="orange") | ||
| elif self.plex_service.errors: | ||
| self._update_status(self.plex_service.errors[0], color="red") | ||
| elif self.plex_service.warnings: | ||
| self._update_status(f"Config saved. {self.plex_service.warnings[0]}", color="orange") | ||
| else: | ||
| tv_count = len(self.plex_service.tv_libraries) | ||
| movie_count = len(self.plex_service.movie_libraries) | ||
| parts = [] | ||
| if tv_count: | ||
| parts.append(f"{tv_count} TV") | ||
| if movie_count: | ||
| parts.append(f"{movie_count} Movie") | ||
| lib_summary = ", ".join(parts) + " " + ("libraries" if tv_count + movie_count != 1 else "library") if parts else "no libraries" | ||
| self._update_status(f"Config saved. Connected — {lib_summary} loaded", color="#4CAF50") |
There was a problem hiding this comment.
When a library is not found (lines 100-103), it's logged as a warning but still allows the process to continue. However, in _save_config (line 424-425), the code counts tv_libraries and movie_libraries that were successfully loaded. If the user configured 3 TV libraries but only 2 were found, the status will show "2 TV libraries loaded" which might be confusing without also showing the warning. The warning is only shown if there are no errors and the user would see "Config saved. TV library named 'X' not found" which doesn't indicate that other libraries were successfully loaded. Consider showing both the success count and the warning together, or making the warning message more informative about which libraries were successful.
| def _start_plex_oauth(self): | ||
| """Start the Plex OAuth sign-in flow.""" | ||
| from plexapi.myplex import MyPlexPinLogin | ||
|
|
||
| self._update_oauth_status("Opening Plex sign-in page...", color="#E5A00D") | ||
| try: | ||
| pin_login = MyPlexPinLogin(oauth=True) | ||
| oauth_url = pin_login.oauthUrl() | ||
| webbrowser.open(oauth_url) | ||
| self._update_oauth_status("Waiting for sign-in...", color="#E5A00D") | ||
| threading.Thread(target=self._poll_plex_oauth, args=(pin_login,), daemon=True).start() | ||
| except Exception as e: | ||
| self._update_oauth_status(f"OAuth error: {e}", color="red") |
There was a problem hiding this comment.
The _start_plex_oauth method doesn't prevent multiple concurrent OAuth flows. If the user clicks the "Sign in with Plex" button multiple times while waiting for authentication, multiple browser tabs will open and multiple polling threads will be created, which could lead to race conditions when updating the UI fields and config. Consider disabling the OAuth button while an OAuth flow is in progress and re-enabling it when the flow completes or times out.
| self.base_url_entry.insert(0, base_url) | ||
| self.token_entry.delete(0, ctk.END) | ||
| self.token_entry.insert(0, token) | ||
| self._update_oauth_status(f"Signed in as {username}!", color="#4CAF50") |
There was a problem hiding this comment.
The username retrieved from account.username (line 625) is only used for display in the OAuth status message. If the account.username attribute doesn't exist or is None, this could cause an exception. Consider adding a check to ensure the username exists before using it, or provide a fallback like "Signed in successfully!" if the username is not available.
| self._update_oauth_status(f"Signed in as {username}!", color="#4CAF50") | |
| # Safely handle cases where username is missing or not a valid string | |
| display_name = username if isinstance(username, str) and username.strip() else None | |
| if display_name: | |
| message = f"Signed in as {display_name}!" | |
| else: | |
| message = "Signed in successfully!" | |
| self._update_oauth_status(message, color="#4CAF50") |
| def _start_plex_oauth(self): | ||
| """Start the Plex OAuth sign-in flow.""" | ||
| from plexapi.myplex import MyPlexPinLogin | ||
|
|
||
| self._update_oauth_status("Opening Plex sign-in page...", color="#E5A00D") | ||
| try: | ||
| pin_login = MyPlexPinLogin(oauth=True) | ||
| oauth_url = pin_login.oauthUrl() | ||
| webbrowser.open(oauth_url) | ||
| self._update_oauth_status("Waiting for sign-in...", color="#E5A00D") | ||
| threading.Thread(target=self._poll_plex_oauth, args=(pin_login,), daemon=True).start() | ||
| except Exception as e: | ||
| self._update_oauth_status(f"OAuth error: {e}", color="red") | ||
|
|
||
| def _poll_plex_oauth(self, pin_login): | ||
| """Poll for Plex OAuth completion in a background thread.""" | ||
| import time | ||
| for _ in range(150): # 5 minute timeout | ||
| time.sleep(2) | ||
| if pin_login.checkLogin(): | ||
| token = pin_login.token | ||
| if token: | ||
| try: | ||
| from plexapi.myplex import MyPlexAccount | ||
| account = MyPlexAccount(token=token) | ||
| resources = [r for r in account.resources() if 'server' in r.provides] | ||
| base_url = "" | ||
| if resources: | ||
| server = resources[0] | ||
| try: | ||
| plex_server = server.connect() | ||
| base_url = plex_server._baseurl | ||
| except Exception: | ||
| if server.connections: | ||
| base_url = server.connections[0].uri | ||
| self.app.after(0, lambda bu=base_url, t=token, u=account.username: self._finish_plex_oauth(bu, t, u)) | ||
| except Exception as e: | ||
| self.app.after(0, lambda err=e: self._update_oauth_status(f"OAuth error: {err}", color="red")) | ||
| return | ||
| self.app.after(0, lambda: self._update_oauth_status("Sign-in timed out", color="red")) | ||
|
|
||
| def _finish_plex_oauth(self, base_url, token, username): | ||
| """Complete the OAuth flow by populating UI fields and saving config.""" | ||
| self.base_url_entry.delete(0, ctk.END) | ||
| self.base_url_entry.insert(0, base_url) | ||
| self.token_entry.delete(0, ctk.END) | ||
| self.token_entry.insert(0, token) | ||
| self._update_oauth_status(f"Signed in as {username}!", color="#4CAF50") | ||
| self._save_config() | ||
|
|
||
| def _update_oauth_status(self, message, color="#696969"): | ||
| """Update the OAuth status label on the settings tab.""" | ||
| def update(): | ||
| if self.settings_tab and hasattr(self.settings_tab, 'oauth_status_label'): | ||
| self.settings_tab.oauth_status_label.configure(text=message, text_color=color) | ||
| self.app.after(0, update) | ||
|
|
There was a problem hiding this comment.
The new OAuth functionality (_start_plex_oauth, _poll_plex_oauth, _finish_plex_oauth) lacks test coverage. While the repository has some tests (test_module.py), they focus on scraping functionality. Consider adding unit tests for the OAuth flow, particularly for edge cases like timeout scenarios, error handling, and the case where no Plex servers are found.
| @@ -33,6 +35,9 @@ def setup(self, gui_mode: bool = False) -> Tuple[List, List]: | |||
| Returns: | |||
| Tuple of (tv_libraries, movie_libraries). | |||
There was a problem hiding this comment.
The setup method's docstring doesn't mention the new behavior where errors and warnings are collected in self.errors and self.warnings lists. The documentation should be updated to note that callers can check these lists to get detailed error and warning information after calling setup.
| Tuple of (tv_libraries, movie_libraries). | |
| Tuple of (tv_libraries, movie_libraries). | |
| Notes: | |
| This method clears ``self.errors`` and ``self.warnings`` at the start of | |
| each call, then populates them with any errors or warnings encountered | |
| while connecting to the Plex server and setting up libraries. Callers | |
| can inspect these lists after calling ``setup`` for detailed diagnostic | |
| information. |
| self._update_oauth_status(f"Signed in as {username}!", color="#4CAF50") | ||
| self._save_config() | ||
|
|
||
| def _update_oauth_status(self, message, color="#696969"): |
There was a problem hiding this comment.
The _update_oauth_status method lacks type hints for its parameters, while the similar _update_status method (line 499) has complete type hints (message: str, color: str = "white"). For consistency with the codebase conventions, consider adding type hints to this method's parameters.
| def _update_oauth_status(self, message, color="#696969"): | |
| def _update_oauth_status(self, message: str, color: str = "#696969"): |
| def _poll_plex_oauth(self, pin_login): | ||
| """Poll for Plex OAuth completion in a background thread.""" | ||
| import time | ||
| for _ in range(150): # 5 minute timeout | ||
| time.sleep(2) | ||
| if pin_login.checkLogin(): | ||
| token = pin_login.token | ||
| if token: | ||
| try: | ||
| from plexapi.myplex import MyPlexAccount | ||
| account = MyPlexAccount(token=token) | ||
| resources = [r for r in account.resources() if 'server' in r.provides] | ||
| base_url = "" | ||
| if resources: | ||
| server = resources[0] | ||
| try: | ||
| plex_server = server.connect() | ||
| base_url = plex_server._baseurl | ||
| except Exception: | ||
| if server.connections: | ||
| base_url = server.connections[0].uri | ||
| self.app.after(0, lambda bu=base_url, t=token, u=account.username: self._finish_plex_oauth(bu, t, u)) | ||
| except Exception as e: | ||
| self.app.after(0, lambda err=e: self._update_oauth_status(f"OAuth error: {err}", color="red")) | ||
| return | ||
| self.app.after(0, lambda: self._update_oauth_status("Sign-in timed out", color="red")) | ||
|
|
||
| def _finish_plex_oauth(self, base_url, token, username): |
There was a problem hiding this comment.
The _poll_plex_oauth and _finish_plex_oauth methods lack type hints for their parameters. For consistency with other methods in the codebase that use type hints (e.g., _update_status on line 499, _update_progress on line 510), consider adding type hints to these methods.
| def _poll_plex_oauth(self, pin_login): | ||
| """Poll for Plex OAuth completion in a background thread.""" | ||
| import time | ||
| for _ in range(150): # 5 minute timeout | ||
| time.sleep(2) | ||
| if pin_login.checkLogin(): | ||
| token = pin_login.token | ||
| if token: | ||
| try: | ||
| from plexapi.myplex import MyPlexAccount | ||
| account = MyPlexAccount(token=token) | ||
| resources = [r for r in account.resources() if 'server' in r.provides] | ||
| base_url = "" | ||
| if resources: | ||
| server = resources[0] | ||
| try: | ||
| plex_server = server.connect() | ||
| base_url = plex_server._baseurl | ||
| except Exception: | ||
| if server.connections: | ||
| base_url = server.connections[0].uri | ||
| self.app.after(0, lambda bu=base_url, t=token, u=account.username: self._finish_plex_oauth(bu, t, u)) | ||
| except Exception as e: | ||
| self.app.after(0, lambda err=e: self._update_oauth_status(f"OAuth error: {err}", color="red")) | ||
| return | ||
| self.app.after(0, lambda: self._update_oauth_status("Sign-in timed out", color="red")) |
There was a problem hiding this comment.
The _poll_plex_oauth method polls every 2 seconds for 150 iterations (5 minutes), but if the user completes authentication quickly, the thread continues polling unnecessarily. Consider adding a flag to stop polling once the OAuth flow is complete or cancelled. Additionally, there's no way for the user to cancel the OAuth flow after it starts, which could lead to confusion if they accidentally clicked the button or changed their mind.
Make issues more informative & use plexapi to login in browser on settings page
#57
#58