feat: support language and instructions for mind maps#263
feat: support language and instructions for mind maps#263voidborne-d wants to merge 6 commits intoteng-lin:mainfrom
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:
📝 WalkthroughWalkthroughAdded optional Changes
Sequence DiagramsequenceDiagram
participant User as User/CLI
participant CLI as CLI Handler
participant API as ArtifactsAPI
participant RPC as RPC Layer
User->>CLI: invoke generate mind-map (--language, --instructions)
CLI->>CLI: resolve_language() -> language
CLI->>CLI: build generate_kwargs {source_ids, language, instructions}
CLI->>API: generate_mind_map(notebook_id, **generate_kwargs)
API->>RPC: GENERATE_MIND_MAP(..., ["interactive_mindmap", [["[CONTEXT]", instructions or ""]], language])
RPC-->>API: returns mind map result
API-->>CLI: returns result dict
CLI->>User: outputs result (JSON or stdout)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/test_source_selection.py (1)
512-534: Consider adding a fallback-case assertion forinstructions=None.This test is strong for explicit values; adding one more case for default
instructions=None→ context""would lock in the backward-compatible payload behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_source_selection.py` around lines 512 - 534, Add a case in test_generate_mind_map_language_and_instructions_encoding to exercise ArtifactsAPI.generate_mind_map with instructions=None and assert the RPC payload uses an empty context string; specifically call generate_mind_map with instructions=None (or omit the instructions arg), capture mock_core.rpc_call.call_args.args[1] and assert params[5] equals ["interactive_mindmap", [["[CONTEXT]", ""]], "ja"] (referencing mock_core.rpc_call and params[5] to locate the assertion).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/cli/test_generate.py`:
- Around line 434-437: Patch the test to stub get_language so
resolve_language(None) doesn't read local config: in the test, before invoking
the code that calls mock_client.artifacts.generate_mind_map, monkeypatch (or
unittest.mock.patch) the get_language function used by the CLI to a small stub
that returns "en" when called with None (and delegates for other inputs if
needed); this forces the deterministic default-path so the later assertions on
kwargs["language"] == "en" and kwargs["instructions"] is None are stable. Ensure
you patch the same get_language symbol the CLI code imports (the one that
resolve_language uses) and unpatch/let the fixture cleanup handle restoration.
---
Nitpick comments:
In `@tests/unit/test_source_selection.py`:
- Around line 512-534: Add a case in
test_generate_mind_map_language_and_instructions_encoding to exercise
ArtifactsAPI.generate_mind_map with instructions=None and assert the RPC payload
uses an empty context string; specifically call generate_mind_map with
instructions=None (or omit the instructions arg), capture
mock_core.rpc_call.call_args.args[1] and assert params[5] equals
["interactive_mindmap", [["[CONTEXT]", ""]], "ja"] (referencing
mock_core.rpc_call and params[5] to locate the assertion).
🪄 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: c638f80e-c9c9-4fb7-958a-73c3b52fc6dc
📒 Files selected for processing (6)
README.mddocs/rpc-reference.mdsrc/notebooklm/_artifacts.pysrc/notebooklm/cli/generate.pytests/unit/cli/test_generate.pytests/unit/test_source_selection.py
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for specifying a language and custom instructions when generating mind maps. These changes span the core API, the CLI, documentation, and unit tests. The feedback suggests improving the README example by using the returned note_id to ensure the correct mind map is downloaded, making the demonstration more robust.
README.md
Outdated
| result = await client.artifacts.generate_mind_map( | ||
| nb.id, | ||
| language="ja", | ||
| instructions="Focus on chronology and causal links", | ||
| ) | ||
| await client.artifacts.download_mind_map(nb.id, "mindmap.json") |
There was a problem hiding this comment.
To make the example more robust and better demonstrate the API, consider using the note_id from the result to download the specific mind map that was just generated. Currently, download_mind_map without an artifact_id will download the first mind map it finds, which may not be the one just created if others exist.
| result = await client.artifacts.generate_mind_map( | |
| nb.id, | |
| language="ja", | |
| instructions="Focus on chronology and causal links", | |
| ) | |
| await client.artifacts.download_mind_map(nb.id, "mindmap.json") | |
| result = await client.artifacts.generate_mind_map( | |
| nb.id, | |
| language="ja", | |
| instructions="Focus on chronology and causal links", | |
| ) | |
| if result and result.get("note_id"): | |
| await client.artifacts.download_mind_map(nb.id, "mindmap.json", artifact_id=result["note_id"]) | |
| else: | |
| # Fallback for safety, though generate_mind_map should return a note_id | |
| await client.artifacts.download_mind_map(nb.id, "mindmap.json") |
2 participants
Summary
languageandinstructionssupport togenerate_mind_map()and encode both into the RPC payload--languageand--instructionsonnotebooklm generate mind-mapTesting
python3 -m pytest tests/unit/cli/test_generate.py -k 'mind_map' tests/unit/test_source_selection.py -k 'mind_map'\n\nCloses Mind map generation ignores language and instructions parameters #250Summary by CodeRabbit
New Features
Documentation
Tests