[rocprofiler-compute][tui] menu bar lag fix #1942

xuchen-amd · 2025-11-19T20:23:31Z

Motivation

TUI mode is experiencing lags, specifically in the "menu bar".
The cause is because button press are not properly handled, especially when multiple parts/screens are triggered.
i.e., when the dropdown menu and the recent directory are both active, button press events may not be handled properly by the respective event.
Third-party filepicker is also not properly handling double button presses.

Technical Details

Properly handle button press events by closing/hiding menus where applicable.
Use custom file/directory picker instead of third-party fspicker.

Test Plan

ctest
manual test of TUI on linux, windows, mac

Test Result

ctest
manual test of TUI on linux, windows, mac

Submission Checklist

Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.

Copilot

Pull Request Overview

This PR addresses lag issues in the TUI menu bar by improving button press event handling and replacing a third-party file picker with a custom implementation. The changes ensure that UI events (button presses, dropdown menus, directory selection) are properly isolated and handled without interference.

Key changes:

Refactored button press handlers to use targeted event decorators (@on) for more precise event handling
Implemented a custom DirectoryPicker widget to replace the third-party textual-fspicker library
Added explicit dropdown menu closing when modals are opened to prevent UI state conflicts

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
recent_directories.py	Refactored button press handling with `@on` decorators for targeted event handling and added focus management
menu_bar/menu_bar.py	Updated `MenuButton` to use `on_button_pressed` with event filtering and added dropdown closing before opening modals
directory_picker.py	New custom directory picker widget with DirectoryTree, breadcrumb navigation, and keyboard shortcuts
tui_app.py	Integrated custom DirectoryPicker, added error handling, and removed third-party fspicker dependency
requirements.txt	Removed textual-fspicker dependency

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

projects/rocprofiler-compute/src/rocprof_compute_tui/widgets/recent_directories.py

projects/rocprofiler-compute/src/rocprof_compute_tui/widgets/menu_bar/menu_bar.py

projects/rocprofiler-compute/src/rocprof_compute_tui/widgets/directory_picker.py

projects/rocprofiler-compute/src/rocprof_compute_tui/tui_app.py

…ecent_directories.py Co-authored-by: Copilot <[email protected]>

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 15 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

Copilot · 2025-11-27T16:15:10Z

projects/rocprofiler-compute/src/rocprof_compute_tui/widgets/recent_directories.py

+        parent = self.parent
+        list_view = None
+        while parent is not None:
+            from textual.widgets import ListView


The ListView import is redundant here since it's already imported at the module level (line 29). The local import inside the while loop is unnecessary and impacts code clarity.

Suggested change

from textual.widgets import ListView

Copilot · 2025-11-27T16:15:10Z

projects/rocprofiler-compute/src/rocprof_compute_tui/widgets/directory_picker.py

+            event.stop()
+            self.dismiss(None)
+
+    def on_key(self, event) -> None:  # noqa: ANN001


Missing type annotation for the event parameter. Consider adding proper type hints from textual.events for better code maintainability. For example: event: events.Key

Copilot · 2025-11-27T16:15:11Z

projects/rocprofiler-compute/src/rocprof_compute_tui/utils/tui_utils.py

+            return
+
+        # Detect if we are on UI thread
+        in_ui_thread = threading.get_ident() == app._thread_id


Accessing the private _thread_id attribute is fragile and may break if the Textual framework changes its internals. Consider using a public API or documented approach for thread detection, or encapsulate this check in a helper method for consistency with the same pattern used in main_view.py.

Copilot · 2025-11-27T16:15:11Z

projects/rocprofiler-compute/src/rocprof_compute_tui/widgets/menu_bar/menu_bar.py

+        ):
+            self.hide()
+
+    def is_ancestor_of(self, widget) -> bool:  # noqa: ANN001


Missing type annotation for the widget parameter. Consider adding proper type hints for better code maintainability. For example: widget: Widget | None

Copilot · 2025-11-27T16:15:11Z

projects/rocprofiler-compute/src/rocprof_compute_tui/tui_app.py

+
+                self.notify("Running analysis…", severity="information")
+                self.main_view.run_analysis()
+                self.notify("Analysis completed", severity="information")


The notification "Analysis completed" at line 169 may be misleading. At this point, the analysis has just been started (via run_analysis()), not completed. The actual completion is notified in _analysis_success. Consider changing this to "Starting analysis..." or removing it to avoid confusion.

Suggested change

self.notify("Analysis completed", severity="information")

Copilot · 2025-11-27T16:15:13Z

projects/rocprofiler-compute/src/rocprof_compute_tui/tui_app.py

+        """Open directory picker and handle selection."""
+        # Close the dropdown first
+        dropdown = self.query_one("#file-dropdown", DropdownMenu)
+        dropdown.add_class("hidden")


