feat(im/chat-list): support --types flag for listing p2p single chats#1077
feat(im/chat-list): support --types flag for listing p2p single chats#1077shifengjuan-dev wants to merge 1 commit into
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 a ChangesP2P Chat Filtering Feature
Sequence DiagramsequenceDiagram
participant Caller as User/Bot CLI
participant Flags as "--types" flag
participant Validate as ImChatList.Validate
participant Normalize as normalizeTypes
participant Resolve as resolveTypes
participant Request as buildChatListParams
participant API as Lark API
participant Renderer as Renderer
Caller->>Flags: provide --types CSV
Flags->>Validate: parse & validate
Validate->>Normalize: normalizeTypes()
Normalize->>Resolve: normalized list
Resolve->>Resolve: apply identity rules (user passthrough, bot strip p2p)
Resolve-->>Request: effectiveTypes (CSV)
Request->>API: HTTP call (includes types=... only if non-empty)
API-->>Renderer: response (may include chat_mode, p2p_target_*)
Renderer->>Caller: table/JSON output with fields/hints
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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
domain/im
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1077 +/- ##
==========================================
+ Coverage 67.79% 68.05% +0.26%
==========================================
Files 591 601 +10
Lines 55238 55821 +583
==========================================
+ Hits 37449 37990 +541
- Misses 14680 14688 +8
- Partials 3109 3143 +34 ☔ 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@7b2bf99be8602012303a4d0db793c641ec19c2d3🧩 Skill updatenpx skills add shifengjuan-dev/lark-cli#feat/chat-list-types-p2p -y -g |
shifengjuan-dev
force-pushed
the
feat/chat-list-types-p2p
branch
2 times, most recently
from
4a6369a to
c3db76e
Compare
![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)
shortcuts/im/im_chat_list.go (1)
65-103:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMissing
bot_strip_p2pfilter metadata when bot requests mixed types.The bot downgrade is applied, but no
filter.applied="bot_strip_p2p"notice is emitted, so the documented/output contract for downgraded bot requests is not met (and cannot override exclude-muted metadata as specified).Also applies to: 194-213
🤖 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/im/im_chat_list.go` around lines 65 - 103, resolveTypes(runtime) currently returns a second value that is ignored; capture that second return (e.g., downgraded or botStripP2P bool) and use it to emit the proper filter metadata when a bot request is downgraded: change "effective, _ := resolveTypes(runtime)" to capture the flag, and after computing outData and mfOut, if runtime.IsBot() && downgraded then set outData["filter"] = MuteFilterMetaToMap(MuteFilterMeta{Applied:"bot_strip_p2p", ...}) or merge with existing mfOut.Meta so that mfOut.Meta.Applied/"filter" reflects "bot_strip_p2p"; reference resolveTypes, effective, runtime.IsBot(), MaybeApplyMuteFilter, mfOut.Meta.Applied and outData["filter"] when making this change.
🧹 Nitpick comments (1)
shortcuts/im/im_chat_list_test.go (1)
26-57: ⚡ Quick winAlign test runtime setup with repo test guidelines.
The helper builds runtime context without
cmdutil.TestFactory(t, config)and without config-dir isolation viat.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()). Please switch the helper to the standard test factory pattern so all added tests inherit the expected isolation behavior.As per coding guidelines
**/*_test.go: "Usecmdutil.TestFactory(t, config)for test factories in unit tests" and "Uset.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())to isolate config state in tests".🤖 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/im/im_chat_list_test.go` around lines 26 - 57, The helper newChatListTestRuntimeContextWithIdentity currently builds a cobra.Command and sets flags manually; replace that with the repo standard test factory pattern: call t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) to isolate config, create a factory via cmdutil.TestFactory(t, config) (or cmdutil.TestFactory(t, nil) if no extra config), and pass that factory into common.TestNewRuntimeContextWithIdentity instead of constructing cmd directly; ensure you still set the same flags on the returned command/context so existing tests keep the same behavior.
🤖 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 `@skills/lark-im/references/lark-im-chat-list.md`:
- Around line 89-90: Update the doc text for the bot downgrade examples to
reflect the actual runtime behavior: when using the CLI flag combination `--as
bot --types=p2p,group` the service emits metadata
`filter.applied="bot_strip_p2p"` (and you should mention interaction with
`filter-priority`) rather than silently stripping `p2p`; change the example
lines and any related examples in this section to state that metadata is
included in the JSON response and show the proper emitted field
(`filter.applied="bot_strip_p2p"`) and how it affects results; ensure all other
examples in the same section are updated consistently to mention the emitted
metadata.
---
Outside diff comments:
In `@shortcuts/im/im_chat_list.go`:
- Around line 65-103: resolveTypes(runtime) currently returns a second value
that is ignored; capture that second return (e.g., downgraded or botStripP2P
bool) and use it to emit the proper filter metadata when a bot request is
downgraded: change "effective, _ := resolveTypes(runtime)" to capture the flag,
and after computing outData and mfOut, if runtime.IsBot() && downgraded then set
outData["filter"] = MuteFilterMetaToMap(MuteFilterMeta{Applied:"bot_strip_p2p",
...}) or merge with existing mfOut.Meta so that mfOut.Meta.Applied/"filter"
reflects "bot_strip_p2p"; reference resolveTypes, effective, runtime.IsBot(),
MaybeApplyMuteFilter, mfOut.Meta.Applied and outData["filter"] when making this
change.
---
Nitpick comments:
In `@shortcuts/im/im_chat_list_test.go`:
- Around line 26-57: The helper newChatListTestRuntimeContextWithIdentity
currently builds a cobra.Command and sets flags manually; replace that with the
repo standard test factory pattern: call t.Setenv("LARKSUITE_CLI_CONFIG_DIR",
t.TempDir()) to isolate config, create a factory via cmdutil.TestFactory(t,
config) (or cmdutil.TestFactory(t, nil) if no extra config), and pass that
factory into common.TestNewRuntimeContextWithIdentity instead of constructing
cmd directly; ensure you still set the same flags on the returned
command/context so existing tests keep the same 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: b992e0c3-12c0-45c4-8df8-184707262b10
📒 Files selected for processing (4)
shortcuts/im/im_chat_list.goshortcuts/im/im_chat_list_test.goskills/lark-im/SKILL.mdskills/lark-im/references/lark-im-chat-list.md
✅ Files skipped from review due to trivial changes (1)
- skills/lark-im/SKILL.md
shifengjuan-dev
force-pushed
the
feat/chat-list-types-p2p
branch
from
c3db76e to
657b56c
Compare
|
Pushed force update addressing review feedback (commit
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/im/im_chat_list_test.go (1)
26-57: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse the standard unit-test factory helper in this runtime constructor.
This helper is a unit-test factory path but it builds runtime context via
common.TestNewRuntimeContextWithIdentityinstead ofcmdutil.TestFactory(t, config).As per coding guidelines,
**/*_test.go: “Usecmdutil.TestFactory(t, config)for test factories in unit tests”.🤖 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/im/im_chat_list_test.go` around lines 26 - 57, The helper newChatListTestRuntimeContextWithIdentity builds a runtime context using common.TestNewRuntimeContextWithIdentity(cmd, nil, as) but per test guidelines you should create a test factory with cmdutil.TestFactory(t, config) and pass that factory into the runtime constructor instead of nil; update the function to build a cfg (or reuse defaults), call factory := cmdutil.TestFactory(t, cfg) and then return common.TestNewRuntimeContextWithIdentity(cmd, factory, as) (keeping the existing cmd setup and flags), so the test uses the standard cmdutil.TestFactory(t, ...) path.
🤖 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.
Outside diff comments:
In `@shortcuts/im/im_chat_list_test.go`:
- Around line 26-57: The helper newChatListTestRuntimeContextWithIdentity builds
a runtime context using common.TestNewRuntimeContextWithIdentity(cmd, nil, as)
but per test guidelines you should create a test factory with
cmdutil.TestFactory(t, config) and pass that factory into the runtime
constructor instead of nil; update the function to build a cfg (or reuse
defaults), call factory := cmdutil.TestFactory(t, cfg) and then return
common.TestNewRuntimeContextWithIdentity(cmd, factory, as) (keeping the existing
cmd setup and flags), so the test uses the standard cmdutil.TestFactory(t, ...)
path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f62c55c5-99b5-4349-b007-eaa8c6aa94c6
📒 Files selected for processing (4)
shortcuts/im/im_chat_list.goshortcuts/im/im_chat_list_test.goskills/lark-im/SKILL.mdskills/lark-im/references/lark-im-chat-list.md
✅ Files skipped from review due to trivial changes (2)
- skills/lark-im/references/lark-im-chat-list.md
- skills/lark-im/SKILL.md
shifengjuan-dev
force-pushed
the
feat/chat-list-types-p2p
branch
from
657b56c to
c7ae9ba
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/im/im_chat_list_test.go (1)
26-57: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftUse
cmdutil.TestFactory(t, config)in this unit-test runtime helper.This helper currently constructs runtime context via
common.TestNewRuntimeContextWithIdentity; please switch the unit-test factory path tocmdutil.TestFactory(t, config)to align with repo test-factory requirements.As per coding guidelines:
**/*_test.go: Usecmdutil.TestFactory(t, config)for test factories in unit tests.🤖 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/im/im_chat_list_test.go` around lines 26 - 57, The helper newChatListTestRuntimeContextWithIdentity currently returns common.TestNewRuntimeContextWithIdentity(cmd, nil, as); replace that with the repo-standard cmdutil.TestFactory(t, config) call: build a Factory config object containing the prepared *cobra.Command and the Identity (e.g., a struct/value holding cmd and as), then call cmdutil.TestFactory(t, config) and return its runtime context instead of using common.TestNewRuntimeContextWithIdentity; update any imports to include cmdutil and remove the old common call.
🤖 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.
Outside diff comments:
In `@shortcuts/im/im_chat_list_test.go`:
- Around line 26-57: The helper newChatListTestRuntimeContextWithIdentity
currently returns common.TestNewRuntimeContextWithIdentity(cmd, nil, as);
replace that with the repo-standard cmdutil.TestFactory(t, config) call: build a
Factory config object containing the prepared *cobra.Command and the Identity
(e.g., a struct/value holding cmd and as), then call cmdutil.TestFactory(t,
config) and return its runtime context instead of using
common.TestNewRuntimeContextWithIdentity; update any imports to include cmdutil
and remove the old common call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 152ff382-3368-4042-92ea-e1e65a0b1bf0
📒 Files selected for processing (4)
shortcuts/im/im_chat_list.goshortcuts/im/im_chat_list_test.goskills/lark-im/SKILL.mdskills/lark-im/references/lark-im-chat-list.md
✅ Files skipped from review due to trivial changes (2)
- skills/lark-im/references/lark-im-chat-list.md
- skills/lark-im/SKILL.md
|
Holding the original judgment — the No matching signature to swap:
Same-package precedent for hand-rolling: What was applied: Per the AI-agent prompt accompanying this review ("Fix only still-valid issues, skip the rest with a brief reason"), recording this as skipped with the reasons above. Won't change. @coderabbitai review |
|
Kicking off the review now. ✏️ Learnings added
✅ Actions performedReview triggered.
|
Add a new --types flag (string_slice; values from {group, p2p}) to
+chat-list, backed by the new GET /open-apis/im/v1/chats `types` query
parameter. Accepts CSV (--types group,p2p) and repeated-flag forms
(--types group --types p2p).
Defaults to groups-only (backward compatible). Under user identity,
p2p single chats appear with chat_mode="p2p" plus p2p_target_type /
p2p_target_id fields. Under bot identity:
- --types=p2p alone is rejected at validation
- --types=p2p,group is silently downgraded to types=group (no runtime
notice; skill docs document this contract)
Updates Shortcut.Description, lark-im SKILL.md (frontmatter trigger
+ shortcut table row), and the chat-list reference doc with command
examples, the new parameter, output field documentation, and a
dedicated "Bot identity and p2p" section.
Change-Id: I637ce23b3c6ce4ec350f0ac26dbac8120761bb71
shifengjuan-dev
force-pushed
the
feat/chat-list-types-p2p
branch
from
c7ae9ba to
7b2bf99
Compare
|
Pushed force update Why the change: A P2 review off-thread flagged that the silent-strip behavior — "success but quietly returning fewer kinds of data" — is misleading to both humans and agents. The same concern is what motivated the original PR-body contract; we walked it back earlier because no implementation existed. This commit restores it with a cleaner shape than the original draft (which had tried to overload Design — chose B3 (separate
Why not "priority resolution" on a single filter slot: Pattern lineage in this repo:
Changes:
|
1 participant
Summary
--typesCSV flag tolark-cli im +chat-list, supported values:group/p2p/p2p,group. Backed by the newtypesquery parameter onGET /open-apis/im/v1/chats.--typesreturns groups only (backward compatible).--types=p2palone at validation; for mixed--types=p2p,groupstripp2pand sendtypes=group, surfacing the request-level adjustment two ways — awarning: bot_strip_p2p: ...line on stderr (matches thewarning: <code>: <message>convention inshortcuts/common/runner.go) and a structured{code, message}entry underoutData["notices"]in stdout JSON. Thenoticesslot is a separate top-level key fromoutData["filter"](which stays scoped to--exclude-muted), so the two never collide and no priority resolution is needed when both fire. DryRun emits the same stderr warning for parity withshortcuts/drive/drive_search.go.chat_mode/p2p_target_type/p2p_target_id) conditionally in table output; passes them through unchanged in JSON.Shortcut.Description,skills/lark-im/SKILL.md, andskills/lark-im/references/lark-im-chat-list.md.Test Plan
go build ./...succeedsgo test ./shortcuts/im/... -count=1passes (added ~10 new test functions covering normalizeTypes, resolveTypes, Validate, DryRun, Execute integration with HTTP mock, filter priority, p2p row rendering, defensivechat_mode-absent fallback, and user+mute+p2p mute-filter path)go vet ./shortcuts/im/...cleanlark-cli im +chat-list --as user --types p2p,group --format jsonreturns at least one row withchat_mode="p2p"and a non-emptyp2p_target_id(spec risk readme 优化建议 #2)Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests