feat: support source file preview artifacts - #2085
Conversation
|
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 preview supports direct ChangesDrive preview and download fallback
Markdown preview-download migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant DrivePreview
participant DriveAPI
participant OutputFile
CLI->>DrivePreview: request source_file preview
DrivePreview->>DriveAPI: GET preview_download with preview_type 16
DriveAPI-->>DrivePreview: source artifact
DrivePreview->>OutputFile: save artifact
DrivePreview-->>CLI: return download metadata
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2085 +/- ##
==========================================
+ Coverage 75.44% 75.48% +0.03%
==========================================
Files 924 928 +4
Lines 98297 98600 +303
==========================================
+ Hits 74163 74427 +264
- Misses 18494 18519 +25
- Partials 5640 5654 +14 ☔ 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@0f86082ffbfc368c9ed4abfa8d24b017940e8db0🧩 Skill updatenpx skills add larksuite/cli#feat/drive-preview-source-file -y -g |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/drive/drive_errors.go`:
- Around line 34-43: Add table-driven 403 test cases for the error-handling flow
around the existing drive +preview check: verify an existing drive +preview hint
is preserved unchanged, and verify a different non-empty hint receives the
generated download-forbidden preview hint appended to it. Use the existing test
helpers and expected error structure.
In `@shortcuts/drive/drive_io_test.go`:
- Around line 1565-1638: The download error tests in
shortcuts/drive/drive_io_test.go lines 1565-1638 must assert
errs.SubtypeNetworkTransport via errs.ProblemOf and verify the underlying
transport cause is preserved; update both
TestDriveDownloadHTTP403SuggestsPreview and
TestDriveDownloadHTTP404DoesNotSuggestPreview as applicable. The preview error
tests in shortcuts/drive/drive_preview_test.go lines 713-762 must assert the API
subtype and verify the underlying cause supplied by the classified API error
remains available.
In `@shortcuts/drive/drive_preview_common.go`:
- Around line 825-839: Add tests covering the error-hint handling around
errs.ProblemOf: non-API errors, hints already containing "--type source_file",
and empty or whitespace-only server hints. Verify each guard preserves or
appends the expected hint behavior without changing the existing implementation.
In `@shortcuts/drive/drive_preview_test.go`:
- Around line 503-530: Add direct assertions that requested_type equals
"source_file" in both source-file dry-run contracts:
shortcuts/drive/drive_preview_test.go lines 503-530 near the existing
selected_type checks, and tests/cli_e2e/drive/drive_preview_dryrun_test.go lines
118-139 near the response field assertions.
In `@tests/cli_e2e/drive/drive_preview_dryrun_test.go`:
- Around line 96-140: Add a live bot-credential E2E test alongside
TestDrivePreviewDryRun_SourceFile that creates or uploads a source file, invokes
drive +preview with type source_file, verifies the downloaded artifact’s
contents, and cleans up all created resources and temporary output. Keep the
flow self-contained and use the existing E2E helpers and cleanup conventions.
🪄 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: 6301d6f6-9799-4295-bc66-dfd5e8b2b373
📒 Files selected for processing (10)
shortcuts/drive/drive_download.goshortcuts/drive/drive_errors.goshortcuts/drive/drive_io_test.goshortcuts/drive/drive_preview.goshortcuts/drive/drive_preview_common.goshortcuts/drive/drive_preview_test.goskills/lark-drive/SKILL.mdskills/lark-drive/references/lark-drive-download.mdskills/lark-drive/references/lark-drive-preview.mdtests/cli_e2e/drive/drive_preview_dryrun_test.go
22d665a to
c4e0c8a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/cli_e2e/markdown/markdown_dryrun_test.go (1)
151-156: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert version parameters on each preview-download call.
The two broad version assertions only prove that both strings appear somewhere in the dry-run output; they do not prove that each version is attached to the correct
[1]or[2]API request. Decode the API steps and assert each request’sparams.versionalongsidepreview_type.🤖 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/markdown/markdown_dryrun_test.go` around lines 151 - 156, Update the dry-run assertions around the preview-download output in the markdown test to decode the API steps and inspect each request individually. Assert that both `[1]` and `[2]` requests contain preview_type "16" with their respective params.version values, rather than relying on broad output-wide version assertions.Source: Coding guidelines
🤖 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 `@skills/lark-drive/SKILL.md`:
- Around line 143-144: Update the +download shortcut row in the command
reference table to include the fallback destination argument, using `+preview
--type source_file --output <path>`, so users can save the preview output when
replacing a failed 403 download.
---
Nitpick comments:
In `@tests/cli_e2e/markdown/markdown_dryrun_test.go`:
- Around line 151-156: Update the dry-run assertions around the preview-download
output in the markdown test to decode the API steps and inspect each request
individually. Assert that both `[1]` and `[2]` requests contain preview_type
"16" with their respective params.version values, rather than relying on broad
output-wide version assertions.
🪄 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: 57c5f664-e901-467f-a206-fb22cbe7383f
📒 Files selected for processing (20)
shortcuts/drive/drive_download.goshortcuts/drive/drive_errors.goshortcuts/drive/drive_io_test.goshortcuts/drive/drive_preview.goshortcuts/drive/drive_preview_common.goshortcuts/drive/drive_preview_test.goshortcuts/markdown/helpers.goshortcuts/markdown/markdown_diff.goshortcuts/markdown/markdown_diff_test.goshortcuts/markdown/markdown_fetch.goshortcuts/markdown/markdown_patch.goshortcuts/markdown/markdown_patch_test.goshortcuts/markdown/markdown_test.goskills/lark-drive/SKILL.mdskills/lark-drive/references/lark-drive-download.mdskills/lark-drive/references/lark-drive-preview.mdskills/lark-markdown/SKILL.mdskills/lark-markdown/references/lark-markdown-fetch.mdtests/cli_e2e/drive/drive_preview_dryrun_test.gotests/cli_e2e/markdown/markdown_dryrun_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- shortcuts/drive/drive_download.go
- skills/lark-drive/references/lark-drive-download.md
- shortcuts/drive/drive_io_test.go
- tests/cli_e2e/drive/drive_preview_dryrun_test.go
- skills/lark-drive/references/lark-drive-preview.md
- shortcuts/drive/drive_preview_common.go
- shortcuts/drive/drive_errors.go
- shortcuts/drive/drive_preview.go
- shortcuts/drive/drive_preview_test.go
c4e0c8a to
d02f3d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/markdown/helpers.go`:
- Around line 249-266: Update markdownSourceFilePreviewQuery and
markdownSourceFilePreviewDryRunParams to validate the supplied version at the
input boundary, returning a typed validation error for whitespace-only values
instead of omitting the version. For valid non-blank versions, preserve and pass
the original version string unchanged in both request paths; adjust function
signatures as needed to propagate the validation error.
🪄 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: bddd4a07-bcae-4df5-abca-9092f1ae8bb1
📒 Files selected for processing (19)
shortcuts/drive/drive_download.goshortcuts/drive/drive_errors.goshortcuts/drive/drive_io_test.goshortcuts/drive/drive_preview.goshortcuts/drive/drive_preview_common.goshortcuts/drive/drive_preview_test.goshortcuts/markdown/helpers.goshortcuts/markdown/markdown_diff.goshortcuts/markdown/markdown_diff_test.goshortcuts/markdown/markdown_fetch.goshortcuts/markdown/markdown_patch.goshortcuts/markdown/markdown_patch_test.goshortcuts/markdown/markdown_test.goskills/lark-drive/SKILL.mdskills/lark-drive/references/lark-drive-download.mdskills/lark-drive/references/lark-drive-preview.mdtests/cli_e2e/drive/drive_preview_dryrun_test.gotests/cli_e2e/drive/drive_preview_workflow_test.gotests/cli_e2e/markdown/markdown_dryrun_test.go
🚧 Files skipped from review as they are similar to previous changes (15)
- shortcuts/drive/drive_download.go
- shortcuts/markdown/markdown_patch.go
- shortcuts/markdown/markdown_fetch.go
- skills/lark-drive/references/lark-drive-preview.md
- shortcuts/markdown/markdown_diff.go
- shortcuts/drive/drive_io_test.go
- tests/cli_e2e/drive/drive_preview_dryrun_test.go
- tests/cli_e2e/markdown/markdown_dryrun_test.go
- shortcuts/drive/drive_errors.go
- shortcuts/markdown/markdown_test.go
- shortcuts/markdown/markdown_patch_test.go
- shortcuts/markdown/markdown_diff_test.go
- shortcuts/drive/drive_preview_common.go
- shortcuts/drive/drive_preview.go
- shortcuts/drive/drive_preview_test.go
d02f3d4 to
41aded2
Compare
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/markdown/markdown_patch_test.go (1)
480-484: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert the typed validation contract on this error path.
This test only matches text. Assert category/subtype with
errs.ProblemOf, readParamviaerrors.Ason the validation error, and verify the expected cause state. As per coding guidelines, error-path tests must assert typed metadata and cause preservation rather than relying only on message substrings. Based on learnings,errs.ProblemOfdoes not exposeParam; obtain it from*errs.ValidationError.🤖 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/markdown/markdown_patch_test.go` around lines 480 - 484, Update TestMarkdownPatchRejectsEmptyPatchedContent to assert the typed validation contract instead of only matching error text: use errs.ProblemOf to verify the expected category and subtype, extract Param from the resulting *errs.ValidationError with errors.As, and verify the expected preserved cause state.Sources: Coding guidelines, Learnings
🤖 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/markdown/markdown_patch_test.go`:
- Around line 480-484: Update TestMarkdownPatchRejectsEmptyPatchedContent to
assert the typed validation contract instead of only matching error text: use
errs.ProblemOf to verify the expected category and subtype, extract Param from
the resulting *errs.ValidationError with errors.As, and verify the expected
preserved cause state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6bb9d205-c341-4d07-b21f-6a17565ca473
📒 Files selected for processing (19)
shortcuts/drive/drive_download.goshortcuts/drive/drive_errors.goshortcuts/drive/drive_io_test.goshortcuts/drive/drive_preview.goshortcuts/drive/drive_preview_common.goshortcuts/drive/drive_preview_test.goshortcuts/markdown/helpers.goshortcuts/markdown/markdown_diff.goshortcuts/markdown/markdown_diff_test.goshortcuts/markdown/markdown_fetch.goshortcuts/markdown/markdown_patch.goshortcuts/markdown/markdown_patch_test.goshortcuts/markdown/markdown_test.goskills/lark-drive/SKILL.mdskills/lark-drive/references/lark-drive-download.mdskills/lark-drive/references/lark-drive-preview.mdtests/cli_e2e/drive/drive_preview_dryrun_test.gotests/cli_e2e/drive/drive_preview_workflow_test.gotests/cli_e2e/markdown/markdown_dryrun_test.go
🚧 Files skipped from review as they are similar to previous changes (14)
- skills/lark-drive/references/lark-drive-download.md
- shortcuts/drive/drive_download.go
- tests/cli_e2e/drive/drive_preview_workflow_test.go
- skills/lark-drive/references/lark-drive-preview.md
- shortcuts/drive/drive_io_test.go
- shortcuts/markdown/markdown_test.go
- shortcuts/markdown/helpers.go
- shortcuts/drive/drive_preview.go
- tests/cli_e2e/markdown/markdown_dryrun_test.go
- shortcuts/drive/drive_errors.go
- tests/cli_e2e/drive/drive_preview_dryrun_test.go
- shortcuts/markdown/markdown_fetch.go
- shortcuts/markdown/markdown_diff_test.go
- shortcuts/drive/drive_preview_test.go
41aded2 to
2593c7f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/drive/drive_preview_test.go`:
- Around line 155-163: Update the httpmock.Stub registration for the preview
download endpoint to use RawBody instead of Body, preserving the literal
markdown artifact bytes so the preview-reading logic receives "# markdown\n"
directly.
🪄 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: 21944183-5df4-4c51-8644-6ff52272eeeb
📒 Files selected for processing (19)
shortcuts/drive/drive_download.goshortcuts/drive/drive_errors.goshortcuts/drive/drive_io_test.goshortcuts/drive/drive_preview.goshortcuts/drive/drive_preview_common.goshortcuts/drive/drive_preview_test.goshortcuts/markdown/helpers.goshortcuts/markdown/markdown_diff.goshortcuts/markdown/markdown_diff_test.goshortcuts/markdown/markdown_fetch.goshortcuts/markdown/markdown_patch.goshortcuts/markdown/markdown_patch_test.goshortcuts/markdown/markdown_test.goskills/lark-drive/SKILL.mdskills/lark-drive/references/lark-drive-download.mdskills/lark-drive/references/lark-drive-preview.mdtests/cli_e2e/drive/drive_preview_dryrun_test.gotests/cli_e2e/drive/drive_preview_workflow_test.gotests/cli_e2e/markdown/markdown_dryrun_test.go
🚧 Files skipped from review as they are similar to previous changes (13)
- shortcuts/drive/drive_download.go
- skills/lark-drive/references/lark-drive-download.md
- skills/lark-drive/references/lark-drive-preview.md
- tests/cli_e2e/drive/drive_preview_dryrun_test.go
- shortcuts/markdown/markdown_patch.go
- shortcuts/drive/drive_errors.go
- tests/cli_e2e/markdown/markdown_dryrun_test.go
- shortcuts/markdown/markdown_test.go
- shortcuts/markdown/helpers.go
- shortcuts/markdown/markdown_diff.go
- shortcuts/markdown/markdown_diff_test.go
- shortcuts/markdown/markdown_patch_test.go
- shortcuts/drive/drive_preview.go
da2cb49 to
dd9079e
Compare
d237150 to
81f2a19
Compare
81f2a19 to
0f86082
Compare
Summary
Support source file preview artifacts as a first-class content viewing path for Drive files and native Markdown file reads.
Changes
drive +preview --type source_fileas a direct source file preview artifact mode that skips preview candidate lookup.preview_resultdiagnostics fordrive +preview --list-onlyfailures while appending an actionablesource_filerecovery hint for non-retryable API errors.drive +download, suggestingdrive +preview --type source_file --output <path>when direct download is blocked.markdown +fetch,+patch, and+diff) to use the same source file preview artifact path withpreview_type=16.Test Plan
go test ./shortcuts/drive -run 'TestDrivePreview(DryRunSourceFileDocumentsDirectDownload|ListOnlyErrorAddsSourceFileHint|ListOnlyRateLimitKeepsOriginalHint|SourceFileHintGuards)|TestDriveDownloadHTTP(403SuggestsPreview|404DoesNotSuggestPreview)' -count=1go test ./shortcuts/markdown -run 'TestMarkdown(Fetch|Patch|Diff)' -count=1go build -o tmp/lark-cli-source-file-preview-review .LARK_CLI_BIN=/Users/bytedance/go/src/github.com/larksuite/cli/tmp/lark-cli-source-file-preview-review go test ./tests/cli_e2e/drive -run 'TestDrivePreviewDryRun_(SourceFile|Download|ListOnly)' -count=1LARK_CLI_BIN=/Users/bytedance/go/src/github.com/larksuite/cli/tmp/lark-cli-source-file-preview-review go test ./tests/cli_e2e/markdown -run 'TestMarkdown(DiffDryRun|FetchDryRun|PatchDryRun)_' -count=1LARK_CLI_BIN=/Users/bytedance/go/src/github.com/larksuite/cli/tmp/lark-cli-source-file-preview-review go test ./tests/cli_e2e/drive -run 'TestDrive_PreviewAndCoverWorkflow/source_file_download' -count=1 -vgit diff --check origin/main...HEADRelated Issues
Summary by CodeRabbit
--type source_filesupport fordrive +preview, including direct source-file downloads andsource_file-specific dry-run/execute behavior.drive +previewsource-file recovery flow.drive +preview --list-onlyhinting for eligible API errors; rate-limit errors keep original hint behavior.drive +previewanddrive +downloaddocs to describe source-file viewing/recovery.