Complete mail rule reorder ID list - #2185
Open
yangr-happy wants to merge 2 commits into
Open
Conversation
Co-authored-by: TRAE CLI <noreply@bytedance.com>
github-actions
Bot
added
the
size/L
📝 WalkthroughWalkthroughThe service completes partial mail rule reorder requests. It fetches current rules across pages, validates identifiers and responses, appends omitted rules, and submits the completed order. Tests cover pagination, parameter preservation, dry-run behavior, validation, and API failures. ChangesMail rule reorder flow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant serviceMethodRun
participant completeMailRuleReorderRequest
participant mailboxRuleListAPI
participant mailboxRuleReorderAPI
serviceMethodRun->>completeMailRuleReorderRequest: complete reorder request
completeMailRuleReorderRequest->>mailboxRuleListAPI: fetch paginated current rules
mailboxRuleListAPI-->>completeMailRuleReorderRequest: return rule pages
completeMailRuleReorderRequest->>completeMailRuleReorderRequest: validate and append omitted rule IDs
serviceMethodRun->>mailboxRuleReorderAPI: submit completed rule_ids
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 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@4b162b9eb85046f8008e5f24ed5ce998211b4a60🧩 Skill updatenpx skills add yangr-happy/cli#feat/ce4d012 -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
cmd/service/mail_rule_reorder.go (1)
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant path check and rename the constant.
Line 48 trims the leading slash, so
pathstarts withopen-apis/mail/v1/user_mailboxes/whenever the prefix check on line 49 passes. That string already contains/user_mailboxes/, so thestrings.Containscheck on line 51 can never fail independently. The constant namemailRuleReorderSuffixalso describes a suffix, but the value is an inner path segment.♻️ Proposed simplification
-const mailRuleReorderSuffix = "/user_mailboxes/" -path := strings.Trim(request.URL, "/") return strings.HasPrefix(path, "open-apis/mail/v1/user_mailboxes/") && - strings.HasSuffix(path, "/rules/reorder") && - strings.Contains(path, mailRuleReorderSuffix) + strings.HasSuffix(path, "/rules/reorder") }Also applies to: 48-51
🤖 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 `@cmd/service/mail_rule_reorder.go` at line 16, The redundant path check using mailRuleReorderSuffix should be removed because the preceding prefix validation already guarantees the user_mailboxes segment; rename the constant to reflect that it represents an inner path segment, and update any remaining references consistently.cmd/service/service_test.go (2)
1133-1137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFailure-path tests never verify that registered stubs were consumed. Both tests register a mail rule list stub and assert only the returned error, so neither proves that the list request was actually issued before the failure.
reg.Verify(t)reports unmatched stubs and closes that gap.
cmd/service/service_test.go#L1133-L1137: addreg.Verify(t)after theerr == nilcheck inTestServiceMethod_MailRuleReorderListFailureDoesNotCallReorder.cmd/service/service_test.go#L1179-L1183: addreg.Verify(t)after the error assertion inTestServiceMethod_MailRuleReorderValidationErrorsBeforeReorder.🤖 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 `@cmd/service/service_test.go` around lines 1133 - 1137, Failure-path tests do not verify that registered mail rule list stubs were consumed. In cmd/service/service_test.go lines 1133-1137 within TestServiceMethod_MailRuleReorderListFailureDoesNotCallReorder, add reg.Verify(t) after the err == nil check; do the same in lines 1179-1183 within TestServiceMethod_MailRuleReorderValidationErrorsBeforeReorder after the error assertion.
1264-1274: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace
stringSlicesEqualwithslices.Equal.The module targets Go 1.23, so the standard library provides this behavior. After import
"slices", replace the helper and both call sites withslices.Equal.🤖 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 `@cmd/service/service_test.go` around lines 1264 - 1274, Remove the stringSlicesEqual function definition and add an import for the standard library "slices" package at the top of the file. Then update both call sites where stringSlicesEqual is invoked to use slices.Equal instead, passing the same arguments in the same order.
🤖 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 `@cmd/service/mail_rule_reorder.go`:
- Around line 18-39: Update the dry-run handling in serviceMethodRun to detect
mail-rule reorder requests and print a stderr hint that rule_ids are expanded at
execution time, since completeMailRuleReorderRequest cannot run without an API
call. Keep the existing dry-run request body unchanged and preserve normal
completion through completeMailRuleReorderRequest for real executions.
- Around line 187-189: Update the empty-list branch in the mail rule reorder
flow to return errs.SubtypeFailedPrecondition instead of
errs.SubtypeInvalidArgument, and remove the rule_ids parameter from the error.
Preserve the existing validation message and nil result behavior.
- Around line 154-158: The pagination response parsing in fetchAllMailRuleIDs
must reject malformed has_more values instead of treating them as false. Check
whether data["has_more"] is present and validate its type; when present but
non-boolean, return an errs.SubtypeInvalidResponse error, while preserving the
existing behavior for a missing or valid boolean value and the page-token
handling.
- Around line 79-81: Update the parameter preparation in the mailbox rule
reorder flow to stop deleting or setting pagination keys in the params map. Pass
the copied request parameters unchanged to the mailbox rule list API, without
forcing page_size or page_token support; preserve the existing reorder behavior
otherwise.
- Around line 85-116: Add iteration protection to the pagination loop starting
at the for statement to prevent infinite loops when a server repeats the same
page_token with has_more=true. Either implement a maximum page iteration cap or
track previously seen tokens in a set and stop the loop if pageToken appears
again in the extractMailRulePage extraction. Keep the existing empty-token
validation at the nextToken check intact, and ensure the protection is checked
after extractMailRulePage assigns the next token but before it is used in the
next iteration.
In `@cmd/service/service_test.go`:
- Around line 1044-1049: The error-path tests currently rely on message
substrings instead of typed metadata. In cmd/service/service_test.go lines
1044-1049, extend the test table with wantSubtype and wantParam, then assert
category and subtype via errs.ProblemOf and extract *errs.ValidationError with
errors.As to verify Param after the existing substring check. In
cmd/service/service_test.go lines 1179-1182, replace the substring-only
assertion with errs.ProblemOf category/subtype checks and use errors.As to
verify Param equals rule_ids; preserve cause verification if already covered by
the test.
---
Nitpick comments:
In `@cmd/service/mail_rule_reorder.go`:
- Line 16: The redundant path check using mailRuleReorderSuffix should be
removed because the preceding prefix validation already guarantees the
user_mailboxes segment; rename the constant to reflect that it represents an
inner path segment, and update any remaining references consistently.
In `@cmd/service/service_test.go`:
- Around line 1133-1137: Failure-path tests do not verify that registered mail
rule list stubs were consumed. In cmd/service/service_test.go lines 1133-1137
within TestServiceMethod_MailRuleReorderListFailureDoesNotCallReorder, add
reg.Verify(t) after the err == nil check; do the same in lines 1179-1183 within
TestServiceMethod_MailRuleReorderValidationErrorsBeforeReorder after the error
assertion.
- Around line 1264-1274: Remove the stringSlicesEqual function definition and
add an import for the standard library "slices" package at the top of the file.
Then update both call sites where stringSlicesEqual is invoked to use
slices.Equal instead, passing the same arguments in the same order.
🪄 Autofix
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: 6a8bc7ef-9961-4123-a0fa-41f713a19449
📒 Files selected for processing (3)
cmd/service/mail_rule_reorder.gocmd/service/service.gocmd/service/service_test.go
Change-Type: ci-fix Co-authored-by: TRAE CLI <noreply@bytedance.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@cmd/service/service_test.go`:
- Around line 235-236: Isolate CLI configuration state by setting
LARKSUITE_CLI_CONFIG_DIR to t.TempDir() with t.Setenv before each
cmdutil.TestFactory call in cmd/service/service_test.go at lines 235-236,
1125-1128, 1196-1199, and 1219-1222.
🪄 Autofix
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: 54257c9a-d9ae-4d19-8f27-ff573e7d7e46
📒 Files selected for processing (3)
cmd/service/mail_rule_reorder.gocmd/service/service.gocmd/service/service_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/service/service.go
- cmd/service/mail_rule_reorder.go
Comment on lines
+235
to
+236
| func TestServiceMethod_DryRun_MailRuleReorderWarnsAboutExecutionExpansion(t *testing.T) { | ||
| f, stdout, stderr, _ := cmdutil.TestFactory(t, testConfig) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Isolate CLI configuration state in the new HTTP tests.
Set LARKSUITE_CLI_CONFIG_DIR to t.TempDir() before each cmdutil.TestFactory call.
cmd/service/service_test.go#L235-L236: addt.Setenvbefore creating the factory.cmd/service/service_test.go#L1125-L1128: addt.Setenvbefore creating the factory.cmd/service/service_test.go#L1196-L1199: addt.Setenvbefore creating the factory.cmd/service/service_test.go#L1219-L1222: addt.Setenvbefore creating the factory.
As per coding guidelines: “set LARKSUITE_CLI_CONFIG_DIR to t.TempDir() with t.Setenv to isolate configuration state.”
📍 Affects 1 file
cmd/service/service_test.go#L235-L236(this comment)cmd/service/service_test.go#L1125-L1128cmd/service/service_test.go#L1196-L1199cmd/service/service_test.go#L1219-L1222
🤖 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 `@cmd/service/service_test.go` around lines 235 - 236, Isolate CLI
configuration state by setting LARKSUITE_CLI_CONFIG_DIR to t.TempDir() with
t.Setenv before each cmdutil.TestFactory call in cmd/service/service_test.go at
lines 235-236, 1125-1128, 1196-1199, and 1219-1222.
Sources: Coding guidelines, Learnings
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This updates the mailbox rule reorder command so partial rule ID input is expanded into the complete ordered list before submitting the reorder request.
Summary by CodeRabbit
New Features
Bug Fixes