feat(im): add feed shortcut create, list, and remove shortcuts#1273
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 IM "feed shortcuts" (create/remove/list) with chat-id validation, list enrichment via batched chat lookups, per-item failure labeling and ledgering, CLI commands and dry-run previews, extensive unit/e2e tests, and documentation updates. ChangesFeed Shortcuts Feature
Sequence Diagram — List enrichment flow: sequenceDiagram
participant CLI as lark-cli
participant FeedAPI as /open-apis/im/v2/feed_shortcuts
participant Enricher as enrichFeedShortcutDetail
participant ChatBatch as /open-apis/im/v1/chats/batch_query
CLI->>FeedAPI: GET feed_shortcuts (optional page_token)
FeedAPI-->>CLI: shortcuts page
CLI->>Enricher: enrichFeedShortcutDetail(shortcuts)
Enricher->>ChatBatch: POST chats/batch_query (CHAT ids chunked)
ChatBatch-->>Enricher: chat objects keyed by chat_id
Enricher-->>CLI: shortcuts with attached "detail" for CHAT items
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@45f69d2d81e921d622b3b55cce0782f1eadcfc52🧩 Skill updatenpx skills add larksuite/cli#feat/im-feed-shortcuts -y -g |
0ac0e3c to
b1d8fa3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/cli_e2e/im/feed_shortcut_workflow_test.go (1)
277-305: ⚡ Quick winCleanup helper should not rely on a single list page before deciding removals.
Line 278 fetches only one page, then Line 299 conditionally removes IDs only if seen there. For accounts with many shortcuts (or entries pinned with
--tail), created IDs can be off-page and skipped, leaving residual state after failures.Refactor direction
- listResult, listErr := clie2e.RunCmd(cleanupCtx, clie2e.Request{ - Args: []string{"im", "+feed-shortcut-list", "--no-detail"}, - DefaultAs: defaultAs, - }) + // Iterate pages until all target IDs are found or has_more=false. + // Then remove any found IDs.As per coding guidelines
tests/cli_e2e/**/*_test.go: live E2E tests should be self-contained (create->use->cleanup).🤖 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 `@tests/cli_e2e/im/feed_shortcut_workflow_test.go` around lines 277 - 305, The cleanup currently fetches only one page via clie2e.RunCmd with Args ["im", "+feed-shortcut-list", "--no-detail"] and so may miss created IDs; change it to paginate until the list is exhausted: repeatedly call clie2e.RunCmd(Request{Args: []string{"im", "+feed-shortcut-list", "--no-detail", /* + page token if supported */}, DefaultAs: defaultAs}) and after each call merge all feed_card_id values from gjson.Get(listResult.Stdout, "data.shortcuts").Array() into the present map, using any next-page cursor/token field returned (e.g. meta.next_cursor or next_page_token) to drive the loop (or use a provided --all flag if CLI supports it); once all pages are collected, build args (the ["im", "+feed-shortcut-remove"] block that appends "--chat-id", id for ids present) and proceed with removal as before.
🤖 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-im/references/lark-im-feed-shortcut-remove.md`:
- Line 14: The doc has contradictory semantics for removing a non-existent
shortcut: one paragraph says the server treats “not in shortcut list” as success
(returning ok:true and full-success ledger with failed_shortcuts only for true
server-side failures) while another labels the same case as an invalid_item
failure; pick one behavior and make the contract consistent: either (A) always
treat removing a non-existent shortcut as idempotent success — remove the
mention of invalid_item and ensure examples and references to failed_shortcuts,
ok, and exit code reflect ok:true and zero exit on idempotent removals, or (B)
treat it as a client error — remove the idempotent-success wording and update
the earlier paragraph to state the server returns invalid_item, ok:false,
non-zero exit with failed_shortcuts populated; update all occurrences of
failed_shortcuts, invalid_item, ok and exit code text to match the chosen
semantics.
In `@tests/cli_e2e/im/feed_shortcut_workflow_test.go`:
- Around line 371-375: Update the validate-stage dry-run assertions to expect
the specific exit code 2 and to check combined output; replace the generic
require.NotEqual(t, 0, result.ExitCode, ...) with require.Equal(t, 2,
result.ExitCode, ...) and change require.Contains(t, result.Stderr, "mutually
exclusive") to require.Contains(t, result.Stdout+result.Stderr, "mutually
exclusive") in the test block using the Result variable (named result), and make
the same two replacements for the other assertion block around Lines 387-389 so
both validate-stage dry-run checks assert exit code 2 and inspect
result.Stdout+result.Stderr.
---
Nitpick comments:
In `@tests/cli_e2e/im/feed_shortcut_workflow_test.go`:
- Around line 277-305: The cleanup currently fetches only one page via
clie2e.RunCmd with Args ["im", "+feed-shortcut-list", "--no-detail"] and so may
miss created IDs; change it to paginate until the list is exhausted: repeatedly
call clie2e.RunCmd(Request{Args: []string{"im", "+feed-shortcut-list",
"--no-detail", /* + page token if supported */}, DefaultAs: defaultAs}) and
after each call merge all feed_card_id values from gjson.Get(listResult.Stdout,
"data.shortcuts").Array() into the present map, using any next-page cursor/token
field returned (e.g. meta.next_cursor or next_page_token) to drive the loop (or
use a provided --all flag if CLI supports it); once all pages are collected,
build args (the ["im", "+feed-shortcut-remove"] block that appends "--chat-id",
id for ids present) and proceed with removal as before.
🪄 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: b51b3b73-66b5-4496-bc40-bac958b61082
📒 Files selected for processing (13)
shortcuts/im/helpers.goshortcuts/im/helpers_test.goshortcuts/im/im_feed_shortcut_create.goshortcuts/im/im_feed_shortcut_list.goshortcuts/im/im_feed_shortcut_remove.goshortcuts/im/im_feed_shortcut_test.goshortcuts/im/shortcuts.goskill-template/domains/im.mdskills/lark-im/SKILL.mdskills/lark-im/references/lark-im-feed-shortcut-create.mdskills/lark-im/references/lark-im-feed-shortcut-list.mdskills/lark-im/references/lark-im-feed-shortcut-remove.mdtests/cli_e2e/im/feed_shortcut_workflow_test.go
b1d8fa3 to
0ee5b83
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1273 +/- ##
==========================================
+ Coverage 70.18% 70.22% +0.04%
==========================================
Files 671 674 +3
Lines 65240 65471 +231
==========================================
+ Hits 45786 45975 +189
- Misses 15783 15812 +29
- Partials 3671 3684 +13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
d19888a to
670a38c
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
shortcuts/im/im_messages_search.go (1)
461-463: ⚡ Quick winAvoid silently swallowing chat-context enrichment failures.
At Line 463, dropping
queryChatBatcherrors entirely hides partial/full enrichment failures. Keep best-effort behavior, but emit a compact stderr warning when any chunk fails so users can distinguish “no chat context” vs “enrichment failed.”Proposed patch
func batchQueryChatContexts(runtime *common.RuntimeContext, chatIds []string) map[string]map[string]interface{} { chatContexts := map[string]map[string]interface{}{} // Best-effort: a failed chunk only loses its own entries. + failedChunks := 0 for _, batch := range chunkStrings(chatIds, chatBatchQuerySize) { - _ = queryChatBatch(runtime, batch, chatContexts) + if err := queryChatBatch(runtime, batch, chatContexts); err != nil { + failedChunks++ + } + } + if failedChunks > 0 { + fmt.Fprintf(runtime.Factory.IOStreams.ErrOut, "warning: chat context enrichment skipped for %d batch(es)\n", failedChunks) } return chatContexts }As per coding guidelines: "
**/*.go: stdout is data (JSON envelopes), stderr is everything else."🤖 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/im/im_messages_search.go` around lines 461 - 463, The loop currently swallows errors from queryChatBatch, losing visibility into enrichment failures; update the loop that iterates over chunkStrings(chatIds, chatBatchQuerySize) to capture the returned error from queryChatBatch(runtime, batch, chatContexts), and when non-nil emit a compact stderr warning (e.g., fmt.Fprintf(os.Stderr, "...: %v\n", err) or use the existing stderr logger) that identifies the batch and that enrichment failed, then continue (preserve best-effort behavior) so other chunks still run; reference chunkStrings, queryChatBatch, chatContexts, and chatBatchQuerySize to find the location.
🤖 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/im/helpers.go`:
- Around line 1660-1692: In addFeedShortcutWriteLedger: the ledger currently
sets data["failure_count"] = len(failed) which can be inflated by
duplicate/malformed echoes; instead derive failures from the requested items
matched against failedIDs. After building failedIDs and succeeded (or
alternatively compute matchedFailedCount), set data["failure_count"] to the
number of requested items whose FeedCardID appears in failedIDs (or simply
data["failure_count"] = len(requested) - len(succeeded)) so success_count +
failure_count cannot exceed total; use the existing symbols failedIDs,
requested, succeeded, and item.FeedCardID to implement this.
In `@shortcuts/im/im_feed_shortcut_test.go`:
- Around line 53-56: The test helper currently registers the flag with
cmd.Flags().Bool("no-detail", true, "") which flips the default to opt-out;
change that registration to use false so the helper mirrors the real CLI default
(cmd.Flags().Bool("no-detail", false, "")). Then find tests that rely on the
single-call/skip-enrichment path and explicitly set the flag to true in those
tests (e.g., by calling cmd.Flags().Set("no-detail","true") or constructing the
command with the flag) so only those tests exercise the opt-out behavior.
---
Nitpick comments:
In `@shortcuts/im/im_messages_search.go`:
- Around line 461-463: The loop currently swallows errors from queryChatBatch,
losing visibility into enrichment failures; update the loop that iterates over
chunkStrings(chatIds, chatBatchQuerySize) to capture the returned error from
queryChatBatch(runtime, batch, chatContexts), and when non-nil emit a compact
stderr warning (e.g., fmt.Fprintf(os.Stderr, "...: %v\n", err) or use the
existing stderr logger) that identifies the batch and that enrichment failed,
then continue (preserve best-effort behavior) so other chunks still run;
reference chunkStrings, queryChatBatch, chatContexts, and chatBatchQuerySize to
find the location.
🪄 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: 52d546ce-1788-40db-9b09-de45c978d0b1
📒 Files selected for processing (14)
shortcuts/im/helpers.goshortcuts/im/helpers_test.goshortcuts/im/im_feed_shortcut_create.goshortcuts/im/im_feed_shortcut_list.goshortcuts/im/im_feed_shortcut_remove.goshortcuts/im/im_feed_shortcut_test.goshortcuts/im/im_messages_search.goshortcuts/im/shortcuts.goskill-template/domains/im.mdskills/lark-im/SKILL.mdskills/lark-im/references/lark-im-feed-shortcut-create.mdskills/lark-im/references/lark-im-feed-shortcut-list.mdskills/lark-im/references/lark-im-feed-shortcut-remove.mdtests/cli_e2e/im/feed_shortcut_workflow_test.go
✅ Files skipped from review due to trivial changes (4)
- skills/lark-im/references/lark-im-feed-shortcut-create.md
- skills/lark-im/references/lark-im-feed-shortcut-remove.md
- skill-template/domains/im.md
- skills/lark-im/references/lark-im-feed-shortcut-list.md
🚧 Files skipped from review as they are similar to previous changes (6)
- shortcuts/im/shortcuts.go
- shortcuts/im/im_feed_shortcut_create.go
- skills/lark-im/SKILL.md
- shortcuts/im/im_feed_shortcut_remove.go
- tests/cli_e2e/im/feed_shortcut_workflow_test.go
- shortcuts/im/im_feed_shortcut_list.go
670a38c to
0768e2b
Compare
0768e2b to
45f69d2
Compare
…uite#1273) Adds feed shortcut management to the im domain: pin chats to the user's feed sidebar, list pinned entries, and unpin them. Three new shortcuts wrap the im/v2/feed_shortcuts OpenAPI routes, which currently expose CHAT-type entries only and accept user identity only.
Adds feed shortcut management to the im domain: pin chats to the user's feed sidebar, list pinned entries, and unpin them. Three new shortcuts wrap the im/v2/feed_shortcuts OpenAPI routes, which currently expose CHAT-type entries only and accept user identity only.
Summary
Adds feed shortcut management to the
imdomain: pin chats to the user's feed sidebar, list pinned entries, and unpin them. Three new shortcuts wrap theim/v2/feed_shortcutsOpenAPI routes, which currently expose CHAT-type entries only and accept user identity only.Changes
im +feed-shortcut-create— pin up to 10 chats per call (--chat-id, repeatable or comma-separated);--head/--tailcontrol insertion position; partial failures return a batch ledger (total/success_count/failure_count/succeeded_shortcuts) withok:falseand exit 1im +feed-shortcut-remove— unpin up to 10 chats per call with the same batch ledger semanticsim +feed-shortcut-list— list one page of feed shortcuts; entries are enriched with the full chat object underdetailby default (--no-detailto skip the extra call andim:chat:readscope); enrichment failures degrade to a stderr warningreason_labelskills/lark-im/references/Test Plan
go test ./shortcuts/im/— 35 unit tests covering ID validation, batch ledger accounting, failure-reason annotation, detail enrichment (batching, p2p chats, scope errors, degradation), pagination semantics, and dry-run renderinggofmt/go vet/golangci-lint run --new-from-rev=origin/maincleantests/cli_e2e/im/feed_shortcut_workflow_test.go(create→list→remove roundtrip, batch ledger, dry-run), executed by CIdetailattached) → remove restores the original state;--no-detail,--dry-run, and missing--chat-id(exit 2 validation error) all checkedRelated Issues
Fixes #980
Summary by CodeRabbit
New Features
--chat-idinput (max 10 per call),--head/--tail, dry-run previews, ledger-style batch results with deterministic succeeded_shortcuts.Behavior
--no-detail); enrichment batches lookups (50 ids/chunk), warns on failure but returns base list. Per-item failures include stable reason_label; partial failures yield non-zero exit.Tests
Documentation