feat(sheets): use office_sheet_file parent_type for imported office spreadsheets#1606
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSpreadsheet image uploads now derive Drive parent types from spreadsheet tokens, including ChangesOffice spreadsheet image uploads
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
github-actions
Bot
added
domain/ccm
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1606 +/- ##
==========================================
+ Coverage 74.59% 74.66% +0.06%
==========================================
Files 793 806 +13
Lines 79069 81391 +2322
==========================================
+ Hits 58985 60769 +1784
- Misses 15700 16089 +389
- Partials 4384 4533 +149 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@bd0e0509f3c9384b751e8ede807f03452f78730b🧩 Skill updatenpx skills add larksuite/cli#feat/sheets-upload-image-compatible -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/sheets/backward/lark_sheets_float_images.go (1)
65-98: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the token-derived parent type in the single-part dry-run branch.
The multipart
--dry-runpreview was updated, but the small-file/medias/upload_allpreview still hardcodessheet_image. Forfake_office_*tokens,+media-upload --dry-runwill therefore show the wrong request unless the file is >20MB. Please switch that branch tosheetMediaParentType(parentNode)as well, and add a small regression test for the office-token dry-run path.Suggested fix
return dry.Desc("multipart/form-data upload"). POST("/open-apis/drive/v1/medias/upload_all"). Body(map[string]interface{}{ "file_name": fileName, - "parent_type": sheetImageParentType, + "parent_type": sheetMediaParentType(parentNode), "parent_node": parentNode, "size": "<file_size>", "file": "@" + filePath, }). Set("spreadsheet_token", parentNode)🤖 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/sheets/backward/lark_sheets_float_images.go` around lines 65 - 98, The single-part dry-run preview in the sheet image upload path still hardcodes the parent type instead of using the token-derived value. Update the `/medias/upload_all` branch in the same function that builds the dry-run request to use `sheetMediaParentType(parentNode)` just like the multipart path, so `fake_office_*` tokens render the correct request; then add a regression test covering the office-token dry-run case for the single-part path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@shortcuts/sheets/backward/lark_sheets_float_images.go`:
- Around line 65-98: The single-part dry-run preview in the sheet image upload
path still hardcodes the parent type instead of using the token-derived value.
Update the `/medias/upload_all` branch in the same function that builds the
dry-run request to use `sheetMediaParentType(parentNode)` just like the
multipart path, so `fake_office_*` tokens render the correct request; then add a
regression test covering the office-token dry-run case for the single-part path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0fd976e1-92d5-4183-aa7f-ddc2df37ac98
📒 Files selected for processing (7)
shortcuts/sheets/backward/lark_sheets_float_images.goshortcuts/sheets/backward/lark_sheets_sheet_media_upload_test.goshortcuts/sheets/helpers.goshortcuts/sheets/lark_sheet_object_crud.goshortcuts/sheets/lark_sheet_write_cells.goshortcuts/sheets/lark_sheet_write_cells_test.goshortcuts/sheets/sheet_media_parent_type_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/sheets/sheet_media_parent_type_test.go`:
- Around line 109-117: Strengthen TestUploadSheetImage_FileOpenError so it
verifies the exact file-open failure returned by uploadSheetImage, not just that
an error occurred. In the test, assert the typed problem metadata with
errs.ProblemOf (category, subtype, and param) and confirm the original cause is
preserved. Also add an explicit assertion that the upload path was never reached
by proving no upload request was issued, using the uploadSheetImage test
runtime/setup to check the endpoint was not hit.
- Around line 150-156: The multipart body reader in the test helper is
swallowing parse and read failures, which can let malformed uploads pass with
partial data. Update the helper around reader.NextPart() and buf.ReadFrom(part)
to explicitly handle errors: treat only EOF as the loop terminator, and fail the
test immediately on any other NextPart() or ReadFrom() error. Use the multipart
parsing logic in sheet_media_parent_type_test.go to ensure the accumulated body
always reflects a fully valid upload payload.
🪄 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: cea3fccf-10ad-4bca-bd1f-19c04882881c
📒 Files selected for processing (1)
shortcuts/sheets/sheet_media_parent_type_test.go
wittam-01
reviewed
Jun 26, 2026
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/cli_e2e/sheets/sheets_image_upload_dryrun_test.go (1)
18-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the test comment with the cases actually exercised.
Lines 23-25 say this test covers
sheets +create-float-image, but the table only exercises+media-uploadand+cells-set-image. Please either add the float-image dry-run cases here or narrow the comment so this file does not overstate its coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cli_e2e/sheets/sheets_image_upload_dryrun_test.go` around lines 18 - 25, The test comment in TestSheets_ImageUploadDryRunParentType overstates the covered image-upload surfaces by mentioning sheets +create-float-image even though the table only exercises sheets +media-upload and sheets +cells-set-image. Either add the missing float-image dry-run cases to this test, or update the comment so it only describes the scenarios actually covered by the existing table and helper setup.
🤖 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.
Nitpick comments:
In `@tests/cli_e2e/sheets/sheets_image_upload_dryrun_test.go`:
- Around line 18-25: The test comment in TestSheets_ImageUploadDryRunParentType
overstates the covered image-upload surfaces by mentioning sheets
+create-float-image even though the table only exercises sheets +media-upload
and sheets +cells-set-image. Either add the missing float-image dry-run cases to
this test, or update the comment so it only describes the scenarios actually
covered by the existing table and helper setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bf244a9b-2e5c-4751-8ced-e99dee541a4f
📒 Files selected for processing (2)
shortcuts/sheets/sheet_media_parent_type_test.gotests/cli_e2e/sheets/sheets_image_upload_dryrun_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/sheets/sheet_media_parent_type_test.go
fangshuyu-768
approved these changes
Jun 27, 2026
…preadsheets Image uploads to a spreadsheet hard-coded parent_type=sheet_image at every entry point. Imported "office" spreadsheets carry a token prefixed with "fake_office_", for which the drive backend requires parent_type=office_sheet_file. Funnel the parent_type selection through a single sheets-domain helper so the rule lives in one place and every image-upload path (float-image, +cells-set-image, backward +media-upload, and every dry-run preview) stays consistent. - Add sheetMediaParentType(token) in the sheets domain: returns office_sheet_file for fake_office_-prefixed tokens, otherwise sheet_image. - Add an uploadSheetImage(...) collector that builds the DriveMediaUploadAllConfig (including parent_type) once, replacing the per-call-site hand-rolled configs. - Route both main-domain image entries through the collector — float-image local upload and +cells-set-image — covering Execute and the dry-run preview body/desc. - Cover the backward +media-upload entry: single-part, multipart (>20MB), and both dry-run bodies. backward is a separate package and an intentional verbatim mirror of shortcuts/sheets/, so it keeps its own copy of the helper rather than importing the main domain. - Leave the shared common.UploadDriveMediaAllTyped upload layer untouched — the fake_office_ rule is sheets-specific and must not leak into mail/slides/doc/drive/base. Tests: - Pure-function TestSheetMediaParentType (5 cases incl. prefix-only and mid-string non-match). - Main-domain dry-run TestCellsSetImage_DryRunOfficeParentType and TestUploadSheetImage_ParentType / _FileOpenError that exercise the Execute path on the wire, asserting parent_type via the captured multipart body and typed validation metadata (errs.ProblemOf category/subtype, fs.ErrNotExist cause preserved) on file open errors. decodeSheetMediaMultipartBody fails fast on NextPart / ReadFrom errors rather than silently producing a partial body. - backward TestSheetMediaUploadExecuteOfficeParentType (real multipart wire) and TestSheetMediaUploadDryRunSmallFileOfficeParentType (small-file dry-run preview for fake_office_). - cli_e2e tests/cli_e2e/sheets/sheets_image_upload_dryrun_test.go: --dry-run end-to-end across +media-upload and +cells-set-image, native and fake_office_ tokens, asserting api.0 is POST upload_all with parent_type=sheet_image / office_sheet_file and parent_node = token.
xiongyuanwen-byted
force-pushed
the
feat/sheets-upload-image-compatible
branch
from
June 27, 2026 07:30
6be4744 to
bd0e050
Compare
Merged
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
Image uploads to a spreadsheet hard-coded
parent_type=sheet_imageat every entry point. Imported "office" spreadsheets carry a token prefixed withfake_office_, for which the drive backend requiresparent_type=office_sheet_file. This PR funnels the parent_type selection through a single sheets-domain helper so the rule lives in one place and every image-upload path stays consistent.Changes
sheetMediaParentType(token)in the sheets domain: returnsoffice_sheet_fileforfake_office_-prefixed tokens, otherwisesheet_image. Single source of truth for the mapping.uploadSheetImage(...)collector that builds theDriveMediaUploadAllConfig(including parent_type) once, replacing the per-call-site hand-rolled configs.+cells-set-image— covering Execute and the dry-run preview body/desc.backward+media-uploadentry: single-part, multipart (>20MB), and both dry-run bodies.backwardis a separate package and an intentional verbatim mirror ofshortcuts/sheets/, so it keeps its own copy of the helper rather than importing the main domain (avoids a cross-package dependency and preserves the mirror).common.UploadDriveMediaAllTypedupload layer untouched — thefake_office_rule is sheets-specific and must not leak into mail/slides/doc/drive/base.Test Plan
go test ./shortcuts/sheets/...)TestSheetMediaParentType(6 cases incl. prefix-only and mid-string non-match), main-domain dry-runTestCellsSetImage_DryRunOfficeParentType, andbackwardreal-multipart-bodyTestSheetMediaUploadExecuteOfficeParentTypeassertingoffice_sheet_file+parent_nodeactually go out on the wiregofmtclean;golangci-lint run --new-from-rev=HEADreports 0 new issueslark-cli <domain> <command>flow works as expectedRelated Issues
Summary by CodeRabbit
Bug Fixes
parent_nodematching the provided spreadsheet token.Tests
parent_type/parent_nodeassertions and dry-run validation.