Fix/server web default setting #3366
Conversation
|
|
WalkthroughThis pull request introduces various changes across multiple files, primarily focusing on enhancing application configuration management, translation consistency, and component structure. Key updates include the addition of a TypeScript script for modifying server and web application files, renaming translation keys to follow a new naming convention, and introducing new interfaces and constants for better type safety. Additionally, several components are modified to streamline language selection and improve the overall structure and error handling of the application. Changes
Possibly related PRs
Poem
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 (
|
syns2191
force-pushed
the
fix/server-web-default-setting
branch
from
2675b16
to
fc1616e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
🛑 Comments failed to post (28)
apps/web/app/api/desktop-server/route.ts (2)
4-6: 🛠️ Refactor suggestion
Add error handling and type safety
The endpoint implementation could benefit from proper error handling and type definitions.
Consider applying these improvements:
+import { NextRequest } from 'next/server'; +import { DesktopConfig } from '@app/types'; // Assuming this type exists -export async function GET() { +export async function GET(request: NextRequest) { + try { return NextResponse.json(await getDesktopConfig()); + } catch (error) { + console.error('Failed to fetch desktop config:', error); + return NextResponse.json( + { error: 'Failed to fetch desktop configuration' }, + { status: 500 } + ); + } }Committable suggestion skipped: line range outside the PR's diff.
4-6: 💡 Codebase verification
Desktop endpoint requires additional protection
The endpoint is currently unprotected and accessible to any client. While the application uses middleware for authentication and route protection, the
/api/desktop-serverroute is not included in the protected paths. Additionally, there's no verification of theIS_DESKTOP_APPenvironment variable for this endpoint.Key issues:
- The endpoint exposes server runtime configuration without any access restrictions
- No middleware protection for
/api/desktop-serverroute- Missing desktop-only validation check using
IS_DESKTOP_APP🔗 Analysis chain
Verify desktop-only access
Since this is a desktop-specific endpoint, we should verify that it's properly restricted.
Let's check how the desktop configuration is protected:
Let me check the middleware implementation and the desktop configuration to complete the verification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if IS_DESKTOP_APP is properly used for protection rg -A 5 "IS_DESKTOP_APP" # Look for any middleware that might be protecting this route fd --type f "middleware" apps/web/Length of output: 2512
Script:
#!/bin/bash # Check middleware implementation cat apps/web/middleware.ts # Check desktop config implementation rg -l "getDesktopConfig" cat $(rg -l "getDesktopConfig")Length of output: 4209
apps/web/app/services/server/requests/desktop-source.ts (4)
5-6: 🛠️ Refactor suggestion
⚠️ Potential issueImplement configuration validation and security filtering
Returning the entire
serverRuntimeConfigcould potentially expose sensitive information. Consider implementing a whitelist approach.Consider this safer approach:
- const { serverRuntimeConfig } = getConfig(); - return serverRuntimeConfig; + const { serverRuntimeConfig } = getConfig(); + // Whitelist only the necessary configuration properties + const allowedKeys = ['desktopApiUrl', 'desktopPort']; // adjust based on requirements + return Object.fromEntries( + Object.entries(serverRuntimeConfig) + .filter(([key]) => allowedKeys.includes(key)) + );📝 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 { serverRuntimeConfig } = getConfig(); // Whitelist only the necessary configuration properties const allowedKeys = ['desktopApiUrl', 'desktopPort']; // adjust based on requirements return Object.fromEntries( Object.entries(serverRuntimeConfig) .filter(([key]) => allowedKeys.includes(key)) );
4-10: 🛠️ Refactor suggestion
Enhance error handling and logging
The current error handling might hide configuration issues in production. Consider adding structured logging and proper error propagation.
Consider this improvement:
try { const { serverRuntimeConfig } = getConfig(); + if (!serverRuntimeConfig) { + throw new Error('Server runtime configuration is undefined'); + } return serverRuntimeConfig; } catch (error) { - console.log('skip get server runtime config'); + console.error('Failed to load desktop configuration:', error instanceof Error ? error.message : 'Unknown error'); + // Consider if throwing an error would be better than silent failure return {}; }📝 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.try { const { serverRuntimeConfig } = getConfig(); if (!serverRuntimeConfig) { throw new Error('Server runtime configuration is undefined'); } return serverRuntimeConfig; } catch (error) { console.error('Failed to load desktop configuration:', error instanceof Error ? error.message : 'Unknown error'); // Consider if throwing an error would be better than silent failure return {}; }
3-11: 🛠️ Refactor suggestion
Add TypeScript types and documentation
The function lacks type safety and documentation which could lead to runtime issues and maintenance difficulties.
Consider applying this improvement:
+interface DesktopConfig { + // Define expected configuration properties + [key: string]: unknown; +} + +/** + * Retrieves desktop configuration from Next.js server runtime config + * @returns {DesktopConfig} Desktop configuration object + * @throws {Error} When configuration is invalid + */ -export function getDesktopConfig() { +export function getDesktopConfig(): DesktopConfig {Committable suggestion skipped: line range outside the PR's diff.
1-2: 💡 Codebase verification
Next.js configuration needs serverRuntimeConfig section
The Next.js configuration file is missing the required
serverRuntimeConfigsection which is being accessed in multiple files includingdesktop-source.ts. This could lead to runtime errors as the code attempts to access undefined configuration values.
- Add
serverRuntimeConfigsection inapps/web/next.config.js- Required by:
apps/web/app/env.tsaccessingserverRuntimeConfig.GAUZY_API_SERVER_URLapps/web/app/services/server/requests/desktop-source.tsaccessingserverRuntimeConfig🔗 Analysis chain
Verify Next.js configuration setup
Ensure that the necessary configuration is properly set up in the Next.js config files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if Next.js configuration exists and contains desktop-related settings fd -e js -e ts "next.config" | while read -r config_file; do echo "Checking $config_file for desktop configuration..." cat "$config_file" | grep -A 10 "serverRuntimeConfig" doneLength of output: 268
Script:
#!/bin/bash # Let's check the content of the Next.js config file directly echo "Content of Next.js config file:" cat apps/web/next.config.js # Also check for any usage of getConfig in the codebase echo -e "\nUsage of getConfig in the codebase:" rg "getConfig" -A 3Length of output: 7829
apps/server-web/src/main/helpers/interfaces/i-menu.ts (1)
1-6: 🛠️ Refactor suggestion
Add documentation and consider making some properties required.
The
AppMenuinterface would benefit from proper documentation and stricter property requirements:
- Add JSDoc comments explaining the interface's purpose and each property's role
- Consider making
idandlabelrequired as they seem essential for menu items+/** + * Represents a top-level menu item in the application. + */ export interface AppMenu { + /** Unique identifier for the menu item */ - id?: string; + id: string; + /** Display text for the menu item */ - label?: string; + label: string; + /** Handler for menu item click events */ click?: () => void; + /** List of submenu items */ submenu?: AppSubMenu[] }📝 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./** * Represents a top-level menu item in the application. */ export interface AppMenu { /** Unique identifier for the menu item */ id: string; /** Display text for the menu item */ label: string; /** Handler for menu item click events */ click?: () => void; /** List of submenu items */ submenu?: AppSubMenu[] }apps/server-web/src/main/helpers/services/desktop-server-factory.ts (1)
11-11:
⚠️ Potential issueReplace 'this' with class name in static context
Using 'this' in a static context can lead to confusion and potential issues. Use the class name explicitly for better clarity.
Apply this fix:
- this.apiInstance.env = env; + DesktopServerFactory.apiInstance.env = env;📝 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.DesktopServerFactory.apiInstance.env = env;🧰 Tools
🪛 Biome (1.9.4)
[error] 11-11: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
apps/server-web/src/main/helpers/services/web-service.ts (2)
8-8:
⚠️ Potential issueConsider maintaining encapsulation and adding type safety.
The
envparameter has been made public and uses theanytype, which:
- Breaks encapsulation by allowing external modification of internal state
- Loses type safety with the
anytypeConsider this safer implementation:
- public env: any, + private readonly env: ServerEnvironmentConfig,Where
ServerEnvironmentConfigwould be a new interface defining the expected environment configuration shape:interface ServerEnvironmentConfig { // Add your environment configuration properties here serviceName?: string; // ... other properties }
41-42:
⚠️ Potential issueImprove type safety and immutability in configuration assignment.
The current implementation has several concerns:
- Public access allows external modification of internal configuration
- Direct object assignment with spread operator is too permissive
- No validation of required properties
Consider this safer implementation:
- public setApiConfig(): void { - Object.assign(this.args, {...this.env}); + private setApiConfig(): void { + // Create a new object instead of mutating + this.args = { + ...this.args, + // Only copy specific, validated properties + apiUrl: this.validateApiUrl(this.env.apiUrl), + // ... other specific properties + }; + } + + private validateApiUrl(url: string): string { + if (!url) { + throw new Error('API URL is required in environment config'); + } + return url; + }Committable suggestion skipped: line range outside the PR's diff.
apps/server-web/src/renderer/components/LanguageSelector.tsx (3)
31-31: 🛠️ Refactor suggestion
Synchronize state with prop changes
Add a useEffect hook to keep the local state synchronized with the lang prop changes.
+import { useEffect } from 'react'; const [lng, setLng] = useState<string>(lang); + +useEffect(() => { + setLng(lang); +}, [lang]);📝 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.import { useEffect } from 'react'; const [lng, setLng] = useState<string>(lang); useEffect(() => { setLng(lang); }, [lang]);
11-20: 🛠️ Refactor suggestion
Move languages to constants
The languages array should be defined as a constant outside the component since it's static data. This prevents unnecessary re-creation on each render.
+const SUPPORTED_LANGUAGES: ILanguages[] = [ + { + code: 'en', + label: 'English', + }, + { + code: 'bg', + label: 'Bulgarian', + }, +]; const LanguageSelector = ({ lang }: LanguageSelector) => { - const [langs] = useState<ILanguages[]>([ - { - code: 'en', - label: 'English', - }, - { - code: 'bg', - label: 'Bulgarian', - }, - ]); + const langs = SUPPORTED_LANGUAGES;📝 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 SUPPORTED_LANGUAGES: ILanguages[] = [ { code: 'en', label: 'English', }, { code: 'bg', label: 'Bulgarian', }, ]; const LanguageSelector = ({ lang }: LanguageSelector) => { const langs = SUPPORTED_LANGUAGES;
23-29:
⚠️ Potential issueAdd error handling for IPC communication
The language change handler should include error handling for IPC communication and ensure state updates are synchronized with the main process response.
const changeLanguage = (data: ILanguages) => { + try { window.electron.ipcRenderer.sendMessage('setting-page', { type: SettingPageTypeMessage.langChange, data: data.code, }); - setLng(data.code); + // Wait for confirmation from main process before updating state + window.electron.ipcRenderer.once('setting-page-response', (response) => { + if (response.success) { + setLng(data.code); + } else { + console.error('Failed to change language:', response.error); + } + }); + } catch (error) { + console.error('Error changing language:', error); + } };Committable suggestion skipped: line range outside the PR's diff.
apps/server-web/src/renderer/pages/setup/Landing.tsx (1)
13-16:
⚠️ Potential issueAdd error handling for IPC communication
The IPC call could fail, but there's no error handling in place. Also, the function name could better reflect its dual responsibility.
-const getCurrentLanguage = async () => { +const fetchAndSetCurrentLanguage = async () => { + try { const lang = await window.electron.ipcRenderer.invoke('current-language'); setDefaultLang(lang); + } catch (error) { + console.error('Failed to fetch language setting:', error); + // Fallback to browser/system language + setDefaultLang(navigator.language.split('-')[0] || 'en'); + } };📝 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 fetchAndSetCurrentLanguage = async () => { try { const lang = await window.electron.ipcRenderer.invoke('current-language'); setDefaultLang(lang); } catch (error) { console.error('Failed to fetch language setting:', error); // Fallback to browser/system language setDefaultLang(navigator.language.split('-')[0] || 'en'); } };apps/server-web/src/main/helpers/constant.ts (1)
20-22: 💡 Codebase verification
Dev tools are properly guarded but menu items need protection
The dev tools functionality (
toggleDevTools) is properly guarded by theisDebugcheck in main.ts, but the menu items that trigger these dev tools are not restricted to development environments. This could expose development features in production builds.
apps/server-web/src/main/menu.ts: Dev menu items should be conditionally added only whenprocess.env.NODE_ENV === 'development'apps/server-web/src/main/main.ts: The dev tools toggle is correctly guarded behindisDebug🔗 Analysis chain
Verify dev window access controls
The addition of development-specific window constants (
SETTING_WINDOW_DEVandSERVER_WINDOW_DEV) suggests debugging features. Ensure these are properly guarded in production environments.
Let me gather more information about the menu structure and environment checks.
Let me check one more aspect - the menu construction and where these dev tools are placed in the menu structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of dev windows to verify environment checks echo "Searching for dev window usage..." rg -A 3 "SETTING_WINDOW_DEV|SERVER_WINDOW_DEV" --type ts # Look for environment checks near usage echo "Checking for environment guards..." ast-grep --pattern 'if ($env === "development") { $$$ }'Length of output: 1598
Script:
#!/bin/bash # Check menu.ts for environment checks or dev-only sections echo "Checking menu.ts for environment guards..." rg -B 3 -A 3 "SETTING_WINDOW_DEV|SERVER_WINDOW_DEV" apps/server-web/src/main/menu.ts # Look for process.env or NODE_ENV checks echo "Checking for environment variables usage..." rg -B 2 "process\.env|NODE_ENV" apps/server-web/src/main/menu.ts # Check if these dev tools are part of a development-only menu echo "Checking for development menu structure..." ast-grep --pattern 'if (process.env.NODE_ENV === "development") { $$$ }'Length of output: 978
Script:
#!/bin/bash # Check the full menu class structure echo "Checking menu class structure..." ast-grep --pattern 'class $_ { $$$ createMenu() { $$$ } $$$ }' # Check if there's any development mode check in the main process echo "Checking for development mode in main process..." rg -B 2 -A 2 "isDevelopment|development" apps/server-web/src/main/main.tsLength of output: 446
apps/server-web/src/main/helpers/replace-config.ts (1)
46-56: 🛠️ Refactor suggestion
Improve error handling and use async operations
The current implementation has several areas for improvement:
- Error handling is too generic and masks important error types
- Synchronous file operations could block the event loop
- Platform compatibility issues with file paths
Consider this improved implementation:
-export const clearDesktopConfig = (folderPath: string) => { +export const clearDesktopConfig = async (folderPath: string): Promise<void> => { const fileNames = ['desktop-server.body', 'desktop-server.meta']; + // Normalize path for cross-platform compatibility + const normalizedPath = os.platform() === 'win32' + ? folderPath.replace(/\\/g, '/') + : folderPath; + try { - // remove cached desktop server config - fileNames.forEach((file) => { - fs.unlinkSync(path.join(folderPath, file)); - }) + await Promise.all( + fileNames.map(async (file) => { + try { + await fs.promises.unlink(path.join(normalizedPath, file)); + console.log(`Successfully removed ${file}`); + } catch (err) { + if (err.code !== 'ENOENT') { + // Only log real errors, not missing files + console.error(`Failed to remove ${file}:`, err.message); + } + } + }) + ); } catch (error) { - console.log('skip unlink file on not exists'); + console.error('Failed to clear desktop config:', error); + throw error; // Propagate error for proper handling upstream } }Key improvements:
- Uses async/await for non-blocking operations
- Better error handling with specific error types
- Cross-platform path normalization
- Proper error propagation
- Improved logging
📝 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.export const clearDesktopConfig = async (folderPath: string): Promise<void> => { const fileNames = ['desktop-server.body', 'desktop-server.meta']; // Normalize path for cross-platform compatibility const normalizedPath = os.platform() === 'win32' ? folderPath.replace(/\\/g, '/') : folderPath; try { await Promise.all( fileNames.map(async (file) => { try { await fs.promises.unlink(path.join(normalizedPath, file)); console.log(`Successfully removed ${file}`); } catch (err) { if (err.code !== 'ENOENT') { // Only log real errors, not missing files console.error(`Failed to remove ${file}:`, err.message); } } }) ); } catch (error) { console.error('Failed to clear desktop config:', error); throw error; // Propagate error for proper handling upstream } }apps/server-web/src/renderer/components/SideBar.tsx (1)
4-4: 🛠️ Refactor suggestion
Remove unused SelectComponent import
The
SelectComponentimport is no longer used after the migration toLanguageSelector. Remove it to prevent unnecessary code in the bundle.-import { SelectComponent } from './Select'; import { ThemeToggler } from './Toggler'; import LanguageSelector from './LanguageSelector';Also applies to: 6-6
.scripts/configure.electron.ts (4)
46-56: 🛠️ Refactor suggestion
Reduce code duplication with a shared utility function
The
revertWebConstantfunction duplicates much of the logic frommodifiedWebConstant.Consider refactoring both functions to use a shared utility:
+interface FileModification { + searchString: string; + replaceString: string; +} + +function modifyConstantsFile(modification: FileModification) { + const filePath = path.resolve(__dirname, '../apps/web/app/constants.ts'); + const backupPath = `${filePath}.backup`; + + try { + fs.copyFileSync(filePath, backupPath); + let fileContent = fs.readFileSync(filePath, 'utf8'); + + if (!fileContent.includes(modification.searchString)) { + throw new Error('Expected configuration line not found'); + } + + fileContent = fileContent.replace(modification.searchString, modification.replaceString); + fs.writeFileSync(filePath, fileContent, 'utf8'); + fs.unlinkSync(backupPath); + } catch (error) { + if (fs.existsSync(backupPath)) { + fs.copyFileSync(backupPath, filePath); + fs.unlinkSync(backupPath); + } + throw error; + } +} + +function modifiedWebConstant() { + return modifyConstantsFile({ + searchString: `export const IS_DESKTOP_APP = process.env.IS_DESKTOP_APP === 'true';`, + replaceString: `export const IS_DESKTOP_APP = true;` + }); +} + +function revertWebConstant() { + return modifyConstantsFile({ + searchString: `export const IS_DESKTOP_APP = true;`, + replaceString: `export const IS_DESKTOP_APP = process.env.IS_DESKTOP_APP === 'true';` + }); +}Committable suggestion skipped: line range outside the PR's diff.
1-6: 🛠️ Refactor suggestion
Improve type safety for command-line arguments
The current implementation uses
anytype for parsed arguments, which bypasses TypeScript's type checking benefits.Consider defining an interface for the expected arguments and using it with yargs:
+interface Arguments { + type: 'server' | 'constant'; +} -const argv: any = yargs(hideBin(process.argv)).argv; +const argv = yargs(hideBin(process.argv)) + .options({ + type: { + type: 'string', + choices: ['server', 'constant'], + demandOption: true, + description: 'Type of configuration to apply' + } + }) + .parseSync() as Arguments;📝 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.import * as fs from 'fs'; import * as path from 'path'; import { hideBin } from 'yargs/helpers'; import yargs from 'yargs'; interface Arguments { type: 'server' | 'constant'; } const argv = yargs(hideBin(process.argv)) .options({ type: { type: 'string', choices: ['server', 'constant'], demandOption: true, description: 'Type of configuration to apply' } }) .parseSync() as Arguments;
34-44:
⚠️ Potential issueImplement safer file modification strategy
The current implementation lacks error handling and file backup mechanisms.
Consider implementing these improvements:
function modifiedWebConstant() { + const filePath = path.resolve(__dirname, '../apps/web/app/constants.ts'); + const backupPath = `${filePath}.backup`; + + try { + // Create backup + fs.copyFileSync(filePath, backupPath); + let fileContent = fs.readFileSync(filePath, 'utf8'); const searchString = `export const IS_DESKTOP_APP = process.env.IS_DESKTOP_APP === 'true';`; const codeToReplace = `export const IS_DESKTOP_APP = true;`; + if (!fileContent.includes(searchString)) { + throw new Error('Expected configuration line not found'); + } + fileContent = fileContent.replace(searchString, codeToReplace); fs.writeFileSync(filePath, fileContent, 'utf8'); + + // Remove backup on success + fs.unlinkSync(backupPath); + } catch (error) { + // Restore from backup if it exists + if (fs.existsSync(backupPath)) { + fs.copyFileSync(backupPath, filePath); + fs.unlinkSync(backupPath); + } + console.error('Error modifying web constants:', error); + process.exit(1); + } }📝 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.function modifiedWebConstant() { const filePath = path.resolve(__dirname, '../apps/web/app/constants.ts'); const backupPath = `${filePath}.backup`; try { // Create backup fs.copyFileSync(filePath, backupPath); let fileContent = fs.readFileSync(filePath, 'utf8'); const searchString = `export const IS_DESKTOP_APP = process.env.IS_DESKTOP_APP === 'true';`; const codeToReplace = `export const IS_DESKTOP_APP = true;`; if (!fileContent.includes(searchString)) { throw new Error('Expected configuration line not found'); } fileContent = fileContent.replace(searchString, codeToReplace); fs.writeFileSync(filePath, fileContent, 'utf8'); // Remove backup on success fs.unlinkSync(backupPath); } catch (error) { // Restore from backup if it exists if (fs.existsSync(backupPath)) { fs.copyFileSync(backupPath, filePath); fs.unlinkSync(backupPath); } console.error('Error modifying web constants:', error); process.exit(1); } }
8-32:
⚠️ Potential issueAdd error handling and path validation
The function lacks proper error handling for file operations and uses hardcoded paths.
Consider implementing these improvements:
function modifiedNextServer() { + try { const filePath = path.resolve(__dirname, '../apps/server-web/release/app/dist/standalone/apps/web/server.js'); + if (!fs.existsSync(filePath)) { + throw new Error(`Server file not found at: ${filePath}`); + } let fileContent = fs.readFileSync(filePath, 'utf8'); const searchString = 'process.env.__NEXT_PRIVATE_STANDALONE_CONFIG'; + + // Validate required environment variables + const requiredEnvVars = ['GAUZY_API_SERVER_URL', 'NEXT_PUBLIC_GAUZY_API_SERVER_URL']; + const missingEnvVars = requiredEnvVars.filter(env => !process.env[env]); + if (missingEnvVars.length > 0) { + throw new Error(`Missing required environment variables: ${missingEnvVars.join(', ')}`); + } // ... rest of the function ... + } catch (error) { + console.error('Error modifying server configuration:', error); + process.exit(1); + } }Committable suggestion skipped: line range outside the PR's diff.
apps/web/app/constants.ts (1)
41-45:
⚠️ Potential issueSecurity and Configuration Concerns in API URL Handling
Several issues need to be addressed in the API URL configuration:
- Security:
- The hardcoded fallback URL should be environment-specific
- Missing URL validation before usage
- Configuration:
- Consider documenting the configuration hierarchy
- Add validation for the server runtime config
Consider applying these improvements:
-let basePath = process.env.GAUZY_API_SERVER_URL ? process.env.GAUZY_API_SERVER_URL : 'https://api.ever.team'; -if (IS_DESKTOP_APP) { - const serverRuntimeConfig = getServerSideProps() - basePath = serverRuntimeConfig?.GAUZY_API_SERVER_URL || basePath; -} +const validateUrl = (url: string): boolean => { + try { + new URL(url); + return true; + } catch { + return false; + } +}; + +const getFallbackUrl = (): string => { + const envFallbacks = { + development: 'http://localhost:3000', + test: 'http://test-api.ever.team', + production: 'https://api.ever.team' + }; + return envFallbacks[process.env.NODE_ENV || 'development']; +}; + +let basePath: string; +if (IS_DESKTOP_APP) { + const serverRuntimeConfig = getServerSideProps(); + const configUrl = serverRuntimeConfig?.GAUZY_API_SERVER_URL; + basePath = validateUrl(configUrl) ? configUrl : getFallbackUrl(); +} else { + const envUrl = process.env.GAUZY_API_SERVER_URL; + basePath = validateUrl(envUrl) ? envUrl : getFallbackUrl(); +}Would you like me to create documentation for the API URL configuration hierarchy?
📝 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 validateUrl = (url: string): boolean => { try { new URL(url); return true; } catch { return false; } }; const getFallbackUrl = (): string => { const envFallbacks = { development: 'http://localhost:3000', test: 'http://test-api.ever.team', production: 'https://api.ever.team' }; return envFallbacks[process.env.NODE_ENV || 'development']; }; let basePath: string; if (IS_DESKTOP_APP) { const serverRuntimeConfig = getServerSideProps(); const configUrl = serverRuntimeConfig?.GAUZY_API_SERVER_URL; basePath = validateUrl(configUrl) ? configUrl : getFallbackUrl(); } else { const envUrl = process.env.GAUZY_API_SERVER_URL; basePath = validateUrl(envUrl) ? envUrl : getFallbackUrl(); }package.json (1)
90-90: 🛠️ Refactor suggestion
Add error handling for copy operations
The
copy:build:webscript performs multiple copy operations without error handling. A failure in any copy operation could leave the build in an inconsistent state.Consider wrapping the copy operations in a shell script with proper error handling:
-"copy:build:web": "pwd && cp -r apps/web/.next/standalone apps/server-web/release/app/dist && cp -r apps/web/.next/static apps/server-web/release/app/dist/standalone/apps/web/.next && cp -r apps/web/public apps/server-web/release/app/dist/standalone/apps/web/ && yarn config:electron:web:build -- --type=server", +"copy:build:web": "yarn ts-node .scripts/copy-build-web.ts",Create a new file
.scripts/copy-build-web.ts:import { copyFile, mkdir } from 'fs/promises'; import { join } from 'path'; async function copyBuildWeb() { try { const paths = [ { src: 'apps/web/.next/standalone', dest: 'apps/server-web/release/app/dist' }, { src: 'apps/web/.next/static', dest: 'apps/server-web/release/app/dist/standalone/apps/web/.next' }, { src: 'apps/web/public', dest: 'apps/server-web/release/app/dist/standalone/apps/web/' } ]; for (const { src, dest } of paths) { await mkdir(dest, { recursive: true }); await copyFile(src, dest); } // Run electron config await import('./configure.electron').then(m => m.default(['--type=server'])); } catch (error) { console.error('Failed to copy build files:', error); process.exit(1); } } copyBuildWeb();apps/web/app/services/client/axios.ts (1)
90-100:
⚠️ Potential issueFix spelling: 'Overide' should be 'Override' in function name and references
The function name
desktopServerOveridecontains a spelling mistake. It should bedesktopServerOverride. This correction ensures consistency and avoids confusion in the codebase.Apply this diff to fix the spelling errors:
-async function desktopServerOveride() { +async function desktopServerOverride() { ... if (IS_DESKTOP_APP) { // dynamic api host while on desktop mode - const runtimeConfig = await desktopServerOveride(); + const runtimeConfig = await desktopServerOverride(); baseURL = runtimeConfig; }Also applies to: 109-109
🧰 Tools
🪛 GitHub Check: Cspell
[warning] 90-90:
Unknown word (Overide)apps/server-web/src/main/main.ts (4)
417-419:
⚠️ Potential issueHandle Undefined
envConfigto Prevent Malformed URLIn the
OPEN_WEBevent handler, ifenvConfigisundefined,envConfig?.PORTwill also beundefined, leading to a malformed URL likehttp://127.0.0.1:undefined. This can cause errors when attempting to open the URL.Modify the code to handle cases where
envConfigorenvConfig.PORTisundefined:const envConfig: ServerConfig | undefined = getEnvApi(); +if (envConfig && envConfig.PORT) { const url = `http://127.0.0.1:${envConfig.PORT}`; shell.openExternal(url); +} else { + console.error('Unable to open web: PORT is undefined.'); + dialog.showErrorBox('Error', 'Unable to open web: PORT is undefined.'); +}📝 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 envConfig: ServerConfig | undefined = getEnvApi(); if (envConfig && envConfig.PORT) { const url = `http://127.0.0.1:${envConfig.PORT}`; shell.openExternal(url); } else { console.error('Unable to open web: PORT is undefined.'); dialog.showErrorBox('Error', 'Unable to open web: PORT is undefined.'); }
465-465:
⚠️ Potential issueIncomplete Statement Causing Syntax Error
Line 465 appears to be incomplete and will cause a syntax error. The line ends with
setupWindow?.without invoking any method or accessing a property.Please complete the statement or remove it if it's unnecessary:
- setupWindow?. + // Complete the statement or remove itCommittable suggestion skipped: line range outside the PR's diff.
498-500:
⚠️ Potential issueVariable Declaration Inside Switch Case Requires Block Scoping
Declaring
const diFilesPathdirectly within acaseclause without a block can lead to scoping issues. Variables declared in acaseclause are hoisted to the function scope and may interfere with variables in other cases.Wrap the
casebody in braces to create a new block scope:case SettingPageTypeMessage.saveSetting: + { LocalStore.updateConfigSetting({ server: arg.data }); const diFilesPath = getWebDirPath(); await clearDesktopConfig( diFilesPath ); // Other code... + }Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 498-498: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
222-224:
⚠️ Potential issuePotential Runtime Error When Spreading 'undefined'
envValIn the
runServerfunction, spreading...envValcan cause a runtime error ifenvValisundefined. Attempting to spreadundefinedwill throw an error at runtime.To fix this, ensure
envValis an object before spreading it. You can provide a default value:- ...envVal, + ...envVal || {},Alternatively, conditionally spread
envValif it exists:await desktopServer.start( { api: serverPath }, - { - ...envVal, - IS_DESKTOP_APP: true - }, + envVal ? { ...envVal, IS_DESKTOP_APP: true } : { IS_DESKTOP_APP: true }, undefined, signal );Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
apps/server-web/src/main/main.ts (3)
13-14: LGTM! Type safety improvements look good.The addition of explicit types (
AppMenu,ServerConfig,WebServer) enhances code maintainability and reduces potential runtime errors.Consider using TypeScript's strict mode and enabling additional type checking flags in
tsconfig.jsonfor even better type safety:{ "compilerOptions": { + "strict": true, + "noImplicitAny": true, + "strictNullChecks": true } }Also applies to: 94-94, 214-215
264-271: Extract language change handler for better maintainability.The debounced language change handler is good for performance, but consider extracting it into a separate function for better maintainability.
+const handleLanguageChange = (storeConfig: WebServer) => { + if (i18nextMainBackend.isInitialized && storeConfig.general?.setup) { + trayMenuItems = trayMenuItems.length ? trayMenuItems : defaultTrayMenuItem(eventEmitter); + updateTrayMenu('none', {}, eventEmitter, tray, trayMenuItems, i18nextMainBackend); + Menu.setApplicationMenu(appMenu.buildDefaultTemplate(appMenuItems, i18nextMainBackend)) + } +}; const storeConfig:WebServer = LocalStore.getStore('config'); -i18nextMainBackend.on('languageChanged', debounce((lng) => { - if (i18nextMainBackend.isInitialized && storeConfig.general?.setup) { - trayMenuItems = trayMenuItems.length ? trayMenuItems : defaultTrayMenuItem(eventEmitter); - updateTrayMenu('none', {}, eventEmitter, tray, trayMenuItems, i18nextMainBackend); - Menu.setApplicationMenu(appMenu.buildDefaultTemplate(appMenuItems, i18nextMainBackend)) - } -}, 250)); +i18nextMainBackend.on('languageChanged', debounce(() => handleLanguageChange(storeConfig), 250));
484-490: Improve path handling robustness.While the path handling logic is good, consider these improvements:
- More descriptive variable names
- Path existence validation
const getWebDirPath = () => { - const dirFiles = 'standalone/apps/web/.next/server/app/api'; + const apiDirectoryPath = 'standalone/apps/web/.next/server/app/api'; - const devDirFilesPath = path.join(__dirname, resourceDir.webServer, dirFiles); - const packDirFilesPath = path.join(process.resourcesPath, 'release', 'app', 'dist', dirFiles) - const diFilesPath = isPack ? packDirFilesPath : devDirFilesPath; + const devApiPath = path.join(__dirname, resourceDir.webServer, apiDirectoryPath); + const prodApiPath = path.join(process.resourcesPath, 'release', 'app', 'dist', apiDirectoryPath); + const finalPath = isPack ? prodApiPath : devApiPath; + + if (!fs.existsSync(finalPath)) { + throw new Error(`API directory not found at: ${finalPath}`); + } - return diFilesPath; + return finalPath; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/server-web/src/main/main.ts(11 hunks)package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🪛 Biome (1.9.4)
apps/server-web/src/main/main.ts
[error] 498-498: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (1)
apps/server-web/src/main/main.ts (1)
200-201: LGTM! Improved setup window UI.
Hiding the menu bar and setting an empty menu template for the setup window creates a cleaner, more focused user interface.
| setupWindow?.show(); | ||
| setupWindow?. | ||
| setupWindow?.webContents.once('did-finish-load', () => { | ||
| setTimeout(() => { | ||
| setupWindow?.webContents.send('languageSignal', storeConfig.general?.lang); | ||
| }, 50) | ||
| }) |
There was a problem hiding this comment.
Fix syntax errors in setup window initialization.
There's a critical syntax error in the setup window initialization code.
- setupWindow?.show();
- setupWindow?.
- setupWindow?.webContents.once('did-finish-load', () => {
+ setupWindow?.show();
+ setupWindow?.webContents.once('did-finish-load', () => {📝 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.
| setupWindow?.show(); | |
| setupWindow?. | |
| setupWindow?.webContents.once('did-finish-load', () => { | |
| setTimeout(() => { | |
| setupWindow?.webContents.send('languageSignal', storeConfig.general?.lang); | |
| }, 50) | |
| }) | |
| setupWindow?.show(); | |
| setupWindow?.webContents.once('did-finish-load', () => { | |
| setTimeout(() => { | |
| setupWindow?.webContents.send('languageSignal', storeConfig.general?.lang); | |
| }, 50) | |
| }) |
There was a problem hiding this comment.
@nyomansutralia-jubelio ^^^^^ did you saw this!?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| const envVal: ServerConfig | undefined = getEnvApi(); | ||
| const folderPath = getWebDirPath(); | ||
| await clearDesktopConfig(folderPath); | ||
|
|
||
| // Instantiate API and UI servers | ||
| await desktopServer.start( | ||
| { api: serverPath }, | ||
| envVal, | ||
| { | ||
| ...envVal, | ||
| IS_DESKTOP_APP: true | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in server configuration.
While the configuration handling is improved, consider adding explicit error handling for the case when envVal is undefined.
const envVal: ServerConfig | undefined = getEnvApi();
+if (!envVal) {
+ throw new Error('Server configuration is missing or invalid');
+}
const folderPath = getWebDirPath();
await clearDesktopConfig(folderPath);
await desktopServer.start(
{ api: serverPath },
{
...envVal,
IS_DESKTOP_APP: true
},
undefined,
signal
);📝 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 envVal: ServerConfig | undefined = getEnvApi(); | |
| const folderPath = getWebDirPath(); | |
| await clearDesktopConfig(folderPath); | |
| // Instantiate API and UI servers | |
| await desktopServer.start( | |
| { api: serverPath }, | |
| envVal, | |
| { | |
| ...envVal, | |
| IS_DESKTOP_APP: true | |
| }, | |
| const envVal: ServerConfig | undefined = getEnvApi(); | |
| if (!envVal) { | |
| throw new Error('Server configuration is missing or invalid'); | |
| } | |
| const folderPath = getWebDirPath(); | |
| await clearDesktopConfig(folderPath); | |
| // Instantiate API and UI servers | |
| await desktopServer.start( | |
| { api: serverPath }, | |
| { | |
| ...envVal, | |
| IS_DESKTOP_APP: true | |
| }, |
| const diFilesPath = getWebDirPath(); | ||
| await clearDesktopConfig( | ||
| diFilesPath |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix switch statement declaration scoping.
The variable declaration in the switch clause could lead to scoping issues.
case SettingPageTypeMessage.saveSetting:
+{
const diFilesPath = getWebDirPath();
await clearDesktopConfig(
diFilesPath
)
+}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 498-498: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
…ver-co/ever-teams into fix/server-web-default-setting
…ver-co/ever-teams into fix/server-web-default-setting
…ver-co/ever-teams into fix/server-web-default-setting
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
LanguageSelectorcomponent for dynamic language selection.Improvements
SideBarcomponent by integrating theLanguageSelector.Bug Fixes
Chores
package.jsonfor better Electron integration.