feat(cli): add timeout options for source imports#269
feat(cli): add timeout options for source imports#269voidborne-d wants to merge 4 commits intoteng-lin:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as "CLI (source add / add-research)" %%rgba(52,152,219,0.5)
participant Client as "NotebookLMClient" %%rgba(46,204,113,0.5)
participant Import as "import_with_retry" %%rgba(241,196,15,0.5)
participant Remote as "NotebookLM API / Import backend" %%rgba(155,89,182,0.5)
CLI->>Client: instantiate with timeout=float(timeout)
CLI->>Remote: send add/import request (async)
alt import-all requested
Remote->>Import: trigger import flow
Import->>Import: retry loop bounded by max_elapsed=float(timeout)
Import->>Remote: repeated import attempts
Import-->>CLI: import result / completion
else not import-all
Remote-->>CLI: immediate result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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 configurable timeout options for the source add and source add-research CLI commands, allowing users to specify HTTP request timeouts and maximum elapsed time for source imports. Corresponding unit tests have been added to ensure these parameters are correctly passed to the underlying client and retry mechanisms. A review comment suggests updating the help text for the research timeout to more accurately describe its coverage.
src/notebooklm/cli/source.py
Outdated
| default=1800, | ||
| type=int, | ||
| show_default=True, | ||
| help="Maximum seconds to keep retrying source import when --import-all is used", |
There was a problem hiding this comment.
The help text for --timeout in source_add_research is currently limited to the import retry budget. If the timeout is also applied to the research polling phase (as suggested below), this description should be updated to reflect that it covers the entire operation duration.
| help="Maximum seconds to keep retrying source import when --import-all is used", | |
| help="Maximum seconds to wait for research and source import to complete", |
![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)
src/notebooklm/cli/source.py (1)
242-244: Add type hints to the updated command signatures.The modified function signatures are still untyped. Please annotate the newly added
timeoutparameter and surrounding arguments to keep mypy coverage aligned for touched code.As per coding guidelines,
src/notebooklm/**/*.py: "Use type hints and ensure type checking passes with mypy forsrc/notebooklm".Also applies to: 566-567
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notebooklm/cli/source.py` around lines 242 - 244, Add proper type annotations to the updated CLI command signatures (e.g., source_add) so mypy passes: annotate parameters like ctx, content, notebook_id, source_type, title, mime_type, timeout, json_output, and client_auth with appropriate types (e.g., click.Context or similar for ctx, str for string args, Optional[float|int] or Union[float,int,None] for timeout, bool for json_output, and the client auth type used in the codebase), and add return type (likely None). Apply the same pattern to the other modified signature(s) referenced around lines 566-567 so all touched functions in src/notebooklm have explicit type hints consistent with project types and imports.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/notebooklm/cli/source.py`:
- Around line 233-239: The CLI accepts non-positive integers for the --timeout
option which breaks retry behavior; update the two click.option declarations for
"--timeout" (the one around the add-source flow and the other at lines ~552-558)
to validate and reject non-positive values at parse time — either by using
click.IntRange(min=1) as the type or adding a click callback that raises a
UsageError when value < 1 — so only positive seconds are allowed before command
execution.
---
Nitpick comments:
In `@src/notebooklm/cli/source.py`:
- Around line 242-244: Add proper type annotations to the updated CLI command
signatures (e.g., source_add) so mypy passes: annotate parameters like ctx,
content, notebook_id, source_type, title, mime_type, timeout, json_output, and
client_auth with appropriate types (e.g., click.Context or similar for ctx, str
for string args, Optional[float|int] or Union[float,int,None] for timeout, bool
for json_output, and the client auth type used in the codebase), and add return
type (likely None). Apply the same pattern to the other modified signature(s)
referenced around lines 566-567 so all touched functions in src/notebooklm have
explicit type hints consistent with project types and imports.
🪄 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: bceeba2e-edf3-419c-ad0f-b4bf514913ad
📒 Files selected for processing (2)
src/notebooklm/cli/source.pytests/unit/cli/test_source.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/notebooklm/cli/source.py (1)
562-568: Clarify--timeouthelp text scopeCurrent help text says the timeout covers research + import completion, but the value is only applied to
import_with_retry(..., max_elapsed=...)in the--import-allpath. Consider making that explicit in the option help.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notebooklm/cli/source.py` around lines 562 - 568, The --timeout option help is misleading: the value is only passed into import_with_retry(..., max_elapsed=...) when running the --import-all path, not for the research step; update the click.option help string for --timeout to explicitly state it applies to the import retry (import_with_retry max_elapsed) used by the --import-all code path (or alternatively wire the same timeout into the research call if you intend it to cover both). Locate the --timeout click.option and the import_with_retry call in source.py to either change the help wording to mention import_with_retry/--import-all or propagate the timeout value into the research function call so it truly covers both steps.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/notebooklm/cli/source.py`:
- Around line 244-254: The command functions (e.g., source_add) are annotated as
returning None but actually return a coroutine from _run(); update their return
type to an awaitable type such as -> typing.Awaitable[None] (or ->
typing.Coroutine[typing.Any, typing.Any, None]) and add the required import from
typing (Awaitable or Coroutine and Any) at the top of the module; apply the same
change to any other CLI command functions in this file that return _run().
---
Nitpick comments:
In `@src/notebooklm/cli/source.py`:
- Around line 562-568: The --timeout option help is misleading: the value is
only passed into import_with_retry(..., max_elapsed=...) when running the
--import-all path, not for the research step; update the click.option help
string for --timeout to explicitly state it applies to the import retry
(import_with_retry max_elapsed) used by the --import-all code path (or
alternatively wire the same timeout into the research call if you intend it to
cover both). Locate the --timeout click.option and the import_with_retry call in
source.py to either change the help wording to mention
import_with_retry/--import-all or propagate the timeout value into the research
function call so it truly covers both steps.
🪄 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: 0b0fef19-038c-45bd-8314-4da8343c2a42
📒 Files selected for processing (2)
src/notebooklm/cli/source.pytests/unit/cli/test_source.py
2 participants
Summary
--timeouttonotebooklm source addso slow remote URL imports can override the default 30s RPC timeout--timeouttonotebooklm source add-research --import-allso import retry budget matches the caller instead of being fixed at the helper defaultWhy
Two open issues point at the same ergonomics gap around source import timeouts:
source add <https_url>can time out on slower pages with no CLI override (notebooklm source add <https_url> timesout after 30s #192)source add-research --import-allcannot tune its retry budget, unlikeresearch wait --import-all(Add timeout option to source add-research import retries #194)This patch keeps the existing defaults, but exposes the knobs where users actually need them.
Testing
PYTHONPATH=src python3 -m pytest tests/unit/cli/test_source.py tests/unit/cli/test_notebook.py -k 'source_add_research or source_add'Summary by CodeRabbit
New Features
Tests