feat(im): add at-chatter-ids filter to +messages-search#612
Conversation
|
|
github-actions
Bot
added
domain/im
📝 WalkthroughWalkthroughAdds a new CLI flag Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #612 +/- ##
==========================================
+ Coverage 62.67% 62.96% +0.28%
==========================================
Files 434 436 +2
Lines 38072 38604 +532
==========================================
+ Hits 23862 24306 +444
- Misses 12076 12150 +74
- Partials 2134 2148 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@5f1d157306d2a036fc6dc2f63076155cf712b398🧩 Skill updatenpx skills add arnold9672/cli#feat/im-messages-search-at-chatter-ids -y -g |
Add --at-chatter-ids flag to shortcuts/im/im_messages_search.go that passes filter.at_chatter_ids to the search API, restricting results to messages that @mention any of the given user open_ids. Messages that @ALL are included automatically. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
arnold9672
force-pushed
the
feat/im-messages-search-at-chatter-ids
branch
from
3b5dfea to
f618c96
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/im/coverage_additional_test.go (1)
405-432: LGTM — good coverage of happy path and validation error.The two subtests cleanly exercise the new
--at-chatter-idsparsing/validation path: the happy case also implicitly asserts thatcommon.SplitCSVtrims the whitespace in"ou_a, ou_b"down to"ou_a"/"ou_b", and the error case pins the exact error substring emitted bycommon.ValidateUserID. UsingnewMessagesSearchTestRuntimeContextis the right call here since it registerspage-size/page-limitas Int flags (per the fix-up commit).One optional thought: if the PR description's claim that "messages that
@allare also matched" implies any client-side handling (e.g., accepting a literalall/@alltoken in the CSV and translating it), a third subtest would guard that contract. If@allinclusion is purely server-side, feel free to ignore.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/im/coverage_additional_test.go` around lines 405 - 432, Add an optional third subtest to assert how the client should handle an "@all" token: create a test named like "at-chatter-ids accepts `@all` token" that uses newMessagesSearchTestRuntimeContext with "at-chatter-ids": "@all", call buildMessagesSearchRequest, and then assert the resulting request body filter["at_chatter_ids"] contains the expected value (e.g., the literal "all" or "@all") if client-side translation is required; if "@all" must be left for server-side handling instead, add a test that ensures the client does not translate it (and that common.SplitCSV/common.ValidateUserID do not reject it) or skip adding the test if server-only behavior is intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@shortcuts/im/coverage_additional_test.go`:
- Around line 405-432: Add an optional third subtest to assert how the client
should handle an "@all" token: create a test named like "at-chatter-ids accepts
`@all` token" that uses newMessagesSearchTestRuntimeContext with "at-chatter-ids":
"@all", call buildMessagesSearchRequest, and then assert the resulting request
body filter["at_chatter_ids"] contains the expected value (e.g., the literal
"all" or "@all") if client-side translation is required; if "@all" must be left
for server-side handling instead, add a test that ensures the client does not
translate it (and that common.SplitCSV/common.ValidateUserID do not reject it)
or skip adding the test if server-only behavior is intended.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b504b89-c395-408e-bd9f-5955b1ab54ec
📒 Files selected for processing (1)
shortcuts/im/coverage_additional_test.go
newTestRuntimeContext does not register page-size as an int flag, causing buildMessagesSearchRequest to fail validation. Switch to newMessagesSearchTestRuntimeContext which registers page-size/page-limit as Int flags with defaults. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
arnold9672
force-pushed
the
feat/im-messages-search-at-chatter-ids
branch
from
72c6e2e to
5f1d157
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/im/coverage_additional_test.go (1)
405-422: LGTM — positive case correctly exercises CSV trimming.The input
"ou_a, ou_b"(with a leading space onou_b) combined with the expected[]string{"ou_a", "ou_b"}is a nice touch: it locks in that the parser trims whitespace around CSV tokens, which is easy to regress.One optional tightening: rather than only asserting on
filter["at_chatter_ids"], you couldreflect.DeepEqualagainst the full expectedmessagesSearchRequest(as the sibling "valid request" sub-test does on Lines 357–381). That would also catch accidental leakage of unrelated filter fields when only--queryand--at-chatter-idsare set.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/im/coverage_additional_test.go` around lines 405 - 422, The test only asserts the at_chatter_ids slice from the returned request; change it to construct the full expected messagesSearchRequest and compare it with the actual result from buildMessagesSearchRequest using reflect.DeepEqual (like the "valid request" sub-test does). Locate the sub-test named "at-chatter-ids accepts user ids", use buildMessagesSearchRequest to get the actual, build an expected messagesSearchRequest value with Query "standup" and Filter.AtChatterIDs []string{"ou_a","ou_b"} (and other default/zero fields), then replace the single-field assertion on filter["at_chatter_ids"] with a DeepEqual assertion comparing the entire messagesSearchRequest.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@shortcuts/im/coverage_additional_test.go`:
- Around line 405-422: The test only asserts the at_chatter_ids slice from the
returned request; change it to construct the full expected messagesSearchRequest
and compare it with the actual result from buildMessagesSearchRequest using
reflect.DeepEqual (like the "valid request" sub-test does). Locate the sub-test
named "at-chatter-ids accepts user ids", use buildMessagesSearchRequest to get
the actual, build an expected messagesSearchRequest value with Query "standup"
and Filter.AtChatterIDs []string{"ou_a","ou_b"} (and other default/zero fields),
then replace the single-field assertion on filter["at_chatter_ids"] with a
DeepEqual assertion comparing the entire messagesSearchRequest.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 53a417f7-99a2-48cc-bb27-4fcb1ff859ae
📒 Files selected for processing (3)
coverage.txtdownload.htmlshortcuts/im/coverage_additional_test.go
3 participants
Summary
Add a new filter
--at-chatter-idsto theim +messages-searchshortcut so users can narrow search results to messages that@mentionspecific users. Matched results naturally include messages that@all.Changes
shortcuts/im/im_messages_search.go: register the--at-chatter-idsflag; parse comma-separatedou_*user IDs, validate viacommon.ValidateUserID, and pass asfilter.at_chatter_idsin the search bodyshortcuts/im/coverage_additional_test.go: add two table-driven sub-tests (valid IDs populate the filter; malformed ID returns a validation error)shortcuts/im/im_messages_search_execute_test.go: register the new flag in the execute-test runtime helperskills/lark-im/references/lark-im-messages-search.md: document the flag and add a usage exampleTest Plan
make unit-testpasses locallylark-cli im +messages-search --query "release" --at-chatter-ids ou_xxx,ou_yyy— confirm the request payload in--dry-runincludesfilter.at_chatter_ids: [ou_xxx, ou_yyy]lark-cli im +messages-search --at-chatter-ids bad_id— confirm it surfacesinvalid user ID formatRelated Issues
N/A
Summary by CodeRabbit
New Features
Documentation
Tests