feat: support Base attachment APIs#887
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR replaces record-field PATCH-based attachment updates with Base v3 attachment endpoints: append_attachments and remove_attachments, and adds a dedicated download flow. It introduces upload/download/remove shortcuts (batch upload via repeatable ChangesBase Attachment Operations Refactor
Sequence Diagram(s)sequenceDiagram
participant Client
participant BaseAPI as Base v3 API
participant DriveAPI as Drive API
participant FileSystem
rect rgba(100, 200, 255, 0.5)
Note over Client,FileSystem: Upload Attachment (New Flow)
Client->>FileSystem: Read local files
FileSystem-->>Client: File bytes
Client->>BaseAPI: POST /open-apis/base/v3/.../upload_file (multipart if >20MB)
BaseAPI-->>Client: file_token
Client->>BaseAPI: POST /open-apis/base/v3/.../append_attachments {rec_id, field_id, file_token, image_width?, image_height?}
BaseAPI-->>Client: Success
end
rect rgba(100, 255, 200, 0.5)
Note over Client,FileSystem: Download Attachment (New Flow)
Client->>BaseAPI: GET /open-apis/base/v3/.../get_attachments (optional file_token filter)
BaseAPI-->>Client: Attachment metadata list
Client->>DriveAPI: GET /drive/v1/medias/{file_token}/download
DriveAPI-->>FileSystem: Stream file bytes
Client->>FileSystem: Save to --output directory
FileSystem-->>Client: Local file path
end
rect rgba(255, 200, 100, 0.5)
Note over Client,BaseAPI: Remove Attachment (New Flow)
Client->>BaseAPI: POST /open-apis/base/v3/.../remove_attachments {rec_id, field_id, file_tokens: [...]}
BaseAPI-->>Client: Success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
github-actions
Bot
added
domain/base
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #887 +/- ##
==========================================
+ Coverage 65.89% 65.91% +0.01%
==========================================
Files 518 520 +2
Lines 48945 49539 +594
==========================================
+ Hits 32254 32652 +398
- Misses 13921 14083 +162
- Partials 2770 2804 +34 ☔ View full report in Codecov by Sentry. 🚀 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
🤖 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/base/base_execute_test.go`:
- Line 1968: The test name mismatches its assertions: Test currently named
"download attachment uses extra info" but asserts the returned map lacks
extra_info_used because downloadBaseAttachment never returns that key; either
rename the test to reflect what it actually verifies (e.g., "download attachment
includes extra parameter in request" /
TestDownloadAttachment_IncludesExtraParameterInRequest) or change the
implementation to include extra_info_used in downloadBaseAttachment's returned
map; update the test function name and any references accordingly or add the
extra_info_used field to the return value and adjust assertions to expect it.
🪄 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: 3d05e80e-0681-499d-8c5e-52c95bff2812
📒 Files selected for processing (12)
shortcuts/base/base_dryrun_ops_test.goshortcuts/base/base_execute_test.goshortcuts/base/base_shortcuts_test.goshortcuts/base/record_upload_attachment.goshortcuts/base/record_upload_attachment_test.goshortcuts/base/shortcuts.goskills/lark-base/SKILL.mdskills/lark-base/references/lark-base-cell-value.mdskills/lark-base/references/lark-base-record-upload-attachment.mdskills/lark-base/references/lark-base-record.mdtests/cli_e2e/base/base_attachment_dryrun_test.gotests/cli_e2e/base/coverage.md
💤 Files with no reviewable changes (1)
- skills/lark-base/references/lark-base-record-upload-attachment.md
| t.Fatalf("downloaded=%#v", data["downloaded"]) | ||
| } | ||
| got, _ := gotItems[0].(map[string]interface{}) | ||
| if got["file_token"] != "box_a" || got["saved_path"] == "" || got["extra_info_used"] != nil { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the test file
find . -name "base_execute_test.go" -type fRepository: larksuite/cli
Length of output: 95
🏁 Script executed:
# Read the test function around line 1968 with context
# Start from line 1900 to see the full test setup
head -1971 shortcuts/base/base_execute_test.go | tail -100Repository: larksuite/cli
Length of output: 3127
🏁 Script executed:
# Also check the test function name around line 1968
sed -n '1900,1980p' shortcuts/base/base_execute_test.go | head -100Repository: larksuite/cli
Length of output: 2605
🏁 Script executed:
rg "extra_info_used" --type go -B 2 -A 2Repository: larksuite/cli
Length of output: 430
🏁 Script executed:
# Also search for where this output field is set in the implementation
rg "extra_info_used" -A 3 -B 3Repository: larksuite/cli
Length of output: 556
🏁 Script executed:
# Search for where extra_info_used is set in the actual implementation
fd "base_execute" --type f -not -name "*test.go" | xargs rg "extra_info_used" -B 5 -A 5Repository: larksuite/cli
Length of output: 1112
🏁 Script executed:
# Let's look at the base_execute.go file to find the download implementation
fd "base_execute.go" --type f -not -name "*test.go"Repository: larksuite/cli
Length of output: 288
🏁 Script executed:
# Find base_execute.go (the implementation, not the test)
find . -name "base_execute.go" -type f | grep -v testRepository: larksuite/cli
Length of output: 39
🏁 Script executed:
# Search for extra_info_used in non-test files
rg "extra_info_used" --type go --glob '!*test.go' -B 5 -A 5Repository: larksuite/cli
Length of output: 39
🏁 Script executed:
# Find the base_execute.go implementation file
find . -name "base_execute.go" -type fRepository: larksuite/cli
Length of output: 39
🏁 Script executed:
# Search for "downloaded" output structure to understand what fields are returned
rg "downloaded" --type go --glob '!*test.go' -B 3 -A 3 | head -50Repository: larksuite/cli
Length of output: 3746
🏁 Script executed:
# Search for where saved_path is set in the output
rg "saved_path" --type go --glob '!*test.go' -B 3 -A 3Repository: larksuite/cli
Length of output: 6905
🏁 Script executed:
# Let's also search for the structure that gets serialized to the "downloaded" output
rg "file_token.*saved_path" --type go -B 5 -A 5Repository: larksuite/cli
Length of output: 863
🏁 Script executed:
# Search for the record_download_attachment implementation
rg "record_download_attachment\|RecordDownloadAttachment" --type go --glob '!*test.go' -lRepository: larksuite/cli
Length of output: 39
🏁 Script executed:
# Let's look for record_download in base files
find shortcuts/base -name "*.go" -type f ! -name "*test.go" | xargs ls -laRepository: larksuite/cli
Length of output: 7507
🏁 Script executed:
# Search for BaseRecordDownloadAttachment definition
rg "BaseRecordDownloadAttachment" --type go --glob '!*test.go' -B 3 -A 10Repository: larksuite/cli
Length of output: 1805
🏁 Script executed:
# Check the record_upload_attachment.go for download implementation or clues
head -100 shortcuts/base/record_upload_attachment.goRepository: larksuite/cli
Length of output: 4272
🏁 Script executed:
# Search for executeRecordDownloadAttachment implementation
rg "executeRecordDownloadAttachment" --type go --glob '!*test.go' -A 100 | head -150Repository: larksuite/cli
Length of output: 12370
🏁 Script executed:
# Search for downloadBaseAttachment function to see what it returns
rg "func downloadBaseAttachment" --type go -A 80 | head -120Repository: larksuite/cli
Length of output: 6301
🏁 Script executed:
# Let's check if there are any other tests that may use extra_info_used or show expected behavior
rg "extra_info" --type go | grep -v BinaryRepository: larksuite/cli
Length of output: 605
🏁 Script executed:
# Let's check the full implementation of downloadBaseAttachment to understand the complete return structure
grep -A 50 "func downloadBaseAttachment" shortcuts/base/record_upload_attachment.go | tail -40Repository: larksuite/cli
Length of output: 1445
🏁 Script executed:
# Let's verify by looking at the complete return value one more time to be absolutely certain
sed -n '600,700p' shortcuts/base/record_upload_attachment.go | grep -A 15 "return map"Repository: larksuite/cli
Length of output: 505
Test name doesn't match assertion behavior.
The test is named "download attachment uses extra info" and correctly verifies that the download request includes the extra query parameter. However, the assertion got["extra_info_used"] != nil expects this field to be absent (nil) from the output, which contradicts the test name's implication that the output reflects extra info usage.
The implementation confirms that downloadBaseAttachment never includes extra_info_used in its returned map (returns only: record_id, field_id, file_token, name, size, saved_path, size_bytes, content_type), so the assertion is technically correct. But the test name should clarify that it only validates the request includes the extra parameter, not that the output documents extra info usage. Consider renaming to "download attachment includes extra parameter in request" or add extra_info_used to the output if tracking this was the intent.
🤖 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/base/base_execute_test.go` at line 1968, The test name mismatches
its assertions: Test currently named "download attachment uses extra info" but
asserts the returned map lacks extra_info_used because downloadBaseAttachment
never returns that key; either rename the test to reflect what it actually
verifies (e.g., "download attachment includes extra parameter in request" /
TestDownloadAttachment_IncludesExtraParameterInRequest) or change the
implementation to include extra_info_used in downloadBaseAttachment's returned
map; update the test function name and any references accordingly or add the
extra_info_used field to the return value and adjust assertions to expect it.
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@7d715ff932173123c0d4e8d3d15f7b2399fd5162🧩 Skill updatenpx skills add zgz2048/cli#codex/base-attachment-apis -y -g |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
shortcuts/base/record_upload_attachment.go (5)
770-784: ⚡ Quick winMove the existence pre-check before
DoAPIStreamto avoid wasted API calls.The
!overwriteexistence check at Lines 780–784 happens after the download HTTP request has already been initiated viaDoAPIStream. When the target file already exists and--overwriteis false, this causes an avoidable network round-trip, consumes API quota, and burns rate-limit budget — only to fail validation right after.Note that
planAttachmentDownloadTargets(Lines 695–699) already performs the same existence check at plan time, so the post-API repetition appears to be defensive against a TOCTOU between plan and execute. In that case, the re-check still has value, but it should run before the API call.♻️ Suggested reordering
func downloadBaseAttachment(ctx context.Context, runtime *common.RuntimeContext, item baseAttachmentDownloadItem, targetPath string, overwrite bool) (map[string]interface{}, error) { if _, err := runtime.ResolveSavePath(targetPath); err != nil { return nil, output.ErrValidation("unsafe output path: %s", err) } + if !overwrite { + if _, statErr := runtime.FileIO().Stat(targetPath); statErr == nil { + return nil, output.ErrValidation("output file already exists: %s (use --overwrite to replace)", targetPath) + } + } + query := larkcore.QueryParams{} if item.ExtraInfo != "" { query.Set("extra", item.ExtraInfo) } resp, err := runtime.DoAPIStream(ctx, &larkcore.ApiReq{ HttpMethod: http.MethodGet, ApiPath: fmt.Sprintf("/open-apis/drive/v1/medias/%s/download", validate.EncodePathSegment(item.FileToken)), QueryParams: query, }) if err != nil { return nil, output.ErrNetwork("download failed: %v", err) } defer resp.Body.Close() - - if !overwrite { - if _, statErr := runtime.FileIO().Stat(targetPath); statErr == nil { - return nil, output.ErrValidation("output file already exists: %s (use --overwrite to replace)", targetPath) - } - } result, err := runtime.FileIO().Save(targetPath, fileio.SaveOptions{🤖 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/base/record_upload_attachment.go` around lines 770 - 784, The post-request file-existence check should be moved before initiating the download to avoid wasted API calls: before calling runtime.DoAPIStream(...) (the current ApiReq/DoAPIStream block), perform the same !overwrite check using runtime.FileIO().Stat(targetPath) and return output.ErrValidation(...) if the file exists; only after that check succeeds should you call runtime.DoAPIStream to create resp and defer resp.Body.Close(); keep the same error types and messages and preserve the defensive TOCTOU protection by checking immediately prior to the network call.
601-675: 💤 Low valueSame
file_tokenacross multiple attachment fields silently drops all but the first occurrence.In Base it is valid for the same
file_tokento appear in more than one attachment field of the same record. The dedup at Lines 631–633 keeps only the first occurrence (in sortedcurrentFieldIDorder), so theFieldIDreported back to the user in the download summary is whichever field sorted first — not necessarily the one the user expected.For the "download by
--file-token" path this only affects the reportedfield_idin the JSON envelope (the bytes are the same), so it's not a correctness bug, but users tracking attachments by(record, field)pairs may be confused. Worth a docstring onselectAttachmentDownloadItemsor a tip in the shortcut help noting that cross-field duplicates resolve to a single download keyed by the sorted-first field.🤖 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/base/record_upload_attachment.go` around lines 601 - 675, selectAttachmentDownloadItems currently deduplicates byToken which silently drops duplicate file_token occurrences across different fields; update the function to preserve all occurrences by changing byToken from map[string]baseAttachmentDownloadItem to map[string][]baseAttachmentDownloadItem (or similar) so you append each baseAttachmentDownloadItem for a given fileToken instead of skipping when exists, then adjust the downstream assembly: when tokens is empty, flatten all slices into result; when tokens are provided, look up the slice for each token and append all items (error only if token key missing); keep sorting but include FieldID (or FileToken+FieldID) in the tie-breaker to produce deterministic ordering; update references to byToken, the lookup branch that returned a single item, and any sorting comparator in selectAttachmentDownloadItems accordingly.
233-245: 💤 Low valuePossible nil deref guarded only by short-circuit — make the intent explicit.
info.IsDir()at Line 240 is only safe becausestatErr != nilshort-circuits the||. This works in practice (well-behavedStatimplementations don't return(nil, nil)), but readers must mentally trace the short-circuit to confirm safety, and a future change that reorders the operands would dereference a nil interface.Consider making the nil-safety explicit:
- info, statErr := runtime.FileIO().Stat(runtime.Str("output")) - if statErr != nil || !info.IsDir() { + info, statErr := runtime.FileIO().Stat(runtime.Str("output")) + if statErr != nil || info == nil || !info.IsDir() { return common.FlagErrorf("--output must be an existing directory when downloading multiple attachments or when --file-token is omitted") }🤖 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/base/record_upload_attachment.go` around lines 233 - 245, In validateRecordDownloadAttachment, make the nil-check for the FileInfo returned by runtime.FileIO().Stat explicit before calling info.IsDir(): call info, statErr := runtime.FileIO().Stat(runtime.Str("output")), then test if statErr != nil || info == nil || !info.IsDir() to decide returning common.FlagErrorf("--output must be an existing directory when downloading multiple attachments or when --file-token is omitted"); this ensures info is not nil before invoking info.IsDir() and preserves the original behavior.
270-294: 🏗️ Heavy liftPartial-failure on multi-file upload leaks already-uploaded media.
The loop uploads each file to Base media first, then a single
append_attachmentscall links them all at the end. If file N fails to upload, files 1…N-1 are already on the server but never linked to any record — they become orphaned media. Retrying will re-upload them, creating duplicate orphans on every attempt.This is a real concern for the multi-file workflow this PR is enabling. Consider one of:
- Calling
append_attachmentsincrementally per successful upload (more API calls, but no orphans).- Surfacing the orphaned
file_tokens in the error detail so users (or follow-up tooling) can clean up or re-link them.At minimum, the error returned at Line 283 should include the
file_tokens of the already-uploaded files so the user isn't blind.🤖 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/base/record_upload_attachment.go` around lines 270 - 294, The loop that uploads files (see appendItems and uploadAttachmentToBase) can leave already-uploaded media orphaned if a later upload fails; modify the error path inside the loop so that when uploadAttachmentToBase returns an error you collect and return the file_token(s) from the attachments already in appendItems (or from the returned attachment objects) in the error message/return value (e.g., extract file_token fields from attachmentAppendItem/attachment objects and include them in the error), or alternatively switch to calling append_attachments incrementally after each successful upload (i.e., call buildSingleCellAttachmentsBody + baseV3Call per uploaded file) to avoid orphans entirely; update the error returned where uploadAttachmentToBase fails to include the list of uploaded file_token strings so callers can clean up.
395-419: 💤 Low valueDrop
normalizeOptionalAttachmentFileTokensas it has no callers.
normalizeOptionalAttachmentFileTokens(lines 395–400) is unused in the codebase and delegates tonormalizeAttachmentFileTokens, which errors on duplicates. Meanwhile,normalizeOptionalDownloadAttachmentFileTokens(lines 402–419) silently dedupes viadedupeStringsPreserveOrder. The divergent duplicate semantics and unused helper create unnecessary maintenance burden.🤖 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/base/record_upload_attachment.go` around lines 395 - 419, Remove the dead wrapper function normalizeOptionalAttachmentFileTokens: delete the normalizeOptionalAttachmentFileTokens definition since it has no callers and only delegates to normalizeAttachmentFileTokens; before removing, search the repo for any references to normalizeOptionalAttachmentFileTokens and update them to call normalizeAttachmentFileTokens or normalizeOptionalDownloadAttachmentFileTokens as appropriate, and ensure duplicate-handling semantics remain intentional between normalizeAttachmentFileTokens (which errors on duplicates) and normalizeOptionalDownloadAttachmentFileTokens (which dedupes) so behavior is not accidentally changed.
🤖 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/base/record_upload_attachment.go`:
- Around line 506-511: detectAttachmentImageDimensions fails to decode WebP
because only PNG/JPEG/GIF decoders are registered, causing the misleading
warning in the branch that checks strings.HasPrefix(mimeType, "image/"); either
register the WebP decoder by adding the blank import _ "golang.org/x/image/webp"
alongside the existing blank imports (lines where image decoders are registered)
so detectAttachmentImageDimensions can return real dimensions, or change the
warning condition in the code that assigns
attachment["image_width"]/["image_height"] (the if/else block using
detectAttachmentImageDimensions) to only print the warning for the specific mime
types you actually decode (e.g., "image/png","image/jpeg","image/gif") instead
of all image/* MIME types.
---
Nitpick comments:
In `@shortcuts/base/record_upload_attachment.go`:
- Around line 770-784: The post-request file-existence check should be moved
before initiating the download to avoid wasted API calls: before calling
runtime.DoAPIStream(...) (the current ApiReq/DoAPIStream block), perform the
same !overwrite check using runtime.FileIO().Stat(targetPath) and return
output.ErrValidation(...) if the file exists; only after that check succeeds
should you call runtime.DoAPIStream to create resp and defer resp.Body.Close();
keep the same error types and messages and preserve the defensive TOCTOU
protection by checking immediately prior to the network call.
- Around line 601-675: selectAttachmentDownloadItems currently deduplicates
byToken which silently drops duplicate file_token occurrences across different
fields; update the function to preserve all occurrences by changing byToken from
map[string]baseAttachmentDownloadItem to map[string][]baseAttachmentDownloadItem
(or similar) so you append each baseAttachmentDownloadItem for a given fileToken
instead of skipping when exists, then adjust the downstream assembly: when
tokens is empty, flatten all slices into result; when tokens are provided, look
up the slice for each token and append all items (error only if token key
missing); keep sorting but include FieldID (or FileToken+FieldID) in the
tie-breaker to produce deterministic ordering; update references to byToken, the
lookup branch that returned a single item, and any sorting comparator in
selectAttachmentDownloadItems accordingly.
- Around line 233-245: In validateRecordDownloadAttachment, make the nil-check
for the FileInfo returned by runtime.FileIO().Stat explicit before calling
info.IsDir(): call info, statErr :=
runtime.FileIO().Stat(runtime.Str("output")), then test if statErr != nil ||
info == nil || !info.IsDir() to decide returning common.FlagErrorf("--output
must be an existing directory when downloading multiple attachments or when
--file-token is omitted"); this ensures info is not nil before invoking
info.IsDir() and preserves the original behavior.
- Around line 270-294: The loop that uploads files (see appendItems and
uploadAttachmentToBase) can leave already-uploaded media orphaned if a later
upload fails; modify the error path inside the loop so that when
uploadAttachmentToBase returns an error you collect and return the file_token(s)
from the attachments already in appendItems (or from the returned attachment
objects) in the error message/return value (e.g., extract file_token fields from
attachmentAppendItem/attachment objects and include them in the error), or
alternatively switch to calling append_attachments incrementally after each
successful upload (i.e., call buildSingleCellAttachmentsBody + baseV3Call per
uploaded file) to avoid orphans entirely; update the error returned where
uploadAttachmentToBase fails to include the list of uploaded file_token strings
so callers can clean up.
- Around line 395-419: Remove the dead wrapper function
normalizeOptionalAttachmentFileTokens: delete the
normalizeOptionalAttachmentFileTokens definition since it has no callers and
only delegates to normalizeAttachmentFileTokens; before removing, search the
repo for any references to normalizeOptionalAttachmentFileTokens and update them
to call normalizeAttachmentFileTokens or
normalizeOptionalDownloadAttachmentFileTokens as appropriate, and ensure
duplicate-handling semantics remain intentional between
normalizeAttachmentFileTokens (which errors on duplicates) and
normalizeOptionalDownloadAttachmentFileTokens (which dedupes) so behavior is not
accidentally changed.
🪄 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: 80f44a1e-3279-4e26-b754-5276485d097a
📒 Files selected for processing (2)
shortcuts/base/base_execute_test.goshortcuts/base/record_upload_attachment.go
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/base/base_execute_test.go
| if width, height, ok := detectAttachmentImageDimensions(runtime.FileIO(), filePath, mimeType); ok { | ||
| attachment["image_width"] = width | ||
| attachment["image_height"] = height | ||
| } else if strings.HasPrefix(mimeType, "image/") { | ||
| fmt.Fprintf(runtime.IO().ErrOut, "Warning: image dimensions unavailable for %s; attachment may display as square\n", fileName) | ||
| } |
There was a problem hiding this comment.
WebP attachments will always trigger the "image dimensions unavailable" warning.
detectAttachmentMIMEFromContent recognizes image/webp (Lines 882–884), but the blank imports at Lines 11–14 only register PNG/JPEG/GIF decoders for image.DecodeConfig. As a result, any WebP upload reaches detectAttachmentImageDimensions, fails decoding, and then hits the strings.HasPrefix(mimeType, "image/") branch — printing a misleading warning to stderr saying the file "may display as square" even though WebP is a valid image format the server can render with proper dimensions.
Either add _ "golang.org/x/image/webp" (if that dependency is acceptable) so dimensions are detected, or scope the warning to only the formats we actually attempt to decode.
🛡️ Suggested scoping of the warning (no new dep)
if width, height, ok := detectAttachmentImageDimensions(runtime.FileIO(), filePath, mimeType); ok {
attachment["image_width"] = width
attachment["image_height"] = height
- } else if strings.HasPrefix(mimeType, "image/") {
+ } else if mimeType == "image/png" || mimeType == "image/jpeg" || mimeType == "image/gif" {
fmt.Fprintf(runtime.IO().ErrOut, "Warning: image dimensions unavailable for %s; attachment may display as square\n", fileName)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if width, height, ok := detectAttachmentImageDimensions(runtime.FileIO(), filePath, mimeType); ok { | |
| attachment["image_width"] = width | |
| attachment["image_height"] = height | |
| } else if strings.HasPrefix(mimeType, "image/") { | |
| fmt.Fprintf(runtime.IO().ErrOut, "Warning: image dimensions unavailable for %s; attachment may display as square\n", fileName) | |
| } | |
| if width, height, ok := detectAttachmentImageDimensions(runtime.FileIO(), filePath, mimeType); ok { | |
| attachment["image_width"] = width | |
| attachment["image_height"] = height | |
| } else if mimeType == "image/png" || mimeType == "image/jpeg" || mimeType == "image/gif" { | |
| fmt.Fprintf(runtime.IO().ErrOut, "Warning: image dimensions unavailable for %s; attachment may display as square\n", fileName) | |
| } |
🤖 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/base/record_upload_attachment.go` around lines 506 - 511,
detectAttachmentImageDimensions fails to decode WebP because only PNG/JPEG/GIF
decoders are registered, causing the misleading warning in the branch that
checks strings.HasPrefix(mimeType, "image/"); either register the WebP decoder
by adding the blank import _ "golang.org/x/image/webp" alongside the existing
blank imports (lines where image decoders are registered) so
detectAttachmentImageDimensions can return real dimensions, or change the
warning condition in the code that assigns
attachment["image_width"]/["image_height"] (the if/else block using
detectAttachmentImageDimensions) to only print the warning for the specific mime
types you actually decode (e.g., "image/png","image/jpeg","image/gif") instead
of all image/* MIME types.
1 participant
Summary
Refactor Base attachment handling onto the new attachment APIs and add first-class commands for downloading and removing Base record attachments. This improves agent ergonomics for multi-attachment workflows and avoids manual Base attachment download handling.
Changes
base +record-upload-attachmentto upload one or more local files, send image width/height for PNG/JPEG/GIF uploads, and append uploaded file tokens throughappend_attachments.base +record-download-attachmentfor Base attachment downloads byrecord-id, optionally filtered by one or more--file-tokenvalues; omitting--file-tokendownloads all attachments in the record.base +record-remove-attachmentfor removing one or more attachment file tokens from a record attachment cell.Breaking Changes
base +record-upload-attachment --nameis no longer supported; uploaded attachment names come from local file basenames.base +record-upload-attachmentnow prints theappend_attachmentsserverdatadirectly instead of older CLI-composed fields such asupdated,uploaded, orattachment.append_attachmentsinstead of reading the existing record and patching the whole attachment array.Release Note
--namesupport has been removed.Test Plan
make unit-testgo vet ./...gofmt -l .go mod tidyleftgo.mod/go.sumunchangedgo test ./tests/cli_e2e/base -run TestBase_AttachmentDryRun -count=1go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.1.6 run --new-from-rev=upstream/mainNote:
go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.1.6 run --new-from-rev=origin/maincurrently reports pre-existing unrelated depguard/forbidigo issues inshortcuts/event,shortcuts/mail,shortcuts/sheets,shortcuts/minutes, andinternal/keychain; the same lint againstupstream/mainreports 0 issues for this PR diff.Related Issues
Summary by CodeRabbit
New Features
--nameflag deprecated/rejected; image dimension autodetection.Documentation
Tests