diff --git a/src/hooks/restore_hook.py b/src/hooks/restore_hook.py index 9c7c07f..2ec8455 100644 --- a/src/hooks/restore_hook.py +++ b/src/hooks/restore_hook.py @@ -47,6 +47,20 @@ class RestoreHook: def __init__(self, plugin): self.plugin = plugin self._unhooks: List[Any] = [] + self._scope_state = {"done": set(), "rows": {}} + + def invalidate(self) -> None: + """Drops the restore cache so the next frame re-reads saved summaries. + + Called after storing a summary: the gate deliberately skips work for a + scope it has already handled, so a newly saved row would otherwise not + be restored until the chat was reopened. + """ + try: + self._scope_state["done"].clear() + self._scope_state["rows"].clear() + except Exception: + self._scope_state = {"done": set(), "rows": {}} def install(self) -> None: try: @@ -75,10 +89,13 @@ class RestoreHook: return plugin = self.plugin - # updateMessagesVisiblePart fires on every scroll frame, so restoring - # is gated on the card actually being absent: restore_into() bails as - # soon as the summary is already in the list. - last_scope = {"key": None} + # Shared with invalidate(): `updateMessagesVisiblePart` fires on every + # scroll frame, so restoring is gated on a scope identity that survives + # across calls (see _scope_identity) plus the loaded window size, and + # the DB rows are cached per scope. + # Reuse the dict created in __init__ rather than replacing it, so the + # reference invalidate() operates on stays the one the hook reads. + last_scope = self._scope_state class _RestoreAfterLoad(MethodHook): def after_hooked_method(self, param): @@ -107,12 +124,29 @@ class RestoreHook: self._unhooks = [] +def _scope_identity(chat_activity: Any, dialog_id: int, topic_id: int) -> tuple: + """Identity that is stable across hook invocations for one open chat. + + `id(chat_activity)` cannot be used: each Xposed callback hands Python a + fresh Chaquopy wrapper around the same Java object, so its Python id + differs every call and a cache keyed on it never hits. BaseFragment's + classGuid is assigned once per fragment instance, so it identifies the open + ChatActivity for as long as it lives and changes when a new one is opened. + """ + guid = 0 + try: + guid = int(chat_activity.getClassGuid()) + except Exception: + guid = 0 + return (guid, int(dialog_id), int(topic_id)) + + def restore_into(plugin: Any, chat_activity: Any, last_scope: Any = None) -> None: """Injects stored summaries for the chat this ChatActivity is showing. - Called from a high-frequency lifecycle hook, so it must be cheap: it does - one DB read per (dialog, topic) it has not seen yet, and - restore_saved_summaries() skips anything already present in the list. + Called from `updateMessagesVisiblePart`, which runs on every scroll frame, + so the common case must do no work at all: once a scope has been restored + it is remembered and every later frame returns after one tuple compare. """ from services.rich_injector import restore_saved_summaries @@ -134,21 +168,48 @@ def restore_into(plugin: Any, chat_activity: Any, last_scope: Any = None) -> Non except Exception: topic_id = 0 - # Avoid a DB round-trip on every scroll frame; a new scope means a - # different chat/topic was opened, which is the only time a restore is due. - scope_key = (id(chat_activity), dialog_id, topic_id) - if last_scope is not None and last_scope.get("key") == scope_key: - return + # Gate before touching the DB. `updateMessagesVisiblePart` runs on every + # scroll frame, and re-querying SQLite plus rescanning the message list each + # time is what made scrolling stutter in summarized chats. + # + # The gate includes the loaded message count: a card can only be placed + # while its anchor is loaded, and a chat opens with a short window, so + # summaries anchored further up must get another chance once paging widens + # it. Size changes only when messages are added or removed, so scrolling + # within an unchanged window still does no work. + scope_key = _scope_identity(chat_activity, dialog_id, topic_id) if last_scope is not None: - last_scope["key"] = scope_key + try: + from services.rich_injector import get_messages_list - try: - summaries = db.get_for_scope(dialog_id, topic_id) - except Exception as exc: - diagnostics.record_error("restore_hook.db.get_for_scope", exc) - return - # Rows reserved at stream start have empty text until the stream finishes. - summaries = [s for s in summaries if (s.text or "").strip()] + messages_list = get_messages_list(chat_activity) + loaded = int(messages_list.size()) if messages_list is not None else 0 + except Exception: + loaded = 0 + state_key = scope_key + (loaded,) + if state_key in last_scope.get("done", ()): + return + done = last_scope.setdefault("done", set()) + # Bounded: one entry per (fragment, scope, window size) actually seen. + if len(done) > 256: + done.clear() + done.add(state_key) + + # Cache the rows per scope: paging re-runs this for the same chat, and the + # saved set only changes when a new summary is stored. + cache = last_scope.setdefault("rows", {}) if last_scope is not None else {} + summaries = cache.get(scope_key) + if summaries is None: + try: + summaries = db.get_for_scope(dialog_id, topic_id) + except Exception as exc: + diagnostics.record_error("restore_hook.db.get_for_scope", exc) + return + # Rows reserved at stream start have empty text until the stream ends. + summaries = [s for s in summaries if (s.text or "").strip()] + if len(cache) > 32: + cache.clear() + cache[scope_key] = summaries if not summaries: return diff --git a/src/main.py b/src/main.py index 381b0d5..1c6c66d 100644 --- a/src/main.py +++ b/src/main.py @@ -412,6 +412,13 @@ class ChatSummariesPlugin(BasePlugin): rendered("thinking", final=True), len(messages), ) + # The restore gate skips scopes it has already handled, so + # the newly saved row has to invalidate that cache or it + # would not be restored until the chat was reopened. + try: + self.restore_hook.invalidate() + except Exception: + pass except Exception as exc: diagnostics.record_error("_stream_into:db.save", exc) diff --git a/src/services/rich_injector.py b/src/services/rich_injector.py index af78c3f..4211f15 100644 --- a/src/services/rich_injector.py +++ b/src/services/rich_injector.py @@ -181,17 +181,18 @@ def get_chat_adapter(chat_activity: Any) -> Optional[Any]: return None -def find_index_for_anchor(messages_list: Any, anchor_msg_id: int) -> int: +def find_index_for_anchor(messages_list: Any, anchor_msg_id: int, require_anchor: bool = True) -> int: """Index at which to insert so the summary sits directly after the anchor. ChatActivity.messages is newest-first, so "directly after the anchor" chronologically means "immediately before it" in list order. - The anchor is the newest message that was actually analyzed, which is not - the newest message in the chat whenever a non-zero offset was used. If the - anchor itself is not loaded (paged out, or never fetched into this view), - fall back to ordering by id rather than to index 0: index 0 is the bottom - of the chat and would place the card after messages the summary never saw. + Returns -1 when the anchor message is not loaded in this view and + `require_anchor` is set. Positioning by id order alone is not safe: with a + non-zero offset, or a range that has since paged out, the nearest-by-id slot + can sit among messages the summary never analyzed, which is how cards ended + up attached to the wrong message. Callers skip the insert instead and try + again once the anchor is paged in. """ anchor_msg_id = int(anchor_msg_id) try: @@ -200,6 +201,8 @@ def find_index_for_anchor(messages_list: Any, anchor_msg_id: int) -> int: owner = getattr(messages_list.get(i), "messageOwner", None) if owner is not None and int(getattr(owner, "id", 0) or 0) == anchor_msg_id: return i + if require_anchor: + return -1 # Anchor absent: insert before the first real message whose id is at # most the anchor's, i.e. the newest message not newer than the @@ -215,7 +218,7 @@ def find_index_for_anchor(messages_list: Any, anchor_msg_id: int) -> int: return size except Exception as exc: diagnostics.record_error("injector.find_index_for_anchor", exc) - return 0 + return -1 def find_existing(messages_list: Any, virtual_id: int) -> int: @@ -294,15 +297,28 @@ class SummaryInjection: return False self.message_object = message_object - existing = find_existing(messages_list, self.virtual_id) - index = existing if existing >= 0 else find_index_for_anchor(messages_list, self.anchor_msg_id) - + # The index must be computed on the UI thread, immediately before the + # mutation that uses it. Computing it here would race: loading a large + # range takes long enough for paging or an incoming message to shift the + # list, and the stale index then lands the card after the wrong message. def run(): try: + messages_list_now = get_messages_list(self.chat_activity) + if messages_list_now is None: + return + existing = find_existing(messages_list_now, self.virtual_id) if existing >= 0: - messages_list.set(index, message_object) + messages_list_now.set(existing, message_object) else: - messages_list.add(index, message_object) + index = find_index_for_anchor(messages_list_now, self.anchor_msg_id) + if index < 0: + # Anchor not loaded in this view: placing the card by id + # order alone would put it next to unrelated messages. + diagnostics.note( + f"injector: anchor {self.anchor_msg_id} not loaded, skipping insert" + ) + return + messages_list_now.add(index, message_object) adapter = get_chat_adapter(self.chat_activity) if adapter is not None: adapter.notifyDataSetChanged() @@ -391,6 +407,10 @@ class SummaryInjection: messages_list.set(index, replacement) else: index = find_index_for_anchor(messages_list, self.anchor_msg_id) + if index < 0: + # Anchor paged out mid-stream. Keep the streamed + # text; it is re-inserted once the anchor is back. + return messages_list.add(index, replacement) inserted = True for stale_id in self._used_ids: @@ -497,8 +517,16 @@ class SummaryInjection: duplicate = find_existing(messages_list, virtual_id) if duplicate >= 0: messages_list.remove(duplicate) + # The card was just removed, so it must go back somewhere: + # the new anchor came from the loaded list and is normally + # present, but if it paged out in between, fall back to id + # ordering rather than dropping the card entirely. target = find_index_for_anchor(messages_list, anchor_msg_id) - messages_list.add(target, message_object) + if target < 0: + target = find_index_for_anchor( + messages_list, anchor_msg_id, require_anchor=False + ) + messages_list.add(max(target, 0), message_object) adapter = get_chat_adapter(self.chat_activity) if adapter is not None: adapter.notifyDataSetChanged() @@ -520,17 +548,43 @@ def _on_ui(fn) -> None: fn() +def loaded_message_ids(messages_list: Any) -> set: + """Ids currently loaded in the view, in one pass. + + Restoring used to scan the whole list once per saved summary; a single set + keeps it linear in the list length no matter how many summaries exist. + """ + ids = set() + try: + for i in range(messages_list.size()): + owner = getattr(messages_list.get(i), "messageOwner", None) + if owner is not None: + ids.add(int(getattr(owner, "id", 0) or 0)) + except Exception as exc: + diagnostics.record_error("injector.loaded_message_ids", exc) + return ids + + def restore_saved_summaries(chat_activity: Any, account: int, dialog_id: int, topic_id: int, summaries) -> int: - """Re-injects persisted summaries into a freshly opened chat. Returns count.""" + """Re-injects persisted summaries into a freshly opened chat. + + Only summaries whose anchor is currently loaded are injected: a card whose + anchor is outside the loaded window has no correct position, and guessing + one by id order is what stacked every saved card together at the bottom of + the chat. The rest are picked up on a later call, once paging has brought + their anchor in. Returns how many were dispatched for insertion. + """ messages_list = get_messages_list(chat_activity) if messages_list is None or not summaries: return 0 - register_mock_sender(account) + loaded = loaded_message_ids(messages_list) pending = [] for summary in summaries: virtual_id = virtual_id_for(summary.id, summary.anchor_msg_id) - if find_existing(messages_list, virtual_id) >= 0: + if virtual_id in loaded: + continue + if int(summary.anchor_msg_id) not in loaded: continue message_object = build_summary_message( account, @@ -543,11 +597,12 @@ def restore_saved_summaries(chat_activity: Any, account: int, dialog_id: int, to False, ) if message_object is not None: - pending.append((summary.anchor_msg_id, message_object)) + pending.append((summary.anchor_msg_id, virtual_id, message_object)) if not pending: return 0 + register_mock_sender(account) # Newest anchor first: each insert shifts everything below it, so placing # the newest card first keeps the remaining lookups pointing at unmoved # rows. The index is recomputed per card against the live list anyway. @@ -555,11 +610,24 @@ def restore_saved_summaries(chat_activity: Any, account: int, dialog_id: int, to def run(): try: - for anchor_msg_id, message_object in pending: - messages_list.add(find_index_for_anchor(messages_list, anchor_msg_id), message_object) - adapter = get_chat_adapter(chat_activity) - if adapter is not None: - adapter.notifyDataSetChanged() + # Re-resolve on the UI thread: the list this was queued against may + # have paged or been replaced in between. + live = get_messages_list(chat_activity) + if live is None: + return + added = 0 + for anchor_msg_id, virtual_id, message_object in pending: + if find_existing(live, virtual_id) >= 0: + continue + index = find_index_for_anchor(live, anchor_msg_id) + if index < 0: + continue + live.add(index, message_object) + added += 1 + if added: + adapter = get_chat_adapter(chat_activity) + if adapter is not None: + adapter.notifyDataSetChanged() except Exception as exc: diagnostics.record_error("injector.restore", exc) diff --git a/tools/verify_bundle.py b/tools/verify_bundle.py index 392ff61..e0e8dfd 100755 --- a/tools/verify_bundle.py +++ b/tools/verify_bundle.py @@ -1582,28 +1582,35 @@ def main(argv=None) -> int: # Anchor not loaded in this view: falling back to index 0 would put the # card below messages the summary never saw, which is the bug this # ordering fallback exists to prevent. + # These exercise the id-ordering fallback itself, which is now opt-in: + # callers that must place a card somewhere (reanchor) ask for it, while + # the default refuses to guess a position for an unloaded anchor. check( - "an unloaded anchor is ordered by id instead of dropping to the bottom", - injector.find_index_for_anchor(offset_activity.messages, 750) == 2, - f"index={injector.find_index_for_anchor(offset_activity.messages, 750)}", + "requested id ordering places an unloaded anchor by id", + injector.find_index_for_anchor(offset_activity.messages, 750, require_anchor=False) == 2, + f"index={injector.find_index_for_anchor(offset_activity.messages, 750, require_anchor=False)}", ) check( "an anchor older than everything loaded lands at the top of the list", - injector.find_index_for_anchor(offset_activity.messages, 100) + injector.find_index_for_anchor(offset_activity.messages, 100, require_anchor=False) == offset_activity.messages.size(), - f"index={injector.find_index_for_anchor(offset_activity.messages, 100)}", + f"index={injector.find_index_for_anchor(offset_activity.messages, 100, require_anchor=False)}", ) check( "an anchor newer than everything loaded lands at the bottom", - injector.find_index_for_anchor(offset_activity.messages, 1000) == 0, - f"index={injector.find_index_for_anchor(offset_activity.messages, 1000)}", + injector.find_index_for_anchor(offset_activity.messages, 1000, require_anchor=False) == 0, + f"index={injector.find_index_for_anchor(offset_activity.messages, 1000, require_anchor=False)}", ) # The real invariant is positional, not a fixed index: every message # newer than the anchor stays below the card, every older one above, # regardless of injected cards already sitting in the list. def neighbours(activity_obj, anchor): - index = injector.find_index_for_anchor(activity_obj.messages, anchor) + # require_anchor=False so the invariant is checked for unloaded + # anchors too: when a position is chosen at all, it must be correct. + index = injector.find_index_for_anchor( + activity_obj.messages, anchor, require_anchor=False + ) ids = [m.messageOwner.id for m in activity_obj.messages] real = [i for i in ids if i > 0] below = [i for i in ids[:index] if i > 0] @@ -1816,6 +1823,110 @@ def main(argv=None) -> int: f"ids={ids}", ) + # Bug: every saved summary appeared stacked in a row at the bottom of + # the chat. Cards whose anchor is outside the loaded window have no + # correct position, and placing them by id order alone collapsed them + # together. They must be skipped until paging brings their anchor in. + narrow = _FakeChatActivity([600, 590, 580]) + stranded = injector.restore_saved_summaries( + narrow, + 0, + -4242, + 0, + [ + summary_db_mod.StoredSummary(21, -4242, 0, 120, "ancient", "", 5, 0), + summary_db_mod.StoredSummary(22, -4242, 0, 130, "old", "", 5, 0), + summary_db_mod.StoredSummary(23, -4242, 0, 590, "loaded", "", 5, 0), + ], + ) + narrow_ids = [m.messageOwner.id for m in narrow.messages] + injected = [i for i in narrow_ids if i < 0] + check( + "summaries whose anchor is not loaded are not injected at all", + stranded == 1 and injected == [injector.virtual_id_for(23, 590)], + f"dispatched={stranded} injected={injected} ids={narrow_ids}", + ) + check( + "the one restorable summary still lands directly above its anchor", + narrow_ids.index(injector.virtual_id_for(23, 590)) == narrow_ids.index(590) - 1, + f"ids={narrow_ids}", + ) + + # Once paging widens the window, the previously skipped summary has a + # real anchor and must be restored on the next attempt. + for older in (130, 120): + owner_obj = _TLObj("TL_message") + owner_obj.id = older + narrow.messages.add(_FakeMessageObject(0, owner_obj)) + after_paging = injector.restore_saved_summaries( + narrow, + 0, + -4242, + 0, + [ + summary_db_mod.StoredSummary(21, -4242, 0, 120, "ancient", "", 5, 0), + summary_db_mod.StoredSummary(22, -4242, 0, 130, "old", "", 5, 0), + summary_db_mod.StoredSummary(23, -4242, 0, 590, "loaded", "", 5, 0), + ], + ) + paged_ids = [m.messageOwner.id for m in narrow.messages] + check( + "summaries are restored once paging loads their anchor", + after_paging == 2 + and paged_ids.index(injector.virtual_id_for(21, 120)) == paged_ids.index(120) - 1 + and paged_ids.index(injector.virtual_id_for(22, 130)) == paged_ids.index(130) - 1, + f"dispatched={after_paging} ids={paged_ids}", + ) + check( + "re-restoring does not duplicate the summary already on screen", + paged_ids.count(injector.virtual_id_for(23, 590)) == 1, + f"ids={paged_ids}", + ) + + # Bug: the card appeared after the wrong message when many messages + # were included. The insert index was computed on the calling thread and + # applied later on the UI thread; anything arriving in between shifted + # the list and invalidated it. A fake dispatcher that mutates the list + # between computing and applying reproduces that window. + racing = _FakeChatActivity([700, 690, 680]) + real_on_ui = injector._on_ui + + def _racing_on_ui(fn): + owner_obj = _TLObj("TL_message") + owner_obj.id = 701 + racing.messages.add(0, _FakeMessageObject(0, owner_obj)) + fn() + + injector._on_ui = _racing_on_ui + try: + racing_injection = injector.SummaryInjection( + racing, 0, -4242, 0, 690, injector.virtual_id_for(31, 690) + ) + racing_injection.insert("summary body", "", streaming=False) + finally: + injector._on_ui = real_on_ui + racing_ids = [m.messageOwner.id for m in racing.messages] + card = injector.virtual_id_for(31, 690) + check( + "the card lands above its anchor even if the list shifts first", + racing_ids.index(card) == racing_ids.index(690) - 1, + f"ids={racing_ids}", + ) + + # find_index_for_anchor must report an absent anchor rather than + # guessing index 0 (chat bottom) or a nearest-by-id slot. + absent = _FakeChatActivity([900, 800]) + check( + "an unloaded anchor is reported as absent", + injector.find_index_for_anchor(absent.messages, 123) == -1, + f"index={injector.find_index_for_anchor(absent.messages, 123)}", + ) + check( + "id-ordered placement is still available when explicitly requested", + injector.find_index_for_anchor(absent.messages, 123, require_anchor=False) == 2, + f"index={injector.find_index_for_anchor(absent.messages, 123, require_anchor=False)}", + ) + check( "sys.modules gained no entry for bundled plugin module names", not ({"main", "config", "services", "providers", "hooks", "ui.summarize_dialog"} & new_modules),