fix(drive): add rate limit recovery hints - #2253
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds rate-limit recovery hints for document media and Drive errors. It centralizes Drive throttling detection, propagates hints through shortcut execution handlers, updates batch classification, and adds document media and CLI dry-run coverage. ChangesRate-limit recovery guidance
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
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 (1)
shortcuts/drive/drive_upload.go (1)
178-181: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDecorate rate-limited metadata lookup errors before logging.
If
common.FetchDriveMetaURLreturns a typed rate-limit error,Executestill returnsnil. The deferred wrapper does not run onmetaErr. The warning then omits the recovery guidance.Apply
withDriveRateLimitRecoveryHint(metaErr)before writing the warning. Add a regression test for this successful-upload and rate-limited-URL-lookup 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/drive/drive_upload.go` around lines 178 - 181, Update the metaErr warning branch in Execute to pass FetchDriveMetaURL errors through withDriveRateLimitRecoveryHint before logging, while preserving the successful upload result. Add a regression test covering a successful upload followed by a typed rate-limit URL lookup failure and verify the warning includes the recovery guidance.
🤖 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/doc/doc_media_test.go`:
- Around line 828-831: The HTTP 429 contract tests must also validate the
returned error subtype through errs.ProblemOf. In
shortcuts/doc/doc_media_test.go:828-831, assert the expected subtype for the
wrapped preview error; in shortcuts/doc/doc_resource_cover_test.go:135-138,
assert the expected subtype for the wrapped cover-download error, while
preserving the existing category and status-code assertions.
In `@tests/cli_e2e/docs/docs_media_download_dryrun_test.go`:
- Around line 77-104: Extend TestDocsMediaPreviewDryRun_PlansPreviewDownload
with a live E2E workflow that creates or provisions the required media, invokes
docs +media-preview using bot credentials, and validates the actual streamed
preview and rate-limit recovery behavior through runtime.DoAPIStream. Make the
workflow self-contained by cleaning up all created resources, while preserving
the existing dry-run request-plan assertions.
---
Outside diff comments:
In `@shortcuts/drive/drive_upload.go`:
- Around line 178-181: Update the metaErr warning branch in Execute to pass
FetchDriveMetaURL errors through withDriveRateLimitRecoveryHint before logging,
while preserving the successful upload result. Add a regression test covering a
successful upload followed by a typed rate-limit URL lookup failure and verify
the warning includes the recovery guidance.
🪄 Autofix
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 Plus
Run ID: 1f8b5f4e-14b5-4a9e-940b-f84e25de7aee
📒 Files selected for processing (44)
shortcuts/doc/doc_errors.goshortcuts/doc/doc_media_preview.goshortcuts/doc/doc_media_test.goshortcuts/doc/doc_resource_cover.goshortcuts/doc/doc_resource_cover_test.goshortcuts/drive/drive_add_comment.goshortcuts/drive/drive_add_reply.goshortcuts/drive/drive_apply_permission.goshortcuts/drive/drive_batch_query_comments.goshortcuts/drive/drive_copy.goshortcuts/drive/drive_cover.goshortcuts/drive/drive_create_folder.goshortcuts/drive/drive_create_shortcut.goshortcuts/drive/drive_delete.goshortcuts/drive/drive_delete_reply.goshortcuts/drive/drive_download.goshortcuts/drive/drive_errors.goshortcuts/drive/drive_errors_test.goshortcuts/drive/drive_export.goshortcuts/drive/drive_export_download.goshortcuts/drive/drive_import.goshortcuts/drive/drive_inspect.goshortcuts/drive/drive_list_comments.goshortcuts/drive/drive_list_replies.goshortcuts/drive/drive_member_add.goshortcuts/drive/drive_member_list.goshortcuts/drive/drive_move.goshortcuts/drive/drive_permission_get_setting.goshortcuts/drive/drive_preview.goshortcuts/drive/drive_pull.goshortcuts/drive/drive_push.goshortcuts/drive/drive_react_reply.goshortcuts/drive/drive_resolve_comment.goshortcuts/drive/drive_search.goshortcuts/drive/drive_secure_label.goshortcuts/drive/drive_status.goshortcuts/drive/drive_sync.goshortcuts/drive/drive_task_result.goshortcuts/drive/drive_update_reply.goshortcuts/drive/drive_update_title.goshortcuts/drive/drive_upload.goshortcuts/drive/drive_version.gotests/cli_e2e/docs/coverage.mdtests/cli_e2e/docs/docs_media_download_dryrun_test.go
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@aa5de22cab035d0a0753f4cf0b860e5feb4e9e04🧩 Skill updatenpx skills add larksuite/cli#feat/fix-drive-hints -y -g |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2253 +/- ##
==========================================
+ Coverage 76.36% 76.37% +0.01%
==========================================
Files 1011 1011
Lines 111269 111330 +61
==========================================
+ Hits 84970 85032 +62
+ Misses 19815 19813 -2
- Partials 6484 6485 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
28148be to
ee1296a
Compare
Detect rate-limited Drive/Doc responses (SubtypeRateLimit, business code 99991400, or HTTP 429; excluding the 1063006 daily quota) and append exponential-backoff-with-jitter guidance to the typed error hint, so AI agents stop hammering and retry sensibly. Apply the recovery hint explicitly inside each drive shortcut's Execute via a defer, mirroring PR #2218's call-site style, instead of an implicit blanket decorator over the shortcut registry. Also cover the docs +resource-download cover path, which streams from the same Drive media download endpoint.
ee1296a to
aa5de22
Compare
Summary
When a Drive/Doc request is rate limited, append exponential-backoff-with-jitter guidance to the typed error's hint so AI agents stop retrying immediately and back off. Mirrors PR #2218's call-site style rather than an implicit blanket decorator.
Changes
SubtypeRateLimit, business code99991400, or HTTP429— excluding the1063006daily-quota code (recovery is next-day reset, not short backoff) — and append recovery guidance to the typed error hint (withDriveRateLimitRecoveryHint).Executevia a one-linedefer, replacing the implicit blanketdecorateDriveShortcutRateLimitloop over the shortcut registry. Covers all drive shortcuts on every return path while keeping the behavior visible in each shortcut's own code.docs +resource-download(cover) path, which streams from the same Drive media-download endpoint as+media-download, reusingwithDocMediaDownloadRecoveryHint.Test Plan
go test ./shortcuts/drive/... ./shortcuts/doc/...)go build ./...,gofmt -l,go vetcleanlark-cli drive/docsflows work as expectedRelated Issues
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation