feat(doc): add --file-view flag to +media-insert#419
feat(doc): add --file-view flag to +media-insert#419nickzhangcomes wants to merge 3 commits intolarksuite:mainfrom
Conversation
…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.
|
|
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 (1)
📝 WalkthroughWalkthroughAdded a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
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.
| if docRef.Kind == "doc" { | ||
| return output.ErrValidation("docs +media-insert only supports docx documents; use a docx token/URL or a wiki URL that resolves to docx") | ||
| } | ||
| if view := runtime.Str("file-view"); view != "" { |
There was a problem hiding this comment.
Nice addition overall. One gap I noticed is that the new tests only exercise buildCreateBlockData, but the main user-facing contract here lives in Validate: rejecting unknown --file-view values and rejecting --file-view when --type != file.
Could we add focused tests for those validation paths as well? Otherwise a future refactor could silently relax the CLI contract without any test catching it.
There was a problem hiding this comment.
Good call, that gap was bugging me too after you pointed it out. Pushed a table-driven test in 3669613 that runs Validate directly and covers the two rejection branches (unknown value like --file-view bogus, and --file-view preview with --type image), plus the accepted shapes as positive cases so the whole guard is locked down. Future Validate refactors will have to touch these explicitly.
| } | ||
| } | ||
|
|
||
| func TestBuildCreateBlockDataForFileWithPreviewView(t *testing.T) { |
There was a problem hiding this comment.
Could we add coverage for the explicit --file-view card path too?
Omitting --file-view keeps the legacy payload (file: {}), but --file-view card now sends file.view_type = 1. Those are different request shapes, even if they are intended to be semantically equivalent. A small unit test here would help lock down that public flag behavior.
There was a problem hiding this comment.
Agreed, and the divergence is on purpose: omitting the flag keeps the legacy file: {} shape for backward compat, while --file-view card sends an explicit view_type: 1 because the user literally asked for card. I didn't want to silently drop the field and make the flag non-idempotent with its own echo. Added TestBuildCreateBlockDataForFileWithCardView in 3669613 so that expectation is pinned, not just reviewer memory.
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.
3 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