feat: support minutes search#359
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:
📝 WalkthroughWalkthroughAdds a new "minutes +search" shortcut with input validation, request/params/body builders, DryRun and Execute flows calling POST /open-apis/minutes/v1/minutes/search, response parsing (items/total/has_more/page_token), table/JSON output formatting, comprehensive unit tests, and updated skill docs and references. Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as CLI Handler
participant Validator as Validator
participant Builder as Request Builder
participant API as Minutes API
participant Formatter as Output Formatter
User->>CLI: minutes +search --query="..." [filters]
CLI->>Validator: Validate inputs (query length, page size, time range, IDs)
alt Validation fails
Validator-->>CLI: error
CLI-->>User: display error
else Validation passes
Validator-->>CLI: ok
CLI->>Builder: build params & body (parse times, resolve 'me', dedupe IDs)
Builder-->>CLI: prepared request
CLI->>API: POST /open-apis/minutes/v1/minutes/search
API-->>CLI: {items, total, has_more, page_token}
CLI->>Formatter: format results (table or json)
alt has_more and non-json
Formatter-->>CLI: append "more available" + page_token
end
Formatter-->>User: display results or "No minutes."
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
shortcuts/minutes/minutes_search_test.go (1)
179-188: Missingt.Parallel()in test function.This test function is missing
t.Parallel()unlike other test functions in this file. While not a bug, adding it would be consistent with the rest of the test file and enable parallel execution.♻️ Add t.Parallel() for consistency
func TestMinutesSearchValidationMeOwnerID(t *testing.T) { + t.Parallel() + cmd := newMinutesSearchTestCommand() _ = cmd.Flags().Set("owner-ids", "me")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/minutes/minutes_search_test.go` around lines 179 - 188, Add t.Parallel() to the start of the TestMinutesSearchValidationMeOwnerID test function to match other tests and enable parallel execution; locate the TestMinutesSearchValidationMeOwnerID function in minutes_search_test.go and insert a call to t.Parallel() as the first statement (before creating cmd or calling MinutesSearch.Validate).shortcuts/minutes/minutes_search.go (2)
49-55: Silently ignoredtime.Parseerrors could mask subtle bugs.The errors from
time.Parseon lines 50-51 are discarded with_. While this code path is only reached after successfultoRFC3339calls (which should guarantee valid RFC3339 strings), silently ignoring parse errors could mask issues if the upstream parsing behavior changes.Consider adding a defensive check or at minimum a comment explaining why the error is safe to ignore.
💡 Suggested improvement
if startTime != "" && endTime != "" { - st, _ := time.Parse(time.RFC3339, startTime) - et, _ := time.Parse(time.RFC3339, endTime) + st, err1 := time.Parse(time.RFC3339, startTime) + et, err2 := time.Parse(time.RFC3339, endTime) + if err1 != nil || err2 != nil { + // Should not happen: toRFC3339 guarantees valid RFC3339 output + return "", "", output.ErrValidation("internal: failed to re-parse time") + } if st.After(et) { return "", "", output.ErrValidation("--start (%s) is after --end (%s)", start, end) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/minutes/minutes_search.go` around lines 49 - 55, The time.Parse calls for startTime and endTime currently ignore errors (st, _ := time.Parse(...), et, _ := time.Parse(...)); add defensive error handling by capturing the parse errors and returning an appropriate validation error via output.ErrValidation when either parse fails (e.g., check errSt and errEt after calling time.Parse), or if you prefer to keep the current behavior, add a clear comment explaining why the error can be safely ignored because toRFC3339 guarantees validity; reference the variables startTime/endTime, the time.Parse calls that produce st/et, and use output.ErrValidation for consistent error reporting.
254-256: Silently ignoredstrconv.Atoierror.The error from
strconv.Atoion line 255 is discarded. IfbuildMinutesSearchParamsreturns a non-numericpage_sizevalue (which shouldn't happen given the validation), this would silently use0as the page size.This is low-risk since
Validateis called first and enforces validpage-size, but adding a comment or defensive handling would improve robustness.💡 Suggested improvement
params := map[string]interface{}{} - pageSize, _ := strconv.Atoi(buildMinutesSearchParams(runtime).Get("page_size")) + // page_size is validated in Validate(), Atoi won't fail + pageSize, _ := strconv.Atoi(buildMinutesSearchParams(runtime).Get("page_size")) params["page_size"] = pageSize🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/minutes/minutes_search.go` around lines 254 - 256, The strconv.Atoi error from converting buildMinutesSearchParams(...).Get("page_size") is ignored; update the page_size parsing in minutes_search.go to handle the error instead of silently using 0: call strconv.Atoi, check the returned err and either return/propagate the error (from the enclosing function) or set a safe default and log/debug the unexpected value; specifically modify the code around params := map[string]interface{}{}, the pageSize variable assignment and params["page_size"] to handle non-numeric values returned by buildMinutesSearchParams, referencing the pageSize and params symbols so the fix is applied in the correct spot.
🤖 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/minutes/minutes_search_test.go`:
- Around line 179-188: Add t.Parallel() to the start of the
TestMinutesSearchValidationMeOwnerID test function to match other tests and
enable parallel execution; locate the TestMinutesSearchValidationMeOwnerID
function in minutes_search_test.go and insert a call to t.Parallel() as the
first statement (before creating cmd or calling MinutesSearch.Validate).
In `@shortcuts/minutes/minutes_search.go`:
- Around line 49-55: The time.Parse calls for startTime and endTime currently
ignore errors (st, _ := time.Parse(...), et, _ := time.Parse(...)); add
defensive error handling by capturing the parse errors and returning an
appropriate validation error via output.ErrValidation when either parse fails
(e.g., check errSt and errEt after calling time.Parse), or if you prefer to keep
the current behavior, add a clear comment explaining why the error can be safely
ignored because toRFC3339 guarantees validity; reference the variables
startTime/endTime, the time.Parse calls that produce st/et, and use
output.ErrValidation for consistent error reporting.
- Around line 254-256: The strconv.Atoi error from converting
buildMinutesSearchParams(...).Get("page_size") is ignored; update the page_size
parsing in minutes_search.go to handle the error instead of silently using 0:
call strconv.Atoi, check the returned err and either return/propagate the error
(from the enclosing function) or set a safe default and log/debug the unexpected
value; specifically modify the code around params := map[string]interface{}{},
the pageSize variable assignment and params["page_size"] to handle non-numeric
values returned by buildMinutesSearchParams, referencing the pageSize and params
symbols so the fix is applied in the correct spot.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f687ad60-cf42-4081-8800-18e8f844cb7e
📒 Files selected for processing (6)
shortcuts/minutes/minutes_search.goshortcuts/minutes/minutes_search_test.goshortcuts/minutes/shortcuts.goskills/lark-minutes/SKILL.mdskills/lark-minutes/references/lark-minutes-search.mdskills/lark-vc/references/lark-vc-search.md
Greptile SummaryThis PR adds a
Confidence Score: 4/5Safe to merge after correcting the max page-size claim in SKILL.md from 200 to 30. One P1 documentation inconsistency: SKILL.md claims 200 items per page but the CLI enforces 30. This would cause AI agents reading the skill to generate failing requests. All code logic, validation, output formatting, and tests look correct. skills/lark-minutes/SKILL.md — max page-size routing rule needs updating from 200 to 30. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant CLI as lark-cli minutes +search
participant Validate
participant Execute
participant API as POST /open-apis/minutes/v1/minutes/search
User->>CLI: --query / --owner-ids / --participant-ids / --start / --end
CLI->>Validate: parseTimeRange, ValidatePageSize, resolveUserIDs
Validate-->>CLI: error or ok
CLI->>Execute: buildMinutesSearchParams (page_size, page_token)
Execute->>Execute: buildMinutesSearchBody (query + filter)
Execute->>API: POST with params + body
API-->>Execute: items, total, has_more, page_token
Execute->>User: pretty table / JSON / CSV (more available hint if has_more)
Reviews (10): Last reviewed commit: "docs(skills): require shortcut reference..." | Re-trigger Greptile |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@8af28e529dd4dd4997d43791c37e0fbeb3336aa7🧩 Skill updatenpx skills add larksuite/cli#feat/minutes-search -y -g |
|
Tip: Greploop — Automatically fix all review issues by running Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/minutes/minutes_search_test.go`:
- Around line 68-70: Remove the redundant loop-variable shadowing "tt := tt"
inside the test loop so the closure uses the loop variable captured from "for _,
tt := range tests" directly; locate the for-loop that iterates over tests and
the nested t.Run(...) closures (instances where "for _, tt := range tests { tt
:= tt t.Run(...)" occurs) and delete the "tt := tt" lines at all occurrences
(the ones flagged in the review), leaving the t.Run closures to reference tt
without reassigning it.
🪄 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: b586dc77-1fa0-4072-9f2c-ca7628f68690
📒 Files selected for processing (4)
shortcuts/minutes/minutes_search.goshortcuts/minutes/minutes_search_test.goskills/lark-minutes/references/lark-minutes-search.mdskills/lark-vc/references/lark-vc-recording.md
✅ Files skipped from review due to trivial changes (2)
- skills/lark-vc/references/lark-vc-recording.md
- skills/lark-minutes/references/lark-minutes-search.md
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/minutes/minutes_search.go
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/minutes/minutes_search.go`:
- Around line 342-344: The pagination hint is being written to stdout
(fmt.Fprintf(runtime.IO().Out,...)) which can corrupt piped program output;
change the write target to stderr instead by using the runtime's error output
(e.g., runtime.IO().Err) so the "(more available, page_token: %s)" message is
sent to stderr while preserving the existing condition that skips json/empty
formats and using the same pageToken variable.
In `@skills/lark-minutes/references/lark-minutes-search.md`:
- Line 70: The documentation line for the `--page-size` flag is inconsistent
with the code constant `maxMinutesSearchPageSize = 200`; update the table entry
so the max value shows 200 (and the range text "1-200") and keep the default as
15 to match the flag parsing and the `maxMinutesSearchPageSize` constant used by
the minutes search implementation.
- Line 164: The sentence "必须使用 `--format json` 输出,你更佳擅长解析 JSON 数据。" contains a
typo: replace "更佳" with "更加" so it reads "必须使用 `--format json` 输出,你更加擅长解析 JSON
数据。"; locate the exact string in the markdown and update the wording
accordingly.
- Line 173: The markdown link "[lark-vc] -- 视频会议全部命令" in
skills/lark-minutes/references/lark-minutes-search.md currently points to
../SKILL.md (resolving to the minutes skill); update the link target to the VC
skill's SKILL.md (e.g., ../lark-vc/SKILL.md) by locating the link text
"[lark-vc]" in that file and replacing the destination path so it points to the
lark-vc skill's SKILL.md.
🪄 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: 764ef00d-5dc0-49c5-9b5d-55620b1f66d9
📒 Files selected for processing (4)
shortcuts/minutes/minutes_search.goshortcuts/minutes/minutes_search_test.goskills/lark-minutes/SKILL.mdskills/lark-minutes/references/lark-minutes-search.md
✅ Files skipped from review due to trivial changes (1)
- shortcuts/minutes/minutes_search_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- skills/lark-minutes/SKILL.md
Summary
Add a minutes search shortcut that supports keyword, owner, participant, and
time-range filters. Align the output fields with the minutes search schema and
clarify same-day query behavior in the related skill docs.
Changes
minutes +searchshortcut and related testsminutes_search_itemandminutes_metatoken,display_info,description,app_link, andavataras output fieldspage-sizewith the shared page-size validation pattern--endsemantics and same-day queriesTest Plan
make buildmake unit-testgo mod tidyproduced no changes togo.modorgo.sumgo run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.1.6 run --new-from-rev=origin/maingo test -count=1 ./shortcuts/minutes/...lark-cli minutes +searchworks as expectedRelated Issues
Summary by CodeRabbit
New Features
minutes +searchto search/filter minutes by keyword, owner, participants, and time range; supports pagination (default 15, max 200),--page-token,--dry-run,mealias, and formatted table/JSON output (prints "No minutes." when empty).Documentation
--end→ end-of-day), pagination guidance, permissions, and troubleshooting.Tests
Chores