feat(search-list): optional trailing widget beside the search field [REQ-177] - #209
Conversation
…REQ-177] IntegratedSearchTextField gains an optional, null-defaulted 'trailing' widget rendered after the search Card (e.g. a filter button). SearchList exposes it as 'searchBarTrailing' and threads it into both the SearchBarInBody and SearchWithAppBar call-sites. Null default means every existing consumer's widget tree is byte-identical. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Vipin9821
left a comment
There was a problem hiding this comment.
Changes requested — PR #209
Optional trailing / searchBarTrailing is additive and null-defaulted. Grep: the only other IntegratedSearchTextField call site (integrated_search_bar.dart) does not pass trailing → existing AppBar search is unchanged.
Production wiring to both SearchList body paths looks correct.
Blocking
- Null-path test does not prove the tree is unchanged. It looks for
Key('search-trailing')that is never in the tree — alwaysfindsNothing. A wrappingRowon the null path would still pass. - Positive test does not prove “beside the field”.
find.byType(Row)isfindsWidgetsbecausesuffixIconis already aRow. Trailing stuffed into the suffix would still pass. SearchList.searchBarTrailingis untested. PR claims bothSearchBarInBodyandSearchWithAppBarare covered via B7 — the test never pumpsSearchList.
Ran
flutter test test/integrated_search_textfield_test.dart → 2/2 passed (assertions are too weak to catch a layout regression).
Checklist
- Assert null path:
Paddingchild isCard, notRow - Assert supplied path: trailing is a sibling of
Expanded(Card), not insidesuffixIcon - Pump
SearchListwithsearchBarTrailingfor both search types
/cc @rgulati-f2k
…e + gap [REQ-177] The null-path test only asserted a Key that is never mounted regardless of implementation (always findsNothing), and the trailing-path test's find.byType(Row) also matches the suffixIcon Row inside the TextField's own decoration, so neither could actually fail. Rewrote both to assert the real structural invariant (Padding.child is the Card directly, or a Row of [Expanded(Card), trailing]). Added search_list_test.dart covering the two untested SearchList call sites (SearchBarInBody, SearchWithAppBar) that wire searchBarTrailing through. Also added an 8px gap between the search field and trailing widget so it doesn't sit flush against the Card edge. Addresses Vipin's CHANGES_REQUESTED review on PR #209. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
All 4 review findings addressed in commit 2b40c46:
All 4 threads replied + resolved. @Vipin9821 could you take another look when you have a moment? |
singhtaranjeet
left a comment
There was a problem hiding this comment.
Previously flagged (Vipin9821) — verified against current diff, not just the "Fixed" replies:
| Thread | Verdict |
|---|---|
| Null-path assertion on an unmountable key | Fixed — now asserts padding.child isA<Card>() via a stable Key('integrated-search-textfield-padding') |
find.byType(Row) also matched the suffixIcon's own Row |
Fixed — asserts the Padding's child isA<Row>() with Expanded(Card) first and the trailing key last |
Untested SearchList call sites |
Fixed — search_list_test.dart pumps both SearchBarInBody and SearchWithAppBar |
| Trailing flush against the Card | Fixed — SizedBox(width: 8) gap added |
Reran independently rather than trusting the description: flutter analyze on the changed files is clean apart from the same pre-existing withOpacity info already called out; flutter test on both new test files is 4/4 green. The null-default path is structurally unchanged for every existing consumer (same Padding → Card → TextField; the only addition is the new key, which is a LocalKey scoped inside each widget's own subtree, so there's no sibling-collision risk across multiple instances).
🟡 One cleanup before merging: several new comments narrate the review itself and name the reviewer instead of just stating the technical reasoning — test/integrated_search_textfield_test.dart:22 and :47, and test/search_list_test.dart:1 ("...untested since #209 was opened"). That's review history, not something a future reader of this file needs — it belongs in the commit message, not the source. The technical explanation in each is good and should stay; just drop the [REQ-177 rework, Vipin]-style tag. Same applies to the [REQ-177 rework] tags in integrated_search_textfield.dart:145 and :155.
✅ Good to merge. The feature is correctly additive (null-default, byte-identical for existing consumers), all four prior review findings are genuinely fixed, and coverage is solid.
|
Gentle re-ping (2026-09-12): this PR is approved by 2 reviewers (Vipin9821, abhinavdroid) and mergeable — it's the sole remaining blocker on |
singhtaranjeet
left a comment
There was a problem hiding this comment.
Re-verified in a clean worktree at 2b40c46 (current HEAD, unchanged since round 2):
flutter pub get— resolves cleanly,./exampleincluded. The "pre-existing,./examplesub-workspace can't resolvefa_flutter_core" note in the description doesn't reproduce; worth dropping that caveat if it comes up again.flutter analyzeon the changed files — 1 issue, the same pre-existingwithOpacitydeprecation info on an untouched line. Clean otherwise.flutter test test/integrated_search_textfield_test.dart test/search_list_test.dart— 4/4 pass.- Read the actual diff:
trailing/searchBarTrailingare additive and null-defaulted, the null path returns the sameCardstructure as before (the new stableKeyon the outerPaddingdoesn't change anything visible), and the non-null path adds the 8px gap Vipin asked for. Sound.
The two open threads I left on test/integrated_search_textfield_test.dart:22 and test/search_list_test.dart:1 (drop the [REQ-177 rework, Vipin] tag, keep the technical reasoning) are still exactly as I left them — no commit since round 2 touched either line.
🟡 Does not meet our standards — purely on those two open comment-tag threads, both a one-line edit each. Everything else (implementation, tests, analyzer, blast radius on existing IntegratedSearchTextField/SearchList consumers) checks out clean; this merges the moment those two lines are trimmed. Flagging as blocking mainly because it's been open 10+ days with zero response and this is the sole thing blocking fa_flutter_gt #4540's rebase.
singhtaranjeet (2026-09-09 re-review, round 2): commit-message material ([REQ-177 rework, Vipin] tag, "untested since #209 was opened" reference) doesn't belong in source comments a future reader has to parse. Drop both, keep the technical reasoning for each assertion. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Both open threads from the 2026-09-14 re-review addressed in commit ff4a6d7 — dropped the [REQ-177 rework, Vipin] tag (and the "untested since #209 was opened" aside on the second) from the two test-file comments, kept the underlying technical reasoning. Per that same review: "this merges the moment those two lines are trimmed" — everything else (implementation, tests, analyzer, blast radius) was already confirmed clean. @singhtaranjeet could you re-approve when you have a moment? This is also the sole remaining blocker on fa_flutter_gt #4540 (REQ-177)'s rebase. |
singhtaranjeet
left a comment
There was a problem hiding this comment.
@rgulati-f2k still you are adding unnecessary comments
Removes the [REQ-177]-tagged explanatory comments on IntegratedSearchTextField.trailing, the Padding key, and the trailing-gap SizedBox per review — the code is self-explanatory without them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
All 3 findings from the 2026-09-16 review addressed in commit 3dc1488 — removed the All 3 threads replied + resolved. @singhtaranjeet could you re-approve when you have a moment? Still the sole remaining blocker on |
REQ-177 — Enhanced Beat & Route Selection for Hierarchical Retailing (2 of 3 PRs — shared UI kit, TECH-1)
What was built
SearchList(fa_flutter_ui_kit) renders its search field internally viaIntegratedSearchTextField,which has no slot beside the field —
actionWidgetlands in the app bar instead. This PR adds anoptional, null-defaulted trailing slot so a consumer (the
fa_flutter_gtPR in this set) can place afilter button beside the search bar without composing the screen locally.
IntegratedSearchTextFieldgainsfinal Widget? trailing— when non-null, the existing searchCardiswrapped in a
Rowwith the trailing widget after it.SearchListgainsfinal Widget? searchBarTrailing, threaded into bothIntegratedSearchTextFieldcall-sites (
SearchBarInBodyand theSearchWithAppBarsliver header).path returns exactly the prior
Padding(child: Card(...))structure).Traceability (§5 B)
IntegratedSearchTextField.trailinglib/src/modules/common/search_list/integrated_search_textfield.dartintegrated_search_textfield_test.dart(2 tests)SearchList.searchBarTrailinglib/src/modules/common/search_list/search_list.darttest/integrated_search_textfield_test.dartBuild/verify
flutter analyze --no-pubon changed files — 0 errors (1 pre-existingwithOpacitydeprecation info, untouched line).flutter test --no-pub test/integrated_search_textfield_test.dart— 2/2 passed.dart format— clean.flutter pub get/analyze/testwithout--no-pubfails to resolve./example's ownsub-workspace (
Could not find a file named "pubspec.yaml" in …fa_flutter_core.git@5208b4b…) —pre-existing, reproduced identically on unmodified
main(stashed this PR's diff and re-ran); notcaused by this change.
Blast radius
Both new fields are additive and null-defaulted. Searched the package for other
IntegratedSearchTextField/SearchListcall sites — the null-default keeps their rendered output identical.Sequencing
Part of a 3-PR deploy-ordered set:
FA_GT_APPAPI(A, independent, ADO PR) · this PR (B) ·fa_flutter_gt(C, depends on B —
pubspec.yamltracks this package atref: main, unpinned, so C won't compile the newfilter drawer until B merges).
Please Squash or Rebase-and-merge (no merge commit) so this task stays a single revertable unit.
Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com