feat(slides): inline slides reference docs into help output - #2181
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:
📝 WalkthroughWalkthroughThe PR adds the ChangesSlides documentation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 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
the
size/L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
cmd/service/affordance.go (1)
269-278: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the shared route renderer and reuse it for the domain block.
PrepareMethodHelpat Line 343 callsappendSlidesShortcutReferenceRoutes, so the function is no longer shortcut-specific.appendSlidesDomainRoutingHintsat Lines 229-232 also re-implements the same "Slides document routes:" header and indented list. Rename the helper toappendSlidesDocumentRoutesand call it from the domain path so one function owns the block format.♻️ Proposed consolidation
-func appendSlidesShortcutReferenceRoutes(b *strings.Builder, routes []string) { +func appendSlidesDocumentRoutes(b *strings.Builder, routes []string) { if len(routes) == 0 { return } b.WriteString("\n\nSlides document routes:") for _, route := range routes { b.WriteString("\n ") b.WriteString(route) } }Then reuse it in
appendSlidesDomainRoutingHints:routes := slidesDocumentRoutes(skillFS, []string{ "lark-slides/SKILL.md", slidesXMLQuickReferencePath, }) - if len(routes) == 0 { - return - } - b.WriteString("\n\nSlides document routes:") - for _, route := range routes { - fmt.Fprintf(b, "\n %s", route) - } + appendSlidesDocumentRoutes(b, routes) }🤖 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 `@cmd/service/affordance.go` around lines 269 - 278, Rename appendSlidesShortcutReferenceRoutes to appendSlidesDocumentRoutes, update the PrepareMethodHelp call to use the new name, and replace the duplicated “Slides document routes:” rendering in appendSlidesDomainRoutingHints with this shared helper.cmd/service/affordance_test.go (2)
393-401: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the exact route count so the table catches added routes.
The subtest builds
skillFSfromslidesShortcutReferencePaths[command], which is the same map the table is meant to pin. The assertion only checks containment, so an extra path added to the production map produces an extra entry ingotand the test still passes. Compare the length as well, asTestSlidesScreenshotHelpDoesNotIncludeXMLQuickReferencealready does at Lines 484-486.💚 Proposed assertion
got, ok := readSlidesShortcutReferenceRoutes(sc, skillFS) if !ok || len(got) == 0 { t.Fatalf("shortcut %q has no mapped reference", command) } + if len(got) != len(routes) { + t.Fatalf("shortcut %q routes = %#v, want exactly %#v", command, got, routes) + } for _, route := range routes { if !containsString(got, route) { t.Fatalf("shortcut %q routes = %#v, want %q", command, got, route) } }🤖 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 `@cmd/service/affordance_test.go` around lines 393 - 401, Update the subtest around readSlidesShortcutReferenceRoutes to assert that len(got) equals len(routes) before checking route containment, matching the exact-count validation used by TestSlidesScreenshotHelpDoesNotIncludeXMLQuickReference. Keep the existing non-empty and per-route assertions.Source: Coding guidelines
304-315: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStub
affordanceLookupso the reference-only branch is actually pinned.This test leaves the package-level
affordanceLookupat its real implementation and still callscmdmeta.SetAffordanceRef(sc, "slides", "+xml-get"). If an overlay exists for that service/method pair,hasAffordancebecomes true andPrepareShortcutHelpno longer takes the!hasAffordance && hasReferenceRoutespath that this test is named for. Reverting the new allowance inPrepareShortcutHelpwould then not fail this test. Force the no-affordance state, as the sibling tests at Lines 116-120 and Lines 187-194 already do.As per coding guidelines: "contract tests must assert the changed field or behavior directly so reverting the implementation causes failure".
💚 Proposed test hardening
func TestPrepareShortcutHelp_SlidesReferenceRouteWithoutAffordance(t *testing.T) { + orig := affordanceLookup + t.Cleanup(func() { affordanceLookup = orig }) + affordanceLookup = func(_, _ string) (json.RawMessage, bool) { return nil, false } + sc := &cobra.Command{Use: "+xml-get", Short: "Fetch presentation XML"} cmdmeta.SetSource(sc, cmdmeta.SourceShortcut, false) cmdmeta.SetDomain(sc, "slides") cmdmeta.SetAffordanceRef(sc, "slides", "+xml-get") cmdutil.SetRisk(sc, "read")🤖 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 `@cmd/service/affordance_test.go` around lines 304 - 315, Update TestPrepareShortcutHelp_SlidesReferenceRouteWithoutAffordance to stub the package-level affordanceLookup, forcing the slides/+xml-get lookup to report no affordance before calling PrepareShortcutHelp. Follow the stubbing pattern used by the sibling tests around the existing affordanceLookup setup, and assert the reference-only behavior so reverting the PrepareShortcutHelp allowance causes this test to fail.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.
Nitpick comments:
In `@cmd/service/affordance_test.go`:
- Around line 393-401: Update the subtest around
readSlidesShortcutReferenceRoutes to assert that len(got) equals len(routes)
before checking route containment, matching the exact-count validation used by
TestSlidesScreenshotHelpDoesNotIncludeXMLQuickReference. Keep the existing
non-empty and per-route assertions.
- Around line 304-315: Update
TestPrepareShortcutHelp_SlidesReferenceRouteWithoutAffordance to stub the
package-level affordanceLookup, forcing the slides/+xml-get lookup to report no
affordance before calling PrepareShortcutHelp. Follow the stubbing pattern used
by the sibling tests around the existing affordanceLookup setup, and assert the
reference-only behavior so reverting the PrepareShortcutHelp allowance causes
this test to fail.
In `@cmd/service/affordance.go`:
- Around line 269-278: Rename appendSlidesShortcutReferenceRoutes to
appendSlidesDocumentRoutes, update the PrepareMethodHelp call to use the new
name, and replace the duplicated “Slides document routes:” rendering in
appendSlidesDomainRoutingHints with this shared helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0958d71e-0a45-459d-a37a-677a6f0eefb1
📒 Files selected for processing (2)
cmd/service/affordance.gocmd/service/affordance_test.go
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@3fb5c6f15d08e0fe7b8e442bf227c21ff3894d67🧩 Skill updatenpx skills add larksuite/cli#feat/help_info -y -g |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2181 +/- ##
=======================================
Coverage 76.51% 76.52%
=======================================
Files 1019 1019
Lines 112614 112614
=======================================
+ Hits 86172 86176 +4
+ Misses 19873 19869 -4
Partials 6569 6569 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
e31882c to
00b2292
Compare
00b2292 to
75b3209
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
github-actions
Bot
added
size/S
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 `@affordance/slides.md`:
- Around line 59-64: Add a deprecation note to the +replace-pages section in
affordance/slides.md stating that new requests must use +update-slide while
+replace-pages remains available for compatibility.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
75b3209 to
a578101
Compare
github-actions
Bot
added
size/M
a578101 to
e8419f0
Compare
e8419f0 to
3fb5c6f
Compare
fangshuyu-768
left a comment
fangshuyu-768
left a comment
There was a problem hiding this comment.
Verified the updated Slides affordance routing and source coverage. The focused tests and available CI checks pass.
3 participants
Summary
Move
lark-cli slideshelp output from Go hardcoded blocks to declarative markdown inaffordance/slides.md, using the new domain-level## Skillssection and per-command### Skillsblocks. This lets AI agents see actionable pointers to reference docs instead of searching blind.Changes
slides --help): the canonical> skill: lark-slides(SKILL.md) is shown first automatically; the## Skillsdomain section adds a pointer toxml-schema-quick-ref.mdas a display-only extra entry.+create,+add-slide,+delete-slide,+xml-get,+screenshot,+media-upload,+replace-slide,+update-slide,+history-list,+history-revert,+history-revert-status): each command's### Skillsblock routes to its matching reference doc, with paths corrected to the refactored subdirectory structure (cli/,xml/,workflow/).+replace-pagesand hidden alias+update: both route to the+update-slidereplacement docs.appendSlidesDomainRoutingHints,slidesDocumentRoutes,slidesSkillReadPath, and theslidesSkillName/slidesXMLQuickReferencePathconstants fromcmd/service/affordance.go(replaced bywriteDomainSkills). Removed the staleTestPrepareDomainHelp_SlidesIncludesDocumentRoutestest.Test Plan
./lark-cli slides --helpshowsDomain skillswith both SKILL.md and xml-quick-ref--helpoutputs show correctRelated skillsentries with resolved pathsaffordance/slides.mdresolve to existing files (no stale flat paths)Related Issues
Summary by CodeRabbit
New Features
+updatealias for slide updates.Documentation