aminghadersohi commented on PR #36933: URL: https://github.com/apache/superset/pull/36933#issuecomment-3716796933
## Code Review Responses ### ✅ Fixed Issues | Comment | Fix | |---------|-----| | **Null pointer in validateMessageEvent** | Added null check: `if (data == null \|\| typeof data !== 'object' \|\| data.type !== MESSAGE_TYPE)` | | **Race condition with started flag** | Now sets `started = true` before `start()` and resets to `false` on failure | | **Security: exception message leak in 500** | Returns generic `response_500()` without exposing exception message | | **Exception chaining leaks internal details** | Removed `from ex`, re-raises directly or creates new instance | | **Missing docstrings in exceptions** | Added docstrings to all exception classes | ### ❌ Not Applicable / Incorrect Suggestions | Suggestion | Reason | |------------|--------| | **GUEST_TOKEN_HEADER_NAME KeyError** | Config value exists in default `config.py` (line 2121). Existing `embedded/view.py` uses same pattern. | | **Webpack path format** | The leading slash format matches existing `spa` and `embedded` entries which work correctly. | | **Feature flag confusion** | No `EMBEDDABLE_CHARTS` flag exists - only `EMBEDDED_SUPERSET`. The new `EMBEDDABLE_CHARTS_MCP` is intentionally separate. | | **Lazy import in __init__.py** | Follows existing pattern used throughout the MCP service codebase. | | **ttl_minutes None check** | Pydantic model provides `default=60`, so value is never None. | | **Import style in app.py** | Follows existing pattern for other tool imports in the file. | -- 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]
