Skip to content

feat: advanced seach#2234

Merged
Predidit merged 2 commits into
mainfrom
feat-search
Jun 17, 2026
Merged

feat: advanced seach#2234
Predidit merged 2 commits into
mainfrom
feat-search

Conversation

@Predidit

Copy link
Copy Markdown
Owner

No description provided.

Comment thread lib/pages/search/search_controller.dart Outdated
rankRange: filterState.rankRange,
scoreRange: filterState.scoreRange,
weekdays: filterState.weekdays);
if (filterState.weekdays.isNotEmpty) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WARNING: Client-side weekday filtering breaks pagination.

The request uses offset: bangumiList.length (line 94), but here matching items are dropped from result after the fetch. Because bangumiList then grows by fewer than the 20 items the API returned, the next page's offset is computed from the filtered count rather than the number of records actually consumed from the server. This causes the same records to be re-requested (duplicates) and/or records to be skipped on subsequent "load more" calls.

Additionally, when a page yields zero matches, bangumiList stays empty and isTimeOut becomes true even though further pages exist, and the bangumiList.length >= 20 guard in scrollListener can prevent loading more. Consider tracking the raw fetched offset separately from the filtered list length, or relying solely on the server-side air_weekday filter.


Reply with @kilocode-bot fix it to have Kilo Code address this issue.

Comment thread lib/pages/search/search_page.dart Outdated
),
if (draft.scoreRange?.isValid == true)
RangeSlider(
values: RangeValues(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WARNING: RangeSlider can assert/crash when the score range is outside 0..10.

scoreRange is parsed directly from the query text (e.g. typing score:0..100 is accepted by _scoreRegExp) and passed unchanged into initialState. Opening the filter sheet then builds this RangeSlider with values outside its min: 0 / max: 10 bounds, which violates Flutter's assertion values.start >= min && values.end <= max and throws. Clamp draft.scoreRange values into [0, 10] before constructing RangeValues.


Reply with @kilocode-bot fix it to have Kilo Code address this issue.

Comment thread lib/pages/search/search_page.dart Outdated
),
if (draft.rankRange?.isValid == true)
RangeSlider(
values: RangeValues(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WARNING: Same out-of-bounds RangeSlider risk as the score slider.

rankRange is parsed from arbitrary user input (e.g. rank:0..50000), so min can be 0 (below the slider min: 1) and max can exceed max: 10000. Constructing RangeValues outside the slider bounds triggers a Flutter assertion failure when the sheet opens. Clamp the values into [1, 10000] before building RangeValues.


Reply with @kilocode-bot fix it to have Kilo Code address this issue.

@kilo-code-bot

kilo-code-bot Bot commented Jun 17, 2026

Copy link
Copy Markdown

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Overview

Severity Count
CRITICAL 0
WARNING 0
SUGGESTION 0

The latest commit (a40214eAddress advanced search review issues) resolves all 3 issues from the previous review.

Resolved Issues (from previous review)
File Issue Resolution
lib/pages/search/search_controller.dart Client-side weekday filtering desynced pagination offset Client-side filtering removed; weekdays now passed to the server-side filter (bangumi_api.dart sets filter["air_weekday"]), so offset stays consistent with bangumiList.length. ✅
lib/pages/search/search_page.dart Score RangeSlider could exceed 0..10 and crash on assertion New _buildScoreRangeSlider uses _safeRangeValues to clamp values into 0..10. ✅
lib/pages/search/search_page.dart Rank RangeSlider could exceed 1..10000 and crash on assertion New _buildRankRangeSlider uses _safeRangeValues to clamp values into 1..10000. ✅

The new _safeRangeValues helper clamps both endpoints into [min, max] and swaps them when inverted, robustly preventing the out-of-bounds and start > end assertion failures.

Other Observations (not in diff)
  • The removal of persisted searchNotShowWatchedBangumis / searchNotShowAbandonedBangumis settings means these filters reset each session. If intentional this is fine; otherwise the in-memory-only behavior is a minor regression. (Unchanged since prior review.)
Files Reviewed (incremental: 2 files)
  • lib/pages/search/search_controller.dart - 0 issues (weekday filtering fix verified)
  • lib/pages/search/search_page.dart - 0 issues (slider clamping fix verified)
Previous Review Summary (commit 9969fd4)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 9969fd4)

Status: 3 Issues Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 0
WARNING 3
SUGGESTION 0

This PR adds an advanced search workbench (tags, sort, season/date, rank/score ranges, weekday) with a new SearchFilterState model and query (de)serialization in SearchParser, plus accompanying tests. The parser/serializer and buildBangumiSearchParams logic look solid and are well covered by tests. The issues below relate to UI robustness and pagination.

Issue Details (click to expand)

WARNING

File Line Issue
lib/pages/search/search_controller.dart 100 Client-side weekday filtering after a paged fetch desyncs offset (which uses the filtered bangumiList.length), causing duplicate/skipped results and premature isTimeOut / blocked "load more".
lib/pages/search/search_page.dart 774 Score RangeSlider is built from query-parsed scoreRange that can exceed 0..10 (e.g. score:0..100), violating the slider's bounds assertion and crashing when the sheet opens.
lib/pages/search/search_page.dart 811 Rank RangeSlider can receive values outside 1..10000 (e.g. rank:0..50000) from parsed input, triggering the same out-of-bounds assertion failure.
Other Observations (not in diff)
  • The removal of persisted searchNotShowWatchedBangumis / searchNotShowAbandonedBangumis settings (in collect_repository.dart and settings_keys.dart) means these filters now reset to false on each session. If that loss of persistence is intentional it is fine; otherwise the in-memory-only behavior is a regression.
Files Reviewed (11 files)
  • .vscode/launch.json - 0 issues
  • lib/pages/search/search_controller.dart - 1 issue
  • lib/pages/search/search_controller.g.dart - 0 issues (generated)
  • lib/pages/search/search_page.dart - 2 issues
  • lib/repositories/collect_repository.dart - 0 issues
  • lib/request/apis/bangumi_api.dart - 0 issues
  • lib/services/storage/settings_keys.dart - 0 issues
  • lib/utils/date_time.dart - 0 issues
  • lib/utils/search_parser.dart - 0 issues
  • test/bangumi_search_params_test.dart - 0 issues
  • test/search_parser_test.dart - 0 issues

Fix these issues in Kilo Cloud


Reviewed by claude-4.8-opus-20260528 · 240,338 tokens

@Predidit Predidit merged commit b472fd0 into main Jun 17, 2026
9 checks passed
@Predidit Predidit deleted the feat-search branch June 18, 2026 05:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant