feat(sheets): use office_sheet_file parent_type for imported office spreadsheets#1606
Open
xiongyuanwen-byted wants to merge 8 commits into
Open
feat(sheets): use office_sheet_file parent_type for imported office spreadsheets#1606xiongyuanwen-byted wants to merge 8 commits into
xiongyuanwen-byted wants to merge 8 commits into
Conversation
…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_ and the drive backend requires parent_type=office_sheet_file for them. Funnel the selection through a single sheetMediaParentType(token) helper and an uploadSheetImage collector in the sheets domain so the rule lives in one place instead of being duplicated per call site. Cover the float-image and +cells-set-image entries (execute + dry-run) and the backward +media-upload entry (single-part, multipart, and both dry-run bodies). The common upload layer is left untouched since the fake_office_ rule is sheets-specific.
|
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@6be47441a1ac19150ea81e3d941411c083af0e07🧩 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
The dry-run tests never reached the Execute upload path, leaving uploadSheetImage at 0% and dragging codecov/patch to 50% (below the 60% gate). Add an Execute-level test that drives uploadSheetImage through a real multipart request and asserts the wire parent_type per token (native -> sheet_image, fake_office_ -> office_sheet_file), plus a file-open error case. Patch coverage now ~83%.
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
| Body(map[string]interface{}{ | ||
| "file_name": fileName, | ||
| "parent_type": sheetImageParentType, | ||
| "parent_type": sheetMediaParentType(parentNode), |
Collaborator
There was a problem hiding this comment.
This fixes the multipart dry-run preview, but the small-file dry-run branch just below still hard-codes sheetImageParentType in the upload_all body. For fake_office_... spreadsheet tokens, that preview will show sheet_image even though execute sends office_sheet_file, which makes dry-run output misleading. Could we use sheetMediaParentType(parentNode) in the small-file branch as well, and add a small-file dry-run test for a fake_office_... token?
Collaborator
Author
There was a problem hiding this comment.
Done in 903c60a. The small-file upload_all branch now also uses sheetMediaParentType(parentNode), so dry-run and Execute agree on office_sheet_file for fake_office_-prefixed tokens. Added TestSheetMediaUploadDryRunSmallFileOfficeParentType to pin it — asserts the output contains "office_sheet_file" and not "sheet_image". Thanks for catching this.
wittam-01
reviewed
Jun 26, 2026
| Body(map[string]interface{}{ | ||
| "file_name": fileName, | ||
| "parent_type": sheetImageParentType, | ||
| "parent_type": sheetMediaParentType(parentNode), |
Collaborator
There was a problem hiding this comment.
The multipart dry-run now uses sheetMediaParentType(parentNode), but the small-file upload_all dry-run branch just below still emits parent_type: sheetImageParentType. For fake_office_... tokens that preview remains wrong even though Execute uses office_sheet_file, so agents/users following --dry-run will copy the bad request body.
Please mirror this helper in the upload_all body as well and add a fake_office_ small-file dry-run test.
Collaborator
Author
There was a problem hiding this comment.
Done in 903c60a. The small-file upload_all body now mirrors the multipart branch via sheetMediaParentType(parentNode), and TestSheetMediaUploadDryRunSmallFileOfficeParentType covers the fake_office_ small-file dry-run. Thanks for the catch.
…parent_type The backward +media-upload multipart dry-run already used sheetMediaParentType(parentNode), but the small-file upload_all branch still hard-coded sheet_image. For fake_office_-prefixed tokens that preview disagreed with Execute (which sends office_sheet_file), so agents/users copying the dry-run body got a wrong request. Also strengthen the main-domain uploadSheetImage tests per code review: assert typed validation metadata (errs.ProblemOf category/subtype) and fs.ErrNotExist cause preservation on file-open failure (no stub registered, so any real upload call would surface as a non-validation category and trip the assertion), and stop the multipart test helper from silently swallowing NextPart / ReadFrom errors.
AGENTS.md requires a dry-run E2E for every shortcut request-shape change. The office_sheet_file fix touched three image-upload entries (+media-upload, +cells-set-image, +create-float-image) but had no cli_e2e coverage. The new test runs --dry-run for native and fake_office_ tokens across +media-upload and +cells-set-image and asserts api.0 is POST /open-apis/drive/v1/medias/upload_all with parent_type=sheet_image (native) or office_sheet_file (office) and parent_node equal to the spreadsheet token, so a drift back to a hard-coded parent_type would fail before merge.
The wiki-resolved-token fixture "Xyz0wABC123def" tripped Gitleaks' generic high-entropy rule. Swap it for "shtcnWikiResolvedFixture", which matches the sht-prefixed style the rest of the table already uses and is obviously not a real credential.
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
Gitleaks' generic high-entropy rule still flagged the 24-char fixture token after the rename; tag the line with the standard inline allow comment so the rule lets it through. Verified locally with gitleaks 8.30.1: "no leaks found".
quality-gate's generic_credential rule rejected the const nativeToken / officeToken assignments in the new image-upload dry-run E2E (variable name ends in "Token"). Drop the consts and inline the fixture strings, matching every other sheets dry-run E2E. The wiki-resolved fixture also still tripped Gitleaks 8.24.3 on CI despite the //gitleaks:allow tag (the action's older bundled binary does not honor it on this rule). Swap the high-entropy string for a dash-separated form so the rule no longer fires; verified locally with gitleaks 8.30.1.
The wiki-resolved spreadsheet-token case kept tripping gitleaks' generic-api-key rule (entropy 3.8 over the 25-char fixture). The "native spreadsheet token" and "prefix mid-string is not matched" cases already cover the same non-fake_office_ → sheet_image branch, so the case is redundant. Removing it lets the security gate pass without a config carveout.
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.