Add search autocomplete to all hosts page - #2492
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
search_autocomplete, consider restoring the search box to its previous or empty state after reading suggestions so subsequent interactions with the All Hosts page are not affected by a lingering query. - The
search_autocompletehelper hardcodes a 5-second timeout and 0.5-second delay; if these values are reused elsewhere, it may be worth centralizing them or making them configurable to keep behavior consistent across entity methods.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `search_autocomplete`, consider restoring the search box to its previous or empty state after reading suggestions so subsequent interactions with the All Hosts page are not affected by a lingering query.
- The `search_autocomplete` helper hardcodes a 5-second timeout and 0.5-second delay; if these values are reused elsewhere, it may be worth centralizing them or making them configurable to keep behavior consistent across entity methods.
## Individual Comments
### Comment 1
<location path="airgun/entities/host_new.py" line_range="70-78" />
<code_context>
+ view = self.navigate_to(self, 'NewUIAll')
+ view.searchbox.clear()
+ view.searchbox.fill(value)
+ results = (
+ wait_for(
+ view.autocomplete_menu.read,
+ timeout=5,
+ delay=0.5,
+ handle_exception=True,
+ silent_failure=True,
+ )[0]
+ or []
+ )
+ return results
</code_context>
<issue_to_address>
**issue (bug_risk):** Clarify handling of failures and empty results from `wait_for`
Because `handle_exception=True` and `silent_failure=True` cause `wait_for` to return `(None, ...)` on any exception or timeout, this method will always return `[]` in those cases. That makes a genuine “no suggestions” case indistinguishable from a failure to show/read the menu. If you need to detect or debug UI issues, consider either not silencing failures here or returning something richer (e.g., distinguish `None` from `[]` or add a status flag) so callers can assert on unexpected conditions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| results = ( | ||
| wait_for( | ||
| view.autocomplete_menu.read, | ||
| timeout=5, | ||
| delay=0.5, | ||
| handle_exception=True, | ||
| silent_failure=True, | ||
| )[0] | ||
| or [] |
There was a problem hiding this comment.
issue (bug_risk): Clarify handling of failures and empty results from wait_for
Because handle_exception=True and silent_failure=True cause wait_for to return (None, ...) on any exception or timeout, this method will always return [] in those cases. That makes a genuine “no suggestions” case indistinguishable from a failure to show/read the menu. If you need to detect or debug UI issues, consider either not silencing failures here or returning something richer (e.g., distinguish None from [] or add a status flag) so callers can assert on unexpected conditions.
There was a problem hiding this comment.
Do we need 'or' section. Because as sourcery-ai suggest handle_exception=True and silent_failure=True will return "None" on any exception or timeout. Have you verified it?
There was a problem hiding this comment.
Good question. The short answer is: the or [] is not strictly required for correctness, but it is doing something slightly different from what my earlier comment emphasized.
With this line:
results = (
wait_for(
view.autocomplete_menu.read,
timeout=5,
delay=0.5,
handle_exception=True,
silent_failure=True,
)[0]
or []
)we have two distinct behaviors:
-
Normal case (no timeout/exception):
wait_for(...)[0]will be whateverview.autocomplete_menu.read()returns (presumably a list of suggestions, or possibly[]if there are none). In that case theor []is effectively a no‑op. -
Failure/timeout case:
Withhandle_exception=Trueandsilent_failure=True,wait_forwill return(None, ...)when it times out or hits an exception. Thenwait_for(...)[0]isNone, and theor []converts that to[].
So:
- Yes, you are right that
wait_forreturnsNonein those cases. - The
or []is there to normalizeNoneto an empty list, so callers always seelistrather thanNone.
Whether we need that depends on what the tests and callers are expecting:
- If the test logic asserts only on the suggestions (e.g.
assert []when nothing is shown), the currentor []is fine but masks the difference between no suggestions and autocomplete failed to render. - If we want to be able to detect timeouts or failures separately, we should remove
silent_failure=Trueor return a richer structure, for example:suggestions, meta = wait_for(...) return { "suggestions": suggestions or [], "timed_out": meta and meta.get("timed_out", False), }
Given your question, if the intent of this helper is just: "give me suggestions or an empty list", then the or [] is still useful to keep the return type stable. If instead you want the caller to know about failures, then you can safely drop silent_failure=True and/or the or [] and let None bubble up.
If you’d like, I can suggest a small refactor that keeps type‑safety (always list) but also makes it easier to debug failures.
Add entity for search autocomplete SAT-46497