feat: enhance calendar event search and room finding#679
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 pretty-formatted table output for calendar event creation and normalizes Changes
Sequence Diagram(s)(Skipped — changes are localized formatting, validation, tests, and documentation updates and do not introduce new multi-component control flows requiring sequencing.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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/calendar
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #679 +/- ##
==========================================
+ Coverage 62.92% 62.93% +0.01%
==========================================
Files 484 484
Lines 41522 41541 +19
==========================================
+ Hits 26127 26144 +17
- Misses 13117 13118 +1
- Partials 2278 2279 +1 ☔ 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
🧹 Nitpick comments (3)
shortcuts/calendar/calendar_room_find_test.go (1)
11-31: Solid coverage fornormalizeCommaSeparatedNames.The cases cover pass-through, whitespace trimming, empty/delimiter-only inputs, and consecutive-comma compaction. All expectations match the implementation in
calendar_room_find.go.Optional nit: consider adding a
namefield to the table for clearer failure output when a single case fails (e.g. viat.Run(tt.name, ...)), and a case asserting that full-width commas (，) are intentionally not treated as separators — the doc and CLI help both specify English commas, so locking that contract down with a test would prevent accidental drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/calendar/calendar_room_find_test.go` around lines 11 - 31, Add a descriptive `name` field to each test case and use t.Run(tt.name, func(t *testing.T){...}) when iterating so failures show which case failed (reference: TestNormalizeCommaSeparatedNames and normalizeCommaSeparatedNames), and add an explicit test case asserting that full-width commas (`，`) are NOT treated as separators (e.g., input "a，b" expecting "a，b") to lock the documented contract that only ASCII commas are separators.shortcuts/calendar/calendar_room_find.go (1)
318-333: Per-segment validation is equivalent to validating the raw string — consider simplifying.
common.RejectDangerousCharsiterates the entire input rune-by-rune (seeshortcuts/common/validate.go:95-110), and,(U+002C) is not flagged as dangerous. Splitting the input and validating each non-empty segment therefore produces the same accept/reject set as validating the raw string once.Two options to consider:
- Add
flagRoomNameback to the simple loop at line 318 and drop the per-segment loop entirely.- Or, if you keep the split for clearer error provenance per segment, reuse
normalizeCommaSeparatedNamesto avoid duplicating split/trim logic acrossValidateandbuildRoomFindBaseRequest.Not a correctness issue — purely DRY/clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/calendar/calendar_room_find.go` around lines 318 - 333, The per-segment validation for flagRoomName is redundant because common.RejectDangerousChars("--"+flagRoomName, ...) applied to the raw string yields the same accept/reject result as validating each comma-separated segment; update Validate to include flagRoomName in the initial flags loop (the loop over flagCity, flagBuilding, flagFloor, flagEventRrule, flagTimezone) and remove the subsequent per-segment loop, or alternatively if you want per-segment error messages reuse normalizeCommaSeparatedNames to split/trim before calling common.RejectDangerousChars to avoid duplicating split/trim logic between Validate and buildRoomFindBaseRequest.shortcuts/calendar/calendar_create.go (1)
291-296: Optional: simplify single-row table construction.
var rows []…; rows = append(rows, resultData)can be a one-liner; no behavior change.♻️ Proposed simplification
runtime.OutFormat(resultData, nil, func(w io.Writer) { - var rows []map[string]interface{} - rows = append(rows, resultData) - output.PrintTable(w, rows) + output.PrintTable(w, []map[string]interface{}{resultData}) fmt.Fprintln(w, "\nEvent created successfully") })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/calendar/calendar_create.go` around lines 291 - 296, In the runtime.OutFormat(...) callback that builds a single-row table for output (the block calling output.PrintTable), replace the two-step declaration and append of rows (var rows []map[string]interface{}; rows = append(rows, resultData)) with a single-line literal construction (e.g., rows := []map[string]interface{}{resultData}) to simplify and clarify the creation of the slice before calling output.PrintTable.
🤖 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/calendar/calendar_create.go`:
- Line 76: Add a unit/integration test that exercises the new formatted-output
path introduced by setting HasFormat: true in calendar_create (the
runtime.OutFormat rendering code around the OutFormat branch). Specifically, add
a test that runs the calendar +create command with the --format flag (e.g.,
--format pretty and/or --format json) and asserts the formatted output matches
expectations; follow existing tests for CalendarAgenda or CalendarRoomFind as a
template to verify both "pretty" table rendering and JSON output if applicable.
---
Nitpick comments:
In `@shortcuts/calendar/calendar_create.go`:
- Around line 291-296: In the runtime.OutFormat(...) callback that builds a
single-row table for output (the block calling output.PrintTable), replace the
two-step declaration and append of rows (var rows []map[string]interface{}; rows
= append(rows, resultData)) with a single-line literal construction (e.g., rows
:= []map[string]interface{}{resultData}) to simplify and clarify the creation of
the slice before calling output.PrintTable.
In `@shortcuts/calendar/calendar_room_find_test.go`:
- Around line 11-31: Add a descriptive `name` field to each test case and use
t.Run(tt.name, func(t *testing.T){...}) when iterating so failures show which
case failed (reference: TestNormalizeCommaSeparatedNames and
normalizeCommaSeparatedNames), and add an explicit test case asserting that
full-width commas (`，`) are NOT treated as separators (e.g., input "a，b"
expecting "a，b") to lock the documented contract that only ASCII commas are
separators.
In `@shortcuts/calendar/calendar_room_find.go`:
- Around line 318-333: The per-segment validation for flagRoomName is redundant
because common.RejectDangerousChars("--"+flagRoomName, ...) applied to the raw
string yields the same accept/reject result as validating each comma-separated
segment; update Validate to include flagRoomName in the initial flags loop (the
loop over flagCity, flagBuilding, flagFloor, flagEventRrule, flagTimezone) and
remove the subsequent per-segment loop, or alternatively if you want per-segment
error messages reuse normalizeCommaSeparatedNames to split/trim before calling
common.RejectDangerousChars to avoid duplicating split/trim logic between
Validate and buildRoomFindBaseRequest.
🪄 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: cdb6e74a-3269-4935-be16-9a5ff51bfb6f
📒 Files selected for processing (5)
shortcuts/calendar/calendar_create.goshortcuts/calendar/calendar_room_find.goshortcuts/calendar/calendar_room_find_test.goskills/lark-calendar/SKILL.mdskills/lark-calendar/references/lark-calendar-room-find.md
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@d5079e990130952f71f400feefbe36fbaccfdbfd🧩 Skill updatenpx skills add larksuite/cli#feat/room_and_search_event -y -g |
hugang-lark
force-pushed
the
feat/room_and_search_event
branch
from
21f8f47 to
a65158a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
shortcuts/calendar/calendar_room_find.go (1)
325-333: ReusenormalizeCommaSeparatedNamesto avoid duplicated split/trim/skip logic.The validation loop re-implements the split +
TrimSpace+ skip-empty behavior already encapsulated innormalizeCommaSeparatedNames. Consider extracting a slice-returning helper (e.g.splitCommaSeparatedNames) and havenormalizeCommaSeparatedNamesjoin its result, so both validation and request building share one source of truth.♻️ Proposed refactor
+func splitCommaSeparatedNames(raw string) []string { + parts := strings.Split(raw, ",") + cleaned := make([]string, 0, len(parts)) + for _, p := range parts { + if p = strings.TrimSpace(p); p != "" { + cleaned = append(cleaned, p) + } + } + return cleaned +} + func normalizeCommaSeparatedNames(raw string) string { - parts := strings.Split(raw, ",") - var cleaned []string - for _, p := range parts { - p = strings.TrimSpace(p) - if p != "" { - cleaned = append(cleaned, p) - } - } - return strings.Join(cleaned, ",") + return strings.Join(splitCommaSeparatedNames(raw), ",") } @@ - for _, name := range strings.Split(runtime.Str(flagRoomName), ",") { - name = strings.TrimSpace(name) - if name == "" { - continue - } - if err := common.RejectDangerousChars("--"+flagRoomName, name); err != nil { - return output.ErrValidation(err.Error()) - } - } + for _, name := range splitCommaSeparatedNames(runtime.Str(flagRoomName)) { + if err := common.RejectDangerousChars("--"+flagRoomName, name); err != nil { + return output.ErrValidation(err.Error()) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/calendar/calendar_room_find.go` around lines 325 - 333, The loop that validates room names duplicates split/trim/skip logic; extract a helper (e.g., splitCommaSeparatedNames) that takes runtime.Str(flagRoomName) and returns a []string of non-empty, trimmed names, refactor normalizeCommaSeparatedNames to call join on that slice, and then replace the current validation loop with iteration over splitCommaSeparatedNames's result while calling common.RejectDangerousChars("--"+flagRoomName, name) for each entry so both validation and request building share the same parsing logic.
🤖 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/calendar/calendar_room_find.go`:
- Around line 208-225: The room_name must be a single exact name, not a CSV;
update buildRoomFindBaseRequest to stop passing a comma-joined list from
normalizeCommaSeparatedNames and instead set roomFindRequest.RoomName to the
first non-empty trimmed token (or empty string) from runtime.Str(flagRoomName),
and remove or stop calling normalizeCommaSeparatedNames from this flow; if
multi-room search is required later, replace this flow with a call to the
meeting_room/freebusy/batch_get endpoint using room IDs rather than attempting
CSVs in roomFindRequest.
---
Nitpick comments:
In `@shortcuts/calendar/calendar_room_find.go`:
- Around line 325-333: The loop that validates room names duplicates
split/trim/skip logic; extract a helper (e.g., splitCommaSeparatedNames) that
takes runtime.Str(flagRoomName) and returns a []string of non-empty, trimmed
names, refactor normalizeCommaSeparatedNames to call join on that slice, and
then replace the current validation loop with iteration over
splitCommaSeparatedNames's result while calling
common.RejectDangerousChars("--"+flagRoomName, name) for each entry so both
validation and request building share the same parsing logic.
🪄 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: 4cb61d36-2880-4f3e-9593-5b213624cd51
📒 Files selected for processing (2)
shortcuts/calendar/calendar_room_find.goshortcuts/calendar/calendar_test.go
hugang-lark
force-pushed
the
feat/room_and_search_event
branch
from
a65158a to
a610160
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
shortcuts/calendar/calendar_room_find.go (1)
208-225:⚠️ Potential issue | 🔴 CriticalMulti-room CSV
--room-nameis still broken — prior critical feedback unaddressed.A previous review on this exact range already established (with web verification) that the upstream
/open-apis/calendar/v4/freebusy/room_findAPI treatsroom_nameas a single string for exact normalized matching. Joining tokens with,and sending"16,17,18,19,20"(or"木星,火星") makes the backend look for a single literal room named"16,17,18,19,20", which never matches anything. The CSV semantics this PR is trying to ship cannot work against the current endpoint.This is the root cause; the following changes in this PR are downstream effects of the same defect and should be reverted/redesigned together:
--room-namehelp text at line 287 advertising comma-separated multiple names.- Per-token validation loop at lines 325–333 (only meaningful if CSV is supported).
- The new "批量会议室名称查询" section and rules in
skills/lark-calendar/references/lark-calendar-room-find.md, plus theXX~YYrange expansion guidance — all instruct the agent to produce inputs the API will reject.Two viable directions:
- Drop the multi-room feature: keep
RoomNameas a single trimmed value, restore the original whole-string validation, and revert the doc additions.- Implement true multi-room search by issuing one
room_findcall per token and merging results client-side (similar to the existing per-slot fan-out viacollectRoomFindResults/roomFindWorkers), or by switching tomeeting_room/freebusy/batch_getwith room IDs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/calendar/calendar_room_find.go` around lines 208 - 225, The code currently splits --room-name into a comma-joined string via normalizeCommaSeparatedNames and sets roomFindRequest.RoomName to a CSV which the upstream room_find API treats as a single literal name; fix by choosing one of two approaches: (A) revert to single-name behavior by removing normalizeCommaSeparatedNames usage in buildRoomFindBaseRequest and pass the raw trimmed runtime.Str(flagRoomName) (and revert the per-token validation and CSV help/docs), or (B) implement true multi-room queries by treating runtime.Str(flagRoomName) as a list of tokens, issuing one room_find call per token and merging results client-side (reusing collectRoomFindResults / roomFindWorkers fan-out pattern or switching to the batch_get meeting_room/freebusy endpoint), updating validation/docs accordingly; apply the chosen approach consistently across buildRoomFindBaseRequest, the validation loop, help text, and docs.
🧹 Nitpick comments (1)
shortcuts/calendar/calendar_test.go (1)
151-194: Optional: deduplicate withTestCreate_CreateEventOnlyvia a table-driven test.
TestCreate_CreateEventOnly_PrettyFormatreuses the exact same stub fixture and args asTestCreate_CreateEventOnly, differing only by adding--format prettyand one extra assertion. Consider folding both into a single subtest table to share the stub registration and reduce drift if the fixture changes.Note: if the trailing
"Event created successfully"hint is routed to stderr (see comment incalendar_create.go), the assertion at lines 191–193 will need to read from stderr instead of stdout, andmountAndRunmay need to expose the stderr buffer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/calendar/calendar_test.go` around lines 151 - 194, Two tests (TestCreate_CreateEventOnly_PrettyFormat and TestCreate_CreateEventOnly) duplicate the same HTTP stub and args; refactor into a table-driven test with subtests that share the stub registration and only vary the extra "--format pretty" arg and the additional assertion. Update the test wrapper (mountAndRun) or test invocation to accept and return both stdout and stderr buffers so the subtest that expects the "Event created successfully" hint can assert against stderr if calendar_create.go writes that message to stderr; keep unique identifiers like TestCreate_CreateEventOnly_PrettyFormat, TestCreate_CreateEventOnly, mountAndRun, and calendar_create.go when locating and changing the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@shortcuts/calendar/calendar_room_find.go`:
- Around line 208-225: The code currently splits --room-name into a comma-joined
string via normalizeCommaSeparatedNames and sets roomFindRequest.RoomName to a
CSV which the upstream room_find API treats as a single literal name; fix by
choosing one of two approaches: (A) revert to single-name behavior by removing
normalizeCommaSeparatedNames usage in buildRoomFindBaseRequest and pass the raw
trimmed runtime.Str(flagRoomName) (and revert the per-token validation and CSV
help/docs), or (B) implement true multi-room queries by treating
runtime.Str(flagRoomName) as a list of tokens, issuing one room_find call per
token and merging results client-side (reusing collectRoomFindResults /
roomFindWorkers fan-out pattern or switching to the batch_get
meeting_room/freebusy endpoint), updating validation/docs accordingly; apply the
chosen approach consistently across buildRoomFindBaseRequest, the validation
loop, help text, and docs.
---
Nitpick comments:
In `@shortcuts/calendar/calendar_test.go`:
- Around line 151-194: Two tests (TestCreate_CreateEventOnly_PrettyFormat and
TestCreate_CreateEventOnly) duplicate the same HTTP stub and args; refactor into
a table-driven test with subtests that share the stub registration and only vary
the extra "--format pretty" arg and the additional assertion. Update the test
wrapper (mountAndRun) or test invocation to accept and return both stdout and
stderr buffers so the subtest that expects the "Event created successfully" hint
can assert against stderr if calendar_create.go writes that message to stderr;
keep unique identifiers like TestCreate_CreateEventOnly_PrettyFormat,
TestCreate_CreateEventOnly, mountAndRun, and calendar_create.go when locating
and changing the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d3193b5e-b79c-4e09-a64e-695e331ee4cf
📒 Files selected for processing (6)
shortcuts/calendar/calendar_create.goshortcuts/calendar/calendar_room_find.goshortcuts/calendar/calendar_room_find_test.goshortcuts/calendar/calendar_test.goskills/lark-calendar/SKILL.mdskills/lark-calendar/references/lark-calendar-room-find.md
✅ Files skipped from review due to trivial changes (1)
- shortcuts/calendar/calendar_room_find_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- skills/lark-calendar/SKILL.md
Change-Id: I15ff863fc5de4890b6b3f3d984946b5a60eaef07
hugang-lark
force-pushed
the
feat/room_and_search_event
branch
from
a610160 to
7b63f9f
Compare
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 `@skills/lark-calendar/SKILL.md`:
- Around line 27-30: Remove the duplicated guidance about recurring-event
instance operations so the document only contains a single clear directive;
specifically delete the repeated paragraph that reiterates
"如果涉及到对重复性日程的某个具体实例进行操作...MUST 先定位到对应的那次实例的 `event_id`（可通过`events search_event`
搜索日程，或 `+agenda` 查看对应时间范围的日程等相关查询获取），绝对禁止直接使用原重复性日程的 `event_id` 进行操作。" and keep
the original occurrence (the earlier paragraph) intact to maintain the
instruction without redundancy.
🪄 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: bd777ecb-ab8e-4aa4-9316-243d299c32f0
📒 Files selected for processing (6)
shortcuts/calendar/calendar_create.goshortcuts/calendar/calendar_room_find.goshortcuts/calendar/calendar_room_find_test.goshortcuts/calendar/calendar_test.goskills/lark-calendar/SKILL.mdskills/lark-calendar/references/lark-calendar-room-find.md
✅ Files skipped from review due to trivial changes (1)
- shortcuts/calendar/calendar_room_find_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- skills/lark-calendar/references/lark-calendar-room-find.md
- shortcuts/calendar/calendar_room_find.go
hugang-lark
force-pushed
the
feat/room_and_search_event
branch
from
7b63f9f to
d5079e9
Compare
3 participants
Summary
This PR improves the calendar CLI capabilities by introducing event search support, enabling formatted output for event creation, and allowing users to query multiple meeting rooms at once.
Changes
searchwithsearch_event.--formatflag support forcalendar +createshortcut to enable table output.calendar +room-findand update related documentation.Test Plan
lark-cli calendar events search_event --params '{"calendar_id": "primary"}' --data '{"query": "会议"}'to verify event search functionalitylark-cli calendar +createwith--formatflag and verify the table outputlark-cli calendar +room-find --room-name "A,B"and verify the correct multi-room filteringRelated Issues
Summary by CodeRabbit
New Features
Documentation
Tests