fix(base): preserve attachment metadata on base uploads#563
Merged
Conversation
📝 WalkthroughWalkthroughAdds extension-first then content-based MIME detection for attachments, includes Changes
Sequence DiagramsequenceDiagram
participant Client
participant Upload as uploadAttachmentToBase()
participant Detect as detectAttachmentMIMEType()
participant FS as FileSystem
participant Base as Base API
Client->>Upload: initiate attachment upload
Upload->>Detect: ask MIME (fileName, filePath, file)
Detect->>Detect: strip params, check extension
alt extension yields MIME
Detect-->>Upload: return MIME
else extension unknown
Detect->>FS: read first 512 bytes
FS-->>Detect: return bytes or error
alt read successful
Detect->>Detect: check magic numbers / text heuristic
Detect-->>Upload: return inferred MIME or application/octet-stream
else read error
Detect-->>Upload: return wrapped read error
end
end
Upload->>Upload: build metadata {file_token, name, size, mime_type}
Upload->>Base: PATCH record-upload-attachment with metadata
Base-->>Upload: respond with attachment data
Upload-->>Client: return attachment info
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
domain/base
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #563 +/- ##
==========================================
+ Coverage 60.19% 60.24% +0.04%
==========================================
Files 390 390
Lines 33433 33482 +49
==========================================
+ Hits 20125 20170 +45
- Misses 11426 11428 +2
- Partials 1882 1884 +2 ☔ 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
shortcuts/base/record_upload_attachment.go (2)
109-113:⚠️ Potential issue | 🟡 MinorKeep dry-run PATCH metadata in sync.
The real upload now patches
mime_typeandsize, but dry-run still shows onlyfile_token,name, anddeprecated_set_attachmentfor the uploaded item. This makes the user-facing dry-run output stale.🛠️ Proposed dry-run payload update
map[string]interface{}{ "file_token": "<uploaded_file_token>", "name": fileName, + "mime_type": "<detected_mime_type>", + "size": "<file_size>", "deprecated_set_attachment": true, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/base/record_upload_attachment.go` around lines 109 - 113, The dry-run PATCH metadata in shortcuts/base/record_upload_attachment.go is missing the mime_type and size fields that the real upload now patches; update the dry-run map literal (the uploaded item metadata map containing "file_token", "name", "deprecated_set_attachment") to include "mime_type" and "size" with appropriate placeholder values (e.g., "<mime/type>" and a numeric placeholder or the size variable) so the dry-run output matches the real upload payload.
249-279:⚠️ Potential issue | 🟠 MajorDetect MIME before performing the upload.
detectAttachmentMIMETyperuns afterUploadDriveMediaAll/UploadDriveMediaMultipart. If the post-upload read fails, the command returns an error after the non-idempotent upload already succeeded, leaving uploaded media unattached to the Base record. Fail fast before upload instead.🛠️ Proposed ordering fix
func uploadAttachmentToBase(runtime *common.RuntimeContext, filePath, fileName, baseToken string, fileSize int64) (map[string]interface{}, error) { + mimeType, err := detectAttachmentMIMEType(runtime.FileIO(), filePath, fileName) + if err != nil { + return nil, err + } + parentNode := baseToken var ( fileToken string - err error ) if fileSize <= common.MaxDriveMediaUploadSinglePartSize { fileToken, err = common.UploadDriveMediaAll(runtime, common.DriveMediaUploadAllConfig{ @@ if err != nil { return nil, err } - - mimeType, err := detectAttachmentMIMEType(runtime.FileIO(), filePath, fileName) - if err != nil { - return nil, err - } attachment := map[string]interface{}{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/base/record_upload_attachment.go` around lines 249 - 279, In uploadAttachmentToBase, detectAttachmentMIMEType is called after the non-idempotent UploadDriveMediaAll/UploadDriveMediaMultipart calls, which can leave orphaned uploads if the post-read fails; move the detectAttachmentMIMEType(runtime.FileIO(), filePath, fileName) call to before selecting/performing the upload (i.e., call it at the top of uploadAttachmentToBase and return any error immediately), then proceed to call UploadDriveMediaAll or UploadDriveMediaMultipart with the confirmed MIME; keep the same parameters and error handling for UploadDriveMediaAll/UploadDriveMediaMultipart and ensure any early error aborts before any upload is attempted.
🤖 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/base/record_upload_attachment_test.go`:
- Around line 99-104: The test only checks the binary fallback for
detectAttachmentMIMEFromContent; add a table-driven subtest in
TestDetectAttachmentMIMEFromContentBinaryFallback (or rename to
TestDetectAttachmentMIMEFromContent) that enumerates cases for PNG, JPEG, GIF,
WEBP, PDF, empty content, text (printable bytes), control-byte text
(non-printable ASCII), and the binary fallback, each with input byte slices and
expected MIME strings, and iterate with t.Run for each case asserting
detectAttachmentMIMEFromContent returns the expected value so all signature
branches are covered.
---
Outside diff comments:
In `@shortcuts/base/record_upload_attachment.go`:
- Around line 109-113: The dry-run PATCH metadata in
shortcuts/base/record_upload_attachment.go is missing the mime_type and size
fields that the real upload now patches; update the dry-run map literal (the
uploaded item metadata map containing "file_token", "name",
"deprecated_set_attachment") to include "mime_type" and "size" with appropriate
placeholder values (e.g., "<mime/type>" and a numeric placeholder or the size
variable) so the dry-run output matches the real upload payload.
- Around line 249-279: In uploadAttachmentToBase, detectAttachmentMIMEType is
called after the non-idempotent UploadDriveMediaAll/UploadDriveMediaMultipart
calls, which can leave orphaned uploads if the post-read fails; move the
detectAttachmentMIMEType(runtime.FileIO(), filePath, fileName) call to before
selecting/performing the upload (i.e., call it at the top of
uploadAttachmentToBase and return any error immediately), then proceed to call
UploadDriveMediaAll or UploadDriveMediaMultipart with the confirmed MIME; keep
the same parameters and error handling for
UploadDriveMediaAll/UploadDriveMediaMultipart and ensure any early error aborts
before any upload is attempted.
🪄 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: 2b0cbca4-3a0b-45e1-90d4-b82956f557d0
📒 Files selected for processing (3)
shortcuts/base/base_execute_test.goshortcuts/base/record_upload_attachment.goshortcuts/base/record_upload_attachment_test.go
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@8ec64411704974a0503a3791ca28821c14dc441e🧩 Skill updatenpx skills add larksuite/cli#fix-attachment-size -y -g |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/base/record_upload_attachment.go`:
- Around line 292-308: detectAttachmentMIMEType currently only checks the
extension of fileName (via mime.TypeByExtension(filepath.Ext(fileName))) before
content sniffing, which misses cases where fileName has no extension but
filePath does; update detectAttachmentMIMEType to, after the existing fileName
extension check fails, try mime.TypeByExtension on
strings.ToLower(filepath.Ext(filePath)) (trimmed and passed through
stripMIMEParams) and return that if non-empty before opening/reading the file;
keep existing error handling on fio.Open, reading into buf and falling back to
detectAttachmentMIMEFromContent if no extension-derived MIME is found.
🪄 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: 09be3f2b-01b2-49b7-97c9-6e1217a019d4
📒 Files selected for processing (3)
shortcuts/base/base_dryrun_ops_test.goshortcuts/base/record_upload_attachment.goshortcuts/base/record_upload_attachment_test.go
✅ Files skipped from review due to trivial changes (1)
- shortcuts/base/record_upload_attachment_test.go
timzhong1024
approved these changes
Apr 20, 2026
kongenpei
changed the title
zgz2048
approved these changes
Apr 20, 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
3 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
This PR fixes Base attachment uploads so newly uploaded files preserve attachment metadata when the CLI patches the record cell.
What Changed
sizein the attachment payload written back bybase +record-upload-attachmentmime_typein that payload as wellRoot Cause
The upload step returned a
file_token, but the subsequent Base record patch rebuilt the new attachment item with onlyfile_tokenandname. That droppedsizeandmime_type, which then showed up as missing or incorrect metadata in the web UI.Impact
Base attachment uploads from the CLI now keep the same essential metadata that downstream UI and automation expect:
mime_type: text/plainValidation
make unit-testgo vet ./...gofmt -l .go mod tidywith nogo.mod/go.sumdiffgo test ./shortcuts/base -run 'TestBaseRecordExecute/upload attachment' -vsizemime_type: text/plainNotes
go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.1.6 run --new-from-rev=origin/maincurrently fails in this environment with:main.go:1: : error obtaining VCS status: exit status 128Summary by CodeRabbit
New Features
Tests
Bug Fixes