feat(mail): preserve mailbox context in +triage output for public mailboxes#1238
Conversation
|
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:
📝 WalkthroughWalkthroughThe PR injects ChangesMail triage mailbox context
Sequence Diagram(s)(omitted — change is localized and does not introduce a multi-component sequential flow requiring visualization) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@0a59ebf6c58f818ee55887c1cc1e11d8d2f51585🧩 Skill updatenpx skills add xukuncx/cli#feat/889956e -y -g |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1238 +/- ##
==========================================
+ Coverage 69.19% 69.83% +0.64%
==========================================
Files 637 666 +29
Lines 59753 64423 +4670
==========================================
+ Hits 41345 44991 +3646
- Misses 15067 15812 +745
- Partials 3341 3620 +279 ☔ View full report in Codecov by Harness. 🚀 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.
🧹 Nitpick comments (1)
shortcuts/mail/mail_triage_test.go (1)
2274-2303: 💤 Low valueConsider explicitly verifying that stdout does not contain mailbox_id.
The test name and comment indicate that table format with empty results should not output
mailbox_id. The test currently checks that stderr contains "No messages found" but does not explicitly verify that stdout is empty or lacksmailbox_id.While the implementation returns early without printing to stdout (making this implicitly correct), an explicit assertion would make the test's intent clearer.
📝 Suggested enhancement
errOut := stderr.String() if !strings.Contains(errOut, "No messages found") { t.Fatalf("expected 'No messages found' in stderr for empty table result, got stdout:\n%s\nstderr:\n%s", stdout.String(), errOut) } + out := stdout.String() + if strings.Contains(out, "mailbox_id") { + t.Fatalf("table output for empty results should not contain 'mailbox_id', got:\n%s", out) + } }🤖 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_triage_test.go` around lines 2274 - 2303, The test TestMailTriageTableEmptyResultNoMailboxID should explicitly assert that stdout does not contain a mailbox_id to match its intent; after the existing stderr assertion, add an assertion checking stdout.String() either equals "" or does not contain "mailbox_id" (e.g., fail the test if strings.Contains(stdout.String(), "mailbox_id")), referencing stdout and the test name so the check is colocated with the current stderr verification.
🤖 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.
Nitpick comments:
In `@shortcuts/mail/mail_triage_test.go`:
- Around line 2274-2303: The test TestMailTriageTableEmptyResultNoMailboxID
should explicitly assert that stdout does not contain a mailbox_id to match its
intent; after the existing stderr assertion, add an assertion checking
stdout.String() either equals "" or does not contain "mailbox_id" (e.g., fail
the test if strings.Contains(stdout.String(), "mailbox_id")), referencing stdout
and the test name so the check is colocated with the current stderr
verification.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b62e78c-5f63-440b-8ef9-9507076eea29
📒 Files selected for processing (1)
shortcuts/mail/mail_triage_test.go
xukuncx
force-pushed
the
feat/889956e
branch
3 times, most recently
from
bd4936c to
5e4ddf3
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 `@tests/cli_e2e/mail/mail_triage_dryrun_test.go`:
- Line 37: The assertion expects an unescaped mailbox ID but mailboxPath() uses
url.PathEscape, so update the test to compare against the URL-encoded mailbox
string (or construct the expected URL using url.PathEscape) rather than the raw
"postmaster@larksuite.com"; adjust both assertions that check api.0.url (the
line at require.Equal and the similar one at line 42) to use the escaped mailbox
(e.g., call url.PathEscape or otherwise percent-encode the mailbox) so the
expected value matches the dry-run output.
🪄 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: 88265f2e-06ba-48f7-898e-5271f7caaec0
📒 Files selected for processing (2)
shortcuts/mail/mail_triage_test.gotests/cli_e2e/mail/mail_triage_dryrun_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/mail/mail_triage_test.go
…lboxes
When triaging a public/shared mailbox, downstream AI consumers (e.g.
mail +message) need the mailbox_id to construct correct API paths.
Previously the triage output only included message_id, causing
/user_mailboxes/me/messages/{id} lookups that fail for public mailboxes.
- Add mailbox_id field to every normalized message in structured output
- Add mailbox_id to top-level JSON/data output envelope
- Add mailbox_id to table rows when mailbox is not "me"
- Update stderr next-step tip to include --mailbox for non-me mailboxes
- Update next-page hint to include --mailbox for non-me mailboxes
- Add unit tests covering list, search, and public mailbox paths
- Update triage skill docs to show mailbox_id in output examples
chanthuang
left a comment
chanthuang
left a comment
There was a problem hiding this comment.
LGTM. Fix preserves mailbox context from mail +triage output for public/shared mailboxes, and the added tests/docs look solid.
3 participants
Generated by the harness-coding skill.
Sprints
Source specs
This MR was created autonomously. Quality gates were enforced by the repo's own pre-commit hooks.
Summary by CodeRabbit
New Features
Documentation
Tests