fix(playground): render math formulas in markdown responses#4
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
This pull request introduces a theme switching feature allowing users to choose between "default" (new) and "classic" (old) UI styles. Key changes include backend support for theme persistence in user settings, custom routing logic to serve different frontend assets based on the selected theme, and UI components in both frontends for theme selection. Additionally, the "default" frontend now supports LaTeX/Math rendering in Markdown. Feedback highlights a performance concern regarding database lookups on every static asset request, a logic error in the user update endpoint that prevents batch updates when changing themes, a redundant database query in the user model, and inconsistent API usage in the classic frontend.
| if frontendTheme, themeExists := requestData["frontend_theme"]; themeExists { | ||
| userId := c.GetInt("id") | ||
| user, err := model.GetUserById(userId, false) | ||
| if err != nil { | ||
| common.ApiError(c, err) | ||
| return | ||
| } | ||
|
|
||
| currentSetting := user.GetSetting() | ||
| themeStr, ok := frontendTheme.(string) | ||
| if !ok { | ||
| common.ApiErrorI18n(c, i18n.MsgInvalidParams) | ||
| return | ||
| } | ||
| themeStr = strings.ToLower(strings.TrimSpace(themeStr)) | ||
| if themeStr != "default" && themeStr != "classic" { | ||
| common.ApiErrorI18n(c, i18n.MsgInvalidParams) | ||
| return | ||
| } | ||
| currentSetting.FrontendTheme = themeStr | ||
|
|
||
| user.SetSetting(currentSetting) | ||
| if err := user.Update(false); err != nil { | ||
| common.ApiErrorI18n(c, i18n.MsgUpdateFailed) | ||
| return | ||
| } | ||
| common.SetFrontendThemeCookie(c, themeStr) | ||
|
|
||
| common.ApiSuccessI18n(c, i18n.MsgUpdateSuccess, nil) | ||
| return |
There was a problem hiding this comment.
The logic for updating frontend_theme includes an early return. This prevents other user fields (such as username or display_name) from being updated if they are included in the same request payload. While the current UI might send these as separate requests, the API should ideally support batch updates or at least not silently skip other fields.
| if err = DB.First(user, user.Id).Error; err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
The addition of DB.First(user, user.Id) inside the Update method is redundant and introduces an unnecessary database query. GORM's Updates method already persists the changes to the database. If the caller requires the object to be reloaded (e.g., to see database-generated defaults), it should be handled explicitly at the call site rather than adding overhead to every update operation.
| const updateFrontendThemePreference = async (theme, userId) => { | ||
| const response = await fetch('/api/user/self', { | ||
| method: 'PUT', | ||
| credentials: 'same-origin', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| 'Cache-Control': 'no-store', | ||
| 'New-API-User': String(userId ?? -1), | ||
| }, | ||
| body: JSON.stringify({ | ||
| frontend_theme: theme, | ||
| }), | ||
| }); | ||
|
|
||
| const data = await response.json(); | ||
| if (!response.ok || !data?.success) { | ||
| throw new Error(data?.message || 'save frontend theme failed'); | ||
| } | ||
| return data; | ||
| }; |
There was a problem hiding this comment.
The updateFrontendThemePreference function uses the native fetch API and hardcodes headers like New-API-User. This is inconsistent with the rest of the file (and project), which uses the API helper (imported at line 24). Using the API helper ensures that global configurations, error handling, and authentication headers are applied consistently.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a theme switching mechanism that allows users to toggle between "default" (new) and "classic" (old) UI styles. Key changes include backend support for theme persistence in user settings, cookie-based theme resolution, and dynamic routing to serve the appropriate frontend assets and handle path redirects (e.g., /dashboard vs /console). The PR also introduces LaTeX math rendering support in the new frontend and standardizes JSON marshaling across the codebase. Review feedback highlights several performance concerns, specifically the overhead of database lookups for every static asset request and redundant queries during user updates. Additionally, there are suggestions to improve the API's flexibility by supporting multi-field updates and maintaining consistency by using established API helpers instead of native fetch.
| func resolveFrontendTheme(c *gin.Context) string { | ||
| session := sessions.Default(c) | ||
| if sessionID := session.Get("id"); sessionID != nil { | ||
| if userID, ok := sessionID.(int); ok && userID > 0 { | ||
| user, err := model.GetUserById(userID, false) | ||
| if err == nil { | ||
| theme := common.NormalizeFrontendTheme(user.GetSetting().FrontendTheme) | ||
| if theme != "" { | ||
| common.SetFrontendThemeCookie(c, theme) | ||
| return theme | ||
| } | ||
| } | ||
| } | ||
| } | ||
| themeCookie, err := c.Cookie(common.FrontendThemeCookieName) | ||
| if err == nil { | ||
| theme := common.NormalizeFrontendTheme(themeCookie) | ||
| if theme != "" { | ||
| return theme | ||
| } | ||
| } | ||
| return common.GetTheme() | ||
| } |
There was a problem hiding this comment.
The resolveFrontendTheme function currently prioritizes a database lookup (model.GetUserById) on every non-API request. Since this handler is used for serving all static assets (JS, CSS, images) via the NoRoute logic, this will cause a significant performance bottleneck by hitting the database multiple times for every page load.
It is highly recommended to check the frontend_theme cookie first and only fall back to the database if the cookie is missing and the user is authenticated.
func resolveFrontendTheme(c *gin.Context) string {
themeCookie, err := c.Cookie(common.FrontendThemeCookieName)
if err == nil {
theme := common.NormalizeFrontendTheme(themeCookie)
if theme != "" {
return theme
}
}
session := sessions.Default(c)
if sessionID := session.Get("id"); sessionID != nil {
if userID, ok := sessionID.(int); ok && userID > 0 {
user, err := model.GetUserById(userID, false)
if err == nil {
theme := common.NormalizeFrontendTheme(user.GetSetting().FrontendTheme)
if theme != "" {
common.SetFrontendThemeCookie(c, theme)
return theme
}
}
}
}
return common.GetTheme()
}| if frontendTheme, themeExists := requestData["frontend_theme"]; themeExists { | ||
| userId := c.GetInt("id") | ||
| user, err := model.GetUserById(userId, false) | ||
| if err != nil { | ||
| common.ApiError(c, err) | ||
| return | ||
| } | ||
|
|
||
| currentSetting := user.GetSetting() | ||
| themeStr, ok := frontendTheme.(string) | ||
| if !ok { | ||
| common.ApiErrorI18n(c, i18n.MsgInvalidParams) | ||
| return | ||
| } | ||
| themeStr = strings.ToLower(strings.TrimSpace(themeStr)) | ||
| if themeStr != "default" && themeStr != "classic" { | ||
| common.ApiErrorI18n(c, i18n.MsgInvalidParams) | ||
| return | ||
| } | ||
| currentSetting.FrontendTheme = themeStr | ||
|
|
||
| user.SetSetting(currentSetting) | ||
| if err := user.Update(false); err != nil { | ||
| common.ApiErrorI18n(c, i18n.MsgUpdateFailed) | ||
| return | ||
| } | ||
| common.SetFrontendThemeCookie(c, themeStr) | ||
|
|
||
| common.ApiSuccessI18n(c, i18n.MsgUpdateSuccess, nil) | ||
| return |
There was a problem hiding this comment.
The update logic for frontend_theme returns early, which prevents other user profile fields from being updated in the same request if they are provided together in the payload. While the frontend might currently send these separately, the API should ideally support partial updates of multiple fields simultaneously.
| if err = DB.First(user, user.Id).Error; err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Adding a mandatory DB.First(user, user.Id) after every update operation introduces an additional database query. This can lead to performance degradation in scenarios with frequent user updates. Consider if this full reload is strictly necessary for the subsequent cache update or if it can be optimized by only updating the changed fields in the cache.
| const updateFrontendThemePreference = async (theme, userId) => { | ||
| const response = await fetch('/api/user/self', { | ||
| method: 'PUT', | ||
| credentials: 'same-origin', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| 'Cache-Control': 'no-store', | ||
| 'New-API-User': String(userId ?? -1), | ||
| }, | ||
| body: JSON.stringify({ | ||
| frontend_theme: theme, | ||
| }), | ||
| }); | ||
|
|
||
| const data = await response.json(); | ||
| if (!response.ok || !data?.success) { | ||
| throw new Error(data?.message || 'save frontend theme failed'); | ||
| } | ||
| return data; | ||
| }; |
There was a problem hiding this comment.
The updateFrontendThemePreference function uses the native fetch API instead of the project's established API helper. This inconsistency bypasses centralized configurations for base URLs, authentication headers, and global error handling. It's better to stay consistent with the rest of the file's logic (e.g., how language preferences are saved).
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a theme-switching mechanism between the new and classic UIs, including backend support for theme persistence and path redirection. It also adds LaTeX rendering support to markdown components in the new UI. Feedback highlights an opportunity to optimize the UpdateSelf controller by reducing redundant database operations and resolving variable shadowing, and suggests using the project's API helper instead of fetch in the classic UI for better consistency.
| user, err := model.GetUserById(userId, false) | ||
| if err != nil { | ||
| common.ApiError(c, err) | ||
| return | ||
| } | ||
|
|
||
| // 获取当前用户设置 | ||
| currentSetting := user.GetSetting() | ||
|
|
||
| // 更新sidebar_modules字段 | ||
| if sidebarModulesStr, ok := sidebarModules.(string); ok { | ||
| if sidebarModules, ok := requestData["sidebar_modules"]; ok { | ||
| sidebarModulesStr, valid := sidebarModules.(string) | ||
| if !valid { | ||
| common.ApiErrorI18n(c, i18n.MsgInvalidParams) | ||
| return | ||
| } | ||
| currentSetting.SidebarModules = sidebarModulesStr | ||
| } | ||
|
|
||
| // 保存更新后的设置 | ||
| user.SetSetting(currentSetting) | ||
| if err := user.Update(false); err != nil { | ||
| common.ApiErrorI18n(c, i18n.MsgUpdateFailed) | ||
| return | ||
| if frontendTheme, ok := requestData["frontend_theme"]; ok { | ||
| themeStr, valid := frontendTheme.(string) | ||
| if !valid { | ||
| common.ApiErrorI18n(c, i18n.MsgInvalidParams) | ||
| return | ||
| } | ||
| themeStr = common.NormalizeFrontendTheme(themeStr) | ||
| if themeStr == "" { | ||
| common.ApiErrorI18n(c, i18n.MsgInvalidParams) | ||
| return | ||
| } | ||
| currentSetting.FrontendTheme = themeStr | ||
| updatedTheme = themeStr | ||
| } | ||
|
|
||
| common.ApiSuccessI18n(c, i18n.MsgUpdateSuccess, nil) | ||
| return | ||
| if language, ok := requestData["language"]; ok { | ||
| langStr, valid := language.(string) | ||
| if !valid { | ||
| common.ApiErrorI18n(c, i18n.MsgInvalidParams) | ||
| return | ||
| } | ||
| currentSetting.Language = langStr | ||
| } | ||
|
|
||
| updatedSetting = ¤tSetting | ||
| } | ||
|
|
||
| // 检查是否是语言偏好更新请求 | ||
| if language, langExists := requestData["language"]; langExists { | ||
| userId := c.GetInt("id") | ||
| user, err := model.GetUserById(userId, false) | ||
| hasProfileUpdate := false | ||
| for _, key := range []string{"username", "display_name", "password", "original_password"} { | ||
| if _, ok := requestData[key]; ok { | ||
| hasProfileUpdate = true | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if hasProfileUpdate { | ||
| var user model.User | ||
| requestDataBytes, err := common.Marshal(requestData) | ||
| if err != nil { | ||
| common.ApiError(c, err) | ||
| common.ApiErrorI18n(c, i18n.MsgInvalidParams) | ||
| return | ||
| } | ||
|
|
||
| // 获取当前用户设置 | ||
| currentSetting := user.GetSetting() | ||
|
|
||
| // 更新language字段 | ||
| if langStr, ok := language.(string); ok { | ||
| currentSetting.Language = langStr | ||
| err = common.Unmarshal(requestDataBytes, &user) | ||
| if err != nil { | ||
| common.ApiErrorI18n(c, i18n.MsgInvalidParams) | ||
| return | ||
| } | ||
|
|
||
| // 保存更新后的设置 | ||
| user.SetSetting(currentSetting) | ||
| if err := user.Update(false); err != nil { | ||
| common.ApiErrorI18n(c, i18n.MsgUpdateFailed) | ||
| if user.Password == "" { | ||
| user.Password = "$I_LOVE_U" // make Validator happy :) | ||
| } | ||
| if err := common.Validate.Struct(&user); err != nil { | ||
| common.ApiErrorI18n(c, i18n.MsgInvalidInput) | ||
| return | ||
| } | ||
|
|
||
| common.ApiSuccessI18n(c, i18n.MsgUpdateSuccess, nil) | ||
| return | ||
| cleanUser := model.User{ | ||
| Id: userId, | ||
| Username: user.Username, | ||
| Password: user.Password, | ||
| DisplayName: user.DisplayName, | ||
| } | ||
| if user.Password == "$I_LOVE_U" { | ||
| user.Password = "" | ||
| cleanUser.Password = "" | ||
| } | ||
| updatePassword, err := checkUpdatePassword(user.OriginalPassword, user.Password, cleanUser.Id) | ||
| if err != nil { | ||
| common.ApiError(c, err) | ||
| return | ||
| } | ||
| if err := cleanUser.Update(updatePassword); err != nil { | ||
| common.ApiError(c, err) | ||
| return | ||
| } | ||
| } | ||
|
|
||
| // 原有的用户信息更新逻辑 | ||
| var user model.User | ||
| requestDataBytes, err := json.Marshal(requestData) | ||
| if err != nil { | ||
| common.ApiErrorI18n(c, i18n.MsgInvalidParams) | ||
| return | ||
| } | ||
| err = json.Unmarshal(requestDataBytes, &user) | ||
| if err != nil { | ||
| if !settingChanged && !hasProfileUpdate { | ||
| common.ApiErrorI18n(c, i18n.MsgInvalidParams) | ||
| return | ||
| } | ||
|
|
||
| if user.Password == "" { | ||
| user.Password = "$I_LOVE_U" // make Validator happy :) | ||
| } | ||
| if err := common.Validate.Struct(&user); err != nil { | ||
| common.ApiErrorI18n(c, i18n.MsgInvalidInput) | ||
| return | ||
| if updatedSetting != nil { | ||
| user, err := model.GetUserById(userId, false) | ||
| if err != nil { | ||
| common.ApiError(c, err) | ||
| return | ||
| } | ||
| user.SetSetting(*updatedSetting) | ||
| if err := user.Update(false); err != nil { | ||
| common.ApiErrorI18n(c, i18n.MsgUpdateFailed) | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
The UpdateSelf function is performing multiple redundant database operations and contains variable shadowing that makes the logic inefficient and harder to follow.
- Redundant Fetch: The user is fetched at line 663 and then again at line 763.
- Variable Shadowing: The
uservariable from line 663 is shadowed by a new declaration at line 716 (var user model.User) within the profile update block, and then shadowed again at line 763. - Multiple Updates: If both profile and settings are changed, the code performs two separate
Updatecalls (lines 751 and 769), each triggering a database refresh due to the changes inmodel/user.go.
It is recommended to fetch the user once, apply all changes (profile and settings) to that single object, and then call Update once at the end of the function.
| const updateFrontendThemePreference = async (theme, userId) => { | ||
| const response = await fetch('/api/user/self', { | ||
| method: 'PUT', | ||
| credentials: 'same-origin', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| 'Cache-Control': 'no-store', | ||
| 'New-API-User': String(userId ?? -1), | ||
| }, | ||
| body: JSON.stringify({ | ||
| frontend_theme: theme, | ||
| }), | ||
| }); | ||
|
|
||
| const data = await response.json(); | ||
| if (!response.ok || !data?.success) { | ||
| throw new Error(data?.message || 'save frontend theme failed'); | ||
| } | ||
| return data; | ||
| }; |
There was a problem hiding this comment.
This function uses the native fetch API instead of the API helper used elsewhere in the project. This bypasses global configurations such as base URLs, timeout settings, or request/response interceptors defined in the helper. To maintain consistency and ensure all requests follow the same pipeline, please use the API helper.
const updateFrontendThemePreference = async (theme) => {
const res = await API.put('/api/user/self', {
frontend_theme: theme,
});
if (!res.data.success) {
throw new Error(res.data.message || 'save frontend theme failed');
}
return res.data;
};
2 participants
Summary
Verification