feat: reorganize settings system configuration#8777
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new system config auto-save logic relies on timeouts (
systemConfigAutoSaveTimer) but does not clear them on component unmount; consider adding anonUnmountedhook to clear any pending timers to avoid saves firing after navigation. - The settings section selection is only read from
window.location.hashon mount and is never written back or updated on hash changes; if deep-linking and browser back/forward navigation between sections is desirable, consider syncingactiveSettingsSectionto the URL hash and listening forhashchangeevents.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new system config auto-save logic relies on timeouts (`systemConfigAutoSaveTimer`) but does not clear them on component unmount; consider adding an `onUnmounted` hook to clear any pending timers to avoid saves firing after navigation.
- The settings section selection is only read from `window.location.hash` on mount and is never written back or updated on hash changes; if deep-linking and browser back/forward navigation between sections is desirable, consider syncing `activeSettingsSection` to the URL hash and listening for `hashchange` events.
## Individual Comments
### Comment 1
<location path="dashboard/src/views/Settings.vue" line_range="498-503" />
<code_context>
+const configSave2faSaving = ref(false);
+const configSave2faRotationHint = ref('');
+const configSavePendingData = ref(null);
+const systemConfigAutoSaveTimer = ref(null);
+const activeSettingsSection = ref('general');
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Auto-save timer should be cleared on component unmount to avoid stray saves and leaks
The `systemConfigAutoSaveTimer` timeout is never cleared on unmount. If the user navigates away while a save is scheduled, the callback can still run against a destroyed view and the timer ref will persist. Please add an `onBeforeUnmount`/`onUnmounted` hook to `clearTimeout(systemConfigAutoSaveTimer.value)` and reset the ref to `null`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const systemConfigAutoSaveTimer = ref(null); | ||
| const activeSettingsSection = ref('general'); | ||
|
|
||
| const apiKeyExpiryOptions = computed(() => [ | ||
| { title: tm('apiKey.expiryOptions.day1'), value: 1 }, | ||
| { title: tm('apiKey.expiryOptions.day7'), value: 7 }, |
There was a problem hiding this comment.
issue (bug_risk): Auto-save timer should be cleared on component unmount to avoid stray saves and leaks
The systemConfigAutoSaveTimer timeout is never cleared on unmount. If the user navigates away while a save is scheduled, the callback can still run against a destroyed view and the timer ref will persist. Please add an onBeforeUnmount/onUnmounted hook to clearTimeout(systemConfigAutoSaveTimer.value) and reset the ref to null.
There was a problem hiding this comment.
Code Review
This pull request removes the legacy database migration assistant and restructures the Settings page into a tabbed layout, integrating system configuration options directly with auto-save and 2FA support. Feedback on these changes highlights a potential UI soft-lock in Settings.vue when cancelling the 2FA dialog, as only TOTP settings are reverted instead of the entire configuration. Additionally, it is recommended to clear the auto-save timer in an onUnmounted hook to prevent background API calls, and to reduce the excessively large text-h3 typography class used for the title in ConfirmDialog.vue.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const handleConfigSave2faCancel = () => { | ||
| if (systemConfigLastSavedSnapshot.value && systemConfigData.value?.dashboard?.totp) { | ||
| try { | ||
| const savedConfig = JSON.parse(systemConfigLastSavedSnapshot.value); | ||
| const savedTotp = savedConfig?.dashboard?.totp; | ||
| if (savedTotp) { | ||
| systemConfigData.value.dashboard.totp.enable = savedTotp.enable; | ||
| systemConfigData.value.dashboard.totp.secret = savedTotp.secret; | ||
| systemConfigData.value.dashboard.totp.recovery_code_hash = savedTotp.recovery_code_hash; | ||
| } | ||
| } catch (_) { | ||
| // Ignore invalid snapshots. | ||
| } | ||
| } | ||
| configSavePendingData.value = null; | ||
| configSave2faError.value = ''; | ||
| configSave2faDialogVisible.value = false; | ||
| }; |
There was a problem hiding this comment.
When the user cancels the 2FA dialog, only the TOTP-related settings are reverted. If other settings (like timezone or proxy) were changed and triggered the 2FA prompt, they will remain modified in systemConfigData.value. This keeps systemConfigHasChanges.value as true, causing the auto-save timer or focusout events to repeatedly trigger the 2FA dialog, soft-locking the UI. Reverting the entire systemConfigData.value to the last saved snapshot on cancellation solves this robustly.
const handleConfigSave2faCancel = () => {
if (systemConfigLastSavedSnapshot.value) {
try {
systemConfigData.value = JSON.parse(systemConfigLastSavedSnapshot.value);
} catch (_) {
// Ignore invalid snapshots.
}
}
configSavePendingData.value = null;
configSave2faError.value = '';
configSave2faDialogVisible.value = false;
};
| <v-dialog v-model="isOpen" max-width="400"> | ||
| <v-card> | ||
| <v-card-title class="text-h6">{{ title }}</v-card-title> | ||
| <v-card-title class="text-h3 pl-6 pt-4">{{ title }}</v-card-title> |
There was a problem hiding this comment.
The typography class text-h3 is extremely large in Vuetify 3 (3rem / 48px). For a standard confirmation dialog title, this will likely cause awkward wrapping and layout issues, especially on mobile devices. Consider using a more standard size like text-h5 or text-h6.
<v-card-title class="text-h5 pl-6 pt-4">{{ title }}</v-card-title>
| </template> | ||
|
|
||
| <script setup> | ||
| import { computed, onMounted, ref, watch } from 'vue'; |
| onMounted(async () => { | ||
| await Promise.all([loadApiKeys(), loadSystemConfig()]); | ||
| const hash = window.location.hash; | ||
| if (hash.includes('settings-appearance')) { | ||
| activeSettingsSection.value = 'appearance'; | ||
| } else if (hash.includes('settings-network')) { | ||
| activeSettingsSection.value = 'network'; | ||
| } else if (hash.includes('settings-security')) { | ||
| activeSettingsSection.value = 'security'; | ||
| } else if (hash.includes('settings-maintenance')) { | ||
| activeSettingsSection.value = 'maintenance'; | ||
| } else if (hash.includes('settings-openapi')) { | ||
| activeSettingsSection.value = 'openapi'; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Add an onUnmounted hook to clear the systemConfigAutoSaveTimer when the component is unmounted. This prevents background timeouts from firing and attempting to save configuration after the user has navigated away from the Settings page.
onMounted(async () => {
await Promise.all([loadApiKeys(), loadSystemConfig()]);
const hash = window.location.hash;
if (hash.includes('settings-appearance')) {
activeSettingsSection.value = 'appearance';
} else if (hash.includes('settings-network')) {
activeSettingsSection.value = 'network';
} else if (hash.includes('settings-security')) {
activeSettingsSection.value = 'security';
} else if (hash.includes('settings-maintenance')) {
activeSettingsSection.value = 'maintenance';
} else if (hash.includes('settings-openapi')) {
activeSettingsSection.value = 'openapi';
}
});
onUnmounted(() => {
if (systemConfigAutoSaveTimer.value) {
clearTimeout(systemConfigAutoSaveTimer.value);
}
});
Summary
Tests
Note: running
ruff check .without exclusions is currently blocked by the untracked localpackages/directory.Summary by Sourcery
Reorganize the dashboard settings experience with grouped navigation and integrated system configuration, while removing the legacy migration flow and related API surface.
New Features:
Enhancements:
Chores: