feat: optimize event subscription precheck, links, and consumer guard#1447
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds SubscriptionType and SingleConsumer support to event keys, enforces single-consumer exclusivity at handshake with rejection plumbing, extends console URL generation with launcher addons deep-link encoding for remediation hints, fetches app callback subscriptions via a new API, and updates preflight checks to validate callback subscriptions against the ledger and produce launcher-based hint URLs. ChangesEvent Callback and Exclusive Consumer Feature
Sequence Diagram(s)sequenceDiagram
participant Client
participant Bus
participant Hub
participant Consumer
Client->>Bus: HelloFrame (SingleConsumer event key)
Bus->>Hub: TryRegisterExclusive(subscriptionID)
alt First subscriber
Hub-->>Bus: (true, "")
Bus->>Client: HelloAck (accepted)
Client->>Consumer: start consuming
else Existing subscriber
Hub-->>Bus: (false, "already running PID=12345")
Bus->>Client: HelloAckRejected (reason)
Bus->>Bus: close connection
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
the
size/L
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1447 +/- ##
==========================================
+ Coverage 73.35% 73.42% +0.06%
==========================================
Files 750 753 +3
Lines 69264 70111 +847
==========================================
+ Hits 50811 51476 +665
- Misses 14713 14826 +113
- Partials 3740 3809 +69 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@03d07207aa36b39db620fdf0cc14f751512dfb4d🧩 Skill updatenpx skills add larksuite/cli#feat/event-subscription-optimize -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 (2)
internal/appmeta/app_callbacks_test.go (1)
24-50: ⚡ Quick winAdd edge/error-path coverage for
FetchSubscribedCallbacks.Current tests only cover success + missing
callback_info. Please add cases for:
CallAPIreturning error,- malformed JSON (decode error),
callback_infopresent with missing/nullsubscribed_callbacks(to lock sentinel behavior).As per coding guidelines, 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 `@internal/appmeta/app_callbacks_test.go` around lines 24 - 50, Add unit tests for FetchSubscribedCallbacks to cover the missing edge/error paths: (1) a test that simulates CallAPI returning an error (e.g., create fakeCallbackClient that returns an error and assert FetchSubscribedCallbacks returns that error), (2) a test for malformed JSON (pass raw like "`{bad json`" and assert a decode/unmarshal error is returned), and (3) a test where callback_info exists but subscribed_callbacks is missing or null (e.g., raw with `"callback_info":{}` and another with `"subscribed_callbacks":null`) and assert the function returns the sentinel nil slice/behavior currently expected; add these as new TestFetchSubscribedCallbacks_CallAPIError, TestFetchSubscribedCallbacks_MalformedJSON, and TestFetchSubscribedCallbacks_NullSubscribedCallbacks using the same test patterns as existing tests and the fakeCallbackClient/fetch call to FetchSubscribedCallbacks.Source: Coding guidelines
internal/event/registry_test.go (1)
289-301: ⚡ Quick winAssert the panic reason in the invalid-subtype test.
This currently passes on any panic, including unrelated ones. Reusing
mustPanicwith a SubscriptionType-specific substring makes the test deterministic for this behavior.Suggested test tightening
func TestRegisterKey_InvalidSubscriptionTypePanics(t *testing.T) { - defer func() { - if r := recover(); r == nil { - t.Errorf("expected panic for invalid SubscriptionType") - } - }() + defer mustPanic(t, "SubscriptionType must be") RegisterKey(KeyDefinition{ Key: "test.subtype.bogus", EventType: "test.subtype.bogus", SubscriptionType: "bogus", Schema: SchemaDef{Native: &SchemaSpec{Raw: []byte(`{"type":"object"}`)}}, }) }🤖 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 `@internal/event/registry_test.go` around lines 289 - 301, The test TestRegisterKey_InvalidSubscriptionTypePanics should assert the panic message to ensure it fails only for the expected reason; replace the current defer/recover pattern with a call to the existing mustPanic test helper (invoking RegisterKey with the bogus SubscriptionType) and assert that the returned panic string contains a SubscriptionType-specific substring (e.g., "SubscriptionType"); this uses mustPanic and the RegisterKey call so the test only passes when the panic is about invalid subscription type.
🤖 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 `@internal/appmeta/app_callbacks.go`:
- Around line 31-37: The function FetchSubscribedCallbacks currently returns
envelope.Data.App.CallbackInfo.SubscribedCallbacks directly, which can be nil
when "subscribed_callbacks" is omitted or null; change FetchSubscribedCallbacks
so when envelope.Data.App.CallbackInfo != nil it returns a non-nil empty slice
if SubscribedCallbacks is nil (e.g., replace direct return with a nil-check and
return []string{} when needed) so preflightEventTypes and pf.subscribedCallbacks
treat the presence of callback_info properly; also add unit tests exercising
FetchSubscribedCallbacks for JSON with "subscribed_callbacks": null and with the
field omitted to assert the returned slice is non-nil (empty) and that
preflightEventTypes enforces missing-callback checks accordingly.
In `@internal/event/consume/reject_test.go`:
- Around line 20-26: The test currently only checks prob.Subtype and message
text; update it to assert full typed error metadata: after calling prob, assert
prob.Category equals the expected category (e.g., errs.CategoryInvalidArgument
or the repo-specific category for this path) in addition to prob.Subtype ==
errs.SubtypeFailedPrecondition, and use errors.As(err, &ve) with ve typed as
*errs.ValidationError to assert the ValidationError.Param equals the expected
param name for this rejection; keep the existing message assertion only as a
supplement, not the primary contract.
---
Nitpick comments:
In `@internal/appmeta/app_callbacks_test.go`:
- Around line 24-50: Add unit tests for FetchSubscribedCallbacks to cover the
missing edge/error paths: (1) a test that simulates CallAPI returning an error
(e.g., create fakeCallbackClient that returns an error and assert
FetchSubscribedCallbacks returns that error), (2) a test for malformed JSON
(pass raw like "`{bad json`" and assert a decode/unmarshal error is returned),
and (3) a test where callback_info exists but subscribed_callbacks is missing or
null (e.g., raw with `"callback_info":{}` and another with
`"subscribed_callbacks":null`) and assert the function returns the sentinel nil
slice/behavior currently expected; add these as new
TestFetchSubscribedCallbacks_CallAPIError,
TestFetchSubscribedCallbacks_MalformedJSON, and
TestFetchSubscribedCallbacks_NullSubscribedCallbacks using the same test
patterns as existing tests and the fakeCallbackClient/fetch call to
FetchSubscribedCallbacks.
In `@internal/event/registry_test.go`:
- Around line 289-301: The test TestRegisterKey_InvalidSubscriptionTypePanics
should assert the panic message to ensure it fails only for the expected reason;
replace the current defer/recover pattern with a call to the existing mustPanic
test helper (invoking RegisterKey with the bogus SubscriptionType) and assert
that the returned panic string contains a SubscriptionType-specific substring
(e.g., "SubscriptionType"); this uses mustPanic and the RegisterKey call so the
test only passes when the panic is about invalid subscription type.
🪄 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: 43b5abd5-181b-4b5a-9d4e-ab627f29ef67
📒 Files selected for processing (18)
cmd/event/appmeta_err.gocmd/event/console_url.gocmd/event/console_url_test.gocmd/event/consume.gocmd/event/preflight_test.gointernal/appmeta/app_callbacks.gointernal/appmeta/app_callbacks_test.gointernal/event/bus/bus.gointernal/event/bus/handle_hello_test.gointernal/event/bus/hub.gointernal/event/bus/hub_test.gointernal/event/consume/consume.gointernal/event/consume/reject_test.gointernal/event/protocol/messages.gointernal/event/protocol/messages_test.gointernal/event/registry.gointernal/event/registry_test.gointernal/event/types.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/appmeta/app_callbacks_test.go`:
- Around line 44-59: Test coverage is missing for the edge cases where
callback_info exists but subscribed_callbacks is null or omitted; add two unit
tests to app_callbacks_test.go —
TestFetchSubscribedCallbacks_CallbackInfoPresentButNull and
TestFetchSubscribedCallbacks_CallbackInfoPresentButOmitted — that call
FetchSubscribedCallbacks with fakeCallbackClient raw responses reflecting (1)
callback_info with "subscribed_callbacks":null and (2) callback_info present but
without a subscribed_callbacks field, and assert no error, that the returned
slice is non-nil and has length 0 (mirroring the existing
TestFetchSubscribedCallbacks_NoCallbackInfo assertions).
🪄 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: 8b572df0-e0a4-4165-b549-929446e421de
📒 Files selected for processing (3)
cmd/event/preflight_test.gointernal/appmeta/app_callbacks.gointernal/appmeta/app_callbacks_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/event/preflight_test.go
liangshuo-1
approved these changes
Jun 16, 2026
This was referenced Jul 23, 2026
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
2 participants
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.
Summary
为 event 订阅增加订阅类型标识、回调订阅 precheck、扫码自动开通链接,并支持 per-EventKey 唯一 consumer 限制。本次为基础设施改造,现有事件订阅类 EventKey 行为不变(SubscriptionType 默认 event、SingleConsumer 默认 false)。
Changes
SubscriptionType(event/callback, empty normalized to event) andSingleConsumerfields toKeyDefinitionininternal/event/types.go; normalize/validate ininternal/event/registry.goappmeta.FetchSubscribedCallbacks(internal/appmeta/app_callbacks.go) callingapplication/get; route callback-type precheck againstsubscribed_callbacksincmd/event/consume.gocmd/event/console_url.go(ManifestAddons→ gzip → base64url, brand host viacore.ResolveEndpoints), filling scopes/events/callbacks by identitySingleConsumerkeys viaHub.TryRegisterExclusive(internal/event/bus/hub.go) at the bus handshake (internal/event/bus/bus.go), surfaced throughHelloAck.Rejected(internal/event/protocol/messages.go) as afailed_preconditionerror ininternal/event/consume/consume.goTest Plan
tests/cli_e2e/event3/3 passed (lite mode adds no new sandbox E2E)lark-cli event consume im.message.recieve_v1→ exit 2 typedvalidation/invalid_argument; addons URL generation/decoding observed (brand-aware, valid base64url → gzip → JSON matching the addons schema)SingleConsumersecond consumer rejected (exit 2); precheck-miss emits the scan link; scan link opens correctly on the open platform (open.feishu.cn/page/launcher?clientID=<appID>&addons=...)Related Issues
N/A (internal requirement doc: event 事件优化技术方案)
Summary by CodeRabbit