Move block creation/updating to sub-agents and update docs#4
Move block creation/updating to sub-agents and update docs#4ZmeiGorynych wants to merge 1 commit intomainfrom
Conversation
📝 WalkthroughWalkthroughThis PR introduces a new Changes
Sequence DiagramsequenceDiagram
participant User
participant block-modifier as block-modifier Agent
participant CubeTool as explore-cube Tools
participant UpdateTools as Update Tools<br/>(chart/query/text/table)
participant RenderChart as render_chart
User->>block-modifier: Request block creation/update<br/>(type, config, cube_name)
block-modifier->>block-modifier: Select appropriate<br/>update tool by type
alt Query/Chart Block - Unknown References
block-modifier->>CubeTool: cubes_summary() / inspect_cube()
CubeTool-->>block-modifier: Measures, dimensions,<br/>schema details
end
block-modifier->>UpdateTools: Call tool with structured<br/>query/chart_details<br/>(up to 5 retries)
alt Tool Success
UpdateTools-->>block-modifier: data_preview / content
alt Chart Block
block-modifier->>RenderChart: Verify rendered output
RenderChart-->>block-modifier: Rendered image
block-modifier-->>User: Report success<br/>+ block details<br/>+ verification
else Text/Table Block
block-modifier-->>User: Report success<br/>+ block type/name
end
else Tool Failure (Retries Exhausted)
UpdateTools-->>block-modifier: Error (missing ref/schema/shape/overflow)
block-modifier-->>block-modifier: Apply adjustment<br/>strategy or retry
block-modifier-->>User: Report failure<br/>+ last error<br/>+ block type/name
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/explore-cube/SKILL.md (1)
43-149:⚠️ Potential issue | 🟠 MajorApply terminology migration consistently across all referenced shared documentation.
The shared documentation files still use "master" terminology, creating inconsistency with this file's updated "document" and "report" terminology:
skills/_shared/resolution-context.md: "When a master is resolved...", "configured on the master",get_master_variablesMCP toolskills/_shared/cube-guide.md: "## Tips for Master Building" headerskills/_shared/variable-reference-syntax.md:get_master_variables(master_id=..., show_context_vars=true)Users following cross-references between documents encounter conflicting terminology. Update these shared files to align with the new terminology.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/explore-cube/SKILL.md` around lines 43 - 149, The shared docs still use "master" terminology causing inconsistency; update all occurrences of phrases like "When a master is resolved...", "configured on the master", the header "Tips for Master Building", and the tool/function name get_master_variables so they consistently use the new terminology ("document" or "report" as appropriate) and rename the MCP/tool identifier (e.g., get_master_variables → get_document_variables) and any related examples, links, and API signatures; ensure corresponding references in cube-guide.md, resolution-context.md, and variable-reference-syntax.md are updated and that any code snippets, headers, and parameter names reflect the new term.
🧹 Nitpick comments (5)
skills/create-report/SKILL.md (1)
133-137: Add language specification to code block.The fenced code block should specify a language for syntax highlighting and better accessibility.
📝 Proposed fix
-``` +```javascript render_chart( location={doc_id: <id>, slide_name: "<slide>", block_name: "<chart_block>"} )</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@skills/create-report/SKILL.mdaround lines 133 - 137, The fenced code block
showing the usage of render_chart should include a language specifier for syntax
highlighting and accessibility; update the block that contains
render_chart(location={doc_id: , slide_name: "", block_name:
"<chart_block>"}) to use a language tag (e.g., ```javascript) so readers and
tools correctly recognize the snippet, ensuring the snippet around the
render_chart call and its location object (doc_id, slide_name, block_name) is
wrapped with the language-specified fences.</details> </blockquote></details> <details> <summary>docs/tools.md (1)</summary><blockquote> `64-127`: **Add language specifications to code blocks.** Multiple fenced code blocks are missing language specifications for syntax highlighting. <details> <summary>📝 Proposed fixes</summary> Apply language identifiers to the code blocks at lines 64, 72, 88, 103, and 118: ```diff -``` +```javascript cubes_summary() → List available cubes ``` ```diff -``` +```javascript create_document(name="Q1 Report", source_id=1) ``` And similarly for the other blocks at lines 88, 103, and 118. </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@docs/tools.md` around lines 64 - 127, The fenced code blocks demonstrating API usage (eg. cubes_summary(), create_document(...), get_doc_summary, get_doc_variables, set_doc_variables, update_query_block, update_text_block, update_chart_block, render_chart, export_markdown, html_to_pdf) are missing language identifiers; update each triple-backtick block containing these examples to include an appropriate language tag (for example javascript or python) to enable syntax highlighting while keeping the content unchanged. ``` </details> </blockquote></details> <details> <summary>skills/update-chart/SKILL.md (1)</summary><blockquote> `20-20`: **Consider adding language specifiers to code blocks.** Markdown linters recommend specifying a language for fenced code blocks. While these examples show pseudo-code tool invocations rather than executable code, adding a specifier like `text` or `json` would improve consistency and potentially enable better syntax highlighting. <details> <summary>Example fix for line 85</summary> ```diff -``` +```text update_chart_block( ``` </details> Also applies to: 85-85, 109-109, 130-130, 160-160 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@skills/update-chart/SKILL.mdat line 20, The fenced code blocks in SKILL.md
that show tool invocations (e.g., the block starting with update_chart_block and
the other invocation examples) lack language specifiers; update each
triple-backtick fence to include an appropriate specifier such as text or json
(for example changetotext) for the blocks containing update_chart_block
and the other examples so Markdown linters and syntax highlighters recognize
them consistently.</details> </blockquote></details> <details> <summary>docs/tools/element.md (2)</summary><blockquote> `178-178`: **Clarify what "previous query provides context" means.** This statement is vague and doesn't provide actionable information for the user. Consider either: - Explaining how the context is used (e.g., "for validation", "for schema inference") - Removing the statement if it's an internal implementation detail not relevant to API users <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@docs/tools/element.md` at line 178, Replace the vague sentence "If updating an existing query, the previous query provides context" with a concrete explanation or remove it; for example, modify the line that contains the phrase "previous query provides context" to state how that context is used (e.g., "If updating an existing query, the previous query is used for validation and schema inference to preserve field types and suggest diffs") or remove the sentence entirely if it is an internal detail not relevant to API users. ``` </details> --- `120-120`: **Consider adding language specifiers to code blocks.** Markdown linters recommend specifying a language for fenced code blocks. Adding `text` or `json` would improve consistency with markdown best practices. Also applies to: 182-182, 226-232 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/tools/element.mdat line 120, Several fenced code blocks in the
document use bare triple-backticks; update each fenced block (the
triple-backtick markers) to include an appropriate language specifier such as
"text" or "json" (e.g., changetotext or ```json) to satisfy markdown
linting and improve syntax highlighting and consistency across the file.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@docs/README.md:
- Around line 34-44: Update the README to reflect the corrected tool-count math
(43 total tools originally vs 20 current + 23 disabled = 54% reduction) and add
a migration section that points to docs/tools/_disabled/ for the three archived
categories (Outline, Layout, Master); in that migration section enumerate the 23
disabled tool names (e.g., clear_outline, copy_slide, list_masters,
list_layout_libraries, etc.), for each state whether there is a replacement
workflow (and link to the replacement tool or documentation) or explicitly state
“no replacement”; also add a deprecation timeline entry and a Breaking Changes
notice for API consumers describing when the disabled tools will be removed or
become permanent, and include upgrade steps or suggested alternate APIs where
applicable so consumers can plan migration.In
@skills/update-chart/SKILL.md:
- Around line 22-26: The documentation for update_chart_block currently presents
the location field as required; change the SKILL.md snippet for the
update_chart_block schema to mark location as optional (e.g., annotate with
"location?: { ... }" or add "(optional)" to the location comment) to match
docs/tools/element.md and the actual behavior that supports transient chart
creation; ensure the update references the same parameter name "location" and
the function/operation "update_chart_block" so readers see the fields are
optional and consistent with element.md.In
@skills/update-query-block/SKILL.md:
- Around line 24-28: The tool signature's location parameter should be marked
optional: update the parameter named "location" in the SKILL.md signature to
"location?" so it matches the optional notation used for other fields (e.g.,
time_dimension?, filters?, mode?) and aligns with docs/tools/element.md and the
described transient-query behavior when omitted; ensure the comment for the
field remains unchanged except for adding the question mark to the parameter
name.
Outside diff comments:
In@skills/explore-cube/SKILL.md:
- Around line 43-149: The shared docs still use "master" terminology causing
inconsistency; update all occurrences of phrases like "When a master is
resolved...", "configured on the master", the header "Tips for Master Building",
and the tool/function name get_master_variables so they consistently use the new
terminology ("document" or "report" as appropriate) and rename the MCP/tool
identifier (e.g., get_master_variables → get_document_variables) and any related
examples, links, and API signatures; ensure corresponding references in
cube-guide.md, resolution-context.md, and variable-reference-syntax.md are
updated and that any code snippets, headers, and parameter names reflect the new
term.
Nitpick comments:
In@docs/tools.md:
- Around line 64-127: The fenced code blocks demonstrating API usage (eg.
cubes_summary(), create_document(...), get_doc_summary, get_doc_variables,
set_doc_variables, update_query_block, update_text_block, update_chart_block,
render_chart, export_markdown, html_to_pdf) are missing language identifiers;
update each triple-backtick block containing these examples to include an
appropriate language tag (for example javascript or python) to enable syntax
highlighting while keeping the content unchanged.In
@docs/tools/element.md:
- Line 178: Replace the vague sentence "If updating an existing query, the
previous query provides context" with a concrete explanation or remove it; for
example, modify the line that contains the phrase "previous query provides
context" to state how that context is used (e.g., "If updating an existing
query, the previous query is used for validation and schema inference to
preserve field types and suggest diffs") or remove the sentence entirely if it
is an internal detail not relevant to API users.- Line 120: Several fenced code blocks in the document use bare
triple-backticks; update each fenced block (the triple-backtick markers) to
include an appropriate language specifier such as "text" or "json" (e.g., change
totext or ```json) to satisfy markdown linting and improve syntax
highlighting and consistency across the file.In
@skills/create-report/SKILL.md:
- Around line 133-137: The fenced code block showing the usage of render_chart
should include a language specifier for syntax highlighting and accessibility;
update the block that contains render_chart(location={doc_id: , slide_name:
"", block_name: "<chart_block>"}) to use a language tag (e.g.,the snippet around the render_chart call and its location object (doc_id, slide_name, block_name) is wrapped with the language-specified fences. In `@skills/update-chart/SKILL.md`: - Line 20: The fenced code blocks in SKILL.md that show tool invocations (e.g., the block starting with update_chart_block and the other invocation examples) lack language specifiers; update each triple-backtick fence to include an appropriate specifier such as text or json (for example change ``` to ```text) for the blocks containing update_chart_block and the other examples so Markdown linters and syntax highlighters recognize them consistently.🪄 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:
868604b4-aaed-408d-887c-63d2b1356dda📒 Files selected for processing (14)
agents/block-modifier.mddocs/README.mddocs/skills.mddocs/tools.mddocs/tools/_disabled/layout.mddocs/tools/_disabled/master.mddocs/tools/_disabled/outline.mddocs/tools/element.mdskills/create-report/SKILL.mdskills/explore-cube/SKILL.mdskills/update-chart/SKILL.mdskills/update-query-block/SKILL.mdskills/update-table-block/SKILL.mdskills/update-text-block/SKILL.md
| location: { # Location of the chart block | ||
| doc_id: int, # The deck ID | ||
| doc_id: int, # The document ID | ||
| slide_name: str, # The slide containing the chart block | ||
| block_name: str # The name of the chart block | ||
| }, |
There was a problem hiding this comment.
Clarify whether location is optional.
According to docs/tools/element.md (line 96), the location parameter for update_chart_block is marked as optional ("No" in the Required column) to support transient chart creation. However, this documentation presents location as a required field without indicating optionality.
Please verify the correct requirement status and update accordingly for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/update-chart/SKILL.md` around lines 22 - 26, The documentation for
update_chart_block currently presents the location field as required; change the
SKILL.md snippet for the update_chart_block schema to mark location as optional
(e.g., annotate with "location?: { ... }" or add "(optional)" to the location
comment) to match docs/tools/element.md and the actual behavior that supports
transient chart creation; ensure the update references the same parameter name
"location" and the function/operation "update_chart_block" so readers see the
fields are optional and consistent with element.md.
| location: { # Location of the query's parent block | ||
| doc_id: int, # The deck ID | ||
| doc_id: int, # The document ID | ||
| slide_name: str, # The slide containing the parent block | ||
| parent_block: str # Name of the text or table block containing this query | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check tool definition to confirm if location is optional
ast-grep --pattern 'def update_query_block($$$)'Repository: MotleyAI/motley-skills
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Find the element.md and SKILL.md files
fd "element.md" . --type f
fd "SKILL.md" . --type fRepository: MotleyAI/motley-skills
Length of output: 418
🏁 Script executed:
#!/bin/bash
# Read element.md around line 146 to check the Required column
echo "=== element.md lines 130-160 ==="
sed -n '130,160p' ./docs/tools/element.md
echo ""
echo "=== SKILL.md lines 20-60 ==="
sed -n '20,60p' ./skills/update-query-block/SKILL.mdRepository: MotleyAI/motley-skills
Length of output: 3495
Add ? marker to location parameter to indicate it is optional.
The location parameter in the tool signature should be marked as optional to match docs/tools/element.md (Required: No) and align with the documented behavior that it creates a transient query when omitted. Update line 24 to:
location?: { # Location of the query's parent block
This also maintains consistency with the notation used elsewhere in the signature for optional fields (time_dimension?, filters?, mode?, etc.).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/update-query-block/SKILL.md` around lines 24 - 28, The tool
signature's location parameter should be marked optional: update the parameter
named "location" in the SKILL.md signature to "location?" so it matches the
optional notation used for other fields (e.g., time_dimension?, filters?, mode?)
and aligns with docs/tools/element.md and the described transient-query behavior
when omitted; ensure the comment for the field remains unchanged except for
adding the question mark to the parameter name.
1 participant
Summary by CodeRabbit
New Features
Refactor
Documentation