Repository navigation
feat(chats): keep pins beyond the server limit on the device - #262
TiaraBasori wants to merge 3 commits into
Conversation
Automated PR reviewHead: Review unavailable: Waiting for the exact-head Quality run before using the free-model quota. Remaining gates
No PR code runs with the model key or write token. Free-only routing; provider/rate-limit failures never authorize a merge. Existing review findings must be resolved, not hidden by a new recommendation. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
iebb
left a comment
iebb
left a comment
There was a problem hiding this comment.
[P1] Keep the asynchronous local-pin synchronization bound to its original account.
Reviewed the current head as a legitimate improvement. In lib/chats/chat_list_view_model.dart:1004-1019, _syncLocalPinState snapshots adopted ids, awaits store.unpin(id), then continues using LocalPinStore.shared without checking _pinEpoch, account ownership or disposal. Meanwhile the meId setter changes that shared store to the replacement account. With two adopted ids and a preference write still pending, switching accounts between iterations makes the next unpin operate on the replacement account's keys; the final loop can also project that account's entries into the old model.
Please pin an immutable account owner/generation for this pass and stop before subsequent preference writes or row mutations after an ownership change/disposal. Include a faithful delayed-preferences regression that switches A to B after the first awaited unpin, uses colliding chat ids and verifies B's pins are untouched; also cover disposal. Store-only synchronous account-scoping tests do not exercise this boundary. This is a static production-path trace, not a claim that a new test was executed. Leave this legitimate PR open for revision.
12c9a28 to
4d40eff
Compare
|
Addressed at head [P1] The pass is now bound to one store scope. Pinned by
Mutation checks, quoted: removing the per-iteration gate and the Format |
There was a problem hiding this comment.
Re-reviewed 4d40eff. The same-model meId-switch/disposal regression is substantially improved, but [P1] the captured store generation still is not an ownership guard before effects. _syncLocalPinState captures it at lib/chats/chat_list_view_model.dart:1004 but ownsPass() at 1007 only checks this model's epoch. LocalPinStore.unpin at lib/chats/local_pin_store.dart:219-237 removes the current entry and writes the current captured user key before comparing expectedGeneration. If another live account-scoped ChatListViewModel re-scopes the singleton while the old pass awaits, this model's epoch need not change; its next unpin removes the replacement account's colliding id before the late generation check. Check generation/owner before every in-memory removal, preference write and projection; reject an already-stale expectedGeneration at entry. Add a regression that re-scopes the shared store without setting the original model's meId, and a direct stale-generation unpin case asserting both replacement memory and preferences survive. Also bind _refreshPinCap (870-880) to its epoch/owner: a delayed old-account response currently assigns its cap to the replacement account. These are source-traced remaining boundaries, not a claim that new tests were run locally. The PR remains a legitimate improvement, but account isolation is required before approval.
Integration note against now-current master d411c13: a read-only merge-tree check also confirms conflicts in lib/chats/chat_list_view_model.dart and the eight generated assets/l10n JSON catalogues. After fixing ownership, integrate current master while preserving #246's owner-pinned bulk-read implementation; regenerate the shared assets from the reviewed canonical XML instead of selecting one side's generated catalogue. No conflict resolution or app validation was run locally.
|
Addressed at head [P1] The generation is now the guard, before every effect. In the model, Regressions (
Mutation checks, quoted: removing the entry-level check fails the first case with Master integration. Merged Format |
TiaraBasori
left a comment
TiaraBasori
left a comment
There was a problem hiding this comment.
At new head b778fc62 — an increment on top of the 9c4d69a rebase (which already carried the pre-fix 9e7ec66f), no baseline change: every in-memory removal, preference write and projection now re-checks the captured store generation/owner.
What the head already proved red (mutated, then re-run green):
- Remove before the late check in
LocalPinStore.unpin(lib/chats/local_pin_store.dart): the old shape deleted the current entry and wrote the current captured user key beforeexpectedGenerationwas compared. Now a staleexpectedGenerationis refused at entry (if (!_ownsGeneration(expectedGeneration)) return false;before any mutation), the generation is re-checked after the barrier, after the preference write, and beforenotifyListeners. Stashinglib/chats/local_pin_store.dartfailsa directly stale generation is refused before the store mutateswithExpected: true / Actual: <false>("a stale generation must not remove the replacement entry") — the exact data-loss boundary named. - Shared store re-scoped without this model's meId moving in
_syncLocalPinState(lib/chats/chat_list_view_model.dart): a second live account-scopedChatListViewModelre-scopes the singleton while the first pass is parked ondebugUnpinBarrier;_ownsPinPass(epoch, generation)now requiresstore.loadGeneration == generation && _storeIsOwnAccount && !_disposedand is re-checked before every unpin, before each row mutation and before the resort, so the late pass cannot drop the replacement account's colliding id (regressiona pin pass stops when the account changes under its awaited unpinanda re-scoped store survives a parked unpin from a list that still believes it owns the account— the original model'smeIdis deliberately left untouched in the second one). - Delayed
_refreshPinCap(the cited 870-880 region, now ~993):epoch/generationare captured before thegetOptionawait and re-checked before_pinCapis assigned; removing the gate failsa delayed pin cap does not land on the replacement accountwithExpected: <9> / Actual: <3>.
This increment (b778fc62, 2 files, +164/-2) closes the two projections that still read the shared store without owning its scope — the same class, not the cited lines:
_applyPosition's inline pin branch:isLocalis now_storeIsOwnAccount && store.unlimitedEnabled && store.isLocallyPinned(id). A liveupdateChatPositionfor a chat whose id collides with a pin the replacement account holds used to read that foreign entry and pin this list's row on the foreign band order. New regressiona position that lands while the store is re-scoped must not project a foreign band onto this listdrives the re-scope with the original model'smeIduntouched, then delivers the position: without the gate it fails withExpected: false / Actual: <true>("a re-scoped store must not pin this list's row"); with it the row keepsisPinned: falseand its live order, and the replacement account's memory + preference key both survive._handleLocalPinStoreChange(the switch-off release walk):if (!_storeIsOwnAccount) return;before iteratingstore.pinnedChatIds. The walk previously projected whatever account's ids the store held onto this list's rows (ids collide across accounts), releasing rows this list never held. New regressionturning the switch off while the store is re-scoped releases nothing on this listkeeps the store scoped to the other account whilesetUnlimitedEnabled(false)fires: this list's row keeps its order and the replacement account's stored entry survives.
Gates re-run at this head with CI's toolchain (/tmp/flutter-3.44.2), serially: flutter test --no-pub -j 1 test/local_pins_test.dart → 18 tests passed (was 16; the two new projections included), flutter test --no-pub -j 1 test/local_pin_settings_test.dart → 1 passed, flutter test --no-pub -j 1 test/chat_folder_mark_read_owner_test.dart → 5 passed; flutter analyze --no-pub lib/chats/chat_list_view_model.dart lib/chats/local_pin_store.dart test/local_pins_test.dart → No issues found; dart format --output=none --set-exit-if-changed on the four touched files → 0 changed; the l10n trio stays green.
On the integration note: against current master (82397aef, ahead of the 3c317d0 the 9c4d69a merge integrated) a read-only git merge-tree --write-tree --name-only 82397aef b778fc62 now conflicts only in the eight generated assets/l10n/*.json — lib/chats/chat_list_view_model.dart and every translations/strings/**.xml auto-merge, and git diff origin/master...HEAD -- lib/chats/chat_list_view_model.dart still contains no line touching _bulkReadGeneration, _pinWriteOwner, _writeOwnerIsLive, _readChatListPage, _markChatListsRead or clientForTesting, so #246's owner-pinned bulk-read implementation is preserved verbatim. The generated-catalogue regen merge is intentionally not bundled into this ownership increment; it lands next so this head stays reviewable as a pure ownership delta.
With the unlimited-local-pins switch on, a pin the server would refuse (the cap is reached, or the chat has no position) is kept on this device only: the entry is account-scoped in the preference store, the row sorts above every server pin, and nothing is sent to Telegram. The sync pass that adopts server pins and re-applies the local band now belongs to one store scope: it captures the store's load generation and the list's pin epoch, gates every iteration on them, and hands the generation to unpin, which abandons its preference write if the store was re-scoped under it. A disposal stops the pass before its next write. Two colliding accounts can therefore never have one account's unpin land on the other's keys. Regressions hold the first unpin's preference write open, switch to an account that shares the same chat ids, release it, and assert both of the replacement account's keys survive; the disposal variant disposes the list with the write in flight and asserts the entry it had not reached is untouched.
The store is one singleton shared by every live chat list, so a pass that spans an await can be re-scoped by another list without this model's meId changing. The epoch-only ownsPass() therefore stayed green after such a re-scope, and unpin() removed the colliding entry and wrote the captured account's key before comparing expectedGeneration. Every mutator now checks the captured generation before each effect — the in-memory removal, each preference write and each notification — and an already-stale expectedGeneration is refused at entry. pin() and unpin() return whether they applied. The sync pass's ownsPass() includes the store generation and the store/account ownership, and is re-checked before each row mutation and before the resort; _pruneLocalPins and the toggle gesture hand the generation through and gate the same way; meId no longer reloads a store another list already scoped to the same account. _refreshPinCap captures the epoch and generation at request start and drops a delayed answer that belongs to a superseded account. Regressions: a re-scope of the shared store during a parked unpin with the original meId left untouched (both the replacement entry and its key survive), a directly stale expectedGeneration refused before any mutation while a fresh one still applies, and a parked pin-cap answer dropped after the account is replaced.
b778fc6 to
5ff57ba
Compare
The review's remaining class: the sync pass, the toggle and the prune all re-check the store's generation now, but two projection paths still read the shared store without owning it. A position that lands while the store is scoped to another account read that account's entry for the same chat id and pinned this list's row on the foreign band order, and the switch-off release walked the store's pinned ids — another account's, whose ids collide — and released rows this list never held. Both projections now require _storeIsOwnAccount before they read the store, exactly like every other local-pin effect. The model's meId is deliberately left untouched by the regressions, so only the ownership check can keep the foreign band off this list's rows. Regressions: a main-list position arriving while the shared store is re-scoped to an account holding a pin for the same chat id must not pin this list's row nor move its order, and turning the switch off while the store stays scoped to the other account must neither release this list's rows nor drop the replacement account's stored entry.
5ff57ba to
65996b1
Compare
Change
Adds a device-only pin for the chat list, reachable through a new Advanced settings switch (“Unlimited pinned chats”, off by default). Telegram caps pinned chats per account (5, 10 with Premium, in the main list). With the switch on, a pin that the server would refuse — the account is already at its cap, or the chat has no main-list position yet — is kept on this device instead: no
toggleChatIsPinnedrequest is sent, the chat sorts above the server pins with the usual pin icon and pinned-row colour, and the entry is stored in this app's own preferences, scoped to the account.This is not a sync trick: nothing is written to Telegram and nothing is read back from it. Pinning and unpinning within the server's limit keeps behaving exactly as before (the same request, byte for byte), and so does the whole feature with the switch off. A device-only entry is dropped when the user unpins it, when the chat leaves the main list (archived, deleted, left), or when Telegram itself pins the chat — the server pin then owns the row at the server's own order.
Behaviour details:
pinned_chat_count_maxoption (default 5 until it answers; a value <= 0 means the server does not cap pins and then every pin stays a normal server pin).Affected platforms: all (shared chat-list code; the entry is in the shared Advanced settings page).
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:
Section 1.4 forbids interfering with Telegram's basic functionality: acting on the user's behalf without consent, preventing self-destructing content from disappearing, tampering with read/online/typing statuses. This change touches none of that. It does not send anything to Telegram, does not forge a server-side state, and does not suppress or alter any status. It is a purely client-side ordering preference for the user's own device: the chat is marked with this app's pin styling and placed at the top of this device's list, while Telegram's own pinned chats are loaded, counted and displayed exactly as before. The chat is never reported to Telegram as pinned, and other devices do not see the pin.
toggleChatIsPinnedis still sent for every pin the server can accept, and unpinning a server pin is still a server operation.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:
Toolchain: Flutter 3.44.2 (
/tmp/flutter-3.44.2, the version Quality CI pins). This head integrates master through3c317d0cand carries three commits:9e7ec66f(ownership guards),9c4d69aa(merge of origin/master) andb778fc62(projection ownership: the position projection and the switch-off release walk own the store's account scope before they read it).Formatting:
Analysis:
Tests (serial, CI toolchain):
The last two are #246's owner-pinned bulk-read suites, run on the merged tree.
git diff origin/master...HEAD -- lib/chats/chat_list_view_model.dartcontains no line touching_bulkReadGeneration,_pinWriteOwner,_writeOwnerIsLive,_readChatListPage,_markChatListsReadorclientForTesting, so master's implementation came through the merge verbatim; the only textual conflict in that file wasdispose(), where both sides' work is kept.test/local_pins_test.dartcovers the store (pin/unpin/prune, account scoping, and the three ordering invariants: locals above every server order, newest local first, idempotent band), plus the model through a fake TDLib transport: a pin past the cap sends notoggleChatIsPinnedand lands the row first; the same gesture again releases it and restores the chat's own order; a server position that pins the chat drops the local entry; the switch off keeps the pre-existing request byte-identical and writes no preference.test/local_pin_settings_test.dartdrives the Advanced settings row (cap in the subtitle, toggle persists, toggle off clears the preference).Ownership boundaries (new at this head):
a pin pass stops when the account changes under its awaited unpin— two device-only pins whose chats anupdateChatPositionnow reports server-pinned. The first unpin's preference write is held withLocalPinStore.debugUnpinBarrier, the shared store is re-scoped to an account that shares the same chat ids without touching the original model'smeId, then the write is released; both of the replacement account's preference keys and both entries in memory survive. This is the case the previous head missed: the old pass passed its own epoch check because only the shared store had moved.disposal stops the pin pass before its next write— the same pass disposed while the first write is parked: the entry it had not reached stays stored and no row mutation is scheduled.a stale expected generation cannot unpin the replacement account— a directunpin(id, expectedGeneration: stale)after the re-scope: the replacement's in-memory entry and its preference value are both untouched, and a fresh-generation unpin then succeeds.a delayed pin-cap answer from the old account does not replace the new one— the firstgetOptionis held, the account is replaced and reads9, then the old answer (3) lands: the cap stays9.a position that lands while the store is re-scoped must not project a foreign band onto this list(new atb778fc62) — another live list re-scopes the shared store while the original model'smeIdis left untouched; a liveupdateChatPositionfor a chat id the replacement account also pins must neither pin this list's row nor move its order. Mutation: stashing the_storeIsOwnAccountterm in_applyPositionfails it withExpected: false / Actual: <true>("a re-scoped store must not pin this list's row").turning the switch off while the store is re-scoped releases nothing on this list(new atb778fc62) — the switch-off release walk owns the account scope before iteratingpinnedChatIds: with the store still scoped to the other account,setUnlimitedEnabled(false)neither releases this list's rows nor drops the replacement account's stored entry.Effect points, so the guard is not only at the cited lines:
LocalPinStore.unpin/pin/pruneToreject an already-staleexpectedGenerationat entry and re-check after everyawait(barrier, preference write) before the in-memory mutation, the notification and the next iteration;_loadAccountEntriesis the single owner of the generation bump;ChatListViewModel._ownsPinPass(epoch, generation)additionally requires the store to still be scoped to this model'smeIdand is re-checked before every unpin, before each row mutation and before the resort;_pruneLocalPinsand_togglePinUnlockthread the same generation; themeIdsetter no longer re-scopes a shared store another live list already scoped to the same account;_refreshPinCapcaptures_pinEpoch+loadGenerationat request start and drops a delayed answer that lost either.Mutation checks, quoted: removing the entry-level generation check in
unpinfails the first case withExpected: true / Actual: <false>attest/local_pins_test.dart:782("a stale generation must not remove the replacement entry"); leaving the check only after_entries.remove(the reviewer's cited shape) fails it the same way; removing the_ownsPinPassgate in_refreshPinCapfails the cap case withExpected: <9> / Actual: <3>. All three were reverted and the suite re-run green.Localization/generated files: two new keys (
chatListLocalPinEnabled,chatListLocalPinLimitReached) through all three layers —lib/l10n/app_localizations.dart,translations/strings/<8 locales>/strings.xml, regeneratedassets/l10n/*.json. On the merged tree:python3 translations/tools/check.py(8 locales x 3936 keys, OK),python3 translations/tools/gen_assets.py --check(9 locale assets are up to date),python3 tool/check_l10n_strings.py(No visible UI string literals outside lib/l10n). The eight conflictedassets/l10n/*.jsonwere regenerated from the merged canonical XML rather than resolved by picking one side.Build/platform paths: Dart-only change; no new plugin, asset, permission, or native code, so the iOS/Android/macOS/Windows/Linux build paths are untouched. No store build or packaging change.
Line counts (
git diff --numstat origin/master...HEAD, headb778fc62):Section 1.4 exception checks
Complete every item for an exception. For a compliant/unchanged PR, write
Not applicablehere.Not applicable
Agentic reviewer confirmation
Agentic code reviewer only: record this confirmation in the PR review or check these boxes before approval.