Skip to content

fix(vc): don't fail +detail for in-progress meetings#1930

Merged
liangshuo-1 merged 1 commit into
mainfrom
fix/vc-detail-in-progress-meeting
Jul 17, 2026
Merged

fix(vc): don't fail +detail for in-progress meetings#1930
liangshuo-1 merged 1 commit into
mainfrom
fix/vc-detail-in-progress-meeting

Conversation

@calendar-assistant

@calendar-assistant calendar-assistant commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Summary

vc +detail failed with exit 1 / ok:false for meetings that are still in progress, because the supplementary recording lookup returned an unclassified error (no minute/note generated yet) that was surfaced as a hard error — even though meeting.get had already succeeded.

Changes

  • Detect the in-progress state up front in fetchMeetingDetail (same start/end heuristic as +meeting-events, reading raw timestamps via meetingTimeField/meetingInProgress) and skip the recording call, returning meeting metadata with an informational hint instead of an error.
  • Degrade recording lookup failures for ended meetings to a hint as well, so a best-effort supplement never fails the whole command.
  • Update the contract test for the recording-error path and add TestFetchMeetingDetail_MeetingInProgress (registers no recording stub, so a regression that re-adds the recording call would fail).
  • vc-agent skill: note that sending an in-meeting message only needs meeting_id and must not pre-fetch +detail / +recording / +notes.

Test Plan

  • Unit tests pass (go test ./shortcuts/vc/)
  • go vet ./shortcuts/vc/ and gofmt -l clean
  • Manual local verification confirms the lark-cli vc +detail flow works as expected

Related Issues

  • None

Summary by CodeRabbit

  • Bug Fixes

    • Meeting details now load successfully when recording information is unavailable, while displaying a helpful warning.
    • In-progress meetings no longer trigger unnecessary recording lookups and clearly indicate their status.
    • Meeting metadata remains available even when minute or recording retrieval fails.
  • Documentation

    • Clarified how to send messages to meetings identified by a 9-digit meeting number.

@github-actions github-actions Bot added domain/vc PR touches the vc domain size/L Large or sensitive change across domains or core paths labels Jul 17, 2026
@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

fetchMeetingDetail now detects in-progress meetings before recording lookup, downgrades recording failures to hints, and adds tests for both behaviors. Meeting message-send guidance now requires only meeting_id after resolving nine-digit meeting numbers.

Changes

Meeting detail handling

Layer / File(s) Summary
Conditional minute-token lookup and validation
shortcuts/vc/vc_detail.go, shortcuts/vc/vc_detail_test.go
Meeting timestamps are normalized and evaluated to skip recording lookup for in-progress meetings; recording failures preserve meeting metadata and populate hints instead of errors. Tests cover both outcomes.

Message-send guidance

Layer / File(s) Summary
Message-send workflow documentation
skills/lark-vc-agent/SKILL.md
The guidance resolves nine-digit meeting numbers to meeting_id and sends messages without a prior detail lookup.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant fetchMeetingDetail
  participant MeetingAPI
  participant RecordingAPI
  fetchMeetingDetail->>MeetingAPI: fetch meeting metadata
  fetchMeetingDetail->>fetchMeetingDetail: determine meetingInProgress
  alt meeting is in progress
    fetchMeetingDetail-->>fetchMeetingDetail: set informational hint
  else meeting is not in progress
    fetchMeetingDetail->>RecordingAPI: query minute_token
    RecordingAPI-->>fetchMeetingDetail: return token or lookup error
    fetchMeetingDetail-->>fetchMeetingDetail: preserve metadata and set hint on error
  end
Loading

