Open
Conversation
github-actions
Bot
added
domain/im
|
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 five IM CLI shortcuts (app-feed-card create/update/delete and two feed-card time-sensitive toggles), their implementations, extensive unit and E2E tests, and registers the new shortcuts at the start of the shortcuts list. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Shortcut
participant Validator
participant IM_API
CLI->>Shortcut: parse flags / --card-json / --user-ids (dry-run or execute)
Shortcut->>Validator: normalize link/buttons/status, derive update_fields, validate inputs
Validator-->>Shortcut: validation result
alt dry-run
Shortcut-->>CLI: emit dry-run JSON with HTTP method, path, params, body
else execute
Shortcut->>IM_API: POST/PUT/DELETE /open-apis/im/v2/app_feed_card{,/batch} with payload
IM_API-->>Shortcut: response (biz_id, requested_count, failed_count, failed_cards)
Shortcut->>CLI: format stdout (biz_id, requested_count, failed_count, table)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/im/im_app_feed_card_create.go`:
- Around line 228-238: The raw-JSON branch in buildAppFeedCardUpdateBody (and
analogous branches at the other locations) only checks non-empty user_id via
parseAppFeedCardsJSON/validateAppFeedCardUpdateItems, so malformed open_ids can
slip through; update the raw JSON handling to call validateAppFeedUserIDs on the
extracted feedCards (same validation used for flag-built paths) before
returning, e.g., after parseAppFeedCardsJSON succeeds invoke
validateAppFeedUserIDs(runtime, feedCards, "--user-id-type") (or the equivalent
function signature used elsewhere) and return its error if any; apply the same
change to the other affected functions/blocks referenced (around the 275-285 and
571-612 regions) so raw JSON and flag-built paths share identical user_id
validation.
- Around line 680-714: The validation currently only checks multi_url map length
and allows invalid/opaque HTTPS forms; update the logic so that when actionType
== "url_page" you require at least one non-empty, successfully validated URL in
multiURL (count validated URLs in the loop over keys
"url","android_url","ios_url","pc_url" and treat an empty or invalid URL as not
counting), and change validateAppFeedURL(fieldName, raw, allowAppLink) to reject
opaque or hostless HTTPS URLs by requiring u.Scheme == "https" to have a
non-empty u.Host (and keep applink handling via allowAppLink as before); ensure
the earlier loop calls validateAppFeedURL and increments a valid-count, and if
actionType == "url_page" and valid-count == 0 return the ErrValidation about
multi_url being required.
In `@tests/cli_e2e/im/app_feed_card_workflow_test.go`:
- Around line 140-144: Register the cleanup before asserting the response by
moving the t.Cleanup(...) up so it always runs even if the require fails; use
the original requested biz ID (e.g., the existing bizID/requestedBizID variable
used to create the card) as the cleanup fallback and only attempt to delete the
returnedBizID if it's non-empty, toggling the deleted flag inside the cleanup
when deletion succeeds, then perform require.NotEmpty(t, returnedBizID, ...)
after the cleanup is registered.
- Around line 38-40: The test is asserting the "=== Dry Run ===" banner against
result.Stderr while firstIMDryRunRequest(t, result.Stdout) parses the banner
from stdout; update the assertions that currently call assert.Contains(t,
result.Stderr, "=== Dry Run ===") to assert.Contains(t, result.Stdout, "=== Dry
Run ===") (there are multiple occurrences near the firstIMDryRunRequest calls),
so the banner checks and the parser both use Stdout consistently.
🪄 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: d6ae427f-4e98-4151-8b8e-2b5f15c19255
📒 Files selected for processing (5)
shortcuts/im/helpers_test.goshortcuts/im/im_app_feed_card_create.goshortcuts/im/im_app_feed_card_create_test.goshortcuts/im/shortcuts.gotests/cli_e2e/im/app_feed_card_workflow_test.go
renxianwei98
force-pushed
the
feat/app_feed
branch
from
6bc0086 to
642f492
Compare
|
|
|
renxianwei seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
tests/cli_e2e/im/app_feed_card_workflow_test.go (2)
140-162:⚠️ Potential issue | 🟠 MajorRegister cleanup before asserting the returned
biz_id.If create succeeds but the response omits
data.biz_id,require.NotEmptyaborts before cleanup is registered, leaving the requested live card behind. Use the requestedbizIDas a cleanup fallback, then assert the returned field.🧹 Proposed fix
returnedBizID := gjson.Get(result.Stdout, "data.biz_id").String() - require.NotEmpty(t, returnedBizID, "stdout:\n%s", result.Stdout) + cleanupBizID := returnedBizID + if cleanupBizID == "" { + cleanupBizID = bizID + } deleted := false t.Cleanup(func() { @@ "feed_cards": []map[string]string{{ - "biz_id": returnedBizID, + "biz_id": cleanupBizID, "user_id": selfOpenID, }}, }, }) clie2e.ReportCleanupFailure(t, "delete app feed card", cleanupResult, cleanupErr) }) + require.NotEmpty(t, returnedBizID, "stdout:\n%s", result.Stdout)As per coding guidelines, Live E2E tests must be self-contained with create/use/cleanup flows and placed in
tests/cli_e2e/<domain>/.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/im/app_feed_card_workflow_test.go` around lines 140 - 162, Register the cleanup before asserting the response: move the t.Cleanup registration so it runs immediately after creating the requested bizID (the value you sent in the create request), use that requested bizID as a fallback inside the cleanup block (instead of only using returnedBizID), and then assert require.NotEmpty(t, returnedBizID) afterwards; update references to returnedBizID and the deleted flag inside the t.Cleanup closure so the cleanup always runs even if require.NotEmpty aborts, and keep the existing clie2e.RunCmd cleanup call and clie2e.ReportCleanupFailure usage intact.
38-40:⚠️ Potential issue | 🟡 MinorCheck the dry-run banner on stdout, not stderr.
firstIMDryRunRequestparses the dry-run payload fromStdout, but these assertions look for the banner inStderr. This can fail even when the dry-run output is valid.🧪 Proposed fix
- assert.Contains(t, result.Stderr, "=== Dry Run ===") + assert.Contains(t, result.Stdout, "=== Dry Run ===") @@ - assert.Contains(t, updateResult.Stderr, "=== Dry Run ===") + assert.Contains(t, updateResult.Stdout, "=== Dry Run ===") @@ - assert.Contains(t, deleteResult.Stderr, "=== Dry Run ===") + assert.Contains(t, deleteResult.Stdout, "=== Dry Run ===")Also applies to: 73-75, 97-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/im/app_feed_card_workflow_test.go` around lines 38 - 40, The test is asserting the dry-run banner in result.Stderr but the dry-run payload is on result.Stdout (firstIMDryRunRequest reads Stdout), so update the failing assertions to check result.Stdout instead of result.Stderr; specifically replace assert.Contains(t, result.Stderr, "=== Dry Run ===") with assert.Contains(t, result.Stdout, "=== Dry Run ===") at the occurrences around firstIMDryRunRequest and the other two similar blocks (the same change applies to the assertions at the other two ranges).shortcuts/im/im_app_feed_card_create.go (2)
228-238:⚠️ Potential issue | 🟡 MinorValidate raw
feed_cards[].user_idagainst--user-id-type.The raw
--feed-cards-jsonpaths still only check thatuser_idis non-empty, while the flag-built paths callvalidateAppFeedUserIDs. With the defaultopen_id, malformed IDs can reach update/delete APIs.🛡️ Proposed fix
- if err := validateAppFeedCardUpdateItems(feedCards); err != nil { + if err := validateAppFeedCardUpdateItems(feedCards, appFeedCardUserIDType(runtime)); err != nil { return nil, err } @@ - if err := validateAppFeedCardDeleteItems(feedCards); err != nil { + if err := validateAppFeedCardDeleteItems(feedCards, appFeedCardUserIDType(runtime)); err != nil { return nil, err } @@ -func validateAppFeedCardDeleteItems(feedCards []interface{}) error { +func validateAppFeedCardDeleteItems(feedCards []interface{}, userIDType string) error { for i, raw := range feedCards { @@ - if strings.TrimSpace(stringField(item, "user_id")) == "" { + userID := strings.TrimSpace(stringField(item, "user_id")) + if userID == "" { return output.ErrValidation("feed_cards[%d].user_id is required", i) } + if err := validateAppFeedUserIDs([]string{userID}, userIDType, fmt.Sprintf("feed_cards[%d].user_id", i)); err != nil { + return err + } } return nil } -func validateAppFeedCardUpdateItems(feedCards []interface{}) error { +func validateAppFeedCardUpdateItems(feedCards []interface{}, userIDType string) error { for i, raw := range feedCards { @@ - if strings.TrimSpace(stringField(item, "user_id")) == "" { + userID := strings.TrimSpace(stringField(item, "user_id")) + if userID == "" { return output.ErrValidation("feed_cards[%d].user_id is required", i) } + if err := validateAppFeedUserIDs([]string{userID}, userIDType, fmt.Sprintf("feed_cards[%d].user_id", i)); err != nil { + return err + }Also applies to: 275-285, 571-612
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/im/im_app_feed_card_create.go` around lines 228 - 238, The raw JSON path in buildAppFeedCardUpdateBody parses feed cards via parseAppFeedCardsJSON and calls validateAppFeedCardUpdateItems but does not validate each feed_cards[].user_id against the current --user-id-type; update this flow to call validateAppFeedUserIDs (the same validator used by the flag-built paths) after parsing (or inside validateAppFeedCardUpdateItems) so malformed IDs are rejected before returning map[string]interface{}{"feed_cards": feedCards}; apply the same change to the other raw-JSON handlers mentioned (the similar blocks around the other feed-card create/update/delete code).
680-714:⚠️ Potential issue | 🟠 MajorRequire an actual HTTPS URL for
url_pagebuttons.
multi_url: {"url": ""}or a non-string URL currently satisfies thelen(multiURL) > 0check, andhttps:example.compasses becauseurl.Parsetreats it as an opaque URL with no host. Require at least one non-empty validated URL and reject hostless HTTPS values.🛡️ Proposed fix
if multiURL, _ := button["multi_url"].(map[string]interface{}); len(multiURL) > 0 { + hasURL := false for _, key := range []string{"url", "android_url", "ios_url", "pc_url"} { - if raw, _ := multiURL[key].(string); strings.TrimSpace(raw) != "" { - if err := validateAppFeedURL(fmt.Sprintf("buttons.buttons[%d].multi_url.%s", index, key), raw, false); err != nil { + rawValue, exists := multiURL[key] + if !exists { + continue + } + raw, ok := rawValue.(string) + if !ok { + return output.ErrValidation("buttons.buttons[%d].multi_url.%s must be a string", index, key) + } + if strings.TrimSpace(raw) != "" { + hasURL = true + if err := validateAppFeedURL(fmt.Sprintf("buttons.buttons[%d].multi_url.%s", index, key), raw, false); err != nil { return err } } } + if actionType == "url_page" && !hasURL { + return output.ErrValidation("buttons.buttons[%d].multi_url must contain at least one HTTPS URL when action_type is url_page", index) + } } @@ switch u.Scheme { case "https": + if u.Host == "" { + return output.ErrValidation("%s must be a valid HTTPS URL", fieldName) + } return nilIn Go net/url, does url.Parse("https:example.com") parse successfully with Scheme "https" and an empty Host due to scheme:opaque syntax?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/im/im_app_feed_card_create.go` around lines 680 - 714, The code currently treats a non-empty multi_url map as valid and accepts hostless HTTPS values like "https:example.com"; update the validation so validateAppFeedURL(fieldName, raw, allowAppLink) rejects hostless HTTPS by requiring u.Scheme=="https" and u.Host!="" (treat applink per allowAppLink), and change the multi_url handling (in the loop that inspects buttons["multi_url"]) to only consider keys whose values are strings with strings.TrimSpace(raw) != "" and treat the entire action_type == "url_page" check as requiring at least one successfully validated URL (call validateAppFeedURL and count successes) rather than only checking len(multiURL) > 0, returning ErrValidation if none are valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/im/im_app_feed_card_create_test.go`:
- Around line 115-121: The test's expected error string should match the actual
error returned by validateAppFeedUserIDs (which returns common.ValidateUserID);
update the test case ("invalid recipient open id") to assert against
common.ValidateUserID (e.g., compare error strings with
common.ValidateUserID.Error() or check equality to that error) instead of the
current substring "invalid user ID format" so the assertion matches the exact
error returned by validateAppFeedUserIDs.
---
Duplicate comments:
In `@shortcuts/im/im_app_feed_card_create.go`:
- Around line 228-238: The raw JSON path in buildAppFeedCardUpdateBody parses
feed cards via parseAppFeedCardsJSON and calls validateAppFeedCardUpdateItems
but does not validate each feed_cards[].user_id against the current
--user-id-type; update this flow to call validateAppFeedUserIDs (the same
validator used by the flag-built paths) after parsing (or inside
validateAppFeedCardUpdateItems) so malformed IDs are rejected before returning
map[string]interface{}{"feed_cards": feedCards}; apply the same change to the
other raw-JSON handlers mentioned (the similar blocks around the other feed-card
create/update/delete code).
- Around line 680-714: The code currently treats a non-empty multi_url map as
valid and accepts hostless HTTPS values like "https:example.com"; update the
validation so validateAppFeedURL(fieldName, raw, allowAppLink) rejects hostless
HTTPS by requiring u.Scheme=="https" and u.Host!="" (treat applink per
allowAppLink), and change the multi_url handling (in the loop that inspects
buttons["multi_url"]) to only consider keys whose values are strings with
strings.TrimSpace(raw) != "" and treat the entire action_type == "url_page"
check as requiring at least one successfully validated URL (call
validateAppFeedURL and count successes) rather than only checking len(multiURL)
> 0, returning ErrValidation if none are valid.
In `@tests/cli_e2e/im/app_feed_card_workflow_test.go`:
- Around line 140-162: Register the cleanup before asserting the response: move
the t.Cleanup registration so it runs immediately after creating the requested
bizID (the value you sent in the create request), use that requested bizID as a
fallback inside the cleanup block (instead of only using returnedBizID), and
then assert require.NotEmpty(t, returnedBizID) afterwards; update references to
returnedBizID and the deleted flag inside the t.Cleanup closure so the cleanup
always runs even if require.NotEmpty aborts, and keep the existing clie2e.RunCmd
cleanup call and clie2e.ReportCleanupFailure usage intact.
- Around line 38-40: The test is asserting the dry-run banner in result.Stderr
but the dry-run payload is on result.Stdout (firstIMDryRunRequest reads Stdout),
so update the failing assertions to check result.Stdout instead of
result.Stderr; specifically replace assert.Contains(t, result.Stderr, "=== Dry
Run ===") with assert.Contains(t, result.Stdout, "=== Dry Run ===") at the
occurrences around firstIMDryRunRequest and the other two similar blocks (the
same change applies to the assertions at the other two ranges).
🪄 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: 6db85f16-fd68-4f3f-89e3-9cc64e3e89e2
📒 Files selected for processing (5)
shortcuts/im/helpers_test.goshortcuts/im/im_app_feed_card_create.goshortcuts/im/im_app_feed_card_create_test.goshortcuts/im/shortcuts.gotests/cli_e2e/im/app_feed_card_workflow_test.go
✅ Files skipped from review due to trivial changes (2)
- shortcuts/im/shortcuts.go
- shortcuts/im/helpers_test.go
renxianwei98
force-pushed
the
feat/app_feed
branch
from
642f492 to
77bf970
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
shortcuts/im/im_app_feed_card_create_test.go (1)
115-120:⚠️ Potential issue | 🟠 MajorKeep invalid open-ID assertions in sync with
ValidateUserID.These cases still expect
"invalid user ID format", but the implementation returnscommon.ValidateUserIDdirectly for defaultopen_idvalidation, so assert the helper’s actual error text instead.#!/bin/bash # Description: Verify the exact error emitted by common.ValidateUserID and locate stale assertions. rg -n -C3 'func ValidateUserID|ValidateUserID|invalid user ID format|user ID must start' --type=go🧪 Proposed fix
- wantErr: "invalid user ID format", + wantErr: "user ID must start with 'ou_'", @@ - wantErr: "invalid user ID format", + wantErr: "user ID must start with 'ou_'", @@ - wantErr: "invalid user ID format", + wantErr: "user ID must start with 'ou_'",Also applies to: 336-345
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/im/im_app_feed_card_create_test.go` around lines 115 - 120, The test expectation for the "invalid recipient open id" case (and the similar case around lines 336-345) is stale: instead of asserting the literal "invalid user ID format" you should assert the actual error string produced by common.ValidateUserID; update the test cases in im_app_feed_card_create_test.go (the case named "invalid recipient open id" and its duplicate) to call or reference common.ValidateUserID's error text (or assert equality with the error returned by common.ValidateUserID) so the test matches ValidateUserID's current message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@shortcuts/im/im_app_feed_card_create_test.go`:
- Around line 115-120: The test expectation for the "invalid recipient open id"
case (and the similar case around lines 336-345) is stale: instead of asserting
the literal "invalid user ID format" you should assert the actual error string
produced by common.ValidateUserID; update the test cases in
im_app_feed_card_create_test.go (the case named "invalid recipient open id" and
its duplicate) to call or reference common.ValidateUserID's error text (or
assert equality with the error returned by common.ValidateUserID) so the test
matches ValidateUserID's current message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 709d5ff2-4354-40a7-8f2d-fbb1b2e0da77
📒 Files selected for processing (5)
shortcuts/im/helpers_test.goshortcuts/im/im_app_feed_card_create.goshortcuts/im/im_app_feed_card_create_test.goshortcuts/im/shortcuts.gotests/cli_e2e/im/app_feed_card_workflow_test.go
✅ Files skipped from review due to trivial changes (1)
- shortcuts/im/shortcuts.go
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/im/helpers_test.go
renxianwei98
force-pushed
the
feat/app_feed
branch
from
77bf970 to
6ad7c55
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
shortcuts/im/im_app_feed_card_create_test.go (1)
115-121:⚠️ Potential issue | 🟠 Major
wantErrsubstring likely does not matchcommon.ValidateUserID's actual error message.The "invalid recipient open id" case (and the analogous raw-JSON cases on lines 338-339 and 344-345) expects the substring
"invalid user ID format", butvalidateAppFeedUserIDssimply propagatescommon.ValidateUserIDunchanged (seeim_app_feed_card_create.golines 561-567). Prior reviews indicated that helper returns something like"user ID must start with 'ou_'", which would make all three of these subtests fail if they actually run. Please confirm against the helper and align the expected substring (e.g., to"user ID must start with 'ou_'") or switch to anerrors.Is/exact-match assertion.#!/bin/bash # Verify exact error string(s) produced by common.ValidateUserID. rg -nP -C3 '\bfunc\s+ValidateUserID\s*\(' --type=go rg -nP -C1 '"user ID.*ou_|invalid user ID format"' --type=go🛡️ Proposed fix (adjust if verification yields a different substring)
- wantErr: "invalid user ID format", + wantErr: "user ID must start with 'ou_'", @@ - wantErr: "invalid user ID format", + wantErr: "user ID must start with 'ou_'", @@ - wantErr: "invalid user ID format", + wantErr: "user ID must start with 'ou_'",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/im/im_app_feed_card_create_test.go` around lines 115 - 121, The test case in im_app_feed_card_create_test.go expects the wrong error substring; update the "invalid recipient open id" subtest (and the analogous raw-JSON subtests) to match the actual error returned by common.ValidateUserID (or change the assertion style). Locate the failing tests referencing validateAppFeedUserIDs and common.ValidateUserID and either replace the wantErr value with the exact message produced by common.ValidateUserID (e.g., "user ID must start with 'ou_'" if that is what ValidateUserID returns) or modify the test to perform an errors.Is/exact-match/assert.Contains check against the helper's actual error instead of the current "invalid user ID format" substring.
🧹 Nitpick comments (2)
shortcuts/im/im_app_feed_card_create.go (2)
535-538: Nit: redundant empty check.
len(nil) == 0for maps, socard == nil || len(card) == 0can be reduced tolen(card) == 0.♻️ Diff
- if card == nil || len(card) == 0 { + if len(card) == 0 { return output.ErrValidation("app_feed_card cannot be empty") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/im/im_app_feed_card_create.go` around lines 535 - 538, In validateAppFeedCardUpdateObject, remove the redundant nil check—replace the conditional "if card == nil || len(card) == 0" with a single "if len(card) == 0" check so the function (validateAppFeedCardUpdateObject) still returns output.ErrValidation("app_feed_card cannot be empty") for empty maps but avoids the unnecessary card == nil branch.
263-272: Optional: don't share the samecardmap across everyfeed_cardsentry.Each element in
feedCardspoints at the exact samecardmap, so any downstream code that later mutatesentry["app_feed_card"]for one user will alter all of them. Today nothing mutates it, but adeep-copy-per-entry(or assigning the map by value into a fresh map per user) would make this more robust against future refactors. Low priority.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/im/im_app_feed_card_create.go` around lines 263 - 272, The current loop appends the same card map reference for every user (feedCards built from userIDs), so mutate-in-place later would affect all entries; fix by creating a fresh copy of the card map for each user inside the loop (e.g., shallow-copy the map into a new map and use that for the "app_feed_card" value, or perform a deep copy if card contains nested maps/slices) when constructing each entry that uses updateFields and userID in the feedCards slice.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@shortcuts/im/im_app_feed_card_create_test.go`:
- Around line 115-121: The test case in im_app_feed_card_create_test.go expects
the wrong error substring; update the "invalid recipient open id" subtest (and
the analogous raw-JSON subtests) to match the actual error returned by
common.ValidateUserID (or change the assertion style). Locate the failing tests
referencing validateAppFeedUserIDs and common.ValidateUserID and either replace
the wantErr value with the exact message produced by common.ValidateUserID
(e.g., "user ID must start with 'ou_'" if that is what ValidateUserID returns)
or modify the test to perform an errors.Is/exact-match/assert.Contains check
against the helper's actual error instead of the current "invalid user ID
format" substring.
---
Nitpick comments:
In `@shortcuts/im/im_app_feed_card_create.go`:
- Around line 535-538: In validateAppFeedCardUpdateObject, remove the redundant
nil check—replace the conditional "if card == nil || len(card) == 0" with a
single "if len(card) == 0" check so the function
(validateAppFeedCardUpdateObject) still returns
output.ErrValidation("app_feed_card cannot be empty") for empty maps but avoids
the unnecessary card == nil branch.
- Around line 263-272: The current loop appends the same card map reference for
every user (feedCards built from userIDs), so mutate-in-place later would affect
all entries; fix by creating a fresh copy of the card map for each user inside
the loop (e.g., shallow-copy the map into a new map and use that for the
"app_feed_card" value, or perform a deep copy if card contains nested
maps/slices) when constructing each entry that uses updateFields and userID in
the feedCards slice.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1608c05d-ca97-4312-9da2-e104b268f362
📒 Files selected for processing (5)
shortcuts/im/helpers_test.goshortcuts/im/im_app_feed_card_create.goshortcuts/im/im_app_feed_card_create_test.goshortcuts/im/shortcuts.gotests/cli_e2e/im/app_feed_card_workflow_test.go
✅ Files skipped from review due to trivial changes (1)
- shortcuts/im/helpers_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/im/shortcuts.go
Add IM shortcuts for app feed card create, update, and delete. The shortcuts wrap POST /open-apis/im/v2/app_feed_card and PUT/DELETE /open-apis/im/v2/app_feed_card/batch with bot identity. Support common card fields through flags, including title, preview, link, status label, buttons, time_sensitive, and notification settings. Also support raw JSON inputs for app_feed_card, buttons, and batch feed_cards payloads. Normalize app feed card update_fields from friendly names such as title, preview, link, and notify to API enum values such as 1, 3, 12, and 103 to avoid field validation failures. Address PR review feedback by validating raw feed_cards user IDs consistently with flag-built paths, rejecting hostless HTTPS URLs and url_page buttons without a non-empty HTTPS URL, keeping dry-run E2E focused on stdout request JSON, registering live workflow cleanup before response biz_id assertions, aligning tests with common.ValidateUserID errors, and avoiding shared app_feed_card map references across batch update entries. Verification: go test ./shortcuts/im -run 'TestBuildAppFeedCard|TestAppFeedCard|TestShortcuts' -count=1; go test ./shortcuts/im ./shortcuts -count=1; go vet ./shortcuts/im; go build -o /tmp/lark-cli-app-feed-card-test .; dry-run E2E for create/update/delete. Live workflow is permission-aware and skips when im:app_feed_card:write is unavailable.
renxianwei98
force-pushed
the
feat/app_feed
branch
2 times, most recently
from
7f86e32 to
2fc2119
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/cli_e2e/im/app_feed_card_workflow_test.go`:
- Around line 181-191: The cleanup registration must happen immediately after
verifying the command exited successfully to ensure teardown always runs; change
the flow in the test around result.AssertExitCode(t, 0) /
result.AssertStdoutStatus(t, true) by initializing cleanupBizID := bizID and
calling t.Cleanup(...) right after result.AssertExitCode(t, 0), then parse
returnedBizID (from gjson.Get(result.Stdout, "data.biz_id").String()) and, if
non-empty, update cleanupBizID = returnedBizID before any assertions that might
fail; keep the deleted flag and cleanup logic intact but ensure t.Cleanup is
registered before calling AssertStdoutStatus and parsing returnedBizID.
🪄 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: 681d6f4f-3824-4eb3-a1c7-eede1c7bad09
📒 Files selected for processing (7)
shortcuts/im/helpers_test.goshortcuts/im/im_app_feed_card_create.goshortcuts/im/im_app_feed_card_create_test.goshortcuts/im/im_feed_card_time_sensitive.goshortcuts/im/im_feed_card_time_sensitive_test.goshortcuts/im/shortcuts.gotests/cli_e2e/im/app_feed_card_workflow_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/im/shortcuts.go
renxianwei98
force-pushed
the
feat/app_feed
branch
4 times, most recently
from
4a31645 to
cc58fe7
Compare
Add bot-only IM shortcuts for feed card temporary pinning: +feed-card-bot-time-sensitive wraps PATCH /open-apis/im/v2/feed_cards/bot_time_sentive, and +feed-card-time-sensitive wraps PATCH /open-apis/im/v2/feed_cards/{feed_card_id}.
Both shortcuts accept --user-ids, --user-id-type, and an explicit --time-sensitive flag so true pins and false unpins are both represented in the request body. The feed_card_id path segment is escaped before execution.
Add unit tests for payload construction, validation, dry-run shape, and execute requests, plus CLI dry-run E2E coverage for both new commands.
Address PR review feedback by registering the app feed card live workflow cleanup immediately after a successful create exit before stdout assertions can fail.
Verification: go test ./shortcuts/im -run 'TestBuildFeedCard|TestFeedCard|TestShortcuts' -count=1; go test ./shortcuts/im ./shortcuts -count=1; go vet ./shortcuts/im; go build -o /tmp/lark-cli-feed-card-test .; env LARK_CLI_BIN=/tmp/lark-cli-feed-card-test go test ./tests/cli_e2e/im -run 'TestIM_(AppFeedCard|FeedCard).*DryRun' -count=1.
Signed-off-by: renxianwei <renxianwei@bytedance.com>
renxianwei98
force-pushed
the
feat/app_feed
branch
from
cc58fe7 to
a4adaa2
Compare
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
validation and stronger test coverage.