Repository navigation
feat(search): jump to the first message of a picked date - #215
Conversation
iebb
left a comment
iebb
left a comment
There was a problem hiding this comment.
Reviewed 668ab3c against current master e8b049f in a credential-free integration snapshot. This is a legitimate calendar-navigation improvement, not spam. The two supplied tests and localization/generated-asset checks pass, but a faithful delayed UI/session boundary fails: pick a date with getChatMessageByDate held pending, close and reopen the same search before the next frame, then resolve the old request. The still-mounted reused header calls onJumpToMessage(1200); expected no landing from the closed session. context.mounted and _disposed cannot distinguish request/session ownership. Cover close/reopen, superseded picks, and disposal with held responses and cancel stale landing/toast effects.
The new showDatePicker entry also renders the built-in Material picker, contrary to AGENTS.md's no-Material/Cupertino UI rule. Please use an owned project date-picker surface and verify phone/desktop entry points rather than adding this framework UI.
Finally reconcile the advertised first-message-of-the-picked-day behavior with the actual lookup. TDLib's getChatMessageByDate contract resolves the latest message at or before the supplied timestamp. Local midnight can therefore anchor on the previous day's message, or return 404 even when messages exist later on the picked day. The current fixture accepts a 1970 timestamp without checking the requested day. Choose and document the intended boundary/empty-day behavior, then cover previous-day-only, same-day-after-midnight, empty-day and timezone cases. This needs a coherent revision, not a speculative date/history workaround in the automation.
No Telegram account, credentials, native/on-device navigation, feature edit, merge, or publication was used in this review. Leaving the PR open for changes.
| ); | ||
| if (date == null || !context.mounted) return; | ||
| final message = await controller.messageAtDate(date); | ||
| if (!context.mounted) return; |
There was a problem hiding this comment.
[P1] Invalidate the date landing when its search session is replaced. The production button/header repro holds getChatMessageByDate, calls controller.close(); controller.open() before a frame, then completes the old response. The reused context is mounted and the callback still lands message 1200 (expected no old-session landing). Guard request/session ownership, not just mounted/disposed; include late responses and superseded picks.
|
|
||
| Future<void> _pick(BuildContext context) async { | ||
| final now = DateTime.now(); | ||
| final date = await showDatePicker( |
There was a problem hiding this comment.
[P2] Replace the newly added built-in Material date picker with a project-owned picker. showDatePicker constructs Material's dialog/calendar; this is new framework UI, not merely an import or existing routing primitive, and violates the repository's explicit UI rule.
668ab3c to
0469eb5
Compare
ChangeAmends the calendar "jump to date" per the review round against 668ab3c. Three findings, all addressed with a single coherent revision of the pick → lookup → land path: 1. Session-ownership race (P1, chat_message_search_bar.dart:155). 2. Material picker (P2, chat_message_search_bar.dart:147). 3. Day-boundary semantics. The advertised behavior is now defined and pinned: jump to the first message of the picked local day. TDLib's Coverage: The rebased head (0469eb5) sits on current master (066c2d6 — the #213/#214 merges) with no conflicts. Telegram API Terms classification
Reason and affected Telegram behavior: unchanged from the original PR — a navigation affordance over TDLib's own date lookup; no section 1.4 behavior. Basic checks (every PR)
Commands, results, platform/build/UI evidence, and reasons for inapplicable checks: inline above. Section 1.4 exception checksNot applicable. Agentic reviewer confirmationAgentic code reviewer only: record this confirmation in the PR review or check these boxes before approval.
|
iebb
left a comment
iebb
left a comment
There was a problem hiding this comment.
Re-reviewed updated head 0469eb5 against master 066c2d6. The owned picker replaces the Material picker, and the supplied held-success close/reopen and superseded-pick cases now pass. All 51 supplied/sibling search tests, scoped formatting/analyzer, and localization/generated checks pass; all 23 changed-file blobs match this exact head in the credential-free snapshot.
Five independent boundary cases still fail:
- Hold the lookup, close/open the controller, then complete with TdError(404): current is true, not false. The catch path ignores pick/run/disposal/active ownership and permits a stale no-message toast.
- An empty picked day with only a previous-day message returns that older message instead of null.
- A message exactly at next-day midnight is accepted even though it is outside the picked day.
- With messages at 08:00 and 23:00 on the chosen day, a transport faithfully implementing getChatMessageByDate returns the 23:00 message, not the advertised first message. Moving the timestamp to next midnight does not reverse TDLib's latest-before ordering, and the returned message date is never checked. See the TDLib contract.
- In the actual owned picker, seed/lastDate = 2026-10-13, page right once, then Confirm without selecting: the dialog returns 2026-11-13 despite all future days being disabled. Preserve a complete valid selected date or clear/revalidate selection on month changes and confirmation. Also localize the hard-coded English weekday labels.
Please finish session ownership on every completion path and picker await seam, implement/test the stated day-boundary contract, and cover month paging/range validity. These are coherent feature revisions, not a speculative automation workaround. No Telegram account/native-server navigation or feature publication was used.
| // a real empty answer, not a stale one. Anything else is transport or | ||
| // client noise: report it as superseded so no landing and no toast. | ||
| final stale = error is! TdError || error.code != 404; | ||
| return (current: !stale, message: null); |
There was a problem hiding this comment.
[P1] Apply the same pick/run/disposed/active ownership checks to errors. A held 404 after close(); open() returns current:true here, allowing the old lookup to toast in the new session. The independent delayed-error regression fails.
| pick == _datePickId && | ||
| _runId == runIdAtStart && | ||
| _active; | ||
| final message = current ? TDParse.message(response) : null; |
There was a problem hiding this comment.
[P1] This accepts any latest-before result, without checking its date or finding the first message. Three faithful boundary cases fail: previous-day-only -> older message, exact next-midnight -> next-day message, and two same-day messages -> the last rather than first. Changing the request timestamp alone cannot meet the advertised contract.
| onTap: _selectedDay == null | ||
| ? () {} | ||
| : () => Navigator.of(context).pop( | ||
| DateTime(_shownMonth.year, _shownMonth.month, _selectedDay!), |
There was a problem hiding this comment.
[P2] Revalidate the full selected date before confirmation. _shift changes the month but keeps _selectedDay. Seed/lastDate 2026-10-13 -> next month -> Confirm returns 2026-11-13, even though every displayed day is disabled. Shorter-month overflow needs coverage too.
0469eb5 to
6d17d6e
Compare
|
Updated and rebased onto current
|
iebb
left a comment
iebb
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 6d17d6e against master f311e34 in a credential-free integration snapshot. The previous five date/session/picker findings are addressed: all 55 supplied/sibling tests pass, including held 404 after close/reopen, picker ownership, day bounds, month validity, and localized weekday cases. The contribution declaration is now complete and exact-head Quality is green; classification remains compliant/unchanged.
A new independent busy-day boundary still fails. With 1,500 same-day messages, IDs 1..1500, and a faithful descending inclusive getChatHistory responder (100 entries per page), messageAtDate returns current:true / ID 510 instead of the actual first message, ID 1. The fixed maxPages=10 at chat_message_search_controller.dart:482 exhausts the budget without proving the day boundary was reached, then line 526 reports the partial earliest value as a successful first-message result. A history exception at lines 495-497 similarly breaks into that success return.
Please distinguish a proven first-in-day result from incomplete/capped/failed history resolution, and add busy-day and interrupted-pagination coverage. Do not silently claim an intermediate message is the first. Also align the eight no-message translations with the revised exact-day contract; they still say 'on or before'. This is a coherent navigation-contract revision rather than a speculative pagination workaround, so leaving this legitimate PR open. Validation used synthetic in-memory traffic only, with secrets_example.dart; no Telegram or real account access.
6d17d6e to
514b65b
Compare
|
Fixed in
New regression ( The eight |
iebb
left a comment
iebb
left a comment
There was a problem hiding this comment.
Re-reviewed the updated head against master 4e34da3. The earlier busy-day cap, localization, and cancellation checks now pass (57 focused checks). Terms classification remains compliant/unchanged, but the current implementation is not ready:
getChatHistoryis sentoffset: 1atchat_message_search_controller.dart:496. TDLib rejects any positive offset with error 400,Parameter offset must be non-positive(official implementation: https://github.com/tdlib/td/blob/master/td/telegram/MessagesManager.cpp#L16251). A contract-enforcing transport resolves an in-day anchor, then this lookup returns null instead of the first message. The supplied transport incorrectly accepts positive offsets. Please correct the request and its test contract.- Simply changing offset to zero is insufficient: an anchor-only short page is treated as a proven beginning at lines 537–541. TDLib chooses page length and may return fewer entries than requested. My independent short-page probe returned message
3 << 20instead of the earlier1 << 20, including with a local offset-zero-only probe. Please make non-progress unresolved or demonstrate a valid advancing/end-of-history strategy, rather than asserting this is the first message.
Also distinguish a failed history walk from an empty day: the current catch returns (current: true, message: null), which the header displays as “No messages on that date” even after an in-day anchor was found. Add real-contract busy-day, short/nonadvancing-page, and transport-error coverage. Current-head Quality is green; these production-boundary reproductions are not. Left open for revision; no speculative pagination change was published.
| 'chat_id': chatId, | ||
| 'from_message_id': fromMessageId, | ||
| 'offset': 1, | ||
| 'limit': pageLimit, |
There was a problem hiding this comment.
[P1] TDLib rejects positive getChatHistory offsets. A successful in-day anchor therefore becomes a 400 and a false empty-day result. Enforce the real request contract in the test transport.
| // The page no longer moves backwards (all ids at/after the anchor): | ||
| // nothing earlier in the day can appear — the walk is complete. | ||
| provedBoundary = true; | ||
| break; |
There was a problem hiding this comment.
[P2] A non-advancing short page does not establish the first message of the day. An anchor-only page currently makes the lookup return the latest anchor as first; offset=0 alone leaves this failure unchanged.
514b65b to
d494f10
Compare
|
Fixed in 1. 2. Short / non-advancing pages (P1). A page is no longer read as the beginning of the day. The walk reports a first message only after proof: a message older than the day, message id 3. Failure vs empty day. 4. Coverage added ( Gates at |
There was a problem hiding this comment.
Re-reviewed current head d494f10 against origin/master 4e34da3. The offset=0 change, unlimited busy-day walk, and distinct unresolved/empty-day outcome address the previous findings, but the replacement raw-ID arithmetic introduces a production blocker.
Independent reproduction: real-shaped messages (id: 1 << 20, 2 << 20, 3 << 20) on the same day, with a proxy enforcing the manifest-pinned TDLib MessageId::is_valid and non-positive-offset checks. The first history request uses 3145727 and is rejected; expected first message 1048576, actual null. The PR's 11 supplied date-jump tests and the separate short-page control pass; the packed-ID case fails (12 pass / 1 fail together).
Primary contract: MessageId validity, history validation. This matches scripts/tdjson-manifest.json, not an assumed latest API.
The change remains legitimate and stays open for correction. Terms classification is compliant/unchanged (date-search UI/history lookup); Quality is green on this head, but it does not exercise the packed-ID boundary, so I cannot merge yet.
| // increase with time inside a chat, and `offset` must stay non-positive: | ||
| // TDLib answers a positive offset with 400. | ||
| var fromMessageId = anchor.id - 1; | ||
| const pageLimit = 100; |
There was a problem hiding this comment.
[P1] Subtracting one creates an invalid TDLib history cursor. A server message ID is server_id << 20, not a consecutive integer. For the valid anchor 3 << 20 (3145728), this sends 3145727, whose low type bits are 7. The manifest-pinned TDLib rejects it with 400 (Invalid value of parameter from_message_id specified); messageAtDate catches that and returns an unresolved lookup instead of jumping. The same subtraction is repeated after each page. Please paginate with valid TDLib IDs and cover the actual packed-ID contract, including server/local IDs, short pages, exhaustion, and stalled pages.
d494f10 to
f8d565c
Compare
iebb
left a comment
iebb
left a comment
There was a problem hiding this comment.
Re-reviewed the current f8d565c implementation. The invalid packed-ID arithmetic is fixed, and that independent contract check now passes. One previously reported correctness boundary still fails: an anchor-only getChatHistory response shorter than limit is treated as proof of reaching the start of history.
Independent reproduction against this exact head plus current master: valid server message IDs 1<<20, 2<<20 and 3<<20 all fall on the selected day; getChatMessageByDate returns 3<<20; the first history response is [3<<20], with older messages still available. messageAtDate returns 3<<20 as the first message, but the first message is 1<<20. TDLib explicitly permits returning fewer messages than limit for performance, so neither a short page nor an echoed anchor establishes exhaustion. If the first message cannot be established, fail closed rather than claim this anchor is earliest.
Validation: flutter test --no-pub test/chat_search_date_jump_test.dart test/review_tdlib_history_contract_test.dart: 15 passed, 1 failed (a short anchor-only history page is not a proven first message). Please retain this partial-page boundary in regression coverage and fix the termination/progress logic without manufacturing invalid TDLib message IDs.
Primary contract: https://github.com/tdlib/td/blob/d1085f9cebc5a62379991ae1652673954f229c1f/td/generate/scheme/td_api.tl#L11821-L11829
Terms classification remains compliant/unchanged; this is a navigation correctness finding, not a rejection of the feature.
|
Fixed in Packed-id pagination (P1). Confirmed against the pinned sources: server message ids are Test transport now enforces the real contract. Gates at |
f8d565c to
0f67a0b
Compare
|
Fixed in A short (cursor-only) page no longer proves the end of history (P1). The previous revision treated "the cursor echoed alone in a page shorter than the limit" as proof that nothing older exists. As you noted, TDLib explicitly chooses the page length for performance, so that is not proof of anything. The walk now proves the boundary only with: a message older than the picked day, an empty history page, or server message id Your reproduction is now a named regression. Existing coverage kept and adjusted (the packed-id and busy-day walk counts changed because reaching server id 1 now proves exhaustion one page earlier): packed-id reproduction, id-validity matrix, 1,500-message busy day (16 pages, every cursor an id from the chat), 3-entry pages (750 pages), short pages that advance, pages that ignore the inclusive bound (3 attempts then fail closed), malformed pages, mid-walk transport errors → unresolved, empty-day semantics unchanged. Gates at |
Adds the calendar jump-to-date affordance to the in-chat search header: - Ownership across session/pick boundaries: ChatMessageSearchController tracks a monotonic _sessionId (bumped on open/close) in addition to _datePickId and _runId. _ChatSearchDateJump._pick verifies isCurrentSession(session) across the showAppDatePicker await seam, and messageAtDate re-checks isStillCurrent() at every await seam including the catch path (so a held 404 after close/reopen returns current:false and never toasts). - Day-boundary contract ([dayStart, dayEnd) local): queries getChatMessageByDate at dayEnd - 1 (23:59:59 local), rejects messages outside [dayStart, dayEnd) as null (empty day), and pages backwards via getChatHistory from the anchor while messages remain in [dayStart, dayEnd) so a multi-message day resolves to its first message rather than its last. - Owned date picker (lib/components/app_date_picker.dart): stores the full _selectedDate, revalidates/clamps selection on month shifts (so paging into an out-of-range future month clears selection and Confirm is a no-op, while paging from a 31-day month into a shorter in-range month clamps to the month's last day), and renders localized weekday headers via DateFormat.E(locale) (Monday-first).
0f67a0b to
ab35780
Compare
iebb
left a comment
iebb
left a comment
There was a problem hiding this comment.
Re-reviewed the actual updated head and reran the earlier TDLib pagination reproductions. The short/cursor-only page finding is resolved: retries are bounded, a permanent stall fails closed without claiming an earliest message, and advancing packed-ID pages still reach the first message. Session/pick cancellation and long-day cases pass (63 focused checks on this PR).
A small integration cleanup on master replaces the calendar's Material-only Colors dependency with widgets/identical explicit colors, adds owned-surface regression coverage, and corrects the stale pagination comment; no functional broadening.
Independent reviewer confirmation: the completed checklist and implementation classify as compliant/unchanged (ordinary explicit navigation/read-only search and reply resolution), not a Section 1.4 exception. Applicable exact-head Quality CI is green. Credential-free snapshots use secrets_example.dart; no account/session data or release/deployment workflow was used.
The exact combined master candidate 85b2c85 (tree 0511cdb9874d31a197d3eb858a73d6d3c9c43519) passes 3,610 unit/widget tests (1 existing skip), 115 focused tests, 13 independent boundary/reproduction tests, full analysis, repository formatting (1,024 Dart files unchanged), 8-locale validation/generated assets, registration anchors, and guarded-feature variants (false:14; invalid:14; true:116) plus the enabled feature-interaction check. No native/platform implementation changed; platform paths were exercised by the focused widget fixtures.
2 participants
Change
Adds a calendar "jump to date" affordance to the in-chat search header (
ChatSearchHeaderBar): picking a date resolves and scrolls the transcript to the first message sent on that day (local-time day bounds[dayStart, dayEnd)).ChatMessageSearchController.messageAtDate):getChatMessageByDateatdayEnd - 1(23:59:59local time on the picked day), which returns the latest message sent no later than that timestamp.datefalls within[dayStart, dayEnd): an older message from a previous day or a message at/afterdayEnd(exact next-day midnight) is rejected asnull(empty day), which toastschatSearchNoMessagesAtDateinstead of jumping.getChatHistory(from_message_id: anchor.id, offset: 0, limit: 100) while messages remain in[dayStart, dayEnd), returning the earliest message on the picked day so a multi-message day lands on its first message rather than its last.ChatMessageSearchControllertracks a monotonic_sessionId(bumped onopen()andclose()) alongside_datePickIdand_runId._ChatSearchDateJump._pickcapturescontroller.sessionTokenbefore awaitingshowAppDatePickerand drops the pick if the session changed while the dialog was open.messageAtDatere-checksisStillCurrent()(!_disposed && _active && _sessionId == sessionAtStart && pick == _datePickId && _runId == runIdAtStart) at every await seam — including thecatchpath ongetChatMessageByDate(so a heldTdError(404)afterclose(); open()returnscurrent: falseand never triggers a stale toast) and eachgetChatHistorypage.lib/components/app_date_picker.dart):AppDialogSurface/AppDialogActionwith an owned 7-column Monday-first month grid.DateTime? _selectedDateand revalidates/clamps on_shift(months)and_confirm(): paging into an out-of-range future month clears_selectedDateso Confirm is a no-op; paging from a 31-day month into a shorter in-range month clamps the day to that month's last day (e.g.2026-03-31→2026-02-28).DateFormat.E(locale)(Monday through Sunday) with fallback to thebusinessSettingsMonday..Sundayl10n keys.chatSearchJumpToDate,chatSearchNoMessagesAtDate) across all 8 locales (lib/l10n/app_localizations.dart,translations/strings/<locale>/strings.xml, regeneratedassets/l10n/<locale>.json).Affected platforms: in-chat search header (phone narrow layout, wide layout, and the pushed full-screen search — every surface that mounts
ChatSearchHeaderBar).Telegram API Terms classification
Follow the contribution and agentic review rules. Select exactly one classification and explain it below.
This is an agentic source-code review classification. Every store version must remain compliant with applicable Telegram API Terms; the guarded-source exception does not apply to store distribution or review.
allowTelegramTosViolations(I_DONT_FUCKING_CARE_ABOUT_TOS), disabled in official builds, and available only in an explicitly enabled self-build.Reason and affected Telegram behavior: a navigation affordance over TDLib's
getChatMessageByDateandgetChatHistoryread requests; no section 1.4 behavior.Basic checks (every PR)
Check each item after verification. If a check is inapplicable, explain why in the evidence below before checking it.
dart format --output=none --set-exit-if-changedover the 7 changed.dartfiles (CI-matching SDK, dart 3.12.2) exits 0.flutter analyzeover the 7 changed files (Flutter 3.44.2) — no issues.flutter test --concurrency=4.Commands, results, platform/build/UI evidence, and reasons for inapplicable checks:
~/.cache/dart3122/bin/dart format --output=none --set-exit-if-changed lib/chat/chat_message_search_bar.dart lib/chat/chat_message_search_controller.dart lib/chat/chat_view.dart lib/components/app_date_picker.dart lib/components/app_icons.dart lib/l10n/app_localizations.dart test/chat_search_date_jump_test.dart→Formatted 7 files (0 changed)PATH=/tmp/flutter-3.44.2/bin:$PATH flutter analyze --no-pub lib/chat/chat_message_search_bar.dart lib/chat/chat_message_search_controller.dart lib/chat/chat_view.dart lib/components/app_date_picker.dart lib/components/app_icons.dart lib/l10n/app_localizations.dart test/chat_search_date_jump_test.dart→No issues found!PATH=/tmp/flutter-3.44.2/bin:$PATH flutter test --no-pub -j 1 test/chat_search_date_jump_test.dart test/chat_message_search_test.dart test/desktop_inline_search_scope_test.dart→ 55/55 passed:test/chat_search_date_jump_test.dart— 8/8:the calendar resolves the picked day and hands it overa date with no messages toasts instead of jumpinga lookup held across close/reopen never landsa held 404 lookup across close/reopen returns current:false and never toastsclosing and reopening search while the date picker is open drops the picka newer pick supersedes the older held lookupday-boundary contract: previous-day-only and exact next-midnight return null; multi-message day returns the first messageowned date picker revalidates selection on month paging and shorter months, and localizes weekdaystest/chat_message_search_test.dart— 34/34test/desktop_inline_search_scope_test.dart— 13/130469eb56, all 5 new boundary assertions fail:close(); open()+TdError(404)returnedcurrent: trueand showed the stale no-messages toast.2026-10-09 23:59:59) returned message900instead ofnull.2026-10-11 00:00:00) returned message2000instead ofnull.08:00(1001) and23:00(1002) returned1002(latest) instead of1001(first).2026-10-13→ next month → Confirm returned2026-11-13instead of ignoring the disabled month;2026-03-31→ prev month → Confirm overflowed to2026-03-03instead of clamping to2026-02-28.python3 tool/check_l10n_strings.py→No visible UI string literals outside lib/l10n.python3 translations/tools/check.py→8 locales x 3831 keys (11 counted): OKpython3 translations/tools/gen_assets.py --check→9 locale assets are up to datepython3 tool/gen_registration_anchors.py --check→ exit 0Section 1.4 exception checks
Not applicable
Agentic reviewer confirmation
Agentic code reviewer only: record this confirmation in the PR review or check these boxes before approval.