feat: auto-complete mail rule reorder ids#1623
Open
jqmseu wants to merge 2 commits into
Open
Conversation
|
|
github-actions
Bot
added
domain/mail
📝 WalkthroughWalkthroughAdds ChangesMail rule reorder shortcut
Sequence Diagram(s)sequenceDiagram
participant MailRuleReorder
participant listMailboxRules
participant buildRuleReorderPlan
participant RulesReorderAPI
MailRuleReorder->>listMailboxRules: GET current mailbox rules
listMailboxRules-->>MailRuleReorder: rule items
MailRuleReorder->>buildRuleReorderPlan: complete IDs or apply move
alt dry-run
MailRuleReorder-->>MailRuleReorder: print preview
else execute
MailRuleReorder->>RulesReorderAPI: POST completed_rule_ids
RulesReorderAPI-->>MailRuleReorder: reorder response
MailRuleReorder-->>MailRuleReorder: print preview
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@shortcuts/mail/mail_rule_reorder_test.go`:
- Around line 58-73: The dry-run test for MailRuleReorder only verifies the GET
and POST calls, but it does not assert the reorder payload body. Update
TestMailRuleReorderDryRunListsAndReorders to check apis[1].Body["rule_ids"] for
the expected rule ID list so the POST contract in MailRuleReorder.DryRun remains
covered if the payload shape changes.
- Around line 21-37: Add a non-numeric failure test for parseRuleIDsInput to
cover the strconv.ParseInt error path. In
shortcuts/mail/mail_rule_reorder_test.go, add a case like "3,abc" and assert the
validation error carries the expected typed metadata from parseRuleIDsInput as
well as the preserved underlying cause via the wrapped error. Reuse the existing
test pattern around parseRuleIDsInput and the duplicate-id test, but extend it
to verify both the validation type and cause for the non-numeric input.
- Around line 32-37: The rule ID parsing tests are only asserting error text, so
they can miss regressions in typed validation metadata. Update
TestParseRuleIDsInputRejectsDuplicate and the related tests in the same area to
assert the returned error with errs.ProblemOf for category/subtype and use
errors.As against *errs.ValidationError to check Param, instead of relying on
substring matching. Keep the focus on parseRuleIDsInput and the
duplicate/invalid input cases so the tests verify the structured error fields
directly.
In `@shortcuts/mail/mail_rule_reorder.go`:
- Around line 81-90: The single-rule branch in mail_rule_reorder.Execute is
returning a success preview before unknown rule IDs are validated. Move the
buildCompletedRuleOrder call ahead of the len(rules) == 1 short-circuit so the
same existence check runs for all inputs, then keep the preview/no-op return
path using the validated reorderedRules and completedIDs. Ensure the behavior in
buildRuleReorderPreview and buildCompletedRuleOrder stays consistent with the
existing not-found handling.
🪄 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: 69a8bef4-fc67-41f6-a126-f8d8de743b61
📒 Files selected for processing (6)
shortcuts/mail/mail_rule_reorder.goshortcuts/mail/mail_rule_reorder_test.goshortcuts/mail/shortcuts.goskills/lark-mail/SKILL.mdskills/lark-mail/references/lark-mail-rule-reorder.mdtests/cli_e2e/mail/coverage.md
Comment on lines
+32
to
+37
| func TestParseRuleIDsInputRejectsDuplicate(t *testing.T) { | ||
| _, err := parseRuleIDsInput("3,1,3") | ||
| if err == nil || !strings.Contains(err.Error(), "duplicate") { | ||
| t.Fatalf("expected duplicate validation error, got %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert typed error metadata here.
These tests only match error text, so they will miss regressions in category, subtype, and param. Use errs.ProblemOf for problem fields and errors.As(..., *errs.ValidationError) for Param instead of substring checks. As per coding guidelines, error-path tests must assert typed metadata, and based on learnings errs.ProblemOf does not expose Param directly.
Also applies to: 112-121
🤖 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/mail/mail_rule_reorder_test.go` around lines 32 - 37, The rule ID
parsing tests are only asserting error text, so they can miss regressions in
typed validation metadata. Update TestParseRuleIDsInputRejectsDuplicate and the
related tests in the same area to assert the returned error with errs.ProblemOf
for category/subtype and use errors.As against *errs.ValidationError to check
Param, instead of relying on substring matching. Keep the focus on
parseRuleIDsInput and the duplicate/invalid input cases so the tests verify the
structured error fields directly.
Sources: Coding guidelines, Learnings
Comment on lines
+58
to
+73
| func TestMailRuleReorderDryRunListsAndReorders(t *testing.T) { | ||
| runtime := runtimeForMailRuleReorderDryRun(t, map[string]string{ | ||
| "mailbox": "me", | ||
| "rule-ids": "3,1", | ||
| }) | ||
| apis := dryRunAPIsForMailRuleReorderTest(t, MailRuleReorder.DryRun(context.Background(), runtime)) | ||
| if len(apis) != 2 { | ||
| t.Fatalf("expected 2 API calls in dry-run, got %d", len(apis)) | ||
| } | ||
| if apis[0].Method != "GET" || apis[0].URL != mailboxPath("me", "rules") { | ||
| t.Fatalf("first dry-run API = %+v, want GET %s", apis[0], mailboxPath("me", "rules")) | ||
| } | ||
| if apis[1].Method != "POST" || apis[1].URL != mailboxPath("me", "rules", "reorder") { | ||
| t.Fatalf("second dry-run API = %+v, want POST %s", apis[1], mailboxPath("me", "rules", "reorder")) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The dry-run test never checks the reorder payload.
A regression that drops or renames the POST rule_ids body would still pass, even though that payload is part of the new dry-run contract. Please assert apis[1].Body["rule_ids"] too. As per coding guidelines, every behavior change needs a test, and the dry-run implementation in shortcuts/mail/mail_rule_reorder.go:52-66 includes the POST body 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 `@shortcuts/mail/mail_rule_reorder_test.go` around lines 58 - 73, The dry-run
test for MailRuleReorder only verifies the GET and POST calls, but it does not
assert the reorder payload body. Update
TestMailRuleReorderDryRunListsAndReorders to check apis[1].Body["rule_ids"] for
the expected rule ID list so the POST contract in MailRuleReorder.DryRun remains
covered if the payload shape changes.
Source: Coding guidelines
github-actions
Bot
added
size/L
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 `@shortcuts/mail/mail_rule_reorder.go`:
- Around line 267-271: The validation metadata for the rule reorder checks is
inconsistent because the MoveRuleID/BeforeRuleID and MoveRuleID/AfterRuleID
branches pass plain labels instead of the CLI flag names. Update the error
construction in mail_rule_reorder.go within the rule reorder validation logic to
use the same "--before" and "--after" identifiers as the other validation paths,
so mailValidationParamError is called consistently for the BeforeRuleID and
AfterRuleID checks.
🪄 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: a2a381ec-36e8-41f7-a001-60d432e54ccc
📒 Files selected for processing (2)
shortcuts/mail/mail_rule_reorder.goshortcuts/mail/mail_rule_reorder_test.go
Comment on lines
+267
to
+271
| if input.MoveRuleID == input.BeforeRuleID && input.BeforeRuleID != "" { | ||
| return ruleReorderInput{}, mailValidationParamError("before", "--before cannot reference the same rule as --move") | ||
| } | ||
| if input.MoveRuleID == input.AfterRuleID && input.AfterRuleID != "" { | ||
| return ruleReorderInput{}, mailValidationParamError("after", "--after cannot reference the same rule as --move") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the CLI flag name in validation metadata.
These two paths pass "before" / "after" while the other validation paths use "--before" / "--after", so typed error metadata becomes inconsistent for the same flags.
Proposed fix
- if input.MoveRuleID == input.BeforeRuleID && input.BeforeRuleID != "" {
- return ruleReorderInput{}, mailValidationParamError("before", "--before cannot reference the same rule as --move")
+ if input.MoveRuleID == input.BeforeRuleID && input.BeforeRuleID != "" {
+ return ruleReorderInput{}, mailValidationParamError("--before", "--before cannot reference the same rule as --move")
}
if input.MoveRuleID == input.AfterRuleID && input.AfterRuleID != "" {
- return ruleReorderInput{}, mailValidationParamError("after", "--after cannot reference the same rule as --move")
+ return ruleReorderInput{}, mailValidationParamError("--after", "--after cannot reference the same rule as --move")
}📝 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.
Suggested change
| if input.MoveRuleID == input.BeforeRuleID && input.BeforeRuleID != "" { | |
| return ruleReorderInput{}, mailValidationParamError("before", "--before cannot reference the same rule as --move") | |
| } | |
| if input.MoveRuleID == input.AfterRuleID && input.AfterRuleID != "" { | |
| return ruleReorderInput{}, mailValidationParamError("after", "--after cannot reference the same rule as --move") | |
| if input.MoveRuleID == input.BeforeRuleID && input.BeforeRuleID != "" { | |
| return ruleReorderInput{}, mailValidationParamError("--before", "--before cannot reference the same rule as --move") | |
| } | |
| if input.MoveRuleID == input.AfterRuleID && input.AfterRuleID != "" { | |
| return ruleReorderInput{}, mailValidationParamError("--after", "--after cannot reference the same rule as --move") | |
| } |
🤖 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/mail/mail_rule_reorder.go` around lines 267 - 271, The validation
metadata for the rule reorder checks is inconsistent because the
MoveRuleID/BeforeRuleID and MoveRuleID/AfterRuleID branches pass plain labels
instead of the CLI flag names. Update the error construction in
mail_rule_reorder.go within the rule reorder validation logic to use the same
"--before" and "--after" identifiers as the other validation paths, so
mailValidationParamError is called consistently for the BeforeRuleID and
AfterRuleID checks.
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
2 participants
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.
Summary
mail +rule-reordershortcut for partial rule id reorder requestsTesting
Summary by CodeRabbit
New Features
mail +rule-reorderto reorder inbox rules using either a partial--rule-idslist or move-style placement (--movewith--before/--after/--to-top).--dry-runpreviews and automatically completes omitted rule IDs while preserving existing relative order.Bug Fixes
Documentation
Tests