feat: add docs media-preview shortcut#334
Merged
fangshuyu-768 merged 1 commit intomainfrom Apr 8, 2026
Merged
Conversation
github-actions
bot
added
domain/ccm
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as "docs +media-preview"
participant API as "Drive API"
participant FS as "File System"
User->>CLI: run --token T --output /path/out
CLI->>CLI: validate token and initial output path
CLI->>API: GET /open-apis/drive/v1/medias/:token/preview_download?preview_type=16
API-->>CLI: stream response body + Content-Type
CLI->>CLI: if output lacks ext -> determine ext from Content-Type
CLI->>CLI: re-validate final output path
CLI->>FS: ensure writable / enforce overwrite rules
CLI->>FS: create parent directories
CLI->>FS: write file atomically (stream -> disk)
FS-->>CLI: return written byte count
CLI-->>User: output saved_path, size_bytes, content_type
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR adds a The implementation closely mirrors Confidence Score: 5/5Safe to merge; the only new finding is a P2 naming-convention nit on the exported constant. All remaining findings are P2 (style/cleanup). The three substantive issues (duplicate MIME map, magic number, post-API overwrite check) were raised in prior review threads and are pre-existing patterns; the only novel finding is the non-idiomatic shortcuts/doc/doc_media_preview.go — constant naming and the inherited post-API overwrite-check ordering noted in prior threads.
|
| Filename | Overview |
|---|---|
| shortcuts/doc/doc_media_preview.go | New shortcut implementing media preview via /preview_download; logic mirrors doc_media_download.go closely — same duplicate MIME map, same post-API overwrite check, same atomic-write path. One non-idiomatic exported constant name (PreviewType_SOURCE_FILE). |
| shortcuts/doc/doc_media_test.go | Adds three new tests for DocMediaPreview covering dry-run URL, overwrite rejection, and HTTP error handling — good parity with the download tests. |
| shortcuts/doc/shortcuts.go | DocMediaPreview registered in the Shortcuts() slice between DocMediaInsert and DocMediaDownload — straightforward and correct. |
| shortcuts/register_test.go | Adds TestRegisterShortcutsMountsDocsMediaPreview to confirm the new shortcut is wired into the command tree — clean and minimal. |
| skills/lark-doc/SKILL.md | Quick-decision table updated to prefer +media-preview for view-only requests and route explicit downloads / whiteboard thumbnails to +media-download. |
| skills/lark-doc/references/lark-doc-media-preview.md | New reference doc for +media-preview; selection rules, parameters, and token-source guidance are clear and consistent with the implementation. |
| skills/lark-doc/references/lark-doc-media-download.md | Updated to cross-reference +media-preview and add a fallback troubleshooting tip for HTTP 403 errors. |
| skills/lark-doc/references/lark-doc-fetch.md | Extraction steps updated to prefer +media-preview for view-only use cases and +media-download for explicit downloads and whiteboard tokens. |
Sequence Diagram
sequenceDiagram
participant CLI as lark-cli
participant Val as validate pkg
participant API as Lark API
participant FS as filesystem
CLI->>Val: SafeOutputPath(outputPath) [early check]
Val-->>CLI: ok / error
CLI->>API: GET /drive/v1/medias/:token/preview_download?preview_type=16
API-->>CLI: 200 + Body + Content-Type header
CLI->>CLI: auto-append ext if outputPath has no extension
CLI->>Val: SafeOutputPath(finalPath) [final check]
Val-->>CLI: ok / error
CLI->>CLI: EnsureWritableFile(safePath, overwrite)
CLI->>FS: MkdirAll(parent dir)
CLI->>FS: AtomicWriteFromReader(safePath, body, 0600)
FS-->>CLI: sizeBytes
CLI-->>CLI: Out {saved_path, size_bytes, content_type}
Reviews (3): Last reviewed commit: "feat: add docs media-preview shortcut" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@367c195322cb467d0ce6f08326ad34fc3a5498bb🧩 Skill updatenpx skills add larksuite/cli#feat/docs-media-preview-shortcut -y -g |
![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 (1)
shortcuts/doc/doc_media_preview.go (1)
83-92: Consider handling unknown MIME types more gracefully.When the user provides an output path without extension and the server returns an unrecognized
Content-Type, the file will be saved without an extension. While this works, it may confuse users.For consistency with
DocMediaDownload(which defaults to.pngfor whiteboards), consider logging a warning to stderr or defaulting to a generic extension like.binwhen the MIME type isn't recognized.💡 Optional: Add fallback handling
if ext, ok := previewMimeToExt[mimeType]; ok { finalPath = outputPath + ext + } else if mimeType != "" { + fmt.Fprintf(runtime.IO().ErrOut, "Warning: unknown Content-Type %q, saving without extension\n", mimeType) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/doc_media_preview.go` around lines 83 - 92, The code currently leaves finalPath without an extension when currentExt is empty and resp.Header.Get("Content-Type") yields an unknown MIME, which can confuse users; update the block around finalPath, currentExt, and previewMimeToExt so that if mimeType isn't found you either append a safe default extension (e.g., ".bin") or emit a warning to stderr (use log.Printf or fmt.Fprintln(os.Stderr,...)) before returning finalPath; mirror DocMediaDownload's behavior for whiteboards (defaulting to .png) as guidance and ensure previewMimeToExt lookup remains unchanged so known types still map correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/lark-doc/references/lark-doc-media-preview.md`:
- Around line 24-29: The parameters table is missing documentation for the
implemented --overwrite flag; update
skills/lark-doc/references/lark-doc-media-preview.md to add a new row for
`--overwrite` (optional) describing that it is a boolean flag which, when
present, allows existing local files to be overwritten when saving previews
(refer to the flag in doc_media_preview.go to ensure the description matches its
behavior), and include a short example or note if it changes default behavior
(e.g., "optional; if set, overwrite existing files").
---
Nitpick comments:
In `@shortcuts/doc/doc_media_preview.go`:
- Around line 83-92: The code currently leaves finalPath without an extension
when currentExt is empty and resp.Header.Get("Content-Type") yields an unknown
MIME, which can confuse users; update the block around finalPath, currentExt,
and previewMimeToExt so that if mimeType isn't found you either append a safe
default extension (e.g., ".bin") or emit a warning to stderr (use log.Printf or
fmt.Fprintln(os.Stderr,...)) before returning finalPath; mirror
DocMediaDownload's behavior for whiteboards (defaulting to .png) as guidance and
ensure previewMimeToExt lookup remains unchanged so known types still map
correctly.
🪄 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: cdd36f65-457b-4d2f-a2f1-310f70b09392
📒 Files selected for processing (8)
shortcuts/doc/doc_media_preview.goshortcuts/doc/doc_media_test.goshortcuts/doc/shortcuts.goshortcuts/register_test.goskills/lark-doc/SKILL.mdskills/lark-doc/references/lark-doc-fetch.mdskills/lark-doc/references/lark-doc-media-download.mdskills/lark-doc/references/lark-doc-media-preview.md
wittam-01
force-pushed
the
feat/docs-media-preview-shortcut
branch
from
6f38120 to
1f693da
Compare
|
Tip: Greploops — Automatically fix all review issues by running Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
shortcuts/doc/doc_media_preview.go (1)
85-108: Consider extracting the stream-to-file flow into a shared helper.This block now mirrors
shortcuts/doc/doc_media_download.go:94-123almost verbatim. Pulling the extension resolution, final-path validation, overwrite guard, and atomic write into one helper would keep+media-previewand+media-downloadfrom drifting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/doc_media_preview.go` around lines 85 - 108, The streaming-to-file logic is duplicated; extract it into a shared helper (e.g., SaveStreamToValidatedFile or writeStreamToSafeFile) that accepts outputPath, resp.Header (or contentType string), body io.Reader, overwrite bool, and the previewMimeToExt map and returns (sizeBytes int64, err error); inside the helper perform the ext resolution using previewMimeToExt and contentType, call validate.SafeOutputPath, common.EnsureWritableFile, vfs.MkdirAll for the parent dir, and validate.AtomicWriteFromReader; then replace the mirrored blocks in the functions that currently use previewMimeToExt, validate.SafeOutputPath, common.EnsureWritableFile, vfs.MkdirAll and validate.AtomicWriteFromReader to call the new helper and propagate its returned sizeBytes and error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/doc/doc_media_preview.go`:
- Around line 63-66: The current early validation relies solely on
validate.SafeOutputPath which allows empty or directory-like values; add a guard
before that check to reject blank or directory targets by returning
output.ErrValidation (e.g., when outputPath == "" or outputPath ends with a path
separator or otherwise has no file basename) so we fail fast instead of letting
extension inference create hidden files; apply the same guard in the second
validation block referenced (the block around lines 85-96) and keep using
output.ErrValidation with a clear message like "invalid output path: %s"
referencing outputPath and the underlying error.
- Around line 80-81: The current preview error handling double-wraps stream
errors by always calling output.ErrNetwork; modify the error return in the
preview handler (the block handling the result of DoAPIStream/DoStream in
doc_media_preview.go) to first check whether the error is already an
output.ExitError using errors.As(err, &exitErr) and if so return the original
exitErr (or err) directly; only call output.ErrNetwork("preview failed: %v",
err) for non-ExitError errors—follow the pattern used in drive_upload.go and
drive_media_upload.go.
---
Nitpick comments:
In `@shortcuts/doc/doc_media_preview.go`:
- Around line 85-108: The streaming-to-file logic is duplicated; extract it into
a shared helper (e.g., SaveStreamToValidatedFile or writeStreamToSafeFile) that
accepts outputPath, resp.Header (or contentType string), body io.Reader,
overwrite bool, and the previewMimeToExt map and returns (sizeBytes int64, err
error); inside the helper perform the ext resolution using previewMimeToExt and
contentType, call validate.SafeOutputPath, common.EnsureWritableFile,
vfs.MkdirAll for the parent dir, and validate.AtomicWriteFromReader; then
replace the mirrored blocks in the functions that currently use
previewMimeToExt, validate.SafeOutputPath, common.EnsureWritableFile,
vfs.MkdirAll and validate.AtomicWriteFromReader to call the new helper and
propagate its returned sizeBytes and 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
Run ID: c64dd4a5-1062-4e9e-a207-75640ad72e83
📒 Files selected for processing (8)
shortcuts/doc/doc_media_preview.goshortcuts/doc/doc_media_test.goshortcuts/doc/shortcuts.goshortcuts/register_test.goskills/lark-doc/SKILL.mdskills/lark-doc/references/lark-doc-fetch.mdskills/lark-doc/references/lark-doc-media-download.mdskills/lark-doc/references/lark-doc-media-preview.md
✅ Files skipped from review due to trivial changes (4)
- shortcuts/doc/shortcuts.go
- skills/lark-doc/references/lark-doc-media-download.md
- shortcuts/doc/doc_media_test.go
- skills/lark-doc/references/lark-doc-media-preview.md
🚧 Files skipped from review as they are similar to previous changes (3)
- shortcuts/register_test.go
- skills/lark-doc/SKILL.md
- skills/lark-doc/references/lark-doc-fetch.md
Change-Id: I5db9e52008e175f975838c8a9c03254afa30f52b
wittam-01
force-pushed
the
feat/docs-media-preview-shortcut
branch
from
1f693da to
367c195
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
shortcuts/doc/doc_media_preview.go (2)
80-82:⚠️ Potential issue | 🟡 MinorAvoid re-wrapping stream errors that are already
ExitError.Line 81 wraps all stream failures with
output.ErrNetwork. IfDoAPIStreamalready returns anExitError, this loses the original classification details and creates nested messaging. Return existingExitErroras-is; wrap only non-ExitErrorfailures.Suggested patch
import ( "context" + "errors" "fmt" @@ if err != nil { - return output.ErrNetwork("preview failed: %v", err) + var exitErr *output.Error + if errors.As(err, &exitErr) { + return err + } + return output.ErrNetwork("preview failed: %v", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/doc_media_preview.go` around lines 80 - 82, The error handling after DoAPIStream currently wraps all errors with output.ErrNetwork which hides existing ExitError details; change it to detect if err is an ExitError (using errors.As or a type assertion for the ExitError type from the output package) and if so return that original ExitError directly, otherwise wrap the non-ExitError with output.ErrNetwork; look for the DoAPIStream call and the current "if err != nil { return output.ErrNetwork(...)" branch in doc_media_preview.go and update it to first check/return the ExitError, then wrap others.
63-66:⚠️ Potential issue | 🟡 MinorReject blank or directory-like
--outputvalues before/after extension inference.Line 64 and Line 96 rely on
validate.SafeOutputPath, but edge cases like empty or separator-terminated paths can still resolve into hidden files after extension inference (for example.pngorpreview/.png). Add an explicit “must be file path” guard before API call and afterfinalPathresolution.Suggested patch
import ( "context" + "errors" "fmt" "net/http" "path/filepath" "strings" @@ Execute: func(ctx context.Context, runtime *common.RuntimeContext) error { @@ if err := validate.ResourceName(token, "--token"); err != nil { return output.ErrValidation("%s", err) } + if strings.TrimSpace(outputPath) == "" { + return output.ErrValidation("output must be a file path, got empty value") + } + cleanedOutput := filepath.Clean(outputPath) + if cleanedOutput == "." || cleanedOutput == ".." || + strings.HasSuffix(outputPath, "/") || strings.HasSuffix(outputPath, "\\") { + return output.ErrValidation("output must be a file path, got %q", outputPath) + } // Early path validation before API call (final validation after auto-extension below) if _, err := validate.SafeOutputPath(outputPath); err != nil { return output.ErrValidation("unsafe output path: %s", err) } @@ safePath, err := validate.SafeOutputPath(finalPath) if err != nil { return output.ErrValidation("unsafe output path: %s", err) } + if info, err := vfs.Stat(safePath); err == nil && info.IsDir() { + return output.ErrValidation("output must be a file path, got directory %q", finalPath) + } else if err != nil && !errors.Is(err, vfs.ErrNotExist) { + return output.Errorf(output.ExitInternal, "io", "cannot access output path %s: %v", finalPath, err) + } if err := common.EnsureWritableFile(safePath, overwrite); err != nil { return err }Also applies to: 85-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/doc_media_preview.go` around lines 63 - 66, The code currently relies only on validate.SafeOutputPath (called with outputPath and later with finalPath) but still allows blank or directory-like values (e.g., "" or paths ending with a separator) that become hidden/file-only names after extension inference; add explicit guards that reject empty strings, single-dot/ double-dot entries, and any path that ends with a path separator (or whose base is empty) before calling the API and again after you compute finalPath; return output.ErrValidation with a clear message (e.g., "output must be a file path, not a directory or empty") when outputPath or finalPath fails this guard, leaving existing validate.SafeOutputPath checks in place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@shortcuts/doc/doc_media_preview.go`:
- Around line 80-82: The error handling after DoAPIStream currently wraps all
errors with output.ErrNetwork which hides existing ExitError details; change it
to detect if err is an ExitError (using errors.As or a type assertion for the
ExitError type from the output package) and if so return that original ExitError
directly, otherwise wrap the non-ExitError with output.ErrNetwork; look for the
DoAPIStream call and the current "if err != nil { return output.ErrNetwork(...)"
branch in doc_media_preview.go and update it to first check/return the
ExitError, then wrap others.
- Around line 63-66: The code currently relies only on validate.SafeOutputPath
(called with outputPath and later with finalPath) but still allows blank or
directory-like values (e.g., "" or paths ending with a separator) that become
hidden/file-only names after extension inference; add explicit guards that
reject empty strings, single-dot/ double-dot entries, and any path that ends
with a path separator (or whose base is empty) before calling the API and again
after you compute finalPath; return output.ErrValidation with a clear message
(e.g., "output must be a file path, not a directory or empty") when outputPath
or finalPath fails this guard, leaving existing validate.SafeOutputPath checks
in place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dda53326-9c05-4cc9-98c0-36577bb375b6
📒 Files selected for processing (8)
shortcuts/doc/doc_media_preview.goshortcuts/doc/doc_media_test.goshortcuts/doc/shortcuts.goshortcuts/register_test.goskills/lark-doc/SKILL.mdskills/lark-doc/references/lark-doc-fetch.mdskills/lark-doc/references/lark-doc-media-download.mdskills/lark-doc/references/lark-doc-media-preview.md
✅ Files skipped from review due to trivial changes (4)
- shortcuts/doc/shortcuts.go
- shortcuts/register_test.go
- skills/lark-doc/references/lark-doc-media-download.md
- skills/lark-doc/references/lark-doc-media-preview.md
🚧 Files skipped from review as they are similar to previous changes (3)
- skills/lark-doc/SKILL.md
- skills/lark-doc/references/lark-doc-fetch.md
- shortcuts/doc/doc_media_test.go
fangshuyu-768
approved these changes
Apr 8, 2026
This was referenced Apr 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Change-Id: I5db9e52008e175f975838c8a9c03254afa30f52b
Summary
Add a new
docs +media-previewshortcut for document media preview and update thelark-docskill guidance to prefer preview for view-only requests. Keep explicit download flows and whiteboard-related cases ondocs +media- download.Changes
docs +media-previewshortcut, register it underdocs, and cover it with shortcut tests.lark-docskill references so media preview is preferred for file/image viewing, while explicit downloads and whiteboard cases continue to usedocs +media-download.Test Plan
lark xxxcommand works as expectedRelated Issues
Summary by CodeRabbit
New Features
Documentation
Tests