feat: report upload file events - #2093
Conversation
github-actions
Bot
added
domain/ccm
|
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:
📝 WalkthroughWalkthroughDrive uploads now emit structured file-event reports for success and failure paths. Shared helpers enforce one report per runtime, preserve upload errors, add validated tenant-capacity hints, and represent reports in dry-run output. Drive upload, import, typed media, unit, and end-to-end tests were updated. ChangesDrive upload event reporting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant DriveUpload
participant DriveAPI
participant FileEventReport
CLI->>DriveUpload: execute upload
DriveUpload->>DriveAPI: upload_all or multipart request
DriveAPI-->>DriveUpload: upload result or typed error
DriveUpload->>FileEventReport: POST success or error event
FileEventReport-->>DriveUpload: report response and optional capacity URL
DriveUpload-->>CLI: file token or original upload error
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 |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@109095ce7414e13943d5331502582b89ea816aaf🧩 Skill updatenpx skills add larksuite/cli#feat/upload-file-event-report -y -g |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2093 +/- ##
==========================================
+ Coverage 76.34% 76.40% +0.05%
==========================================
Files 991 993 +2
Lines 106019 106229 +210
==========================================
+ Hits 80940 81163 +223
+ Misses 18944 18899 -45
- Partials 6135 6167 +32 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
tests/cli_e2e/drive/drive_import_workflow_test.go (1)
70-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePut
ctxfirst in the helper signature.
waitDriveImportReady(t *testing.T, ctx context.Context, ...)inverts the idiomatic ordering (ctxfirst, or immediately aftertper repo precedent);revive/golangci-lintcontext-as-argument checks commonly flag this.🤖 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/drive/drive_import_workflow_test.go` at line 70, Update the waitDriveImportReady helper signature to place ctx immediately after t, before ticket and fallbackType, and adjust every call site to pass arguments in the matching order.shortcuts/drive/drive_io_test.go (2)
1738-1747: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlso assert
Category/Subtypeon these error paths.Per repo test guidelines, error-path assertions should pin typed metadata (category and subtype) rather than only
Code/Hint, so a regression in classification still fails the test.As per coding guidelines: "Error-path tests must assert typed metadata through
errs.ProblemOf(category,subtype, andparam) and verify cause preservation rather than relying only on message substrings."Also applies to: 1795-1801, 1843-1849
🤖 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_io_test.go` around lines 1738 - 1747, Extend the error-path assertions around errs.ProblemOf in the affected tests to verify the expected Category, Subtype, and param metadata, and assert that the original cause is preserved. Apply the same typed-metadata and cause checks to the analogous cases near the existing assertions at lines 1795-1801 and 1843-1849, while retaining the current Code and Hint checks.Source: Coding guidelines
1771-1776: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the existing
withDriveWorkingDirhelper instead of manualChdir.The other tests in this file use
withDriveWorkingDir(t, t.TempDir()); here theos.Getwderror is dropped anddefer os.Chdir(origDir)is unchecked (likely anerrcheckhit under the pinned golangci-lint config).♻️ Proposed refactor
- origDir, _ := os.Getwd() - tmpDir := t.TempDir() - if err := os.Chdir(tmpDir); err != nil { - t.Fatalf("Chdir() error: %v", err) - } - defer os.Chdir(origDir) - - fh, err := os.Create("large.bin") + withDriveWorkingDir(t, t.TempDir()) + + fh, err := os.Create("large.bin")🤖 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_io_test.go` around lines 1771 - 1776, Replace the manual working-directory setup around this test with the existing withDriveWorkingDir helper, passing t and t.TempDir(). Remove the os.Getwd, os.Chdir, and deferred restoration logic while preserving the test’s execution in the temporary drive directory.shortcuts/common/lark_cli_file_event_test.go (1)
150-175: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winError-path test only checks identity, not typed metadata.
TestReportUploadFileEventOnError_ReportsAndPreservesErroronly assertsreturned != uploadErr(reference identity) andtags["code"]; it never assertserrs.ProblemOfCategory/Subtypefor the returned error, as required for error-path tests.As per coding guidelines: "Error-path tests must assert typed metadata through
errs.ProblemOf(category,subtype, andparam) and verify cause preservation rather than relying only on message substrings."🤖 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/common/lark_cli_file_event_test.go` around lines 150 - 175, Strengthen TestReportUploadFileEventOnError_ReportsAndPreservesError by applying errs.ProblemOf to the returned error and asserting the expected category, subtype, and param metadata, while also verifying the original cause is preserved. Retain the existing identity, reporting-count, and tag assertions.Source: Coding guidelines
shortcuts/common/drive_media_upload_typed_test.go (2)
1-1: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNew error-path tests don't fully assert typed error metadata. Several new tests for the upload-reporting feature assert
p.Code/p.Hint(or bare error identity) viaerrs.ProblemOfbut omitCategory/Subtypeassertions, as required for error-path tests.
shortcuts/common/drive_media_upload_typed_test.go#L399-430: inTestUploadDriveMediaAllTypedReportsFileEventOnError, add assertions onp.Category/p.Subtypealongside the existingp.Codecheck.shortcuts/common/drive_media_upload_typed_test.go#L432-471: inTestUploadDriveMediaAllTypedReportFailureKeepsUploadError, addp.Category/p.Subtypeassertions alongsidep.Code/p.Hint.shortcuts/common/lark_cli_file_event_test.go#L150-175: inTestReportUploadFileEventOnError_ReportsAndPreservesError, asserterrs.ProblemOf(returned)Category/Subtypein addition to the existing identity check.As per coding guidelines: "Error-path tests must assert typed metadata through
errs.ProblemOf(category,subtype, andparam) and verify cause preservation rather than relying only on message substrings."🤖 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/common/drive_media_upload_typed_test.go` at line 1, Update the error-path tests TestUploadDriveMediaAllTypedReportsFileEventOnError, TestUploadDriveMediaAllTypedReportFailureKeepsUploadError, and TestReportUploadFileEventOnError_ReportsAndPreservesError to assert errs.ProblemOf metadata for Category and Subtype alongside the existing checks; preserve the current cause or error-identity assertions and add the required param assertion where applicable.Source: Coding guidelines
399-430: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winError-path tests assert
Codebut omitCategory/Subtype.
TestUploadDriveMediaAllTypedReportsFileEventOnErrorandTestUploadDriveMediaAllTypedReportFailureKeepsUploadError(432-471) assertp.Code/p.Hintviaerrs.ProblemOfbut don't assertCategory/Subtype, as required for error-path tests exercising typed errors.As per coding guidelines: "Error-path tests must assert typed metadata through
errs.ProblemOf(category,subtype, andparam) and verify cause preservation rather than relying only on message substrings." This shares the same root cause as the gap flagged inshortcuts/common/lark_cli_file_event_test.go; see the consolidated comment.🤖 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/common/drive_media_upload_typed_test.go` around lines 399 - 430, Update TestUploadDriveMediaAllTypedReportsFileEventOnError and TestUploadDriveMediaAllTypedReportFailureKeepsUploadError to fully validate errs.ProblemOf metadata, asserting the expected category, subtype, and param alongside the existing code or hint checks. Also verify that the typed error preserves its underlying cause, rather than relying only on message content.Source: Coding guidelines
shortcuts/common/lark_cli_file_event.go (1)
63-138: 🧹 Nitpick | 🔵 TrivialBest-effort report blocks the command for up to 3s on every upload.
ReportUploadFileEvent/ReportUploadFileEventOnErrorcallpostUploadFileEventsynchronously before the upload function returns, bounded by a 3s timeout (uploadFileEventReportTimeout). On a slow/lossy network this adds up to 3 extra seconds of perceived latency to every single Drive upload, on both success and failure paths, even though the report result is discarded. This is likely an intentional tradeoff (CLI process exit would otherwise race a fire-and-forget goroutine), but worth confirming the 3s bound is acceptable UX for interactive use, and possibly worth lowering further for the common success 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/common/lark_cli_file_event.go` around lines 63 - 138, Make the upload event reporting path non-blocking so ReportUploadFileEvent and ReportUploadFileEventOnError do not add the full uploadFileEventReportTimeout to interactive uploads, while preserving best-effort behavior, once-per-RuntimeContext deduplication, and the original upload error. Ensure any asynchronous report has sufficient runtime/context lifetime to complete before process exit, or reduce the reporting timeout—especially for the successful upload path—if the existing synchronous lifecycle must be retained.
🤖 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 `@tests/cli_e2e/drive/drive_import_workflow_test.go`:
- Around line 18-27: Add clie2e.SkipWithoutTenantAccessToken(t) as the first
operation in TestDrive_ImportWorkflow, before creating the context, fixture, or
invoking any live Drive commands, so the test skips when tenant credentials are
unavailable.
---
Nitpick comments:
In `@shortcuts/common/drive_media_upload_typed_test.go`:
- Line 1: Update the error-path tests
TestUploadDriveMediaAllTypedReportsFileEventOnError,
TestUploadDriveMediaAllTypedReportFailureKeepsUploadError, and
TestReportUploadFileEventOnError_ReportsAndPreservesError to assert
errs.ProblemOf metadata for Category and Subtype alongside the existing checks;
preserve the current cause or error-identity assertions and add the required
param assertion where applicable.
- Around line 399-430: Update
TestUploadDriveMediaAllTypedReportsFileEventOnError and
TestUploadDriveMediaAllTypedReportFailureKeepsUploadError to fully validate
errs.ProblemOf metadata, asserting the expected category, subtype, and param
alongside the existing code or hint checks. Also verify that the typed error
preserves its underlying cause, rather than relying only on message content.
In `@shortcuts/common/lark_cli_file_event_test.go`:
- Around line 150-175: Strengthen
TestReportUploadFileEventOnError_ReportsAndPreservesError by applying
errs.ProblemOf to the returned error and asserting the expected category,
subtype, and param metadata, while also verifying the original cause is
preserved. Retain the existing identity, reporting-count, and tag assertions.
In `@shortcuts/common/lark_cli_file_event.go`:
- Around line 63-138: Make the upload event reporting path non-blocking so
ReportUploadFileEvent and ReportUploadFileEventOnError do not add the full
uploadFileEventReportTimeout to interactive uploads, while preserving
best-effort behavior, once-per-RuntimeContext deduplication, and the original
upload error. Ensure any asynchronous report has sufficient runtime/context
lifetime to complete before process exit, or reduce the reporting
timeout—especially for the successful upload path—if the existing synchronous
lifecycle must be retained.
In `@shortcuts/drive/drive_io_test.go`:
- Around line 1738-1747: Extend the error-path assertions around errs.ProblemOf
in the affected tests to verify the expected Category, Subtype, and param
metadata, and assert that the original cause is preserved. Apply the same
typed-metadata and cause checks to the analogous cases near the existing
assertions at lines 1795-1801 and 1843-1849, while retaining the current Code
and Hint checks.
- Around line 1771-1776: Replace the manual working-directory setup around this
test with the existing withDriveWorkingDir helper, passing t and t.TempDir().
Remove the os.Getwd, os.Chdir, and deferred restoration logic while preserving
the test’s execution in the temporary drive directory.
In `@tests/cli_e2e/drive/drive_import_workflow_test.go`:
- Line 70: Update the waitDriveImportReady helper signature to place ctx
immediately after t, before ticket and fallbackType, and adjust every call site
to pass arguments in the matching order.
🪄 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 Plus
Run ID: 3d3dfa38-e600-4491-b734-7189eb854062
📒 Files selected for processing (16)
shortcuts/common/call_api_typed_test.goshortcuts/common/drive_media_upload.goshortcuts/common/drive_media_upload_typed_test.goshortcuts/common/lark_cli_file_event.goshortcuts/common/lark_cli_file_event_test.goshortcuts/common/runner.goshortcuts/drive/drive_import.goshortcuts/drive/drive_import_test.goshortcuts/drive/drive_io_test.goshortcuts/drive/drive_upload.gotests/cli_e2e/drive/coverage.mdtests/cli_e2e/drive/drive_import_dryrun_test.gotests/cli_e2e/drive/drive_import_workflow_test.gotests/cli_e2e/drive/drive_upload_dryrun_test.gotests/cli_e2e/drive/drive_upload_workflow_test.gotests/cli_e2e/sheets/sheets_workbook_import_dryrun_test.go
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
33626c5 to
d38de40
Compare
github-actions
Bot
added
domain/base
1 participant
Summary
Report best-effort upload file events from Drive file/media upload flows, including Drive import and Sheets workbook import. Preserve the original upload result while enriching tenant-capacity errors with a server-provided expansion URL when available; all reports use
file_scene: lark-cli.Changes
Test Plan
go test ./shortcuts/common ./shortcuts/drive -count=1Local
make unit-teston macOS can hit an unrelatedinternal/qualitygate/publiccontentTempDir cleanup race (.git/objectsor.git/airemains non-empty). The affected packages pass, and the latest base commit passes the complete Linux CI workflow.Related Issues
Summary by CodeRabbit