feat: add login reminder message for WeChat session activation and im…#18
Merged
feat: add login reminder message for WeChat session activation and im…#18
Conversation
…plement idempotent message deduplication
📝 WalkthroughWalkthroughThis PR introduces a login reminder feature for WeChat channels. It adds a new reminder message function, integrates reminder sends into the channel enable and web login flows, implements message deduplication in the WeChat client, and adds a frontend UI banner to display the reminder to users. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/components/SettingsDialog.vue (1)
37-60:⚠️ Potential issue | 🟡 MinorReset
channelLoginReminderso the banner doesn't stick across sessions.
channelLoginReminderis only cleared by the user clicking ✕ (line 418). If the user closes the dialog with the banner still up, it will reappear next time the dialog opens, even if the channel has since been disconnected or is no longer in the scanning→enabled transition. Clear it alongsidechannelQRContenton close (and/or onchannelLogout) so the banner lifecycle matches the actual connection flow.🩹 Proposed fix
} else { channelQRContent.value = '' + channelLoginReminder.value = false }And in
channelLogout():try { await api.channelLogout() channelState.value = 'none' channelQRContent.value = '' + channelLoginReminder.value = false store.channelEnabled = false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/SettingsDialog.vue` around lines 37 - 60, When the SettingsDialog watcher for props.open closes the dialog it only clears channelQRContent, so channelLoginReminder can persist across dialog reopen; update the watcher (the else branch in the watch(() => props.open, ...)) to also reset channelLoginReminder.value = '' (or false depending on its type), and also clear channelLoginReminder inside the channelLogout() function (the channelLogout method) so the reminder lifecycle is reset both on dialog close and explicit logout.
🧹 Nitpick comments (1)
internal/web/channel.go (1)
56-64: Unnecessary nested goroutine.The outer
go func()(line 46) is already detached and does nothing after this block, so wrapping the twoSendTextcalls in an inner goroutine adds scheduling overhead and makes the flow harder to follow without any concurrency benefit. Inline the sends in the outer goroutine.♻️ Proposed simplification
- // Send welcome and login reminder messages - go func() { - if err := s.wechatClient.SendText(channelpkg.WelcomeMessage(time.Now())); err != nil { - config.Logger().Printf("[wechat] failed to send welcome: %v", err) - } - if err := s.wechatClient.SendText(channelpkg.LoginReminderMessage()); err != nil { - config.Logger().Printf("[wechat] failed to send login reminder: %v", err) - } - }() + // Send welcome and login reminder messages + if err := s.wechatClient.SendText(channelpkg.WelcomeMessage(time.Now())); err != nil { + config.Logger().Printf("[wechat] failed to send welcome: %v", err) + } + if err := s.wechatClient.SendText(channelpkg.LoginReminderMessage()); err != nil { + config.Logger().Printf("[wechat] failed to send login reminder: %v", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/web/channel.go` around lines 56 - 64, The nested go func around the two SendText calls is redundant; remove the inner goroutine and inline the two sends directly in the outer goroutine so you call s.wechatClient.SendText(channelpkg.WelcomeMessage(time.Now())) and s.wechatClient.SendText(channelpkg.LoginReminderMessage()) sequentially, preserving the existing error logging (config.Logger().Printf(...)) for each call and eliminating the extra scheduling overhead introduced by the inner anonymous goroutine.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/pkg/weixin/client.go`:
- Around line 275-286: The current dedup eviction uses map iteration which is
random and can drop recent seqs; replace c.seenSeqs map-only approach with an
insertion-ordered structure (e.g., maintain a slice or container/list alongside
c.seenSeqs) so you can evict the oldest entries deterministically when len
exceeds dedupWindowSize: on insertion of seq (where c.seenSeqs[seq] is set) also
append seq to the ordered queue and, while len(queue) > dedupWindowSize, pop
from the front and delete that key from c.seenSeqs; update any references to
c.seenSeqs and the dedup check around seq to use the map for fast membership and
the queue for eviction to guarantee a true most-recent-N window.
---
Outside diff comments:
In `@web/src/components/SettingsDialog.vue`:
- Around line 37-60: When the SettingsDialog watcher for props.open closes the
dialog it only clears channelQRContent, so channelLoginReminder can persist
across dialog reopen; update the watcher (the else branch in the watch(() =>
props.open, ...)) to also reset channelLoginReminder.value = '' (or false
depending on its type), and also clear channelLoginReminder inside the
channelLogout() function (the channelLogout method) so the reminder lifecycle is
reset both on dialog close and explicit logout.
---
Nitpick comments:
In `@internal/web/channel.go`:
- Around line 56-64: The nested go func around the two SendText calls is
redundant; remove the inner goroutine and inline the two sends directly in the
outer goroutine so you call
s.wechatClient.SendText(channelpkg.WelcomeMessage(time.Now())) and
s.wechatClient.SendText(channelpkg.LoginReminderMessage()) sequentially,
preserving the existing error logging (config.Logger().Printf(...)) for each
call and eliminating the extra scheduling overhead introduced by the inner
anonymous goroutine.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 53111d7b-7ec5-4653-99a6-351025c72ca2
📒 Files selected for processing (5)
internal/channel/messages.gointernal/command/channel.gointernal/pkg/weixin/client.gointernal/web/channel.goweb/src/components/SettingsDialog.vue
Comment on lines
+275
to
+286
| // Mark as seen; evict oldest entries when window is full. | ||
| c.seenSeqs[seq] = struct{}{} | ||
| if len(c.seenSeqs) > dedupWindowSize { | ||
| newSeen := make(map[int]struct{}, dedupWindowSize) | ||
| for k := range c.seenSeqs { | ||
| newSeen[k] = struct{}{} | ||
| if len(newSeen) >= dedupWindowSize/2 { | ||
| break | ||
| } | ||
| } | ||
| c.seenSeqs = newSeen | ||
| } |
There was a problem hiding this comment.
Dedup eviction keeps an arbitrary half, not the most-recent — undermines idempotency.
Go's for k := range c.seenSeqs iterates in randomized order, so the newSeen map retains ~128 keys selected at random. The most-recently-seen Seq values may be dropped, and if the server retransmits one of those Seqs on the next long-poll cycle, the dedup check at line 270 will miss and the message will be delivered a second time — the exact scenario this data structure is meant to prevent.
To preserve a true "most recent N" window, keep insertion order (e.g., with a slice used as a FIFO, or container/list). Sketch:
♻️ Proposed refactor to FIFO eviction
type Client struct {
mu sync.Mutex
state channel.State
creds *Credentials
cancel context.CancelFunc
onMessage func(from, text string)
// idempotent deduplication: track recently processed message Seq values.
- seenSeqs map[int]struct{}
+ seenSeqs map[int]struct{}
+ seqOrder []int // FIFO of Seq insertion order, same length as seenSeqs
} // Mark as seen; evict oldest entries when window is full.
c.seenSeqs[seq] = struct{}{}
- if len(c.seenSeqs) > dedupWindowSize {
- newSeen := make(map[int]struct{}, dedupWindowSize)
- for k := range c.seenSeqs {
- newSeen[k] = struct{}{}
- if len(newSeen) >= dedupWindowSize/2 {
- break
- }
- }
- c.seenSeqs = newSeen
- }
+ c.seqOrder = append(c.seqOrder, seq)
+ if len(c.seqOrder) > dedupWindowSize {
+ drop := c.seqOrder[0]
+ c.seqOrder = c.seqOrder[1:]
+ delete(c.seenSeqs, drop)
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/pkg/weixin/client.go` around lines 275 - 286, The current dedup
eviction uses map iteration which is random and can drop recent seqs; replace
c.seenSeqs map-only approach with an insertion-ordered structure (e.g., maintain
a slice or container/list alongside c.seenSeqs) so you can evict the oldest
entries deterministically when len exceeds dedupWindowSize: on insertion of seq
(where c.seenSeqs[seq] is set) also append seq to the ordered queue and, while
len(queue) > dedupWindowSize, pop from the front and delete that key from
c.seenSeqs; update any references to c.seenSeqs and the dedup check around seq
to use the map for fast membership and the queue for eviction to guarantee a
true most-recent-N window.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…plement idempotent message deduplication
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes