feat: update memo when its source message is edited - #208
Conversation
Tracks which memo a Telegram message created, keyed by message ID. Needed to support updating a memo when the source message is later edited. Persisted the same way as the existing user access token map: a flat file, atomic rename on write, in-memory sync.Map cache.
Currently editing a message you sent to the bot triggers an error: the update arrives with Message == nil (EditedMessage is set instead), which fails the existing nil check and sends "invalid message structure: missing required fields" back to the user. Route EditedMessage updates to a dedicated handler before that check. It looks up the memo the original message created and calls UpdateMemo with the new content. No reply is sent on success, matching the existing pattern where Telegram already marks the message as edited client-side.
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
WalkthroughThe handler detects edited Telegram messages before normal validation, validates access and message mappings, converts edited text or captions, and updates the associated memo. Newly created memos record chat and message IDs. The Store persists and reloads chat-scoped mappings, with tests covering persistence, missing mappings, and duplicate message IDs across chats. Documentation describes the editing behavior. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
README.md (1)
118-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant wording: "originally created".
✏️ Proposed wording fix
-- Edit a sent message: Update the memo that message originally created. Edits to messages Memogram never captured (for example, messages sent before this feature was available) are ignored. +- Edit a sent message: Update the memo that message created. Edits to messages Memogram never captured (for example, messages sent before this feature was available) are ignored.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` at line 118, Remove the redundant wording “originally created” from the README description of editing a sent message, while preserving the existing meaning about updating the message’s memo and ignoring uncaptured edits.Source: Linters/SAST tools
memogram.go (1)
211-219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate content-extraction logic between
handler()andeditedMessageHandler.Both blocks pick
Text/Entities, override withCaption/CaptionEntities, then runformatContent— identical logic. Extract into a shared helper, e.g.extractContent(text, caption string, textEntities, captionEntities []models.MessageEntity) string, to avoid future drift between the create and edit paths.Also applies to: 367-375
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@memogram.go` around lines 211 - 219, Extract the duplicated text/caption selection and formatting logic from handler() and editedMessageHandler into a shared extractContent helper accepting text, caption, textEntities, and captionEntities. Replace both inline blocks with calls to this helper, preserving the existing preference for captions and applying formatContent only when entities are present.store/message.go (3)
68-97: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant file open when creating a fresh
.messagesfile.When the file doesn't exist, it's created via
os.Create(withClosedeferred to the end of the function) and then immediately reopened viaos.Opena few lines later — two file handles to the same freshly-created (empty) file. Since a newly created file has nothing to scan, you can just returnnilright after creating it.♻️ Proposed simplification
func (s *Store) loadMessageMemoMapFromFile() error { dataPath := s.messageMemoMapDataPath() if _, err := os.Stat(dataPath); os.IsNotExist(err) { file, err := os.Create(dataPath) if err != nil { return err } - defer file.Close() + return file.Close() } file, err := os.Open(dataPath)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@store/message.go` around lines 68 - 97, Update loadMessageMemoMapFromFile so the os.IsNotExist branch returns nil immediately after successfully creating and closing the new file, avoiding the subsequent os.Open on an empty file. Preserve the existing error return when os.Create fails and the normal scanning flow for files that already exist.
22-27: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftUnbounded growth + synchronous full-file rewrite on every message.
Every
SetMemoForMessagecall re-snapshots the entire map, sorts it, and rewrites the whole.messagesfile — and entries are never pruned. Since Telegram only allows editing messages within a limited time window, retaining every mapping forever means both the in-memorysync.Mapand the on-disk file grow indefinitely, and each write's cost scales with the total number of memos ever created by the service's lifetime.Consider either capping/evicting old entries (e.g., LRU by insertion order or by tracking last-write time) or batching/debouncing the persistence instead of doing a full synchronous rewrite per message.
Also applies to: 33-66
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@store/message.go` around lines 22 - 27, The SetMemoForMessage persistence flow causes unbounded memo retention and rewrites the complete memo file synchronously on every update. Update SetMemoForMessage and saveMessageMemoMapToFile to bound or evict stale entries and batch/debounce persistence so each message update does not trigger an immediate full-file rewrite, while preserving memo lookup and persistence behavior for retained entries.
41-82: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnchecked error returns flagged by golangci-lint (errcheck).
Lines 41, 46, 51, 55, 75, 82 ignore returns from
os.Remove/file.Close()/tmpFile.Close(). Most are benign defer/error-path cleanups, but consider at least logging them for observability (e.g., a failedos.Removeleaving stray temp files behind).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@store/message.go` around lines 41 - 82, Handle the ignored cleanup errors in the temporary-file write flow and loadMessageMemoMapFromFile: wrap deferred os.Remove and file.Close calls so their errors are checked and logged or otherwise propagated appropriately. Preserve the existing primary error returns, while ensuring cleanup failures—especially removal of temporary files—are observable and satisfy errcheck.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@store/message.go`:
- Around line 14-27: Change the message memo cache from messageID-only keys to a
composite (Chat.ID, Message.ID) key across GetMemoForMessage and
SetMemoForMessage, preserving the existing persisted file format while updating
serialization/deserialization accordingly. Update memogram.go call sites and
edit handling to pass both chat and message identifiers, ensuring identical
message IDs in different Telegram chats remain independent.
---
Nitpick comments:
In `@memogram.go`:
- Around line 211-219: Extract the duplicated text/caption selection and
formatting logic from handler() and editedMessageHandler into a shared
extractContent helper accepting text, caption, textEntities, and
captionEntities. Replace both inline blocks with calls to this helper,
preserving the existing preference for captions and applying formatContent only
when entities are present.
In `@README.md`:
- Line 118: Remove the redundant wording “originally created” from the README
description of editing a sent message, while preserving the existing meaning
about updating the message’s memo and ignoring uncaptured edits.
In `@store/message.go`:
- Around line 68-97: Update loadMessageMemoMapFromFile so the os.IsNotExist
branch returns nil immediately after successfully creating and closing the new
file, avoiding the subsequent os.Open on an empty file. Preserve the existing
error return when os.Create fails and the normal scanning flow for files that
already exist.
- Around line 22-27: The SetMemoForMessage persistence flow causes unbounded
memo retention and rewrites the complete memo file synchronously on every
update. Update SetMemoForMessage and saveMessageMemoMapToFile to bound or evict
stale entries and batch/debounce persistence so each message update does not
trigger an immediate full-file rewrite, while preserving memo lookup and
persistence behavior for retained entries.
- Around line 41-82: Handle the ignored cleanup errors in the temporary-file
write flow and loadMessageMemoMapFromFile: wrap deferred os.Remove and
file.Close calls so their errors are checked and logged or otherwise propagated
appropriately. Preserve the existing primary error returns, while ensuring
cleanup failures—especially removal of temporary files—are observable and
satisfy errcheck.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1efbf941-993a-4ea5-aa16-50bd8ef36e48
📒 Files selected for processing (5)
README.mdmemogram.gostore/message.gostore/message_test.gostore/store.go
nord-winter
changed the title
Telegram message IDs are only unique within a chat, not globally. The mapping was keyed by message ID alone, so two chats with a colliding message ID (very likely: IDs start low per chat, and the bot supports multiple users via ALLOWED_USERNAMES) could silently overwrite each other's entry, and an edit in one chat could update the wrong memo. Key the cache and persisted file by (chat ID, message ID) instead.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
memogram.go (1)
356-360: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReapply the allowlist before accepting an edit.
A sender with a cached token and existing mapping can still change a memo after being removed from the allowlist (or changing usernames). The normal-message path rejects this at Lines 183-191; apply the same
isUserAllowedgate here before looking up the token.Proposed fix
if message == nil || message.From == nil || message.Chat.ID == 0 { return } +if !s.isUserAllowed(message.From.Username) { + return +} + userID := message.From.ID accessToken, ok := s.store.GetUserAccessToken(userID)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@memogram.go` around lines 356 - 360, Update the edit-handling path around GetUserAccessToken to call isUserAllowed for the current sender before looking up or accepting the cached access token. Return immediately when the sender is no longer allowed, preserving the existing token and edit flow for allowed users.
🧹 Nitpick comments (1)
store/message_test.go (1)
48-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReload the same-ID/different-chat case.
This verifies in-memory isolation only. Recreate the store before lookup so the test also covers the new
chatID:messageID:memoNamepersistence format.Proposed test adjustment
store.SetMemoForMessage(1, 5, "memos/chat-one") store.SetMemoForMessage(2, 5, "memos/chat-two") -memoName, ok := store.GetMemoForMessage(1, 5) +reloaded := NewStore(dataPath) +if err := reloaded.Init(); err != nil { + t.Fatalf("init reloaded store: %v", err) +} + +memoName, ok := reloaded.GetMemoForMessage(1, 5) // ... -memoName, ok = store.GetMemoForMessage(2, 5) +memoName, ok = reloaded.GetMemoForMessage(2, 5)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@store/message_test.go` around lines 48 - 70, Update TestMessageMemoMapScopedByChat to recreate or reload the Store after setting both memos and before calling GetMemoForMessage. Reinitialize the new store with the same dataPath, then verify both chat/message lookups still return their respective memo names, covering persistence of the chatID:messageID:memoName format.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@store/message.go`:
- Around line 29-32: Update SetMemoForMessage to serialize the
messageMemoCache.Store mutation together with saveMessageMemoMapToFile using the
store’s existing synchronization mechanism. Hold the lock across both operations
so concurrent calls cannot persist snapshots out of order, while preserving the
existing error logging behavior.
---
Outside diff comments:
In `@memogram.go`:
- Around line 356-360: Update the edit-handling path around GetUserAccessToken
to call isUserAllowed for the current sender before looking up or accepting the
cached access token. Return immediately when the sender is no longer allowed,
preserving the existing token and edit flow for allowed users.
---
Nitpick comments:
In `@store/message_test.go`:
- Around line 48-70: Update TestMessageMemoMapScopedByChat to recreate or reload
the Store after setting both memos and before calling GetMemoForMessage.
Reinitialize the new store with the same dataPath, then verify both chat/message
lookups still return their respective memo names, covering persistence of the
chatID:messageID:memoName format.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b38238da-20ff-42ef-a75b-f01249fd4498
📒 Files selected for processing (3)
memogram.gostore/message.gostore/message_test.go
editedMessageHandler accepted edits from anyone with a cached access token, skipping the isUserAllowed check the normal message path applies. A user removed from ALLOWED_USERNAMES (or renamed) could still edit memos through a stale cached token. Also serialize the cache-store-then-file-save sequence in SetMemoForMessage and SetUserAccessToken with a mutex: without it, concurrent writes can persist an older snapshot after a newer one, losing the newer entry across a restart. TestMessageMemoMapScopedByChat now reloads the store from disk to cover the persisted format, not just the in-memory cache.
1 participant
Closes #207