feat(slides): add history rollback shortcuts#1714
Conversation
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds three new slides CLI shortcuts for listing history, reverting to a history version, and checking revert status, along with shared validation/helpers, tests, and updated slide skill documentation. ChangesSlides history shortcuts implementation and validation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as Slides CLI
participant Wiki as Wiki API
participant Slides as Slides API
User->>CLI: +history-list / +history-revert / +history-revert-status
CLI->>CLI: validate args and build request data
alt wiki presentation URL
CLI->>Wiki: resolve wiki node
Wiki-->>CLI: slides presentation token
end
alt +history-list
CLI->>Slides: GET histories
Slides-->>CLI: history list response
else +history-revert
CLI->>Slides: POST history/revert
Slides-->>CLI: task_id and status
else +history-revert-status
CLI->>Slides: GET history/revert_status
Slides-->>CLI: revert task status
end
CLI-->>User: raw API response
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 #1714 +/- ##
========================================
Coverage 74.52% 74.53%
========================================
Files 851 852 +1
Lines 87155 87316 +161
========================================
+ Hits 64952 65080 +128
- Misses 17231 17249 +18
- Partials 4972 4987 +15 ☔ 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@40a828cb5e1942d109e4ea24d59e66e7b03fdbde🧩 Skill updatenpx skills add larksuite/cli#feat/slides_revert -y -g |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tests/cli_e2e/slides/slides_history_dryrun_test.go (1)
46-59: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winZero-value assertion can't distinguish "field present as 0" from "field missing".
gjson.Get(stdout, "api.0.body.wait_timeout_ms").Int()returns0both when the field is present with value0and when the path doesn't exist. If the request struct ever addsomitemptyonwait_timeout_ms(or the value is dropped some other way), this assertion would keep passing even though the field silently disappeared from the request body. Recommend also asserting.Exists()for this zero-value case.🔧 Suggested strengthening
assertion: func(t *testing.T, stdout string) { require.Equal(t, "42", gjson.Get(stdout, "api.0.body.history_version_id").String(), stdout) - require.Equal(t, int64(0), gjson.Get(stdout, "api.0.body.wait_timeout_ms").Int(), stdout) + waitTimeout := gjson.Get(stdout, "api.0.body.wait_timeout_ms") + require.True(t, waitTimeout.Exists(), "wait_timeout_ms missing from body: %s", stdout) + require.Equal(t, int64(0), waitTimeout.Int(), stdout) },🤖 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/slides/slides_history_dryrun_test.go` around lines 46 - 59, Strengthen the zero-value check in the slides history dry-run test so it verifies both presence and value of wait_timeout_ms. In the assertion for the revert case, update the gjson-based check on stdout to confirm the path exists before asserting it equals 0, using the existing assertion block in slides_history_dryrun_test.go and the api.0.body.wait_timeout_ms field to keep the test from passing if the field is omitted.tests/cli_e2e/slides/slides_history_workflow_test.go (2)
20-28: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTight 3-minute context budget vs. cumulative retry/wait timeouts risks flaky failures.
The shared
ctx(line 27) has a 3-minute deadline and is passed to everyRunCmdcall in the test. The retry loops alone can consume up to 45s (history-listEventually, line 138) + 30s (--wait-timeout-ms 30000synchronous wait inside+history-revert, line 146) + 60s (revert-statusEventually, line 172) = 135s, leaving only ~45s of margin for+create, twoapi getfetches,+replace-pages, and the finalapi getfetch. If any step runs slower than usual, the shared ctx can expire mid-poll, causingRunCmd/require.NoErrorfailures unrelated to the actual revert logic and producing misleading failure messages.Consider increasing the ctx timeout (e.g., to 5 minutes) to give real margin over the sum of the retry/wait budgets, since this is an opt-in live E2E test where flakiness costs debugging time rather than CI throughput.
Also applies to: 116-173
🤖 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/slides/slides_history_workflow_test.go` around lines 20 - 28, The shared timeout in TestSlides_HistoryWorkflow is too tight for the cumulative retry and wait budgets used by the RunCmd steps, which can cause unrelated context-deadline failures. Increase the context deadline created with context.WithTimeout in TestSlides_HistoryWorkflow to give enough margin for all eventual/retry waits, and keep using that ctx for the existing RunCmd and require checks so the live E2E flow remains stable under slower conditions.
117-138: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSwallowed errors reduce debuggability on
Eventuallyfailure.Both retry closures discard
listErr/statusErron failure paths (returningfalsewithout recording them). If the loop times out, the failure message only says "did not expose..."/"did not finish" without the underlying CLI error, making root-causing flaky runs harder. Consider capturing the last error/exit code and including it in therequire.Eventuallyfailure message.Also applies to: 158-172
🤖 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/slides/slides_history_workflow_test.go` around lines 117 - 138, The retry closures in the slide history workflow tests are swallowing CLI failures, so `require.Eventually` times out without the underlying error context. Update the `slides_history_workflow_test.go` logic around the `slides +history-list` and related status checks to record the last `listErr`/`statusErr` and non-zero exit code, then include that captured detail in the final `require.Eventually` failure message. Use the existing `require.Eventually` blocks and the `RunCmd` calls as the places to preserve and surface the error for debugging.shortcuts/slides/slides_history_test.go (1)
343-383: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a negative test for wiki resolution to a non-slides node.
TestSlidesHistoryExecuteResolvesWikiPresentationonly covers the happy path whereobj_type == "slides". A test asserting the behavior when a wiki node resolves to a differentobj_type(e.g.,doc) would guard the resolution logic referenced inslides_history.goagainst regressions on this realistic user error path.🤖 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/slides/slides_history_test.go` around lines 343 - 383, Add a negative test around SlidesHistoryList wiki resolution to cover non-slides nodes: extend the existing resolution flow in TestSlidesHistoryExecuteResolvesWikiPresentation or add a sibling test that stubs /open-apis/wiki/v2/spaces/get_node to return an obj_type other than "slides" (for example "doc"), then verify runSlidesShortcut returns the expected error and does not proceed to fetch histories. Use the SlidesHistoryList path and the wiki node resolution logic in slides_history.go to locate the behavior under test.
🤖 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/slides/slides_history_test.go`:
- Around line 106-128: The validation error test in the slides history suite
only checks that the error is typed and matches Param, but it does not verify
the full typed metadata or wrapped cause. Update the test cases in the
`runSlidesShortcut` loop to inspect the `errs.ProblemOf` result for `Category`,
`Subtype`, and `Param`, and add assertions that the original cause is preserved
with `errors.Is` and/or `errors.Unwrap`. Keep using `errs.ValidationError` and
`errs.ProblemOf` to locate the error shape, but strengthen the checks so the
error-path coverage matches the test guidelines.
In `@shortcuts/slides/slides_history.go`:
- Around line 45-65: The three validators in validateSlidesHistoryPageSize,
validateSlidesHistoryVersionID, and validateSlidesHistoryWaitTimeout are putting
recovery guidance into the main error text instead of using .WithHint(...).
Update each errs.NewValidationError call so the message only states the invalid
input and keep the user-facing guidance like the valid range or “returned by
slides +history-list” in a separate .WithHint(...) on the returned error.
---
Nitpick comments:
In `@shortcuts/slides/slides_history_test.go`:
- Around line 343-383: Add a negative test around SlidesHistoryList wiki
resolution to cover non-slides nodes: extend the existing resolution flow in
TestSlidesHistoryExecuteResolvesWikiPresentation or add a sibling test that
stubs /open-apis/wiki/v2/spaces/get_node to return an obj_type other than
"slides" (for example "doc"), then verify runSlidesShortcut returns the expected
error and does not proceed to fetch histories. Use the SlidesHistoryList path
and the wiki node resolution logic in slides_history.go to locate the behavior
under test.
In `@tests/cli_e2e/slides/slides_history_dryrun_test.go`:
- Around line 46-59: Strengthen the zero-value check in the slides history
dry-run test so it verifies both presence and value of wait_timeout_ms. In the
assertion for the revert case, update the gjson-based check on stdout to confirm
the path exists before asserting it equals 0, using the existing assertion block
in slides_history_dryrun_test.go and the api.0.body.wait_timeout_ms field to
keep the test from passing if the field is omitted.
In `@tests/cli_e2e/slides/slides_history_workflow_test.go`:
- Around line 20-28: The shared timeout in TestSlides_HistoryWorkflow is too
tight for the cumulative retry and wait budgets used by the RunCmd steps, which
can cause unrelated context-deadline failures. Increase the context deadline
created with context.WithTimeout in TestSlides_HistoryWorkflow to give enough
margin for all eventual/retry waits, and keep using that ctx for the existing
RunCmd and require checks so the live E2E flow remains stable under slower
conditions.
- Around line 117-138: The retry closures in the slide history workflow tests
are swallowing CLI failures, so `require.Eventually` times out without the
underlying error context. Update the `slides_history_workflow_test.go` logic
around the `slides +history-list` and related status checks to record the last
`listErr`/`statusErr` and non-zero exit code, then include that captured detail
in the final `require.Eventually` failure message. Use the existing
`require.Eventually` blocks and the `RunCmd` calls as the places to preserve and
surface the error for debugging.
🪄 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: fd3fd404-7459-4fdb-bf02-e8803e08359e
📒 Files selected for processing (7)
shortcuts/slides/shortcuts.goshortcuts/slides/slides_history.goshortcuts/slides/slides_history_test.goskills/lark-slides/SKILL.mdskills/lark-slides/references/lark-slides-history.mdtests/cli_e2e/slides/slides_history_dryrun_test.gotests/cli_e2e/slides/slides_history_workflow_test.go
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| f, stdout, _, _ := cmdutil.TestFactory(t, slidesTestConfig(t, "")) | ||
| err := runSlidesShortcut(t, f, stdout, tt.shortcut, tt.args) | ||
| if err == nil { | ||
| t.Fatal("expected validation error, got nil") | ||
| } | ||
| _, ok := errs.ProblemOf(err) | ||
| if !ok { | ||
| t.Fatalf("error is not typed: %T %v", err, err) | ||
| } | ||
| var validationErr *errs.ValidationError | ||
| if !errors.As(err, &validationErr) { | ||
| t.Fatalf("expected validation error, got %T: %v", err, err) | ||
| } | ||
| if validationErr.Param != tt.param { | ||
| t.Fatalf("param = %q, want %q (err: %v)", validationErr.Param, tt.param, err) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validation tests don't assert category/subtype or cause preservation.
Per the coding guideline for **/*_test.go files, error-path tests should assert typed metadata via errs.ProblemOf covering category/subtype/param, plus cause preservation — not just message/param. Here, only Param is checked (via errors.As + ValidationError), and the errs.ProblemOf call at Line 115 is only used to confirm the error is typed (ok), without inspecting Category or Subtype. Cause preservation (errors.Is/errors.Unwrap) is also not asserted.
✅ Suggested strengthening
_, ok := errs.ProblemOf(err)
if !ok {
t.Fatalf("error is not typed: %T %v", err, err)
}
+ if problem, _ := errs.ProblemOf(err); problem.Category != errs.CategoryValidation {
+ t.Fatalf("category = %v, want validation", problem.Category)
+ }
var validationErr *errs.ValidationError
if !errors.As(err, &validationErr) {
t.Fatalf("expected validation error, got %T: %v", err, err)
}
if validationErr.Param != tt.param {
t.Fatalf("param = %q, want %q (err: %v)", validationErr.Param, tt.param, err)
}Based on coding guidelines, **/*_test.go: "Error-path tests must assert typed metadata via errs.ProblemOf (category / subtype / param) and cause preservation, not message substrings alone".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| t.Parallel() | |
| f, stdout, _, _ := cmdutil.TestFactory(t, slidesTestConfig(t, "")) | |
| err := runSlidesShortcut(t, f, stdout, tt.shortcut, tt.args) | |
| if err == nil { | |
| t.Fatal("expected validation error, got nil") | |
| } | |
| _, ok := errs.ProblemOf(err) | |
| if !ok { | |
| t.Fatalf("error is not typed: %T %v", err, err) | |
| } | |
| var validationErr *errs.ValidationError | |
| if !errors.As(err, &validationErr) { | |
| t.Fatalf("expected validation error, got %T: %v", err, err) | |
| } | |
| if validationErr.Param != tt.param { | |
| t.Fatalf("param = %q, want %q (err: %v)", validationErr.Param, tt.param, err) | |
| } | |
| }) | |
| } | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| t.Parallel() | |
| f, stdout, _, _ := cmdutil.TestFactory(t, slidesTestConfig(t, "")) | |
| err := runSlidesShortcut(t, f, stdout, tt.shortcut, tt.args) | |
| if err == nil { | |
| t.Fatal("expected validation error, got nil") | |
| } | |
| _, ok := errs.ProblemOf(err) | |
| if !ok { | |
| t.Fatalf("error is not typed: %T %v", err, err) | |
| } | |
| if problem, _ := errs.ProblemOf(err); problem.Category != errs.CategoryValidation { | |
| t.Fatalf("category = %v, want validation", problem.Category) | |
| } | |
| var validationErr *errs.ValidationError | |
| if !errors.As(err, &validationErr) { | |
| t.Fatalf("expected validation error, got %T: %v", err, err) | |
| } | |
| if validationErr.Param != tt.param { | |
| t.Fatalf("param = %q, want %q (err: %v)", validationErr.Param, tt.param, err) | |
| } | |
| }) | |
| } | |
| } |
🤖 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/slides/slides_history_test.go` around lines 106 - 128, The
validation error test in the slides history suite only checks that the error is
typed and matches Param, but it does not verify the full typed metadata or
wrapped cause. Update the test cases in the `runSlidesShortcut` loop to inspect
the `errs.ProblemOf` result for `Category`, `Subtype`, and `Param`, and add
assertions that the original cause is preserved with `errors.Is` and/or
`errors.Unwrap`. Keep using `errs.ValidationError` and `errs.ProblemOf` to
locate the error shape, but strengthen the checks so the error-path coverage
matches the test guidelines.
Source: Coding guidelines
| func validateSlidesHistoryPageSize(pageSize int) error { | ||
| if pageSize < 1 || pageSize > 20 { | ||
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "invalid --page-size %d: must be between 1 and 20", pageSize).WithParam("--page-size") | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func validateSlidesHistoryVersionID(historyVersionID string) error { | ||
| version, err := strconv.ParseInt(strings.TrimSpace(historyVersionID), 10, 64) | ||
| if err != nil || version <= 0 { | ||
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "--history-version-id must be a positive integer string returned by slides +history-list").WithParam("--history-version-id") | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func validateSlidesHistoryWaitTimeout(timeoutMs int) error { | ||
| if timeoutMs < 0 || timeoutMs > 30000 { | ||
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "invalid --wait-timeout-ms %d: must be between 0 and 30000", timeoutMs).WithParam("--wait-timeout-ms") | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move recovery guidance into .WithHint(...).
All three validators embed the "must be between X and Y" / "returned by slides +history-list" guidance directly in the error message instead of using .WithHint(...).
🔧 Proposed fix
func validateSlidesHistoryPageSize(pageSize int) error {
if pageSize < 1 || pageSize > 20 {
- return errs.NewValidationError(errs.SubtypeInvalidArgument, "invalid --page-size %d: must be between 1 and 20", pageSize).WithParam("--page-size")
+ return errs.NewValidationError(errs.SubtypeInvalidArgument, "invalid --page-size %d", pageSize).
+ WithParam("--page-size").
+ WithHint("must be between 1 and 20")
}
return nil
}
func validateSlidesHistoryVersionID(historyVersionID string) error {
version, err := strconv.ParseInt(strings.TrimSpace(historyVersionID), 10, 64)
if err != nil || version <= 0 {
- return errs.NewValidationError(errs.SubtypeInvalidArgument, "--history-version-id must be a positive integer string returned by slides +history-list").WithParam("--history-version-id")
+ return errs.NewValidationError(errs.SubtypeInvalidArgument, "--history-version-id must be a positive integer string").
+ WithParam("--history-version-id").
+ WithHint("use the history_version_id returned by slides +history-list")
}
return nil
}
func validateSlidesHistoryWaitTimeout(timeoutMs int) error {
if timeoutMs < 0 || timeoutMs > 30000 {
- return errs.NewValidationError(errs.SubtypeInvalidArgument, "invalid --wait-timeout-ms %d: must be between 0 and 30000", timeoutMs).WithParam("--wait-timeout-ms")
+ return errs.NewValidationError(errs.SubtypeInvalidArgument, "invalid --wait-timeout-ms %d", timeoutMs).
+ WithParam("--wait-timeout-ms").
+ WithHint("must be between 0 and 30000")
}
return nil
}As per coding guidelines: "param field in errors should only name the user input that actually failed; recovery guidance goes in .WithHint(...)".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func validateSlidesHistoryPageSize(pageSize int) error { | |
| if pageSize < 1 || pageSize > 20 { | |
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "invalid --page-size %d: must be between 1 and 20", pageSize).WithParam("--page-size") | |
| } | |
| return nil | |
| } | |
| func validateSlidesHistoryVersionID(historyVersionID string) error { | |
| version, err := strconv.ParseInt(strings.TrimSpace(historyVersionID), 10, 64) | |
| if err != nil || version <= 0 { | |
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "--history-version-id must be a positive integer string returned by slides +history-list").WithParam("--history-version-id") | |
| } | |
| return nil | |
| } | |
| func validateSlidesHistoryWaitTimeout(timeoutMs int) error { | |
| if timeoutMs < 0 || timeoutMs > 30000 { | |
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "invalid --wait-timeout-ms %d: must be between 0 and 30000", timeoutMs).WithParam("--wait-timeout-ms") | |
| } | |
| return nil | |
| } | |
| func validateSlidesHistoryPageSize(pageSize int) error { | |
| if pageSize < 1 || pageSize > 20 { | |
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "invalid --page-size %d", pageSize). | |
| WithParam("--page-size"). | |
| WithHint("must be between 1 and 20") | |
| } | |
| return nil | |
| } | |
| func validateSlidesHistoryVersionID(historyVersionID string) error { | |
| version, err := strconv.ParseInt(strings.TrimSpace(historyVersionID), 10, 64) | |
| if err != nil || version <= 0 { | |
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "--history-version-id must be a positive integer string"). | |
| WithParam("--history-version-id"). | |
| WithHint("use the history_version_id returned by slides +history-list") | |
| } | |
| return nil | |
| } | |
| func validateSlidesHistoryWaitTimeout(timeoutMs int) error { | |
| if timeoutMs < 0 || timeoutMs > 30000 { | |
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "invalid --wait-timeout-ms %d", timeoutMs). | |
| WithParam("--wait-timeout-ms"). | |
| WithHint("must be between 0 and 30000") | |
| } | |
| return nil | |
| } |
🤖 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/slides/slides_history.go` around lines 45 - 65, The three
validators in validateSlidesHistoryPageSize, validateSlidesHistoryVersionID, and
validateSlidesHistoryWaitTimeout are putting recovery guidance into the main
error text instead of using .WithHint(...). Update each errs.NewValidationError
call so the message only states the invalid input and keep the user-facing
guidance like the valid range or “returned by slides +history-list” in a
separate .WithHint(...) on the returned error.
Source: Coding guidelines
Summary
slides +history-list,slides +history-revert, andslides +history-revert-statusshortcutsTests
go test ./shortcuts/slides -run 'TestSlidesHistory'\n-go test ./tests/cli_e2e/slides -run 'TestSlidesHistoryDryRunE2E'\n\n## Notes\n- live workflow test is gated byLARK_SLIDES_HISTORY_E2E=1and was not run in this PR flow\nSummary by CodeRabbit
history_version_id, and check revert task status.history_version_id, wait timeout, and task id.