feat(mail): add email priority support for compose and read#538
feat(mail): add email priority support for compose and read#538chanthuang merged 7 commits intomainfrom
Conversation
github-actions
Bot
added
domain/mail
|
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:
📝 WalkthroughWalkthroughAdds CLI Changes
Sequence Diagram(s)sequenceDiagram
participant User as CLI User
participant Runtime as RuntimeContext
participant Helpers as Priority Helpers
participant Builder as emlbuilder.Builder
participant Server as Mail API
User->>Runtime: run mail +send --priority high
Runtime->>Helpers: parsePriority("high")
Helpers-->>Runtime: "1"
Runtime->>Builder: build body (HTML/text)
Runtime->>Helpers: applyPriority(builder, "1")
Helpers->>Builder: set header X-Cli-Priority: 1
Builder->>Server: send generated EML
Server-->>User: API response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #538 +/- ##
==========================================
+ Coverage 59.26% 59.91% +0.65%
==========================================
Files 388 388
Lines 32983 33147 +164
==========================================
+ Hits 19546 19859 +313
+ Misses 11606 11420 -186
- Partials 1831 1868 +37 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/mail/helpers.go (1)
1150-1175:⚠️ Potential issue | 🟠 Major
priority_typeandpriority_type_textcan disagree in the public output when the label override fires.
buildMessageOutputseedsoutfrompickSafeMessageFields(msg), which copies the rawpriority_typefrom the API response. Onlypriority_type_textis then overwritten fromnormalized.PriorityTypeText(lines 1165-1167). Whenlabel_idscontainsHIGH_PRIORITY/LOW_PRIORITYbut the rawpriority_typeis"0","3", or absent, the+messageJSON ends up like:{ "priority_type": "0", "priority_type_text": "high", "label_ids": ["HIGH_PRIORITY", ...] }That contradicts the PR objective ("surfaces priority via
priority_type/priority_type_text, deriving priority fromHIGH_PRIORITY/LOW_PRIORITY") and any consumer that branches onpriority_type.🐛 Suggested fix — overwrite `priority_type` together with `_text`
- if normalized.PriorityType != "" { - out["priority_type_text"] = normalized.PriorityTypeText - } + if normalized.PriorityType != "" { + out["priority_type"] = normalized.PriorityType + out["priority_type_text"] = normalized.PriorityTypeText + }Minor side note: the loop at 1250-1259 lets the last matching label win if both
HIGH_PRIORITYandLOW_PRIORITYare somehow present; probably impossible in practice, but worth abreakor explicit precedence if you want to be defensive.Also applies to: 1244-1259
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/helpers.go` around lines 1150 - 1175, buildMessageOutput currently copies raw priority_type via pickSafeMessageFields(msg) but only replaces priority_type_text from normalized.PriorityTypeText, causing mismatches when label-derived priority overrides are applied; update buildMessageOutput to also set out["priority_type"] = normalized.PriorityType (or the corresponding normalized field) whenever normalized.PriorityTypeText is used so both fields stay consistent, and adjust the label-derivation loop (the code that inspects label_ids / HIGH_PRIORITY / LOW_PRIORITY inside buildMessageForCompose or the label-to-priority helper) to stop after the first match or enforce explicit precedence (e.g., break when a priority label is found) to avoid last-wins ambiguity.
🧹 Nitpick comments (4)
shortcuts/mail/mail_forward.go (1)
89-92: Good placement of early validation.Parsing
--prioritybefore the network fetch on line 100 lets invalid values fail fast without hitting the backend.One minor suggestion: since
parsePriorityis a pure input validation, consider also invoking it insideValidate(likevalidateSendTime) so--dry-runand pre-flight validation also surface invalid priority values without reachingExecute.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_forward.go` around lines 89 - 92, Add priority validation into the command's Validate path so invalid --priority values surface during pre-flight/dry-run; call parsePriority(runtime.Str("priority")) inside the Validate method (similar to how validateSendTime is used) and return the error if parsing fails, and keep the existing parse in Execute or remove duplicate if you prefer single validation point (ensure Execute still assumes a valid parsed priority or re-parse consistently).shortcuts/mail/mail_reply.go (1)
81-84: LGTM — fail-fast priority parse mirrorsmail_forward.go. Same optional suggestion applies: consider duplicating this check inValidatefor parity withvalidateSendTime.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_reply.go` around lines 81 - 84, Add the same priority parsing check into the type's Validate method so validation fails fast like validateSendTime: call parsePriority with the runtime value (runtime.Str("priority")) inside Validate, return any error from parsePriority, and avoid mutating state there—this mirrors the parsePriority usage around priority in mail_reply.go and keeps parity with validateSendTime.shortcuts/mail/mail_draft_create_test.go (1)
36-36: Consider adding a test that exercises a non-empty priority value.All existing call sites pass
""for the newpriorityargument, which exercises the no-op branch ofapplyPriority. Per the coding guideline "Every behavior change must have an accompanying test", consider adding a case withpriority = "1"(or via the full--priority highflow) to assert that anX-Cli-Priorityheader is present in the built EML, ensuring the new behavior has direct coverage.As per coding guidelines: "Every behavior change must have an accompanying test".
Also applies to: 61-61, 96-96, 116-116, 136-136, 156-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_draft_create_test.go` at line 36, Add a test case in mail_draft_create_test.go that calls buildRawEMLForDraftCreate (or the higher-level flow that feeds it) with a non-empty priority value (e.g., "1" or the full CLI flow that maps to --priority high) so applyPriority's non-no-op branch is exercised; assert the resulting raw EML contains an X-Cli-Priority header with the expected value to verify the new behavior. Locate existing calls to buildRawEMLForDraftCreate (lines where "" is passed for priority) and add a parallel test using priority "1", checking for the header in the produced EML output. Ensure the test uses the same helpers/newRuntimeWithFrom and input setup as the other cases and fails if X-Cli-Priority is missing or wrong.skills/lark-mail/references/lark-mail-draft-edit.md (1)
73-73: Consider clarifying--set-priority normalsemantics.The description says
normal"清除已有优先级" which accurately reflects theremove_header X-Cli-Priorityop per the referenced snippet inshortcuts/mail/mail_draft_edit.go:280-291. However, users may expectnormalto mean "set to normal" rather than "clear". A brief note, e.g. "normal等同于未设置优先级，会移除X-Cli-Priority头" would reduce ambiguity and align with the--prioritydocs in sibling commands.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/lark-mail/references/lark-mail-draft-edit.md` at line 73, Update the `--set-priority` row in skills/lark-mail/references/lark-mail-draft-edit.md to clarify that `normal` does not explicitly set a "normal" priority but instead clears any existing priority header: state that "`normal` 等同于未设置优先级，会移除 `X-Cli-Priority` 头" (matching the remove_header X-Cli-Priority behavior in shortcuts/mail/mail_draft_edit.go), and note parity with the `--priority` flag semantics in sibling commands to avoid user confusion.
🤖 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/mail/helpers.go`:
- Around line 1722-1751: Add unit tests for parsePriority and applyPriority: for
parsePriority assert "high"→"1", "low"→"5", "normal" and ""→"" and that an
invalid value returns an error; for applyPriority assert that when priority==""
the original emlbuilder.Builder is returned unchanged and when priority!="", the
builder contains the "X-Cli-Priority" header with the expected value (use the
real emlbuilder.Builder or a small test double that records Header calls). Also
add an integration test in the compose shortcut tests (e.g.,
mail_draft_create_test.go) that exercises the priorityFlag parsing and
application path end-to-end so a compose with --priority high results in an
outgoing message/header with X-Cli-Priority "1". Reference functions/variables:
parsePriority, applyPriority, and priorityFlag.
---
Outside diff comments:
In `@shortcuts/mail/helpers.go`:
- Around line 1150-1175: buildMessageOutput currently copies raw priority_type
via pickSafeMessageFields(msg) but only replaces priority_type_text from
normalized.PriorityTypeText, causing mismatches when label-derived priority
overrides are applied; update buildMessageOutput to also set
out["priority_type"] = normalized.PriorityType (or the corresponding normalized
field) whenever normalized.PriorityTypeText is used so both fields stay
consistent, and adjust the label-derivation loop (the code that inspects
label_ids / HIGH_PRIORITY / LOW_PRIORITY inside buildMessageForCompose or the
label-to-priority helper) to stop after the first match or enforce explicit
precedence (e.g., break when a priority label is found) to avoid last-wins
ambiguity.
---
Nitpick comments:
In `@shortcuts/mail/mail_draft_create_test.go`:
- Line 36: Add a test case in mail_draft_create_test.go that calls
buildRawEMLForDraftCreate (or the higher-level flow that feeds it) with a
non-empty priority value (e.g., "1" or the full CLI flow that maps to --priority
high) so applyPriority's non-no-op branch is exercised; assert the resulting raw
EML contains an X-Cli-Priority header with the expected value to verify the new
behavior. Locate existing calls to buildRawEMLForDraftCreate (lines where "" is
passed for priority) and add a parallel test using priority "1", checking for
the header in the produced EML output. Ensure the test uses the same
helpers/newRuntimeWithFrom and input setup as the other cases and fails if
X-Cli-Priority is missing or wrong.
In `@shortcuts/mail/mail_forward.go`:
- Around line 89-92: Add priority validation into the command's Validate path so
invalid --priority values surface during pre-flight/dry-run; call
parsePriority(runtime.Str("priority")) inside the Validate method (similar to
how validateSendTime is used) and return the error if parsing fails, and keep
the existing parse in Execute or remove duplicate if you prefer single
validation point (ensure Execute still assumes a valid parsed priority or
re-parse consistently).
In `@shortcuts/mail/mail_reply.go`:
- Around line 81-84: Add the same priority parsing check into the type's
Validate method so validation fails fast like validateSendTime: call
parsePriority with the runtime value (runtime.Str("priority")) inside Validate,
return any error from parsePriority, and avoid mutating state there—this mirrors
the parsePriority usage around priority in mail_reply.go and keeps parity with
validateSendTime.
In `@skills/lark-mail/references/lark-mail-draft-edit.md`:
- Line 73: Update the `--set-priority` row in
skills/lark-mail/references/lark-mail-draft-edit.md to clarify that `normal`
does not explicitly set a "normal" priority but instead clears any existing
priority header: state that "`normal` 等同于未设置优先级，会移除 `X-Cli-Priority` 头"
(matching the remove_header X-Cli-Priority behavior in
shortcuts/mail/mail_draft_edit.go), and note parity with the `--priority` flag
semantics in sibling commands to avoid user confusion.
🪄 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: 9b94d466-c562-4989-a98d-ae4e16e3c313
📒 Files selected for processing (14)
shortcuts/mail/helpers.goshortcuts/mail/mail_draft_create.goshortcuts/mail/mail_draft_create_test.goshortcuts/mail/mail_draft_edit.goshortcuts/mail/mail_forward.goshortcuts/mail/mail_reply.goshortcuts/mail/mail_reply_all.goshortcuts/mail/mail_send.goskills/lark-mail/references/lark-mail-draft-create.mdskills/lark-mail/references/lark-mail-draft-edit.mdskills/lark-mail/references/lark-mail-forward.mdskills/lark-mail/references/lark-mail-reply-all.mdskills/lark-mail/references/lark-mail-reply.mdskills/lark-mail/references/lark-mail-send.md
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@156259cbf49b7082bef71947d825d13f95f0b63c🧩 Skill updatenpx skills add larksuite/cli#feat/mail-priority -y -g |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/mail/helpers_test.go (1)
1116-1147: Add assertions for error-path return value and expand priority test coverage.Two minor gaps in test completeness:
TestParsePriorityerror cases (lines 1098–1099) return early without asserting thatgotis empty. A future regression that returns a non-empty string alongside an error would pass the test. Add a check:if got != "" { t.Errorf("expected empty got on error, got %q", got) }before returning.
TestApplyPriorityonly exercises the"1"priority branch. Add a second case testingapplyPriority(highBld, "5")to verify thelowpriority mapping is preserved end-to-end (line 1130), locking down the full user-visible contract.(Note:
decodeBase64URLhelper exists atshortcuts/mail/helpers.go:1563, so no compile error.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/helpers_test.go` around lines 1116 - 1147, In TestParsePriority add an assertion in the error-path branch to assert the returned got string is empty (e.g. if got != "" { t.Errorf("expected empty got on error, got %q", got) }) before returning so a non-empty value with an error fails; and in TestApplyPriority add a second case that calls applyPriority (same builder flow as the "1" case) with priority "5" and assert the resulting decoded EML contains the exact header "X-Cli-Priority: 5" to exercise the low-priority mapping end-to-end (referencing TestParsePriority, TestApplyPriority, applyPriority, and the got variable).
🤖 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/mail/helpers_test.go`:
- Around line 1116-1147: In TestParsePriority add an assertion in the error-path
branch to assert the returned got string is empty (e.g. if got != "" {
t.Errorf("expected empty got on error, got %q", got) }) before returning so a
non-empty value with an error fails; and in TestApplyPriority add a second case
that calls applyPriority (same builder flow as the "1" case) with priority "5"
and assert the resulting decoded EML contains the exact header "X-Cli-Priority:
5" to exercise the low-priority mapping end-to-end (referencing
TestParsePriority, TestApplyPriority, applyPriority, and the got variable).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e127cd1b-03c6-481d-9b09-1569dd1eed85
📒 Files selected for processing (2)
shortcuts/mail/helpers_test.goshortcuts/mail/mail_draft_create_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/mail/mail_draft_create_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (3)
shortcuts/mail/mail_draft_edit_test.go (3)
75-80: Consider asserting the invalid-priority error is the parse error, not an unrelated failure.
TestBuildDraftEditPatch_InvalidPriorityonly checkserr != nil, so the test would still pass ifbuildDraftEditPatchstarted failing earlier (e.g., during an unrelated validation step) without ever reachingparsePriority. Asserting on a sentinel error or a substring like"priority"/"urgent"would tighten the contract.- if _, err := buildDraftEditPatch(rt); err == nil { - t.Fatal("expected error for invalid --set-priority value") - } + _, err := buildDraftEditPatch(rt) + if err == nil { + t.Fatal("expected error for invalid --set-priority value") + } + if !strings.Contains(err.Error(), "priority") { + t.Fatalf("error does not reference priority: %v", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_draft_edit_test.go` around lines 75 - 80, Update TestBuildDraftEditPatch_InvalidPriority to assert the failure comes from parsing priority rather than any other step: call buildDraftEditPatch(rt) as before but check err is the expected parse error (either compare to the sentinel ErrInvalidPriority if one exists or assert the error string contains a clear indicator like "priority" or the invalid value "urgent"); reference buildDraftEditPatch and parsePriority (or ErrInvalidPriority) so the test fails only when the priority parsing isn't triggered.
23-25: Consider surfacingFlags().Seterrors in test helper.Silently discarding the error from
cmd.Flags().Set(name, val)means a typo in a flag name (e.g., future rename ofset-priority) would cause the test helper to set nothing and the assertions to fail with a confusing message rather than pointing at the real cause. At.Fatalfon error here would make such regressions easier to diagnose.♻️ Proposed refactor
-func newDraftEditRuntime(flags map[string]string) *common.RuntimeContext { +func newDraftEditRuntime(t *testing.T, flags map[string]string) *common.RuntimeContext { + t.Helper() cmd := &cobra.Command{Use: "test"} for _, name := range []string{ "set-subject", "set-to", "set-cc", "set-bcc", "set-priority", "patch-file", } { cmd.Flags().String(name, "", "") } for name, val := range flags { - _ = cmd.Flags().Set(name, val) + if err := cmd.Flags().Set(name, val); err != nil { + t.Fatalf("set flag %q=%q: %v", name, val, err) + } } return &common.RuntimeContext{Cmd: cmd} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_draft_edit_test.go` around lines 23 - 25, The loop that sets CLI flags currently ignores errors from cmd.Flags().Set(name, val); modify the test helper so that each call checks the returned error and calls t.Fatalf (or t.Fatalff) with a clear message including the flag name and error when non-nil. Specifically update the block iterating over flags to handle the error from cmd.Flags().Set(name, val) instead of discarding it, so typos or renamed flags fail fast and point at the real cause.
50-59: AssertOpandNamealongsideValuefor the low-priority case.Unlike the
highandnormaltests, this one only checksValue != "5"and omitsOp == "set_header"andName == "X-Cli-Priority". A regression that emitted, say,remove_headerwith value"5"would still pass. Mirroring the assertions fromTestBuildDraftEditPatch_SetPriorityHighwould keep coverage symmetric.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_draft_edit_test.go` around lines 50 - 59, The test TestBuildDraftEditPatch_SetPriorityLow only checks the patch.Ops[0].Value and should also assert the operation and header name like the high/normal tests; update TestBuildDraftEditPatch_SetPriorityLow to assert patch.Ops has length 1 and that patch.Ops[0].Op == "set_header", patch.Ops[0].Name == "X-Cli-Priority", and patch.Ops[0].Value == "5" so it mirrors TestBuildDraftEditPatch_SetPriorityHigh and prevents regressions that change the Op or Name.
🤖 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/mail/mail_draft_edit_test.go`:
- Around line 75-80: Update TestBuildDraftEditPatch_InvalidPriority to assert
the failure comes from parsing priority rather than any other step: call
buildDraftEditPatch(rt) as before but check err is the expected parse error
(either compare to the sentinel ErrInvalidPriority if one exists or assert the
error string contains a clear indicator like "priority" or the invalid value
"urgent"); reference buildDraftEditPatch and parsePriority (or
ErrInvalidPriority) so the test fails only when the priority parsing isn't
triggered.
- Around line 23-25: The loop that sets CLI flags currently ignores errors from
cmd.Flags().Set(name, val); modify the test helper so that each call checks the
returned error and calls t.Fatalf (or t.Fatalff) with a clear message including
the flag name and error when non-nil. Specifically update the block iterating
over flags to handle the error from cmd.Flags().Set(name, val) instead of
discarding it, so typos or renamed flags fail fast and point at the real cause.
- Around line 50-59: The test TestBuildDraftEditPatch_SetPriorityLow only checks
the patch.Ops[0].Value and should also assert the operation and header name like
the high/normal tests; update TestBuildDraftEditPatch_SetPriorityLow to assert
patch.Ops has length 1 and that patch.Ops[0].Op == "set_header",
patch.Ops[0].Name == "X-Cli-Priority", and patch.Ops[0].Value == "5" so it
mirrors TestBuildDraftEditPatch_SetPriorityHigh and prevents regressions that
change the Op or Name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 036d2d93-ed39-49b4-bfc6-053e724f5ea7
📒 Files selected for processing (2)
shortcuts/mail/helpers_test.goshortcuts/mail/mail_draft_edit_test.go
✅ Files skipped from review due to trivial changes (1)
- shortcuts/mail/helpers_test.go
Write: all compose shortcuts (+send, +reply, +reply-all, +forward, +draft-create) accept --priority (high/normal/low) which sets the X-Cli-Priority EML header. +draft-edit accepts --set-priority. Read: normalizeMessage now infers priority from label_ids (HIGH_PRIORITY/LOW_PRIORITY), with priority_type as fallback. Change-Id: Ib5bc4e99331c6ce0d3850865825fcd1ff2183f0c Co-Authored-By: AI
Update 6 skill reference docs: +send, +reply, +reply-all, +forward, +draft-create add --priority param; +draft-edit adds --set-priority. Change-Id: I75d13fbf6a5ca4dfbf76e84fe39e4ee55b689751 Co-Authored-By: AI
- helpers_test.go: cover parsePriority (valid/invalid/case/whitespace) and applyPriority (empty vs non-empty) end-to-end via EML builder - mail_draft_create_test.go: verify --priority propagates to X-Cli-Priority header in the built EML, and no header when priority is empty Change-Id: I62ca96b3e296b5898798cfa681f5efd4f101cb40 Co-Authored-By: AI
…priority - helpers_test.go: TestBuildMessageOutput_PriorityFromLabels verifies HIGH_PRIORITY/LOW_PRIORITY labels map to priority_type_text, and that label values override the priority_type fallback field - mail_draft_edit_test.go (new): cover --set-priority high/low/normal (set_header vs remove_header), invalid value rejection, and absence of priority op when the flag is unused Change-Id: Idd5ace2fb812cf3eb329c79eeab3c8b9808fcf0b Co-Authored-By: AI
chanthuang
force-pushed
the
feat/mail-priority
branch
from
1f0fa47 to
4601c60
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/mail/mail_draft_edit_test.go (1)
50-59: Consider assertingOpandNamein the low-priority test for parity.
TestBuildDraftEditPatch_SetPriorityLowonly checksValue == "5", while the high/normal tests also assertOp/Name. Tightening this gives symmetric coverage and catches regressions that would swap header name or op type without changing the value.♻️ Suggested diff
- if len(patch.Ops) != 1 || patch.Ops[0].Value != "5" { - t.Fatalf("expected single set_header with value 5, got %+v", patch.Ops) - } + if len(patch.Ops) != 1 { + t.Fatalf("expected 1 op, got %d", len(patch.Ops)) + } + op := patch.Ops[0] + if op.Op != "set_header" || op.Name != "X-Cli-Priority" || op.Value != "5" { + t.Errorf("expected set_header X-Cli-Priority=5, got %+v", op) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_draft_edit_test.go` around lines 50 - 59, TestBuildDraftEditPatch_SetPriorityLow only checks patch.Ops[0].Value and should also assert the operation type and header name for parity with the high/normal tests; update the test to verify patch.Ops has length 1 and that patch.Ops[0].Op equals the expected op (e.g., "set_header") and patch.Ops[0].Name equals the expected header key (e.g., "X-Priority" or whatever the other tests expect) in addition to Value == "5", using the same assertions style as the other priority tests and keeping the call to buildDraftEditPatch and error handling unchanged.
🤖 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/mail/mail_draft_edit_test.go`:
- Around line 50-59: TestBuildDraftEditPatch_SetPriorityLow only checks
patch.Ops[0].Value and should also assert the operation type and header name for
parity with the high/normal tests; update the test to verify patch.Ops has
length 1 and that patch.Ops[0].Op equals the expected op (e.g., "set_header")
and patch.Ops[0].Name equals the expected header key (e.g., "X-Priority" or
whatever the other tests expect) in addition to Value == "5", using the same
assertions style as the other priority tests and keeping the call to
buildDraftEditPatch and error handling unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eb592d0b-8fb8-4036-aec9-b4ef792c1502
📒 Files selected for processing (16)
shortcuts/mail/helpers.goshortcuts/mail/helpers_test.goshortcuts/mail/mail_draft_create.goshortcuts/mail/mail_draft_create_test.goshortcuts/mail/mail_draft_edit.goshortcuts/mail/mail_draft_edit_test.goshortcuts/mail/mail_forward.goshortcuts/mail/mail_reply.goshortcuts/mail/mail_reply_all.goshortcuts/mail/mail_send.goskills/lark-mail/references/lark-mail-draft-create.mdskills/lark-mail/references/lark-mail-draft-edit.mdskills/lark-mail/references/lark-mail-forward.mdskills/lark-mail/references/lark-mail-reply-all.mdskills/lark-mail/references/lark-mail-reply.mdskills/lark-mail/references/lark-mail-send.md
✅ Files skipped from review due to trivial changes (7)
- skills/lark-mail/references/lark-mail-draft-edit.md
- skills/lark-mail/references/lark-mail-send.md
- shortcuts/mail/helpers_test.go
- skills/lark-mail/references/lark-mail-reply-all.md
- skills/lark-mail/references/lark-mail-forward.md
- skills/lark-mail/references/lark-mail-reply.md
- skills/lark-mail/references/lark-mail-draft-create.md
🚧 Files skipped from review as they are similar to previous changes (5)
- shortcuts/mail/mail_reply_all.go
- shortcuts/mail/mail_draft_create_test.go
- shortcuts/mail/helpers.go
- shortcuts/mail/mail_draft_edit.go
- shortcuts/mail/mail_forward.go
buildMessageOutput only wrote priority_type_text but not priority_type when priority was inferred from HIGH_PRIORITY/LOW_PRIORITY labels. Also covers the case where label overrides an explicit priority_type field. Change-Id: I7879976d21235b8006b5c8ebe6a413e2815354e1
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/mail/helpers.go (1)
1251-1260: Minor: label iteration order determines priority when both labels present.If
label_idssomehow contains bothHIGH_PRIORITYandLOW_PRIORITY, the last one in the slice wins, which makes the derived priority dependent on API ordering. In practice the backend is unlikely to emit both, but a small guard (e.g., preferHIGH_PRIORITYon first match andbreak) would make behavior deterministic.♻️ Optional hardening
- for _, label := range out.LabelIDs { - switch label { - case "HIGH_PRIORITY": - out.PriorityType = "1" - out.PriorityTypeText = "high" - case "LOW_PRIORITY": - out.PriorityType = "5" - out.PriorityTypeText = "low" - } - } + for _, label := range out.LabelIDs { + if label == "HIGH_PRIORITY" { + out.PriorityType, out.PriorityTypeText = "1", "high" + break + } + if label == "LOW_PRIORITY" { + out.PriorityType, out.PriorityTypeText = "5", "low" + // don't break; allow a later HIGH_PRIORITY to win + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/helpers.go` around lines 1251 - 1260, The loop over out.LabelIDs currently lets the last matching label set out.PriorityType and out.PriorityTypeText, making priority dependent on slice order; update the logic in the for _, label := range out.LabelIDs / switch label block so that when a HIGH_PRIORITY match is found it sets out.PriorityType="1" and out.PriorityTypeText="high" and then stops further processing (e.g., break out of the loop or return), ensuring HIGH_PRIORITY wins deterministically; alternatively, check for HIGH_PRIORITY first before applying LOW_PRIORITY to guarantee a consistent result.
🤖 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/mail/helpers.go`:
- Around line 1251-1260: The loop over out.LabelIDs currently lets the last
matching label set out.PriorityType and out.PriorityTypeText, making priority
dependent on slice order; update the logic in the for _, label := range
out.LabelIDs / switch label block so that when a HIGH_PRIORITY match is found it
sets out.PriorityType="1" and out.PriorityTypeText="high" and then stops further
processing (e.g., break out of the loop or return), ensuring HIGH_PRIORITY wins
deterministically; alternatively, check for HIGH_PRIORITY first before applying
LOW_PRIORITY to guarantee a consistent result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5e848387-1e60-4af1-8ea2-133c9c92dcaa
📒 Files selected for processing (2)
shortcuts/mail/helpers.goshortcuts/mail/helpers_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/mail/helpers_test.go
…ore dry-run/Execute Change-Id: Ic277ab683967c47f28c892d3512b0ab745bd86f6
…rejected in Validate Change-Id: I7f12c0a0b0d15c491c28fdcb8729f2f648ba0244
2 participants
Summary
Add email priority support to the mail module:
+send,+reply,+reply-all,+forward,+draft-create) accept a new--priorityflag (high/normal/low). When set, the draft's EML carries anX-Cli-Priorityheader that the backend uses to apply the priority.+draft-editadds a--set-prioritydirect flag consistent with--set-subject/--set-tonaming.+messagenow surfaces priority inpriority_type/priority_type_text, inferred from theHIGH_PRIORITY/LOW_PRIORITYlabels inlabel_ids(with the existingpriority_typefield kept as a fallback).normaland omitted--priorityare equivalent — no header is set.Changes
shortcuts/mail/helpers.go: addpriorityFlag,parsePriority,applyPriority; extendnormalizeMessageto derive priority fromlabel_ids.shortcuts/mail/mail_send.go/mail_reply.go/mail_reply_all.go/mail_forward.go/mail_draft_create.go: registerpriorityFlag, apply the header when building the EML.shortcuts/mail/mail_draft_edit.go: register--set-priority; translate it toset_header X-Cli-Priorityorremove_header X-Cli-Priority.Test plan
go build ./...go test ./shortcuts/mail/...--priority high/--priority low/ omitted; reading back to seepriority_type_textandlabel_ids;+draft-edit --set-priority high/--set-priority normal(clears priority).Backward compatibility
--prioritykeeps behavior identical to before (no header).--priority normalis equivalent to omitting it.🤖 Generated with Claude Code
Summary by CodeRabbit