feat: split note domain#1418
Closed
Ren1104 wants to merge 2 commits into
Closed
Conversation
github-actions
Bot
added
domain/ccm
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new note shortcut domain with two commands (note +detail, note +transcript), delegates VC note-detail parsing to the shared note package, updates auth/registry/lint/tests, and revises cross-skill routing/docs to use note_display_type and vc-node-id→note_id rules. ChangesNote Domain Feature & Integration
Sequence Diagram(s)sequenceDiagram
participant CLI as note +transcript
participant NoteDetail as FetchDetail
participant TranscriptAPI as unified_note_transcript API
participant FS as File output
CLI->>NoteDetail: verify note_display_type by note_id
NoteDetail-->>CLI: Detail(normal/unified/unknown)
CLI->>TranscriptAPI: GET page with cursor/format/locale
TranscriptAPI-->>CLI: content + has_more + next_cursor
CLI->>CLI: repeat until done with cursor checks
CLI->>FS: write unified transcript file
FS-->>CLI: saved path and size
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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 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 #1418 +/- ##
==========================================
+ Coverage 72.83% 72.84% +0.01%
==========================================
Files 732 737 +5
Lines 69140 69704 +564
==========================================
+ Hits 50356 50777 +421
- Misses 15003 15118 +115
- Partials 3781 3809 +28 ☔ 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@417bf4cd232a93cfc73bdd385cebe4907854fa51🧩 Skill updatenpx skills add larksuite/cli#codex/reapply-note-domain-split -y -g |
![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 (1)
shortcuts/note/note_transcript_test.go (1)
198-234: ⚡ Quick winAdd a positive
--overwritebehavior test.Current coverage validates rejection when the file exists without
--overwrite, but it does not assert that--overwritesuccessfully replaces existing output. Please add one happy-path test to lock that contract.As per coding guidelines, “Every behavior change needs a test alongside the change.”
🤖 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/note/note_transcript_test.go` around lines 198 - 234, Add a new happy-path test (e.g., TestNoteTranscriptAllowsOverwrite) mirroring TestNoteTranscriptRejectsExistingOutputBeforeFetch: create the same existing outPath file, invoke runNoteShortcut with NoteTranscript and include the "--overwrite" flag, assert no error is returned, assert the output file was replaced (read file and check it contains the new transcript content), and ensure no ValidationError is produced; reuse the same helpers (noteShortcutTestFactory, runNoteShortcut, NoteTranscript, stdout) and verify stdout remains as expected.Source: Coding guidelines
🤖 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/note/note_transcript.go`:
- Around line 174-176: Replace the raw returns of ctx.Err() in this file with
typed errs.* errors that preserve the original cause: capture err := ctx.Err()
and if err != nil return a specific typed error (e.g.,
errs.Canceled.WithCause(err) when err == context.Canceled, or
errs.DeadlineExceeded.WithCause(err) when err == context.DeadlineExceeded)
instead of returning ctx.Err() directly; do this for each occurrence that
currently returns ctx.Err() (the ctx.Err() return paths in note_transcript.go)
so command-facing code always receives the typed errs.* envelope with the
underlying cause attached.
---
Nitpick comments:
In `@shortcuts/note/note_transcript_test.go`:
- Around line 198-234: Add a new happy-path test (e.g.,
TestNoteTranscriptAllowsOverwrite) mirroring
TestNoteTranscriptRejectsExistingOutputBeforeFetch: create the same existing
outPath file, invoke runNoteShortcut with NoteTranscript and include the
"--overwrite" flag, assert no error is returned, assert the output file was
replaced (read file and check it contains the new transcript content), and
ensure no ValidationError is produced; reuse the same helpers
(noteShortcutTestFactory, runNoteShortcut, NoteTranscript, stdout) and verify
stdout remains as expected.
🪄 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: f84f5690-ffcb-43ce-af80-be4be802b240
📒 Files selected for processing (26)
cmd/auth/login_messages.gocmd/auth/login_test.gointernal/registry/service_descriptions.jsonlint/errscontract/rule_no_legacy_common_helper_call.golint/errscontract/rule_no_legacy_envelope_literal.golint/errscontract/rules_test.goshortcuts/note/note.goshortcuts/note/note_detail.goshortcuts/note/note_test.goshortcuts/note/note_transcript.goshortcuts/note/note_transcript_test.goshortcuts/note/shortcuts.goshortcuts/register.goshortcuts/vc/vc_notes.goshortcuts/vc/vc_notes_test.goskills/lark-doc/SKILL.mdskills/lark-minutes/SKILL.mdskills/lark-note/SKILL.mdskills/lark-note/references/lark-note-detail.mdskills/lark-note/references/lark-note-transcript.mdskills/lark-vc/SKILL.mdskills/lark-vc/references/lark-vc-notes.mdskills/lark-vc/references/vc-domain-boundaries.mdskills/lark-workflow-meeting-summary/SKILL.mdtests/cli_e2e/note/coverage.mdtests/cli_e2e/note/note_dryrun_test.go
Ren1104
force-pushed
the
codex/reapply-note-domain-split
branch
from
de45647 to
24a9f1e
Compare
There was a problem hiding this comment.
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 `@skills/lark-vc/SKILL.md`:
- Line 50: Update the routing row that currently lists "lark-drive /
lark-doc，必要时再到 lark-note" so it clarifies that lark-note is only a fallback when
an explicit note id/VC node id is available: change the wording to instruct
agents to search lark-doc (and lark-drive) first and only call lark-note after
the docs +fetch step returns a concrete note_id / vc-node-id; reference the
services by name (lark-doc, lark-drive, lark-note) and explicitly require the
note_id/vc-node-id before routing to lark-note so agents won’t attempt an
impossible direct hop.
🪄 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: 4b1b95aa-cac9-4b19-9ac3-ab6f0dbb6f97
📒 Files selected for processing (26)
cmd/auth/login_messages.gocmd/auth/login_test.gointernal/registry/service_descriptions.jsonlint/errscontract/rule_no_legacy_common_helper_call.golint/errscontract/rule_no_legacy_envelope_literal.golint/errscontract/rules_test.goshortcuts/note/note.goshortcuts/note/note_detail.goshortcuts/note/note_test.goshortcuts/note/note_transcript.goshortcuts/note/note_transcript_test.goshortcuts/note/shortcuts.goshortcuts/register.goshortcuts/vc/vc_notes.goshortcuts/vc/vc_notes_test.goskills/lark-doc/SKILL.mdskills/lark-minutes/SKILL.mdskills/lark-note/SKILL.mdskills/lark-note/references/lark-note-detail.mdskills/lark-note/references/lark-note-transcript.mdskills/lark-vc/SKILL.mdskills/lark-vc/references/lark-vc-notes.mdskills/lark-vc/references/vc-domain-boundaries.mdskills/lark-workflow-meeting-summary/SKILL.mdtests/cli_e2e/note/coverage.mdtests/cli_e2e/note/note_dryrun_test.go
✅ Files skipped from review due to trivial changes (4)
- tests/cli_e2e/note/coverage.md
- skills/lark-note/references/lark-note-detail.md
- lint/errscontract/rule_no_legacy_envelope_literal.go
- skills/lark-vc/references/lark-vc-notes.md
🚧 Files skipped from review as they are similar to previous changes (16)
- cmd/auth/login_test.go
- skills/lark-doc/SKILL.md
- shortcuts/note/shortcuts.go
- cmd/auth/login_messages.go
- lint/errscontract/rule_no_legacy_common_helper_call.go
- tests/cli_e2e/note/note_dryrun_test.go
- internal/registry/service_descriptions.json
- shortcuts/register.go
- lint/errscontract/rules_test.go
- shortcuts/note/note_detail.go
- shortcuts/note/note_test.go
- shortcuts/note/note_transcript.go
- shortcuts/vc/vc_notes.go
- shortcuts/note/note.go
- shortcuts/note/note_transcript_test.go
- shortcuts/vc/vc_notes_test.go
Guccitalo
approved these changes
Jun 11, 2026
Collaborator
Author
|
Replaced by #1435 using branch |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reintroduce the Note domain split after #1417 consolidated the logic back into
vc. This restoresnote +detailandnote +transcriptas the canonical known-note_identry points while keepingvc +notesresponsible for locating notes from meeting context.Changes
shortcuts/notewithnote +detailandnote +transcript, including unified transcript pagination, cursor-cycle protection, typed errors, and dry-run E2E coverage.vc +notesto delegate note-detail parsing to the note domain and surfacenote_id/note_display_typefor downstream routing.lark-note,lark-vc,lark-minutes, and thevc-transcribe-tabdoc boundary.Test Plan
go test ./shortcuts/note ./shortcuts/vc ./cmd/auth ./tests/cli_e2e/notelark-cli <domain> <command>flow works as expected:git diff --cached --checkbefore commit and dry-run E2E coverage fornote +detail/note +transcriptRelated Issues
Summary by CodeRabbit
New Features
Behavior
Documentation
Tests