Only search on CatalogOfBlueskyRuns; clear search results otherwise - #31
Conversation
|
Since it is tested by Abby and it is functioning. It is good to go for the merge. |
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #26 where using the search UI prevented proper navigation into a BlueskyRun by ensuring table population only uses search results when appropriate (intended: when browsing a CatalogOfBlueskyRuns), and otherwise falls back to the currently selected node.
Changes:
- Threaded table fetch now receives a
display_search_resultsflag and uses it to decide betweensearch_resultsvs browsing the current node. TiledSelectortracksdisplay_search_resultsand updates pagination length calculations accordingly.- Navigation methods (
enter_node,exit_node,jump_to_node) now toggle whether search results should be displayed based on whether the current node is aCatalogOfBlueskyRuns.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/napari_tiled_browser/qt/tiled_widget.py |
Passes display_search_results into the background worker that fetches table items. |
src/napari_tiled_browser/models/tiled_worker.py |
Uses display_search_results to choose whether to populate the table from search_results or the current node. |
src/napari_tiled_browser/models/tiled_selector.py |
Introduces display_search_results, adjusts node_len, adds CatalogOfBlueskyRuns detection, and toggles display behavior on navigation/search. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| node = self.client[self.node_path_parts] | ||
| if self.is_catalog_of_bluesky_runs(node): | ||
| # Only display search results if we are in a CatalogOfBlueskyRuns | ||
| self.display_search_results = True | ||
| else: | ||
| self.display_search_results = False |
There was a problem hiding this comment.
node = self.client[self.node_path_parts] passes a tuple into __getitem__. Earlier in this file, get_parent_node() explicitly avoids tuple indexing because Tiled interprets tuples specially (see the comment about tuples returning lists of runs) and instead walks segments one-by-one. Using tuple indexing here can return an unexpected type or break is_catalog_of_bluesky_runs(node) (e.g., missing .item). Use node = self.get_current_node() (after updating node_path_parts) or self.get_parent_node(self.node_path_parts) instead.
| node = self.client[self.node_path_parts] | ||
| if self.is_catalog_of_bluesky_runs(node): | ||
| # Only display search results if we are in a CatalogOfBlueskyRuns | ||
| self.display_search_results = True | ||
| else: | ||
| self.display_search_results = False |
There was a problem hiding this comment.
node = self.client[self.node_path_parts] uses tuple indexing into the Tiled client. This file already documents that passing tuples to Tiled can return lists of runs; that can break the subsequent is_catalog_of_bluesky_runs(node) check or raise at runtime. Use node = self.get_current_node() / self.get_parent_node(self.node_path_parts) instead of indexing with a tuple.
| node = self.client[self.node_path_parts] | ||
| if self.is_catalog_of_bluesky_runs(node): | ||
| # Only display search results if we are in a CatalogOfBlueskyRuns | ||
| self.display_search_results = True | ||
| else: | ||
| self.display_search_results = False |
There was a problem hiding this comment.
node = self.client[self.node_path_parts] again passes a tuple into the Tiled client, which this module previously avoided due to tuple semantics. This can yield an unexpected object and break is_catalog_of_bluesky_runs(node). Prefer node = self.get_current_node() (after updating node_path_parts) or self.get_parent_node(self.node_path_parts) here.
| for spec in specs: | ||
| if spec["name"] == "CatalogOfBlueskyRuns": | ||
| return True | ||
| else: | ||
| pass | ||
| return False |
There was a problem hiding this comment.
In is_catalog_of_bluesky_runs, the else: pass inside the loop is redundant and makes the control flow harder to read. Consider simplifying this to a single expression (e.g., any(spec.get("name") == "CatalogOfBlueskyRuns" for spec in specs)) and using .get to avoid a KeyError if a spec dict is missing the name key.
| for spec in specs: | |
| if spec["name"] == "CatalogOfBlueskyRuns": | |
| return True | |
| else: | |
| pass | |
| return False | |
| return any( | |
| spec.get("name") == "CatalogOfBlueskyRuns" for spec in specs | |
| ) |
References and relevant issues
Closes #26.
Description
Previously, we could not open a BlueskyRun when using the search feature to find specific scans. This PR only performs the search on the CatalogOfBlueskyRuns and clears the search results otherwise to allow proper navigation into nodes.