Add Account Settings page #8672
Conversation
- Changed the "Change Password" link to point to "/account/password". - Added new links for "Auth Token" and "Account Settings" in the navbar. - Introduced secured routes for account settings in the router.
📝 Walkthrough""" WalkthroughThis change introduces a new user account settings page consolidating profile, password, auth token, and theme preference management into a unified interface. It removes the old password and auth token views, updates navigation and routing to reflect the new structure, and moves relevant logic from the navbar to the dedicated account settings components. Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…ount-settings-page
- Updated the navbar link for managing organizations to point to the overview page. - Replaced `AccountSettingsTitle` with `SettingsTitle` in multiple views for consistency. - Introduced `SettingsCard` component for better organization of settings-related UI. - Added new views for organization notifications and overview, enhancing the organization management experience. - Cleaned up unused code and improved the structure of organization-related components.
- Updated the CustomPlanUpgradeCard to include a MailOutlined icon and improved layout with responsive columns. - Modified OrganizationNotificationsView to dispatch the updated organization state after saving notification settings. - Added organization name editing functionality in OrganizationOverviewView with validation and state management. - Refactored organization overview stats display for better readability and structure. - Improved imports in organization-related components for consistency.
- Updated OrganizationController to include the current user's identity in the organization JSON response. - Modified organization cards to export components for better reusability. - Improved the upgrade plan modal by wrapping components in a ConfigProvider for consistent theming and layout adjustments. - Refactored the modal body to use responsive columns for displaying upgrade options. These changes aim to improve the user experience and maintainability of the organization management interface.
…ount-settings-page
Co-authored-by: Philipp Otto <philippotto@users.noreply.github.com>
…sos into orga-settings-page
|
@philippotto Please only review the account settings stuff. Ignore the orga settings stuff since you already reviewed that. Regarding, "change email" and "passkeys". I think we should merge this PR and then I will integrate charlie's and robert's changes into the account settings. |
|
some preliminary feedback: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/javascripts/admin/account/account_password_view.tsx (1)
25-39: Error handling implementation looks good.The password change API call now includes proper error handling with user feedback, which addresses the previous review concern.
🧹 Nitpick comments (2)
frontend/javascripts/admin/account/account_password_view.tsx (2)
41-54: Review password validation logic for robustness.The
checkPasswordsAreMatchingfunction handles cross-field validation well, but consider edge cases:
- The
form.getFieldError()call might need error handling if the field doesn't exist- The
form.validateFields()call could benefit from error handlingConsider adding error handling:
function checkPasswordsAreMatching(value: string, otherPasswordFieldKey: string[]) { const otherFieldValue = form.getFieldValue(otherPasswordFieldKey); if (value && otherFieldValue) { if (value !== otherFieldValue) { return Promise.reject(new Error(messages["auth.registration_password_mismatch"])); - } else if (form.getFieldError(otherPasswordFieldKey).length > 0) { + } else if (form.getFieldError(otherPasswordFieldKey)?.length > 0) { // If the other password field still has errors, revalidate it. - form.validateFields([otherPasswordFieldKey]); + form.validateFields([otherPasswordFieldKey]).catch(() => { + // Validation errors are handled by the form itself + }); } } return Promise.resolve(); }
21-202: Consider component decomposition for better maintainability.The component handles multiple concerns (password form, validation, passkeys display). Consider splitting into smaller, focused components for better maintainability and testability.
Potential structure:
PasswordFormcomponent for the form logicPasskeysSectioncomponent for the passkeys placeholder- Keep
AccountPasswordViewas the orchestrating component
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/admin/account/account_password_view.tsx(1 hunks)
🧰 Additional context used
🪛 GitHub Check: frontend-tests
frontend/javascripts/admin/account/account_password_view.tsx
[failure] 196-196:
Property 'action' does not exist on type '{ title: string; value: string; }'.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
🔇 Additional comments (1)
frontend/javascripts/admin/account/account_password_view.tsx (1)
81-81: To locate the actualchangePasswordhandler (and how it consumesformValues), let’s search the entire repo without file-type filters:#!/bin/bash # Search for any definition or invocation of changePassword and usage of formValues rg -n "changePassword" -A 5 -B 5 rg -n "formValues" -A 5 -B 5
Co-authored-by: Philipp Otto <philippotto@users.noreply.github.com>
…ount-settings-page
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
unreleased_changes/8672.md (1)
1-2: Avoid duplication & minor grammar tweak in changelog entryBecause the header already says “Added”, repeating the word in the bullet is redundant. Also, insert the serial/Oxford comma after “passwords” for consistency with the project’s existing changelog style.
-### Added -- Added a new Account Settings page for managing user preferences, passwords and authentication methods. +### Added +- Account Settings page for managing user preferences, passwords, and authentication methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
unreleased_changes/8672.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
unreleased_changes/8672.md
[duplication] ~1-~1: Possible typo: you repeated a word.
Context: ### Added - Added a new Account Settings page for managin...
(ENGLISH_WORD_REPEAT_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/javascripts/admin/account/account_password_view.tsx (2)
76-90: Consider enhancing password validation rules.The current validation only checks minimum length and matching passwords. Consider adding additional security rules for stronger passwords.
+const validatePasswordStrength = (value: string) => { + if (!value) return Promise.resolve(); + + const hasUpperCase = /[A-Z]/.test(value); + const hasLowerCase = /[a-z]/.test(value); + const hasNumbers = /\d/.test(value); + const hasSpecialChar = /[!@#$%^&*(),.?":{}|<>]/.test(value); + + if (!hasUpperCase || !hasLowerCase || !hasNumbers || !hasSpecialChar) { + return Promise.reject(new Error("Password must contain uppercase, lowercase, numbers, and special characters")); + } + + return Promise.resolve(); +}; { min: MIN_PASSWORD_LENGTH, message: messages["auth.registration_password_length"], }, +{ + validator: (_, value: string) => validatePasswordStrength(value), +},Also applies to: 105-118
150-150: Consider improving password masking display.The hardcoded asterisks could be more dynamic or semantic.
-"***********" +"••••••••••••"Or use a more semantic approach:
-"***********" +<span style={{ fontFamily: 'monospace', letterSpacing: '2px' }}>••••••••••••</span>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/javascripts/admin/account/account_auth_token_view.tsx(1 hunks)frontend/javascripts/admin/account/account_password_view.tsx(1 hunks)frontend/javascripts/admin/account/account_settings_view.tsx(1 hunks)frontend/javascripts/admin/organization/organization_view.tsx(1 hunks)frontend/javascripts/admin/rest_api.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/javascripts/admin/account/account_settings_view.tsx
- frontend/javascripts/admin/organization/organization_view.tsx
- frontend/javascripts/admin/account/account_auth_token_view.tsx
- frontend/javascripts/admin/rest_api.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: backend-tests
- GitHub Check: frontend-tests
- GitHub Check: build-smoketest-push
🔇 Additional comments (3)
frontend/javascripts/admin/account/account_password_view.tsx (3)
22-34: Good implementation of error handling.The password change logic now includes proper error handling with user feedback, addressing the previous review concern. The flow correctly handles success and failure scenarios.
36-49: Well-implemented password validation logic.The password matching validation function properly handles both password fields and revalidates when necessary. The logic correctly checks for matching passwords and clears errors on the other field when validation passes.
158-165: TypeScript issue resolved appropriately.The addition of
action: undefinedproperly addresses the previous TypeScript error while maintaining the placeholder structure for future passkey functionality.
Co-authored-by: Philipp Otto <philippotto@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
unreleased_changes/8679.md (1)
1-2: Clarify wording and fix possessive apostrophe in changelog entryThe current phrasing is slightly confusing (“organizations settings page”) and is missing the possessive apostrophe flagged by the linter. A clearer wording would be:
-### Changed -- Updated the UI for the organizations settings page. +### Changed +- Updated the UI for the organization’s settings page.This keeps the entry crisp while eliminating the grammatical issue.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
unreleased_changes/8679.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
unreleased_changes/8679.md
[uncategorized] ~2-~2: It seems likely that a singular genitive (’s) apostrophe is missing.
Context: ### Changed - Updated the UI for the organizations settings page.
(AI_HYDRA_LEO_APOSTROPHE_S_XS)
…ount-settings-page
…nossos into account-settings-page
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/admin/account/account_auth_token_view.tsx(1 hunks)frontend/javascripts/viewer/view/components/command_palette.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/admin/account/account_auth_token_view.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/javascripts/viewer/view/components/command_palette.tsx (2)
frontend/javascripts/theme.tsx (1)
getSystemColorTheme(67-74)frontend/javascripts/admin/rest_api.ts (1)
updateSelectedThemeOfUser(223-231)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
🔇 Additional comments (2)
frontend/javascripts/viewer/view/components/command_palette.tsx (2)
1-1: LGTM - Necessary imports for theme functionalityThe new imports correctly bring in the required dependencies for theme switching functionality.
Also applies to: 10-10, 15-16
217-217: LGTM - Proper integration of theme commandsThe theme entries are correctly integrated into the command palette following the established pattern used by other command generators.
| const getThemeEntries = () => { | ||
| if (activeUser == null) return []; | ||
| const commands: CommandWithoutId[] = []; | ||
|
|
||
| const themesWithNames = [ | ||
| ["auto", "System-default"], | ||
| ["light", "Light"], | ||
| ["dark", "Dark"], | ||
| ] as const; | ||
|
|
||
| for (let [theme, name] of themesWithNames) { | ||
| commands.push({ | ||
| name: `Switch to “${name}” color theme`, | ||
| command: async () => { | ||
| if (theme === "auto") theme = getSystemColorTheme(); | ||
|
|
||
| const newUser = await updateSelectedThemeOfUser(activeUser.id, theme); | ||
| Store.dispatch(setThemeAction(theme)); | ||
| Store.dispatch(setActiveUserAction(newUser)); | ||
| }, | ||
| color: commandEntryColor, | ||
| }); | ||
| } | ||
|
|
||
| return commands; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and improve code clarity
The theme switching functionality is well-implemented and follows the established patterns in the component. However, consider these improvements:
- Missing error handling: The async API call could fail, leaving users without feedback
- Variable reassignment: Reassigning the
themeparameter can be confusing - Hardcoded strings: Theme names could be extracted to constants
Apply this diff to improve error handling and code clarity:
const getThemeEntries = () => {
if (activeUser == null) return [];
const commands: CommandWithoutId[] = [];
const themesWithNames = [
["auto", "System-default"],
["light", "Light"],
["dark", "Dark"],
] as const;
for (let [theme, name] of themesWithNames) {
commands.push({
name: `Switch to "${name}" color theme`,
- command: async () => {
- if (theme === "auto") theme = getSystemColorTheme();
-
- const newUser = await updateSelectedThemeOfUser(activeUser.id, theme);
- Store.dispatch(setThemeAction(theme));
- Store.dispatch(setActiveUserAction(newUser));
- },
+ command: async () => {
+ try {
+ const actualTheme = theme === "auto" ? getSystemColorTheme() : theme;
+ const newUser = await updateSelectedThemeOfUser(activeUser.id, actualTheme);
+ Store.dispatch(setThemeAction(actualTheme));
+ Store.dispatch(setActiveUserAction(newUser));
+ } catch (error) {
+ console.error("Failed to update theme:", error);
+ // TODO: Show user-friendly error notification
+ }
+ },
color: commandEntryColor,
});
}
return commands;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const getThemeEntries = () => { | |
| if (activeUser == null) return []; | |
| const commands: CommandWithoutId[] = []; | |
| const themesWithNames = [ | |
| ["auto", "System-default"], | |
| ["light", "Light"], | |
| ["dark", "Dark"], | |
| ] as const; | |
| for (let [theme, name] of themesWithNames) { | |
| commands.push({ | |
| name: `Switch to “${name}” color theme`, | |
| command: async () => { | |
| if (theme === "auto") theme = getSystemColorTheme(); | |
| const newUser = await updateSelectedThemeOfUser(activeUser.id, theme); | |
| Store.dispatch(setThemeAction(theme)); | |
| Store.dispatch(setActiveUserAction(newUser)); | |
| }, | |
| color: commandEntryColor, | |
| }); | |
| } | |
| return commands; | |
| }; | |
| const getThemeEntries = () => { | |
| if (activeUser == null) return []; | |
| const commands: CommandWithoutId[] = []; | |
| const themesWithNames = [ | |
| ["auto", "System-default"], | |
| ["light", "Light"], | |
| ["dark", "Dark"], | |
| ] as const; | |
| for (let [theme, name] of themesWithNames) { | |
| commands.push({ | |
| name: `Switch to "${name}" color theme`, | |
| command: async () => { | |
| try { | |
| const actualTheme = theme === "auto" ? getSystemColorTheme() : theme; | |
| const newUser = await updateSelectedThemeOfUser(activeUser.id, actualTheme); | |
| Store.dispatch(setThemeAction(actualTheme)); | |
| Store.dispatch(setActiveUserAction(newUser)); | |
| } catch (error) { | |
| console.error("Failed to update theme:", error); | |
| // TODO: Show user-friendly error notification | |
| } | |
| }, | |
| color: commandEntryColor, | |
| }); | |
| } | |
| return commands; | |
| }; |
🤖 Prompt for AI Agents
In frontend/javascripts/viewer/view/components/command_palette.tsx around lines
169 to 194, add try-catch blocks around the async call to
updateSelectedThemeOfUser to handle potential errors and provide user feedback.
Avoid reassigning the theme variable by introducing a new variable for the
resolved theme value. Extract the theme names from the hardcoded array into
separate constants to improve clarity and maintainability.
This PR fixes a regression of PR #8672 that would always show a warning that your orga plan is using too much storage. For some reason, in a previous WK build most of this code was commented out and got commented in with PR #8672 causing the regression. <img width="1295" alt="Screenshot 2025-06-30 at 16 37 44" src="https://github.com/user-attachments/assets/d6516a5b-5798-4893-8871-aa9d86aea67e" /> ### Issues: - fixes https://scm.slack.com/archives/C5AKLAV0B/p1751293404276169 ------ (Please delete unneeded items, merge only when none are left open) - [ ] Added changelog entry (create a `$PR_NUMBER.md` file in `unreleased_changes` or use `./tools/create-changelog-entry.py`) - [ ] Added migration guide entry if applicable (edit the same file as for the changelog) - [ ] Updated [documentation](../blob/master/docs) if applicable - [ ] Adapted [wk-libs python client](https://github.com/scalableminds/webknossos-libs/tree/master/webknossos/webknossos/client) if relevant API parts change - [ ] Removed dev-only changes like prints and application.conf edits - [ ] Considered [common edge cases](../blob/master/.github/common_edge_cases.md) - [ ] Needs datastore update after deployment
This PR adds a new "Account Settings" page to host various other sub-pages for:
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
(Please delete unneeded items, merge only when none are left open)