refactor kick a bit: consolidate token refresh and fix failing tests #1204
Conversation
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
Reviewer's GuideThis PR refactors Kick OAuth handling by centralizing token refresh into a new async utility, simplifying UI redirect URI management with an auto‐generated label, updating token introspection to the correct POST endpoint, adding a periodic status timer for real‐time updates in the settings UI, and cleaning up tests to mock the consolidated logic and drop obsolete cases. Sequence diagram for consolidated token refresh in chat/launchsequenceDiagram
participant ChatOrLaunch as Chat/Launch
participant Utils as attempt_token_refresh
participant KickOAuth2
participant Config as ConfigFile
ChatOrLaunch->>Utils: await attempt_token_refresh(config)
Utils->>KickOAuth2: new KickOAuth2(config)
Utils->>KickOAuth2: get_stored_tokens()
alt Access token present
Utils->>KickOAuth2: validate_token(access_token)
alt Token valid
Utils-->>ChatOrLaunch: return True
else Token invalid
alt Refresh token present
Utils->>KickOAuth2: refresh_access_token(refresh_token)
Utils-->>ChatOrLaunch: return True
else No refresh token
Utils-->>ChatOrLaunch: return False
end
end
else No access token
Utils-->>ChatOrLaunch: return False
end
Class diagram for consolidated Kick OAuth token refreshclassDiagram
class KickOAuth2 {
+get_stored_tokens() tuple[str|None, str|None]
+refresh_access_token(refresh_token)
+validate_token(token)
+revoke_token(token)
access_token
refresh_token
config
}
class ConfigFile
class Utils {
+attempt_token_refresh(config: ConfigFile) bool
+validate_kick_token_async(config: ConfigFile, access_token: str|None) dict|None
+qtsafe_validate_kick_token(access_token: str) bool
}
KickOAuth2 <.. Utils : uses
ConfigFile <.. KickOAuth2 : used by
ConfigFile <.. Utils : used by
Class diagram for KickSettings UI changesclassDiagram
class KickSettings {
widget
oauth
refresh_token
status_timer
+connect(uihelp, widget)
+load(config, widget)
+save(config, widget, subprocesses)
+verify(widget)
+authenticate_oauth()
+start_status_timer()
+stop_status_timer()
+cleanup()
+update_oauth_status()
}
class QTimer
KickSettings --> QTimer : manages
KickSettings --> KickOAuth2 : uses
KickSettings --> ConfigFile : uses
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 @aw-was-here - I've reviewed your changes - here's some feedback:
- Ensure Settings.cleanup() is wired up to be called when the UI closes so the QTimer is stopped and doesn’t leak resources.
- Add a test covering non-default webserver ports to verify the auto-generated redirect URI logic works for all configured ports.
- Consider unifying the sync and async token-introspection implementations to reduce duplicate endpoint handling logic.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Ensure Settings.cleanup() is wired up to be called when the UI closes so the QTimer is stopped and doesn’t leak resources.
- Add a test covering non-default webserver ports to verify the auto-generated redirect URI logic works for all configured ports.
- Consider unifying the sync and async token-introspection implementations to reduce duplicate endpoint handling logic.
## Individual Comments
### Comment 1
<location> `nowplaying/kick/settings.py:49` </location>
<code_context>
# Initialize OAuth2 handler
self.oauth = nowplaying.kick.oauth2.KickOAuth2(config)
self.update_oauth_status()
+ self.start_status_timer()
@staticmethod
</code_context>
<issue_to_address>
Consider stopping the status timer when the settings UI is closed.
Ensure the cleanup method reliably stops the timer whenever the settings UI is closed to avoid background processes persisting.
Suggested implementation:
```python
self.start_status_timer()
```
```python
def stop_status_timer(self):
"""Stop the status timer if it is running."""
if hasattr(self, 'status_timer') and self.status_timer is not None:
self.status_timer.stop()
self.status_timer = None
@staticmethod
```
You will also need to:
1. Call `self.stop_status_timer()` in the method that handles the closing or cleanup of the settings UI (e.g., in a `closeEvent`, `cleanup`, or equivalent method for your settings dialog/class).
2. Ensure that `self.status_timer` is the QTimer instance started in `start_status_timer()`.
If you provide the relevant cleanup/close method, I can show the exact code to insert the call.
</issue_to_address>
### Comment 2
<location> `nowplaying/kick/utils.py:61` </location>
<code_context>
+ return await oauth.validate_token(access_token)
+
def qtsafe_validate_kick_token(access_token: str) -> bool:
- ''' validate kick token synchronously (shared by settings and launch) '''
+ ''' Validate kick token synchronously (Qt-safe for UI components) '''
</code_context>
<issue_to_address>
Consider handling network errors explicitly in qtsafe_validate_kick_token.
Returning False for network errors makes them indistinguishable from invalid tokens. Logging or surfacing these errors would improve diagnostics.
</issue_to_address>
### Comment 3
<location> `nowplaying/kick/oauth2.py:276` </location>
<code_context>
+ async with aiohttp.ClientSession() as session:
</code_context>
<issue_to_address>
Consider handling non-200/401 HTTP responses more robustly in validate_token.
Logging unexpected HTTP errors as warnings or errors will make troubleshooting API issues easier.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
aw-was-here
force-pushed
the
kick_is_broken
branch
from
be1e87a to
d6e29ca
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1204 +/- ##
==========================================
- Coverage 66.56% 66.54% -0.02%
==========================================
Files 63 63
Lines 10697 10714 +17
==========================================
+ Hits 7120 7130 +10
- Misses 3577 3584 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
* Consolidate duplicated token refresh logic from chat.py/launch.py into utils.py * Add attempt_token_refresh() function to replace individual OAuth method calls * Change redirect URI from user-editable lineedit to auto-generated label * Update OAuth2 to use correct /public/v1/token/introspect endpoint (POST) * Fix broken help URL reference to use "Developer tab in streamer settings" * Add real-time OAuth status updates with 30s QTimer * Update tests to mock consolidated function instead of individual OAuth methods * Fix test expectations for redirecturi_label and port 8899 default * Remove 6 obsolete tests that were testing implementation details * Simplify authentication test scenarios from complex flows to success/failure
aw-was-here
force-pushed
the
kick_is_broken
branch
from
d6e29ca to
e79a3c2
Compare
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.
Summary by Sourcery
Centralize OAuth token handling by consolidating refresh and validation logic into a single utility function, auto-generate the redirect URI, add live status polling, update chat and launch flows to use the new function, and adjust tests and UI accordingly.
New Features:
Bug Fixes:
Enhancements:
Tests: