-
Notifications
You must be signed in to change notification settings - Fork 62
fix: auto refresh recents page #1212
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
🎉 This PR is included in version 0.56.6 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
| if not self.show_action_menu: | ||
| self.action_menu_selection = 0 # Reset selection when closing | ||
|
|
||
| def _refresh(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Race condition: Manual refresh can run concurrently with background auto-refresh
Without locking, _refresh can overlap with _auto_refresh_loop, leading to concurrent updates and redraws. Guard the manual refresh with the same lock.
| def _refresh(self): | |
| def _refresh(self): | |
| """Refresh the agent runs list.""" | |
| # Prevent concurrent refresh if background refresh is running | |
| acquired = self._refresh_lock.acquire(blocking=False) | |
| if not acquired: | |
| return | |
| try: | |
| # Indicate refresh and redraw immediately so the user sees it | |
| self.is_refreshing = True | |
| self._clear_and_redraw() | |
| if self._load_agent_runs(): | |
| self.selected_index = 0 # Reset selection | |
| finally: | |
| # Clear refresh indicator and redraw with updated data | |
| self.is_refreshing = False | |
| self._clear_and_redraw() | |
| self._refresh_lock.release() |
| finally: | ||
| self.is_refreshing = False | ||
|
|
||
| # Redraw only if still on recents and app running |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic bug: Refresh indicator cleared before redraw
is_refreshing is set to False before calling _clear_and_redraw(), so the header never shows the "Refreshing…" indicator during auto-refresh. Keep the flag set through the redraw, then clear it.
| # Redraw only if still on recents and app running | |
| def _background_refresh(self): | |
| """Refresh data without disrupting selection/menu state; redraw if still on recents.""" | |
| self.is_refreshing = True | |
| previous_index = self.selected_index | |
| try: | |
| if self._load_agent_runs(): | |
| if self.agent_runs: | |
| self.selected_index = max(0, min(previous_index, len(self.agent_runs) - 1)) | |
| else: | |
| self.selected_index = 0 | |
| finally: | |
| # Redraw while the indicator is still visible | |
| if self.running and self.current_tab == 0: | |
| self._clear_and_redraw() | |
| # Now clear the indicator | |
| self.is_refreshing = False |
| signal.signal(signal.SIGINT, self._signal_handler) | ||
|
|
||
| # Start background auto-refresh thread (daemon) | ||
| self._auto_refresh_thread = threading.Thread(target=self._auto_refresh_loop, daemon=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Runtime error: self.org_id may be undefined when unauthenticated
The background thread can trigger _load_agent_runs() which checks self.org_id before it's set for unauthenticated users, causing AttributeError. Initialize self.org_id unconditionally before starting the thread.
| self._auto_refresh_thread = threading.Thread(target=self._auto_refresh_loop, daemon=True) | |
| def __init__(self): | |
| self.token = get_current_token() | |
| self.is_authenticated = bool(self.token) | |
| self.org_id = None # ensure attribute is always defined | |
| if self.is_authenticated: | |
| self.org_id = resolve_org_id() | |
| self.agent_runs: list[dict[str, Any]] = [] | |
| self.selected_index = 0 | |
| self.running = True | |
| self.show_action_menu = False | |
| self.action_menu_selection = 0 | |
| # Tab management | |
| self.tabs = ["recents", "new", "web"] | |
| self.current_tab = 0 | |
| # Refresh state | |
| self.is_refreshing = False | |
| self._auto_refresh_interval_seconds = 10 | |
| self._refresh_lock = threading.Lock() | |
| # New tab state | |
| self.prompt_input = "" | |
| self.cursor_position = 0 | |
| self.input_mode = False # When true, we're typing in the input box | |
| # Set up signal handler for Ctrl+C | |
| signal.signal(signal.SIGINT, self._signal_handler) | |
| # Start background auto-refresh thread (daemon) | |
| self._auto_refresh_thread = threading.Thread(target=self._auto_refresh_loop, daemon=True) | |
| self._auto_refresh_thread.start() |
|
|
||
| # Redraw only if still on recents and app running | ||
| if self.running and self.current_tab == 0: | ||
| self._clear_and_redraw() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thread-safety/UI race: Background thread redraws can interleave with main-thread redraws
Printing from both threads can corrupt terminal output. Signal the main loop to redraw instead of drawing from the background thread.
| self._clear_and_redraw() | |
| def _background_refresh(self): | |
| """Refresh data without disrupting selection/menu state; request redraw on main thread.""" | |
| self.is_refreshing = True | |
| previous_index = self.selected_index | |
| try: | |
| if self._load_agent_runs(): | |
| if self.agent_runs: | |
| self.selected_index = max(0, min(previous_index, len(self.agent_runs) - 1)) | |
| else: | |
| self.selected_index = 0 | |
| finally: | |
| # Request a redraw by main loop instead of drawing from this thread | |
| self._pending_redraw = True | |
| self.is_refreshing = False | |
| # In run(), trigger redraws when requested by background thread | |
| # (Place near the top of the while loop, before waiting for keypress) | |
| # while self.running: | |
| # try: | |
| # if getattr(self, "_pending_redraw", False): | |
| # self._clear_and_redraw() | |
| # self._pending_redraw = False | |
| # key = self._get_char() | |
| # self._handle_keypress(key) | |
| # if self.running: | |
| # self._clear_and_redraw() | |
| # except KeyboardInterrupt: | |
| # break |
|
|
||
| def _auto_refresh_loop(self): | ||
| """Background loop to auto-refresh recents tab every interval.""" | ||
| while True: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Busy-wait risk: while True loop sleeps unconditionally even after running is set False
If running becomes False, you still sleep another interval before exiting. Check running before sleeping to exit promptly.
| while True: | |
| def _auto_refresh_loop(self): | |
| """Background loop to auto-refresh recents tab every interval.""" | |
| while self.running: | |
| # Sleep first so we don't immediately spam a refresh on start | |
| time.sleep(self._auto_refresh_interval_seconds) | |
| if not self.running: | |
| break | |
| if self.current_tab == 0 and not self.is_refreshing: | |
| acquired = self._refresh_lock.acquire(blocking=False) | |
| if not acquired: | |
| continue | |
| try: | |
| if self.running and self.current_tab == 0 and not self.is_refreshing: | |
| self._background_refresh() | |
| finally: | |
| self._refresh_lock.release() |
| # Only refresh when on recents tab and not currently refreshing | ||
| if self.current_tab == 0 and not self.is_refreshing: | ||
| # Try background refresh; if lock is busy, skip this tick | ||
| acquired = self._refresh_lock.acquire(blocking=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential deadlock/missed refresh: Skipping refresh when lock not acquired can starve updates under frequent manual refreshes
If the lock is often held by manual refreshes, the thread may keep skipping and never update. Use a short timeout instead of non-blocking acquire.
| acquired = self._refresh_lock.acquire(blocking=False) | |
| def _auto_refresh_loop(self): | |
| """Background loop to auto-refresh recents tab every interval.""" | |
| while self.running: | |
| time.sleep(self._auto_refresh_interval_seconds) | |
| if not self.running: | |
| break | |
| if self.current_tab == 0 and not self.is_refreshing: | |
| # Try acquire with a small timeout to avoid starvation | |
| acquired = self._refresh_lock.acquire(timeout=0.25) | |
| if not acquired: | |
| continue | |
| try: | |
| if self.running and self.current_tab == 0 and not self.is_refreshing: | |
| self._background_refresh() | |
| finally: | |
| self._refresh_lock.release() |
|
Found 5 issues. Please review my inline comments above. |
Motivation
Content
Testing
Please check the following before marking your PR as ready for review