Skip to content

feat: support PDF imports as slides#1327

Closed
ethan-zhx wants to merge 1 commit into
mainfrom
feat/import_pdf_as_slides
Closed

feat: support PDF imports as slides#1327
ethan-zhx wants to merge 1 commit into
mainfrom
feat/import_pdf_as_slides

Conversation

@ethan-zhx

@ethan-zhx ethan-zhx commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adds support for importing PDF files as Slides through the drive import flow.

Changes

  • Map PDF imports to the slides import type.
  • Update drive import docs and E2E coverage notes.
  • Add common import tests and dry-run E2E coverage for PDF-to-slides request structure.

Test Plan

  • Unit tests pass
  • Manual local verification confirms the lark-cli drive +import flow works as expected
  • Not run locally per repository instruction to avoid build/test commands unless explicitly requested.

Related Issues

  • None

Summary by CodeRabbit

  • New Features

    • Added PDF import support for slides destinations, with a 500MB non-multipart upload cap.
  • Documentation

    • Updated docs with PDF→slides examples, conversion mapping, and updated size/validation guidance.
  • Tests

    • Added tests for PDF import validation, size limits, and a dry-run PDF→slides scenario.
  • Chores

    • Updated CLI help and validation messages and refreshed CLI e2e coverage metrics.

@coderabbitai

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c871e6e7-0d2a-415e-bbc9-4fc5a8a39226

📥 Commits

Reviewing files that changed from the base of the PR and between 9edaeb9 and a84fe91.

📒 Files selected for processing (7)
  • shortcuts/drive/drive_import.go
  • shortcuts/drive/drive_import_common.go
  • shortcuts/drive/drive_import_common_test.go
  • skills/lark-drive/references/lark-drive-import.md
  • skills/lark-slides/SKILL.md
  • tests/cli_e2e/drive/coverage.md
  • tests/cli_e2e/drive/drive_import_dryrun_test.go
✅ Files skipped from review due to trivial changes (4)
  • skills/lark-slides/SKILL.md
  • shortcuts/drive/drive_import.go
  • tests/cli_e2e/drive/coverage.md
  • skills/lark-drive/references/lark-drive-import.md
🚧 Files skipped from review as they are similar to previous changes (3)
  • tests/cli_e2e/drive/drive_import_dryrun_test.go
  • shortcuts/drive/drive_import_common_test.go
  • shortcuts/drive/drive_import_common.go

📝 Walkthrough

Walkthrough

This PR extends the drive +import command to support PDF files for conversion to Feishu slides, mirroring existing PPTX functionality. Changes include validation rules, file-size constraints (500MB), CLI documentation, unit and integration tests, and coverage tracking updates.

Changes

PDF Support for Drive Import Feature

Layer / File(s) Summary
Core PDF support in drive import
shortcuts/drive/drive_import.go, shortcuts/drive/drive_import_common.go
drive_import_common.go adds PDF to the extension-to-document-type compatibility map for slides, enforces the 500MB size limit matching PPTX, groups PDF with PPTX in file-type mismatch hints, and updates error messages to reference PDF. The --file flag description in drive_import.go is updated to list PDF among supported formats and clarify size limits.
Unit tests for PDF validation
shortcuts/drive/drive_import_common_test.go
TestValidateDriveImportSpec and TestValidateDriveImportFileSize are extended with test cases validating PDF acceptance when importing as slides, rejection for non-slides targets, and enforcement of the 500MB file-size limit.
User documentation for PDF import
skills/lark-drive/references/lark-drive-import.md
Documentation updated to list PDF among supported local files, adds CLI examples for PDF-to-slides import, extends the file-type-to-target conversion table with .pdfslides, updates validation rules to cover PDF alongside PPTX, and reflects the 500MB size cap for both formats.
End-to-end test and coverage tracking
tests/cli_e2e/drive/drive_import_dryrun_test.go, tests/cli_e2e/drive/coverage.md
New TestDriveImportDryRun_PDFToSlides test validates the complete dry-run flow: creates a temporary PDF, runs the import command with --type slides --dry-run, and asserts successful exit and correct API payload structure (media upload endpoint with PDF filename and extra metadata, followed by import task with PDF extension and slides type). Coverage metrics and command table updated to reflect the new test case.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

  • larksuite/cli#1169: Related docs/routing changes for skills/lark-slides exclusions and delegation to lark-drive for local imports.
  • larksuite/cli#1068: Changes touching the same drive import validation/extension→type mapping and size-limit logic.
  • larksuite/cli#599: Prior extension-of-supported formats in drive import validation and tests; overlaps with the same code paths.