Suggested reviewers: liangshuo-1

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly matches the main change: making +detail tolerate in-progress meetings.
Description check ✅ Passed The description follows the template with Summary, Changes, Test Plan, and Related Issues filled in.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/vc-detail-in-progress-meeting

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@calendar-assistant
calendar-assistant force-pushed the fix/vc-detail-in-progress-meeting branch from 2753888 to 64dc78f Compare July 17, 2026 06:59
An ongoing meeting has no minute/note yet, so the recording lookup in
+detail returned an unclassified error that was surfaced as a hard error,
making the whole command exit 1 / ok:false even though meeting.get had
already succeeded.

Detect the in-progress state up front (same start/end heuristic as
+meeting-events, reading raw timestamps) and skip the recording call,
returning the meeting metadata with an informational hint instead of an
error. Recording failures for ended meetings are likewise degraded to a
hint rather than failing the command.

Also note in the vc-agent skill that sending an in-meeting message only
needs meeting_id and must not pre-fetch +detail / +recording / +notes.
@calendar-assistant
calendar-assistant force-pushed the fix/vc-detail-in-progress-meeting branch from 64dc78f to 7d7607a Compare July 17, 2026 07:01
@github-actions

github-actions Bot commented Jul 17, 2026

Copy link
Copy Markdown

🚀 PR Preview Install Guide

🧰 CLI update

npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@7d7607a9f0b5482e33c5d988463ac76455d289ba

🧩 Skill update

npx skills add larksuite/cli#fix/vc-detail-in-progress-meeting -y -g

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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/vc/vc_detail_test.go`:
- Around line 289-306: Update TestFetchMeetingDetail_MeetingInProgress and its
fixture to use numeric start_time and end_time values, preserving equal
timestamps to represent an ongoing meeting. Add an adjacent assertion or case
that verifies no recording request is made for this numeric-timestamp scenario.

In `@shortcuts/vc/vc_detail.go`:
- Around line 130-159: Update meetingTimeField and the VC detail timestamp
handling in shortcuts/vc/vc_detail.go:130-159 and the related payload flow at
shortcuts/vc/vc_detail.go:72-77 to preserve numeric JSON timestamps as parseable
values, explicitly supporting the numeric types returned by the API instead of
formatting them through %v. Ensure common.FormatTime and parseFlexibleTime
receive correct timestamp representations so meetingInProgress prevents
recording ongoing meetings. Add a numeric-timestamp fixture in
shortcuts/vc/vc_detail_test.go:289-306 and retain the no-recording-stub
assertion.
🪄 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: 10049d68-a5a2-4243-8b9d-390366ebe7a5

📥 Commits

Reviewing files that changed from the base of the PR and between 4e0a6a9 and 7d7607a.

📒 Files selected for processing (3)
  • shortcuts/vc/vc_detail.go
  • shortcuts/vc/vc_detail_test.go
  • skills/lark-vc-agent/SKILL.md

Comment thread shortcuts/vc/vc_detail_test.go
Comment thread shortcuts/vc/vc_detail.go
@codecov

codecov Bot commented Jul 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.50000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.96%. Comparing base (4e0a6a9) to head (7d7607a).

Files with missing lines Patch % Lines
shortcuts/vc/vc_detail.go 87.50% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1930   +/-   ##
=======================================
  Coverage   74.96%   74.96%           
=======================================
  Files         892      892           
  Lines       94058    94077   +19     
=======================================
+ Hits        70506    70529   +23     
+ Misses      18139    18137    -2     
+ Partials     5413     5411    -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

github-actions Bot commented Jul 17, 2026

Copy link
Copy Markdown

PR Quality Summary

CI did not complete successfully. Use the failed check links below to decide whether this PR needs a code change or a rerun.

Failed checks

@liangshuo-1
liangshuo-1 merged commit 27df16d into main Jul 17, 2026
53 of 59 checks passed
@liangshuo-1
liangshuo-1 deleted the fix/vc-detail-in-progress-meeting branch July 17, 2026 07:54
@liangshuo-1 liangshuo-1 mentioned this pull request Jul 17, 2026
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain/vc PR touches the vc domain size/L Large or sensitive change across domains or core paths

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants