Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix/server web implement config #3368

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Fix/server web implement config #3368

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e47b321
fix: server web default setting
syns2191 Nov 23, 2024
fc16b31
fix: handle dynamic gauzy api host from desktop config
syns2191 Nov 24, 2024
dbb8c89
fix: setup server web
syns2191 Nov 24, 2024
10f062c
fix: remove unecessary code
syns2191 Nov 25, 2024
48e6cbd
fix: miss spell word
syns2191 Nov 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 64 additions & 0 deletions .scripts/configure.electron.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
import * as fs from 'fs';
import * as path from 'path';
import { hideBin } from 'yargs/helpers';
import yargs from 'yargs';

const argv: any = yargs(hideBin(process.argv)).argv;
Comment on lines +1 to +6
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve type safety and argument validation

The current implementation could benefit from better type safety and argument validation:

  1. Replace any type with a proper interface
  2. Add argument validation to ensure required parameters are provided

Consider applying these changes:

 import { hideBin } from 'yargs/helpers';
 import yargs from 'yargs';
 
-const argv: any = yargs(hideBin(process.argv)).argv;
+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 modify'
+    }
+  })
+  .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.

Suggested change
import * as fs from 'fs';
import * as path from 'path';
import { hideBin } from 'yargs/helpers';
import yargs from 'yargs';
const argv: any = yargs(hideBin(process.argv)).argv;
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 modify'
}
})
.parseSync() as Arguments;


function modifiedNextServer() {
const filePath = path.resolve(__dirname, '../apps/server-web/release/app/dist/standalone/apps/web/server.js');

let fileContent = fs.readFileSync(filePath, 'utf8');
const searchString = 'process.env.__NEXT_PRIVATE_STANDALONE_CONFIG';
const codeToInsert = `
nextConfig.serverRuntimeConfig = {
"GAUZY_API_SERVER_URL": process.env.GAUZY_API_SERVER_URL,
"NEXT_PUBLIC_GAUZY_API_SERVER_URL": process.env.NEXT_PUBLIC_GAUZY_API_SERVER_URL
}
`;

let lines = fileContent.split('\n');
const index = lines.findIndex((line) => line.includes(searchString));

if (index !== -1) {
lines.splice(index - 1, 0, codeToInsert);

fileContent = lines.join('\n');
fs.writeFileSync(filePath, fileContent, 'utf8');
console.log('Line of code successfully inserted.');
} else {
console.log(`The string "${searchString}" was not found in the file.`);
}
}
Comment on lines +8 to +32
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add error handling and environment variable validation

The function lacks proper error handling and validation:

  1. File operations could fail
  2. Environment variables might be undefined
  3. String manipulation could be fragile

Consider applying these improvements:

 function modifiedNextServer() {
     const filePath = path.resolve(__dirname, '../apps/server-web/release/app/dist/standalone/apps/web/server.js');
 
+    if (!process.env.GAUZY_API_SERVER_URL || !process.env.NEXT_PUBLIC_GAUZY_API_SERVER_URL) {
+        throw new Error('Required environment variables are not defined');
+    }
+
+    try {
         let fileContent = fs.readFileSync(filePath, 'utf8');
         const searchString = 'process.env.__NEXT_PRIVATE_STANDALONE_CONFIG';
         const codeToInsert = `
         nextConfig.serverRuntimeConfig = {
             "GAUZY_API_SERVER_URL": process.env.GAUZY_API_SERVER_URL,
             "NEXT_PUBLIC_GAUZY_API_SERVER_URL": process.env.NEXT_PUBLIC_GAUZY_API_SERVER_URL
         }
         `;
 
         let lines = fileContent.split('\n');
         const index = lines.findIndex((line) => line.includes(searchString));
 
         if (index !== -1) {
             lines.splice(index - 1, 0, codeToInsert);
 
             fileContent = lines.join('\n');
             fs.writeFileSync(filePath, fileContent, 'utf8');
             console.log('Line of code successfully inserted.');
         } else {
             throw new Error(`The string "${searchString}" was not found in the file.`);
         }
+    } catch (error) {
+        console.error('Failed to modify server configuration:', 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.

Suggested change
function modifiedNextServer() {
const filePath = path.resolve(__dirname, '../apps/server-web/release/app/dist/standalone/apps/web/server.js');
let fileContent = fs.readFileSync(filePath, 'utf8');
const searchString = 'process.env.__NEXT_PRIVATE_STANDALONE_CONFIG';
const codeToInsert = `
nextConfig.serverRuntimeConfig = {
"GAUZY_API_SERVER_URL": process.env.GAUZY_API_SERVER_URL,
"NEXT_PUBLIC_GAUZY_API_SERVER_URL": process.env.NEXT_PUBLIC_GAUZY_API_SERVER_URL
}
`;
let lines = fileContent.split('\n');
const index = lines.findIndex((line) => line.includes(searchString));
if (index !== -1) {
lines.splice(index - 1, 0, codeToInsert);
fileContent = lines.join('\n');
fs.writeFileSync(filePath, fileContent, 'utf8');
console.log('Line of code successfully inserted.');
} else {
console.log(`The string "${searchString}" was not found in the file.`);
}
}
function modifiedNextServer() {
const filePath = path.resolve(__dirname, '../apps/server-web/release/app/dist/standalone/apps/web/server.js');
if (!process.env.GAUZY_API_SERVER_URL || !process.env.NEXT_PUBLIC_GAUZY_API_SERVER_URL) {
throw new Error('Required environment variables are not defined');
}
try {
let fileContent = fs.readFileSync(filePath, 'utf8');
const searchString = 'process.env.__NEXT_PRIVATE_STANDALONE_CONFIG';
const codeToInsert = `
nextConfig.serverRuntimeConfig = {
"GAUZY_API_SERVER_URL": process.env.GAUZY_API_SERVER_URL,
"NEXT_PUBLIC_GAUZY_API_SERVER_URL": process.env.NEXT_PUBLIC_GAUZY_API_SERVER_URL
}
`;
let lines = fileContent.split('\n');
const index = lines.findIndex((line) => line.includes(searchString));
if (index !== -1) {
lines.splice(index - 1, 0, codeToInsert);
fileContent = lines.join('\n');
fs.writeFileSync(filePath, fileContent, 'utf8');
console.log('Line of code successfully inserted.');
} else {
throw new Error(`The string "${searchString}" was not found in the file.`);
}
} catch (error) {
console.error('Failed to modify server configuration:', error);
process.exit(1);
}
}


function modifiedWebConstant() {
const filePath = path.resolve(__dirname, '../apps/web/app/constants.ts');

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;`;

fileContent = fileContent.replace(searchString, codeToReplace);

fs.writeFileSync(filePath, fileContent, 'utf8');
}

function revertWebConstant() {
const filePath = path.resolve(__dirname, '../apps/web/app/constants.ts');

let fileContent = fs.readFileSync(filePath, 'utf8');
const codeToReplace = `export const IS_DESKTOP_APP = process.env.IS_DESKTOP_APP === 'true';`;
const searchString = `export const IS_DESKTOP_APP = true;`;

fileContent = fileContent.replace(searchString, codeToReplace);

fs.writeFileSync(filePath, fileContent, 'utf8');
}
Comment on lines +34 to +56
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Refactor duplicate code and improve error handling

The functions contain duplicate code and lack error handling. Consider refactoring into a single function with a parameter.

Consider this refactoring:

-function modifiedWebConstant() {
-    const filePath = path.resolve(__dirname, '../apps/web/app/constants.ts');
-
-    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;`;
-
-    fileContent = fileContent.replace(searchString, codeToReplace);
-
-    fs.writeFileSync(filePath, fileContent, 'utf8');
-}
-
-function revertWebConstant() {
+function updateWebConstant(setDesktopApp: boolean) {
     const filePath = path.resolve(__dirname, '../apps/web/app/constants.ts');
 
-    let fileContent = fs.readFileSync(filePath, 'utf8');
-    const codeToReplace = `export const IS_DESKTOP_APP = process.env.IS_DESKTOP_APP === 'true';`;
-    const searchString = `export const IS_DESKTOP_APP = true;`;
-
-    fileContent = fileContent.replace(searchString, codeToReplace);
-
-    fs.writeFileSync(filePath, fileContent, 'utf8');
+    try {
+        let fileContent = fs.readFileSync(filePath, 'utf8');
+        const envCheck = `export const IS_DESKTOP_APP = process.env.IS_DESKTOP_APP === 'true';`;
+        const hardcoded = `export const IS_DESKTOP_APP = true;`;
+        
+        const [from, to] = setDesktopApp ? [envCheck, hardcoded] : [hardcoded, envCheck];
+        
+        if (!fileContent.includes(from)) {
+            throw new Error(`Expected content not found in ${filePath}`);
+        }
+        
+        fileContent = fileContent.replace(from, to);
+        fs.writeFileSync(filePath, fileContent, 'utf8');
+        console.log(`Successfully ${setDesktopApp ? 'set' : 'reverted'} IS_DESKTOP_APP`);
+    } catch (error) {
+        console.error('Failed to update 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.

Suggested change
function modifiedWebConstant() {
const filePath = path.resolve(__dirname, '../apps/web/app/constants.ts');
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;`;
fileContent = fileContent.replace(searchString, codeToReplace);
fs.writeFileSync(filePath, fileContent, 'utf8');
}
function revertWebConstant() {
const filePath = path.resolve(__dirname, '../apps/web/app/constants.ts');
let fileContent = fs.readFileSync(filePath, 'utf8');
const codeToReplace = `export const IS_DESKTOP_APP = process.env.IS_DESKTOP_APP === 'true';`;
const searchString = `export const IS_DESKTOP_APP = true;`;
fileContent = fileContent.replace(searchString, codeToReplace);
fs.writeFileSync(filePath, fileContent, 'utf8');
}
function updateWebConstant(setDesktopApp: boolean) {
const filePath = path.resolve(__dirname, '../apps/web/app/constants.ts');
try {
let fileContent = fs.readFileSync(filePath, 'utf8');
const envCheck = `export const IS_DESKTOP_APP = process.env.IS_DESKTOP_APP === 'true';`;
const hardcoded = `export const IS_DESKTOP_APP = true;`;
const [from, to] = setDesktopApp ? [envCheck, hardcoded] : [hardcoded, envCheck];
if (!fileContent.includes(from)) {
throw new Error(`Expected content not found in ${filePath}`);
}
fileContent = fileContent.replace(from, to);
fs.writeFileSync(filePath, fileContent, 'utf8');
console.log(`Successfully ${setDesktopApp ? 'set' : 'reverted'} IS_DESKTOP_APP`);
} catch (error) {
console.error('Failed to update constants:', error);
process.exit(1);
}
}



if (argv.type === 'server') {
modifiedNextServer();
revertWebConstant();
} else if (argv.type === 'constant') {
modifiedWebConstant();
}
24 changes: 12 additions & 12 deletions apps/server-web/src/locales/i18n/bg/translation.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,19 @@
"SERVER_WINDOW": "Прозорец на сървъра"
},
"MENU_APP": {
"ABOUT": "Относно",
"QUIT": "Изход",
"WINDOW": "Прозорец",
"SUBMENU": {
"SETTING": "Настройки",
"SERVER_WINDOW": "Сървърен прозорец",
"LEARN_MORE": "Научете повече",
"DOC": "Документация",
"SETTING_DEV": "Настройки за разработчици",
"SERVER_DEV": "Сървър за разработчици"
"APP_ABOUT": "Относно",
"APP_QUIT": "Изход",
"APP_WINDOW": "Прозорец",
"APP_SUBMENU": {
"APP_SETTING": "Настройки",
"APP_SERVER_WINDOW": "Сървърен прозорец",
"APP_LEARN_MORE": "Научете повече",
"APP_DOC": "Документация",
"APP_SETTING_DEV": "Настройки за разработчици",
"APP_SERVER_DEV": "Сървър за разработчици"
},
"DEV": "Разработчик",
"HELP": "Помощ"
"APP_DEV": "Разработчик",
"APP_HELP": "Помощ"
Comment on lines +18 to +30
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove duplicate translation keys and improve consistency.

There are several issues with the translations:

  1. Duplicate keys: "APP_ABOUT" exists in both the root "MENU" object and "MENU_APP". This can lead to confusion and maintenance issues.
  2. Inconsistent capitalization: Some translations start with capital letters (e.g., "Относно") while others don't (e.g., "относно" in the root MENU).

Suggested fixes:

  1. Remove duplicate translations from the root "MENU" object
  2. Ensure consistent capitalization across translations
{
  "MENU": {
    "SERVER": "Сървър",
-   "APP_ABOUT": "относно",
-   "APP_QUIT": "Откажете се",
    // ... other MENU translations
  },
  "MENU_APP": {
    "APP_ABOUT": "Относно",
    "APP_QUIT": "Изход",
    // ... rest of MENU_APP translations
  }
}

Committable suggestion skipped: line range outside the PR's diff.

},
"FORM": {
"FIELDS": {
Expand Down
24 changes: 12 additions & 12 deletions apps/server-web/src/locales/i18n/en/translation.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,19 @@
"SERVER_WINDOW": "Server Window"
},
"MENU_APP": {
"ABOUT": "About",
"QUIT": "Quit",
"WINDOW": "Window",
"SUBMENU": {
"SETTING": "Setting",
"SERVER_WINDOW": "Server Window",
"LEARN_MORE": "Learn More",
"DOC": "Documentation",
"SETTING_DEV": "Setting Dev.",
"SERVER_DEV": "Server Dev."
"APP_ABOUT": "About",
"APP_QUIT": "Quit",
"APP_WINDOW": "Window",
"APP_SUBMENU": {
"APP_SETTING": "Setting",
"APP_SERVER_WINDOW": "Server Window",
"APP_LEARN_MORE": "Learn More",
"APP_DOC": "Documentation",
"APP_SETTING_DEV": "Setting Dev.",
"APP_SERVER_DEV": "Server Dev."
},
"DEV": "Developer",
"HELP": "Help"
"APP_DEV": "Developer",
"APP_HELP": "Help"
},
"FORM": {
"FIELDS": {
Expand Down
7 changes: 5 additions & 2 deletions apps/server-web/src/main/helpers/constant.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@ export const EventLists = {
SERVER_WINDOW: 'SERVER_WINDOW',
RESTART_SERVER: 'RESTART_SERVER',
CHANGE_THEME: 'CHANGE_THEME',
SETUP_WINDOW: 'SETUP_WINDOW'
SETUP_WINDOW: 'SETUP_WINDOW',
SETTING_WINDOW_DEV: 'SETTING_WINDOW_DEV',
SERVER_WINDOW_DEV: 'SERVER_WINDOW_DEV'
}

export const SettingPageTypeMessage = {
Expand All @@ -38,7 +40,8 @@ export const SettingPageTypeMessage = {
updateSettingResponse: 'update-setting-response',
updateCancel: 'update-cancel',
restartServer: 'restart-server',
themeChange: 'theme-change'
themeChange: 'theme-change',
autoUpdateSetting: 'auto-update-setting'
Comment on lines +43 to +44
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Auto-update setting constant is added but implementation appears to be missing

The constant autoUpdateSetting is defined but there's no evidence of its actual implementation:

  • No IPC handlers or event listeners for auto-update functionality
  • No UI components or settings pages using this constant
  • No auto-update related logic found in the codebase

This suggests the constant might be added prematurely without the corresponding implementation, or the implementation is planned for future changes.

🔗 Analysis chain

Verify the auto-update setting implementation

The addition of autoUpdateSetting suggests new functionality for controlling automatic updates. Let's verify its implementation in both the UI and main process.

Let me gather more information about the auto-update setting implementation.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check the implementation of auto-update settings

# Search for auto-update setting handling
echo "Searching for auto-update setting handlers..."
rg -A 5 "autoUpdateSetting" --type ts

# Look for settings-related components that might use this
echo "Checking settings components..."
ast-grep --pattern 'const $_ = ($$$) => {
  $$$
  autoUpdateSetting
  $$$
}'

Length of output: 708

Script:

#!/bin/bash
# Search for any files related to auto-update settings
echo "Searching for auto-update related files..."
rg -l "auto.?update" --type ts

# Search for IPC or event handling related to auto-update
echo "Checking IPC/event handlers..."
rg -A 5 "ipcMain\.on.*auto.?update|ipcRenderer\.send.*auto.?update" --type ts

# Look for settings UI components
echo "Looking for settings components..."
fd "settings?" -e ts -e tsx -x rg -l "auto.?update" {}

Length of output: 461

}

export const ServerPageTypeMessage = {
Expand Down
2 changes: 1 addition & 1 deletion apps/server-web/src/main/helpers/interfaces/i-constant.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
export type Channels = 'setting-page' | 'ipc-renderer' | 'language-set' | 'updater-page' | 'server-page' | 'theme-change' | 'current-theme';
export type Channels = 'setting-page' | 'ipc-renderer' | 'language-set' | 'updater-page' | 'server-page' | 'theme-change' | 'current-theme' | 'current-language';
15 changes: 15 additions & 0 deletions apps/server-web/src/main/helpers/interfaces/i-menu.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
export interface AppMenu {
id?: string;
label?: string;
click?: () => void;
submenu?: AppSubMenu[]
}
Comment on lines +1 to +6
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider making critical menu properties required and adding JSDoc documentation.

The current interface structure might allow creation of invalid menu items. Consider:

  1. Making label required as menu items typically need visible text
  2. Adding JSDoc documentation to explain the purpose of each property
  3. Adding a type guard or validation utility
+/**
+ * Represents a top-level application menu item
+ */
 export interface AppMenu {
+    /** Unique identifier for the menu item */
     id?: string;
-    label?: string;
+    /** Display text for the menu item - required for visibility */
+    label: string;
+    /** Handler for menu item click events */
     click?: () => void;
+    /** Optional 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.

Suggested change
export interface AppMenu {
id?: string;
label?: string;
click?: () => void;
submenu?: AppSubMenu[]
}
/**
* Represents a top-level application menu item
*/
export interface AppMenu {
/** Unique identifier for the menu item */
id?: string;
/** Display text for the menu item - required for visibility */
label: string;
/** Handler for menu item click events */
click?: () => void;
/** Optional submenu items */
submenu?: AppSubMenu[]
}


export interface AppSubMenu {
id?: string;
label?: string;
click?: () => void;
selector?: string;
type?: string;
accelerator?: string;
}
Comment on lines +8 to +15
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance type safety for submenu properties.

The type and accelerator properties would benefit from stricter typing:

  1. type should be constrained to valid Electron menu item types
  2. accelerator should follow Electron's keyboard shortcut format
+/** Valid menu item types based on Electron's MenuItemConstructorOptions */
+type MenuItemType = 'normal' | 'separator' | 'submenu' | 'checkbox' | 'radio';
+
+/**
+ * Represents a submenu item in the application menu
+ */
 export interface AppSubMenu {
+    /** Unique identifier for the submenu item */
     id?: string;
-    label?: string;
+    /** Display text for the submenu item - required for visibility */
+    label: string;
     click?: () => void;
     selector?: string;
-    type?: string;
+    /** Type of menu item - defaults to 'normal' in Electron */
+    type?: MenuItemType;
+    /** Keyboard shortcut for the menu item (e.g., 'CommandOrControl+Q') */
     accelerator?: string;
 }
📝 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.

Suggested change
export interface AppSubMenu {
id?: string;
label?: string;
click?: () => void;
selector?: string;
type?: string;
accelerator?: string;
}
/** Valid menu item types based on Electron's MenuItemConstructorOptions */
type MenuItemType = 'normal' | 'separator' | 'submenu' | 'checkbox' | 'radio';
/**
* Represents a submenu item in the application menu
*/
export interface AppSubMenu {
/** Unique identifier for the submenu item */
id?: string;
/** Display text for the submenu item - required for visibility */
label: string;
click?: () => void;
selector?: string;
/** Type of menu item - defaults to 'normal' in Electron */
type?: MenuItemType;
/** Keyboard shortcut for the menu item (e.g., 'CommandOrControl+Q') */
accelerator?: string;
}

4 changes: 2 additions & 2 deletions apps/server-web/src/main/helpers/interfaces/i-server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
interface GeneralConfig {
export interface GeneralConfig {
lang?: string
autoUpdate?: boolean
updateCheckPeriode?: string
Expand All @@ -7,7 +7,7 @@ interface GeneralConfig {
[key: string]: any
}

interface ServerConfig {
export interface ServerConfig {
PORT: number;
NEXT_PUBLIC_GAUZY_API_SERVER_URL: string;
GAUZY_API_SERVER_URL: string;
Expand Down
1 change: 1 addition & 0 deletions apps/server-web/src/main/helpers/interfaces/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
export * from './i-server';
export * from './i-desktop-dialog';
export * from './i-constant';
export * from './i-menu';
12 changes: 12 additions & 0 deletions apps/server-web/src/main/helpers/replace-config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,3 +42,15 @@ export const replaceConfig = async (folderPath: string, envOptions: EnvOptions)
console.log('error on replacing file', error);
}
}

export const clearDesktopConfig = (folderPath: string) => {
const fileNames = ['desktop-server.body', 'desktop-server.meta'];
try {
// remove cached desktop server config
fileNames.forEach((file) => {
fs.unlinkSync(path.join(folderPath, file));
})
} catch (error) {
console.log('skip unlink file on not exists');
}
}
Comment on lines +46 to +56
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve robustness of the desktop config cleanup function.

The current implementation has several areas that need improvement:

  1. Using synchronous operations could block the event loop
  2. Error handling loses important context
  3. No validation of the folderPath parameter
  4. No indication of operation success/failure

Consider this improved implementation:

+ const DESKTOP_CONFIG_FILES = ['desktop-server.body', 'desktop-server.meta'] as const;
+
- export const clearDesktopConfig = (folderPath: string) => {
-   const fileNames = ['desktop-server.body', 'desktop-server.meta'];
-   try {
-     // remove cached desktop server config
-     fileNames.forEach((file) => {
-       fs.unlinkSync(path.join(folderPath, file));
-     })
-   } catch (error) {
-     console.log('skip unlink file on not exists');
-   }
- }
+ export const clearDesktopConfig = async (folderPath: string): Promise<boolean> => {
+   if (!folderPath || typeof folderPath !== 'string') {
+     throw new Error('Invalid folder path provided');
+   }
+
+   try {
+     await Promise.all(
+       DESKTOP_CONFIG_FILES.map(async (file) => {
+         const filePath = path.join(folderPath, file);
+         try {
+           await fs.promises.unlink(filePath);
+         } catch (error) {
+           if ((error as NodeJS.ErrnoException).code !== 'ENOENT') {
+             throw error;
+           }
+         }
+       })
+     );
+     return true;
+   } catch (error) {
+     console.error('Failed to clear desktop config:', error);
+     return false;
+   }
+ }
📝 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.

Suggested change
export const clearDesktopConfig = (folderPath: string) => {
const fileNames = ['desktop-server.body', 'desktop-server.meta'];
try {
// remove cached desktop server config
fileNames.forEach((file) => {
fs.unlinkSync(path.join(folderPath, file));
})
} catch (error) {
console.log('skip unlink file on not exists');
}
}
const DESKTOP_CONFIG_FILES = ['desktop-server.body', 'desktop-server.meta'] as const;
export const clearDesktopConfig = async (folderPath: string): Promise<boolean> => {
if (!folderPath || typeof folderPath !== 'string') {
throw new Error('Invalid folder path provided');
}
try {
await Promise.all(
DESKTOP_CONFIG_FILES.map(async (file) => {
const filePath = path.join(folderPath, file);
try {
await fs.promises.unlink(filePath);
} catch (error) {
if ((error as NodeJS.ErrnoException).code !== 'ENOENT') {
throw error;
}
}
})
);
return true;
} catch (error) {
console.error('Failed to clear desktop config:', error);
return false;
}
}

1 change: 1 addition & 0 deletions apps/server-web/src/main/helpers/services/desktop-server-factory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ export class DesktopServerFactory {
if (!this.apiInstance && !!env) {
this.apiInstance = new WebService(path, env, win, signal, eventEmitter);
}
this.apiInstance.env = env;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix static context usage and potential inconsistency issues

The current implementation has several concerns:

  1. Using this in a static context is not recommended
  2. Reassigning env on every call could lead to inconsistent behavior
  3. Missing null check before env reassignment

Consider applying this fix:

- this.apiInstance.env = env;
+ if (DesktopServerFactory.apiInstance && 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.

Suggested change
this.apiInstance.env = env;
if (DesktopServerFactory.apiInstance && env) {
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)

return this.apiInstance;
}
}
50 changes: 37 additions & 13 deletions apps/server-web/src/main/helpers/services/libs/desktop-store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import Store from 'electron-store';
import { WebServer } from '../../interfaces';
import { WebServer, ServerConfig } from '../../interfaces';
const store = new Store();
const DEFAULT_CONFIG:any = {
const DEFAULT_CONFIG:WebServer = {
server: {
PORT: 3002,
GAUZY_API_SERVER_URL: 'http://localhost:3000',
Expand All @@ -11,7 +11,7 @@ const DEFAULT_CONFIG:any = {
general: {
lang: 'en',
autoUpdate: true,
updateCheckPeriode: '1140'
updateCheckPeriode: '1140' // Time in minutes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Correct the misspelling of "updateCheckPeriode" to "updateCheckPeriod".

The property updateCheckPeriode appears to be misspelled. It should be updateCheckPeriod to maintain consistent and correct English spelling.

Apply this diff to correct the property name:

-    		updateCheckPeriode: '1140' // Time in minutes
+    		updateCheckPeriod: '1140' // Time in minutes

Please ensure all references to this property are updated throughout the codebase.

📝 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.

Suggested change
updateCheckPeriode: '1140' // Time in minutes
updateCheckPeriod: '1140' // Time in minutes

}
}
export const LocalStore = {
Expand All @@ -35,17 +35,41 @@ export const LocalStore = {
});
},

deepMerge<T>(target: T, source: Partial<T>): T {
const result: T = { ...target };
Object.keys(source).forEach(key => {
const value = source[key as keyof T];
if (value && typeof value === 'object') {
result[key as keyof T] = this.deepMerge(
target[key as keyof T],
value as any
);
} else if (value !== undefined) {
result[key as keyof T] = value as any;
}
});
return result;
},
Comment on lines +38 to +52
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Ensure the deepMerge method properly handles arrays.

The deepMerge method may not handle arrays as intended because arrays are also of type 'object' in JavaScript. This could lead to unexpected behavior when merging configurations that contain arrays.

Consider modifying the method to correctly handle arrays. For example:

-    		if (value && typeof value === 'object') {
+    		if (value && typeof value === 'object' && !Array.isArray(value)) {

This change prevents recursive merging of arrays, allowing you to decide whether to replace or concatenate arrays based on your requirements.

📝 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.

Suggested change
deepMerge<T>(target: T, source: Partial<T>): T {
const result: T = { ...target };
Object.keys(source).forEach(key => {
const value = source[key as keyof T];
if (value && typeof value === 'object') {
result[key as keyof T] = this.deepMerge(
target[key as keyof T],
value as any
);
} else if (value !== undefined) {
result[key as keyof T] = value as any;
}
});
return result;
},
deepMerge<T>(target: T, source: Partial<T>): T {
const result: T = { ...target };
Object.keys(source).forEach(key => {
const value = source[key as keyof T];
if (value && typeof value === 'object' && !Array.isArray(value)) {
result[key as keyof T] = this.deepMerge(
target[key as keyof T],
value as any
);
} else if (value !== undefined) {
result[key as keyof T] = value as any;
}
});
return result;
},


setDefaultServerConfig: () => {
const defaultConfig: WebServer | any = store.get('config') || {};
Object.keys(DEFAULT_CONFIG).forEach((key) => {
Object.keys(DEFAULT_CONFIG[key]).forEach((keySub) => {
defaultConfig[key] = defaultConfig[key] || {};
defaultConfig[key][keySub] = defaultConfig[key][keySub] || DEFAULT_CONFIG[key][keySub];
})
})
store.set({
config: defaultConfig
validateConfig(config: WebServer): void {
const required = ['PORT', 'GAUZY_API_SERVER_URL'];
required.forEach(field => {
if (!config || !config.server || !config?.server[field as keyof ServerConfig]) {
throw new Error(`Missing required field: ${field}`);
}
});
},

setDefaultServerConfig() {
try {
const storedConfig = store.get('config') as Partial<WebServer> || {};
const mergedConfig = this.deepMerge<WebServer>(DEFAULT_CONFIG, storedConfig)
this.validateConfig(mergedConfig || {});

store.set({ config: mergedConfig });
} catch (error) {
console.error('Failed to set default configuration:', error);
store.set({ config: DEFAULT_CONFIG });
}
}
};
10 changes: 3 additions & 7 deletions apps/server-web/src/main/helpers/services/web-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { EventEmitter } from 'stream';
export class WebService extends ServerTask {
constructor(
readonly path: string,
readonly env: any,
public env: any,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider maintaining encapsulation and improving type safety

Making env public allows external modification of server configuration which could lead to unexpected behavior. Additionally, the any type bypasses TypeScript's type safety features.

Consider these improvements:

  1. Keep env private and provide controlled access methods if needed
  2. Define a proper interface for the environment configuration
- public env: any,
+ private readonly env: IServerEnvironment,

Where IServerEnvironment would be:

interface IServerEnvironment {
  // Add specific environment properties here
  serviceName?: string;
  // ... other config properties
}

readonly window: BrowserWindow,
readonly signal: AbortSignal,
readonly eventEmitter: EventEmitter
Expand Down Expand Up @@ -38,11 +38,7 @@ export class WebService extends ServerTask {
}
}

private setApiConfig(): void {
// Object.assign(this.args, {
// API_HOST: '0.0.0.0',
// API_PORT: this.config.setting.PORT,
// API_BASE_URL: this.config.apiUrl
// });
public setApiConfig(): void {
Object.assign(this.args, {...this.env});
}
}
Loading