fix: optimize calendar skills and shortcuts#1768
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (28)
💤 Files with no reviewable changes (10)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (11)
📝 WalkthroughWalkthroughAdds a new ChangesCalendar, minutes, and documentation updates
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant CalendarGet
participant CalendarAPI
CLI->>CalendarGet: Execute(calendar-id, event-id)
CalendarGet->>CalendarAPI: GET event
CalendarAPI-->>CalendarGet: event JSON
CalendarGet->>CalendarGet: parseCalendarEvent
CalendarGet->>CalendarGet: buildCalendarEventOutput
CalendarGet-->>CLI: formatted table row
sequenceDiagram
participant CLI
participant MinutesDetail
participant MinutesAPI
CLI->>MinutesDetail: Execute(--wait-ready)
MinutesDetail->>MinutesAPI: fetch metadata/artifacts
MinutesAPI-->>MinutesDetail: processing error (2091003)
loop until ready or timeout
MinutesDetail->>MinutesAPI: retry after interval
MinutesAPI-->>MinutesDetail: processing or ready response
end
MinutesDetail-->>CLI: table row (PROCESSING or success)
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1768 +/- ##
==========================================
- Coverage 74.46% 74.44% -0.03%
==========================================
Files 855 856 +1
Lines 88568 88753 +185
==========================================
+ Hits 65954 66072 +118
- Misses 17543 17599 +56
- Partials 5071 5082 +11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@36c7a64e63d370dcb27ee2ee1e37c59733dfeda3🧩 Skill updatenpx skills add larksuite/cli#feat/latency-skill -y -g |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/lark-calendar/references/lark-calendar-suggestion.md (1)
111-121: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRestore the
+freebusycross-link.The comparison table and selection guidance still mention
calendar +freebusy, but the references block no longer gives readers a way to jump to the matching command doc.🤖 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 `@skills/lark-calendar/references/lark-calendar-suggestion.md` around lines 111 - 121, The `+freebusy` command is still referenced in the comparison table and selection guidance, but the references section no longer links to its matching document. Update the references block in `lark-calendar-suggestion.md` to restore the cross-link for `lark-calendar-freebusy`, matching the existing style used for `lark-calendar-create` and the `SKILL.md` entry so readers can navigate directly to the `+freebusy` doc.
🧹 Nitpick comments (4)
skills/lark-minutes/references/lark-minutes-upload.md (1)
89-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify that
minute_tokenis conditional in the output.The code only adds
minute_tokenwhenextractUploadedMinuteTokensuccessfully parses a token (empty/malformedminute_urlomits the field entirely), but this example and field table present it as always returned. Consumers (including AI agents parsing this doc) may assume the field is guaranteed.📝 Suggested doc clarification
| 字段 | 说明 | |------|------| | `minute_url` | 生成的妙记访问链接 | -| `minute_token` | 从 `minute_url` 提取出的妙记 Token,可直接传给 `minutes +detail --minute-tokens` | +| `minute_token` | 从 `minute_url` 提取出的妙记 Token(仅当能成功解析时返回),可直接传给 `minutes +detail --minute-tokens` |🤖 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 `@skills/lark-minutes/references/lark-minutes-upload.md` around lines 89 - 99, Clarify the upload response schema in lark-minutes-upload.md so `minute_token` is shown as optional/conditional rather than always present. Update the JSON example and the field table near `minute_url`, `minute_token`, and `extractUploadedMinuteToken` to state that `minute_token` is only returned when a token can be parsed from `minute_url`, and is omitted for empty or malformed URLs.shortcuts/calendar/calendar_get.go (1)
122-174: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated timestamp/all-day conversion logic vs.
calendar_create.go.The
create_time→ RFC3339,start_time/end_timetimestamp →datetime, and all-day end-date rewind-by-1s logic here is a near-exact copy of the equivalent block incalendar_create.go(Execute, lines ~263-287). Therrule/recurrencenaming drift above is a direct symptom of this duplication — the two call sites already disagree despite being conceptually the same transformation. Extracting a shared helper (e.g., inshortcuts/calendar) would keep both shortcuts consistent going forward.🤖 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/calendar/calendar_get.go` around lines 122 - 174, The transformation logic in buildCalendarEventOutput is duplicated with the equivalent calendar_create.go path, so the two shortcuts can drift apart (as seen with the rrule/recurrence naming mismatch). Extract the shared create_time/start_time/end_time/all-day normalization into a common helper in shortcuts/calendar, then have both buildCalendarEventOutput and the create flow call that helper so they stay consistent. Keep the helper responsible for the RFC3339 conversion, timestamp removal, and all-day end-date rewind behavior.shortcuts/minutes/minutes_detail_test.go (1)
443-503: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffReal-time sleeps make wait-ready tests slower / mildly flaky.
TestDetail_WaitReady_MetadataEventuallyReadyandTestDetail_WaitReady_ArtifactsEventuallyReadyrely on real--wait-interval-seconds 1waits insidecallMinutesDetailAPIUntilReady, adding real wall-clock delay per test run and coupling correctness to real timer behavior under CI load. Consider parameterizingcallMinutesDetailAPIUntilReadywith an injectable sleep/clock function so these paths can be tested with a fake clock instead of realtime.Sleep/time.NewTimer.🤖 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/minutes/minutes_detail_test.go` around lines 443 - 503, The wait-ready tests are using real wall-clock sleeps, which makes them slow and flaky. Update callMinutesDetailAPIUntilReady to accept an injectable sleep/clock dependency instead of calling time.Sleep/time.NewTimer directly, then thread that dependency through the MinutesDetail path so TestDetail_WaitReady_MetadataEventuallyReady and TestDetail_WaitReady_ArtifactsEventuallyReady can use a fake clock without real delays.shortcuts/minutes/minutes_detail.go (1)
181-204: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueLGTM overall, minor readability nit on deadline boundary.
The retry loop correctly handles context cancellation and deadline expiry. One edge case: when
interval == remainingexactly, the loop sleeps the full interval and performs one extra API call right at/after the nominal deadline — a minor, self-recovering boundary quirk given real-world timing imprecision already dwarfs this.🤖 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/minutes/minutes_detail.go` around lines 181 - 204, The retry boundary in callMinutesDetailAPIUntilReady is a readability/behavior nit: when interval exactly matches the remaining time, the loop still sleeps and retries at the deadline edge. Update the deadline check in callMinutesDetailAPIUntilReady to treat an exact match as expired (or otherwise make the boundary explicit) so the timeout behavior is clear and consistent.
🤖 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/calendar/calendar_get.go`:
- Line 94: `calendarEvent.Recurrence` is still exposed as `recurrence` in
`calendar_get.go`, which conflicts with `calendar +create` using `rrule` for the
same recurrence data. Update `buildCalendarEventOutput` (and any related output
mapping in `calendarEvent`) so `calendar +get` renames the field to `rrule`
consistently, matching the create shortcut’s naming and keeping the shortcut
outputs aligned.
In `@shortcuts/calendar/calendar_test.go`:
- Around line 3285-3321: The all-day event test in
TestGet_AllDayEvent_AdjustsEndDate is asserting a date string that also appears
in start_time, so it does not uniquely verify the end-date adjustment. Update
the assertion to target the end_time field specifically in the output from
CalendarGet, using a more precise check around the event JSON so the test fails
if the rewind-by-1s logic is not applied.
In `@skills/lark-calendar/references/lark-calendar-update.md`:
- Around line 66-73: The combined update flow in the lark-calendar update docs
should be described as all-or-nothing instead of sequential best-effort. Update
the guidance around the +update path to state that the full write sequence must
be atomic, or that compensating rollback is required if a later step fails;
reference the “日程字段 -> 移除参会人 -> 添加参会人” flow and the +update/完整 API 命令 sections
so the documentation matches the intended behavior.
---
Outside diff comments:
In `@skills/lark-calendar/references/lark-calendar-suggestion.md`:
- Around line 111-121: The `+freebusy` command is still referenced in the
comparison table and selection guidance, but the references section no longer
links to its matching document. Update the references block in
`lark-calendar-suggestion.md` to restore the cross-link for
`lark-calendar-freebusy`, matching the existing style used for
`lark-calendar-create` and the `SKILL.md` entry so readers can navigate directly
to the `+freebusy` doc.
---
Nitpick comments:
In `@shortcuts/calendar/calendar_get.go`:
- Around line 122-174: The transformation logic in buildCalendarEventOutput is
duplicated with the equivalent calendar_create.go path, so the two shortcuts can
drift apart (as seen with the rrule/recurrence naming mismatch). Extract the
shared create_time/start_time/end_time/all-day normalization into a common
helper in shortcuts/calendar, then have both buildCalendarEventOutput and the
create flow call that helper so they stay consistent. Keep the helper
responsible for the RFC3339 conversion, timestamp removal, and all-day end-date
rewind behavior.
In `@shortcuts/minutes/minutes_detail_test.go`:
- Around line 443-503: The wait-ready tests are using real wall-clock sleeps,
which makes them slow and flaky. Update callMinutesDetailAPIUntilReady to accept
an injectable sleep/clock dependency instead of calling time.Sleep/time.NewTimer
directly, then thread that dependency through the MinutesDetail path so
TestDetail_WaitReady_MetadataEventuallyReady and
TestDetail_WaitReady_ArtifactsEventuallyReady can use a fake clock without real
delays.
In `@shortcuts/minutes/minutes_detail.go`:
- Around line 181-204: The retry boundary in callMinutesDetailAPIUntilReady is a
readability/behavior nit: when interval exactly matches the remaining time, the
loop still sleeps and retries at the deadline edge. Update the deadline check in
callMinutesDetailAPIUntilReady to treat an exact match as expired (or otherwise
make the boundary explicit) so the timeout behavior is clear and consistent.
In `@skills/lark-minutes/references/lark-minutes-upload.md`:
- Around line 89-99: Clarify the upload response schema in
lark-minutes-upload.md so `minute_token` is shown as optional/conditional rather
than always present. Update the JSON example and the field table near
`minute_url`, `minute_token`, and `extractUploadedMinuteToken` to state that
`minute_token` is only returned when a token can be parsed from `minute_url`,
and is omitted for empty or malformed URLs.
🪄 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: 686dd587-1e5e-470c-b54c-6a1ff84c112c
📒 Files selected for processing (30)
shortcuts/calendar/calendar_create.goshortcuts/calendar/calendar_get.goshortcuts/calendar/calendar_test.goshortcuts/calendar/shortcuts.goshortcuts/minutes/minutes_detail.goshortcuts/minutes/minutes_detail_test.goshortcuts/minutes/minutes_upload.goshortcuts/minutes/minutes_upload_test.goskills/lark-calendar/SKILL.mdskills/lark-calendar/references/lark-calendar-agenda.mdskills/lark-calendar/references/lark-calendar-create.mdskills/lark-calendar/references/lark-calendar-freebusy.mdskills/lark-calendar/references/lark-calendar-room-find.mdskills/lark-calendar/references/lark-calendar-rsvp.mdskills/lark-calendar/references/lark-calendar-schedule-clear-time.mdskills/lark-calendar/references/lark-calendar-schedule-fuzzy-time.mdskills/lark-calendar/references/lark-calendar-schedule-meeting.mdskills/lark-calendar/references/lark-calendar-search-event.mdskills/lark-calendar/references/lark-calendar-suggestion.mdskills/lark-calendar/references/lark-calendar-update.mdskills/lark-minutes/references/lark-minutes-download.mdskills/lark-minutes/references/lark-minutes-search.mdskills/lark-minutes/references/lark-minutes-speaker-replace.mdskills/lark-minutes/references/lark-minutes-summary.mdskills/lark-minutes/references/lark-minutes-todo.mdskills/lark-minutes/references/lark-minutes-update.mdskills/lark-minutes/references/lark-minutes-upload.mdskills/lark-vc/references/lark-vc-recording.mdtests/cli_e2e/calendar/calendar_get_dryrun_test.gotests/cli_e2e/minutes/minutes_detail_dryrun_test.go
💤 Files with no reviewable changes (10)
- skills/lark-minutes/references/lark-minutes-search.md
- skills/lark-calendar/references/lark-calendar-search-event.md
- skills/lark-minutes/references/lark-minutes-speaker-replace.md
- skills/lark-calendar/references/lark-calendar-agenda.md
- skills/lark-calendar/references/lark-calendar-freebusy.md
- skills/lark-minutes/references/lark-minutes-todo.md
- skills/lark-vc/references/lark-vc-recording.md
- skills/lark-minutes/references/lark-minutes-download.md
- skills/lark-minutes/references/lark-minutes-summary.md
- skills/lark-minutes/references/lark-minutes-update.md
584599f to
6d61132
Compare
feat: remove reference lark-sharded skill docs: clarify calendar write feedback docs: split calendar scheduling workflow docs: tighten calendar skill references docs: annotate +freebusy scope to avoid unnecessary reads in scheduling flow When users express "find free time + create event" intent, AI would previously read freebusy.md before entering the scheduling workflow. Adding a scope note to the shortcut table directs AI to use +suggestion instead, reducing token consumption. feat: support calendar +get fix: optimize calendar skill fix: optimize shortcuts and meta api fix: optimize skill
6d61132 to
36c7a64
Compare
Summary
Adds a new
calendar +getshortcut for fetching a single event's full detail, and introduces readiness handling forminutes +detailso callers can wait until a minute's metadata and artifacts finish processing. Along the way, refreshes the calendar skill docs — splitting scheduling into clear-time and fuzzy-time workflows — and returns a few more useful fields from existing shortcuts.Changes
calendar +getshortcut to retrieve a single event bycalendar_idandevent_id.--wait-ready(with--wait-timeout-seconds/--wait-interval-seconds) tominutes +detail; surfacestatus=PROCESSING,retryable,hint, andnext_commandwhen the minute is still being generated.rrulefromcalendar +createoutput.lark-calendarskill: split scheduling workflow into clear-time vs. fuzzy-time references, tighten shortcut references, and annotate+freebusyscope to steer AI toward+suggestionfor scheduling intents.Test Plan
make unit-testpasses.lark-cli calendar +get --calendar-id <cid> --event-id <eid>returns full event detail.lark-cli minutes +detail --minute-tokens <token> --summary --wait-readywaits and eventually returns artifacts.Related Issues
Summary by CodeRabbit
New Features
Bug Fixes
Documentation