feat(mail): expose draft priority in --inspect projection#779
Conversation
📝 WalkthroughWalkthroughAdds DraftProjection.Priority and logic to parse and normalize priority from headers (prefer X-Cli-Priority, fallback X-Priority), assigns it in Project(), adds tests, and updates CLI inspect/template output. ChangesMail draft priority projection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
github-actions
Bot
added
domain/mail
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/mail/mail_draft_edit.go (1)
251-295:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
priorityis missing from the human-readable--inspecttext output.The JSON envelope will include
priority(it's part ofDraftProjection), but the text-mode callback never prints it. Since the PR goal is to expose priority in--inspect, text-format users won't see it at all.🐛 Proposed fix
if len(projection.Warnings) > 0 { fmt.Fprintf(w, "warnings: %s\n", sanitizeForTerminal(strings.Join(projection.Warnings, "; "))) } + if projection.Priority != "" && projection.Priority != "unknown" { + fmt.Fprintf(w, "priority: %s\n", sanitizeForTerminal(projection.Priority)) + } })🤖 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_draft_edit.go` around lines 251 - 295, The human-readable inspect output in the runtime.OutFormat callback for draft inspection is missing the DraftProjection priority field; update the callback in mail_draft_edit.go (inside the runtime.OutFormat anonymous func) to print projection.Priority (use the same sanitizeForTerminal + fmt.Fprintf pattern as other fields), e.g. add a conditional that emits "priority: <value>\n" when projection.Priority is non-empty so text-mode --inspect matches the JSON envelope.
🧹 Nitpick comments (2)
shortcuts/mail/draft/projection.go (1)
177-188: 💤 Low value
"normal"string form is not mapped — returns"unknown"for non-CLI writers.The CLI write-side removes
X-Cli-Priorityfor--set-priority normal, so this path won't be hit by CLI-authored drafts. However, an external OAPI client that writesX-Cli-Priority: normal(orX-Priority: normal) would incorrectly project to"unknown"instead of"normal". Consider adding the alias if the backendPriorityValueToTypevocabulary includes it.🛡️ Proposed fix
switch strings.ToLower(strings.TrimSpace(raw)) { case "1", "high", "1 (highest)": return "high" - case "3": + case "3", "normal": return "normal" case "5", "low", "5 (lowest)": return "low"🤖 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/draft/projection.go` around lines 177 - 188, mapPriorityValue currently maps several textual/numeric inputs to internal priority types but omits the plain "normal" token, causing external OAPI/HTTP writers sending X-Cli-Priority: normal or X-Priority: normal to be projected as "unknown"; update mapPriorityValue to treat "normal" (case-insensitive, trimmed) as the same as "3" by adding "normal" to the switch case that returns "normal" so it aligns with the backend PriorityValueToType vocabulary and existing CLI behavior.shortcuts/mail/draft/model.go (1)
169-169: 💤 Low value
omitemptyis a no-op here — consider dropping it for clarity.
parsePriorityFromHeadersalways returns a non-empty string (minimum"unknown"), soomitemptynever suppresses the field. The PR description says the goal is "a stable read-side surface"; removingomitemptymakes that guarantee self-documenting in the struct tag.- Priority string `json:"priority,omitempty"` + Priority string `json:"priority"`🤖 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/draft/model.go` at line 169, The JSON tag on the Priority field currently uses `omitempty` even though parsePriorityFromHeaders always returns a non-empty string (at least "unknown"), so `omitempty` is misleading; update the struct field `Priority` in shortcuts/mail/draft/model.go to remove `,omitempty` from its `json:"priority,omitempty"` tag (make it `json:"priority"`) so the read-side surface is stable and self-documenting, and keep parsePriorityFromHeaders behavior 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.
Outside diff comments:
In `@shortcuts/mail/mail_draft_edit.go`:
- Around line 251-295: The human-readable inspect output in the
runtime.OutFormat callback for draft inspection is missing the DraftProjection
priority field; update the callback in mail_draft_edit.go (inside the
runtime.OutFormat anonymous func) to print projection.Priority (use the same
sanitizeForTerminal + fmt.Fprintf pattern as other fields), e.g. add a
conditional that emits "priority: <value>\n" when projection.Priority is
non-empty so text-mode --inspect matches the JSON envelope.
---
Nitpick comments:
In `@shortcuts/mail/draft/model.go`:
- Line 169: The JSON tag on the Priority field currently uses `omitempty` even
though parsePriorityFromHeaders always returns a non-empty string (at least
"unknown"), so `omitempty` is misleading; update the struct field `Priority` in
shortcuts/mail/draft/model.go to remove `,omitempty` from its
`json:"priority,omitempty"` tag (make it `json:"priority"`) so the read-side
surface is stable and self-documenting, and keep parsePriorityFromHeaders
behavior unchanged.
In `@shortcuts/mail/draft/projection.go`:
- Around line 177-188: mapPriorityValue currently maps several textual/numeric
inputs to internal priority types but omits the plain "normal" token, causing
external OAPI/HTTP writers sending X-Cli-Priority: normal or X-Priority: normal
to be projected as "unknown"; update mapPriorityValue to treat "normal"
(case-insensitive, trimmed) as the same as "3" by adding "normal" to the switch
case that returns "normal" so it aligns with the backend PriorityValueToType
vocabulary and existing CLI behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: af93c6af-c710-4b94-a7fe-0a8fbe7cd07e
📒 Files selected for processing (4)
shortcuts/mail/draft/model.goshortcuts/mail/draft/projection.goshortcuts/mail/draft/projection_test.goshortcuts/mail/mail_draft_edit.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #779 +/- ##
==========================================
+ Coverage 65.40% 66.94% +1.54%
==========================================
Files 508 568 +60
Lines 46795 52990 +6195
==========================================
+ Hits 30605 35475 +4870
- Misses 13548 14584 +1036
- Partials 2642 2931 +289 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@f2ea65ea64818ccad47c44ceb60a3abc092fad43🧩 Skill updatenpx skills add xzcong0820/larksuite-cli#harness/01kqenandss7ynchapf94ck9d -y -g |
chanthuang
left a comment
chanthuang
left a comment
There was a problem hiding this comment.
Found one issue that should be addressed before merge:
mapPriorityValue only maps normal priority when the raw header is exactly 3, but this PR explicitly supports the X-Priority fallback for historical/imported drafts and already accepts Outlook-style strings for high/low (1 (Highest), 5 (Lowest)). The normal counterparts (Normal, 3 (Normal)) currently project as unknown, so a draft with an explicit normal priority loses that signal while high/low do not. Please include normal and 3 (normal) in the mapping and add a regression test for the fallback/header string form.
xzcong0820
force-pushed
the
harness/01kqenandss7ynchapf94ck9d
branch
2 times, most recently
from
83614de to
34fd79f
Compare
…t --set-priority Add a Priority field to DraftProjection populated from the EML header pair X-Cli-Priority (CLI/OAPI primary) → X-Priority (RFC fallback for IMAP-回灌 historical drafts), with case-insensitive lookup via the existing headerValue helper and a local mapping table aligned with the backend gopkg/mail_priority.PriorityValueToType vocabulary. When neither header is present (the symmetric read of --set-priority normal=remove_header) the projection emits "unknown" so agents have a stable read-side surface. Append one notes entry to buildDraftEditPatchTemplate documenting the --set-priority flag and the X-Cli-Priority translation contract. The write-side (--set-priority flag, parsePriority helper, translation branch in mail_draft_edit.go, EML header target) is unchanged — already shipped on master. sprint: S4
xzcong0820
force-pushed
the
harness/01kqenandss7ynchapf94ck9d
branch
from
34fd79f to
f2ea65e
Compare
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/draft/projection_test.go`:
- Around line 219-233: The test TestProjectPriorityBothAbsentNormal and its
assertion are out of sync with the PR contract: when both headers are absent
Project(snapshot) should set proj.Priority to "unknown". Rename the test and/or
comment to reflect "both absent => unknown" and change the assertion in
TestProjectPriorityBothAbsentNormal (which calls Project(snapshot) and checks
proj.Priority) to expect "unknown" instead of "normal".
🪄 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: 3dd71fab-cdca-4d37-bcfc-56d334ef59ff
📒 Files selected for processing (4)
shortcuts/mail/draft/model.goshortcuts/mail/draft/projection.goshortcuts/mail/draft/projection_test.goshortcuts/mail/mail_draft_edit.go
🚧 Files skipped from review as they are similar to previous changes (2)
- shortcuts/mail/draft/projection.go
- shortcuts/mail/mail_draft_edit.go
2 participants
Generated by the harness-coding skill.
Add a Priority field to DraftProjection populated from X-Cli-Priority (CLI/OAPI primary) → X-Priority (RFC fallback for IMAP-回灌 historical drafts), with case-insensitive lookup via the existing headerValue helper. When neither header is present, the projection emits 'normal' — absence of a priority header is the standard email convention for normal priority, symmetric with how mail clients interpret untagged messages.
Append one notes entry to buildDraftEditPatchTemplate documenting the --set-priority flag and the X-Cli-Priority translation contract.
The write-side (--set-priority flag, parsePriority helper, translation branch in mail_draft_edit.go, EML header target) is unchanged — already shipped on master.