fix(im): unify messages-search pagination int flags#446
fix(im): unify messages-search pagination int flags#446YangJunzhou-01 merged 1 commit intolarksuite:mainfrom
Conversation
github-actions
Bot
added
domain/im
|
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:
📝 WalkthroughWalkthroughPagination flags ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
Greptile SummaryThis PR fixes Confidence Score: 5/5Safe to merge — the fix is minimal, targeted, and directly resolves a real validation regression with no side effects. All findings are P2 style suggestions. The core bug fix is correct: switching from Str to Int for an int-registered flag, with test helpers aligned accordingly. No P0/P1 issues found. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as CLI (cobra)
participant RT as RuntimeContext
participant BSR as buildMessagesSearchRequest
participant MPC as messagesSearchPaginationConfig
participant SM as searchMessages
CLI->>RT: --page-limit=2 (int flag)
RT->>BSR: runtime.Int("page-limit") → 2
BSR->>BSR: validate: 1 ≤ 2 ≤ 40 ✓
BSR-->>CLI: *messagesSearchRequest (no error)
CLI->>SM: Execute(req)
SM->>MPC: runtime.Int("page-limit") → 2
MPC-->>SM: autoPaginate=true, pageLimit=min(2,40)=2
SM-->>CLI: results (up to 2 pages)
Reviews (1): Last reviewed commit: "fix(im): handle messages-search page-lim..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@06c66d90c65ba217ed908e385af51cb571899465🧩 Skill updatenpx skills add YangJunzhou-01/cli#fix/pagelimit -y -g |
YangJunzhou-01
force-pushed
the
fix/pagelimit
branch
from
de80169 to
548f377
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #446 +/- ##
==========================================
- Coverage 60.64% 60.04% -0.60%
==========================================
Files 393 406 +13
Lines 33789 43082 +9293
==========================================
+ Hits 20490 25870 +5380
- Misses 11391 15194 +3803
- Partials 1908 2018 +110 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
YangJunzhou-01
force-pushed
the
fix/pagelimit
branch
from
548f377 to
eba1fae
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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/im/builders_test.go`:
- Around line 576-580: Add a positive regression assertion that an explicit page
limit is accepted by the validation/request-building path: using the existing
test runtime created by newMessagesSearchTestRuntimeContext (with
"page-limit":"3"), call the relevant validation/build path (either Validate on
the command or buildMessagesSearchRequest) and assert it returns no error and
that the produced request contains the expected pageLimit (or that validation
passes). This should be added alongside the existing
messagesSearchPaginationConfig check so the test covers both config parsing and
the Validate/buildMessagesSearchRequest flow (refer to
messagesSearchPaginationConfig, newMessagesSearchTestRuntimeContext, Validate,
and buildMessagesSearchRequest).
🪄 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: e6d00c1e-67cf-481c-9baf-c2d74f882c6f
📒 Files selected for processing (4)
shortcuts/im/builders_test.goshortcuts/im/coverage_additional_test.goshortcuts/im/im_messages_search.goshortcuts/im/im_messages_search_execute_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/im/im_messages_search_execute_test.go
YangJunzhou-01
changed the title
YangJunzhou-01
force-pushed
the
fix/pagelimit
branch
from
eba1fae to
5879f2d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/im/im_messages_search.go (1)
254-255: Nit: derive error messages from constants to avoid drift.The error strings hardcode
1 and 40and1 and 50, duplicatingmessagesSearchMaxPageLimitandmessagesSearchMaxPageSize. If either bound ever changes, the messages will silently go stale. Since these errors are also parsed by AI agents, keeping them in sync with the actual limit is worthwhile.♻️ Suggested change
- return nil, output.ErrValidation("--page-limit must be an integer between 1 and 40") + return nil, output.ErrValidation("--page-limit must be an integer between 1 and %d", messagesSearchMaxPageLimit) @@ - return nil, output.ErrValidation("--page-size must be an integer between 1 and 50") + return nil, output.ErrValidation("--page-size must be an integer between 1 and %d", messagesSearchMaxPageSize)(Adjust to
fmt.Sprintf(...)ifoutput.ErrValidationisn't format-aware.)Also applies to: 334-336
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/im/im_messages_search.go` around lines 254 - 255, Replace hardcoded numeric bounds in validation error messages with formatted strings that use the constants messagesSearchMaxPageLimit and messagesSearchMaxPageSize so the message always reflects the actual limits; update the calls to output.ErrValidation to pass a formatted string (e.g. via fmt.Sprintf) using those constants where the checks occur (the pageLimit and pageSize validation branches in im_messages_search.go, including the other occurrence around the block referenced at lines 334-336) so the error text derives from the constants rather than hardcoded values.
🤖 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/im_messages_search.go`:
- Around line 254-255: Replace hardcoded numeric bounds in validation error
messages with formatted strings that use the constants
messagesSearchMaxPageLimit and messagesSearchMaxPageSize so the message always
reflects the actual limits; update the calls to output.ErrValidation to pass a
formatted string (e.g. via fmt.Sprintf) using those constants where the checks
occur (the pageLimit and pageSize validation branches in im_messages_search.go,
including the other occurrence around the block referenced at lines 334-336) so
the error text derives from the constants rather than hardcoded values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7565214f-e87b-4ced-93c5-615e1269707d
📒 Files selected for processing (4)
shortcuts/im/builders_test.goshortcuts/im/coverage_additional_test.goshortcuts/im/im_messages_search.goshortcuts/im/im_messages_search_execute_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- shortcuts/im/coverage_additional_test.go
- shortcuts/im/im_messages_search_execute_test.go
- shortcuts/im/builders_test.go
Change-Id: Ic4876f4ca7f410a8fe3234e08e41b54ce26990d9
YangJunzhou-01
force-pushed
the
fix/pagelimit
branch
from
5879f2d to
06c66d9
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/im/builders_test.go (1)
28-87: Optional: deduplicate the two runtime-context helpers.
newMessagesSearchTestRuntimeContextis ~95% a copy ofnewTestRuntimeContext(only difference: it also registerspage-sizeasInt). Consider collapsing to a single helper that accepts an optional list of extra Int flag names, or factor the shared body (parse + set loops) into an unexported helper to avoid drift if the flag-setting protocol changes later.♻️ Sketch
-func newTestRuntimeContext(t *testing.T, stringFlags map[string]string, boolFlags map[string]bool) *common.RuntimeContext { - ... -} - -func newMessagesSearchTestRuntimeContext(t *testing.T, stringFlags map[string]string, boolFlags map[string]bool) *common.RuntimeContext { - ... -} +func newTestRuntimeContext(t *testing.T, stringFlags map[string]string, boolFlags map[string]bool, intFlags ...string) *common.RuntimeContext { + t.Helper() + cmd := &cobra.Command{Use: "test"} + intSet := map[string]struct{}{"page-limit": {}} + for _, n := range intFlags { + intSet[n] = struct{}{} + } + for n := range intSet { + cmd.Flags().Int(n, 20, "") + } + for name := range stringFlags { + if _, ok := intSet[name]; ok { + continue + } + cmd.Flags().String(name, "", "") + } + // ... rest unchanged +} + +func newMessagesSearchTestRuntimeContext(t *testing.T, s map[string]string, b map[string]bool) *common.RuntimeContext { + return newTestRuntimeContext(t, s, b, "page-size") +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/im/builders_test.go` around lines 28 - 87, Both newTestRuntimeContext and newMessagesSearchTestRuntimeContext duplicate almost all logic; refactor by creating a single helper (e.g., newTestRuntimeContext) that accepts an optional slice of additional int flag names (extraIntFlags []string) or by extracting the shared parsing/setting logic into an unexported helper (e.g., prepareCmdFlags) used by both functions; update newMessagesSearchTestRuntimeContext to call the unified helper (or call prepareCmdFlags) and ensure you still register "page-limit" (and "page-size" when passed via extraIntFlags) and keep the same flag parsing and Flags().Set loops (preserve behavior of stringFlags and boolFlags handling and error t.Fatalf calls).
🤖 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/builders_test.go`:
- Around line 28-87: Both newTestRuntimeContext and
newMessagesSearchTestRuntimeContext duplicate almost all logic; refactor by
creating a single helper (e.g., newTestRuntimeContext) that accepts an optional
slice of additional int flag names (extraIntFlags []string) or by extracting the
shared parsing/setting logic into an unexported helper (e.g., prepareCmdFlags)
used by both functions; update newMessagesSearchTestRuntimeContext to call the
unified helper (or call prepareCmdFlags) and ensure you still register
"page-limit" (and "page-size" when passed via extraIntFlags) and keep the same
flag parsing and Flags().Set loops (preserve behavior of stringFlags and
boolFlags handling and error t.Fatalf calls).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 213e381d-653e-4155-bf6c-a55ffb9e9a3a
📒 Files selected for processing (4)
shortcuts/im/builders_test.goshortcuts/im/coverage_additional_test.goshortcuts/im/im_messages_search.goshortcuts/im/im_messages_search_execute_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- shortcuts/im/coverage_additional_test.go
- shortcuts/im/im_messages_search_execute_test.go
3 participants
Summary
Unify
lark-cli im +messages-searchpagination flags to use int semantics consistently.Previously,
page-limitwas registered as an int flag whilepage-sizewas still handled as a string flag and parsed manually. This led to inconsistent runtime behavior inside the same shortcut and allowed test helpers to drift from the real CLI flag registration.This PR keeps the
page-limitfix and further updates+messages-searchso that bothpage-limitandpage-sizeare handled as int flags. The implementation now reads:page-limitviaruntime.Int("page-limit")page-sizeviaruntime.Int("page-size")Related test helpers and coverage tests are also aligned with the real CLI int flag registration for this shortcut.
Scope
lark-cli im +messages-search--page-allbehavior remains unchangedpage-size > 50is still capped at 50--page-sizevalues now fail earlier at flag parsing instead of business validationTest Plan
go test ./shortcuts/im -count=1Also verified locally that valid usages remain consistent, including:
and quoted numeric values such as:
continue to work as expected in normal shell usage.
Summary by CodeRabbit
Refactor
Tests