fix: Explicit NEEDS_APPLICATION_ID declaration #87
Conversation
WalkthroughIntroduces NEEDS_APPLICATION_ID flags on auth adapter functions and updates init_auth to use config_factory(), resolve adapters from a mapping, consult getattr(adapter, "NEEDS_APPLICATION_ID", False) to decide whether to prompt for application_id, read client_secret via getpass, broaden device_auth signature, and remove config.store persistence. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant init_auth
participant Adapter as Auth Adapter (pin/oauth/device)
participant ConfigFactory as config_factory()
participant Config as config
participant Prompt as get_client_info
Note over init_auth,ConfigFactory: init_auth constructs config via config_factory()
User->>init_auth: Start authentication
init_auth->>ConfigFactory: config = config_factory()
init_auth->>Adapter: Resolve adapter from methods mapping (default pin_auth)
init_auth->>Adapter: Read NEEDS_APPLICATION_ID via getattr(adapter, "NEEDS_APPLICATION_ID", False)
alt NEEDS_APPLICATION_ID == True
init_auth->>Config: Read application_id, client_id, client_secret
init_auth->>Prompt: Prompt(app_id + client_id + client_secret) (client_secret via getpass)
else NEEDS_APPLICATION_ID == False
init_auth->>Config: Read client_id, client_secret
init_auth->>Prompt: Prompt(client_id + client_secret) (client_secret via getpass)
end
Prompt-->>init_auth: Credentials
init_auth->>Adapter: Call adapter with provided credentials
Adapter-->>User: Auth result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
trakt/auth/__init__.py (3)
66-68: Bug: default branch returns a constant, not a callable.
methods.get(method, PIN_AUTH)falls back to the string/constant (e.g., "pin"), not the wrapper function. Use the function itself.Apply:
- adapter = methods.get(method, PIN_AUTH) + adapter = methods.get(method, pin_auth)
72-74: Harden against missing NEEDS_APPLICATION_ID on custom adapters.If a downstream project supplies a different callable without this attribute, this will raise. Default to False.
Apply:
- client_id, client_secret = get_client_info(adapter.NEEDS_APPLICATION_ID, config) + client_id, client_secret = get_client_info(getattr(adapter, "NEEDS_APPLICATION_ID", False), config)
33-49: Don’t echo secrets to the terminal.Use getpass for the client secret input.
Apply:
-def get_client_info(app_id: bool, config: AuthConfig): +def get_client_info(app_id: bool, config: AuthConfig): + from getpass import getpass @@ - client_secret = input('Please enter your client secret: ') + client_secret = getpass('Please enter your client secret: ')
🧹 Nitpick comments (1)
trakt/auth/__init__.py (1)
23-27: Unify wrapper signatures to avoid TypeError when kwargs are passed.
init_authforwards*args, **kwargs.device_authdoes not accept them, risking a TypeError if any are provided.Apply:
-def device_auth(config): +def device_auth(*args, config, **kwargs): from trakt.auth.device import DeviceAuthAdapter - return DeviceAuthAdapter(client=api(), config=config).authenticate() + return DeviceAuthAdapter(*args, client=api(), config=config, **kwargs).authenticate()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
trakt/auth/__init__.py(1 hunks)
🔇 Additional comments (1)
trakt/auth/__init__.py (1)
28-31: Good call: explicit NEEDS_APPLICATION_ID flags on wrappers.Clear, localized signal for prompting behavior. This aligns with the PR goal.
|
Actually good advices here: #87 (review) |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
trakt/auth/__init__.py (1)
68-76: Fail fast on unknown auth method and load config before prompting.
Defaulting to PIN on unknownmethodcan surprise callers; raise instead. Also ensure config is loaded before prompting.- config = config_factory() - adapter = methods.get(method, pin_auth) + config = config_factory() + config.load() + try: + adapter = methods[method] + except KeyError: + raise ValueError( + f"Unknown auth method: {method!r}. Expected one of: {', '.join(methods)}" + )
🧹 Nitpick comments (1)
trakt/auth/__init__.py (1)
25-28: Clarify that device_auth ignores extra args.
*args/**kwargsare accepted but unused; add a short docstring/comment to prevent misuse.def device_auth(*args, config, **kwargs): from trakt.auth.device import DeviceAuthAdapter - - return DeviceAuthAdapter(client=api(), config=config).authenticate() + """Wrapper for DeviceAuthAdapter.authenticate(); *args/**kwargs are ignored.""" + # For API symmetry with other auth wrappers; we intentionally ignore *args/**kwargs. + return DeviceAuthAdapter(client=api(), config=config).authenticate()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
trakt/auth/__init__.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
trakt/auth/__init__.py (3)
trakt/core.py (2)
config(73-83)api(87-104)trakt/auth/device.py (1)
DeviceAuthAdapter(10-115)trakt/config.py (2)
AuthConfig(12-72)store(66-72)
🔇 Additional comments (4)
trakt/auth/__init__.py (4)
4-6: Minor: Import and metadata update look good.
No action needed.
13-17: Unified wrapper signatures are consistent and explicit (keyword-only config).
Good use of keyword-only param forconfigand pass-through of*args/**kwargs.Also applies to: 19-23
31-34: Good: NEEDS_APPLICATION_ID is explicit on wrappers.
This cleanly decouples prompting from adapter internals.
58-86: Verified init_auth usage and config.load behavior—no changes needed
| print( | ||
| "If you do not have a client ID and secret. Please visit the " | ||
| "following url to create them." | ||
| ) | ||
| print("https://trakt.tv/oauth/applications") | ||
| client_id = input("Please enter your client id: ") | ||
| client_secret = getpass("Please enter your client secret: ") | ||
| if app_id: | ||
| msg = f'Please enter your application ID ({config.APPLICATION_ID}): ' | ||
| msg = f"Please enter your application ID ({config.APPLICATION_ID}): " | ||
| user_input = input(msg) | ||
| if user_input: | ||
| config.APPLICATION_ID = user_input | ||
| return client_id, client_secret |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make prompts resilient and reuse existing config values.
Allow pressing Enter to keep preloaded values; don’t force retyping secrets.
- print(
- "If you do not have a client ID and secret. Please visit the "
- "following url to create them."
- )
- print("https://trakt.tv/oauth/applications")
- client_id = input("Please enter your client id: ")
- client_secret = getpass("Please enter your client secret: ")
+ print(
+ "If you do not have a client ID and secret. Please visit the "
+ "following url to create them."
+ )
+ print("https://trakt.tv/oauth/applications")
+ default_client_id = config.CLIENT_ID or ""
+ cid_prompt = "Please enter your client id"
+ if default_client_id:
+ cid_prompt += f" ({default_client_id})"
+ cid_prompt += ": "
+ client_id = input(cid_prompt) or default_client_id
+
+ secret_prompt = "Please enter your client secret (leave blank to keep existing): "
+ secret_input = getpass(secret_prompt)
+ client_secret = secret_input or (config.CLIENT_SECRET or "")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| print( | |
| "If you do not have a client ID and secret. Please visit the " | |
| "following url to create them." | |
| ) | |
| print("https://trakt.tv/oauth/applications") | |
| client_id = input("Please enter your client id: ") | |
| client_secret = getpass("Please enter your client secret: ") | |
| if app_id: | |
| msg = f'Please enter your application ID ({config.APPLICATION_ID}): ' | |
| msg = f"Please enter your application ID ({config.APPLICATION_ID}): " | |
| user_input = input(msg) | |
| if user_input: | |
| config.APPLICATION_ID = user_input | |
| return client_id, client_secret | |
| print( | |
| "If you do not have a client ID and secret. Please visit the " | |
| "following url to create them." | |
| ) | |
| print("https://trakt.tv/oauth/applications") | |
| # Allow keeping existing client ID | |
| default_client_id = config.CLIENT_ID or "" | |
| cid_prompt = "Please enter your client id" | |
| if default_client_id: | |
| cid_prompt += f" ({default_client_id})" | |
| cid_prompt += ": " | |
| client_id = input(cid_prompt) or default_client_id | |
| # Allow keeping existing client secret | |
| secret_prompt = "Please enter your client secret (leave blank to keep existing): " | |
| secret_input = getpass(secret_prompt) | |
| client_secret = secret_input or (config.CLIENT_SECRET or "") | |
| if app_id: | |
| msg = f"Please enter your application ID ({config.APPLICATION_ID}): " | |
| user_input = input(msg) | |
| if user_input: | |
| config.APPLICATION_ID = user_input | |
| return client_id, client_secret |
🤖 Prompt for AI Agents
In trakt/auth/__init__.py around lines 43 to 55, the interactive prompts always
force the user to retype values and expose no defaults; update prompts to
display existing config values as defaults and accept empty input (Enter) to
keep them: if config.CLIENT_ID or config.CLIENT_SECRET exist show them in the
prompt and when input is empty preserve the existing value (for secrets use
getpass but treat blank as “keep existing” without printing), and for
APPLICATION_ID keep the current config.APPLICATION_ID as the default and only
replace it if the user types a non-empty value; finally return the possibly
unchanged client_id and client_secret.
| def init_auth( | ||
| method: str, *args, client_id=None, client_secret=None, store=False, **kwargs | ||
| ): |
There was a problem hiding this comment.
Bug: store parameter is unused (functional regression).
Docstring advertises persistence via store, but there is no call to config.store(). Either implement persistence or remove the param/docs.
See proposed fix in Lines 86.
🤖 Prompt for AI Agents
In trakt/auth/__init__.py around lines 58-60, the init_auth function accepts a
store parameter but never persists the resulting config, causing a functional
regression; modify the function so that after the config is created/updated
(where config variable is available, e.g., after login/creation at the later
lines around 86), if store is True call config.store() (handle/propagate any
exceptions or log errors as appropriate) so the configuration is actually
persisted; alternatively, if persistence is intentionally removed, remove the
store parameter and update the docstring accordingly.
| if client_id is None and client_secret is None: | ||
| client_id, client_secret = get_client_info(adapter.NEEDS_APPLICATION_ID, config) | ||
| client_id, client_secret = get_client_info( | ||
| getattr(adapter, "NEEDS_APPLICATION_ID", False), config | ||
| ) | ||
| config.CLIENT_ID, config.CLIENT_SECRET = client_id, client_secret |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prompt when either client_id or client_secret is missing; keep provided values intact.
Currently prompts only when both are None; handle partial input.
- if client_id is None and client_secret is None:
- client_id, client_secret = get_client_info(
- getattr(adapter, "NEEDS_APPLICATION_ID", False), config
- )
+ if client_id is None or client_secret is None:
+ entered_id, entered_secret = get_client_info(
+ getattr(adapter, "NEEDS_APPLICATION_ID", False), config
+ )
+ client_id = client_id or entered_id
+ client_secret = client_secret or entered_secret📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if client_id is None and client_secret is None: | |
| client_id, client_secret = get_client_info(adapter.NEEDS_APPLICATION_ID, config) | |
| client_id, client_secret = get_client_info( | |
| getattr(adapter, "NEEDS_APPLICATION_ID", False), config | |
| ) | |
| config.CLIENT_ID, config.CLIENT_SECRET = client_id, client_secret | |
| if client_id is None or client_secret is None: | |
| entered_id, entered_secret = get_client_info( | |
| getattr(adapter, "NEEDS_APPLICATION_ID", False), config | |
| ) | |
| client_id = client_id or entered_id | |
| client_secret = client_secret or entered_secret | |
| config.CLIENT_ID, config.CLIENT_SECRET = client_id, client_secret |
🤖 Prompt for AI Agents
In trakt/auth/__init__.py around lines 80 to 84, the code only prompts for
credentials when both client_id and client_secret are None; change it to prompt
when either is missing and preserve any provided value. Specifically, if
client_id is None or client_secret is None call get_client_info(getattr(adapter,
"NEEDS_APPLICATION_ID", False), config), then only overwrite the missing values
with the returned tuple (i.e., keep any non-None input intact), and finally
assign config.CLIENT_ID and config.CLIENT_SECRET from the merged values.
| ) | ||
| config.CLIENT_ID, config.CLIENT_SECRET = client_id, client_secret | ||
|
|
||
| adapter(*args, config=config, **kwargs) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return adapter result and persist when requested.
Preserve return value semantics and implement store=True.
- adapter(*args, config=config, **kwargs)
+ result = adapter(*args, config=config, **kwargs)
+ if store:
+ config.store()
+ return result📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| adapter(*args, config=config, **kwargs) | |
| result = adapter(*args, config=config, **kwargs) | |
| if store: | |
| config.store() | |
| return result |
🤖 Prompt for AI Agents
In trakt/auth/__init__.py around line 86, the adapter invocation currently
doesn't return its result nor handle persistence; change it to assign the
adapter call to a variable (result = adapter(*args, config=config, **kwargs)),
then if persistence is requested (e.g., config.get("store") is True or a store
flag in kwargs) call the configured persistence API to save the result (for
example invoke config.persist(result) or config["storage"].save(result)
depending on how config exposes storage), and finally return result so
return-value semantics are preserved.
1 participant
solves #80