feat(doc): add --file-view flag to +media-insert#419
feat(doc): add --file-view flag to +media-insert#419fangshuyu-768 merged 4 commits intolarksuite:mainfrom
Conversation
|
|
github-actions
Bot
added
domain/ccm
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
shortcuts/doc/doc_media_insert.go (1)
243-245: Defensively constrainview_typeto known enums before writing payload.
fileViewType > 0allows unexpected positives to pass if validation is bypassed by future call paths. Narrowing to known values prevents malformed API payloads.Proposed hardening
- if fileViewType > 0 { + if fileViewType == 1 || fileViewType == 2 || fileViewType == 3 { fileData["view_type"] = fileViewType }🤖 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 243 - 245, Narrow the check that writes fileData["view_type"] by validating fileViewType against the known enum set instead of just fileViewType > 0; in the function that builds fileData, replace the loose positive check with an explicit whitelist (e.g., switch or set membership) of allowed view enum constants and only assign fileData["view_type"] when fileViewType equals one of those known values (referencing the fileViewType variable and the fileData map key "view_type"); this prevents unknown positive integers from being emitted in the payload.shortcuts/doc/doc_media_insert_test.go (1)
107-118: Consider asserting required mappings instead of full-map equality.Exact equality can become brittle if additional valid aliases are introduced later; key-by-key assertions keep intent while allowing safe extension.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/doc/doc_media_insert_test.go` around lines 107 - 118, Replace the brittle full-map equality in TestFileViewMapCoversDocumentedValues with explicit assertions that the required keys exist and map to the expected ints: inside TestFileViewMapCoversDocumentedValues (which currently compares fileViewMap to want), iterate over the expected entries (e.g., keys "card","preview","inline" with values 1,2,3) and for each assert that fileViewMap[key] exists and equals the expected value (use t.Fatalf or t.Errorf on mismatch); do not assert that fileViewMap has no extra keys so future aliases can be added safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@shortcuts/doc/doc_media_insert_test.go`:
- Around line 107-118: Replace the brittle full-map equality in
TestFileViewMapCoversDocumentedValues with explicit assertions that the required
keys exist and map to the expected ints: inside
TestFileViewMapCoversDocumentedValues (which currently compares fileViewMap to
want), iterate over the expected entries (e.g., keys "card","preview","inline"
with values 1,2,3) and for each assert that fileViewMap[key] exists and equals
the expected value (use t.Fatalf or t.Errorf on mismatch); do not assert that
fileViewMap has no extra keys so future aliases can be added safely.
In `@shortcuts/doc/doc_media_insert.go`:
- Around line 243-245: Narrow the check that writes fileData["view_type"] by
validating fileViewType against the known enum set instead of just fileViewType
> 0; in the function that builds fileData, replace the loose positive check with
an explicit whitelist (e.g., switch or set membership) of allowed view enum
constants and only assign fileData["view_type"] when fileViewType equals one of
those known values (referencing the fileViewType variable and the fileData map
key "view_type"); this prevents unknown positive integers from being emitted in
the payload.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 08412741-0c56-4e7e-a146-270e86f20523
📒 Files selected for processing (2)
shortcuts/doc/doc_media_insert.goshortcuts/doc/doc_media_insert_test.go
Greptile SummaryThis PR adds a Confidence Score: 5/5Safe to merge — clean, backward-compatible feature addition with thorough test coverage. All findings from prior review threads are addressed. No new bugs found. The implementation is defensive (zero-value map miss → 0 → no view_type field), validation is correct, and new tests cover all branches including Validate edge cases previously missing. No files require special attention. Important Files Changed
Reviews (3): Last reviewed commit: "test(doc): cover --file-view Validate co..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Address review feedback from automated reviewers on larksuite#419: - Replace `fileViewType > 0` with an explicit 1|2|3 whitelist inside buildCreateBlockData so a stray positive int cannot escape into the request payload if a future caller bypasses Validate. - Relax TestFileViewMapCoversDocumentedValues to assert only the documented keys rather than full-map equality, so future aliases (e.g. a "player" synonym for preview) do not falsely break the test. No behaviour change for any existing --file-view input.
|
Thanks again for the thoughtful contribution and for taking the time to address the feedback. The latest changes look good, and I really appreciate the follow-up updates and added test coverage. We’d also love to have you keep following the project as it evolves. Please feel free to continue sharing any feedback, suggestions, or ideas, they’re always very welcome. |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@f8c32db75a6543d603d9df042ea96b672c0dd0b1🧩 Skill updatenpx skills add nickzhangcomes/cli#feat/docs-media-insert-view -y -g |
Address review feedback from automated reviewers on #419: - Replace `fileViewType > 0` with an explicit 1|2|3 whitelist inside buildCreateBlockData so a stray positive int cannot escape into the request payload if a future caller bypasses Validate. - Relax TestFileViewMapCoversDocumentedValues to assert only the documented keys rather than full-map equality, so future aliases (e.g. a "player" synonym for preview) do not falsely break the test. No behaviour change for any existing --file-view input.
Address review feedback from automated reviewers on #419: - Replace `fileViewType > 0` with an explicit 1|2|3 whitelist inside buildCreateBlockData so a stray positive int cannot escape into the request payload if a future caller bypasses Validate. - Relax TestFileViewMapCoversDocumentedValues to assert only the documented keys rather than full-map equality, so future aliases (e.g. a "player" synonym for preview) do not falsely break the test. No behaviour change for any existing --file-view input.
…ring The docx File block supports three render modes via view_type (1=card, 2=preview inline player, 3=inline), but --type=file today always creates with the default card view. Because view_type can only be set at creation time (PATCH replace_file ignores it), callers wanting an inline audio/video player had to abandon the shortcut and reimplement the full 4-step orchestration manually. Add --file-view card|preview|inline that threads into file.view_type on block creation. Omitting the flag preserves the exact request body that the shortcut sends today, so existing users are unaffected. --file-view is rejected when combined with --type=image (images have their own rendering) and when an unknown value is passed.
Address review feedback from automated reviewers on larksuite#419: - Replace `fileViewType > 0` with an explicit 1|2|3 whitelist inside buildCreateBlockData so a stray positive int cannot escape into the request payload if a future caller bypasses Validate. - Relax TestFileViewMapCoversDocumentedValues to assert only the documented keys rather than full-map equality, so future aliases (e.g. a "player" synonym for preview) do not falsely break the test. No behaviour change for any existing --file-view input.
Pins down the two CLI guard branches (unknown --file-view value and
--file-view passed with --type!=file) that were previously only covered
indirectly through buildCreateBlockData. Also adds the --file-view card
case so the explicit view_type=1 payload (different from the legacy
file: {} shape when the flag is omitted) is locked in as part of the
public flag contract.
Change-Id: I8c6bb69bfa22c9455a2cbb0f46b401e2cbe87762
wittam-01
force-pushed
the
feat/docs-media-insert-view
branch
from
3669613 to
f8c32db
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #419 +/- ##
==========================================
+ Coverage 59.26% 59.27% +0.01%
==========================================
Files 388 388
Lines 32983 32994 +11
==========================================
+ Hits 19546 19557 +11
Misses 11606 11606
Partials 1831 1831 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4 participants
Summary
Add a
--file-viewflag todocs +media-insertso callers can choose how a file block is rendered:card(default, current behaviour),preview(inline player for audio/video), orinline. The value is plumbed intofile.view_typewhen the block is created.Motivation
Today
docs +media-insert --type filealways creates the file block with the defaultview_type=1(card view). The docx open platform actually supports two other rendering modes for file blocks:view_type=2— preview view; renders audio/video files as an inline playerview_type=3— inline viewThese can only be set at block creation time — the PATCH
replace_fileendpoint does not acceptview_type, so once the block is created with the default the caller cannot upgrade it afterwards. Without this flag, anyone who wants an inline audio/video player has to abandon the shortcut and manually re-implement the full 4-step orchestration (query root → create block → upload → bind + rollback) vialark-cli apicalls.Changes
shortcuts/doc/doc_media_insert.gofileViewMap(card/preview/inline→1/2/3).--file-viewflag on the shortcut, validated inValidate:--file-view+--type=imagerejected (images have their own rendering).buildCreateBlockDatagains afileViewType intparameter; when> 0it setsfile.view_typeon the created child.0preserves the current behaviour (emptyfile: {}object), so servers still get exactly the same payload for existing callers.ExecuteandDryRunthread the resolved view type through tobuildCreateBlockData.shortcuts/doc/doc_media_insert_test.goTestBuildCreateBlockDataUsesConcreteAppendIndexandTestBuildCreateBlockDataForFileIncludesFilePayloadupdated to pass the new0argument; expected payload unchanged.TestBuildCreateBlockDataForFileWithPreviewViewTestBuildCreateBlockDataForFileWithInlineViewTestBuildCreateBlockDataForImageIgnoresFileViewType(verifiesview_typenever leaks into image blocks)TestFileViewMapCoversDocumentedValues(guards the public enum surface)Usage
Backward compatibility
Fully backward compatible:
--file-viewis omitted,fileViewMap[""] == 0, which skips the newif fileViewType > 0branch — the request body sent to the server is byte-for-byte identical to the current code.Test plan
go test ./shortcuts/doc/...— all existing and new tests passgo vet ./...— cleangofmt -l— clean--file-view previewwith an--type=fileupload — confirmed the resulting file block is wrapped in ablock_type=33view withview.view_type=2, as expected by the open platform spec--file-view— request body identical to current behaviour--file-view bogus→ validation error--type image --file-view preview→ validation errorRelationship to #335
#335 (
feat(doc): add --selection-with-ellipsis position flag to +media-insert) also extendsdocs +media-insertwith a new flag, but along an orthogonal axis — positioning (where to insert) vs. rendering (how it is displayed). The two changes do not overlap semantically. If #335 lands first I am happy to rebase; conflicts should be limited to theFlagsslice and theExecute/DryRunlocal variable blocks.Summary by CodeRabbit