fix(ui): stable stack layout prevents search focus reset on first keystroke - #89
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughStabilizes the Iced search bar widget tree by always rendering ChangesSearch bar focus bug fix
Sequence Diagram(s) sequenceDiagram
participant App as HonkHonk
participant View as view_search_bar()
participant Iced as Iced_reconciler
participant TextInput as text_input
App->>View: render search bar (always stack![input, overlay])
View->>Iced: emit stable widget tree
Iced->>TextInput: reuse widget instance (preserve focus)
Note over App: Keyboard subscription maps Escape -> Message::EscapePressed
App->>App: on Message::EscapePressed -> update: close menu / consume blur / clear query
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/app.rs (1)
468-479: ⚡ Quick winConsider adding unit tests for the new Escape key behavior.
The new
EscapePressedmessage andsearch_had_focusstate machine lack automated test coverage. While the PR objectives mention manual testing, unit tests would prevent regressions and document the intended behavior.Suggested test cases:
- First Escape sets
search_had_focusto false when true- Second Escape clears
search_querywhen non-empty- Escape closes context menu when open
- SearchChanged sets
search_had_focusto true📋 Example test structure
#[test] fn escape_pressed_consumes_search_focus_flag() { let mut app = HonkHonk::new_for_test(); let _ = app.update(Message::SearchChanged("honk".into())); assert!(app.search_had_focus); let _ = app.update(Message::EscapePressed); assert!(!app.search_had_focus); assert_eq!(app.search_query(), "honk"); // query not cleared yet } #[test] fn escape_pressed_clears_query_after_focus_consumed() { let mut app = HonkHonk::new_for_test(); let _ = app.update(Message::SearchChanged("honk".into())); let _ = app.update(Message::EscapePressed); // consume focus let _ = app.update(Message::EscapePressed); // clear query assert_eq!(app.search_query(), ""); } #[test] fn escape_pressed_closes_context_menu() { let mut app = HonkHonk::new_for_test(); let _ = app.update(Message::OpenContextMenu("test-id".into())); let _ = app.update(Message::EscapePressed); assert!(app.context_menu().is_none()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app.rs` around lines 468 - 479, Add unit tests covering the Escape key behavior around Message::EscapePressed and the search focus state machine: create tests that use HonkHonk::new_for_test() and call app.update(Message::SearchChanged(...)) to assert search_had_focus becomes true, then call app.update(Message::EscapePressed) to assert search_had_focus becomes false without clearing search_query() on the first press, then call app.update(Message::EscapePressed) again to assert search_query() is cleared; add a separate test that opens the context menu via app.update(Message::OpenContextMenu(...)) and then app.update(Message::EscapePressed) to assert context_menu() is None; place tests alongside existing app unit tests and make sure to use the public accessors (search_query(), context_menu()) and update() to drive state transitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app.rs`:
- Around line 468-479: The EscapePressed handler currently clears
self.context_menu and also modifies self.search_had_focus, which consumes the
blur step; change the logic in Message::EscapePressed so that if a context menu
is open (self.context_menu.is_some()), you clear self.context_menu and
self.context_menu_pos and immediately return Task::none() without touching
self.search_had_focus or self.search_query, preserving the two-Escape semantics;
otherwise keep the existing blur/clear behavior for search_had_focus and
search_query.
---
Nitpick comments:
In `@src/app.rs`:
- Around line 468-479: Add unit tests covering the Escape key behavior around
Message::EscapePressed and the search focus state machine: create tests that use
HonkHonk::new_for_test() and call app.update(Message::SearchChanged(...)) to
assert search_had_focus becomes true, then call
app.update(Message::EscapePressed) to assert search_had_focus becomes false
without clearing search_query() on the first press, then call
app.update(Message::EscapePressed) again to assert search_query() is cleared;
add a separate test that opens the context menu via
app.update(Message::OpenContextMenu(...)) and then
app.update(Message::EscapePressed) to assert context_menu() is None; place tests
alongside existing app unit tests and make sure to use the public accessors
(search_query(), context_menu()) and update() to drive state transitions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
5 tasks
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.
Closes #87
Summary
view_search_bar()previously returnedcontainer(input)for empty query andstack![input, overlay]for non-empty query — a structural widget tree change that caused Iced's reconciler to treat thetext_inputas a new instance on first keystroke, resetting focusstack![input, overlay]regardless of query state; overlay isrow![].into()(no hit area, no render cost) when empty, and the ✕ clear button container when non-emptytext_inputidentity and focusTest Plan
cargo test— 178 tests passcargo clippy -- -D warnings— zero warningscargo fmt -- --check— cleanSummary by CodeRabbit
Bug Fixes
New Features
Documentation
Tests