The dropdown.add_class("hidden") approach is inconsistent with the new visibility management system. The DropdownMenu now has show() and hide() methods that properly manage all visibility-related styles. Using add_class("hidden") here could conflict with the state managed by InstantMenuButton.is_open. Consider using dropdown.hide() or just setting menu_button.is_open = False which should handle the dropdown automatically.

Suggested change

dropdown.add_class("hidden")

dropdown.hide()

Copilot · 2025-11-27T16:15:13Z

projects/rocprofiler-compute/src/rocprof_compute_tui/widgets/menu_bar/menu_bar.py


        self.app.push_screen(
-            RecentDirectoriesScreen(self.app.recent_dirs), on_recent_selected
+            RecentDirectoriesScreen(self.app.recent_dirs),


The RecentDirectoriesScreen is instantiated without passing the current_dir parameter, which means it will always default to None. The screen's on_mount method tries to match current_dir with recent directories to pre-select it (lines 117-122 in recent_directories.py). Consider passing self.app.main_view.selected_path as the current_dir argument to enable proper pre-selection of the currently selected directory.

Suggested change

RecentDirectoriesScreen(self.app.recent_dirs),

RecentDirectoriesScreen(self.app.recent_dirs, current_dir=self.app.main_view.selected_path),

Copilot · 2025-11-27T16:15:13Z

projects/rocprofiler-compute/src/rocprof_compute_tui/widgets/menu_button.py

+        try:
+            dropdown = self.app.query_one(f"#{self.menu_id}")
+        except Exception:
+            return


Bare except Exception is too broad. Consider catching specific exceptions (e.g., LookupError) or at least documenting what failures are expected here. The empty return on exception might silently hide widget configuration issues.

Copilot · 2025-11-27T16:15:14Z

projects/rocprofiler-compute/src/rocprof_compute_tui/widgets/menu_bar/menu_bar.py

+    # -------------------------------------------------------------------------
+    # Click outside to close menu
+    # -------------------------------------------------------------------------
+    def on_click(self, event) -> None:  # noqa: ANN001


Missing type annotation for the event parameter. Consider adding proper type hints from textual.events for better code maintainability. For example: event: events.Click

Copilot · 2025-11-27T16:15:14Z

projects/rocprofiler-compute/src/rocprof_compute_tui/widgets/recent_directories.py

+        if self.current_dir:
+            try:
+                index = self.recent_dirs.index(self.current_dir)
+            except ValueError:


'except' clause does nothing but pass and there is no explanatory comment.

Suggested change

except ValueError:

except ValueError:

# If current_dir is not in recent_dirs, keep default index (0)

feizheng10 · 2025-11-28T04:20:57Z

projects/rocprofiler-compute/requirements.txt

 tabulate
 textual
 textual_plotext
-textual-fspicker>=0.4.3


Why this got removed?

xuchen-amd added 3 commits November 19, 2025 10:36

rever previous changes.

742e572

revert previous changes

a841a3a

use custom file picker

54bf0ed

Copilot AI review requested due to automatic review settings November 19, 2025 20:23

xuchen-amd requested a review from a team as a code owner November 19, 2025 20:23

github-actions bot added the project: rocprofiler-compute label Nov 19, 2025

Copilot started reviewing on behalf of xuchen-amd November 19, 2025 20:24 View session

Copilot finished reviewing on behalf of xuchen-amd November 19, 2025 20:25

Copilot AI reviewed Nov 19, 2025

View reviewed changes

systems-assistant bot added the organization: ROCm label Nov 19, 2025

xuchen-amd and others added 6 commits November 21, 2025 15:27

Update projects/rocprofiler-compute/src/rocprof_compute_tui/widgets/r…

4c6f2fe

…ecent_directories.py Co-authored-by: Copilot <[email protected]>

fix redundancy

e09bcb6

fix css

6b76c49

improve tui notify messages

30089d2

fix button press WIP

c6e3787

fix recent directories selection

af2bdd5

xuchen-amd requested a review from Copilot November 27, 2025 16:09

Copilot started reviewing on behalf of xuchen-amd November 27, 2025 16:10 View session

Copilot finished reviewing on behalf of xuchen-amd November 27, 2025 16:12

Copilot AI reviewed Nov 27, 2025

View reviewed changes

feizheng10 reviewed Nov 28, 2025

View reviewed changes

	RecentDirectoriesScreen(self.app.recent_dirs),
	RecentDirectoriesScreen(self.app.recent_dirs, current_dir=self.app.main_view.selected_path),

	except ValueError:
	except ValueError:
	# If current_dir is not in recent_dirs, keep default index (0)

[rocprofiler-compute][tui] menu bar lag fix #1942

Are you sure you want to change the base?

[rocprofiler-compute][tui] menu bar lag fix #1942

Uh oh!

Conversation

xuchen-amd commented Nov 19, 2025

Motivation

Technical Details

Test Plan

Test Result

Submission Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

feizheng10 Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants