fix(im): make flag list page-all exhaustive#1874
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe IM flag list command now defaults ChangesIM flag pagination
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
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
@finebyme99 @liangshuo-1 Could you please review this fix for the silent 20-page truncation in |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/im/im_flag_list.go (1)
226-229: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant bounds check.
Since
validateListOptionsalready strictly enforces that--page-limitis between 0 and 1000,maxPageswill 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
📒 Files selected for processing (4)
shortcuts/im/im_flag_list.goshortcuts/im/im_flag_test.goskills/lark-im/references/lark-im-flag-list.mdtests/cli_e2e/im/flag_workflow_test.go
Summary
Fixes #1859.
im +flag-list --page-allnow 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
--page-limit 0the default and define it as unlimited pagination.truncated: trueand emit a stderr warning.Test Plan
GOTOOLCHAIN=go1.23.0 go test ./shortcuts/im -count=1LARK_CLI_BIN=<absolute-built-binary> GOTOOLCHAIN=go1.23.0 go test ./tests/cli_e2e/im -run TestIM_FlagDryRun -count=1GOTOOLCHAIN=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-testpassed all changed and related packages, includingshortcuts/im. On this machine, the unrelatedshortcuts/minutesdownload tests fail because the environment resolves/blocksexample.comas a local/internal host; no minutes code is changed by this PR.Related Issues
Summary by CodeRabbit
im +flag-list --page-all --page-limit 0now enables unlimited pagination, and--page-limitnow defaults to0(max1000).truncated: true.--page-limitis set to a value greater than0.truncatedresponse field behavior.