feat: add iconpark lookup for lark slides - #1123
Conversation
📝 WalkthroughWalkthroughThis PR adds a new IconPark icon search and resolution tool for Lark Slides, along with comprehensive developer documentation. The implementation includes a 362-line Python CLI tool with multi-language query tokenization, relevance scoring, and three core commands (search, resolve, list-categories), validated by a 99-line test suite, and documented across process guidelines and reference materials. ChangesIconPark Icon Search and Integration
Sequence Diagram(s)sequenceDiagram
participant Developer
participant CLI as iconpark_tool.py
participant Loader as load_index()
participant Tokenizer as tokenize_query()
participant Scorer as score_icon()
participant Index as Index JSON
Developer->>CLI: search --query "arrow"
CLI->>Loader: load_index()
Loader->>Index: read icons.json
Index-->>Loader: icon entries
Loader-->>CLI: index_data
CLI->>Tokenizer: tokenize_query("arrow")
Tokenizer-->>CLI: tokens: ["arrow"]
CLI->>Scorer: rank candidates by score(token)
loop For each icon in index
Scorer->>Scorer: exact match, field match, substring, boost
end
Scorer-->>CLI: ranked results
CLI-->>Developer: JSON results [{"iconType": "...", ...}]
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1123 +/- ##
=======================================
Coverage 71.14% 71.14%
=======================================
Files 685 685
Lines 65770 65770
=======================================
Hits 46793 46793
Misses 15326 15326
Partials 3651 3651 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@eddb0bf8fecffcaa6c8004cfd87389287c57b6da🧩 Skill updatenpx skills add larksuite/cli#feat/slide_icon_park -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
skills/lark-slides/scripts/iconpark_tool.py (2)
50-51: ⚡ Quick winAdd
NoReturntype annotation for proper type checking.The
fail()function always raises but lacks a return type annotation. Type checkers will infer-> None, causing spurious "missing return" warnings in callers likeparse_limit()andload_index().Proposed fix
+from typing import Any, NoReturn -from typing import Any -def fail(message: str) -> None: +def fail(message: str) -> NoReturn: raise IconParkToolError(message)🤖 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-slides/scripts/iconpark_tool.py` around lines 50 - 51, The fail() helper currently lacks a NoReturn annotation which causes type-checkers to infer -> None and produce "missing return" warnings in callers like parse_limit() and load_index(); update the fail function signature to def fail(message: str) -> NoReturn: and add the required import from typing (NoReturn), leaving the implementation raise IconParkToolError(message) unchanged so callers are correctly treated as unreachable after calling fail().
214-220: 💤 Low valueConsider precomputing tag tokens to avoid repeated work.
field_tokens(tag)is called inside nested loops, recomputing the same token set for each query token. For a query with 10 tokens and an icon with 5 tags, this results in 50 calls instead of 5.Proposed optimization
name_tokens = field_tokens(name) category_tokens = field_tokens(category) tag_tokens = field_tokens(*tags) icon_type_tokens = field_tokens(icon_type) + tag_token_sets = [field_tokens(tag) for tag in tags] search_text = icon_search_text(entry) ... - for tag in tags: + for i, tag in enumerate(tags): if token == tag: score += 60 - elif token in field_tokens(tag): + elif token in tag_token_sets[i]: score += 45 elif allows_substring_match(token) and token in tag: score += 20🤖 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-slides/scripts/iconpark_tool.py` around lines 214 - 220, The nested scoring loop calls field_tokens(tag) repeatedly for each query token; to fix, compute the token set for each tag once before iterating tokens: inside the outer loop over tags (the block using variables tag and score and calling field_tokens and allows_substring_match), call field_tokens(tag) a single time (e.g., tag_token_set) and reuse it for equality/in-set checks, and use the original tag string only for substring checks (allows_substring_match(token) and token in tag); this eliminates repeated field_tokens calls and preserves the existing scoring logic.
🤖 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 `@skills/lark-slides/scripts/iconpark_tool.py`:
- Around line 50-51: The fail() helper currently lacks a NoReturn annotation
which causes type-checkers to infer -> None and produce "missing return"
warnings in callers like parse_limit() and load_index(); update the fail
function signature to def fail(message: str) -> NoReturn: and add the required
import from typing (NoReturn), leaving the implementation raise
IconParkToolError(message) unchanged so callers are correctly treated as
unreachable after calling fail().
- Around line 214-220: The nested scoring loop calls field_tokens(tag)
repeatedly for each query token; to fix, compute the token set for each tag once
before iterating tokens: inside the outer loop over tags (the block using
variables tag and score and calling field_tokens and allows_substring_match),
call field_tokens(tag) a single time (e.g., tag_token_set) and reuse it for
equality/in-set checks, and use the original tag string only for substring
checks (allows_substring_match(token) and token in tag); this eliminates
repeated field_tokens calls and preserves the existing scoring logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b7b48cca-bbfa-4d26-99de-3a5927d18a0f
📒 Files selected for processing (7)
skills/lark-slides/SKILL.mdskills/lark-slides/references/iconpark-index.jsonskills/lark-slides/references/iconpark.mdskills/lark-slides/references/lark-slides-replace-slide.mdskills/lark-slides/references/xml-schema-quick-ref.mdskills/lark-slides/scripts/iconpark_tool.pyskills/lark-slides/scripts/iconpark_tool_test.py
fangshuyu-768
left a comment
fangshuyu-768
left a comment
There was a problem hiding this comment.
Overall I did not find a blocking functional bug in this PR, but I do see a few non-blocking follow-ups worth tracking:
-
Testing coverage
The new test file exercises the helper APIs well, but it does not cover the CLI contract yet. It would be helpful to add a few end-to-end CLI tests forsearch,resolve,list-categories,--help, and invalid argument handling, so future refactors do not silently change exit codes or stderr behavior. -
Ranking maintainability
iconpark_tool.pyrelies on a heuristic combination ofCURATED_ICON_BOOSTS, token expansion, and the generated offline index. That looks reasonable for now, but ranking behavior can drift when the index is regenerated or when boost rules are adjusted. A small set of deterministic top-result contract tests for the highest-value queries would make this much safer to evolve. -
Data refresh risk
Because this feature depends on a large generatediconpark-index.json, the long-term correctness is partly a data maintenance problem, not only a code problem. I would recommend documenting the regeneration/update workflow for that index somewhere near the script or reference docs, so future updates stay reproducible and reviewers can tell whether a ranking change came from code or from data refresh.
None of these look blocking to me for the current diff, but they are the main deeper risks I would keep an eye on.
0ea840a to
3a4c25d
Compare
3a4c25d to
eddb0bf
Compare
2 participants
Summary
Add IconPark lookup support for the lark-slides skill, including reference docs, index data, and a helper script for searching and selecting icons.
Changes
iconpark_tool.pywith lookup helpers and tests.Test Plan
lark-cli <domain> <command>flow works as expectedRelated Issues
Summary by CodeRabbit
New Features
Documentation