Conversation
…e-to-17 [Fix] Upgrade Ngx Translation
* fix: close unused window and init window when in use * fix: hide setting window and reduce window memory load * fix: reduce bundle size desktop-timer * fix: reduce bundle size desktop-timer * fix: manage window desktop timer * fix: remove unused config webpack * fix: server, apiserver, and desktop app * fix: server, apiserver, and desktop app * fix: remove unused imported binding module * fix: remove unused variable * fix: review and fix ai suggestion * feat: Implement lazy loading and module optimizations for desktop timer, refactor AppWindowManager properties, and add new build scripts. * fix: revert yarn.lock * fix: clean code * fix: clean code * fix: clean code
Contributor
|
Too many files changed for review. ( |
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
10 issues found across 104 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/desktop-ui-lib/src/lib/settings/plugins/component/plugin-marketplace/+state/effects/plugin-subscription-access.effects.ts">
<violation number="1" location="packages/desktop-ui-lib/src/lib/settings/plugins/component/plugin-marketplace/+state/effects/plugin-subscription-access.effects.ts:5">
P2: Avoid deep importing from @gauzy/ui-core/core/src/…; it bypasses the public API and is brittle for library builds. Import ToastrService from the package entry point as done elsewhere in the repo.</violation>
</file>
<file name="packages/desktop-lib/src/lib/desktop-tray.ts">
<violation number="1" location="packages/desktop-lib/src/lib/desktop-tray.ts:42">
P2: When the settings window already exists, the Settings tray action does not call `show()`, so a hidden settings window stays hidden. Add a show call after `settingShow` so the menu consistently opens the window.</violation>
</file>
<file name="apps/desktop/src/index.ts">
<violation number="1" location="apps/desktop/src/index.ts:342">
P1: `appWindowManager.setupWindow` can be null here, which prevents `DesktopServerFactory` from creating the API service and leaves the local server stopped in the already-configured flow. Ensure a valid BrowserWindow is passed to `server.start` (e.g., fall back to an existing window or initialize the setup window).</violation>
</file>
<file name="packages/desktop-ui-lib/src/lib/image-viewer/image-viewer.component.ts">
<violation number="1" location="packages/desktop-ui-lib/src/lib/image-viewer/image-viewer.component.ts:76">
P2: removeListener uses a new bound function instance, so the original 'show_image' listener is never removed. Store the bound handler and reuse it for both on/removeListener to avoid leaking listeners.</violation>
</file>
<file name="packages/desktop-ui-lib/src/lib/time-tracker/time-tracker.module.ts">
<violation number="1" location="packages/desktop-ui-lib/src/lib/time-tracker/time-tracker.module.ts:4">
P3: Avoid deep-importing from @gauzy/ui-core/theme/src/...; this bypasses the library’s public API and couples the module to internal paths. Import NbTablerIconsModule from the package root instead.</violation>
</file>
<file name="packages/desktop-lib/src/lib/desktop-menu.ts">
<violation number="1" location="packages/desktop-lib/src/lib/desktop-menu.ts:63">
P2: Duplicated logic for opening the settings window. This block is repeated 3 times with only the navigation target changing. Extract this logic into a private helper method (e.g., `private async openSettingsWindow(nav: string): Promise<void>`) to improve maintainability and fix the race condition in one place.</violation>
<violation number="2" location="packages/desktop-lib/src/lib/desktop-menu.ts:65">
P1: Missing `preloadPath` argument in `initSettingWindow` call. The previous implementation passed `windowPath.preloadPath` to `createSettingsWindow`, but it's omitted here. This may break functionality relying on preload scripts.</violation>
<violation number="3" location="packages/desktop-lib/src/lib/desktop-menu.ts:67">
P2: Data payload mismatch: `AppWindowManager.settingShow` sends a different payload structure than `LocalStore.getApplicationConfig()` used in the old code. Specifically, the `activeProject` object is replaced by flattened project fields (e.g., `projectId`), which may break frontend components expecting `activeProject`.</violation>
</file>
<file name="packages/desktop-lib/src/lib/desktop-ipc.ts">
<violation number="1" location="packages/desktop-lib/src/lib/desktop-ipc.ts:881">
P2: Opening an already-created settings window no longer refreshes the app_setting payload, so the settings UI can show stale data. Consider calling settingShow (or equivalent refresh) when the window already exists before showing it.</violation>
</file>
<file name="packages/desktop-lib/src/lib/app-window-manager.ts">
<violation number="1" location="packages/desktop-lib/src/lib/app-window-manager.ts:84">
P2: Pass the preload path into createSetupWindow so the setup window gets the same preload/IPC configuration as other windows when _preloadPath is set.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| try { | ||
| console.log('Starting local server...', path.join(__dirname, 'api/main.js')); | ||
| await server.start({ api: path.join(__dirname, 'api/main.js') }, process.env, setupWindow, signal); | ||
| await server.start({ api: path.join(__dirname, 'api/main.js') }, process.env, appWindowManager.setupWindow, signal); |
Contributor
There was a problem hiding this comment.
P1: appWindowManager.setupWindow can be null here, which prevents DesktopServerFactory from creating the API service and leaves the local server stopped in the already-configured flow. Ensure a valid BrowserWindow is passed to server.start (e.g., fall back to an existing window or initialize the setup window).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/index.ts, line 342:
<comment>`appWindowManager.setupWindow` can be null here, which prevents `DesktopServerFactory` from creating the API service and leaves the local server stopped in the already-configured flow. Ensure a valid BrowserWindow is passed to `server.start` (e.g., fall back to an existing window or initialize the setup window).</comment>
<file context>
@@ -334,7 +339,7 @@ async function startServer(value, restart = false) {
try {
console.log('Starting local server...', path.join(__dirname, 'api/main.js'));
- await server.start({ api: path.join(__dirname, 'api/main.js') }, process.env, setupWindow, signal);
+ await server.start({ api: path.join(__dirname, 'api/main.js') }, process.env, appWindowManager.setupWindow, signal);
} catch (error) {
console.error('ERROR: Occurred while server start:' + error);
</file context>
Suggested change
| await server.start({ api: path.join(__dirname, 'api/main.js') }, process.env, appWindowManager.setupWindow, signal); | |
| const serverWindow = appWindowManager.setupWindow ?? timeTrackerWindow; | |
| await server.start({ api: path.join(__dirname, 'api/main.js') }, process.env, serverWindow, signal); |
| settingsWindow = await createSettingsWindow(settingsWindow, windowPath.timeTrackerUi); | ||
| const appWindowManager = AppWindowManager.getInstance(); | ||
| if (!appWindowManager.settingWindow) { | ||
| await appWindowManager.initSettingWindow(windowPath.timeTrackerUi); |
Contributor
There was a problem hiding this comment.
P1: Missing preloadPath argument in initSettingWindow call. The previous implementation passed windowPath.preloadPath to createSettingsWindow, but it's omitted here. This may break functionality relying on preload scripts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/desktop-lib/src/lib/desktop-menu.ts, line 65:
<comment>Missing `preloadPath` argument in `initSettingWindow` call. The previous implementation passed `windowPath.preloadPath` to `createSettingsWindow`, but it's omitted here. This may break functionality relying on preload scripts.</comment>
<file context>
@@ -58,12 +60,16 @@ export class AppMenu {
- settingsWindow = await createSettingsWindow(settingsWindow, windowPath.timeTrackerUi);
+ const appWindowManager = AppWindowManager.getInstance();
+ if (!appWindowManager.settingWindow) {
+ await appWindowManager.initSettingWindow(windowPath.timeTrackerUi);
+ ipcMain.once('setting_window_ready', () => {
+ appWindowManager.settingShow('goto_update');
</file context>
Suggested change
| await appWindowManager.initSettingWindow(windowPath.timeTrackerUi); | |
| await appWindowManager.initSettingWindow(windowPath.timeTrackerUi, windowPath.preloadPath); |
|
|
||
| import { Injectable } from '@angular/core'; | ||
| import { ToastrService } from '@gauzy/ui-core/core'; | ||
| import { ToastrService } from '@gauzy/ui-core/core/src/lib/services/notification/toastr.service'; |
Contributor
There was a problem hiding this comment.
P2: Avoid deep importing from @gauzy/ui-core/core/src/…; it bypasses the public API and is brittle for library builds. Import ToastrService from the package entry point as done elsewhere in the repo.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/desktop-ui-lib/src/lib/settings/plugins/component/plugin-marketplace/+state/effects/plugin-subscription-access.effects.ts, line 5:
<comment>Avoid deep importing from @gauzy/ui-core/core/src/…; it bypasses the public API and is brittle for library builds. Import ToastrService from the package entry point as done elsewhere in the repo.</comment>
<file context>
@@ -2,7 +2,7 @@
import { Injectable } from '@angular/core';
-import { ToastrService } from '@gauzy/ui-core/core';
+import { ToastrService } from '@gauzy/ui-core/core/src/lib/services/notification/toastr.service';
import { NbDialogService } from '@nebular/theme';
import { createEffect, ofType } from '@ngneat/effects';
</file context>
Suggested change
| import { ToastrService } from '@gauzy/ui-core/core/src/lib/services/notification/toastr.service'; | |
| import { ToastrService } from '@gauzy/ui-core/core'; |
| if (!appWindowManager.settingWindow) { | ||
| await appWindowManager.initSettingWindow(windowPath.timeTrackerUi); | ||
| ipcMain.once('setting_window_ready', () => { | ||
| appWindowManager.settingShow('goto_top_menu'); |
Contributor
There was a problem hiding this comment.
P2: When the settings window already exists, the Settings tray action does not call show(), so a hidden settings window stays hidden. Add a show call after settingShow so the menu consistently opens the window.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/desktop-lib/src/lib/desktop-tray.ts, line 42:
<comment>When the settings window already exists, the Settings tray action does not call `show()`, so a hidden settings window stays hidden. Add a show call after `settingShow` so the menu consistently opens the window.</comment>
<file context>
@@ -26,44 +27,64 @@ export class TrayIcon {
+ if (!appWindowManager.settingWindow) {
+ await appWindowManager.initSettingWindow(windowPath.timeTrackerUi);
+ ipcMain.once('setting_window_ready', () => {
+ appWindowManager.settingShow('goto_top_menu');
+ });
+ appWindowManager.settingWindow?.show?.();
</file context>
| } | ||
|
|
||
| ngOnDestroy(): void { | ||
| this._electronService.ipcRenderer.removeListener('show_image', this.getImages.bind(this)); |
Contributor
There was a problem hiding this comment.
P2: removeListener uses a new bound function instance, so the original 'show_image' listener is never removed. Store the bound handler and reuse it for both on/removeListener to avoid leaking listeners.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/desktop-ui-lib/src/lib/image-viewer/image-viewer.component.ts, line 76:
<comment>removeListener uses a new bound function instance, so the original 'show_image' listener is never removed. Store the bound handler and reuse it for both on/removeListener to avoid leaking listeners.</comment>
<file context>
@@ -49,26 +49,33 @@ export class ImageViewerComponent implements OnInit {
}
+ ngOnDestroy(): void {
+ this._electronService.ipcRenderer.removeListener('show_image', this.getImages.bind(this));
+ }
+
</file context>
| async click() { | ||
| if (!settingsWindow) { | ||
| settingsWindow = await createSettingsWindow(settingsWindow, windowPath.timeTrackerUi); | ||
| const appWindowManager = AppWindowManager.getInstance(); |
Contributor
There was a problem hiding this comment.
P2: Duplicated logic for opening the settings window. This block is repeated 3 times with only the navigation target changing. Extract this logic into a private helper method (e.g., private async openSettingsWindow(nav: string): Promise<void>) to improve maintainability and fix the race condition in one place.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/desktop-lib/src/lib/desktop-menu.ts, line 63:
<comment>Duplicated logic for opening the settings window. This block is repeated 3 times with only the navigation target changing. Extract this logic into a private helper method (e.g., `private async openSettingsWindow(nav: string): Promise<void>`) to improve maintainability and fix the race condition in one place.</comment>
<file context>
@@ -58,12 +60,16 @@ export class AppMenu {
async click() {
- if (!settingsWindow) {
- settingsWindow = await createSettingsWindow(settingsWindow, windowPath.timeTrackerUi);
+ const appWindowManager = AppWindowManager.getInstance();
+ if (!appWindowManager.settingWindow) {
+ await appWindowManager.initSettingWindow(windowPath.timeTrackerUi);
</file context>
| if (!appWindowManager.settingWindow) { | ||
| await appWindowManager.initSettingWindow(windowPath.timeTrackerUi); | ||
| ipcMain.once('setting_window_ready', () => { | ||
| appWindowManager.settingShow('goto_update'); |
Contributor
There was a problem hiding this comment.
P2: Data payload mismatch: AppWindowManager.settingShow sends a different payload structure than LocalStore.getApplicationConfig() used in the old code. Specifically, the activeProject object is replaced by flattened project fields (e.g., projectId), which may break frontend components expecting activeProject.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/desktop-lib/src/lib/desktop-menu.ts, line 67:
<comment>Data payload mismatch: `AppWindowManager.settingShow` sends a different payload structure than `LocalStore.getApplicationConfig()` used in the old code. Specifically, the `activeProject` object is replaced by flattened project fields (e.g., `projectId`), which may break frontend components expecting `activeProject`.</comment>
<file context>
@@ -58,12 +60,16 @@ export class AppMenu {
+ if (!appWindowManager.settingWindow) {
+ await appWindowManager.initSettingWindow(windowPath.timeTrackerUi);
+ ipcMain.once('setting_window_ready', () => {
+ appWindowManager.settingShow('goto_update');
+ });
+ } else {
</file context>
| settingWindow.webContents.send('setting_page_ipc', { | ||
| type: 'goto_top_menu' | ||
| }); | ||
| appWindowManager.settingWindow?.show?.(); |
Contributor
There was a problem hiding this comment.
P2: Opening an already-created settings window no longer refreshes the app_setting payload, so the settings UI can show stale data. Consider calling settingShow (or equivalent refresh) when the window already exists before showing it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/desktop-lib/src/lib/desktop-ipc.ts, line 881:
<comment>Opening an already-created settings window no longer refreshes the app_setting payload, so the settings UI can show stale data. Consider calling settingShow (or equivalent refresh) when the window already exists before showing it.</comment>
<file context>
@@ -852,29 +872,13 @@ export function ipcTimer(
- settingWindow.webContents.send('setting_page_ipc', {
- type: 'goto_top_menu'
- });
+ appWindowManager.settingWindow?.show?.();
});
</file context>
| if (this._setupWindow) { | ||
| return this._setupWindow; | ||
| } | ||
| this._setupWindow = await createSetupWindow(this._setupWindow, false, filePath); |
Contributor
There was a problem hiding this comment.
P2: Pass the preload path into createSetupWindow so the setup window gets the same preload/IPC configuration as other windows when _preloadPath is set.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/desktop-lib/src/lib/app-window-manager.ts, line 84:
<comment>Pass the preload path into createSetupWindow so the setup window gets the same preload/IPC configuration as other windows when _preloadPath is set.</comment>
<file context>
@@ -0,0 +1,197 @@
+ if (this._setupWindow) {
+ return this._setupWindow;
+ }
+ this._setupWindow = await createSetupWindow(this._setupWindow, false, filePath);
+ this.eventCloseWindow(this._setupWindow, WindowName.SETUP);
+ return this._setupWindow;
</file context>
| import { NgModule } from '@angular/core'; | ||
| import { FormsModule, ReactiveFormsModule } from '@angular/forms'; | ||
| import { NbTablerIconsModule } from '@gauzy/ui-core/theme'; | ||
| import { NbTablerIconsModule } from '@gauzy/ui-core/theme/src/lib/icons/tabler-icons.module'; |
Contributor
There was a problem hiding this comment.
P3: Avoid deep-importing from @gauzy/ui-core/theme/src/...; this bypasses the library’s public API and couples the module to internal paths. Import NbTablerIconsModule from the package root instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/desktop-ui-lib/src/lib/time-tracker/time-tracker.module.ts, line 4:
<comment>Avoid deep-importing from @gauzy/ui-core/theme/src/...; this bypasses the library’s public API and couples the module to internal paths. Import NbTablerIconsModule from the package root instead.</comment>
<file context>
@@ -1,7 +1,7 @@
import { NgModule } from '@angular/core';
import { FormsModule, ReactiveFormsModule } from '@angular/forms';
-import { NbTablerIconsModule } from '@gauzy/ui-core/theme';
+import { NbTablerIconsModule } from '@gauzy/ui-core/theme/src/lib/icons/tabler-icons.module';
import {
NbBadgeModule,
</file context>
|
View your CI Pipeline Execution ↗ for commit b5408b3
☁️ Nx Cloud last updated this comment at |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR
Please note: we will close your PR without comment if you do not check the boxes above and provide ALL requested information.
Summary by cubic
Upgraded internationalization to ngx-translate v17 and rebuilt the i18n plumbing. Added a new AppWindowManager and lazy-loaded routes to cut desktop memory usage and reduce bundle sizes.
Refactors
Dependencies
Written for commit b5408b3. Summary will update on new commits.