feat: add note domain split#1345
Open
Ren1104 wants to merge 1 commit into
Open
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 a new note shortcut domain with ChangesNote domain implementation and integration
🎯 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 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/ccm
Ren1104
force-pushed
the
feat/note-domain-split
branch
6 times, most recently
from
ce93d8d to
cbbf3ed
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1345 +/- ##
==========================================
+ Coverage 72.75% 72.80% +0.04%
==========================================
Files 730 735 +5
Lines 69030 69345 +315
==========================================
+ Hits 50224 50484 +260
- Misses 15034 15069 +35
- Partials 3772 3792 +20 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Ren1104
force-pushed
the
feat/note-domain-split
branch
5 times, most recently
from
1c356e9 to
d02c54f
Compare
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@e4cb58643d7cda84ac2bacdd24b0b76d337e605e🧩 Skill updatenpx skills add larksuite/cli#feat/note-domain-split -y -g |
Ren1104
force-pushed
the
feat/note-domain-split
branch
from
d02c54f to
5861ec5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/lark-vc/SKILL.md (1)
148-170:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSpecify language for fenced code block.
The fenced code block at line 148 should specify a language identifier for proper syntax highlighting.
📝 Proposed fix
-``` +```text Meeting (视频会议)🤖 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-vc/SKILL.md` around lines 148 - 170, The fenced code block that starts with ``` and contains the meeting tree (starting "Meeting (视频会议)") lacks a language identifier; update the opening fence to include a language token (e.g., ```text) so the block becomes ```text and closes with ``` to enable proper syntax highlighting and rendering for that tree diagram.Source: Linters/SAST tools
🧹 Nitpick comments (2)
lint/errscontract/rule_no_legacy_envelope_literal.go (1)
34-34: 💤 Low valueConsider reordering
shortcuts/im/for consistency.The
shortcuts/im/entry is currently at the end of the list but should be in alphabetical order (betweenshortcuts/event/andshortcuts/mail/, around line 27) to match the ordering inmigratedCommonHelperPathsinrule_no_legacy_common_helper_call.go.📝 Suggested reordering
Move line 34 to maintain alphabetical order:
"shortcuts/event/", + "shortcuts/im/", "shortcuts/mail/", "shortcuts/minutes/", "shortcuts/note/", "shortcuts/okr/", "shortcuts/task/", "shortcuts/vc/", "shortcuts/whiteboard/", - "shortcuts/im/",🤖 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 `@lint/errscontract/rule_no_legacy_envelope_literal.go` at line 34, Move the "shortcuts/im/" list entry in rule_no_legacy_envelope_literal.go so the array remains alphabetically ordered: place the "shortcuts/im/" string between "shortcuts/event/" and "shortcuts/mail/" to match the ordering used by migratedCommonHelperPaths in rule_no_legacy_common_helper_call.go.skills/lark-vc/references/vc-domain-boundaries.md (1)
150-150: 💤 Low valueOptional: Add language specifier to fenced code block.
The fenced code block on line 150 lacks a language specifier, triggering a markdownlint warning. While the ASCII tree diagram renders correctly, consider adding
```textinstead of```to silence the linter and follow markdown best practices.📝 Proposed change
-``` +```text Meeting ├── meeting_id🤖 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-vc/references/vc-domain-boundaries.md` at line 150, The fenced code block containing the ASCII tree (starting with "Meeting" and the subsequent tree lines) is missing a language specifier; update the opening backticks to include a language label (e.g., replace ``` with ```text) so the block becomes ```text and the linter warning is resolved while preserving the ASCII diagram formatting.Source: Linters/SAST tools
🤖 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.
Outside diff comments:
In `@skills/lark-vc/SKILL.md`:
- Around line 148-170: The fenced code block that starts with ``` and contains
the meeting tree (starting "Meeting (视频会议)") lacks a language identifier; update
the opening fence to include a language token (e.g., ```text) so the block
becomes ```text and closes with ``` to enable proper syntax highlighting and
rendering for that tree diagram.
---
Nitpick comments:
In `@lint/errscontract/rule_no_legacy_envelope_literal.go`:
- Line 34: Move the "shortcuts/im/" list entry in
rule_no_legacy_envelope_literal.go so the array remains alphabetically ordered:
place the "shortcuts/im/" string between "shortcuts/event/" and
"shortcuts/mail/" to match the ordering used by migratedCommonHelperPaths in
rule_no_legacy_common_helper_call.go.
In `@skills/lark-vc/references/vc-domain-boundaries.md`:
- Line 150: The fenced code block containing the ASCII tree (starting with
"Meeting" and the subsequent tree lines) is missing a language specifier; update
the opening backticks to include a language label (e.g., replace ``` with
```text) so the block becomes ```text and the linter warning is resolved while
preserving the ASCII diagram formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6a92ba67-addb-484a-8d26-958b92024349
📒 Files selected for processing (28)
.gitignorecmd/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-doc/references/lark-doc-fetch.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
Collaborator
Author
|
Addressed CodeRabbit feedback in be00e90: reordered |
Ren1104
force-pushed
the
feat/note-domain-split
branch
from
be00e90 to
5e81d9e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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_detail.go`:
- Around line 76-81: The branch that converts a typed 121005 into an
*errs.PermissionError (where mappedProblem := *problem, mappedProblem.Message =
message, return &errs.PermissionError{Problem: mappedProblem, Cause: err}) must
also normalize the embedded Problem metadata so callers using errs.ProblemOf see
authorization/permission-denied, not the original stale category/subtype; update
the mappedProblem fields (e.g., mappedProblem.Category and
mappedProblem.Subtype) to the canonical values for permission errors before
constructing the errs.PermissionError (keep mappedProblem.Message and Hint logic
intact).
In `@shortcuts/note/note_transcript.go`:
- Around line 108-110: The current file-exists branch embeds recovery steps in
the error Message and always attaches .WithParam("--output") even when the CLI
used the default outPath; instead create the
errs.NewValidationError(errs.SubtypeFailedPrecondition, "...file already exists:
%s", outPath) and move the recovery guidance ("use --overwrite to replace" or
"pick a different path") into .WithHint(...); only call .WithParam("--output")
when the user actually supplied the output flag (detect via whatever
runtime/flag check your CLI provides), and apply the same change to the similar
block around the other occurrence (the block at ~147-152).
In `@shortcuts/note/note.go`:
- Around line 148-155: The parseLooseInt function currently coerces float-like
inputs to int (e.g., float64 and json.Number) which silently accepts fractional
inputs like 1.9; update parseLooseInt to detect fractional values and reject
them instead of truncating: for json.Number try parsing as int first and if that
fails parse as float64 and check whether it has a fractional part (use
math.Trunc/Modf or compare float64(int(val)) == val); for float64 check the
fractional part similarly; if a fractional part exists return a clear invalid
sentinel (e.g., -1) so callers (note_display_type / artifact_type) can treat it
as unknown/ignored rather than a coerced enum value. Ensure changes are made in
the parseLooseInt function only.
🪄 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: b23d01f0-30db-4043-b130-67758d6f6b2a
📒 Files selected for processing (28)
.gitignorecmd/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-doc/references/lark-doc-fetch.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 (8)
- tests/cli_e2e/note/coverage.md
- cmd/auth/login_messages.go
- .gitignore
- skills/lark-note/references/lark-note-transcript.md
- skills/lark-vc/references/lark-vc-notes.md
- skills/lark-minutes/SKILL.md
- skills/lark-note/SKILL.md
- skills/lark-workflow-meeting-summary/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (16)
- cmd/auth/login_test.go
- skills/lark-note/references/lark-note-detail.md
- shortcuts/register.go
- lint/errscontract/rule_no_legacy_envelope_literal.go
- internal/registry/service_descriptions.json
- shortcuts/note/shortcuts.go
- skills/lark-doc/references/lark-doc-fetch.md
- lint/errscontract/rule_no_legacy_common_helper_call.go
- lint/errscontract/rules_test.go
- skills/lark-doc/SKILL.md
- shortcuts/note/note_transcript_test.go
- shortcuts/vc/vc_notes.go
- skills/lark-vc/SKILL.md
- skills/lark-vc/references/vc-domain-boundaries.md
- tests/cli_e2e/note/note_dryrun_test.go
- shortcuts/vc/vc_notes_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
shortcuts/note/note_transcript_test.go (2)
251-252: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd typed error metadata assertions.
The test checks only message substrings but does not assert
CategoryorSubtype(expected:errs.CategoryInternal,errs.SubtypeInvalidResponsepernote_transcript.goL195). As per coding guidelines, error-path tests must assert typed metadata viaerrs.ProblemOf, not message substrings alone.🧪 Suggested addition
if got := err.Error(); !strings.Contains(got, "transcript is empty") || !strings.Contains(got, "note_empty") { t.Fatalf("err = %q, want empty transcript error", got) } + problem, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("expected typed error, got %T", err) + } + if problem.Category != errs.CategoryInternal || problem.Subtype != errs.SubtypeInvalidResponse { + t.Fatalf("category/subtype = %v/%v, want Internal/InvalidResponse", problem.Category, problem.Subtype) + }🤖 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 251 - 252, Test currently only checks error message substrings; update it to assert the typed error metadata using errs.ProblemOf. After the existing error is obtained (err), call p := errs.ProblemOf(err) and assert p.Category == errs.CategoryInternal and p.Subtype == errs.SubtypeInvalidResponse (these are set in note_transcript.go around L195); you can keep the existing message substring checks but add these two explicit metadata assertions to satisfy error-path testing guidelines.Source: Coding guidelines
315-316: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd typed error metadata assertions.
The test checks only message substrings but does not assert
CategoryorSubtype(expected:errs.CategoryInternal,errs.SubtypeInvalidResponsepernote_transcript.goL191). As per coding guidelines, error-path tests must assert typed metadata viaerrs.ProblemOf, not message substrings alone.🧪 Suggested addition
if got := err.Error(); !strings.Contains(got, "pagination cursor did not advance") { t.Fatalf("err = %q, want cursor advance error", got) } + problem, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("expected typed error, got %T", err) + } + if problem.Category != errs.CategoryInternal || problem.Subtype != errs.SubtypeInvalidResponse { + t.Fatalf("category/subtype = %v/%v, want Internal/InvalidResponse", problem.Category, problem.Subtype) + }🤖 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 315 - 316, The test currently only checks the error message substring; update the assertion to extract typed metadata using errs.ProblemOf(err) and assert the returned Problem has Category == errs.CategoryInternal and Subtype == errs.SubtypeInvalidResponse (as set in note_transcript.go at L191), while you may retain the existing message substring check for completeness; reference the test location that currently calls err.Error() and replace/add assertions using errs.ProblemOf, comparing Problem.Category and Problem.Subtype to the expected constants.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_test.go`:
- Around line 211-221: Add an assertion that the typed validation error's
Subtype equals errs.SubtypeFailedPrecondition: after you already cast err to
*errs.ValidationError in note_transcript_test.go, check validationErr.Subtype ==
errs.SubtypeFailedPrecondition (mirrors the value set in note_transcript.go) so
the test validates category/subtype in addition to Hint and Param.
- Around line 35-37: The test currently checks problem.Hint but omits asserting
the error subtype; update the assertion after calling errs.ProblemOf(err) in
note_transcript_test.go to also check that problem.Subtype equals
errs.SubtypeFailedPrecondition (as set by ensureUnifiedNote in
note_transcript.go) and, per guidelines, include any other required
category/param assertions from ProblemOf to fully validate the typed error
metadata.
---
Outside diff comments:
In `@shortcuts/note/note_transcript_test.go`:
- Around line 251-252: Test currently only checks error message substrings;
update it to assert the typed error metadata using errs.ProblemOf. After the
existing error is obtained (err), call p := errs.ProblemOf(err) and assert
p.Category == errs.CategoryInternal and p.Subtype == errs.SubtypeInvalidResponse
(these are set in note_transcript.go around L195); you can keep the existing
message substring checks but add these two explicit metadata assertions to
satisfy error-path testing guidelines.
- Around line 315-316: The test currently only checks the error message
substring; update the assertion to extract typed metadata using
errs.ProblemOf(err) and assert the returned Problem has Category ==
errs.CategoryInternal and Subtype == errs.SubtypeInvalidResponse (as set in
note_transcript.go at L191), while you may retain the existing message substring
check for completeness; reference the test location that currently calls
err.Error() and replace/add assertions using errs.ProblemOf, comparing
Problem.Category and Problem.Subtype to the expected constants.
🪄 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: 06916fbb-8836-4234-9777-1e545bca49c3
📒 Files selected for processing (5)
shortcuts/note/note.goshortcuts/note/note_detail.goshortcuts/note/note_test.goshortcuts/note/note_transcript.goshortcuts/note/note_transcript_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- shortcuts/note/note_detail.go
- shortcuts/note/note_test.go
- shortcuts/note/note_transcript.go
- shortcuts/note/note.go
Ren1104
force-pushed
the
feat/note-domain-split
branch
from
4f729c6 to
c2b7329
Compare
Add note shortcuts for note detail and unified transcript retrieval, route vc note detail parsing through the note domain, and update note/vc/minutes skill guidance for normal versus unified transcript handling. Includes dry-run E2E coverage for the new note shortcuts and documents the remaining live E2E fixture gap.
Ren1104
force-pushed
the
feat/note-domain-split
branch
from
f46548d to
e4cb586
Compare
zhaoleibd
approved these changes
Jun 11, 2026
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
Add a dedicated
noteshortcut domain for VC note artifacts and make the normal/unified note split explicit for agents.vc +notesnow exposes the routing fields needed to decide whether transcript retrieval should usenote +transcriptfor unified notes ordocs +fetchfor normal note transcript documents.Changes
notedomain, register it for auth/login discovery, and document it in service descriptions.note +detailto fetch VC note metadata bynote_id, includingnote_display_type,note_doc_token,verbatim_doc_token, and shared document tokens.note +transcriptfor unified-note transcript export with--transcript-format, locale handling, output overwrite protection, typed errors, and defensive cursor pagination.vc +notesto delegate note detail lookup to the note domain, preservenote_id-only successful details, and surfacenote_id/note_display_typein table and JSON output.vc-node-idwhen present, then route note handling through the note metadata.Test Plan
go test ./shortcuts/note ./shortcuts/vc ./internal/core ./internal/cmdutilgo test ./shortcuts/vc ./shortcuts/note ./tests/cli_e2e/notegofmt -l shortcuts/vc/vc_notes.go shortcuts/vc/vc_notes_test.gogit diff --checknote +detailandnote +transcriptunified-note flowsRelated Issues