korbit-ai[bot] commented on code in PR #35345:
URL: https://github.com/apache/superset/pull/35345#discussion_r2392273981
##########
superset/views/core.py:
##########
@@ -762,16 +762,28 @@ def dashboard(
default value to appease pylint
"""
+ def redirect_to_login() -> FlaskResponse:
+ login_url = appbuilder.get_url_for_login
+ parsed = parse.urlparse(login_url)
+ query = parse.parse_qs(parsed.query, keep_blank_values=True)
+ query["next"] = [request.url]
+ encoded_query = parse.urlencode(query, doseq=True)
+ redirect_url =
parse.urlunparse(parsed._replace(query=encoded_query))
+ return redirect(redirect_url)
Review Comment:
### Login redirect logic not properly abstracted <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The redirect_to_login function is defined inside the dashboard method but
contains logic that could be reused across multiple endpoints that need login
redirection.
###### Why this matters
This violates the DRY principle and makes the codebase harder to maintain.
If login redirect logic needs to change, it would need to be updated in
multiple places.
###### Suggested change ∙ *Feature Preview*
Extract the redirect_to_login function to a utility module or base class:
```python
# In utils.py or auth.py
def get_login_redirect_url(next_url: str) -> str:
login_url = appbuilder.get_url_for_login
parsed = parse.urlparse(login_url)
query = parse.parse_qs(parsed.query, keep_blank_values=True)
query["next"] = [next_url]
encoded_query = parse.urlencode(query, doseq=True)
return parse.urlunparse(parsed._replace(query=encoded_query))
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b0972601-2ef9-49ea-b707-3826db899e3a/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b0972601-2ef9-49ea-b707-3826db899e3a?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b0972601-2ef9-49ea-b707-3826db899e3a?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b0972601-2ef9-49ea-b707-3826db899e3a?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b0972601-2ef9-49ea-b707-3826db899e3a)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:7773efde-f9b1-4823-967c-6f387a4b2d0e -->
[](7773efde-f9b1-4823-967c-6f387a4b2d0e)
##########
superset-frontend/src/pages/Login/index.tsx:
##########
@@ -82,6 +82,26 @@ export default function Login() {
const dispatch = useDispatch();
const bootstrapData = getBootstrapData();
+ const nextUrl = useMemo(() => {
+ try {
+ const params = new URLSearchParams(window.location.search);
+ return params.get('next') || '';
+ } catch (_error) {
+ return '';
+ }
+ }, []);
Review Comment:
### Missing dependency in nextUrl useMemo <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The nextUrl useMemo has an empty dependency array but accesses
window.location.search, which can change during the component's lifecycle
without triggering a re-computation.
###### Why this matters
This could lead to stale URL parameter values if the component remains
mounted while the URL changes, causing incorrect redirect behavior and
potential security issues with outdated next parameters.
###### Suggested change ∙ *Feature Preview*
Add window.location.search to the dependency array or use useEffect with
proper cleanup to listen for URL changes:
```typescript
const nextUrl = useMemo(() => {
try {
const params = new URLSearchParams(window.location.search);
return params.get('next') || '';
} catch (_error) {
return '';
}
}, [window.location.search]);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cdc66088-a51c-45a0-90ab-5562f5b22c89/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cdc66088-a51c-45a0-90ab-5562f5b22c89?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cdc66088-a51c-45a0-90ab-5562f5b22c89?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cdc66088-a51c-45a0-90ab-5562f5b22c89?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cdc66088-a51c-45a0-90ab-5562f5b22c89)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:3e84f52a-b2a1-436d-b9b1-602148049caa -->
[](3e84f52a-b2a1-436d-b9b1-602148049caa)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
