Conversation
![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
The pull request aims to fix redirect URLs by adding a forward slash. However, this approach can lead to double slashes if the base URL already ends with one, which could cause issues. I've suggested a more robust way to construct the URLs using rstrip('/') to prevent this. I also pointed out that this URL construction logic is duplicated in several places and could be refactored for better maintainability.
| raise HTTPException(status_code=500, detail="Todoist not configured") | ||
|
|
||
| redirect_uri = f'{base_url}v2/integrations/todoist/callback' | ||
| redirect_uri = f'{base_url}/v2/integrations/todoist/callback' |
There was a problem hiding this comment.
While adding the / fixes the issue when base_url is missing a trailing slash, it introduces a potential issue: if base_url does have a trailing slash, the resulting URL will have a double slash (e.g., http://host//v2/...). While many servers handle this gracefully, it's better to be more robust by ensuring there's always exactly one slash.
This pattern is repeated for all providers in this function (todoist, asana, google_tasks, clickup). You should apply this more robust approach for all of them. You could also consider refactoring this to define redirect_uri once before the if/elif block to reduce duplication.
| redirect_uri = f'{base_url}/v2/integrations/todoist/callback' | |
| redirect_uri = f'{base_url.rstrip("/")}/v2/integrations/todoist/callback' |
| return render_oauth_response(request, 'asana', success=False, error_type='config_error') | ||
|
|
||
| redirect_uri = f'{base_url}v2/integrations/asana/callback' | ||
| redirect_uri = f'{base_url}/v2/integrations/asana/callback' |
There was a problem hiding this comment.
Similar to my other comment, adding a hardcoded / can lead to double slashes (//) in the URL if base_url already has a trailing slash. Using rstrip('/') makes this more robust and prevents potential issues with the redirect URI.
| redirect_uri = f'{base_url}/v2/integrations/asana/callback' | |
| redirect_uri = f'{base_url.rstrip("/")}/v2/integrations/asana/callback' |
| return render_oauth_response(request, 'google_tasks', success=False, error_type='config_error') | ||
|
|
||
| redirect_uri = f'{base_url}v2/integrations/google-tasks/callback' | ||
| redirect_uri = f'{base_url}/v2/integrations/google-tasks/callback' |
There was a problem hiding this comment.
Similar to my other comments, adding a hardcoded / can lead to double slashes (//) in the URL if base_url already has a trailing slash. Using rstrip('/') makes this more robust and prevents potential issues with the redirect URI.
| redirect_uri = f'{base_url}/v2/integrations/google-tasks/callback' | |
| redirect_uri = f'{base_url.rstrip("/")}/v2/integrations/google-tasks/callback' |
2 participants
No description provided.