feat(mail): consume watch events via event framework - #1534
Open
bubbmon233 wants to merge 1 commit into
Open
Conversation
Migrate mail +watch from direct SDK WebSocket handling to the shared event consume framework while preserving the shortcut flags, dry-run, schema output, mailbox/filter params, and output modes. Add mail.user_mailbox.event.message_received_v1 EventKey with mailbox SubscriptionKey, NormalizeParams, Match, Process, and PreConsume subscribe/unsubscribe lifecycle. Document event consume parity and output-dir filename differences. contract divergence: type=cli prelude references larksuite-cli-registry meta refresh, but this sprint is scoped to github.com/larksuite/cli and explicitly forbids Meta/registry changes; no meta-gen was run. sprint: S1
📝 WalkthroughWalkthroughThe PR migrates the ChangesMail watch event-consume migration
Sequence Diagram(s)sequenceDiagram
participant User as User / CLI
participant MailWatch as MailWatch.Execute
participant consume.Run as consume.Run
participant PreConsume as mailboxEventPreConsume
participant SubscribeAPI as POST .../event/subscribe
participant normalizeWatchParams
participant ProfileAPI as GET mailbox profile
participant matchWatchMailbox
participant processWatchEvent
participant MessageAPI as GET message
User->>MailWatch: flags (mailbox, format, msg_format, labels, folders)
MailWatch->>consume.Run: Options{EventKey, Params, Runtime}
consume.Run->>PreConsume: invoke pre-consume hook
PreConsume->>SubscribeAPI: POST subscribe (event_type:1)
PreConsume-->>consume.Run: cleanup func
consume.Run->>normalizeWatchParams: resolve mailbox + filter IDs
normalizeWatchParams->>ProfileAPI: fetch primary_email_address
normalizeWatchParams-->>consume.Run: mailbox_api, mailbox_email, label_ids, folder_ids
loop each incoming event
consume.Run->>matchWatchMailbox: compare event mail_address to mailbox_email
matchWatchMailbox-->>consume.Run: match / drop
consume.Run->>processWatchEvent: extract, fetch, filter, marshal
processWatchEvent->>MessageAPI: GET message (when msg_format != event)
processWatchEvent-->>consume.Run: JSON output or nil (filtered)
end
consume.Run->>PreConsume: invoke cleanup
PreConsume->>SubscribeAPI: POST unsubscribe
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
github-actions
Bot
added
domain/mail
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@28b543c16ee3a6ad5f3b33572b07101bcadc7c3e🧩 Skill updatenpx skills add bubbmon233/cli#feat/97a3db0 -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
events/mail/preconsume_test.go (1)
11-30: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd typed error-path tests for pre-consume failures.
This test only covers success flow. Please add failure-path cases (e.g., subscribe failure and nil runtime) and assert typed metadata (
category/subtype) plus cause preservation, so the new error-classification behavior is regression-safe.As per coding guidelines, "
**/*_test.go: Error-path tests must assert typed metadata viaerrs.ProblemOf(category/subtype/param) and cause preservation, not message substrings alone" and "Every behavior change needs a test alongside the change`."🤖 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 `@events/mail/preconsume_test.go` around lines 11 - 30, The TestMailboxEventPreConsumeLifecycle test only covers the success path. Add additional test cases to verify failure scenarios including when the subscribe API call fails and when the runtime is nil, using errs.ProblemOf to assert the typed error metadata fields (category and subtype) and verify cause preservation rather than just checking error message substrings. These test cases should validate that the mailboxEventPreConsume function properly classifies and preserves errors in its error-path behavior.Source: Coding guidelines
🤖 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 `@events/mail/api.go`:
- Around line 96-99: The resolveNamesByAPI function is being called
unconditionally even when remainingNames is empty, which causes unnecessary API
calls and can fail watch startup on network issues. Add a conditional check to
skip the resolveNamesByAPI call when remainingNames is empty, so the remote
name-resolution is only attempted when there are actually names to resolve.
In `@shortcuts/mail/mail_watch.go`:
- Around line 231-258: The CallAPI method in mailWatchEventRuntime is currently
ignoring the incoming context parameter (note the underscore in the method
signature) and not propagating it to the DoAPI call. To fix this, accept the
context parameter properly in the CallAPI method signature (remove the
underscore), and then pass this context to the DoAPI call through the ApiReq
struct or as a parameter, ensuring that cancellation signals and deadlines from
the caller are properly honored for these API requests.
---
Nitpick comments:
In `@events/mail/preconsume_test.go`:
- Around line 11-30: The TestMailboxEventPreConsumeLifecycle test only covers
the success path. Add additional test cases to verify failure scenarios
including when the subscribe API call fails and when the runtime is nil, using
errs.ProblemOf to assert the typed error metadata fields (category and subtype)
and verify cause preservation rather than just checking error message
substrings. These test cases should validate that the mailboxEventPreConsume
function properly classifies and preserves errors in its error-path behavior.
🪄 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: b06a9c20-dc60-4dc5-b60a-76b75db5bb46
📒 Files selected for processing (12)
events/mail/api.goevents/mail/preconsume.goevents/mail/preconsume_test.goevents/mail/register.goevents/mail/watch.goevents/mail/watch_test.goevents/register.goshortcuts/mail/helpers.goshortcuts/mail/mail_watch.goshortcuts/mail/mail_watch_test.goskills/lark-mail/SKILL.mdskills/lark-mail/references/lark-mail-watch.md
Comment on lines
+96
to
+99
| resolvedNames, err := resolveNamesByAPI(ctx, rt, mailboxID, remainingNames, kind) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Skip remote name-resolution calls when there are no names to resolve.
resolveWatchFilterIDs calls resolveNamesByAPI even when remainingNames is empty. This can fail watch startup on network/API issues despite users supplying only explicit IDs.
♻️ Proposed fix
- resolvedNames, err := resolveNamesByAPI(ctx, rt, mailboxID, remainingNames, kind)
+ if len(remainingNames) == 0 {
+ return sortedSetKeys(set), nil
+ }
+ resolvedNames, err := resolveNamesByAPI(ctx, rt, mailboxID, remainingNames, kind)
if err != nil {
return nil, err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| resolvedNames, err := resolveNamesByAPI(ctx, rt, mailboxID, remainingNames, kind) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if len(remainingNames) == 0 { | |
| return sortedSetKeys(set), nil | |
| } | |
| resolvedNames, err := resolveNamesByAPI(ctx, rt, mailboxID, remainingNames, kind) | |
| if err != nil { | |
| return nil, err | |
| } |
🤖 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 `@events/mail/api.go` around lines 96 - 99, The resolveNamesByAPI function is
being called unconditionally even when remainingNames is empty, which causes
unnecessary API calls and can fail watch startup on network issues. Add a
conditional check to skip the resolveNamesByAPI call when remainingNames is
empty, so the remote name-resolution is only attempted when there are actually
names to resolve.
Comment on lines
+231
to
+258
| func (r *mailWatchEventRuntime) CallAPI(_ context.Context, method, path string, body interface{}) (json.RawMessage, error) { | ||
| apiPath := path | ||
| query := larkcore.QueryParams{} | ||
| if u, err := url.Parse(path); err == nil && u.RawQuery != "" { | ||
| apiPath = u.Path | ||
| for k, values := range u.Query() { | ||
| for _, value := range values { | ||
| query.Add(k, value) | ||
| } | ||
| handleEvent(eventData) | ||
| return nil | ||
| } | ||
| } | ||
| req := &larkcore.ApiReq{ | ||
| HttpMethod: method, | ||
| ApiPath: apiPath, | ||
| QueryParams: query, | ||
| } | ||
| if body != nil { | ||
| req.Body = body | ||
| } | ||
| resp, err := r.runtime.DoAPI(req) | ||
| if err != nil { | ||
| return nil, client.WrapDoAPIError(err) | ||
| } | ||
| if _, err := r.runtime.ClassifyAPIResponse(resp); err != nil { | ||
| return json.RawMessage(resp.RawBody), err | ||
| } | ||
| return json.RawMessage(resp.RawBody), nil | ||
| } |
There was a problem hiding this comment.
Context cancellation is currently dropped in the API adapter.
Line 231 ignores the incoming ctx, and requests are routed through RuntimeContext.DoAPI, which uses the runtime’s stored context instead of the per-call context. That means consume.Run deadlines/cancel signals (including cleanup timeout paths) are not reliably enforced for these API calls.
🤖 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 `@shortcuts/mail/mail_watch.go` around lines 231 - 258, The CallAPI method in
mailWatchEventRuntime is currently ignoring the incoming context parameter (note
the underscore in the method signature) and not propagating it to the DoAPI
call. To fix this, accept the context parameter properly in the CallAPI method
signature (remove the underscore), and then pass this context to the DoAPI call
through the ApiReq struct or as a parameter, ensuring that cancellation signals
and deadlines from the caller are properly honored for these API requests.
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.
Generated by the harness-coding skill.
Sprints
This MR was created autonomously. Quality gates were enforced by the repo's own pre-commit hooks.
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Documentation
--print-output-schemato view output format per configuration.