feat: persist user frontend theme preference#2
Conversation
|
/gemini review |
![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 implements a frontend theme switching feature, allowing users to choose between "classic" and "default" UI styles. The implementation includes backend logic for theme normalization, cookie management, and persistence in user settings, alongside frontend components for theme selection and automatic redirection. Feedback identifies performance concerns regarding redundant database queries in the theme resolution and user update processes, and recommends using the standard API utility in the classic UI for consistency and better error handling.
| 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 current implementation of resolveFrontendTheme performs a database query (model.GetUserById) on every request that hits NoRoute, which includes all frontend routes and static assets. This will cause significant performance degradation under load.
It is highly recommended to check the frontend_theme cookie first. If the cookie is present and valid, use it to avoid the database lookup. Only query the database if the cookie is missing and a valid session exists.
func resolveFrontendTheme(c *gin.Context) string {
if themeCookie, err := c.Cookie(common.FrontendThemeCookieName); err == nil {
if theme := common.NormalizeFrontendTheme(themeCookie); 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 err = DB.First(user, user.Id).Error; err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Adding a DB.First call inside the general Update method introduces an extra database query for every user update operation across the entire application. This is inefficient and usually unnecessary as GORM's Updates already modifies the fields in the local struct instance. If a specific caller needs to ensure the object is fully synced with database-side changes (like triggers or default values), they should perform the reload explicitly.
| 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 with hardcoded paths and custom headers, bypassing the application's standard API utility. This leads to inconsistency, bypasses global error handling/interceptors, and may fail if the application is hosted on a subpath or requires specific authentication headers. It should be removed in favor of using the existing API helper.
| const previousTheme = currentFrontendTheme; | ||
|
|
||
| try { | ||
| await updateFrontendThemePreference(theme, userState?.user?.id); |
There was a problem hiding this comment.
Replace the custom fetch call with the standard API utility for consistency and better error handling.
| await updateFrontendThemePreference(theme, userState?.user?.id); | |
| const res = await API.put('/api/user/self', { frontend_theme: theme }); | |
| if (!res.data.success) throw new Error(res.data.message || t('界面风格切换失败')); |
2 participants
Summary
frontend_themeinto user settings for cross-device syncTesting