Fix: Double session initialization#647
Open
hdd wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThe pull request modernizes the Connect application file with extensive style updates (docstrings, string quotes, f-strings) alongside behavioral improvements: explicit session cleanup during reconfiguration, refactored plugin discovery with compatibility logic, and enhanced about-dialog debug information with compatibility tagging. ChangesConnect Application Modernization and Session Management
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Coverage report
This PR does not seem to contain any modification to coverable code. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/connect/source/ftrack_connect/ui/application.py (1)
217-225:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard restart cleanup when
instanceis not provided.Line [221] and Line [224] dereference
self._instanceeven though__init__acceptsinstance=None. Triggering restart in that state will raise before relaunch.Suggested fix
def restart(self): """restart connect application""" self.logger.info("Connect restarting....") QtWidgets.QApplication.quit() - self._instance.__del__() + if self._instance is not None: + self._instance.__del__() # Give enough time to ensure the lockfile has been removed - while os.path.exists(self._instance.lockfile): + lockfile = getattr(self._instance, "lockfile", None) + while lockfile and os.path.exists(lockfile): time.sleep(0.1)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/connect/source/ftrack_connect/ui/application.py` around lines 217 - 225, The restart method dereferences self._instance without guarding for None (__init__ can set instance=None); update restart to check that self._instance is truthy before calling self._instance.__del__ and before accessing self._instance.lockfile in the while loop (e.g., if self._instance: ...), so that restart safely no-ops cleanup when no instance was provided.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/connect/source/ftrack_connect/ui/application.py`:
- Around line 798-800: The code compares raw version strings
(current_dir_plugin["version"] and plugin["version"]) lexically, causing
incorrect ordering (e.g., "10" < "2"); replace the direct string comparison with
a version-aware comparison by parsing both versions with a proper version parser
(e.g., packaging.version.parse or packaging.version.Version) and then compare
the parsed versions; update the import and change the comparison expression used
where current_dir_plugin and plugin version values are compared so the newer
plugin is chosen reliably.
- Around line 1075-1079: The loop over self._widget_plugin_instances is
iterating dictionary keys rather than widget instances so isinstance(plugin,
ConnectWidget) always fails; change the iteration to loop over the actual values
(widget instances) and then append plugin.get_debug_information() for each
ConnectWidget instance—specifically update the loop that references
self._widget_plugin_instances and the isinstance check against ConnectWidget and
calls to get_debug_information() so it inspects the plugin objects (e.g., for
plugin in self._widget_plugin_instances.values() or equivalent) instead of keys.
- Line 654: The inline implementation comment contains a temporal marker "NEW:";
remove the "NEW:" prefix and rewrite the comment so it reads as a timeless
instruction (e.g., "Properly close the existing session before creating a new
one") near the session-creation/teardown logic (the comment attached to the
session cleanup and recreation code), ensuring no draft/history markers remain
and the comment is concise and permanent.
- Around line 449-451: The login overlay message passed to CancelOverlay uses a
malformed closing tag ("<h2/>"); update the message string in the creation of
self._login_overlay (CancelOverlay(..., message=...)) to use a properly closed
heading tag (e.g., "<h2>Signing in</h2>") so the HTML is valid and renders
consistently.
---
Outside diff comments:
In `@apps/connect/source/ftrack_connect/ui/application.py`:
- Around line 217-225: The restart method dereferences self._instance without
guarding for None (__init__ can set instance=None); update restart to check that
self._instance is truthy before calling self._instance.__del__ and before
accessing self._instance.lockfile in the while loop (e.g., if self._instance:
...), so that restart safely no-ops cleanup when no instance was provided.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 841b3aa0-9b7f-4f68-9e8c-073752bda5d8
📒 Files selected for processing (1)
apps/connect/source/ftrack_connect/ui/application.py
Comment on lines
449
to
451
| self._login_overlay = ftrack_connect.ui.widget.overlay.CancelOverlay( | ||
| self._login_widget, message='<h2>Signing in<h2/>' | ||
| self._login_widget, message="<h2>Signing in<h2/>" | ||
| ) |
There was a problem hiding this comment.
Fix malformed heading HTML in login overlay message.
Line [450] closes h2 incorrectly (<h2/>), which can render inconsistently.
Suggested fix
self._login_overlay = ftrack_connect.ui.widget.overlay.CancelOverlay(
- self._login_widget, message="<h2>Signing in<h2/>"
+ self._login_widget, message="<h2>Signing in</h2>"
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| self._login_overlay = ftrack_connect.ui.widget.overlay.CancelOverlay( | |
| self._login_widget, message='<h2>Signing in<h2/>' | |
| self._login_widget, message="<h2>Signing in<h2/>" | |
| ) | |
| self._login_overlay = ftrack_connect.ui.widget.overlay.CancelOverlay( | |
| self._login_widget, message="<h2>Signing in</h2>" | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/connect/source/ftrack_connect/ui/application.py` around lines 449 - 451,
The login overlay message passed to CancelOverlay uses a malformed closing tag
("<h2/>"); update the message string in the creation of self._login_overlay
(CancelOverlay(..., message=...)) to use a properly closed heading tag (e.g.,
"<h2>Signing in</h2>") so the HTML is valid and renders consistently.
| self.logger.exception("Failed to disconnect from event hub.") | ||
| pass | ||
|
|
||
| # NEW: Properly close the existing session before creating a new one |
There was a problem hiding this comment.
Remove temporal marker from implementation comment.
Line [654] uses NEW, which is draft-history residue and will stale over time.
Suggested fix
- # NEW: Properly close the existing session before creating a new one
+ # Close the existing session before creating a new one.As per coding guidelines, implementation code/comments must stand on their own and should not include temporal references like prior-step or draft markers.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # NEW: Properly close the existing session before creating a new one | |
| # Close the existing session before creating a new one. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/connect/source/ftrack_connect/ui/application.py` at line 654, The inline
implementation comment contains a temporal marker "NEW:"; remove the "NEW:"
prefix and rewrite the comment so it reads as a timeless instruction (e.g.,
"Properly close the existing session before creating a new one") near the
session-creation/teardown logic (the comment attached to the session cleanup and
recreation code), ensuring no draft/history markers remain and the comment is
concise and permanent.
Comment on lines
+798
to
800
| current_dir_plugin["version"] | ||
| < plugin["version"] | ||
| ): |
There was a problem hiding this comment.
Version selection uses lexical comparison instead of version-aware ordering.
Line [798]-Line [800] compares raw version values directly, which can choose the wrong “latest” plugin (e.g., "10" vs "2").
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/connect/source/ftrack_connect/ui/application.py` around lines 798 - 800,
The code compares raw version strings (current_dir_plugin["version"] and
plugin["version"]) lexically, causing incorrect ordering (e.g., "10" < "2");
replace the direct string comparison with a version-aware comparison by parsing
both versions with a proper version parser (e.g., packaging.version.parse or
packaging.version.Version) and then compare the parsed versions; update the
import and change the comparison expression used where current_dir_plugin and
plugin version values are compared so the newer plugin is chosen reliably.
Comment on lines
1075
to
1079
| for plugin in self._widget_plugin_instances: | ||
| if isinstance(plugin, ConnectWidget): | ||
| debug_information['widgets'].append( | ||
| debug_information["widgets"].append( | ||
| plugin.get_debug_information() | ||
| ) |
There was a problem hiding this comment.
Widget debug info loop iterates keys, not widget instances.
This loop iterates dict keys, so isinstance(plugin, ConnectWidget) is always false and widget debug info is silently skipped.
Suggested fix
- for plugin in self._widget_plugin_instances:
+ for plugin in self._widget_plugin_instances.values():
if isinstance(plugin, ConnectWidget):
debug_information["widgets"].append(
plugin.get_debug_information()
)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/connect/source/ftrack_connect/ui/application.py` around lines 1075 -
1079, The loop over self._widget_plugin_instances is iterating dictionary keys
rather than widget instances so isinstance(plugin, ConnectWidget) always fails;
change the iteration to loop over the actual values (widget instances) and then
append plugin.get_debug_information() for each ConnectWidget
instance—specifically update the loop that references
self._widget_plugin_instances and the isinstance check against ConnectWidget and
calls to get_debug_information() so it inspects the plugin objects (e.g., for
plugin in self._widget_plugin_instances.values() or equivalent) instead of keys.
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
1 participant
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 CodeRabbit
Bug Fixes
Improvements