Skip to content

feat(source): add timeout to add-research imports#262

Open
voidborne-d wants to merge 3 commits intoteng-lin:mainfrom
voidborne-d:feat/source-add-research-timeout
Open

feat(source): add timeout to add-research imports#262
voidborne-d wants to merge 3 commits intoteng-lin:mainfrom
voidborne-d:feat/source-add-research-timeout

Conversation

@voidborne-d
Copy link
Copy Markdown
Contributor

@voidborne-d voidborne-d commented Apr 9, 2026

Summary

  • add a --timeout option to source add-research so --import-all can use an explicit retry budget
  • keep the existing 1800s default for backwards-compatible behavior
  • add CLI regression coverage and document the new flag

Testing

  • python3 -m pytest tests/unit/cli/test_source.py -k add_research

Closes #194

Summary by CodeRabbit

  • New Features

    • Added a configurable timeout for the "source add-research" command so import-all operations can use a custom maximum duration (default 1800s).
  • Documentation

    • CLI reference updated to include the new --timeout flag, adjusted flag formatting, and updated examples showing --import-all with a custom timeout.
  • Tests

    • Added a unit test verifying custom timeout is honored during import-all operations.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 9, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 125fbcbc-c533-446e-bf34-d2c9f5d4916f

📥 Commits

Reviewing files that changed from the base of the PR and between 8e8d5b1 and 8b6428a.

📒 Files selected for processing (1)
  • docs/cli-reference.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • docs/cli-reference.md

📝 Walkthrough

Walkthrough

Added a --timeout FLOAT option to source add-research (default 1800.0). When --import-all is used, the timeout is forwarded to import_with_retry() as max_elapsed. Documentation updated and a unit test asserting a custom timeout was added.

Changes

Cohort / File(s) Summary
Documentation
docs/cli-reference.md
Documented new --timeout flag, updated example to --import-all --timeout 600, and escaped pipe characters in the rendered flag table for --mode/--from.
CLI Implementation
src/notebooklm/cli/source.py
Added --timeout Click option (float, default 1800.0) to source_add_research; timeout added to function signature and passed as max_elapsed to import_with_retry() when --import-all is used; docstring examples updated.
Unit Tests
tests/unit/cli/test_source.py
Added test_add_research_with_import_all_passes_custom_timeout to assert import_with_retry is awaited with max_elapsed=600.0 when invoked with --import-all --timeout 600. Existing default-timeout test remains.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I nudged a flag into the ground,
A tiny hop to tame the round,
Timeout counted, retries stay,
Imports finish, come what may,
I nibble logs and bound away.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: adding a timeout option to the add-research command for import retries.
Linked Issues check ✅ Passed The PR fully addresses issue #194: adds --timeout option to source add-research, passes it to import_with_retry, maintains 1800s default, and includes tests.
Out of Scope Changes check ✅ Passed All changes are directly related to issue #194: CLI option, implementation, documentation update, and unit test coverage for the timeout feature.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/cli-reference.md`:
- Line 114: The table row for the add-research CLI entry breaks parsing and the
example omits --import-all; fix by escaping the pipe in the flags cell (change
`--mode [fast|deep]` to `--mode [fast\|deep]` or otherwise escape the `|` inside
the code span), ensure the `--timeout FLOAT` cell remains code-formatted, and
update the example to show the timeout together with import-all (e.g. `source
add-research "AI" --mode deep --timeout 600 --import-all`) so semantics are
correct; apply these changes to the add-research table row (symbols:
add-research, --mode [fast|deep], --timeout FLOAT, --import-all).
🪄 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: d463b3fd-ce17-4f00-b68a-135c38753557

📥 Commits

Reviewing files that changed from the base of the PR and between a997718 and eeb2dea.

📒 Files selected for processing (3)
  • docs/cli-reference.md
  • src/notebooklm/cli/source.py
  • tests/unit/cli/test_source.py

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a --timeout option to the source add-research CLI command, allowing users to specify a retry budget for importing sources. The documentation and tests have been updated to reflect this change. Feedback suggests removing the FLOAT type hint from the cli-reference.md for consistency and changing the --timeout option's type to int in source.py, also for consistency, and clarifying its scope to cover both research and import phases.

| `add <content>` | URL/file/text | - | `source add "https://..."` |
| `add-drive <id> <title>` | Drive file ID | - | `source add-drive abc123 "Doc"` |
| `add-research <query>` | Search query | `--mode [fast|deep]`, `--from [web|drive]`, `--import-all`, `--no-wait` | `source add-research "AI" --mode deep --no-wait` |
| `add-research <query>` | Search query | `--mode [fast|deep]`, `--from [web|drive]`, `--import-all`, `--timeout FLOAT`, `--no-wait` | `source add-research "AI" --mode deep --timeout 600` |
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Remove the FLOAT type hint from the options list to maintain consistency with other entries in the table, which do not specify types for options.

Suggested change
| `add-research <query>` | Search query | `--mode [fast|deep]`, `--from [web|drive]`, `--import-all`, `--timeout FLOAT`, `--no-wait` | `source add-research "AI" --mode deep --timeout 600` |
| `add-research <query>` | Search query | `--mode [fast|deep]`, `--from [web|drive]`, `--import-all`, `--timeout`, `--no-wait` | `source add-research "AI" --mode deep --timeout 600` |

Comment on lines +544 to +549
"--timeout",
type=float,
default=1800.0,
show_default=True,
help="Retry budget in seconds when --import-all is used",
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

For consistency with other commands in the source group (such as source wait), consider using type=int for the --timeout option. Additionally, since the command performs both research polling and source importing, the timeout should ideally apply to the entire operation rather than just the import phase.

Suggested change
"--timeout",
type=float,
default=1800.0,
show_default=True,
help="Retry budget in seconds when --import-all is used",
)
"--timeout",
type=int,
default=1800,
show_default=True,
help="Maximum seconds to wait for research and import",

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add timeout option to source add-research import retries

2 participants