feat(mail): add --page-token and --page-size to mail +triage#301
feat(mail): add --page-token and --page-size to mail +triage#301haidaodashushu wants to merge 1 commit intomainfrom
Conversation
…il +triage Support external pagination for mail +triage with two new flags: - --page-token: resume from a previous response's page token - --page-size: alias for --max Token carries a "search:" or "list:" prefix to identify the API path, with strict validation: conflicting parameters (e.g. list: token with --query) fail fast, and bare tokens without prefix are rejected. JSON/data output now returns an object with messages, total, has_more, and page_token fields. Table output shows next-page hint on stderr.
📝 WalkthroughWalkthroughThe PR introduces pagination support for the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MailTriage as Mail Triage Command
participant PathResolver as Path Resolver
participant SearchAPI as Search API
participant ListAPI as List API
participant Output as Output Formatter
User->>MailTriage: Execute with --page-token and --query/--filter
MailTriage->>PathResolver: resolveTriagePath(token, query, filter)
alt Token has "search:" prefix
PathResolver->>PathResolver: Strip "search:" prefix
PathResolver->>MailTriage: Return search path
MailTriage->>SearchAPI: buildSearchParams with stripped token
SearchAPI->>SearchAPI: Execute search, get results + nextToken
SearchAPI->>MailTriage: Return messages + nextToken
MailTriage->>MailTriage: Wrap nextToken as "search:<token>"
else Token has "list:" prefix
PathResolver->>PathResolver: Strip "list:" prefix, validate no query
PathResolver->>MailTriage: Return list path
MailTriage->>ListAPI: buildListParams with stripped token
ListAPI->>ListAPI: Execute list, get results + nextToken
ListAPI->>MailTriage: Return messages + nextToken
MailTriage->>MailTriage: Wrap nextToken as "list:<token>"
else No token or empty token
PathResolver->>PathResolver: Determine path from query presence
PathResolver->>MailTriage: Return search or list path
end
MailTriage->>Output: Format with pagination metadata
Output->>User: Emit messages + total + has_more + page_token
alt has_more is true
Output->>User: Emit next-page command hint to stderr
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 docstrings
🧪 Generate unit tests (beta)
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.
🧹 Nitpick comments (2)
skills/lark-mail/references/lark-mail-triage.md (1)
103-106: Add language specifier to fenced code block.The code block is missing a language identifier. Since this shows shell/terminal output, use an appropriate specifier.
-``` +```text 15 message(s) next page: mail +triage --page-token 'list:FfccvoqPd...' ... tip: use mail +message --message-id <id> to read full content🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/lark-mail/references/lark-mail-triage.md` around lines 103 - 106, The fenced code block that currently contains the terminal output starting with "15 message(s)" should include a language specifier to indicate plain text; update the triple-backtick fence for that block to use a language tag such as "text" (e.g., change ``` to ```text) so the shell/terminal output is rendered correctly.shortcuts/mail/mail_triage_test.go (1)
1052-1060: Duplicate test coverage for bare token rejection.
TestResolveTriagePathBareTokenRejected(lines 1052-1060) andTestPageTokenBareTokenRejected(lines 1097-1105) test the same scenario. Consider consolidating to reduce test duplication.Also applies to: 1097-1105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_triage_test.go` around lines 1052 - 1060, Two tests duplicate the same scenario: TestResolveTriagePathBareTokenRejected and TestPageTokenBareTokenRejected both assert that a bare token without a prefix causes an error; consolidate by removing or merging one of them. Keep a single test (either TestResolveTriagePathBareTokenRejected or TestPageTokenBareTokenRejected) that calls resolveTriagePath("baretoken123", "", triageFilter{}) and asserts err != nil and that err.Error() contains "prefix"; update or remove the duplicate test accordingly and ensure any helper names referenced (resolveTriagePath, TestResolveTriagePathBareTokenRejected, TestPageTokenBareTokenRejected) are adjusted so there is only one coverage point for this case.
🤖 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_triage_test.go`:
- Around line 1052-1060: Two tests duplicate the same scenario:
TestResolveTriagePathBareTokenRejected and TestPageTokenBareTokenRejected both
assert that a bare token without a prefix causes an error; consolidate by
removing or merging one of them. Keep a single test (either
TestResolveTriagePathBareTokenRejected or TestPageTokenBareTokenRejected) that
calls resolveTriagePath("baretoken123", "", triageFilter{}) and asserts err !=
nil and that err.Error() contains "prefix"; update or remove the duplicate test
accordingly and ensure any helper names referenced (resolveTriagePath,
TestResolveTriagePathBareTokenRejected, TestPageTokenBareTokenRejected) are
adjusted so there is only one coverage point for this case.
In `@skills/lark-mail/references/lark-mail-triage.md`:
- Around line 103-106: The fenced code block that currently contains the
terminal output starting with "15 message(s)" should include a language
specifier to indicate plain text; update the triple-backtick fence for that
block to use a language tag such as "text" (e.g., change ``` to ```text) so the
shell/terminal output is rendered correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5898bb3f-2cdc-4b1d-a7ba-a8b3f559a1d3
📒 Files selected for processing (3)
shortcuts/mail/mail_triage.goshortcuts/mail/mail_triage_test.goskills/lark-mail/references/lark-mail-triage.md
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@236249eb6d70c18d0657df7c2699a353ca975b42🧩 Skill updatenpx skills add larksuite/cli#feat/mail-triage-pagination -y -g |
Greptile SummaryThis PR adds external pagination controls ( The one backward-incompatible change to note: Confidence Score: 5/5Safe to merge; all remaining findings are P2 and do not block correctness. Core pagination logic (prefix routing, conflict detection, hasMore/nextPageToken tracking, token stripping) is correct and well-covered by tests. The two P2 items are a documented breaking output-format change and a whitespace-query edge case in an already-commented guard. shortcuts/mail/mail_triage.go — output format change at lines 253-261 and whitespace-query guard at line 897 are worth a second look if backward compatibility is a concern. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant CLI
participant resolveTriagePath
participant ListAPI as Lark List API
participant BatchGet as Lark batch_get API
User->>CLI: mail +triage --page-token list:abc --max 10
CLI->>resolveTriagePath: (token=list:abc, query empty)
resolveTriagePath-->>CLI: useSearch=false, err=nil
CLI->>ListAPI: GET /messages?page_token=abc&page_size=10
ListAPI-->>CLI: {items, has_more=true, page_token=xyz}
CLI->>BatchGet: POST /messages/batch_get {message_ids}
BatchGet-->>CLI: {messages}
CLI-->>User: {messages, total=10, has_more=true, page_token=list:xyz}
Reviews (1): Last reviewed commit: "feat(mail): add --page-token and --page-..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| switch outFormat { | ||
| case "json", "data": | ||
| output.PrintJson(runtime.IO().Out, messages) | ||
| outData := map[string]interface{}{ | ||
| "messages": messages, | ||
| "total": len(messages), | ||
| "has_more": hasMore, | ||
| "page_token": nextPageToken, | ||
| } | ||
| output.PrintJson(runtime.IO().Out, outData) |
There was a problem hiding this comment.
--format json / --format data
The JSON/data output shape changed from a bare array to a {messages, total, has_more, page_token} envelope. Any existing script piping through jq '.[].subject' or .[0].from will silently produce a type error at runtime. The skill doc is updated, but downstream consumers have no in-band deprecation signal.
If backward compat is needed for a transition period, consider keeping --format data as the flat-array mode (its original pipe-friendliness purpose) and using the new envelope for --format json only.
| case strings.HasPrefix(pageToken, "search:"): | ||
| if !paramWantsSearch && (query != "" || len(triageQueryFilterFields(filter)) > 0) { | ||
| // This shouldn't normally happen (query/search fields → paramWantsSearch=true), | ||
| // but guard against future changes. | ||
| return false, fmt.Errorf("--page-token has search: prefix but current --query/--filter parameters indicate list path; remove conflicting parameters or use the correct token") | ||
| } |
There was a problem hiding this comment.
search: prefix guard
The condition query != "" is not equivalent to strings.TrimSpace(query) != "" used inside usesTriageSearchPath. A caller passing --page-token search:xyz --query " " (whitespace-only query) will hit this branch — paramWantsSearch is false (trimmed query is empty) while query != "" is true — and get a misleading error claiming the params indicate the list path, even though search: is valid for a continuation. Aligning the guard with the same trim logic closes the gap:
| case strings.HasPrefix(pageToken, "search:"): | |
| if !paramWantsSearch && (query != "" || len(triageQueryFilterFields(filter)) > 0) { | |
| // This shouldn't normally happen (query/search fields → paramWantsSearch=true), | |
| // but guard against future changes. | |
| return false, fmt.Errorf("--page-token has search: prefix but current --query/--filter parameters indicate list path; remove conflicting parameters or use the correct token") | |
| } | |
| if !paramWantsSearch && (strings.TrimSpace(query) != "" || len(triageQueryFilterFields(filter)) > 0) { |
1 participant
Summary
--page-tokenand--page-sizeflags tomail +triagefor external pagination controlsearch:orlist:prefix to identify API path, with strict validation (conflicting params fail fast, bare tokens rejected){ messages, total, has_more, page_token }object; table shows next-page hint on stderrTest plan
resolveTriagePath: search/list prefix routing, bare token rejection, conflict detectionresolveTriagePageSize: --page-size priority over --max, clamping--max 40baseline--query "周报" --max 30baseline--queryworksgo test ./shortcuts/mail/...)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
--page-sizeflag to control results per page--page-tokenflag for pagination continuationtotal,has_more,page_token)Documentation