feat(doc): add --from-clipboard flag to docs +media-insert#508
Open
herbertliu wants to merge 20 commits intomainfrom
Open
feat(doc): add --from-clipboard flag to docs +media-insert#508herbertliu wants to merge 20 commits intomainfrom
herbertliu wants to merge 20 commits intomainfrom
Conversation
Allow users to upload the current clipboard image directly to a Lark
document without saving to a local file first.
- New --from-clipboard bool flag (mutually exclusive with --file)
- shortcuts/doc/clipboard.go: readClipboardToTempFile() with per-OS impl
macOS — osascript (built-in, no extra deps)
Windows — PowerShell + System.Windows.Forms (built-in)
Linux — tries xclip / wl-paste / xsel in order; clear install hint
on failure
- No new Go dependencies, no Cgo
- Temp file is created before upload and removed via defer cleanup()
- --file changed from Required:true to optional; Validate enforces
exactly-one of --file / --from-clipboard
…er-copied images - Add TIFF fallback (macOS screenshots default to TIFF, not PNG) - Add HTML base64 fallback (images copied from Feishu/browser embed data URI) - Use current directory for temp file so FileIO path validation passes
…URIs Extend attempt-3 fallback to iterate all text-based clipboard formats (HTML, RTF, UTF-8, plain text) rather than only HTML. Any format that contains a "data:<mime>;base64,<data>" pattern is accepted, covering images copied from Feishu, Chrome, Safari, and other apps that embed base64 in non-HTML clipboard slots. Also handle URL-safe base64.
…threshold Cover decodeHex, hexVal, decodeOsascriptData, reBase64DataURI, and extractBase64ImageFromClipboard (via fake osascript on PATH). Package coverage: 57% → 61.2%.
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:
📝 WalkthroughWalkthroughAdd in-memory, cross-platform clipboard image extraction and decoding; wire clipboard-sourced bytes into the doc media CLI upload flow; extend drive-media upload helpers to accept an io.Reader content source; and add tests covering decoding, extraction, and tool-absence behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant User as CLI User
participant CLI as doc_media_insert.Execute
participant Reader as readClipboardImageBytes
participant OS as OS-specific handler
participant Tool as System Tool
participant Upload as Drive Upload
User->>CLI: run command with --from-clipboard
CLI->>Reader: request image bytes
Reader->>OS: dispatch by runtime.GOOS
OS->>Tool: invoke osascript / powershell / xclip|wl-paste|xsel
Tool-->>OS: return hex/base64/raw bytes
OS->>OS: decode/convert to PNG bytes
OS-->>Reader: return PNG bytes
Reader-->>CLI: supply bytes + size/filename
CLI->>Upload: pass io.Reader + size to upload
Upload-->>CLI: respond with upload result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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 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@a0b5548cd425d84b0644d7db16c45f4f06ffcee9🧩 Skill updatenpx skills add larksuite/cli#feat/media-insert-clipboard -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: 4
🧹 Nitpick comments (1)
shortcuts/doc/clipboard_test.go (1)
12-56: These cleanup tests never callreadClipboardToTempFile().Both cases rebuild a local
cleanupclosure instead of exercising the production helper, so they won't catch regressions in temp-file creation, platform dispatch, or the returned cleanup contract. If you want coverage on the real path, make the temp-file creator / platform reader injectable and test throughreadClipboardToTempFile().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/clipboard_test.go` around lines 12 - 56, The tests currently recreate a local cleanup closure instead of exercising the production function readClipboardToTempFile, so they don't validate the real temp-file creation or cleanup contract; update the tests to call readClipboardToTempFile directly (or refactor to inject a temp-file creator and platform reader into readClipboardToTempFile) and assert that the returned path is empty on error and that the returned cleanup function removes the temp file and is idempotent; reference the TestReadClipboardToTempFile_EmptyResultRemovesTempFile and TestReadClipboardToTempFile_CleanupIsIdempotent tests and the readClipboardToTempFile function when making the change.
🤖 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/clipboard_test.go`:
- Around line 167-185: The test writes a POSIX shell fake osascript and
hardcodes ":" as the PATH separator, which fails on Windows; update the test in
shortcuts/doc/clipboard_test.go to either skip the fake-osascript harness on
non-Darwin platforms or make it platform-aware: check runtime.GOOS (or use
runtime.GOOS == "darwin") and call t.Skip on Windows/other OSes before creating
fakeScript/outputFile, and when prepending to PATH use
string(os.PathListSeparator) (or os.PathListSeparator) instead of ":" when
setting PATH around orig, ensuring the fakeScript, outputFile and PATH
manipulation (variables fakeScript, outputFile, orig and the t.Cleanup that
resets PATH) are only used on supported platforms.
In `@shortcuts/doc/clipboard.go`:
- Line 66: The regex reBase64DataURI currently omits URL-safe base64 characters
'-' and '_', causing URL-safe payloads to be skipped before
base64.URLEncoding.DecodeString is called; update the character class in
reBase64DataURI to include '-' and '_' (and ensure '=' padding is still allowed)
so it matches both standard and URL-safe base64 data URIs, and apply the same
change to the other similar pattern(s) referenced around the decode logic (the
other occurrence that feeds into base64.URLEncoding.DecodeString) so both
branches handle URL-safe payloads.
- Around line 244-253: The loop over tools in clipboard.go currently returns as
soon as it finds a tool in PATH even if that tool yields empty output; change
the logic in the for loop that iterates over tools (the block using
exec.LookPath and exec.Command) so that if a found tool returns an error or
empty output you continue to the next tool instead of returning an error, and
only return a final error after all tools have been tried and none produced
non-empty output; when a tool succeeds (non-empty out from exec.Command), write
the file via os.WriteFile(destPath, out, 0600) and return nil. Ensure
exec.LookPath(t.name), exec.Command(t.name, t.args...), the tools slice, and
os.WriteFile are the referenced symbols you update.
- Around line 29-58: The clipboard code is using os.* directly (os.CreateTemp,
os.Stat, os.Remove, os.WriteFile) and must use the repo filesystem abstraction
instead; change all temp-file operations in the function that creates
path/cleanup and the branches that call
readClipboardDarwin/readClipboardWindows/readClipboardLinux to use the vfs
equivalents (e.g., vfs.CreateTemp, vfs.Stat, vfs.Remove, vfs.WriteFile or the
repo's provided names) and ensure the cleanup closure calls vfs.Remove; keep the
same error handling and verification logic (checking file Size via vfs.Stat) so
behavior is identical but uses the vfs API for testability and consistency.
---
Nitpick comments:
In `@shortcuts/doc/clipboard_test.go`:
- Around line 12-56: The tests currently recreate a local cleanup closure
instead of exercising the production function readClipboardToTempFile, so they
don't validate the real temp-file creation or cleanup contract; update the tests
to call readClipboardToTempFile directly (or refactor to inject a temp-file
creator and platform reader into readClipboardToTempFile) and assert that the
returned path is empty on error and that the returned cleanup function removes
the temp file and is idempotent; reference the
TestReadClipboardToTempFile_EmptyResultRemovesTempFile and
TestReadClipboardToTempFile_CleanupIsIdempotent tests and the
readClipboardToTempFile function when making the change.
🪄 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: 6ebf6f50-c2fa-4fdc-b045-4986ec204dcd
📒 Files selected for processing (3)
shortcuts/doc/clipboard.goshortcuts/doc/clipboard_test.goshortcuts/doc/doc_media_insert.go
- Extend reBase64DataURI regex to cover URL-safe base64 chars (-_) so URL-safe payloads are matched before decoding is attempted - Fix readClipboardLinux to continue to next tool when a found tool returns empty output instead of failing immediately - Guard fake-osascript test with runtime.GOOS == "darwin" skip - Use os.PathListSeparator instead of hardcoded ":" in test PATH setup
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
shortcuts/doc/clipboard_test.go (1)
130-145: Add a URL-safe base64 regression case forreBase64DataURI.This test currently covers only standard base64. Since the regex was expanded to accept
-/_, add a case that includes URL-safe characters to prevent regressions.💡 Suggested test addition
func TestReBase64DataURI_Match(t *testing.T) { imgBytes := []byte{0x89, 0x50, 0x4e, 0x47} // PNG magic bytes b64 := base64.StdEncoding.EncodeToString(imgBytes) html := `<img src="data:image/png;base64,` + b64 + `">` @@ if string(m[2]) != b64 { t.Errorf("base64 payload mismatch") } + + // URL-safe payload should also match (contains '-' and '_'). + urlSafeHTML := `<img src="data:image/png;base64,-_8=">` + m2 := reBase64DataURI.FindSubmatch([]byte(urlSafeHTML)) + if m2 == nil { + t.Fatal("expected regex to match URL-safe base64 data URI") + } + if string(m2[2]) != "-_8=" { + t.Errorf("url-safe payload mismatch: got %q", m2[2]) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/clipboard_test.go` around lines 130 - 145, Add a URL-safe base64 regression test for reBase64DataURI: create a second test (or extend TestReBase64DataURI_Match) that encodes sample bytes using base64.URLEncoding (or base64.RawURLEncoding) so the encoded string contains '-' and '_' characters, build an HTML data URI like `<img src="data:image/png;base64,` + urlSafeB64 + `">`, call reBase64DataURI.FindSubmatch on that HTML, assert the match is non-nil, assert m[1] == "image/png" and assert m[2] equals the URL-safe encoded payload to ensure the regex accepts '-' and '_' variants.
🤖 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/clipboard_test.go`:
- Around line 31-34: The failure message can panic because info may be nil when
os.Stat fails; change the check in the test to first handle err (if err != nil
then call t.Fatalf with the error only) and only call info.Size() when err is
nil (e.g., else if info.Size() != 0 then t.Fatalf including the size); reference
the os.Stat call and the variables info, err and the t.Fatalf invocation when
making the change.
---
Nitpick comments:
In `@shortcuts/doc/clipboard_test.go`:
- Around line 130-145: Add a URL-safe base64 regression test for
reBase64DataURI: create a second test (or extend TestReBase64DataURI_Match) that
encodes sample bytes using base64.URLEncoding (or base64.RawURLEncoding) so the
encoded string contains '-' and '_' characters, build an HTML data URI like
`<img src="data:image/png;base64,` + urlSafeB64 + `">`, call
reBase64DataURI.FindSubmatch on that HTML, assert the match is non-nil, assert
m[1] == "image/png" and assert m[2] equals the URL-safe encoded payload to
ensure the regex accepts '-' and '_' variants.
🪄 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: d47886f0-197c-4f1e-97dd-e58a56d11f08
📒 Files selected for processing (2)
shortcuts/doc/clipboard.goshortcuts/doc/clipboard_test.go
✅ Files skipped from review due to trivial changes (1)
- shortcuts/doc/clipboard.go
Fixes forbidigo lint violations in shortcuts/doc: os.CreateTemp, os.Remove, os.Stat, os.WriteFile are banned in shortcuts/; replaced with vfs.* equivalents for sips TIFF→PNG conversion, and eliminated temp files entirely elsewhere by having platform clipboard readers return []byte directly. - readClipboardDarwin: osascript outputs hex literals decoded in Go (no file I/O) - readClipboardWindows: PowerShell outputs base64 to stdout, decoded in Go - readClipboardLinux: tool stdout bytes returned directly - convertTIFFToPNGViaSips: still needs temp files — uses vfs.CreateTemp/Remove - DriveMediaUploadAllConfig/DriveMediaMultipartUploadConfig: add Content io.Reader field so in-memory clipboard bytes skip FileIO.Open() path - Fix ineffassign in clipboard_test.go (scriptBody double-assignment) - Update TestReadClipboardLinux_NoToolsReturnsError for new signature
github-actions
bot
added
size/L
herbertliu
force-pushed
the
feat/media-insert-clipboard
branch
from
b3023fe to
6df0695
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/clipboard.go`:
- Around line 258-273: The loop that checks tools via exec.LookPath and
exec.Command currently discards backend-specific failures and always returns the
generic "no supported tool found" error; modify the loop in the clipboard image
read routine to track whether any backend was present and preserve the last
backend-specific error: introduce a foundBackend boolean and lastErr error, set
foundBackend=true when exec.LookPath(t.name) succeeds, and on
exec.Command(t.name, t.args...).Output() failures or empty output assign a
descriptive lastErr (wrap the underlying err when non-nil or create an error
noting empty output for t.name) and continue; after the loop, if lastErr != nil
return lastErr, else return the existing generic "no supported tool found"
error. Ensure references to exec.LookPath, exec.Command(...).Output(), tools,
foundBackend, and lastErr are used to locate and implement the change.
In `@shortcuts/doc/doc_media_insert.go`:
- Line 33: The help text for the "from-clipboard" flag is missing xsel for Linux
users; update the flag description (the entry where Name: "from-clipboard" is
defined) to list "xclip, xsel or wl-paste" for Linux, so it matches the
implementation in readClipboardLinux(); ensure the wording mirrors the accepted
tools and keeps platform notes (macOS/Windows built-in; Linux requires xclip,
xsel or wl-paste).
🪄 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: be9c1b92-92b4-43d0-99dc-d40be8d6016e
📒 Files selected for processing (5)
shortcuts/common/drive_media_upload.goshortcuts/doc/clipboard.goshortcuts/doc/clipboard_test.goshortcuts/doc/doc_media_insert.goshortcuts/doc/doc_media_upload.go
✅ Files skipped from review due to trivial changes (2)
- shortcuts/doc/doc_media_upload.go
- shortcuts/doc/clipboard_test.go
Collaborator
fangshuyu-768
left a comment
fangshuyu-768
left a comment
There was a problem hiding this comment.
Nice feature overall, especially the move to in-memory uploads. I do not think this is ready to merge yet, though: CI is currently red, and the remaining temp-file-based TIFF fallback still feels more fragile than the other clipboard paths.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
shortcuts/doc/clipboard.go (1)
258-273:⚠️ Potential issue | 🟡 MinorPreserve the last Linux backend failure.
If a backend is installed but returns empty output or exits non-zero, this now falls through to
no supported tool found, which points users at the wrong fix. Keep the last backend-specific error once any tool has been discovered.Proposed fix
+ var lastErr error + foundTool := false for _, t := range tools { if _, lookErr := exec.LookPath(t.name); lookErr != nil { continue } + foundTool = true out, err := exec.Command(t.name, t.args...).Output() if err != nil || len(out) == 0 { - // Tool found but returned nothing — try the next one. + if err != nil { + lastErr = fmt.Errorf("clipboard image read failed via %s: %w", t.name, err) + } else { + lastErr = fmt.Errorf("clipboard contains no image data (%s returned empty output)", t.name) + } continue } return out, nil } + if foundTool && lastErr != nil { + return nil, lastErr + } + return nil, fmt.Errorf( "clipboard image read failed: no supported tool found\n" + " X11: sudo apt install xclip (or: sudo yum install xclip)\n" + " Wayland: sudo apt install wl-clipboard")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/clipboard.go` around lines 258 - 273, Loop over tools but currently discards backend-specific failures and always returns "no supported tool found"; modify the logic in the for loop that iterates tools (the code using exec.LookPath and exec.Command on each t.name) to record the last error when a tool is present but returns non-zero or empty output (e.g., store lastErr and lastToolName when exec.Command fails or out is empty), and at the end, if lastErr != nil return nil with a message that includes that backend-specific error (or a clear note that the discovered tool <lastToolName> failed) instead of the generic "no supported tool found" message; keep the existing generic install hint only when no tool was found at all.
🧹 Nitpick comments (1)
shortcuts/doc/clipboard_test.go (1)
130-145: Add a URL-safe data-URI regression case.This matcher was expanded specifically to admit
-and_, but the test still covers only the standard+/alphabet. A URL-safe example here would protect the exact bugfix that motivated the regex change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/clipboard_test.go` around lines 130 - 145, Update the TestReBase64DataURI_Match unit to include a URL-safe base64 data-URI case that contains '-' and '_' characters to exercise the expanded alphabet in reBase64DataURI: generate or use a short URL-safe base64 payload (e.g. with '-' and '_'), build an HTML <img> src like `data:image/png;base64,<payload>`, call reBase64DataURI.FindSubmatch on it (as in TestReBase64DataURI_Match), and add assertions that m is non-nil and that m[1] equals "image/png" and m[2] equals the URL-safe payload to ensure the regex accepts the URL-safe alphabet.
🤖 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/clipboard.go`:
- Around line 252-255: The tools fallback list includes "xsel" which cannot
negotiate MIME types and can return non-image data; remove the {"xsel", ...}
entry from the tools slice or, if keeping it, validate the clipboard output
before returning by checking the PNG magic bytes (the first 8 bytes equal to 89
50 4E 47 0D 0A 1A 0A) on the variable "out" in the clipboard reading routine;
locate the tools slice in shortcuts/doc/clipboard.go and the code path that
returns "out" after invoking a tool and either drop the xsel item or add the
PNG-magic-number check and return an error when the magic header does not match.
---
Duplicate comments:
In `@shortcuts/doc/clipboard.go`:
- Around line 258-273: Loop over tools but currently discards backend-specific
failures and always returns "no supported tool found"; modify the logic in the
for loop that iterates tools (the code using exec.LookPath and exec.Command on
each t.name) to record the last error when a tool is present but returns
non-zero or empty output (e.g., store lastErr and lastToolName when exec.Command
fails or out is empty), and at the end, if lastErr != nil return nil with a
message that includes that backend-specific error (or a clear note that the
discovered tool <lastToolName> failed) instead of the generic "no supported tool
found" message; keep the existing generic install hint only when no tool was
found at all.
---
Nitpick comments:
In `@shortcuts/doc/clipboard_test.go`:
- Around line 130-145: Update the TestReBase64DataURI_Match unit to include a
URL-safe base64 data-URI case that contains '-' and '_' characters to exercise
the expanded alphabet in reBase64DataURI: generate or use a short URL-safe
base64 payload (e.g. with '-' and '_'), build an HTML <img> src like
`data:image/png;base64,<payload>`, call reBase64DataURI.FindSubmatch on it (as
in TestReBase64DataURI_Match), and add assertions that m is non-nil and that
m[1] equals "image/png" and m[2] equals the URL-safe payload to ensure the regex
accepts the URL-safe alphabet.
🪄 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: 773f5719-c9c5-4b27-a0a7-554670776db6
📒 Files selected for processing (5)
shortcuts/common/drive_media_upload.goshortcuts/doc/clipboard.goshortcuts/doc/clipboard_test.goshortcuts/doc/doc_media_insert.goshortcuts/doc/doc_media_upload.go
✅ Files skipped from review due to trivial changes (1)
- shortcuts/doc/doc_media_upload.go
🚧 Files skipped from review as they are similar to previous changes (2)
- shortcuts/common/drive_media_upload.go
- shortcuts/doc/doc_media_insert.go
- Update --from-clipboard flag description to list xclip, xsel and wl-paste - Preserve last backend-specific error in readClipboardLinux so users see a meaningful message when a tool is found but fails - Validate PNG magic bytes for xsel output (xsel cannot negotiate MIME types) - Add URL-safe base64 regression test for reBase64DataURI
…ard data URI HTML and RTF clipboard content often line-wraps base64 at 76 characters. FindSubmatch returns the raw wrapped token so direct decode would fail. Normalize whitespace with strings.Fields before passing to base64.Decode.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
shortcuts/doc/clipboard_test.go (1)
31-34:⚠️ Potential issue | 🟡 MinorGuard against nil dereference in the failure message.
If
os.Statfails,infocan be nil, butinfo.Size()is still evaluated int.Fatalf, which will panic and hide the real failure.🐛 Proposed fix to separate error checks
info, err := os.Stat(emptyPath) - if err != nil || info.Size() != 0 { - t.Fatalf("expected empty file, got size=%d err=%v", info.Size(), err) - } + if err != nil { + t.Fatalf("stat empty file: %v", err) + } + if info.Size() != 0 { + t.Fatalf("expected empty file, got size=%d", info.Size()) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/clipboard_test.go` around lines 31 - 34, The failure message can dereference a nil info when os.Stat fails; modify the os.Stat check so you first handle err != nil (log t.Fatalf with the error and do not call info.Size()), and only if err == nil then check info.Size() and fail with size information; reference the os.Stat call, the info and err variables, and the t.Fatalf invocation in shortcuts/doc/clipboard_test.go to locate and update the logic.
🧹 Nitpick comments (1)
shortcuts/doc/doc_media_insert.go (1)
129-131: Consider detecting actual image format for clipboard filename.The filename is hardcoded as
clipboard.png, but clipboard images extracted from base64 data URIs could be JPEG or other formats. While the backend likely detects the actual format from content, using the correct extension would improve metadata accuracy.💡 Potential enhancement (optional)
You could extend
readClipboardImageBytes()to return the detected MIME type alongside the bytes, then derive the extension:// In clipboard.go, change signature to: // func readClipboardImageBytes() ([]byte, string, error) // returns bytes, mimeType, error // In doc_media_insert.go: clipboardContent, mimeType, err := readClipboardImageBytes() // ... ext := ".png" if mimeType == "image/jpeg" { ext = ".jpg" } fileName = "clipboard" + ext🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/doc_media_insert.go` around lines 129 - 131, The filename for clipboard images is hardcoded to "clipboard.png" even though the actual clipboard image may be JPEG/other; update readClipboardImageBytes (used by doc_media_insert.go) to return the detected MIME type alongside the []byte (e.g., func readClipboardImageBytes() ([]byte, string, error)), then in the block where clipboardContent is set (variables clipboardContent, fileSize, fileName) derive the extension from the returned mimeType (map "image/jpeg" -> ".jpg", "image/png" -> ".png", etc.), fall back to ".png" if unknown, and set fileName = "clipboard"+ext; preserve existing error handling when readClipboardImageBytes returns an error.
🤖 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/clipboard_test.go`:
- Around line 31-34: The failure message can dereference a nil info when os.Stat
fails; modify the os.Stat check so you first handle err != nil (log t.Fatalf
with the error and do not call info.Size()), and only if err == nil then check
info.Size() and fail with size information; reference the os.Stat call, the info
and err variables, and the t.Fatalf invocation in
shortcuts/doc/clipboard_test.go to locate and update the logic.
---
Nitpick comments:
In `@shortcuts/doc/doc_media_insert.go`:
- Around line 129-131: The filename for clipboard images is hardcoded to
"clipboard.png" even though the actual clipboard image may be JPEG/other; update
readClipboardImageBytes (used by doc_media_insert.go) to return the detected
MIME type alongside the []byte (e.g., func readClipboardImageBytes() ([]byte,
string, error)), then in the block where clipboardContent is set (variables
clipboardContent, fileSize, fileName) derive the extension from the returned
mimeType (map "image/jpeg" -> ".jpg", "image/png" -> ".png", etc.), fall back to
".png" if unknown, and set fileName = "clipboard"+ext; preserve existing error
handling when readClipboardImageBytes returns an error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f463ec34-95bc-46e5-8a2f-0348366ce1c4
📒 Files selected for processing (3)
shortcuts/doc/clipboard.goshortcuts/doc/clipboard_test.goshortcuts/doc/doc_media_insert.go
✅ Files skipped from review due to trivial changes (1)
- shortcuts/doc/clipboard.go
depguard rule shortcuts-no-vfs forbids shortcuts/ from importing internal/vfs directly. The only caller was the sips TIFF→PNG conversion, which was already a fragile best-effort fallback that required temp files. Remove the TIFF fallback entirely; the remaining two attempts cover the real-world cases: 1. osascript → PNG hex literal — native screenshots and most apps 2. scan text clipboard formats for base64 data URI — Feishu/browsers
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #508 +/- ##
==========================================
+ Coverage 60.74% 60.95% +0.20%
==========================================
Files 394 400 +6
Lines 33909 34305 +396
==========================================
+ Hits 20599 20910 +311
- Misses 11400 11469 +69
- Partials 1910 1926 +16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Collaborator
Author
Code reviewFound 1 issue:
cli/shortcuts/doc/doc_media_insert.go Lines 202 to 211 in b092d32 Downstream sink where the contract is evaluated: cli/shortcuts/common/drive_media_upload.go Lines 53 to 65 in b092d32 Fix: either pass 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
…r path Added tests: - TestReadClipboardLinux_XselRejectsNonPNG: fake xsel that returns plain text is rejected by the PNG-magic check, preventing text from being uploaded as an "image". - TestHasPNGMagic: table-driven coverage of the PNG signature check. - TestReadClipboardImageBytes_UnsupportedPlatform: exercises the shared dispatcher post-processing and asserts the (nil, nil) invariant. Raises clipboard.go diff coverage and brings the package from 61.6% to 63.8% overall.
Adds unit tests for the new Content io.Reader branches introduced by the clipboard feature: - UploadDriveMediaAll with in-memory Content (drive_media_upload.go 87.5%) - UploadDriveMediaMultipart with in-memory Content (84.6%) - uploadDocMediaFile single-part and multipart with clipboard bytes (doc_media_upload.go 0% -> 88.9%) Adds TestNewRuntimeContextForAPI helper that wires Factory, context, and bot identity so package tests can invoke DoAPI without mounting the full cobra command tree.
Adds unit tests for the clipboard-related Validate/DryRun paths that Codecov patch-coverage was flagging as uncovered: - Validate error when neither --file nor --from-clipboard is supplied - Validate error when both are supplied (mutual exclusion) - DryRun output contains <clipboard image> placeholder - Self-test for TestNewRuntimeContextForAPI so shortcuts/common sees coverage for the new helper (not just shortcuts/doc)
Makes readClipboardImageBytes swappable in tests by routing the call through a package-level variable readClipboardImage. Tests inject a synthetic PNG payload so the full Execute clipboard flow (resolve → create block → upload in-memory bytes → bind) runs under unit test without a real pasteboard. Covers: - TestDocMediaInsertExecuteFromClipboard: end-to-end happy path - TestDocMediaInsertExecuteClipboardReadError: early-return on readClipboardImage() failure
Previous push to 9dedb7a did not trigger the main CI workflow via the pull_request event (only PR Labels ran). The workflow_dispatch run I triggered manually lacks PR-scoped secrets so security and e2e-live failed. An empty commit replays the pull_request event so the full matrix (deadcode, license-header, security, e2e-live) runs with proper context.
…pboard # Conflicts: # shortcuts/doc/doc_media_insert.go
CodeRabbit flagged that 't.Fatalf("... size=%d err=%v", info.Size(), err)'
evaluates info.Size() even when os.Stat returned (nil, err), which nil-derefs.
Split the check into two stages so the error-path t.Fatalf does not touch
info.
Collaborator
fangshuyu-768
left a comment
fangshuyu-768
left a comment
There was a problem hiding this comment.
A few review notes beyond what CodeRabbit already flagged and that have been addressed. Prioritised roughly by impact: correctness/trust issues first, then code quality / UX. Happy to discuss any of these.
Seven code changes driven by review feedback: 1. clipboard.go: stop using CombinedOutput() on osascript / powershell. Stdout is decoded, stderr is captured separately via cmd.Stderr and surfaced in the terminal error message, so locale warnings or AppleEvent permission prompts no longer pollute the hex/base64 payload or mask the real failure. 2. clipboard.go: validate decoded base64 data URI bytes against known image magic headers (PNG/JPEG/GIF/WebP/BMP). A text clipboard that happens to contain a literal 'data:image/...;base64,...' fragment (documentation, tutorials, pasted HTML source) no longer silently becomes an image upload. 3. clipboard.go: simplify the Linux 'no tool found' install hint to a distro-agnostic phrasing instead of apt/yum only. 4. clipboard_test.go: delete the stale TestReadClipboardToTempFile_* tests. They referenced a readClipboardToTempFile function that no longer exists and only exercised os.CreateTemp/os.Remove. Replace with TestReadClipboardImageBytes_EmptyResultReturnsError which actually locks in the 'empty clipboard' → error contract of the current API (Linux-only since mac/Windows need a real pasteboard). 5. doc_media_upload.go: introduce UploadDocMediaFileConfig struct so uploadDocMediaFile takes a named config instead of 8 positional params. Drops the //nolint:lll the old call site had to carry. 6. doc_media_insert.go: convert the clipboard upload call to the new config struct and only set Config.Content when the clipboard branch actually produced bytes — this also fixes a latent typed-nil bug where a nil *bytes.Reader was being passed through an io.Reader parameter, which tripped the 'if cfg.Content != nil' check in UploadDriveMediaAll and crashed --file uploads. 7. shortcuts/common/testing.go: TestNewRuntimeContextForAPI now takes the identity as an explicit core.Identity parameter instead of hardcoding core.AsBot, and its self-test covers both AsBot and AsUser. Existing call sites pass core.AsBot explicitly. Also annotates DryRun output with an 'upload_size_note' when --from-clipboard is set, since DryRun never reads the pasteboard and can't predict whether the payload will take the single-part or multipart path.
5 tasks
HTML and RTF clipboard content commonly folds base64 payloads at
76 chars (standard MIME folding). The previous character class
[A-Za-z0-9+/\-_]+=* stopped at the first \n, so the downstream
strings.Fields normalisation was a no-op (nothing to strip) and
extractBase64ImageFromClipboard silently uploaded a truncated
payload whose 8-byte prefix happened to pass hasKnownImageMagic.
Extend the class to include \s so the Fields strip actually has
whitespace to remove before base64 decoding. Terminators (", <,
), ;) remain outside the class so the match still ends at the
URI boundary.
Add TestReBase64DataURI_LineWrapped covering \n, \r\n, and \t
folds, full round-trip byte-equality, and the terminator-boundary
invariant so any future regression trips a failing test.
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.
Summary
Add
--from-clipboardtodocs +media-insertso users can paste an image directly from the system pasteboard without first saving it to disk.Changes
shortcuts/doc/clipboard.go(new): cross-platform clipboard image reader, no external deps beyond what each OS ships with.shortcuts/doc/doc_media_insert.go: new--from-clipboardbool flag, mutually exclusive with--file; in-memory bytes flow through the same upload pipeline as a file.shortcuts/common/drive_media_upload.go:DriveMedia*UploadConfignow accepts anio.ReaderContentfield so the clipboard path doesn't need a temp file.shortcuts/doc/doc_media_upload.go: replaced 8 positional params withUploadDocMediaFileConfig.shortcuts/common/testing.go: newTestNewRuntimeContextForAPI(ctx, cmd, cfg, factory, identity)helper.shortcuts/doc/clipboard_test.go: coverage for the decode helpers, the LinuxxselPNG validation, and the dispatcher's empty-result error path.Platform strategy
osascript→ clipboard as«class PNGf»→ hex decode; (2) scan text-based clipboard formats (HTML / RTF / plain text) for an embeddeddata:image/...;base64,...URI and decode after validating image magic (PNG/JPEG/GIF/WebP/BMP). osascript's stderr is captured separately so diagnostics don't contaminate the decoded payload.System.Windows.Forms.Clipboard::GetImage()→[Convert]::ToBase64Stringon stdout; decoded in Go. Stderr captured separately.xclip(X11),wl-paste(Wayland),xsel(X11 fallback) in that order.xclip/wl-pasterequestimage/pngvia MIME;xselcannot negotiate MIME so its output is validated against the PNG magic header. Generic "install one of xclip / wl-clipboard / xsel via your distro's package manager" hint when none is present.Robustness fixes (follow-up to
fangshuyu-768review)exec.CombinedOutput→exec.Output+ separate stderr capture on every platform path.TestReadClipboardToTempFile_*tests replaced withTestReadClipboardImageBytes_EmptyResultReturnsErrorthat actually covers the current in-memory API.TestNewRuntimeContextForAPItakes identity as a parameter (not hardcodedAsBot).uploadDocMediaFiletakes anUploadDocMediaFileConfiginstead of 8 positional params.doc_media_insert.gonow only setsConfig.Contentwhen the clipboard branch actually produced bytes, fixing a latent typed-nil*bytes.Readerbug that made the downstreamif cfg.Content != nilcheck take the wrong branch on the--filepath.upload_size_notewhen--from-clipboardis set, since it cannot predict the single-part vs multipart decision without reading the pasteboard.Test Plan
go test ./shortcuts/doc/... ./shortcuts/common/...passgo vet ./shortcuts/...cleangofmt -l shortcuts/cleangolangci-lint run --new-from-rev=origin/main— 0 issues--file foo.png --doc <url>still works after the typed-nil fix--from-clipboardon macOS with an actual screenshot (verifies PNG path); with a Feishu-copied image (verifies HTML base64 data URI path with image-magic validation)Related
User-requested ergonomic improvement: paste an image directly from clipboard into a Lark document.
🤖 Generated with Claude Code