Repository navigation
fix(chat): resolve and preserve reply anchors in channel post comment threads - #224
Conversation
iebb
left a comment
There was a problem hiding this comment.
Reviewed exact head 8492e6a against master f311e34 with template secrets and synthetic transports. All 34 supplied/adjacent reply, merge, and Moments tests pass, and exact-head Quality is green. The completed declaration and compliant/unchanged classification are noted.
Independent production-logic boundary tests find three failures:
- The reply sheet's new byId cache is keyed only by message ID and is seeded with the channel root (message_replies_sheet.dart:280-287). A channel root ID 42 and an unrelated discussion parent ID 42 collide. The child explicitly replies to discussion chat -2002 / message 42, but the parent lookup is skipped and the channel root is rendered as its quote instead of the correct discussion parent. The actual widget reproduction confirms the wrong root is shown.
- mergeChatMessages now carries resolved reply fields without checking the full reply target. Refreshing a message from reply(chat -1001, ID 42) to reply(chat -2002, ID 42) retains 'Old target' and its sender rather than resolving the new target.
- A restriction-redacted refresh with no reply target regains synthetic cached quote plaintext through lines 90-95. Only image fields are guarded by !isContentRestricted. This is a model-field preservation finding; no real-data disclosure or account-level exploit is being claimed.
Please key parent caches and navigation by (chatId, messageId) throughout both comment surfaces, carry cached quote fields only for an unchanged valid reply identity and permitted content state, and add these collision/restriction regressions. The asynchronous parent lookups also need account/session and cancellation coverage. This spans resolution, merge invariants, and routing, so I am not applying a speculative partial fix under this automation. No real credentials, Telegram access, native account/device QA, commit, or publication was used for this review; leaving the useful PR open.
8492e6a to
296fe01
Compare
|
Fixed in
|
iebb
left a comment
There was a problem hiding this comment.
Re-reviewed updated head 296fe01 against master 4e34da3 and confirmed the earlier merge-identity, redaction, and cross-chat quote-cache findings were addressed. Intended standard reply presentation/navigation remains compliant/unchanged. Current-head Quality (39000362198) is green, but the following production boundaries still fail:
- A delayed
getMessagesresponse is applied to the latest message reference after checking only the parent message ID. Hold a lookup for chat A/message 42, refresh the reply to chat B/message 42, then complete A: the refreshed reply gets A's old plaintext. A restriction-marked refresh can also be rehydrated. Validate the complete effective target chat + message identity, restriction state, and returned quote chat before applying it. - The sheet still removes rows by
message.id == widget.message.idwithout checking chat ownership. A discussion comment whose ID equals the channel post's ID disappears entirely. A faithful sheet fixture with channel root (-1001,42) and discussion comment (-2002,42) fails; root filtering must use composite identities too. - The in-sheet lazy-list jump remains broken for variable row heights. Reproduction: load 100 discussion comments, give comments 1–10 100 lines of normal text, have comment 100 reply to comment 50, jump to the bottom, and tap its quote. After
_scrollToSheetReplyfinishes, parent 50 is still not materialized/visible (expected one parent row, actual zero). The same proportional offset is guessed three times, and_rowKeys[targetMessageId]is captured before a lazy target can acquire its key. Please use a reliable measured/index-aware target strategy or a verified navigation fallback, and add an actual off-screen/variable-height widget regression.
I prepared narrow fixes for (1) and (2) in an isolated credential-free master snapshot: the failing cases plus disposal, cleared-target, wrong-response-chat, and live-target controls pass. That candidate passes full flutter analyze, all 3,485 unit/widget tests (1 existing skip), formatting (1,003 files unchanged), eight-locale/generated-assets/registration checks, and explicit false/true/invalid build-flag checks. Nevertheless, the independent end-to-end lazy-scroll regression in (3) still fails even with those repairs. It needs a validated scrolling solution rather than another speculative tweak, so I have not committed, pushed, or merged this candidate. This review is not approval of the unmodified head. Left open for revision.
| final refs = _messageRefs(m.id); | ||
| final targets = refs.isEmpty ? <ChatMessage>[m] : refs; | ||
| for (final message in targets) { | ||
| if (message.replyToMessageId != quoted.id || |
There was a problem hiding this comment.
[P1] Match the complete reply target before applying this delayed quote. If the latest reference now replies to another chat with the same message ID, the old response still overwrites it; restriction-marked refreshes need guarding too.
| ) | ||
| .toList() | ||
| ..sort((a, b) => a.date.compareTo(b.date)); | ||
| await _hydrateMessageSender(widget.message); |
There was a problem hiding this comment.
[P2] Preserve discussion rows whose numeric ID equals the channel post ID. The preceding ID-only root filter drops (-2002,42) when the source post is (-1001,42), before this composite lookup can run.
| final fraction = _messages.length <= 1 | ||
| ? 0.0 | ||
| : (targetIndex / (_messages.length - 1)).clamp(0.0, 1.0); | ||
| final targetOffset = (position.maxScrollExtent * fraction).clamp( |
There was a problem hiding this comment.
[P2] A proportional offset is not the parent row's position with variable-height messages. A 100-comment real-sheet fixture with long first rows leaves parent 50 off-screen after tapping its quote from comment 100. The key is also read once before lazy materialization; three repeated guesses do not establish a successful jump.
296fe01 to
4271627
Compare
|
Fixed in 1. Delayed 2. Sheet root filtering uses composite identity (P2). Rows are no longer removed by 3. In-sheet lazy jump reaches the parent (P2). Coverage added in Gates at |
iebb
left a comment
There was a problem hiding this comment.
Re-reviewed 4271627 against origin/master 4e34da3 and reran the earlier reproductions. The complete reply identity/restriction/disposal guards and composite root filtering now pass: retargeted/cleared/restricted/disposed replies, wrong-chat responses, unchanged-target control, and cross-chat numeric-ID collisions. Thank you for addressing those.
The variable-height lazy-list blocker remains. The exact independent test from the prior review still fails on the new implementation: 100 comments, first ten each 100 text lines, comment 100 replying to comment 50; jump to the real ListView's bottom, verify 50 is unmaterialized, tap messageReplyOpenOriginal, pumpAndSettle, expect the parent row. Actual: zero matching parent rows. Running the supplied message_replies_sheet suite plus independent async/identity/collision/scroll cases gives 27 pass / 1 fail; the PR's new shorter scroll fixture passes but does not cover this distribution.
_scrollToSheetReply returns void on both success and exhaustion, while _openReplyTarget always returns after awaiting it. Consequently there is no verified navigation fallback despite the new comment's claim. This needs a production-widget regression with strongly non-uniform heights, with target visibility (not just an offset change) asserted.
Terms classification remains compliant/unchanged: normal reply presentation/navigation, with redaction guards inspected. Current Quality is green, but the confirmed navigation failure blocks merging; leaving the useful PR open for revision.
| ? _sheetRowIndexOf(targetMessageId, chatId: targetChatId) | ||
| : -1; | ||
| if (targetIndex >= 0) { | ||
| await _scrollToSheetReply(targetIndex, targetMessageId); |
There was a problem hiding this comment.
[P1] A failed measured/probe scroll is still treated as successful navigation. The unchanged independent production-widget reproduction still fails on this head: load 100 discussion comments, make comments 1–10 each 100 lines, scroll to comment 100 which replies to loaded comment 50, then tap its actual reply anchor. After settling, comment 50 is still not materialized. The five probes plus ten upward half-viewport steps can exhaust or reach the wrong range, and this unconditional return prevents any explicit navigation fallback. Please make the actual target visible reliably, or report scroll failure and use a verified correctly addressed fallback rather than silently swallowing the tap.
4271627 to
599bb7f
Compare
|
Fixed in The in-sheet jump is now a measured sweep, not an estimated one (P1). The old descent guessed a probe offset in the remaining pixel range and read the top row index back; with ten 100-line comments in front of the target it kept the wrong half of the range and ran out of its five-step budget, and on exhaustion it returned silently. It now decides every step from the window the sliver has actually built — the target's item index is compared against the inclusive range of materialized rows: target materialized → jump to its measured reveal offset and verify; target above the built range → step up by 0.8 viewport; below → step down by 0.8 viewport. Scroll offset is monotone in item index, so each step moves strictly toward the target and the sweep terminates (budget 240 frames, well beyond the ~64 steps the reviewer's 45,000 px distribution needs). Nothing is proportional, nothing is estimated, and success means the target row is materialized AND intersects the viewport — never that some offset changed. A failed jump is no longer swallowed. Coverage added:
Gates at |
iebb
left a comment
There was a problem hiding this comment.
Re-reviewed the updated implementation and reran the earlier lazy-row, colliding chat/message ID, and asynchronous hydration reproductions. The long/nonuniform lazy-list jump now succeeds only when the exact target is actually visible, with an explicit correctly paired navigation fallback on failure. Quote redaction, retargeting, clear/dispose, cross-chat identity, and account-scoped Moments metadata cases remain correct (37 focused checks on this PR). Earlier change requests are resolved for this head.
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.
Change
Fix missing/broken reply anchors when replying to comments within a channel post across comment sheets, Moments post detail views, and linked discussion chats (all platforms):
lib/tdlib/td_models.dart: Parsereply_to.chat_idonmessageReplyToMessageintoChatMessage.replyToChatId(ignoring0/unknown) so cross-chat and discussion-scoped reply targets retain their owning chat ID.lib/chat/message_replies_sheet.dart: Seed the reply-lookup table withwidget.message, batch-fetch uncached parent comments viagetMessageson their resolvedreplyToChatId/ thread chat (replyTarget.chatId), and hydrate their sender names so replies to comments outside the initial 100-message window still render their reply quote block. When tapping a reply anchor inside the sheet, scroll to the parent comment in-sheet if loaded, or route cross-chat discussion targets throughonViewInChatwith the parent comment'smessageIdinstead of searching for the discussion message ID in the channel chat.lib/moments/moments_view.dart: InChannelPostDetailView, batch-fetch parent comments not present in the loaded thread window viagetMessages, resolve their sender names intoChannelPostComment.replyToSenderName, and display the reply anchor prefix (momentsReplyPrefix) even when the parent comment falls outsidebyId.lib/chat/chat_view_model.dart: Group unresolved reply targets byreplyToChatId ?? chatIdsogetMessagesqueries the target message's actual chat instead of always queryingchatId, and update all_messageRefson completion.lib/chat/chat_message_merge.dart: Preserve resolvedreplyToSender,replyToPreview,replyToDate,replyToEntities, andreplyToImage*acrossmergeChatMessagesso subsequent history/message refreshes do not wipe out resolved reply anchors.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:
Standard Telegram UI/UX bug fix for displaying and navigating reply anchors on comments within channel post threads (
getMessages/messageReplyToMessage). It does not alter read receipts, online status, message deletion/history retention, or any behavior covered by Section 1.4.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-changed lib test integration_test test_driver.flutter analyze.flutter test --concurrency=4.Commands, results, platform/build/UI evidence, and reasons for inapplicable checks:
git show --stat HEAD@8492e6a5):lib/chat/chat_message_merge.dart(+11/-0),lib/chat/chat_view_model.dart(+35/-14),lib/chat/message_replies_sheet.dart(+144/-36),lib/moments/moments_view.dart(+75/-4),lib/tdlib/td_models.dart(+9/-0),test/message_replies_sheet_test.dart(+478/-1).~/.cache/dart3122/bin/dart format --output=none --set-exit-if-changed lib/tdlib/td_models.dart lib/chat/chat_message_merge.dart lib/chat/chat_view_model.dart lib/chat/message_replies_sheet.dart lib/moments/moments_view.dart test/message_replies_sheet_test.dart→Formatted 6 files (0 changed)./tmp/flutter-3.44.2/bin/flutter analyze lib/tdlib/td_models.dart lib/chat/chat_message_merge.dart lib/chat/chat_view_model.dart lib/chat/message_replies_sheet.dart lib/moments/moments_view.dart test/message_replies_sheet_test.dart→No issues found!.test/message_replies_sheet_test.dartand 4 intest/moments_comment_reaction_test.dartpass with the fix):/tmp/flutter-3.44.2/bin/flutter test -j 1 test/message_replies_sheet_test.dart test/moments_comment_reaction_test.dart→00:10 +13: All tests passed!.New test cases in
test/message_replies_sheet_test.dart:mergeChatMessages preserves resolved reply anchor fields on refreshChatViewModel resolves cross-chat reply targets using replyToChatIdreply sheet resolves uncached comment reply anchors and jumps to discussion targetsChannelPostDetailView resolves parent sender anchor when parent comment is outside the thread windowpython3 tool/check_l10n_strings.py→No visible UI string literals outside lib/l10n.python3 translations/tools/check.py→8 locales x 3829 keys (11 counted): OKpython3 translations/tools/gen_assets.py --check→9 locale assets are up to datepython3 tool/gen_registration_anchors.py --check→ passed (exit 0).Section 1.4 exception checks
Complete every item for an exception. For a compliant/unchanged PR, write
Not applicablehere.Not applicable
--dart-define=I_DONT_FUCKING_CARE_ABOUT_TOS=false, the feature is absent and ordinary Telegram behavior remains intact.--dart-define=I_DONT_FUCKING_CARE_ABOUT_TOS=true, the feature works through the guarded entry points in a self-build; I included relevant test and build/UI evidence.falseand have no option that enables it. All store versions remain compliant with applicable Telegram API Terms.Guard locations and evidence for each flag variant:
Not applicable
Agentic reviewer confirmation
Agentic code reviewer only: record this confirmation in the PR review or check these boxes before approval.