Skip to content

fix(im): make flag list page-all exhaustive#1874

Open
AaronZ345 wants to merge 2 commits into
larksuite:mainfrom
AaronZ345:fix/im-flag-list-full-pagination
Open

fix(im): make flag list page-all exhaustive#1874
AaronZ345 wants to merge 2 commits into
larksuite:mainfrom
AaronZ345:fix/im-flag-list-full-pagination

Conversation

@AaronZ345

@AaronZ345 AaronZ345 commented Jul 14, 2026

Copy link
Copy Markdown

Summary

Fixes #1859. im +flag-list --page-all now exhausts all pages by default instead of silently stopping after 20, so agents do not treat a partial count of zero as the real flagged count.

Changes

  • Make --page-limit 0 the default and define it as unlimited pagination.
  • Keep explicit positive page limits, but mark early results with truncated: true and emit a stderr warning.
  • Add a 21-page regression test that fails under the old hidden 20-page cap.
  • Cover validation, limited-result signaling, CLI dry-run, and user-facing documentation.

Test Plan

  • GOTOOLCHAIN=go1.23.0 go test ./shortcuts/im -count=1
  • LARK_CLI_BIN=<absolute-built-binary> GOTOOLCHAIN=go1.23.0 go test ./tests/cli_e2e/im -run TestIM_FlagDryRun -count=1
  • GOTOOLCHAIN=go1.23.0 go vet ./...
  • GOTOOLCHAIN=go1.23.0 go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.1.6 run --new-from-rev=origin/main (0 issues)
  • gofmt -l . (no output)
  • GOTOOLCHAIN=go1.23.0 go mod tidy (no diff)

GOTOOLCHAIN=go1.23.0 make unit-test passed all changed and related packages, including shortcuts/im. On this machine, the unrelated shortcuts/minutes download tests fail because the environment resolves/blocks example.com as a local/internal host; no minutes code is changed by this PR.

Related Issues

Summary by CodeRabbit

  • New Features
    • im +flag-list --page-all --page-limit 0 now enables unlimited pagination, and --page-limit now defaults to 0 (max 1000).
    • When pagination ends early, results include truncated: true.
    • Pagination warnings are shown only when --page-limit is set to a value greater than 0.
  • Documentation
    • Updated command reference to reflect the new unlimited default and the truncated response field behavior.

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@github-actions github-actions Bot added domain/im PR touches the im domain size/M Single-domain feat or fix with limited business impact labels Jul 14, 2026
@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 79742a91-b421-41bd-9cbb-183c8fbc142a

📥 Commits

Reviewing files that changed from the base of the PR and between 3ba90e8 and a654e6f.

📒 Files selected for processing (1)
  • shortcuts/im/im_flag_list.go
💤 Files with no reviewable changes (1)
  • shortcuts/im/im_flag_list.go

📝 Walkthrough

Walkthrough

The IM flag list command now defaults --page-limit to unlimited pagination, accepts zero as valid, continues until the API has no more pages, and reports truncation when a positive limit stops retrieval early. Tests and command documentation cover the updated behavior.

Changes

IM flag pagination

Layer / File(s) Summary
Page-limit contract and validation
shortcuts/im/im_flag_list.go, shortcuts/im/im_flag_test.go
--page-limit defaults to 0, accepts 0..1000, and rejects negative values.
Pagination and truncation handling
shortcuts/im/im_flag_list.go, shortcuts/im/im_flag_test.go
Unlimited mode fetches pages until has_more is false; limited mode marks output as truncated and emits a warning when more pages remain.
Documentation and CLI verification
skills/lark-im/references/lark-im-flag-list.md, tests/cli_e2e/im/flag_workflow_test.go
Documentation describes the new default and response field, while the dry-run workflow verifies page-limit zero.

Estimated code review effort: 2 (Simple) | ~10 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CLI
  participant executeListAllPages
  participant LarkIMFlagsAPI
  CLI->>executeListAllPages: pass page-limit
  executeListAllPages->>LarkIMFlagsAPI: request flag page
  LarkIMFlagsAPI-->>executeListAllPages: flags and has_more
  executeListAllPages->>LarkIMFlagsAPI: request next page when unlimited or below limit
  executeListAllPages-->>CLI: merged response with truncated when limited
Loading

Suggested labels: feature

Suggested reviewers: liangshuo-1, yangjunzhou-01

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main behavior change: making im +flag-list --page-all exhaust pagination.
Description check ✅ Passed The description follows the template and includes the required summary, changes, test plan, and related issues sections.
Linked Issues check ✅ Passed The PR satisfies #1859 by removing the hidden 20-page cap and clearly signaling truncation for bounded pagination.
Out of Scope Changes check ✅ Passed All changes are directly tied to pagination behavior, tests, or documentation for the linked issue, with no unrelated scope.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@AaronZ345

Copy link
Copy Markdown
Author

@finebyme99 @liangshuo-1 Could you please review this fix for the silent 20-page truncation in im +flag-list --page-all? It includes a 21-page regression test, explicit truncation signaling for positive limits, and CLI dry-run coverage. If it looks good, a merge would be appreciated.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
shortcuts/im/im_flag_list.go (1)

226-229: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Remove redundant bounds check.

Since validateListOptions already strictly enforces that --page-limit is between 0 and 1000, maxPages will never exceed 1000 when execution reaches this point. This clamping check is dead code and can be safely removed.

♻️ Proposed refactor
 	maxPages := rt.Int("page-limit")
-	if maxPages > 1000 {
-		maxPages = 1000
-	}
 
 	// Use make([]any, 0) to ensure empty arrays serialize as [] not null
🤖 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_flag_list.go` around lines 226 - 229, Remove the redundant
maxPages upper-bound clamping after reading the "page-limit" option in the
flag-list flow. Rely on validateListOptions to enforce the 0–1000 range and keep
the validated value unchanged.
🤖 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.

Nitpick comments:
In `@shortcuts/im/im_flag_list.go`:
- Around line 226-229: Remove the redundant maxPages upper-bound clamping after
reading the "page-limit" option in the flag-list flow. Rely on
validateListOptions to enforce the 0–1000 range and keep the validated value
unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e27558d3-c36d-4b0e-9361-4a0d42c88940

📥 Commits

Reviewing files that changed from the base of the PR and between 37d490a and 3ba90e8.

📒 Files selected for processing (4)
  • shortcuts/im/im_flag_list.go
  • shortcuts/im/im_flag_test.go
  • skills/lark-im/references/lark-im-flag-list.md
  • tests/cli_e2e/im/flag_workflow_test.go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain/im PR touches the im domain size/M Single-domain feat or fix with limited business impact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

im +flag-list: 默认分页截断导致有效标记数量误报为 0,无法快速读取实时 Flagged 数量

2 participants