feat: support wiki node target in markdown +create#883
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:
📝 WalkthroughWalkthroughAdds WikiToken support to markdown +create, derives parent_type/parent_node via Target(), enforces mutual exclusivity with folder-token, updates dry-run/upload payloads to use the computed target, wires the CLI flag, and adds unit, E2E tests, and docs. ChangesWiki-parent markdown creation support
Sequence DiagramsequenceDiagram
participant CLI as CLI (user)
participant Shortcut as MarkdownCreate shortcut
participant Spec as markdownUploadSpec.Target()
participant API as Upload API (upload_all / upload_prepare)
participant Storage as Storage (Drive or Wiki)
CLI->>Shortcut: invoke +create with flags (--file, --folder-token, --wiki-token)
Shortcut->>Spec: construct spec with FolderToken/WikiToken
Spec->>API: include parent_type/parent_node in request body
API->>Storage: store file under parent_type/parent_node
Storage-->>API: return file_token (wiki: no /file/<token> URL)
API-->>Shortcut: response payload
Shortcut-->>CLI: print output (omit url for wiki)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
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)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
shortcuts/markdown/helpers.go (1)
82-84: ⚡ Quick winClarify the error message for empty wiki-token.
The error message suggests "omit it to upload into Drive root folder," which is confusing when the user explicitly provided
--wiki-token. If they set--wiki-token, they intended wiki upload, not Drive folder upload.Consider revising to focus solely on wiki-token usage, e.g.,
"--wiki-token cannot be empty; provide a valid wiki node token or omit the flag entirely".📝 Proposed message refinement
if markdownFlagExplicitlyEmpty(runtime, "wiki-token") { - return common.FlagErrorf("--wiki-token cannot be empty; omit it to upload into Drive root folder or pass a wiki node token") + return common.FlagErrorf("--wiki-token cannot be empty; provide a valid wiki node token or omit the flag entirely") }🤖 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/markdown/helpers.go` around lines 82 - 84, Update the error text emitted when markdownFlagExplicitlyEmpty(runtime, "wiki-token") is true: replace the confusing Drive-root suggestion with a focused message such as "--wiki-token cannot be empty; provide a valid wiki node token or omit the flag entirely" so users who passed --wiki-token understand they must supply a non-empty token or remove the flag; change the string passed to common.FlagErrorf in that conditional accordingly.
🤖 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 `@tests/cli_e2e/markdown/markdown_dryrun_test.go`:
- Around line 143-145: The test currently asserts the exit code via
result.AssertExitCode(t, 2) but only checks result.Stderr for the validation
message; update the assertion to verify the combined output (result.Stdout +
result.Stderr) contains the expected text "--wiki-token cannot be empty" so
validate-stage dry-run errors emitted to stdout JSON are caught; keep the
exit-code assertion (result.AssertExitCode) and replace assert.Contains(t,
result.Stderr, ...) with assert.Contains(t, result.Stdout+result.Stderr,
"--wiki-token cannot be empty") referencing the same result variable in
markdown_dryrun_test.go.
In `@tests/cli_e2e/markdown/markdown_workflow_test.go`:
- Around line 169-182: The test currently only logs when both cleanup attempts
fail, leaving artifacts; replace the final parentT.Logf call with a test failure
so the E2E test fails if neither identity could delete the file. Specifically,
in the cleanup loop around clie2e.RunCmd (using variables identity, cleanupCtx,
cleanupCancel and function clie2e.RunCmd) ensure cleanupCancel is still called
and then call parentT.Fatalf (or equivalent fail method on parentT) with a
message that includes fileToken to fail the test when both delete attempts
return non-zero/err.
---
Nitpick comments:
In `@shortcuts/markdown/helpers.go`:
- Around line 82-84: Update the error text emitted when
markdownFlagExplicitlyEmpty(runtime, "wiki-token") is true: replace the
confusing Drive-root suggestion with a focused message such as "--wiki-token
cannot be empty; provide a valid wiki node token or omit the flag entirely" so
users who passed --wiki-token understand they must supply a non-empty token or
remove the flag; change the string passed to common.FlagErrorf in that
conditional accordingly.
🪄 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: b3579146-97af-48cc-80e7-103cd8ff22a2
📒 Files selected for processing (6)
shortcuts/markdown/helpers.goshortcuts/markdown/markdown_create.goshortcuts/markdown/markdown_test.goskills/lark-markdown/references/lark-markdown-create.mdtests/cli_e2e/markdown/markdown_dryrun_test.gotests/cli_e2e/markdown/markdown_workflow_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #883 +/- ##
==========================================
+ Coverage 67.58% 67.66% +0.07%
==========================================
Files 575 576 +1
Lines 54269 54510 +241
==========================================
+ Hits 36679 36885 +206
- Misses 14548 14566 +18
- Partials 3042 3059 +17 ☔ 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@95f569df1b772e5c23981455abfb34153ecaa1b8🧩 Skill updatenpx skills add larksuite/cli#feat/markdown-create-wiki-target -y -g |
fdda288 to
3f359ec
Compare
8c83f3a to
d23181a
Compare
fangshuyu-768
left a comment
There was a problem hiding this comment.
Thanks for the PR! Overall the implementation is clean and well-tested. I have a few comments below — two are worth investigating before merge, the rest are minor suggestions.
Change-Id: Idb89464344599571cda3d27d136727553dcf0e7e
d23181a to
95f569d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
shortcuts/markdown/markdown_test.go (2)
555-556: ⚡ Quick winAvoid using production constants as the expected value in these assertions.
Using
markdownUploadParentTypeWikiin the expected side couples the test oracle to implementation details; if the constant drifts, tests can still pass while the API contract breaks. Prefer asserting the literal wire value"wiki".Suggested diff
- if got := body.Fields["parent_type"]; got != markdownUploadParentTypeWiki { - t.Fatalf("parent_type = %q, want %q", got, markdownUploadParentTypeWiki) + if got := body.Fields["parent_type"]; got != "wiki" { + t.Fatalf("parent_type = %q, want %q", got, "wiki") } ... - if got := body["parent_type"]; got != markdownUploadParentTypeWiki { - t.Fatalf("parent_type = %#v, want %q", got, markdownUploadParentTypeWiki) + if got := body["parent_type"]; got != "wiki" { + t.Fatalf("parent_type = %#v, want %q", got, "wiki") }Also applies to: 746-747
🤖 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/markdown/markdown_test.go` around lines 555 - 556, Replace the use of the production constant markdownUploadParentTypeWiki in the test assertions with the literal wire value "wiki" so the test oracle verifies the actual API contract; specifically update the comparisons that check body.Fields["parent_type"] (and the other occurrence that currently asserts against markdownUploadParentTypeWiki) to use the string "wiki" as the expected value.
561-563: ⚡ Quick winAdd a positive stdout assertion in the wiki success tests.
These checks only verify URL absence. Add a positive assertion (e.g.,
file_tokenexists) so empty/partial stdout regressions don’t pass.Suggested diff
if strings.Contains(stdout.String(), `"url":`) { t.Fatalf("stdout should omit url for wiki-hosted markdown files: %s", stdout.String()) } + if !strings.Contains(stdout.String(), `"file_token": "box_md_create_wiki"`) { + t.Fatalf("stdout missing file_token: %s", stdout.String()) + } ... if strings.Contains(stdout.String(), `"url":`) { t.Fatalf("stdout should omit url for wiki-hosted multipart markdown files: %s", stdout.String()) } + if !strings.Contains(stdout.String(), `"file_token": "box_md_multipart_wiki"`) { + t.Fatalf("stdout missing file_token: %s", stdout.String()) + }Also applies to: 752-754
🤖 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/markdown/markdown_test.go` around lines 561 - 563, The test currently only asserts that stdout does not contain `"url":` which can miss empty/partial output regressions; update the wiki success tests (the block that checks stdout via stdout.String() and uses strings.Contains) to also assert a positive indicator is present (for example, verify stdout contains `"file_token"` or another expected token) so the test fails on empty output; make the same change to the second occurrence of the same pattern (the other wiki success test around the later check currently at lines 752-754).
🤖 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 `@shortcuts/markdown/markdown_test.go`:
- Around line 555-556: Replace the use of the production constant
markdownUploadParentTypeWiki in the test assertions with the literal wire value
"wiki" so the test oracle verifies the actual API contract; specifically update
the comparisons that check body.Fields["parent_type"] (and the other occurrence
that currently asserts against markdownUploadParentTypeWiki) to use the string
"wiki" as the expected value.
- Around line 561-563: The test currently only asserts that stdout does not
contain `"url":` which can miss empty/partial output regressions; update the
wiki success tests (the block that checks stdout via stdout.String() and uses
strings.Contains) to also assert a positive indicator is present (for example,
verify stdout contains `"file_token"` or another expected token) so the test
fails on empty output; make the same change to the second occurrence of the same
pattern (the other wiki success test around the later check currently at lines
752-754).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 19ef8ae6-2865-45c7-b752-ddba6804aa56
📒 Files selected for processing (6)
shortcuts/markdown/helpers.goshortcuts/markdown/markdown_create.goshortcuts/markdown/markdown_test.goskills/lark-markdown/references/lark-markdown-create.mdtests/cli_e2e/markdown/markdown_dryrun_test.gotests/cli_e2e/markdown/markdown_workflow_test.go
✅ Files skipped from review due to trivial changes (1)
- skills/lark-markdown/references/lark-markdown-create.md
Change-Id: Idb89464344599571cda3d27d136727553dcf0e7e
Summary
Add
--wiki-tokensupport tomarkdown +createso Markdown files can be created directly under a Wiki node instead of only Drive folders. The implementation stays scoped toshortcuts/markdownand aligns the command behavior with existing Drive wiki-upload semantics.Changes
--wiki-tokensupport, target validation, andparent_type=wikirouting tomarkdown +create/file/<token>URL for wiki-hosted Markdown files, and update command help / skill reference docsTest Plan
go test ./shortcuts/markdown -count=1,go test ./tests/cli_e2e/markdown -count=1,make unit-test)lark xxxcommand works as expected (./lark-cli markdown +create --as bot --wiki-token ...with both--contentand--file, followed by./lark-cli markdown +fetch)Related Issues
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests