Fix/enable edit menu server web #3387
Conversation
WalkthroughThe pull request introduces significant modifications to the Electron application's menu handling and window management. Key changes include the restructuring of the application menu to ensure consistency across different window types, enhancements to event handling for language and theme changes, and refined error handling in server operations. Additionally, updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
apps/server-web/src/main/main.tsOops! Something went wrong! :( ESLint: 8.46.0 ESLint couldn't find the config "erb" to extend from. Please check that the name of the config is correct. The config "erb" was referenced from the config file in "/apps/server-web/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
apps/server-web/src/renderer/components/Updater.tsx (2)
Line range hint
70-79: Consider extracting update settings logicThe
onSelectPeriodfunction is calling two similar prop methods with identical parameters. Consider extracting this pattern to reduce duplication and improve maintainability.const onSelectPeriod = (value: string) => { - props.changeAutoUpdate({ - autoUpdate: props.data.autoUpdate, - updateCheckPeriod: value, - }); - props.saveSettingUpdate({ - autoUpdate: props.data.autoUpdate, - updateCheckPeriod: value, - }); + const settings = { + autoUpdate: props.data.autoUpdate, + updateCheckPeriod: value, + }; + props.changeAutoUpdate(settings); + props.saveSettingUpdate(settings); };
Line range hint
1-234: Consider component architecture improvementsA few architectural suggestions to improve the component:
- The component handles multiple responsibilities (settings, updates, logs). Consider splitting it into smaller, focused components.
- The update logs state could be lifted to a parent component or managed through a state management solution to persist across renders.
- The toast state management could be extracted into a custom hook for reusability.
Would you like me to provide a detailed refactoring proposal for any of these suggestions?
apps/web/app/services/server/requests/desktop-source.ts (1)
Line range hint
5-15: Improve error handling and type safetyWhile the type update is correct, there are several areas for improvement:
- Error logging should be more descriptive
- Consider validating the runtime config structure
- Document which fields are required vs optional
Consider applying these improvements:
export function getDesktopConfig(): Partial<IServerRuntimeConfig> { try { const { serverRuntimeConfig } = getConfig(); + // Validate required fields + if (!serverRuntimeConfig?.GAUZY_API_SERVER_URL) { + throw new Error('Missing required GAUZY_API_SERVER_URL in server config'); + } return serverRuntimeConfig; } catch (error) { - console.log('skip get server runtime config'); + console.error('Failed to load server runtime config:', error instanceof Error ? error.message : 'Unknown error'); return { GAUZY_API_SERVER_URL, NEXT_PUBLIC_GAUZY_API_SERVER_URL: GAUZY_API_BASE_SERVER_URL.value }; } }apps/server-web/src/main/menu.ts (1)
Line range hint
19-58: LGTM! Consider extracting menu templatesThe implementation correctly handles platform-specific accelerators and follows standard Electron menu practices. However, consider extracting the menu templates into separate constants to improve maintainability.
+const EDIT_MENU_TEMPLATE = [ + { label: 'MENU_APP.APP_SUBMENU.APP_UNDO', accelerator: "CmdOrCtrl+Z", role: "undo" }, + { label: "MENU_APP.APP_SUBMENU.APP_REDO", accelerator: "Shift+CmdOrCtrl+Z", role: "redo" }, + { type: "separator" }, + { label: "MENU_APP.APP_SUBMENU.APP_CUT", accelerator: "CmdOrCtrl+X", role: "cut" }, + { label: "MENU_APP.APP_SUBMENU.APP_COPY", accelerator: "CmdOrCtrl+C", role: "copy" }, + { label: "MENU_APP.APP_SUBMENU.APP_PASTE", accelerator: "CmdOrCtrl+V", role: "paste" }, + { label: "MENU_APP.APP_SUBMENU.APP_SELECT_ALL", accelerator: "CmdOrCtrl+A", role: "selectAll" } +];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
apps/server-web/src/main/main.ts(3 hunks)apps/server-web/src/main/menu.ts(3 hunks)apps/server-web/src/renderer/components/Select.tsx(1 hunks)apps/server-web/src/renderer/components/Updater.tsx(2 hunks)apps/server-web/src/renderer/pages/setup/Landing.tsx(2 hunks)apps/web/app/interfaces/IRuntimeServerConfig.ts(1 hunks)apps/web/app/services/server/requests/desktop-source.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/server-web/src/renderer/components/Select.tsx
- apps/web/app/interfaces/IRuntimeServerConfig.ts
🔇 Additional comments (7)
apps/server-web/src/renderer/components/Updater.tsx (1)
Line range hint 70-79: LGTM! Spelling correction from onSelectPeriode to onSelectPeriod
The spelling correction improves code clarity while maintaining the same functionality.
Also applies to: 155-155
apps/server-web/src/renderer/pages/setup/Landing.tsx (2)
6-6: LGTM: Clean import statement
The ThemeToggler import follows the project's import conventions.
29-39: Verify integration with Electron menu system
Since this PR involves menu handling, ensure that theme changes through the ThemeToggler component properly sync with the application menu state.
apps/server-web/src/main/menu.ts (1)
Line range hint 60-127: Consider conditionally showing the dev menu
The dev menu provides access to development windows and settings. Consider showing this menu only in development mode to prevent accidental access in production.
apps/server-web/src/main/main.ts (3)
200-200: LGTM! Consistent menu handling
The change correctly uses the new initialMenu method for the setup window, ensuring a consistent menu structure.
269-270: LGTM! Appropriate menu context handling
The implementation correctly switches between initial and default menus based on the application's setup state.
379-379: LGTM! Consistent theme handling
The change ensures that the setup window receives theme updates, maintaining visual consistency across all windows.
| @@ -1,8 +1,8 @@ | |||
| import getConfig from 'next/config'; | |||
| import { ServerRuntimeConfig } from '@app/interfaces/IRuntimeServerConfig'; | |||
| import { IServerRuntimeConfig } from '@app/interfaces/IRuntimeServerConfig'; | |||
There was a problem hiding this comment.
💡 Codebase verification
Interface renaming needs to be completed across the codebase
The verification reveals inconsistent interface naming:
- The interface has been renamed to
IServerRuntimeConfiginIRuntimeServerConfig.ts - However,
getServerRuntimeConfig()function inenv.tsand its usage inconstants.tsstill use the old naming convention - This creates a mix of old and new naming patterns that should be standardized
Locations needing updates:
apps/web/app/env.ts: Function name should be updated togetIServerRuntimeConfig()apps/web/app/constants.ts: Variable name should be updated to match the new interface naming
🔗 Analysis chain
Verify consistent interface renaming across the codebase
The interface renaming from ServerRuntimeConfig to IServerRuntimeConfig follows TypeScript naming conventions. Let's ensure this change is consistent throughout the codebase.
Let me analyze the results further by checking the implementation details of these files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining occurrences of the old interface name
# and verify the new interface is used consistently
# Search for any remaining instances of the old interface name
echo "Checking for remaining instances of ServerRuntimeConfig..."
rg "ServerRuntimeConfig" --type ts
# Verify the new interface usage
echo "Verifying IServerRuntimeConfig usage..."
rg "IServerRuntimeConfig" --type ts
Length of output: 1213
Script:
#!/bin/bash
# Check the implementation of getServerRuntimeConfig and its usage
echo "Checking env.ts implementation..."
cat apps/web/app/env.ts
echo -e "\nChecking constants.ts implementation..."
cat apps/web/app/constants.ts
echo -e "\nChecking IRuntimeServerConfig interface..."
cat apps/web/app/interfaces/IRuntimeServerConfig.ts
Length of output: 12996
| <div className="flex w-full mb-6 ml-10"> | ||
| <div className="flex flex-col w-6/12"> | ||
| <div> | ||
| <LanguageSelector lang={defaultLang} /> | ||
| </div> | ||
| </div> | ||
| <div className="flex w-6/12 flex-row-reverse mr-10"> | ||
| <div className="flex flex-col w-2/8"> | ||
| <ThemeToggler /> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve layout structure and accessibility
The current implementation has several issues that should be addressed:
- The width class
w-2/8is invalid in Tailwind CSS - The nested structure could be simplified
- Missing accessibility attributes for language and theme controls
Consider applying these improvements:
- <div className="flex w-full mb-6 ml-10">
- <div className="flex flex-col w-6/12">
- <div>
- <LanguageSelector lang={defaultLang} />
- </div>
- </div>
- <div className="flex w-6/12 flex-row-reverse mr-10">
- <div className="flex flex-col w-2/8">
- <ThemeToggler />
- </div>
- </div>
+ <nav className="flex justify-between items-center w-full px-10 mb-6" aria-label="Site settings">
+ <div className="flex-shrink-0" role="region" aria-label="Language selection">
+ <LanguageSelector lang={defaultLang} />
+ </div>
+ <div className="flex-shrink-0" role="region" aria-label="Theme selection">
+ <ThemeToggler />
+ </div>
+ </nav>Changes made:
- Replaced invalid
w-2/8with a more flexible layout usingflex-shrink-0 - Simplified the nested structure
- Added semantic
navelement and ARIA labels - Used consistent padding with
px-10instead ofml-10andmr-10
📝 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.
| <div className="flex w-full mb-6 ml-10"> | |
| <div className="flex flex-col w-6/12"> | |
| <div> | |
| <LanguageSelector lang={defaultLang} /> | |
| </div> | |
| </div> | |
| <div className="flex w-6/12 flex-row-reverse mr-10"> | |
| <div className="flex flex-col w-2/8"> | |
| <ThemeToggler /> | |
| </div> | |
| </div> | |
| <nav className="flex justify-between items-center w-full px-10 mb-6" aria-label="Site settings"> | |
| <div className="flex-shrink-0" role="region" aria-label="Language selection"> | |
| <LanguageSelector lang={defaultLang} /> | |
| </div> | |
| <div className="flex-shrink-0" role="region" aria-label="Theme selection"> | |
| <ThemeToggler /> | |
| </div> | |
| </nav> |
| buildInitialTemplate(menuItems: any, i18nextMainBackend: typeof i18n) { | ||
| return Menu.buildFromTemplate(this.translateAppMenu(i18nextMainBackend, menuItems)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate method implementation
buildInitialTemplate appears to be identical to buildDefaultTemplate. Consider removing this duplication and reusing buildDefaultTemplate instead.
-buildInitialTemplate(menuItems: any, i18nextMainBackend: typeof i18n) {
- return Menu.buildFromTemplate(this.translateAppMenu(i18nextMainBackend, menuItems));
-}Committable suggestion skipped: line range outside the PR's diff.
|
@syns2191 I am merging this PR, but please go over suggestions above by Coderabbit and fix those! Somehow you keep missing them... |
Description
Please include a summary of the changes and the related issue.
Type of Change
Checklist
Previous screenshots
Please add here videos or images of previous status
Current screenshots
Please add here videos or images of previous status
Summary by CodeRabbit
Release Notes
New Features
ThemeTogglercomponent to the landing page for enhanced user interface customization.Bug Fixes
Style
Selectcomponent for improved readability.Refactor
onSelectPeriodetoonSelectPeriod.