feat(dashboard): add auto-switch light and dark button#7527
Open
kyangconn wants to merge 5 commits intoAstrBotDevs:masterfrom
Open
feat(dashboard): add auto-switch light and dark button#7527kyangconn wants to merge 5 commits intoAstrBotDevs:masterfrom
kyangconn wants to merge 5 commits intoAstrBotDevs:masterfrom
Conversation
dosubot
bot
added
the
size:XXL
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- In
StatsPage.vuethe chart color palette now uses the staticPurpleTheme/PurpleThemeDarkdefinitions instead of Vuetify’s current theme, so user-customized primary/secondary colors and presets in the new Settings theme controls will not be reflected in the charts; consider sourcing colors from the active theme/customizer instead to keep visuals consistent. - The theme preset implementation in
Settings.vuestores and matches presets by theirnamestring (which currently mixes Chinese and English), so any future renaming or localization of these labels will break persistence; it would be more robust to store and match by the stableidfield and derive display labels via i18n.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `StatsPage.vue` the chart color palette now uses the static `PurpleTheme`/`PurpleThemeDark` definitions instead of Vuetify’s current theme, so user-customized primary/secondary colors and presets in the new Settings theme controls will not be reflected in the charts; consider sourcing colors from the active theme/customizer instead to keep visuals consistent.
- The theme preset implementation in `Settings.vue` stores and matches presets by their `name` string (which currently mixes Chinese and English), so any future renaming or localization of these labels will break persistence; it would be more robust to store and match by the stable `id` field and derive display labels via i18n.
## Individual Comments
### Comment 1
<location path="dashboard/src/utils/request.ts" line_range="68-73" />
<code_context>
+ return stripTrailingSlashes(baseUrl?.trim() || "");
+}
+
+export function normalizeConfiguredApiBaseUrl(
+ baseUrl: string | null | undefined,
+): string {
+ const cleaned = normalizeBaseUrl(baseUrl);
+ // Prepend https:// if it doesn't already have a protocol
+ if (cleaned && !/^https?:\/\//i.test(cleaned)) {
+ return `https://${cleaned}`;
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Forcing `https://` for base URLs without protocol can break valid `http` setups.
In `normalizeConfiguredApiBaseUrl`, any `baseUrl` without a protocol is forced to `https://`, which will break valid HTTP-only configs (e.g. `localhost:8080` or intranet hosts) by silently switching them to HTTPS. Consider either deriving the protocol from `window.location.protocol` when available, or keeping the value as-is and surfacing invalid URLs via validation instead of implicitly upgrading to HTTPS.
</issue_to_address>
### Comment 2
<location path="dashboard/src/views/Settings.vue" line_range="420-392" />
<code_context>
+);
+
+// Theme presets based on MD3 color system
+const themePresets = [
+ {
+ id: "blue-business",
+ name: "活力商务蓝",
+ nameEn: "Business Blue",
+ primary: "#005FB0",
+ secondary: "#565E71",
+ tertiary: "#006B5B",
+ },
+ {
+ id: "purple-default",
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Storing the selected preset by localized name makes the preset lookup brittle.
In `applyThemePreset` and `selectedThemePreset`, the preset is looked up by `name`, which is a localized label and may change with translations or copy edits, breaking the mapping. Use a stable `id` (e.g. `blue-business`) for storage/comparison, and derive the display label from i18n or from `name`/`nameEn` only when rendering.
Suggested implementation:
```
// Theme presets based on MD3 color system
// Presets are referenced by a stable `id`; localized `name`/`nameEn` are used only for display.
const themePresets = [
```
I can't see the rest of the file, but to fully implement your suggestion you should:
1. **Store the preset by `id` instead of localized name**
- Find where the selected preset is stored, e.g. something like:
```ts
const selectedThemePreset = ref(getStoredColor("themePreset", "优雅紫"));
```
- Change the stored value to the preset `id`:
```ts
const selectedThemePreset = ref(getStoredColor("themePreset", "purple-default"));
```
- Ensure any calls to `setItem`/`storeColor` for the preset write the `id`, not `name`.
2. **Look up presets by `id` (with backward compatibility)**
- Add a helper after the `themePresets` array:
```ts
const getThemePresetByKey = (key: string | null | undefined) => {
if (!key) return themePresets[0];
// preferred: lookup by stable id
let preset = themePresets.find(p => p.id === key);
if (!preset) {
// backward compatibility: previous versions may have stored localized name
preset = themePresets.find(p => p.name === key || p.nameEn === key);
}
return preset ?? themePresets[0];
};
```
3. **Update `applyThemePreset` to use `id`**
- If it currently looks like:
```ts
const applyThemePreset = (presetName: string) => {
const preset = themePresets.find(p => p.name === presetName);
// ...
selectedThemePreset.value = presetName;
storeColor("themePreset", presetName);
};
```
- Change it to:
```ts
const applyThemePreset = (presetId: string) => {
const preset = getThemePresetByKey(presetId);
// ...
selectedThemePreset.value = preset.id;
storeColor("themePreset", preset.id);
};
```
4. **Update `selectedThemePreset` computed/use-sites**
- Wherever the selected preset object is derived, use `getThemePresetByKey(selectedThemePreset.value)` so it works with both the new `id` storage and any legacy stored names.
- When rendering in the template (dropdowns, labels, etc.), use:
```vue
{{ preset.name }} <!-- or i18n key derived from preset.id -->
```
instead of comparing or binding against `preset.name` for identification; use `preset.id` for v-model / value bindings.
These adjustments ensure the selected preset is always keyed by a stable `id` and not by a localized label, while remaining compatible with previously stored values that used the translated name.
</issue_to_address>
### Comment 3
<location path="dashboard/src/views/Settings.vue" line_range="764-773" />
<code_context>
+ }
};
const restartAstrBot = async () => {
- try {
- await restartAstrBotRuntime(wfr.value);
- } catch (error) {
- console.error(error);
+ try {
+ await restartAstrBotRuntime(wfr.value);
+ } catch (error: unknown) {
+ if (error instanceof AxiosError) {
+ showToast(
+ error?.response?.data?.message || tm("apiKey.messages.restartFailed"),
+ "error",
+ );
+ } else {
+ console.error(
+ "An unexpected error occurred while restarting AstrBot:",
+ error,
+ );
+ showToast(tm("apiKey.messages.restartFailed"), "error");
}
-}
</code_context>
<issue_to_address>
**suggestion:** Reusing `apiKey` i18n messages for restart errors couples unrelated domains.
Using `tm("apiKey.messages.restartFailed")` here ties restart error handling to the API key i18n namespace. This creates an unnecessary dependency and risks breakage if the API key messages are renamed or moved. Prefer defining a restart-specific error key under a more appropriate namespace (e.g. `system` or `settings`) and referencing that instead.
Suggested implementation:
```
try {
await restartAstrBotRuntime(wfr.value);
} catch (error: unknown) {
if (error instanceof AxiosError) {
showToast(
error?.response?.data?.message || tm("settings.messages.restartFailed"),
"error",
);
} else {
console.error(
"An unexpected error occurred while restarting AstrBot:",
error,
);
showToast(tm("settings.messages.restartFailed"), "error");
}
}
```
1. Define a new i18n key `settings.messages.restartFailed` (and translations) in your i18n resources, under the appropriate namespace/file for settings/system-level messages.
2. Optionally, if there are any other restart-related toasts in the codebase, update them to use the same `settings.messages.restartFailed` key for consistency.
</issue_to_address>
### Comment 4
<location path="dashboard/src/utils/request.ts" line_range="26" />
<code_context>
+ return strippedPath || "/";
+}
+
+function baseEndsWithApi(baseUrl: string): boolean {
+ if (!baseUrl) {
+ return false;
</code_context>
<issue_to_address>
**issue (complexity):** Consider introducing a single canonical URL builder that encapsulates `/api` handling and base URL normalization so the various helpers and interceptor logic become simpler and more predictable.
You can keep all existing behavior but reduce the mental overhead by collapsing the URL/path helpers into a single canonical resolver and pushing the `/api` rule into a parameter.
Right now, these interact in non‑obvious ways:
- `baseEndsWithApi` → `normalizePathForBase` → `stripLeadingApiPrefix`
- `normalizeBaseUrl` → `normalizeConfiguredApiBaseUrl` → `getApiBaseUrl` / `setApiBaseUrl` / `service.defaults.baseURL`
- `resolveApiUrl` vs interceptor path handling vs `resolveWebSocketUrl`
A small restructuring keeps behavior but simplifies the surface area.
### 1. Canonical URL resolver
Introduce one helper that encodes the `/api` behavior and “absolute URL” handling in one place:
```ts
function buildUrl(
baseUrl: string | null | undefined,
path: string,
options: { stripApiIfBaseEndsWithApi?: boolean } = {},
): string {
const base = normalizeBaseUrl(baseUrl);
if (!path) return "/";
if (isAbsoluteUrl(path)) return path;
let normalizedPath = ensureLeadingSlash(path);
if (options.stripApiIfBaseEndsWithApi && base && base.replace(/\/+$/, "").endsWith("/api")) {
normalizedPath = stripLeadingApiPrefix(normalizedPath);
}
if (!base) return normalizedPath;
return `${stripTrailingSlashes(base)}${normalizedPath}`;
}
```
Then:
```ts
export function resolveApiUrl(
path: string,
baseUrl: string | null | undefined = getApiBaseUrl(),
): string {
return buildUrl(baseUrl, path, { stripApiIfBaseEndsWithApi: true });
}
```
And in the interceptor:
```ts
service.interceptors.request.use((config: InternalAxiosRequestConfig) => {
const effectiveBase = config.baseURL ?? service.defaults.baseURL;
if (typeof config.url === "string") {
config.url = buildUrl(effectiveBase, config.url, { stripApiIfBaseEndsWithApi: true });
}
// headers setup unchanged...
return config;
});
```
This allows you to:
- Remove `baseEndsWithApi`, `normalizePathForBase`, and `joinBaseAndPath`.
- Keep `/api` stripping documented and localized to a single helper.
### 2. Make base URL normalization single-source
`normalizeConfiguredApiBaseUrl` currently both normalizes and conditionally prepends `https://`, while `service.defaults.baseURL` is initialized with `normalizeBaseUrl(import.meta.env.VITE_API_BASE)`.
To avoid the “has this already been through normalizeConfiguredApiBaseUrl?” concern, initialize `service.defaults.baseURL` through the same function you expose:
```ts
const initialBaseUrl = normalizeConfiguredApiBaseUrl(import.meta.env.VITE_API_BASE);
const service = axios.create({
baseURL: initialBaseUrl,
timeout: 10000,
});
```
`getApiBaseUrl` and `setApiBaseUrl` then become straightforward and always operate on the same normalization:
```ts
export function getApiBaseUrl(): string {
return normalizeBaseUrl(service.defaults.baseURL);
}
export function setApiBaseUrl(baseUrl: string | null | undefined): string {
const normalizedBaseUrl = normalizeConfiguredApiBaseUrl(baseUrl);
service.defaults.baseURL = normalizedBaseUrl;
return normalizedBaseUrl;
}
```
This reduces branching about “raw” vs “configured” base URLs and makes the flow from env → axios → helpers easier to follow while preserving all behaviors.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
dosubot
bot
added
the
area:webui
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the theme management system by centralizing logic in the Pinia store, adding support for system theme synchronization and color presets. It also introduces a new centralized API request utility and significantly updates the settings page with a card-based layout and API key management. Review feedback highlights a potential issue with URL normalization for local development, suggests refactoring the large settings component for better maintainability, and identifies opportunities to improve performance and consistency by reusing store instances in Vue components.
fix: unified string about light and dark in i18n fix: type lint in customizer
kyangconn
force-pushed
the
feat/auto-switch-button
branch
from
1dde501 to
e5b2e2d
Compare
kyangconn
changed the title
Contributor
Author
|
/gemini review |
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the theme management system by centralizing state in the customizer store and introducing support for system theme synchronization and custom color presets. Numerous components were updated to use the new store-based theme logic, and the settings page was redesigned for improved clarity. Feedback focuses on using standard Vuetify 3 APIs for theme switching, implementing listeners for real-time system theme changes, and ensuring that custom color updates correctly trigger reactivity.
Comment on lines
+36
to
+40
| if (typeof (vuetify.theme as any).change === "function") { | ||
| (vuetify.theme as any).change(payload); | ||
| } else if (vuetify.theme?.global) { | ||
| vuetify.theme.global.name.value = payload; | ||
| } |
Contributor
There was a problem hiding this comment.
The (vuetify.theme as any).change method is not a standard part of the Vuetify 3 API. In Vuetify 3, the recommended way to switch the global theme is by updating vuetify.theme.global.name.value. This non-standard check adds unnecessary complexity and potential runtime errors if the change method is not defined or behaves unexpectedly.
if (vuetify.theme?.global) {
vuetify.theme.global.name.value = payload;
}
Comment on lines
+141
to
+154
| const applyThemeColors = (primary: string, secondary: string) => { | ||
| const themes = resolveThemes(); | ||
| if (!themes) return; | ||
| [LIGHT_THEME_NAME, DARK_THEME_NAME].forEach((name) => { | ||
| const themeDef = themes[name]; | ||
| if (!themeDef?.colors) return; | ||
| if (primary) themeDef.colors.primary = primary; | ||
| if (secondary) themeDef.colors.secondary = secondary; | ||
| if (primary && themeDef.colors.darkprimary) | ||
| themeDef.colors.darkprimary = primary; | ||
| if (secondary && themeDef.colors.darksecondary) | ||
| themeDef.colors.darksecondary = secondary; | ||
| }); | ||
| }; |
Contributor
There was a problem hiding this comment.
Directly modifying the themeDef.colors object properties might not reliably trigger reactivity in all Vuetify components if the themes object is not deeply reactive. A more robust approach in Vuetify 3 is to replace the entire colors object or use the theme.themes.value[name].colors = { ... } pattern to ensure the change is propagated correctly. Additionally, darkprimary and darksecondary are not standard Vuetify color keys; ensure these are explicitly defined in your theme configuration if they are intended for use.
const applyThemeColors = (primary: string, secondary: string) => {
const themes = resolveThemes();
if (!themes) return;
[LIGHT_THEME_NAME, DARK_THEME_NAME].forEach((name) => {
const themeDef = themes[name];
if (!themeDef?.colors) return;
const newColors = { ...themeDef.colors };
if (primary) {
newColors.primary = primary;
if (newColors.darkprimary !== undefined) newColors.darkprimary = primary;
}
if (secondary) {
newColors.secondary = secondary;
if (newColors.darksecondary !== undefined) newColors.darksecondary = secondary;
}
themeDef.colors = newColors;
});
};
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
feat(dashboard): enhance types and theme management in Settings.vue
rfc(theme): refactor themetypes and presets to types/theme.ts
rfc(dashboard): refactor Setting page, move theme card to single template
fix: move all light and dark mode management into customizer
fix: unified string about light and dark in i18n
fix: type lint in customizer
在Settings.vue中添加了一组按钮和一组预设颜色，按钮用于显式切换深浅色模式+选择根据系统自动切换。预设颜色用于更改主题色。
统一了各处的获取主题的方式，现在可以通过
useCustomizer().isDarkTheme获取是否为深色主题，通过useCustomizer().currentTheme获取主题值。Modifications / 改动点
dashboard/src/views/Settings.vue: 把dev中的Settings页面merge进来，以便更好地实现主题管理，深浅色管理。dashboard/src/stores/customizer.ts: 往里面加入了用于切换深浅色的函数，以及更好地获取当前主题的getter。i18n/*/chat.jsoni18n/*/settings.jsoni18n/*/auth.json: 统一深浅色相关的字符串键值。其他有用isDark变量的页面：采用customizer中的新的主题管理。
dashboard/src/types/api.ts: 用于Settings.vue中的api相关类型增强。dashboard/src/types/themes.ts: 把原本ThemeType搬到这里面，并集成了新的颜色预设变量。dashboard/src/theme/constant.ts: 用于统一默认深浅色的变量值。Screenshots or Test Results / 运行截图或测试结果
Checklist / 检查清单
😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
/ 如果 PR 中有新加入的功能，已经通过 Issue / 邮件等方式和作者讨论过。
👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
/ 我的更改经过了良好的测试，并已在上方提供了“验证步骤”和“运行截图”。
🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in
requirements.txtandpyproject.toml./ 我确保没有引入新依赖库，或者引入了新依赖库的同时将其添加到
requirements.txt和pyproject.toml文件相应位置。😮 My changes do not introduce malicious code.
/ 我的更改没有引入恶意代码。
Summary by Sourcery
Add centralized theme management with light/dark/auto modes, theme presets, and improved API key handling in the Settings page while wiring all consumers to the new customizer store and typed API utilities.
New Features:
Bug Fixes:
Enhancements: