feat(im): add --page-all, align page-size limits, accept common flag aliases - #2138
feat(im): add --page-all, align page-size limits, accept common flag aliases#2138sang-neo03 wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughIM list and search shortcuts now support bounded automatic pagination through ChangesIM pagination and validation
IM flag aliases
Common time parsing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant IMShortcut
participant LarkIMAPI
participant Enrichment
CLI->>IMShortcut: run with --page-all and --page-limit
IMShortcut->>LarkIMAPI: request page
LarkIMAPI-->>IMShortcut: return items and continuation token
IMShortcut->>LarkIMAPI: request subsequent pages
IMShortcut->>Enrichment: process merged items
IMShortcut-->>CLI: return aggregated results and pagination status
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@3561753a7da7584693cacc13d69dca5393184df6🧩 Skill updatenpx skills add larksuite/cli#feat/agent-affordance-fixes -y -g |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
shortcuts/im/im_feed_group_list.go (1)
36-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the repeated
50default-page-size literal into a named constant.Three shortcuts duplicate the literal
50in two places each: thepage-sizeflag'sDefaultstring and thedefaultValueargument passed tovalidateIMPageSize.+chat-members-listalready avoids this by using a named constant (chatMembersListDefaultPageSize). Apply the same pattern here to remove the duplication and reduce the risk of the literal drifting fromimPageSizeLimitsif a limit changes later.
shortcuts/im/im_feed_group_list.go#L36-L36: replace the literal"50"/50with a named constant (e.g.feedGroupListDefaultPageSize) shared between the flagDefaultand thevalidateIMPageSize(rt, "+feed-group-list", ...)call at line 75.shortcuts/im/im_feed_group_list_item.go#L31-L31: replace the literal"50"/50with a named constant (e.g.feedGroupListItemDefaultPageSize) shared between the flagDefaultand thevalidateIMPageSize(rt, "+feed-group-list-item", ...)call at line 75.shortcuts/im/im_flag_list.go#L28-L28: replace the literal"50"/50with a named constant (e.g.flagListDefaultPageSize) shared between the flagDefaultand thevalidateIMPageSize(rt, "+flag-list", ...)call at line 74.🤖 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_feed_group_list.go` at line 36, Extract the repeated default page-size literals into named constants and reuse each constant for both the flag Default and its corresponding validateIMPageSize call. Apply this in shortcuts/im/im_feed_group_list.go (36-36, with the call at 75), shortcuts/im/im_feed_group_list_item.go (31-31, with the call at 75), and shortcuts/im/im_flag_list.go (28-28, with the call at 74), using distinct constants for each shortcut such as feedGroupListDefaultPageSize, feedGroupListItemDefaultPageSize, and flagListDefaultPageSize.shortcuts/im/im_chat_list.go (1)
223-275: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFour copies of one page-all loop. All four fetchers repeat the same algorithm: clamp
--page-limit, droppage_token, accumulateitems, print progress, break on exhaustion, break on a repeated token, emit the cap notice, then rewriteitems,has_more, andpage_tokenon the last response. Each copy also prints the cumulative item count under a per-page label and hardcodes1000and1-1000in user-facing text instead of using the local maximum constant. Extract one helper in theimpackage that takes a page-fetch closure, a page cap, and an item noun, then call it from all four sites.Sketch:
type imPageFetch func(pageToken string) (map[string]interface{}, error) func imFetchAllPages(runtime *common.RuntimeContext, noun string, defaultLimit, maxLimit int, fetch imPageFetch) (map[string]interface{}, error)Each caller then supplies only the closure that issues its own request shape.
shortcuts/im/im_chat_list.go#L223-L275: replace the body with a call to the shared helper, passing a closure that callsruntime.CallAPITyped("GET", imChatListPath, params, nil)and the nounchats.shortcuts/im/im_chat_messages_list.go#L217-L269: replace the body with a call to the shared helper, passing a closure that setsparams["page_token"] = []string{token}and callsruntime.DoAPIJSONTypedwith the nounmessages.shortcuts/im/im_threads_messages_list.go#L193-L245: replace the body with a call to the shared helper, passing themap[string][]stringclosure and the nounthread messages.shortcuts/im/im_chat_search.go#L234-L286: replace the body with a call to the shared helper, passing a closure that callsruntime.CallAPITyped("POST", "/open-apis/im/v2/chats/search", params, body)and the nounchats.Build the cap notice inside the helper from
maxLimitso the text cannot drift from the constants.🤖 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 223 - 275, Replace the duplicated pagination loops with a shared imFetchAllPages helper using imPageFetch, centralizing limit clamping, token handling, accumulation, progress output, termination checks, response rewriting, and max-limit messaging. In shortcuts/im/im_chat_list.go:223-275, call it with a closure invoking runtime.CallAPITyped for imChatListPath and noun “chats”; in shortcuts/im/im_chat_messages_list.go:217-269, pass the []string token closure and noun “messages”; in shortcuts/im/im_threads_messages_list.go:193-245, pass the map[string][]string closure and noun “thread messages”; and in shortcuts/im/im_chat_search.go:234-286, pass the POST search closure and noun “chats”. Build the cap notice from each caller’s maxLimit rather than hardcoding 1000 or 1-1000.
🤖 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/SKILL.md`:
- Line 109: Update the shortcut index entries for +chat-messages-list and
+threads-messages-list in SKILL.md to name the supported --order option instead
of --sort, matching the option documented in their respective reference files.
---
Nitpick comments:
In `@shortcuts/im/im_chat_list.go`:
- Around line 223-275: Replace the duplicated pagination loops with a shared
imFetchAllPages helper using imPageFetch, centralizing limit clamping, token
handling, accumulation, progress output, termination checks, response rewriting,
and max-limit messaging. In shortcuts/im/im_chat_list.go:223-275, call it with a
closure invoking runtime.CallAPITyped for imChatListPath and noun “chats”; in
shortcuts/im/im_chat_messages_list.go:217-269, pass the []string token closure
and noun “messages”; in shortcuts/im/im_threads_messages_list.go:193-245, pass
the map[string][]string closure and noun “thread messages”; and in
shortcuts/im/im_chat_search.go:234-286, pass the POST search closure and noun
“chats”. Build the cap notice from each caller’s maxLimit rather than hardcoding
1000 or 1-1000.
In `@shortcuts/im/im_feed_group_list.go`:
- Line 36: Extract the repeated default page-size literals into named constants
and reuse each constant for both the flag Default and its corresponding
validateIMPageSize call. Apply this in shortcuts/im/im_feed_group_list.go
(36-36, with the call at 75), shortcuts/im/im_feed_group_list_item.go (31-31,
with the call at 75), and shortcuts/im/im_flag_list.go (28-28, with the call at
74), using distinct constants for each shortcut such as
feedGroupListDefaultPageSize, feedGroupListItemDefaultPageSize, and
flagListDefaultPageSize.
🪄 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 Plus
Run ID: f5aacaf0-380b-45cc-a85d-17fc8eccca54
📒 Files selected for processing (24)
shortcuts/im/builders_test.goshortcuts/im/coverage_additional_test.goshortcuts/im/im_chat_list.goshortcuts/im/im_chat_list_test.goshortcuts/im/im_chat_members_list.goshortcuts/im/im_chat_messages_list.goshortcuts/im/im_chat_search.goshortcuts/im/im_feed_group_item_test.goshortcuts/im/im_feed_group_list.goshortcuts/im/im_feed_group_list_item.goshortcuts/im/im_flag_list.goshortcuts/im/im_list_page_all_test.goshortcuts/im/im_messages_search.goshortcuts/im/im_page_size_limits.goshortcuts/im/im_page_size_limits_test.goshortcuts/im/im_threads_messages_list.goshortcuts/im/with_sender_name_test.goskills/lark-im/SKILL.mdskills/lark-im/references/lark-im-chat-list.mdskills/lark-im/references/lark-im-chat-members-list.mdskills/lark-im/references/lark-im-chat-messages-list.mdskills/lark-im/references/lark-im-chat-search.mdskills/lark-im/references/lark-im-threads-messages-list.mdtests/cli_e2e/im/im_list_page_all_dryrun_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2138 +/- ##
==========================================
+ Coverage 75.48% 75.58% +0.09%
==========================================
Files 928 929 +1
Lines 98600 98892 +292
==========================================
+ Hits 74427 74746 +319
+ Misses 18519 18467 -52
- Partials 5654 5679 +25 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Four of the most-used im list commands lacked --page-all while sibling commands in the same domain had it, so callers that learned the flag on +messages-search kept passing it to +threads-messages-list and friends and got "unknown flag". Separately, +threads-messages-list declared a page-size ceiling of 500 while the server accepts 50, so oversized values were forwarded and came back as an opaque "field validation failed" with no indication of which field was wrong. Add --page-all/--page-limit to +threads-messages-list, +chat-messages-list, +chat-list and +chat-search, following the existing +flag-list implementation: pages are capped, has_more and page_token come from the last fetched page so callers can resume, reaching the cap with has_more=true reports an incomplete result on stderr, and a non-advancing page_token stops the loop. Progress goes to stderr; stdout carries data only. Raw items from every page are merged first, then message conversion, sender-name resolution, thread expansion, reaction enrichment and resource download run once over the merged set. Page-size ceilings for the nine paginated im commands now come from a single table with a table-driven test, out-of-range values are rejected locally with a structured validation error that names the limit, and no HTTP request is issued when validation fails. +chat-members-list moves off its hand-written bounds check onto the shared validator. +feed-group-list-item keeps its current ceiling of 50: the public specification for its endpoint is unavailable, so the value is left pending confirmation rather than guessed.
1ecc327 to
eb0bd8a
Compare
Six flags in the im domain are routinely typed under a different name — --start-time for --start, --thread-id for --thread, --message-id for --message-ids, --keyword for --query, --sort-order for --order and --limit for --page-size. The value written alongside them is already valid in every case; only the name is wrong, so the call fails once and has to be retried under the canonical name. Register the eight names as hidden aliases, following the existing pattern in this package: the canonical flag wins when both are given, --help and schema keep listing only the canonical name, and a note naming the canonical flag is written to stderr so callers learn it instead of settling on the alias. The four aliases that already existed now emit that note too. Out-of-range values report the flag the caller actually typed, so --limit 500 is rejected as --limit rather than as --page-size. --thread and --message-ids drop their Required declaration and validate in Validate instead, otherwise cobra rejects the call before an alias can be resolved. ParseTime gains "2006-01-02 15:04:05 Z07:00". A space-separated timestamp with an offset is the most common thing written after --start-time, and without this format the alias would only turn an unknown-flag error into a parse error. The format is additive: inputs that parsed before are unaffected.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/cli_e2e/im/im_chat_search_types_dryrun_test.go`:
- Around line 70-87: Extend the validation assertions in the dry-run cases
around the chat-list and chat-search command results to inspect typed metadata
in result.Stderr, not only error.message. Assert the expected validation
category, subtype, and parameter for the --types failures, using the structured
error fields returned by the validation path while preserving the existing
message and exit-code checks.
- Around line 49-55: Update the test arguments in the +chat-search dry-run case
to use the invalid alias value “p2p” for --types while retaining --chat-modes
topic, so it verifies canonical precedence bypasses alias validation. Keep the
existing exit-code, resolved filter, and stderr assertions unchanged.
🪄 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 Plus
Run ID: 260e349f-10a0-406e-94ed-165969ffd10f
📒 Files selected for processing (8)
shortcuts/im/builders_test.goshortcuts/im/im_chat_search.goshortcuts/im/im_chat_search_test.goshortcuts/im/im_messages_search.goshortcuts/im/sort_flags.goskills/lark-im/references/lark-im-feed-shortcut-list.mdskills/lark-im/references/lark-im-messages-resources-download.mdtests/cli_e2e/im/im_chat_search_types_dryrun_test.go
💤 Files with no reviewable changes (1)
- shortcuts/im/im_messages_search.go
🚧 Files skipped from review as they are similar to previous changes (3)
- shortcuts/im/builders_test.go
- shortcuts/im/im_chat_search.go
- shortcuts/im/sort_flags.go
| Args: []string{"im", "+chat-search", "--query", "team", "--types", "group", "--chat-modes", "topic", "--dry-run"}, | ||
| DefaultAs: "bot", | ||
| }) | ||
| require.NoError(t, err) | ||
| result.AssertExitCode(t, 0) | ||
| require.Equal(t, "thread", clie2e.DryRunGet(result.Stdout, "api.0.body.filter.chat_modes.0").String()) | ||
| require.NotContains(t, result.Stderr, "--types on +chat-search maps") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test canonical precedence with an invalid alias value.
Line 49 uses --types group, which is valid without --chat-modes. Change it to --types p2p. This verifies that canonical --chat-modes topic bypasses alias validation as required.
Proposed test change
- Args: []string{"im", "+chat-search", "--query", "team", "--types", "group", "--chat-modes", "topic", "--dry-run"},
+ Args: []string{"im", "+chat-search", "--query", "team", "--types", "p2p", "--chat-modes", "topic", "--dry-run"},📝 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.
| Args: []string{"im", "+chat-search", "--query", "team", "--types", "group", "--chat-modes", "topic", "--dry-run"}, | |
| DefaultAs: "bot", | |
| }) | |
| require.NoError(t, err) | |
| result.AssertExitCode(t, 0) | |
| require.Equal(t, "thread", clie2e.DryRunGet(result.Stdout, "api.0.body.filter.chat_modes.0").String()) | |
| require.NotContains(t, result.Stderr, "--types on +chat-search maps") | |
| Args: []string{"im", "+chat-search", "--query", "team", "--types", "p2p", "--chat-modes", "topic", "--dry-run"}, | |
| DefaultAs: "bot", | |
| }) | |
| require.NoError(t, err) | |
| result.AssertExitCode(t, 0) | |
| require.Equal(t, "thread", clie2e.DryRunGet(result.Stdout, "api.0.body.filter.chat_modes.0").String()) | |
| require.NotContains(t, result.Stderr, "--types on +chat-search maps") |
🤖 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 `@tests/cli_e2e/im/im_chat_search_types_dryrun_test.go` around lines 49 - 55,
Update the test arguments in the +chat-search dry-run case to use the invalid
alias value “p2p” for --types while retaining --chat-modes topic, so it verifies
canonical precedence bypasses alias validation. Keep the existing exit-code,
resolved filter, and stderr assertions unchanged.
Source: Coding guidelines
| result.AssertExitCode(t, 2) | ||
| require.Empty(t, result.Stdout) | ||
| message := gjson.Get(result.Stderr, "error.message").String() | ||
| require.Contains(t, message, "service does not support p2p") | ||
| require.Contains(t, message, "im +chat-list --types p2p") | ||
| }) | ||
| } | ||
|
|
||
| result, err := clie2e.RunCmd(ctx, clie2e.Request{ | ||
| Args: []string{"im", "+chat-search", "--query", "team", "--types", "xxx", "--dry-run"}, | ||
| DefaultAs: "bot", | ||
| }) | ||
| require.NoError(t, err) | ||
| result.AssertExitCode(t, 2) | ||
| require.Empty(t, result.Stdout) | ||
| message := gjson.Get(result.Stderr, "error.message").String() | ||
| require.Contains(t, message, "--chat-modes (group|topic)") | ||
| require.Contains(t, message, "--search-types (private|external|public_joined|public_not_joined)") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert structured validation metadata.
Lines 72 and 85 only assert error.message. Also assert the validation category, subtype, and parameter from result.Stderr. The validation path returns typed metadata for --types, and message-only checks do not protect that contract.
🤖 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 `@tests/cli_e2e/im/im_chat_search_types_dryrun_test.go` around lines 70 - 87,
Extend the validation assertions in the dry-run cases around the chat-list and
chat-search command results to inspect typed metadata in result.Stderr, not only
error.message. Assert the expected validation category, subtype, and parameter
for the --types failures, using the structured error fields returned by the
validation path while preserving the existing message and exit-code checks.
Sources: Coding guidelines, Learnings
Summary
Four of the most-used
imlist commands lacked--page-allwhile sibling commands in the same domain had it, so callers that learned the flag on+messages-searchkept passing it to+threads-messages-listand friends and gotunknown flag. Separately,+threads-messages-listdeclared a page-size ceiling of 500 while the server accepts 50, so oversized values were forwarded and came back as an opaquefield validation failedthat does not say which field was wrong.Changes
Add
--page-alland--page-limittoim +threads-messages-list,+chat-messages-list,+chat-listand+chat-search, following the existing+flag-listimplementation.--page-limitdefaults to 10 and accepts 1-1000; an explicitly supplied--page-tokentakes precedence and fetches only that page.Merged results keep the single-page shape:
itemsfrom every page in original order,has_moreandpage_tokentaken from the last fetched page so callers can resume.Reaching the page cap with
has_more=truereports an incomplete result on stderr, and a non-advancingpage_tokenstops the loop. Progress and warnings go to stderr; stdout carries data only.Raw items are merged before enrichment, so message conversion, sender-name resolution, thread expansion, reaction enrichment and resource download each run once over the merged set instead of once per page.
Page-size ceilings for the nine paginated
imcommands now come from a single table (shortcuts/im/im_page_size_limits.go) with a table-driven test.+threads-messages-listgoes from 500 to 50; out-of-range values are rejected locally with a structured validation error naming the limit, and no HTTP request is issued when validation fails.+chat-members-listmoves off its hand-written bounds check onto the sharedValidatePageSizeTyped.Update the four command references and
skills/lark-im/SKILL.md, including how to resume frompage_tokenwhen the result is incomplete.Register eight hidden flag aliases in the
imdomain for names callers routinely type:--start-time/--end-time(71 occurrences),--thread-id(15),--message-id(7),--keyword(5),--sort-order(4) and--limit(4). The canonical flag wins when both are given,--helpand schema still list only the canonical name, and a note naming the canonical flag goes to stderr. Out-of-range values report the flag the caller typed, so--limit 500is rejected as--limit.--threadand--message-idsdrop theirRequireddeclaration and validate inValidateinstead; cobra would otherwise reject the call before an alias could be resolved.ParseTimegains"2006-01-02 15:04:05 Z07:00". A space-separated timestamp with an offset is the most common value written after--start-time, and without it the alias would only turn an unknown-flag error into a parse error. The format is additive — inputs that parsed before are unaffected.Accept a hidden, value-aware
--typescompatibility input onim +chat-search.--types groupmaps through the canonical--chat-modes grouppath and emits a note on stderr; an explicit--chat-modeswins when both are supplied.--types p2preturns a validation error directing callers toim +chat-list --types p2p, while other values list the valid--chat-modesand--search-typesdomains. The group-only behavior is confirmed against the live service:filter.chat_modes=["p2p"]returns code99992402(field validation failed). The hidden flag remains absent from--helpand schema output.Clarify two IM reference constraints:
+feed-shortcut-listhas no caller-controlled--page-size, and+messages-resources-downloadhas no--overwriteflag because saving to an existing path replaces the file atomically.Two notes for reviewers:
+feed-group-list-itemhas no public specification for its endpoint, so its ceiling of 50 was established by probing:page_size51 and above returnscode 230001 param is invalid, 50 succeeds. The probe is recorded in the table comment.The four commands each carry their own pagination loop, mirroring the other
--page-allimplementations already in the repo. Collapsing all of them onto one shared loop requires the generic merge ininternal/client/pagination.goto stop droppingpage_token; that is deliberately left to a follow-up.The four aliases that already existed in this package (
--sort,--sort-type,--sort-by) now emit the same stderr note. That is a behaviour change, and intentional: silently accepting an alias means callers never learn the canonical name.ParseTimelives inshortcuts/commonand is shared by every domain. The change only widens what parses; no existing input changes meaning.Test Plan
go test ./shortcuts/im/... -count=1, plusmake vet fmt-check,make unit-test,make quality-gatehas_more/page_tokenfrom the last page, stop on repeated token, incomplete-result warning on stderr with clean stdout, explicit--page-tokendisabling--page-all,--page-limitbounds, enrichment/filtering running exactly once on merged results, and a dry-run e2e test+chat-search --typescoverage:grouprequest parity with--chat-modes group,p2pand mixed-value validation, unknown-value guidance, canonical-flag precedence, stderr/stdout separation, and hidden help surface--helpshowspage size (1-50)for+threads-messages-list;--page-size 51returnsinvalid --page-size 51: must be between 1 and 50;--page-limit 0is rejected;--page-all --dry-runreports auto-paginationRelated Issues
Summary by CodeRabbit
New Features
--page-alland configurable page limits.Bug Fixes
Documentation