Skip to content

bugfix 0702 about speaker replace#1731

Merged
zhangjun-bytedance merged 1 commit into
mainfrom
feat/0702_bugfix
Jul 3, 2026
Merged

bugfix 0702 about speaker replace#1731
zhangjun-bytedance merged 1 commit into
mainfrom
feat/0702_bugfix

Conversation

@zhangjun-bytedance

@zhangjun-bytedance zhangjun-bytedance commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

Summary

Changes

  • Change 1
  • Change 2

Test Plan

  • Unit tests pass
  • Manual local verification confirms the lark-cli <domain> <command> flow works as expected

Related Issues

  • None

Summary by CodeRabbit

  • Bug Fixes

    • Improved speaker replacement so it uses the speaker ID you provide directly, avoiding extra lookup steps.
    • Updated error handling and output so the reported source speaker matches the value you entered.
    • Removed an outdated output field from speaker replacement results.
  • Documentation

    • Clarified that speaker replacement expects a valid speaker ID rather than a display name.

@github-actions github-actions Bot added the size/M Single-domain feat or fix with limited business impact label Jul 3, 2026
@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The minutes +speaker-replace shortcut is refactored to remove automatic speakerlist resolution. DryRun and Execute now build PUT requests directly from raw --from-speaker-id/--from-user-id flag values instead of resolving names via resolveSpeakerReplaceFrom. The minutes_speakers.go file and its resolution helper/tests are deleted, output no longer includes from_speaker_input, and unit tests, e2e tests, and documentation are updated accordingly.

Changes

Speaker-replace behavior change

Layer / File(s) Summary
Core DryRun/Execute and output builder changes
shortcuts/minutes/minutes_speaker_replace.go
DryRun/Execute build PUT requests directly from raw flag values without speakerlist resolution; buildSpeakerReplaceOutputData and speakerReplaceSourceLabel are simplified, dropping from_speaker_input.
Removal of speaker resolution helper
shortcuts/minutes/minutes_speakers.go, shortcuts/minutes/minutes_speakers_test.go
The speakerlist API fetch/parse and resolveSpeakerIDByName logic, along with its tests, are deleted entirely.
Unit tests validating no speakerlist prefetch
shortcuts/minutes/minutes_speaker_replace_test.go
Dry-run name-resolution test is replaced with an execute test using an opaque speaker ID, asserting only a PUT is issued and data.from_speaker_id (not from_speaker_input) is returned.
E2E test and documentation updates
tests/cli_e2e/minutes/minutes_speaker_replace_test.go, skills/lark-minutes/references/lark-minutes-speaker-replace.md
E2E dry-run test asserts no speakerlist endpoint is requested; documentation clarifies that --from-speaker-id is passed through as-is and must be a resolved speaker_id, not a display name.

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

Possibly related PRs

  • larksuite/cli#1036: Modifies the same minutes +speaker-replace implementation and tests introduced in that PR, removing the from_speaker_input/resolution behavior it added.
  • larksuite/cli#1594: Both PRs modify shortcuts/minutes/minutes_speaker_replace.go's DryRun/Execute handling of --from-speaker-id/--from-user-id and the PUT request/output fields, in opposite directions.

Suggested labels: domain/vc

Suggested reviewers: calendar-assistant

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description is only the template with placeholders and no actual summary, changes, or test details. Fill in the Summary, Changes, and Test Plan sections with concrete details and mark the related issues section accurately.
Title check ❓ Inconclusive The title is related to the change but too generic to identify the specific speaker-replace behavior. Rename it to a concrete summary like "Fix speaker-replace to use provided speaker IDs without speakerlist lookup".
✅ Passed checks (3 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
✨ 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 feat/0702_bugfix

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.

@codecov

codecov Bot commented Jul 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.40%. Comparing base (c2d6038) to head (ca1747d).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1731      +/-   ##
==========================================
- Coverage   74.40%   74.40%   -0.01%     
==========================================
  Files         854      853       -1     
  Lines       88375    88311      -64     
==========================================
- Hits        65752    65704      -48     
+ Misses      17552    17544       -8     
+ Partials     5071     5063       -8     

☔ 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 3, 2026

Copy link
Copy Markdown

🚀 PR Preview Install Guide

🧰 CLI update

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

🧩 Skill update

npx skills add larksuite/cli#feat/0702_bugfix -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: 1

🤖 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/minutes/minutes_speaker_replace.go`:
- Around line 67-72: The DryRun preview for the speaker replace flow is missing
the same query parameter that Execute sends, so the preview does not match the
real request. Update the DryRun builder in minutes_speaker_replace.go, alongside
the PUT call in the speaker replace request flow, to include the
user_id_type=open_id query parameter by adding the equivalent Params call so the
dry-run output mirrors Execute.
🪄 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: 72addfd1-adcf-48ce-b8c2-877bc291e969

📥 Commits

Reviewing files that changed from the base of the PR and between c2d6038 and ca1747d.

📒 Files selected for processing (6)
  • shortcuts/minutes/minutes_speaker_replace.go
  • shortcuts/minutes/minutes_speaker_replace_test.go
  • shortcuts/minutes/minutes_speakers.go
  • shortcuts/minutes/minutes_speakers_test.go
  • skills/lark-minutes/references/lark-minutes-speaker-replace.md
  • tests/cli_e2e/minutes/minutes_speaker_replace_test.go
💤 Files with no reviewable changes (2)
  • shortcuts/minutes/minutes_speakers_test.go
  • shortcuts/minutes/minutes_speakers.go

Comment thread shortcuts/minutes/minutes_speaker_replace.go
@zhangjun-bytedance
zhangjun-bytedance merged commit 73be1d0 into main Jul 3, 2026
61 of 65 checks passed
@zhangjun-bytedance
zhangjun-bytedance deleted the feat/0702_bugfix branch July 3, 2026 06:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M Single-domain feat or fix with limited business impact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants