feat: fix Google account switching (#246)#248
feat: fix Google account switching (#246)#248Halmurat-Uyghur wants to merge 4 commits intoteng-lin:mainfrom
Conversation
…eng-lin#246) Fix Google account switching by addressing both bugs reported in teng-lin#246: Bug 1 - Cached browser profile: Add --fresh flag to login that deletes the persistent browser profile before launching Playwright, giving users a clean session. Also add 'auth logout' command that clears both storage_state.json and browser_profile/ for full session cleanup. Bug 2 - TargetClosedError crash: Handle TargetClosedError in both the initial navigation retry loop (Site A) and the cookie-forcing section (Site B) by recovering with context.new_page(). Show browser-specific error message (not network error) when retries are exhausted.
- Use _recover_page for initial page creation (Copilot) - Show distinct "Browser page closed" message for TargetClosedError retries, skip backoff sleep since it's not a network issue (Copilot) - Communicate partial state in auth logout when unlink succeeds but rmtree fails (Greptile P2) - Add missing test for --fresh OSError path (Greptile P2)
📝 WalkthroughWalkthroughAdds a Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Client CLI
participant FS as Filesystem
participant PW as Playwright (Browser/Context/Page)
participant Auth as Auth Page (Google)
rect rgba(135,206,250,0.5)
CLI->>FS: if --fresh -> remove browser_profile dir
FS-->>CLI: success / error
end
rect rgba(144,238,144,0.5)
CLI->>PW: launch persistent context (maybe with storage_state)
PW-->>CLI: context
CLI->>PW: new_page() / goto(GOOGLE_ACCOUNTS_URL)
PW->>Auth: navigate
Auth-->>PW: content / redirects
end
rect rgba(255,182,193,0.5)
PW--x CLI: TargetClosedError (page/context/browser closed)
CLI->>PW: _recover_page(context)
PW-->>CLI: new Page or TargetClosedError
CLI->>PW: retry goto or exit with BROWSER_CLOSED_HELP
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Pull request overview
This PR resolves Google account switching issues during notebooklm login by clearing persistent browser session state on demand, adding a symmetric logout command, and hardening Playwright navigation against page/context closure (TargetClosedError) during account switching.
Changes:
- Add
login --freshto delete the persistent Playwright browser profile directory before launching, enabling clean Google re-auth. - Add
notebooklm auth logoutto remove bothstorage_state.jsonand the cached browser profile for the active profile. - Improve login resilience by recovering from Playwright “target closed” situations via a new
_recover_page()helper and browser-specific guidance when retries are exhausted.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/notebooklm/cli/session.py |
Adds --fresh, introduces auth logout, and implements TargetClosedError recovery + improved help messaging. |
tests/unit/cli/test_session.py |
Adds unit tests for --fresh, auth logout, and TargetClosedError recovery/error messaging behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/notebooklm/cli/session.py (1)
353-368: Add type hints to_recover_pagefor mypy compliance.The function lacks type annotations. Per coding guidelines,
src/notebooklm/**/*.pyfiles should use type hints and pass mypy checks.♻️ Proposed fix to add type hints
-def _recover_page(context, console): +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. """ from playwright.sync_api import Error as PlaywrightErrorYou'll need to add the following import at the top of the file for type checking:
from typing import TYPE_CHECKING if TYPE_CHECKING: from playwright.sync_api import BrowserContext, Page from rich.console import Console🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notebooklm/cli/session.py` around lines 353 - 368, Add mypy-compatible type hints to _recover_page: annotate parameter context as BrowserContext and console as Console and the return type as Page, and import those types under a TYPE_CHECKING guard (from playwright.sync_api import BrowserContext, Page and from rich.console import Console inside an if TYPE_CHECKING: block). Keep the runtime import of PlaywrightError as-is (from playwright.sync_api import Error as PlaywrightError) and ensure the function signature reads _recover_page(context: BrowserContext, console: Console) -> Page so the use of context.new_page() and exception handling with PlaywrightError type-checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/notebooklm/cli/session.py`:
- Around line 353-368: Add mypy-compatible type hints to _recover_page: annotate
parameter context as BrowserContext and console as Console and the return type
as Page, and import those types under a TYPE_CHECKING guard (from
playwright.sync_api import BrowserContext, Page and from rich.console import
Console inside an if TYPE_CHECKING: block). Keep the runtime import of
PlaywrightError as-is (from playwright.sync_api import Error as PlaywrightError)
and ensure the function signature reads _recover_page(context: BrowserContext,
console: Console) -> Page so the use of context.new_page() and exception
handling with PlaywrightError type-checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d344d297-5c61-43c1-b790-9fa33bf665ef
📒 Files selected for processing (2)
src/notebooklm/cli/session.pytests/unit/cli/test_session.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 enhances the authentication flow by introducing a new auth logout command and a --fresh flag for the login command, both designed to clear cached browser profiles. Additionally, it implements a recovery mechanism for TargetClosedError in Playwright, allowing the CLI to obtain a fresh page reference if the browser window is closed or the context becomes stale. Feedback includes suggestions to use lazy formatting in logging calls for better performance and to clear the context.json file during logout to prevent stale notebook and conversation IDs from persisting between sessions.
src/notebooklm/cli/session.py
Outdated
| backoff_seconds = attempt # Linear backoff: 1s, 2s | ||
| logger.debug( | ||
| f"Retryable connection error on attempt {attempt}/{LOGIN_MAX_RETRIES}: {error_str}" | ||
| f"Retryable error on attempt {attempt}/{LOGIN_MAX_RETRIES}: {error_str}" |
There was a problem hiding this comment.
For Python logging, use the % formatting style instead of f-strings to enable lazy formatting, where string interpolation is deferred until the log is actually emitted. This is a best practice for performance and is also specified in the general rules for this repository.
logger.debug(
"Retryable error on attempt %d/%d: %s",
attempt,
LOGIN_MAX_RETRIES,
error_str,
)References
- 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.
There was a problem hiding this comment.
Addressed in e53d686: switched to % formatting for lazy evaluation.
| logger.error( | ||
| f"Browser closed during login after {LOGIN_MAX_RETRIES} attempts. " | ||
| f"Last error: {error_str}" | ||
| ) |
There was a problem hiding this comment.
For Python logging, use the % formatting style instead of f-strings to enable lazy formatting. This applies to all logging calls in this section.
| 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
- 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.
There was a problem hiding this comment.
Addressed in e53d686: switched to % formatting for lazy evaluation.
| @auth_group.command("logout") | ||
| def auth_logout(): | ||
| """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 | ||
| """ | ||
| 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: | ||
| console.print( | ||
| "[red]Cannot remove auth file — it may be in use by another process.[/red]\n" | ||
| "Close any running notebooklm commands and try again.\n" | ||
| f"If the problem persists, manually delete: {storage_path}" | ||
| ) | ||
| raise SystemExit(1) from None | ||
|
|
||
| # Remove browser profile directory | ||
| if browser_profile.exists(): | ||
| try: | ||
| shutil.rmtree(browser_profile) | ||
| removed_any = True | ||
| except OSError: | ||
| partial = ( | ||
| "[yellow]Note: Auth file was removed, but browser profile " | ||
| "could not be deleted.[/yellow]\n" | ||
| if removed_any | ||
| else "" | ||
| ) | ||
| console.print( | ||
| f"{partial}" | ||
| "[red]Cannot remove browser profile — it may be in use by another process.[/red]\n" | ||
| "Close any open browser windows and try again.\n" | ||
| f"If the problem persists, manually delete: {browser_profile}" | ||
| ) | ||
| raise SystemExit(1) from None | ||
|
|
||
| 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.") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
- Fix Windows Python 3.12 CI failure: pass explicit "auto" value to --browser-cookies for cross-platform Click compatibility - Use % formatting in logger.debug/logger.error for lazy evaluation (Gemini code-assist)
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/notebooklm/cli/session.py (2)
557-568: Inconsistent logger formatting—consider using%style for lazy evaluation.Lines 559-562 and 567 use f-strings while lines 529-534 correctly use
%formatting. The commit message mentions preferring%formatting for lazy evaluation in logger calls.♻️ Suggested fix for consistency
elif is_retryable: # Exhausted retries on network errors logger.error( - f"Failed to connect to NotebookLM after {LOGIN_MAX_RETRIES} attempts. " - f"Last error: {error_str}" + "Failed to connect to NotebookLM after %d attempts. Last error: %s", + LOGIN_MAX_RETRIES, + error_str, ) console.print(CONNECTION_ERROR_HELP) raise SystemExit(1) from exc else: # Non-retryable error - re-raise immediately - logger.debug(f"Non-retryable error: {error_str}") + logger.debug("Non-retryable error: %s", error_str) raise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notebooklm/cli/session.py` around lines 557 - 568, The logger calls in the session retry handling use f-strings (logger.error and logger.debug) which is inconsistent with the `%`-style used elsewhere; change these to use %-formatting for lazy evaluation by replacing f"Failed to connect...{LOGIN_MAX_RETRIES}...{error_str}" with a format string and passing LOGIN_MAX_RETRIES and error_str as arguments to logger.error, and similarly replace the f"Non-retryable error: {error_str}" with a format string and pass error_str to logger.debug; leave console.print(CONNECTION_ERROR_HELP), the SystemExit raise, and the retry logic (is_retryable) unchanged.
353-368: Add type hints for mypy compliance.The function is missing type annotations. Per coding guidelines,
src/notebooklm/**/*.pyfiles should use type hints and pass mypy checking.♻️ Suggested type annotations
-def _recover_page(context, console): +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. """You may need to add conditional imports or use
TYPE_CHECKINGfor the type annotations:from typing import TYPE_CHECKING if TYPE_CHECKING: from playwright.sync_api import BrowserContext, Page from rich.console import Console🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notebooklm/cli/session.py` around lines 353 - 368, The function _recover_page lacks type annotations; add mypy-friendly hints by importing TYPE_CHECKING and conditionally importing types (e.g., from playwright.sync_api import BrowserContext, Page and from rich.console import Console) inside an if TYPE_CHECKING block, then annotate the signature as def _recover_page(context: "BrowserContext", console: "Console") -> "Page": and update the local Playwright Error import use (keep PlaywrightError as is) so the function returns a typed Page or raises SystemExit, ensuring no runtime import overhead and satisfying mypy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/notebooklm/cli/session.py`:
- Around line 557-568: The logger calls in the session retry handling use
f-strings (logger.error and logger.debug) which is inconsistent with the
`%`-style used elsewhere; change these to use %-formatting for lazy evaluation
by replacing f"Failed to connect...{LOGIN_MAX_RETRIES}...{error_str}" with a
format string and passing LOGIN_MAX_RETRIES and error_str as arguments to
logger.error, and similarly replace the f"Non-retryable error: {error_str}" with
a format string and pass error_str to logger.debug; leave
console.print(CONNECTION_ERROR_HELP), the SystemExit raise, and the retry logic
(is_retryable) unchanged.
- Around line 353-368: The function _recover_page lacks type annotations; add
mypy-friendly hints by importing TYPE_CHECKING and conditionally importing types
(e.g., from playwright.sync_api import BrowserContext, Page and from
rich.console import Console) inside an if TYPE_CHECKING block, then annotate the
signature as def _recover_page(context: "BrowserContext", console: "Console") ->
"Page": and update the local Playwright Error import use (keep PlaywrightError
as is) so the function returns a typed Page or raises SystemExit, ensuring no
runtime import overhead and satisfying mypy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c87dfd6-0ae2-4d2c-bd4d-ee0241288e51
📒 Files selected for processing (2)
src/notebooklm/cli/session.pytests/unit/cli/test_session.py
✅ Files skipped from review due to trivial changes (1)
- tests/unit/cli/test_session.py
|
@teng-lin Thanks for the detailed root cause analysis and recommended fix in #246 — this PR follows your approach closely:
All edge cases from your comment are handled: first login with 13 new tests, all 2025+ passing across macOS/Ubuntu/Windows CI. Ready for review! |
teng-lin
left a comment
teng-lin
left a comment
There was a problem hiding this comment.
Thanks for this well-crafted PR, @Halmurat-Uyghur! The approach to TargetClosedError recovery is architecturally sound — particularly the insight that context.storage_state() captures cookies at the context level regardless of page lifecycle. The --fresh flag and auth logout give users clean escape hatches for account switching.
I ran this through multiple reviewers (Claude, Codex/gpt-5.4, Gemini 2.5 Pro) and compiled findings below. Two high-priority items and a few suggestions — overall this is in great shape.
Summary
| Priority | Issue | Severity |
|---|---|---|
| 1 | _recover_page catches all PlaywrightError broadly — misleading message for non-target-closed errors |
High |
| 2 | auth logout OSError handlers discard exception details (from None, no logging, unbound exc) |
High |
| 3 | auth logout doesn't check NOTEBOOKLM_AUTH_JSON env var (unlike login) |
Medium |
| 4 | Missing test: cookie-forcing double-failure path (recovered page also dies) | Medium |
| 5 | Missing logger.error() in _recover_page and --fresh rmtree handler |
Medium |
| 6 | Comment says "Playwright >= 1.40" but TargetClosedError was introduced in 1.41 |
Low |
Strengths
- Error message UX is excellent — every path gives actionable guidance
--fresh+--browser-cookiesinteraction properly warns instead of silently ignoring- TargetClosedError vs network error distinction in retry messaging prevents confusion
- Partial cleanup communication in
auth logouttells user what succeeded/failed - Logger calls in new code use
%s-style formatting (lazy evaluation) - Safe fallback design — if Playwright changes the error message, errors propagate unhandled rather than being silently swallowed
See inline comments for details on each finding.
|
|
||
| try: | ||
| return context.new_page() | ||
| except PlaywrightError as exc: |
There was a problem hiding this comment.
[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)
raiseThis 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
There was a problem hiding this comment.
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.
src/notebooklm/cli/session.py
Outdated
| try: | ||
| storage_path.unlink() | ||
| removed_any = True | ||
| except OSError: |
There was a problem hiding this comment.
[High] Three concerns with the OSError handling in auth logout:
- This
except OSError:doesn't bind the exception — the error details are truly discarded from Noneat line 864 explicitly suppresses the exception chain- No
logger.error()call — failure is invisible in logs
The user always sees "may be in use by another process" regardless of actual cause (could be PermissionError, ReadOnlyFilesystemError, or even FileNotFoundError from a TOCTOU race).
Suggestion:
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 excSame pattern applies to the shutil.rmtree handler at line 871.
Found by: Claude Silent Failure Hunter
There was a problem hiding this comment.
Addressed in 4ac30ee: bound OSError as exc, switched from from None to from exc for proper chaining, added logger.error(), and now show the actual error in the user message (e.g. Cannot remove auth file: {exc}).
| pass | ||
|
|
||
| @auth_group.command("logout") | ||
| def auth_logout(): |
There was a problem hiding this comment.
[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)
There was a problem hiding this comment.
Addressed in 4ac30ee: added NOTEBOOKLM_AUTH_JSON warning at the top of auth logout, mirroring login's guard pattern.
src/notebooklm/cli/session.py
Outdated
| # Retryable Playwright connection errors | ||
| RETRYABLE_CONNECTION_ERRORS = ("ERR_CONNECTION_CLOSED", "ERR_CONNECTION_RESET") | ||
| LOGIN_MAX_RETRIES = 3 | ||
| # Playwright TargetClosedError substring — verified against Playwright >= 1.40. |
There was a problem hiding this comment.
[Low] Playwright's TargetClosedError class was introduced in version 1.41 (not 1.40). Consider updating the version reference or replacing it with a source-of-truth reference:
# Playwright TargetClosedError substring — matches the default message from
# Playwright's TargetClosedError class. If a future version changes this
# message, the error will propagate unhandled (safe fallback).Found by: Claude Comment Analyzer
There was a problem hiding this comment.
Addressed in 4ac30ee: updated comment to reference v1.41 and removed the specific version claim in favor of a source-of-truth description.
| shutil.rmtree(browser_profile) | ||
| console.print("[yellow]Cleared cached browser session (--fresh)[/yellow]") | ||
| except OSError as exc: | ||
| console.print( |
There was a problem hiding this comment.
[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
There was a problem hiding this comment.
Addressed in 4ac30ee: added logger.error() to the --fresh OSError path and both paths in _recover_page.
|
|
||
|
|
||
| # ============================================================================= | ||
| # USE COMMAND TESTS |
There was a problem hiding this comment.
[Medium] Missing test: the cookie-forcing double-failure path (session.py:592-595) where the recovered page also raises TargetClosedError is never exercised. This is the final safety net — a regression here would give users an unhandled traceback instead of BROWSER_CLOSED_HELP.
Suggestion — add a test where mock_context.new_page() returns a page whose goto() also raises TargetClosedError for the cookie-forcing URLs (calls 2+).
Found by: Claude Test Analyzer
There was a problem hiding this comment.
Addressed in 4ac30ee: added test_login_cookie_forcing_double_failure_shows_browser_closed — exercises the inner Site B path where the recovered page also raises TargetClosedError. 2026 tests now passing.
Unaddressed review comments from other reviewersI checked all comments from CodeRabbit, Gemini, and Copilot. Most were addressed or correctly rejected. Two sound items remain: 1. Type hints for Both CodeRabbit review rounds flagged the missing type annotations on from typing import TYPE_CHECKING
if TYPE_CHECKING:
from playwright.sync_api import BrowserContext, Page
from rich.console import Console
def _recover_page(context: "BrowserContext", console: "Console") -> "Page":Priority: Low — the function works fine without it, but it aligns with the project's direction. 2. Inconsistent f-string logger formatting (CodeRabbit round 2) Lines 559-562 and 567 still use f-strings in 3. Gemini's @Halmurat-Uyghur's reasoning is sound: |
- Narrow _recover_page catch to TARGET_CLOSED_ERROR only, re-raise other PlaywrightError types to avoid masking real problems - Add logger.error() to all terminal error paths (--fresh, logout, _recover_page) for consistent observability - Bind OSError as exc in auth logout, use 'from exc' for proper exception chaining, show actual error in user messages - Warn about NOTEBOOKLM_AUTH_JSON env var in auth logout - Fix version comment (1.40 -> 1.41) - Add type hints to _recover_page via TYPE_CHECKING guard - Add test for cookie-forcing double-failure path
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/notebooklm/cli/session.py (1)
452-463:⚠️ Potential issue | 🟡 MinorTreat
FileNotFoundErroras success in these cleanup paths.Both branches do an
exists()check before deleting, so another process can still remove the path in between. Right now that race turns an otherwise-idempotent cleanup into a fatal error even though the desired state is already reached.💡 Suggested hardening
if fresh and browser_profile.exists(): try: shutil.rmtree(browser_profile) console.print("[yellow]Cleared cached browser session (--fresh)[/yellow]") + except FileNotFoundError: + pass except OSError as exc: logger.error("Failed to clear browser profile %s: %s", browser_profile, exc) console.print( 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 storage_path.exists(): try: storage_path.unlink() removed_any = True + except FileNotFoundError: + pass 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 if browser_profile.exists(): try: shutil.rmtree(browser_profile) removed_any = True + except FileNotFoundError: + pass 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 "" )Also applies to: 873-905
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notebooklm/cli/session.py` around lines 452 - 463, The cleanup path that calls shutil.rmtree(browser_profile) (inside the fresh branch in session code) should treat FileNotFoundError as success because exists() can race; update the exception handling around shutil.rmtree in the block that currently catches OSError (and in the duplicate block handling the other browser_profile cleanup) to either catch FileNotFoundError separately or detect it on the caught OSError and simply log/debug and continue instead of raising SystemExit(1); preserve the existing behavior for other errors (log error, print message, and exit).
🧹 Nitpick comments (1)
tests/unit/cli/test_session.py (1)
1691-1798: Add coverage for the env-auth warning.
auth_logout()now has a user-facingNOTEBOOKLM_AUTH_JSONbranch, but none of the new logout tests assert it. A small regression case here would keep the success/no-op messaging from becoming misleading again.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/cli/test_session.py` around lines 1691 - 1798, The tests are missing coverage for the new NOTEBOOKLM_AUTH_JSON warning branch in auth_logout; add a test (or extend an existing logout test) that sets the NOTEBOOKLM_AUTH_JSON env var (use patch.dict(os.environ, {"NOTEBOOKLM_AUTH_JSON": "1"}) or runner/monkeypatch) and invokes cli auth logout, then assert the output contains the env-var warning text (e.g., mentions "NOTEBOOKLM_AUTH_JSON" or the specific warning message emitted by auth_logout) while preserving existing exit_code expectations; target the auth_logout call path in notebooklm.cli.session (function auth_logout) so the new branch is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/notebooklm/cli/session.py`:
- Around line 452-463: The cleanup path that calls
shutil.rmtree(browser_profile) (inside the fresh branch in session code) should
treat FileNotFoundError as success because exists() can race; update the
exception handling around shutil.rmtree in the block that currently catches
OSError (and in the duplicate block handling the other browser_profile cleanup)
to either catch FileNotFoundError separately or detect it on the caught OSError
and simply log/debug and continue instead of raising SystemExit(1); preserve the
existing behavior for other errors (log error, print message, and exit).
---
Nitpick comments:
In `@tests/unit/cli/test_session.py`:
- Around line 1691-1798: The tests are missing coverage for the new
NOTEBOOKLM_AUTH_JSON warning branch in auth_logout; add a test (or extend an
existing logout test) that sets the NOTEBOOKLM_AUTH_JSON env var (use
patch.dict(os.environ, {"NOTEBOOKLM_AUTH_JSON": "1"}) or runner/monkeypatch) and
invokes cli auth logout, then assert the output contains the env-var warning
text (e.g., mentions "NOTEBOOKLM_AUTH_JSON" or the specific warning message
emitted by auth_logout) while preserving existing exit_code expectations; target
the auth_logout call path in notebooklm.cli.session (function auth_logout) so
the new branch is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3c32482b-3b91-405e-952f-a75ab46f8a78
📒 Files selected for processing (2)
src/notebooklm/cli/session.pytests/unit/cli/test_session.py
Summary
Fixes #246 — Cannot switch Google accounts: login reuses cached browser profile + crashes on new account.
--freshflag onlogin: Deletes the persistent browser profile (~/.notebooklm/profiles/<profile>/browser_profile/) before launching Playwright, giving users a clean browser session with no cached Google cookiesauth logoutcommand: Clears bothstorage_state.jsonandbrowser_profile/for the active profile, providing CLI symmetry withloginTargetClosedErrorhandling: Catches page destruction in both the initial navigation retry loop and the cookie-forcing section, recovering viacontext.new_page()with a browser-specific error message when recovery failsTest plan
ruff format+ruff check+mypyall cleanERR_CONNECTION_CLOSED,ERR_CONNECTION_RESET,Navigation interrupted) unaffectedChanges
src/notebooklm/cli/session.py--freshflag,auth logout,_recover_pagehelper,TargetClosedErrorhandlingtests/unit/cli/test_session.pyTestLoginCommandandTestAuthLogoutCommandSummary by CodeRabbit
New Features
--freshflag to login to clear cached browser profile before authentication (warns if cookie-only mode is used).auth logoutcommand to remove saved session state and cached browser profile with clear user messaging.Bug Fixes
Tests
--fresh, logout, and recovery/failure scenarios.