Skip to content
Open
Show file tree
Hide file tree
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
196 changes: 180 additions & 16 deletions src/notebooklm/cli/session.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,23 @@
import json
import logging
import os
import shutil
import subprocess
import sys
import time
from collections.abc import Iterator
from contextlib import contextmanager
from pathlib import Path
from typing import Any
from typing import TYPE_CHECKING, Any

import click
import httpx
from rich.table import Table

if TYPE_CHECKING:
from playwright.sync_api import BrowserContext, Page
from rich.console import Console

from ..auth import (
ALLOWED_COOKIE_DOMAINS,
GOOGLE_REGIONAL_CCTLDS,
Expand Down Expand Up @@ -59,6 +64,17 @@
# Retryable Playwright connection errors
RETRYABLE_CONNECTION_ERRORS = ("ERR_CONNECTION_CLOSED", "ERR_CONNECTION_RESET")
LOGIN_MAX_RETRIES = 3
# Playwright TargetClosedError substring — matches the default message from
# Playwright's TargetClosedError class (introduced in v1.41). If a future
# version changes this message, the error will propagate unhandled (safe fallback).
TARGET_CLOSED_ERROR = "Target page, context or browser has been closed"
BROWSER_CLOSED_HELP = (
"[red]The browser window was closed during login.[/red]\n"
"This can happen when switching Google accounts in a persistent browser session.\n\n"
"Try:\n"
" 1. Run: notebooklm login --fresh\n"
" 2. Or run: notebooklm auth logout && notebooklm login"
)
CONNECTION_ERROR_HELP = (
"[red]Failed to connect to NotebookLM after multiple retries.[/red]\n"
"This may be caused by:\n"
Expand Down Expand Up @@ -198,9 +214,7 @@ def _login_with_browser_cookies(storage_path: Path, browser_name: str) -> None:
storage_path.chmod(0o600)
except OSError as e:
logger.error("Failed to save authentication to %s: %s", storage_path, e)
console.print(
f"[red]Failed to save authentication to {storage_path}.[/red]\n" f"Details: {e}"
)
console.print(f"[red]Failed to save authentication to {storage_path}.[/red]\nDetails: {e}")
raise SystemExit(1) from None

console.print(f"\n[green]Authentication saved to:[/green] {storage_path}")
Expand Down Expand Up @@ -340,6 +354,30 @@ def _ensure_chromium_installed() -> None:
)


def _recover_page(context: "BrowserContext", console: "Console") -> "Page":
"""Get a fresh page from a persistent browser context.

Used when the current page reference is stale (TargetClosedError).
A new page in a persistent context inherits all cookies and storage.

Returns a new Page, or raises SystemExit if the context/browser is dead.
Raises the original PlaywrightError for non-TargetClosed failures.
"""
from playwright.sync_api import Error as PlaywrightError

try:
return context.new_page()
except PlaywrightError as exc:
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

[High] PlaywrightError is the base class for all Playwright errors. If context.new_page() fails for a reason other than a dead context (resource exhaustion, browser crash, internal Playwright bug), the user sees "browser window was closed during login" which is misleading.

Suggestion — narrow the catch to only show BROWSER_CLOSED_HELP when it's actually a target-closed error:

def _recover_page(context, console):
    from playwright.sync_api import Error as PlaywrightError

    try:
        return context.new_page()
    except PlaywrightError as exc:
        error_str = str(exc)
        if TARGET_CLOSED_ERROR in error_str:
            logger.error("Browser context is dead, cannot recover page: %s", error_str)
            console.print(BROWSER_CLOSED_HELP)
            raise SystemExit(1) from exc
        # Not a TargetClosedError — don't mask the real problem
        logger.error("Failed to create new page for recovery: %s", error_str)
        raise

This also adds logger.error() which is currently missing (every other terminal error path in this file logs before exiting).

Found by: Claude Silent Failure Hunter, Codex

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Addressed in 4ac30ee: narrowed catch to only show BROWSER_CLOSED_HELP when TARGET_CLOSED_ERROR is in the error string. Other PlaywrightError types now re-raise. Added logger.error() for both paths.

error_str = str(exc)
if TARGET_CLOSED_ERROR in error_str:
logger.error("Browser context is dead, cannot recover page: %s", error_str)
console.print(BROWSER_CLOSED_HELP)
raise SystemExit(1) from exc
# Not a TargetClosedError — don't mask the real problem
logger.error("Failed to create new page for recovery: %s", error_str)
raise


def register_session_commands(cli):
"""Register session commands on the main CLI group."""

Expand Down Expand Up @@ -368,7 +406,13 @@ def register_session_commands(cli):
"Requires: pip install 'notebooklm[cookies]'"
),
)
def login(storage, browser, browser_cookies):
@click.option(
"--fresh",
is_flag=True,
default=False,
help="Start with a clean browser session (deletes cached browser profile). Use to switch Google accounts.",
)
def login(storage, browser, browser_cookies, fresh):
"""Log in to NotebookLM via browser.

Opens a browser window for Google login. After logging in,
Expand All @@ -393,12 +437,31 @@ def login(storage, browser, browser_cookies):

# rookiepy fast-path: skip Playwright entirely
if browser_cookies is not None:
if fresh:
console.print(
"[yellow]Warning: --fresh has no effect with --browser-cookies "
"(no browser profile is used).[/yellow]"
)
resolved_storage = Path(storage) if storage else get_storage_path()
_login_with_browser_cookies(resolved_storage, browser_cookies)
return

storage_path = Path(storage) if storage else get_storage_path()
browser_profile = get_browser_profile_dir()

if fresh and browser_profile.exists():
try:
shutil.rmtree(browser_profile)
console.print("[yellow]Cleared cached browser session (--fresh)[/yellow]")
except OSError as exc:
logger.error("Failed to clear browser profile %s: %s", browser_profile, exc)
console.print(
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

[Medium] This error path is missing logger.error(). Every other terminal error path in this file logs before exiting. The same issue applies to _recover_page() at line 366.

Suggestion:

except OSError as exc:
    logger.error("Failed to clear browser profile %s: %s", browser_profile, exc)
    console.print(...)

Found by: Claude Silent Failure Hunter

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Addressed in 4ac30ee: added logger.error() to the --fresh OSError path and both paths in _recover_page.

f"[red]Cannot clear browser profile: {exc}[/red]\n"
"Close any open browser windows and try again.\n"
f"If the problem persists, manually delete: {browser_profile}"
)
raise SystemExit(1) from exc

if sys.platform == "win32":
# On Windows < Python 3.13, mode= is ignored by mkdir(). On
# Python 3.13+, mode= applies Windows ACLs that can be overly
Expand Down Expand Up @@ -454,7 +517,7 @@ def login(storage, browser, browser_cookies):
try:
context = p.chromium.launch_persistent_context(**launch_kwargs)

page = context.pages[0] if context.pages else context.new_page()
page = context.pages[0] if context.pages else _recover_page(context, console)

# Retry navigation on transient connection errors with backoff
for attempt in range(1, LOGIN_MAX_RETRIES + 1):
Expand All @@ -466,22 +529,45 @@ def login(storage, browser, browser_cookies):
is_retryable = any(
code in error_str for code in RETRYABLE_CONNECTION_ERRORS
)
is_target_closed = TARGET_CLOSED_ERROR in error_str

# Check if we should retry
if is_retryable and attempt < LOGIN_MAX_RETRIES:
# Retryable error with attempts remaining: retry
if (is_retryable or is_target_closed) and attempt < LOGIN_MAX_RETRIES:
# For TargetClosedError, get a fresh page reference
if is_target_closed:
page = _recover_page(context, console)

backoff_seconds = attempt # Linear backoff: 1s, 2s
logger.debug(
f"Retryable connection error on attempt {attempt}/{LOGIN_MAX_RETRIES}: {error_str}"
"Retryable error on attempt %d/%d: %s",
attempt,
LOGIN_MAX_RETRIES,
error_str,
)
console.print(
f"[yellow]Connection interrupted "
f"(attempt {attempt}/{LOGIN_MAX_RETRIES}). "
f"Retrying in {backoff_seconds}s...[/yellow]"
if is_target_closed:
console.print(
f"[yellow]Browser page closed "
f"(attempt {attempt}/{LOGIN_MAX_RETRIES}). "
f"Retrying with fresh page...[/yellow]"
)
else:
console.print(
f"[yellow]Connection interrupted "
f"(attempt {attempt}/{LOGIN_MAX_RETRIES}). "
f"Retrying in {backoff_seconds}s...[/yellow]"
)
time.sleep(backoff_seconds)
elif is_target_closed:
# Exhausted retries on browser-closed errors
logger.error(
"Browser closed during login after %d attempts. Last error: %s",
LOGIN_MAX_RETRIES,
error_str,
)
Comment on lines +562 to 566
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 Python logging, use the % formatting style instead of f-strings to enable lazy formatting. This applies to all logging calls in this section.

Suggested change
logger.error(
f"Browser closed during login after {LOGIN_MAX_RETRIES} attempts. "
f"Last error: {error_str}"
)
logger.error(
"Browser closed during login after %d attempts. Last error: %s",
LOGIN_MAX_RETRIES,
error_str,
)
References
  1. For Python logging, use the % formatting style instead of f-strings to enable lazy formatting, where string interpolation is deferred until the log is emitted.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Addressed in e53d686: switched to % formatting for lazy evaluation.

time.sleep(backoff_seconds)
console.print(BROWSER_CLOSED_HELP)
raise SystemExit(1) from exc
elif is_retryable:
# Exhausted retries on a retryable error
# Exhausted retries on network errors
logger.error(
f"Failed to connect to NotebookLM after {LOGIN_MAX_RETRIES} attempts. "
f"Last error: {error_str}"
Expand All @@ -508,7 +594,20 @@ def login(storage, browser, browser_cookies):
try:
page.goto(url, wait_until="commit")
except PlaywrightError as exc:
if "Navigation interrupted" not in str(exc):
error_str = str(exc)
if TARGET_CLOSED_ERROR in error_str:
# Page was destroyed (e.g. user switched accounts) -- get fresh page
page = _recover_page(context, console)
try:
page.goto(url, wait_until="commit")
except PlaywrightError as inner_exc:
if TARGET_CLOSED_ERROR in str(inner_exc):
# Recovered page also dead -- context/browser is gone
console.print(BROWSER_CLOSED_HELP)
raise SystemExit(1) from inner_exc
elif "Navigation interrupted" not in str(inner_exc):
raise
elif "Navigation interrupted" not in error_str:
raise

current_url = page.url
Expand Down Expand Up @@ -745,6 +844,71 @@ def auth_group():
"""Authentication management commands."""
pass

@auth_group.command("logout")
def auth_logout():
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

[Medium] login blocks when NOTEBOOKLM_AUTH_JSON is set (line 416), but auth logout has no equivalent check. After auth logout, the env var auth remains active — the user sees "Logged out" but the very next CLI command still works with the env var credentials.

Suggestion — mirror login's guard, or at minimum warn:

if os.environ.get("NOTEBOOKLM_AUTH_JSON"):
    console.print(
        "[yellow]Note: NOTEBOOKLM_AUTH_JSON is set — env-based auth will "
        "remain active after logout. Unset it to fully log out.[/yellow]"
    )

Found by: Codex (gpt-5.4)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Addressed in 4ac30ee: added NOTEBOOKLM_AUTH_JSON warning at the top of auth logout, mirroring login's guard pattern.

"""Log out by clearing saved authentication.

Removes both the saved cookie file (storage_state.json) and the
cached browser profile. After logout, run 'notebooklm login' to
authenticate with a different Google account.

\b
Examples:
notebooklm auth logout # Clear auth for active profile
notebooklm -p work auth logout # Clear auth for 'work' profile
"""
# Warn if env-based auth will remain active after logout
if os.environ.get("NOTEBOOKLM_AUTH_JSON"):
console.print(
"[yellow]Note: NOTEBOOKLM_AUTH_JSON is set — env-based auth will "
"remain active after logout. Unset it to fully log out.[/yellow]"
)

storage_path = get_storage_path()
browser_profile = get_browser_profile_dir()

removed_any = False

# Remove storage_state.json
if storage_path.exists():
try:
storage_path.unlink()
removed_any = True
except OSError as exc:
logger.error("Failed to remove auth file %s: %s", storage_path, exc)
console.print(
f"[red]Cannot remove auth file: {exc}[/red]\n"
"Close any running notebooklm commands and try again.\n"
f"If the problem persists, manually delete: {storage_path}"
)
raise SystemExit(1) from exc

# Remove browser profile directory
if browser_profile.exists():
try:
shutil.rmtree(browser_profile)
removed_any = True
except OSError as exc:
logger.error("Failed to remove browser profile %s: %s", browser_profile, exc)
partial = (
"[yellow]Note: Auth file was removed, but browser profile "
"could not be deleted.[/yellow]\n"
if removed_any
else ""
)
console.print(
f"{partial}"
f"[red]Cannot remove browser profile: {exc}[/red]\n"
"Close any open browser windows and try again.\n"
f"If the problem persists, manually delete: {browser_profile}"
)
raise SystemExit(1) from exc

if removed_any:
console.print("[green]Logged out.[/green] Run 'notebooklm login' to sign in again.")
else:
console.print("[yellow]No active session found.[/yellow] Already logged out.")
Comment on lines +847 to +910
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

The auth logout command clears the authentication cookies and browser profile, but it leaves the context.json file intact. This file stores the current notebook ID and conversation ID. If a user logs out and another user logs in on the same profile, the status command will still show the previous user's notebook context until it is manually cleared. Consider also clearing the context file during logout for a cleaner session transition.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Intentionally kept separate — context.json stores notebook context (not credentials) and is owned by the clear command. Clearing it on logout would be an unexpected side effect since a user's active notebook selection is independent of which Google account is logged in. The existing notebooklm clear command handles this when needed.


@auth_group.command("check")
@click.option(
"--test", "test_fetch", is_flag=True, help="Test token fetch (makes network request)"
Expand Down
Loading
Loading