feat: support file-token overwrite and version output for drive +upload - #885
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 (7)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds overwrite support for Drive uploads via ChangesDrive file overwrite with token
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 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 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.
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)
359-375:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMultipart finish path drops
versionfrom output.
uploadFileMultipartonly extractsfile_tokenfrom the finish response and returnsdriveUploadResult{FileToken: fileToken}without populatingVersion. The small-file path (uploadFileToDrive) usesdriveUploadVersionFromData(data)to surface eitherversionordata_version, so the same overwrite call produces aversionfield for files ≤ 20MB but never for files > 20MB. Per the PR description, returningdata.versionshould be uniform across both upload paths.🛠️ Proposed fix
fileToken := common.GetString(finishResult, "file_token") if fileToken == "" { return driveUploadResult{}, output.Errorf(output.ExitAPI, "api_error", "upload_finish succeeded but no file_token returned") } - return driveUploadResult{FileToken: fileToken}, nil + return driveUploadResult{ + FileToken: fileToken, + Version: driveUploadVersionFromData(finishResult), + }, nil }
runtime.CallAPIalready returns the unwrappeddatamap (the same shape used to readfile_token/upload_id), sodriveUploadVersionFromDatacan be reused as-is. Consider extendingTestDriveUploadLargeFileOverwriteUsesMultipart(or adding a sibling test) to assertdata["version"]for the multipart 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 359 - 375, The multipart finish path in uploadFileMultipart only returns FileToken and omits Version; call driveUploadVersionFromData(finishResult) after runtime.CallAPI returns and populate the Version field on the returned driveUploadResult (i.e., return driveUploadResult{FileToken: fileToken, Version: <value_from_driveUploadVersionFromData>}), reusing the existing driveUploadVersionFromData helper so multipart uploads surface the same data.version/data_version as small-file uploads; also add/extend TestDriveUploadLargeFileOverwriteUsesMultipart to assert data["version"] is present for the multipart path.
🤖 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 `@shortcuts/drive/drive_upload.go`:
- Around line 359-375: The multipart finish path in uploadFileMultipart only
returns FileToken and omits Version; call
driveUploadVersionFromData(finishResult) after runtime.CallAPI returns and
populate the Version field on the returned driveUploadResult (i.e., return
driveUploadResult{FileToken: fileToken, Version:
<value_from_driveUploadVersionFromData>}), reusing the existing
driveUploadVersionFromData helper so multipart uploads surface the same
data.version/data_version as small-file uploads; also add/extend
TestDriveUploadLargeFileOverwriteUsesMultipart to assert data["version"] is
present for the multipart path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 200c6cb5-d8f4-48ab-ad73-1db1bee7c388
📒 Files selected for processing (6)
shortcuts/drive/drive_io_test.goshortcuts/drive/drive_upload.goskills/lark-drive/references/lark-drive-upload.mdtests/cli_e2e/drive/coverage.mdtests/cli_e2e/drive/drive_upload_dryrun_test.gotests/cli_e2e/drive/drive_upload_workflow_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #885 +/- ##
==========================================
- Coverage 65.89% 65.89% -0.01%
==========================================
Files 518 520 +2
Lines 48945 49220 +275
==========================================
+ Hits 32254 32435 +181
- Misses 13921 14015 +94
Partials 2770 2770 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@64d49e95fe70f63e8ebef5b79f2bc1cfe2ece243🧩 Skill updatenpx skills add larksuite/cli#feat/drive-upload-file-token -y -g |
6c80d43 to
623edd8
Compare
Change-Id: I76c334578fc2fa5cfd2eedb4525b0d9d735f610e
623edd8 to
64d49e9
Compare
…ad (larksuite#885) Change-Id: I76c334578fc2fa5cfd2eedb4525b0d9d735f610e
Summary
This PR adds in-place overwrite support to drive +upload via an optional --file-token flag and surfaces upload version in the shortcut output when the backend returns it. It also adds coverage and updates the lark-drive skill docs so the new behavior is documented for agents and users.
Changes
Test Plan
Related Issues
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests