Prevent duplicate instances and add IPC for check-now command#70
Merged
Prevent duplicate instances and add IPC for check-now command#70
Conversation
- Use setproctitle and psutil to detect existing update-station processes - Send SIGUSR1 to running instance when 'check-now' is invoked, triggering update check in existing GTK loop - Consolidate notification.py into frontend.py to reduce module count - Simplify boolean returns in backend.py - Exit with error if tray mode started while instance is already running This prevents multiple update-station processes from running simultaneously and allows the check-now command to communicate with the existing tray instance instead of spawning a separate GUI process.
Contributor
Reviewer's GuideImplements single-instance behavior for update-station with IPC for the Sequence diagram for single-instance check-now IPC behaviorsequenceDiagram
actor User
participant CLIUpdateStation as CLI_update_station
participant ProcessChecker as Process_checker
participant RunningTray as Running_tray_instance
participant GTK as GTK_main_loop
participant Backend as Backend_helpers
User->>CLIUpdateStation: run check-now
CLIUpdateStation->>ProcessChecker: search for update-station process
alt existing instance found
ProcessChecker-->>CLIUpdateStation: PID of running instance
CLIUpdateStation-->>RunningTray: send SIGUSR1
RunningTray->>GTK: signal handler posts check-now event
GTK->>Backend: check_for_update()
Backend-->>GTK: update_available / none / major_upgrade
alt updates available
GTK-->>RunningTray: show update window / tray notification
else major upgrade available
GTK-->>RunningTray: show MajorUpgradeWindow
else no updates
GTK-->>RunningTray: show NoUpdateAvailable state
end
CLIUpdateStation-->>User: exit after notifying running instance
else no existing instance
ProcessChecker-->>CLIUpdateStation: no PID found
CLIUpdateStation->>CLIUpdateStation: start tray mode GTK instance
CLIUpdateStation-->>User: tray instance started
end
Updated class diagram for frontend tray, notifier, and major upgrade UIclassDiagram
class Data {
<<static>> bool major_upgrade
<<static>> bool kernel_upgrade
<<static>> bool do_not_upgrade
<<static>> string current_abi
<<static>> string new_abi
<<static>> bool close_session
<<static>> TrayIcon system_tray
<<static>> bool stop_pkg_refreshing
}
class UpdateNotifier {
+NotifyNotification notification
+string msg
+int timeout
+UpdateNotifier()
+notify()
+on_activated(notification, action_name)
}
class TrayIcon {
+GtkStatusIcon status_icon
+GtkMenu menu
+TrayIcon()
+GtkStatusIcon tray_icon()
+GtkMenu nm_menu()
+left_click(status_icon)
+icon_clicked(status_icon, button, time)
}
class MajorUpgradeWindow {
+MajorUpgradeWindow()
+on_clicked(widget)
+on_close(widget, event) bool
}
class GtkWindow
UpdateNotifier --> Data : reads_flags_and_state
UpdateNotifier --> MajorUpgradeWindow : opens_on_major_upgrade
UpdateNotifier --> TrayIcon : hides_tray_icon
TrayIcon --> Data : reads_and_updates_state
TrayIcon --> MajorUpgradeWindow : opens_on_major_upgrade
MajorUpgradeWindow --> Data : updates_upgrade_flags
MajorUpgradeWindow --> UpdateNotifier : indirectly_triggers_update_check
MajorUpgradeWindow ..|> GtkWindow
Flow diagram for single-instance process and tray mode startupflowchart LR
A["User runs update-station (tray or check-now)"] --> B["Set process title and scan for existing update-station instance"]
B --> C{Existing instance found?}
C -- Yes --> D["Send SIGUSR1 to running tray instance"]
D --> E["Existing GTK loop handles signal and triggers check_for_update"]
E --> F["Existing instance shows appropriate UI (update window, MajorUpgradeWindow, or no-update message)"]
C -- No --> G["Start new GTK tray instance"]
G --> H["Initialize TrayIcon and UpdateNotifier in frontend"]
H --> I["Enter GTK main loop as single running instance"]
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}
Contributor
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- In MajorUpgradeWindow, building the prompt string with an f-string inside _() means gettext will never see the full sentence; consider using a format placeholder (e.g. _('Would you like to upgrade from {current} to {new}?').format(current=..., new=...)) so the full translatable string is extracted.
- MajorUpgradeWindow.on_clicked relies on widget.get_label() == 'Yes', which will break if the button label is ever localized or changed; use separate callbacks or data attributes to distinguish the buttons instead of comparing the label text.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In MajorUpgradeWindow, building the prompt string with an f-string inside _() means gettext will never see the full sentence; consider using a format placeholder (e.g. _('Would you like to upgrade from {current} to {new}?').format(current=..., new=...)) so the full translatable string is extracted.
- MajorUpgradeWindow.on_clicked relies on widget.get_label() == 'Yes', which will break if the button label is ever localized or changed; use separate callbacks or data attributes to distinguish the buttons instead of comparing the label text.
## Individual Comments
### Comment 1
<location> `update_station/frontend.py:780-782` </location>
<code_context>
+ vbox.set_border_width(10)
+ self.add(vbox)
+
+ label = Gtk.Label(
+ label=_(f"Would you like to upgrade from {Data.current_abi} to {Data.new_abi}?"
+ "\n\nIf you select No, the upgrade will be skipped until the next boot.")
+ )
</code_context>
<issue_to_address>
**suggestion:** Avoid using f-strings directly inside _() to keep the string properly translatable.
Using an f-string inside _() requires an exact match (including the dynamic values) in the translation catalog, which usually won’t exist and degrades i18n. Prefer a placeholder-based string, e.g.:
`_("Would you like to upgrade from {current} to {new}?").format(current=Data.current_abi, new=Data.new_abi)`
Then either append the second sentence separately or include it in the same translatable string.
```suggestion
vbox = Gtk.Box(orientation=Gtk.Orientation.VERTICAL, spacing=5)
vbox.set_border_width(10)
self.add(vbox)
label = Gtk.Label(
label=_(
"Would you like to upgrade from {current} to {new}?\n\n"
"If you select No, the upgrade will be skipped until the next boot."
).format(current=Data.current_abi, new=Data.new_abi)
)
vbox.pack_start(label, True, True, 0)
```
</issue_to_address>
### Comment 2
<location> `update_station/frontend.py:791-795` </location>
<code_context>
+ vbox.pack_start(label, True, True, 5)
+ hbox = Gtk.Box(orientation=Gtk.Orientation.HORIZONTAL, spacing=5)
+ vbox.pack_start(hbox, False, False, 0)
+ button1 = Gtk.Button(label="Yes")
+ button1.connect("clicked", self.on_clicked)
+ hbox.pack_end(button1, True, True, 0)
+
+ button2 = Gtk.Button(label="No")
+ button2.connect("clicked", self.on_clicked)
+ hbox.pack_end(button2, True, True, 0)
</code_context>
<issue_to_address>
**issue:** Using hardcoded "Yes"/"No" labels and comparing by label makes the logic brittle and non-localizable.
`on_clicked` currently checks `widget.get_label() == "Yes"`, coupling behavior to UI text and blocking localization. Prefer either distinct callbacks per button, attaching explicit identifiers (e.g. `widget.set_name("confirm_upgrade")`), or using response IDs. Also, wrap the button labels in `_()` to match the rest of the UI’s translation handling.
</issue_to_address>
### Comment 3
<location> `update_station/frontend.py:719` </location>
<code_context>
+ The constructor for the TrayIcon class.
+ """
+ self.status_icon = Gtk.StatusIcon()
+ self.status_icon.set_tooltip_text('Update Available')
+ self.menu = Gtk.Menu()
+ self.menu.show_all()
</code_context>
<issue_to_address>
**suggestion:** Tooltip text for the tray icon is not translated, unlike the rest of the UI.
This tooltip should also go through `_()` for consistency with the rest of the UI and to ensure it’s localizable.
Suggested implementation:
```python
self.status_icon = Gtk.StatusIcon()
self.status_icon.set_tooltip_text(_('Update Available'))
self.menu = Gtk.Menu()
```
If `_` is not yet imported/defined in this module, you will also need to:
1. Import it at the top of the file, e.g. `from gettext import gettext as _` (or match the existing i18n pattern used elsewhere in this project).
</issue_to_address>
### Comment 4
<location> `update_station/backend.py:253` </location>
<code_context>
"""
next_version = f'{get_default_repo_url()}/.next_version'
- return True if requests.get(next_version).status_code == 200 else False
+ return requests.get(next_version).status_code == 200
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Network call in is_major_upgrade_available lacks a timeout and may block indefinitely.
Because this may be called from UI-driven flows, an unbounded `requests.get` can cause the UI to hang if the server is slow or unreachable. Please add a reasonable timeout (e.g. `timeout=5`) and handle failures by treating them as "no upgrade available".
Suggested implementation:
```python
def get_pkg_upgrade_data() -> dict:
"""
This function is used to get the upgrade data from pkg.
:return: True if the major upgrade is ready else False.
"""
next_version = f'{get_default_repo_url()}/.next_version'
try:
response = requests.get(next_version, timeout=5)
except requests.RequestException:
# If we cannot reach the server or the request fails for any reason,
# treat it as "no upgrade available" to avoid blocking UI flows.
return False
return response.status_code == 200
```
1. Ensure `requests` is imported at the top of `update_station/backend.py`, e.g.:
`import requests`
2. If the return type is intended to be a boolean, consider updating the type hint and docstring to `-> bool` for accuracy.
</issue_to_address>
### Comment 5
<location> `update_station/backend.py:327` </location>
<code_context>
"""
syncing_url = f'{get_default_repo_url()}/.syncing'
- return True if requests.get(syncing_url).status_code == 200 else False
+ return requests.get(syncing_url).status_code == 200
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Similar to is_major_upgrade_available, repository_is_syncing should use a timeout on the HTTP request.
To prevent the call from hanging on slow or unreachable mirrors, pass a small timeout to `requests.get` (e.g., `timeout=5`) and handle the potential timeout/connection errors so the check remains responsive.
Suggested implementation:
```python
syncing_url = f'{get_default_repo_url()}/.syncing'
try:
response = requests.get(syncing_url, timeout=5)
except (requests.exceptions.Timeout,
requests.exceptions.ConnectionError,
requests.exceptions.RequestException):
return False
return response.status_code == 200
```
1. Ensure `requests` is imported at the top of `update_station/backend.py`:
- `import requests`
2. If there is a shared timeout configuration or constant in this project, replace the hard-coded `timeout=5` with that shared value to stay consistent with existing conventions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Backend: Add try-except handling with timeouts for server request failures (`requests.get`), preventing UI blocking during unreachable servers. - Frontend: Localize UI strings with `_()` for better internationalization support. - UI: Refactor dialog buttons to invoke corresponding "Yes" and "No" click handlers, improving code clarity and maintainability.
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.
This prevents multiple update-station processes from running simultaneously and allows the check-now command to communicate with the existing tray instance instead of spawning a separate GUI process.
Summary by Sourcery
Prevent multiple update-station GUI instances from running and route on-demand update checks to an existing tray process via IPC while improving major-upgrade handling and notifications.
New Features:
Enhancements: