Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions src/notebooklm/cli/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -263,9 +263,10 @@ async def _resolve_partial_id(

Allows users to type partial IDs like 'abc' instead of full UUIDs.
Matches are case-insensitive prefix matches.
Also supports searching by title (prefix match, case-insensitive).

Choose a reason for hiding this comment

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

high

This is a great feature enhancement. However, the new functionality of searching by title is not covered by unit tests. Please add tests to tests/unit/cli/test_resolve.py to cover various scenarios, such as:

  • Matching by a unique title prefix.
  • Prioritizing ID matches over title matches when both could match.
  • Handling ambiguous title matches.
  • Ensuring case-insensitivity for title search.
  • Verifying behavior with items that have no title.
References
  1. When a function's signature is updated to return new values, update tests to assert the state of these new returns, including for error handling and early-return paths.


Args:
partial_id: Full or partial ID to resolve
partial_id: Full or partial ID to resolve, or title to search
list_fn: Async function that returns list of items with id/title attributes
entity_name: Name for error messages (e.g., "notebook", "source")
list_command: CLI command to list items (e.g., "list", "source list")
Expand All @@ -284,7 +285,13 @@ async def _resolve_partial_id(
return partial_id

items = await list_fn()

# First try ID prefix match
matches = [item for item in items if item.id.lower().startswith(partial_id.lower())]

# If no ID matches, try title prefix match
if len(matches) == 0:
matches = [item for item in items if item.title and item.title.lower().startswith(partial_id.lower())]
Comment on lines 290 to +294

Choose a reason for hiding this comment

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

medium

This implementation calls partial_id.lower() inside the list comprehensions, which is inefficient as it's re-evaluated for every item. It's better to compute the lowercase version of partial_id once before the loops. Also, using if not matches: is more idiomatic than if len(matches) == 0:.

Suggested change
matches = [item for item in items if item.id.lower().startswith(partial_id.lower())]
# If no ID matches, try title prefix match
if len(matches) == 0:
matches = [item for item in items if item.title and item.title.lower().startswith(partial_id.lower())]
lower_partial_id = partial_id.lower()
# First try ID prefix match
matches = [item for item in items if item.id.lower().startswith(lower_partial_id)]
# If no ID matches, try title prefix match
if not matches:
matches = [item for item in items if item.title and item.title.lower().startswith(lower_partial_id)]


if len(matches) == 1:
if matches[0].id != partial_id:
Expand All @@ -293,11 +300,11 @@ async def _resolve_partial_id(
return matches[0].id
elif len(matches) == 0:
raise click.ClickException(
f"No {entity_name} found starting with '{partial_id}'. "
f"No {entity_name} found matching '{partial_id}'. "
f"Run 'notebooklm {list_command}' to see available {entity_name}s."
)
else:
lines = [f"Ambiguous ID '{partial_id}' matches {len(matches)} {entity_name}s:"]
lines = [f"Ambiguous input '{partial_id}' matches {len(matches)} {entity_name}s:"]
for item in matches[:5]:
title = item.title or "(untitled)"
lines.append(f" {item.id[:12]}... {title}")
Expand Down
Loading