Suggested reviewers

  • fangshuyu-768

Poem

🐰 A PDF hops in, joins slides' grand scheme,
Five hundred megs, the upload dream.
Validation lined up, messages clear,
Tests run a dry-run, no errors appear.
Hooray — a rabbit-approved import cheer!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: adding PDF import support for slides via the drive import feature.
Description check ✅ Passed The description follows the template with all required sections: Summary, Changes, Test Plan, and Related Issues. It adequately documents the PR objectives and testing strategy.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/import_pdf_as_slides

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions Bot added domain/ccm PR touches the ccm domain size/L Large or sensitive change across domains or core paths labels Jun 8, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
shortcuts/drive/drive_import.go (1)

36-44: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add safe-path validation before any filesystem access for --file.

--file comes from CLI input and is used in fio.Stat(...) without an explicit safe-path validation gate in this flow. Please validate the input path before preflight/stat/read to enforce the CLI trust boundary consistently.

As per coding guidelines: “Validate paths before reading with validate.SafeInputPath because CLI arguments are untrusted (they come from AI agents).”

Also applies to: 159-165

🤖 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/drive/drive_import.go` around lines 36 - 44, The CLI-provided file
path (runtime.Str("file")) is not being checked with the safe-path validator
before filesystem operations; update the Validate closure that builds the
driveImportSpec (and the corresponding preflight/Stat/read paths referenced
later) to call validate.SafeInputPath on the FilePath value and return an error
if it fails; specifically, ensure validateDriveImportSpec (or the Validate func
that constructs driveImportSpec) enforces validate.SafeInputPath(FilePath) prior
to any fio.Stat/fio.Read usage so the untrusted --file input is rejected when
unsafe.

Source: Coding guidelines

🤖 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/drive/drive_import.go`:
- Around line 36-44: The CLI-provided file path (runtime.Str("file")) is not
being checked with the safe-path validator before filesystem operations; update
the Validate closure that builds the driveImportSpec (and the corresponding
preflight/Stat/read paths referenced later) to call validate.SafeInputPath on
the FilePath value and return an error if it fails; specifically, ensure
validateDriveImportSpec (or the Validate func that constructs driveImportSpec)
enforces validate.SafeInputPath(FilePath) prior to any fio.Stat/fio.Read usage
so the untrusted --file input is rejected when unsafe.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ddd70acc-4033-4420-a128-0cbe1703d55a

📥 Commits

Reviewing files that changed from the base of the PR and between 99ceb22 and 9edaeb9.

📒 Files selected for processing (6)
  • shortcuts/drive/drive_import.go
  • shortcuts/drive/drive_import_common.go
  • shortcuts/drive/drive_import_common_test.go
  • skills/lark-drive/references/lark-drive-import.md
  • tests/cli_e2e/drive/coverage.md
  • tests/cli_e2e/drive/drive_import_dryrun_test.go

@codecov

codecov Bot commented Jun 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 71.95%. Comparing base (99ceb22) to head (a84fe91).
⚠️ Report is 20 commits behind head on main.

Files with missing lines Patch % Lines
shortcuts/drive/drive_import_common.go 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1327      +/-   ##
==========================================
+ Coverage   71.47%   71.95%   +0.47%     
==========================================
  Files         688      699      +11     
  Lines       65482    66164     +682     
==========================================
+ Hits        46806    47611     +805     
+ Misses      15031    14864     -167     
- Partials     3645     3689      +44     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown

🚀 PR Preview Install Guide

🧰 CLI update

npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@a84fe91fa12bf3ce22f7a97ea2c85cb9e9578dee

🧩 Skill update

npx skills add larksuite/cli#feat/import_pdf_as_slides -y -g

@ethan-zhx
ethan-zhx force-pushed the feat/import_pdf_as_slides branch from 9edaeb9 to a84fe91 Compare June 10, 2026 07:24
@ethan-zhx ethan-zhx closed this Jul 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain/ccm PR touches the ccm domain size/L Large or sensitive change across domains or core paths

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant