feat(slides): inline slides reference docs into help output - #2181
feat(slides): inline slides reference docs into help output#2181ethan-zhx wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughSlides help now routes domains, methods, and shortcuts to validated reference documents. It adds deprecated command guidance, supports reference-only shortcut help, and tests route mappings, path validity, filtering, and idempotent rendering. ChangesSlides help routing
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant HelpRequest
participant AffordanceHelp
participant SlidesRouteMap
participant SkillsFilesystem
HelpRequest->>AffordanceHelp: request Slides help (domain, method, or shortcut)
AffordanceHelp->>SlidesRouteMap: resolve reference route
SlidesRouteMap->>SkillsFilesystem: validate reference path
SkillsFilesystem-->>AffordanceHelp: return valid path
AffordanceHelp-->>HelpRequest: render documentation route and guidance
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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
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@00b22920a8b37c302a46a3b54a4b3d0eea91c7c4🧩 Skill updatenpx skills add larksuite/cli#feat/help_info -y -g |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2181 +/- ##
==========================================
+ Coverage 75.70% 75.72% +0.01%
==========================================
Files 944 944
Lines 100288 100352 +64
==========================================
+ Hits 75926 75994 +68
+ Misses 18565 18563 -2
+ Partials 5797 5795 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
e31882c to
00b2292
Compare
1 participant
Summary
Add
skills readrouting tolark-cli slideshelp output, so AI agents see actionable pointers to the reference docs instead of searching blind. Covers domain help, all 12 public shortcuts, and the generatedxml_presentation.slide replacemethod.Changes
slides --help): appendSlides list routingandSlides document routesblocks explaining how to enumerate slides and where to read the XML schema reference.+create,+add-slide,+delete-slide,+xml-get,+screenshot,+media-upload,+replace-slide,+update-slide,+history-list,+history-revert,+history-revert-status): each--helpnow appends aSlides document routes:block withlark-cli skills readpointers to its matching reference doc.+replace-pages: routes to the+update-slidereplacement docs (lark-slides-update-slide.md+ edit-workflows + XML quick-ref) and emits aDeprecated: use slides +update-slidehint — no longer points to the removedlark-slides-replace-pages.md.xml_presentation.slide replace --help): appends the sameSlides document routes:block routing tolark-slides-edit-workflows.md, using the fully-qualified affordance method idxml_presentation.slide.replacefor unambiguous matching.TestSlidesShortcutReferencePathsResolvefails if any mapped reference file is missing on disk, preventing silent route drops.Test Plan
make unit-teston a compatible machine)gofmt -l .produces no outputgo vet ./cmd/service/passes./lark-cli slides +update-slide --helpshowsSlides document routes:blockRelated Issues
Summary by CodeRabbit
New Features
+replace-pagesguidance to point to+update-slide.Bug Fixes