feat(slides): export slides#988
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:
📝 WalkthroughWalkthroughThis PR adds Slides export support to the Drive shortcut: CLI description and flag enums include ChangesDrive Slides Export Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/drive/drive_export_common.go`:
- Around line 140-143: The validation allows "pptx" but
ensureExportFileExtension/exportFileSuffix lacks a mapping for it, so add a
branch in the exportFileSuffix function (the switch/if that returns suffix
strings) to return ".pptx" for the "pptx" spec.FileExtension case and ensure
ensureExportFileExtension uses that suffix when appending an extension to
spec.FileName; this restores the auto-add-extension behavior for inputs like
"slides-report".
🪄 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: 3c9b3851-da3b-4515-a4a8-1ba2e7722628
📒 Files selected for processing (5)
shortcuts/drive/drive_export.goshortcuts/drive/drive_export_common.goshortcuts/drive/drive_export_test.goskills/lark-drive/SKILL.mdskills/lark-drive/references/lark-drive-export.md
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #988 +/- ##
==========================================
- Coverage 67.76% 67.76% -0.01%
==========================================
Files 590 590
Lines 55188 55194 +6
==========================================
+ Hits 37398 37400 +2
- Misses 14679 14682 +3
- Partials 3111 3112 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@898fdbf06698bc8cc99343bdbadcea5aac50e950🧩 Skill updatenpx skills add larksuite/cli#feat/export_slides -y -g |
ethan-zhx
force-pushed
the
feat/export_slides
branch
from
734463a to
63210de
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
shortcuts/drive/drive_export_common.go (1)
140-143:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
pptxis validated but not normalized to.pptxfor local filenames.
--file-extension pptxis now accepted, butexportFileSuffixstill has nopptxmapping, soensureExportFileExtension(...)won’t append.pptxfor names like--file-name slides-report.Suggested fix
func exportFileSuffix(fileExtension string) string { switch fileExtension { case "markdown": return ".md" case "docx": return ".docx" case "pdf": return ".pdf" case "xlsx": return ".xlsx" case "csv": return ".csv" case "base": return ".base" + case "pptx": + return ".pptx" default: return "" } }Also applies to: 153-159
🤖 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_export_common.go` around lines 140 - 143, The file-extension handling validates "pptx" but never normalizes it to a filename suffix; update the mapping used by ensureExportFileExtension (look for exportFileSuffix and the function ensureExportFileExtension) to include "pptx" -> ".pptx" so that when spec.FileExtension == "pptx" a missing extension is appended; mirror the same change in the other mapping area noted around the second block (the additional mapping at lines 153-159) so both validation and filename-normalization are consistent.
🧹 Nitpick comments (1)
shortcuts/drive/drive_export_test.go (1)
53-80: ⚡ Quick winAdd a regression test for
.pptxfilename auto-suffixing.These new cases validate doc-type/extension compatibility, but they don’t verify that
--file-namegets normalized to*.pptxfor slides exports. A focused test would prevent this regression from reappearing.🤖 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_export_test.go` around lines 53 - 80, Add a regression test in drive_export_test.go that verifies filename auto-suffixing to .pptx for slides exports: extend the table-driven tests for driveExportSpec by adding a case (e.g. name "slides pptx filename auto-suffix") with DocType "slides", FileExtension "pptx" and a FileName value without ".pptx" and assert the command succeeds and the resulting exported filename (or returned path from the export function) ends with ".pptx"; also add a complementary case where FileName already ends with ".pptx" to ensure it is not double-suffixed. Use the existing driveExportSpec struct and the same test harness/assertion helpers used by the other cases to locate where to verify the normalized filename.
🤖 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.
Duplicate comments:
In `@shortcuts/drive/drive_export_common.go`:
- Around line 140-143: The file-extension handling validates "pptx" but never
normalizes it to a filename suffix; update the mapping used by
ensureExportFileExtension (look for exportFileSuffix and the function
ensureExportFileExtension) to include "pptx" -> ".pptx" so that when
spec.FileExtension == "pptx" a missing extension is appended; mirror the same
change in the other mapping area noted around the second block (the additional
mapping at lines 153-159) so both validation and filename-normalization are
consistent.
---
Nitpick comments:
In `@shortcuts/drive/drive_export_test.go`:
- Around line 53-80: Add a regression test in drive_export_test.go that verifies
filename auto-suffixing to .pptx for slides exports: extend the table-driven
tests for driveExportSpec by adding a case (e.g. name "slides pptx filename
auto-suffix") with DocType "slides", FileExtension "pptx" and a FileName value
without ".pptx" and assert the command succeeds and the resulting exported
filename (or returned path from the export function) ends with ".pptx"; also add
a complementary case where FileName already ends with ".pptx" to ensure it is
not double-suffixed. Use the existing driveExportSpec struct and the same test
harness/assertion helpers used by the other cases to locate where to verify the
normalized filename.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 068fabfb-6d12-4a2a-a60d-242fb7394dec
📒 Files selected for processing (5)
shortcuts/drive/drive_export.goshortcuts/drive/drive_export_common.goshortcuts/drive/drive_export_test.goskills/lark-drive/SKILL.mdskills/lark-drive/references/lark-drive-export.md
✅ Files skipped from review due to trivial changes (2)
- skills/lark-drive/SKILL.md
- skills/lark-drive/references/lark-drive-export.md
fangshuyu-768
left a comment
fangshuyu-768
left a comment
There was a problem hiding this comment.
Review Summary
Overall the PR is clean and well-structured. The validation logic for slides/pptx is correct, docs are updated, and test coverage is reasonable.
However, there is one Major issue that should be fixed before merging: exportFileSuffix is missing a pptx branch, which will cause the filename auto-suffixing to fail for pptx exports.
See inline comments for details.
ethan-zhx
force-pushed
the
feat/export_slides
branch
from
63210de to
f934436
Compare
fangshuyu-768
left a comment
fangshuyu-768
left a comment
There was a problem hiding this comment.
All previously raised issues have been addressed:
- [Major]
exportFileSuffixmissingpptx— Fixed.case "pptx": return ".pptx"added. - [Minor] Missing regression test for
.pptxfilename auto-suffixing — Fixed. Two test cases added indrive_export_common_test.gocovering both auto-suffixing and double-suffix prevention. - [Nit] Redundant validation — No change needed, style observation only.
LGTM.
fangshuyu-768
left a comment
fangshuyu-768
left a comment
There was a problem hiding this comment.
Blocking Issue: drive/v1/export_tasks API does not support slides type
After testing the built binary against the real API, the export call fails with:
field validation failed: type is optional, options: [doc,sheet,bitable,docx]
I also checked the official Feishu OpenAPI docs for Create Export Task. The type parameter only accepts doc, sheet, bitable, docx — slides is not a valid value.
This means the core feature of this PR (exporting slides via drive +export) cannot work with the current drive/v1/export_tasks API. The CLI correctly passes type: "slides" to the API, but the API rejects it.
Possible paths forward:
- If Feishu has a separate API for slides export (e.g., a slides-specific endpoint), the code needs to route slides exports through that API instead of
drive/v1/export_tasks. - If the
drive/v1/export_tasksAPI is being updated to supportslides, this PR should wait until that API change is live. - If neither is the case, the
slidesdoc-type should be removed from this PR to avoid shipping a broken feature.
The previous issues (missing exportFileSuffix branch, missing tests) are fixed, but this API compatibility issue is a blocker.
fangshuyu-768
left a comment
fangshuyu-768
left a comment
There was a problem hiding this comment.
Update: slides export works on feishu-pre
I previously flagged that drive/v1/export_tasks rejects type=slides. After testing against the feishu-pre environment (open.feishu-pre.cn), the API does accept slides and both PDF and PPTX exports succeed:
- PDF: 1.7 MB, downloaded successfully
- PPTX: 96 KB, downloaded successfully
The production environment (open.feishu.cn) still rejects slides, so this feature will only work once the API change rolls out to production. This is expected for a pre-release feature.
With the previous issues (missing exportFileSuffix branch, missing tests) already fixed, and the API confirmed working on feishu-pre, I'm changing my stance — this PR is ready to merge once the API is live in production.
ethan-zhx
force-pushed
the
feat/export_slides
branch
from
f934436 to
7282333
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@skills/lark-drive/SKILL.md`:
- Line 267: The table row for the `+sync` command is broken by unescaped pipe
characters inside the code span for the conflict options; update the
`--on-conflict` fragment (the code span currently split across the row) so the
entire option list is a single inline code span or escape each pipe (e.g. make
it one backtick-wrapped string like
`--on-conflict=remote-wins|local-wins|keep-both|ask` or use
`--on-conflict=remote-wins\|local-wins\|keep-both\|ask`) to prevent Markdown
from treating those pipes as column separators and restore the two-column table
row.
🪄 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: dbe30d4f-bc88-4abb-891d-dc11acc0a883
📒 Files selected for processing (6)
shortcuts/drive/drive_export.goshortcuts/drive/drive_export_common.goshortcuts/drive/drive_export_common_test.goshortcuts/drive/drive_export_test.goskills/lark-drive/SKILL.mdskills/lark-drive/references/lark-drive-export.md
✅ Files skipped from review due to trivial changes (1)
- skills/lark-drive/references/lark-drive-export.md
Change-Id: Ice3e8784e78986d427c4c94664e1e5edff2a4fcd
ethan-zhx
force-pushed
the
feat/export_slides
branch
from
7282333 to
898fdbf
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
skills/lark-drive/SKILL.md (1)
267-267:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEscape pipe characters in the
+synctable row to prevent Markdown table corruption.This row still renders as extra columns because unescaped
|appears inside the description cell.💡 Minimal doc fix
-| `+sync` | Two-way local ↔ Drive sync. Reuses `+status` diff buckets, pulls `new_remote`, pushes `new_local`, and resolves `modified` via `--on-conflict=remote-wins|local-wins|keep-both|ask`. `--quick` enables best-effort modified-time diffing (timestamp mismatches can still trigger real pull/push actions), `--on-duplicate-remote` supports `fail|newest|oldest`, and the command is intentionally non-destructive (no delete on either side). | +| `+sync` | Two-way local ↔ Drive sync. Reuses `+status` diff buckets, pulls `new_remote`, pushes `new_local`, and resolves `modified` via `--on-conflict=remote-wins\|local-wins\|keep-both\|ask`. `--quick` enables best-effort modified-time diffing (timestamp mismatches can still trigger real pull/push actions), `--on-duplicate-remote` supports `fail\|newest\|oldest`, and the command is intentionally non-destructive (no delete on either side). |🤖 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 `@skills/lark-drive/SKILL.md` at line 267, The `+sync` table row contains unescaped pipe characters inside flag examples which break the Markdown table; update that row by escaping each pipe with a backslash (e.g. change --on-conflict=remote-wins|local-wins|keep-both|ask to --on-conflict=remote-wins\|local-wins\|keep-both\|ask and --on-duplicate-remote fail|newest|oldest to --on-duplicate-remote fail\|newest\|oldest), and escape any other literal '|' characters in the same cell so the table renders as a single description cell for `+sync`.
🤖 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.
Duplicate comments:
In `@skills/lark-drive/SKILL.md`:
- Line 267: The `+sync` table row contains unescaped pipe characters inside flag
examples which break the Markdown table; update that row by escaping each pipe
with a backslash (e.g. change --on-conflict=remote-wins|local-wins|keep-both|ask
to --on-conflict=remote-wins\|local-wins\|keep-both\|ask and
--on-duplicate-remote fail|newest|oldest to --on-duplicate-remote
fail\|newest\|oldest), and escape any other literal '|' characters in the same
cell so the table renders as a single description cell for `+sync`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 11fe3806-1a46-4a5d-9c88-52ba997643a4
📒 Files selected for processing (6)
shortcuts/drive/drive_export.goshortcuts/drive/drive_export_common.goshortcuts/drive/drive_export_common_test.goshortcuts/drive/drive_export_test.goskills/lark-drive/SKILL.mdskills/lark-drive/references/lark-drive-export.md
✅ Files skipped from review due to trivial changes (1)
- skills/lark-drive/references/lark-drive-export.md
2 participants
Summary
Add Slides export support to drive +export, allowing Feishu/Lark slides documents to be exported as pptx or pdf.
Improve resilience by providing actionable follow-up commands when async export polling times out or download
fails, and update docs/tests accordingly.
Changes
+export (shortcuts/drive/drive_export.go).
combinations (shortcuts/drive/drive_export_common.go).
documentation (skills/lark-drive/*).
Test Plan
[✓] Unit tests pass (make unit-test)
[ ] Manual local verification confirms the lark xxx command works as expected
Related Issues
• None
Summary by CodeRabbit
New Features
Documentation
Tests