Feat/Fix: Added Vitest/Cleaned up duplicate code by TuringProblem · Pull Request #15 · RyeL1te/RyeliteDesktop

TuringProblem · 2025-12-06T18:16:09Z

This pull request introduces several improvements and refactors across the codebase, primarily focused on code style consistency, enhanced settings management, and improved testing capabilities. The most significant changes include the addition of Vitest for testing, refactoring for better readability and maintainability, and updates to IPC handlers and settings APIs for more robust inter-process communication.

Testing and Tooling Enhancements:

Added Vitest and related dependencies (vitest, @vitest/ui, happy-dom) to package.json, along with new npm scripts for running tests, coverage, and UI testing. [1] [2]

Settings Management and IPC Improvements:

Refactored settings management code in src/main/modules/settingsManagement/index.ts for improved readability, including better formatting, more robust dynamic default handling, and enhanced IPC handlers for settings operations such as get, set, select directory, and validate directory. [1] [2] [3] [4] [5] [6]
Updated the preload settings API in src/preload/index.ts to use explicit TypeScript types and improve the interface for settings and screenshot operations.

Code Style and Readability Refactors:

Refactored multiple files for code style consistency, including converting double quotes to single quotes, improving formatting, and splitting long lines for better readability in files such as src/main/windows/client/index.ts, src/main/modules/screenshotManagement/index.ts, and src/main/windows/settings/index.ts. [1] [2] [3] [4] [5] [6]

Window and Event Handling Updates:

Improved logic for closing windows and managing console window state in src/main/index.ts and src/main/windows/client/modules/windowEventManagement/index.ts. [1] [2]
Updated event handler signatures and window management logic for better maintainability and consistency. [1] [2]

Settings Schema Robustness:

Enhanced the SettingsSchema class in src/preload/settings.ts to better handle incoming data and preserve validation and values during schema updates.

These changes collectively improve the developer experience, code maintainability, and reliability of the application's settings and testing infrastructure.

ash-of-the-meadow · 2025-12-07T22:12:30Z

+    selectDirectory: async (options: any): Promise<string | null> =>
+        ipcRenderer.invoke('settings:select-directory', options),
+    validateDirectory: async (dirPath: string): Promise<boolean> =>
+        ipcRenderer.invoke('settings:validate-directory', dirPath),


I think the original code for this particular section is more readable, but the updated code is more explicit with types. I don't have a better suggestion on how to make the code typesafe than what has already been written.

We should think about how to make code like this more readable in the future, but this is fine for now.

I agree with you, I also know we can dive a little deeper so we can find a better solution in terms of readability while still maintaining types. I'll dive deeper in the docs and build types for these so we can prefer the type over verbose version.

ash-of-the-meadow · 2025-12-07T22:15:57Z

+                            Object.prototype.hasOwnProperty.call(
+                                incomingField,
+                                'value'
+                            )


nitpick: I would recommend putting the two closing parenthesis and opening brace on line 32.

code is fine though, i'm not going to request changes just for that

to be honest the original stylistic choice of the hilite codebase is a little rough around the edges. whilst my suggestion would be more consistent, the current format is more readable.

Yeah again this was most definitely from yarn format - I prefer the first as well. I apologize for having it do that my initially thought was that everyone who is going to be working on the repo in the future would also use yarn format.
We could also setup a .eslintrc if there isn't one already?

TuringProblem · 2025-12-06T18:17:13Z

-                out[sectionKey][field.label] = field.value ?? field.default;
-            });
-        });
+        Object.entries(settingsSchema.settings).forEach(


All this was from yarn build - I image that we all want the same formatting?

TuringProblem · 2025-12-07T00:27:01Z

-                const others = BrowserWindow.getAllWindows().filter(w => w !== consoleWindowRef);
-                if (others.length === 0 && consoleWindowRef && !consoleWindowRef.isDestroyed()) {
+                const others = BrowserWindow.getAllWindows().filter(
+                    w => w !== consoleWindowRef


I personally dislike this, again this was from yarn format.

TuringProblem · 2025-12-07T22:17:52Z

+    selectDirectory: async (options: any): Promise<string | null> =>
+        ipcRenderer.invoke('settings:select-directory', options),
+    validateDirectory: async (dirPath: string): Promise<boolean> =>
+        ipcRenderer.invoke('settings:validate-directory', dirPath),


I agree with you, I also know we can dive a little deeper so we can find a better solution in terms of readability while still maintaining types. I'll dive deeper in the docs and build types for these so we can prefer the type over verbose version.

ash-of-the-meadow · 2025-12-07T22:22:16Z

+                    display: flex;
+                    height: -webkit-fill-available;
+                    align-items: anchor-center;
+                "


nit: i would put closing quote at the end of line 69

ash-of-the-meadow · 2025-12-07T22:27:06Z

-            if (warningIcon) {
-                warningIcon.classList.remove('warning', 'error');
-            }
+            if (warningIcon) warningIcon.classList.remove('warning', 'error');


In the future, I am trying to avoid single-line if/else/loop statements in new code -- always prefer braces.

ash-of-the-meadow · 2025-12-07T22:30:03Z

-closeButton.addEventListener('click', () => {
-    window.electron.ipcRenderer.send('close-window');
-});
+setupWindowControls();


Nice work here!!

ash-of-the-meadow · 2025-12-07T22:39:15Z

+
+    describe('Storage Management', () => {
+        it('should handle cache limit enforcement', () => {
+            const enforceMessageLimit = (


The tests should be importing a function from an actual module we're using, and testing that said function operates as it should.

Not all functions are suitable for unit testing, and you may have to change how you do things in some functions in order to unit test properly.

Please remove or update this file to instead import the actual functions you want to unit-test, and then test said functions. Take a look at the sum.test.js example here: https://vitest.dev/guide/

ash-of-the-meadow · 2025-12-07T22:49:05Z

+            btn.addEventListener('click', e => {
+                const type = (e.target as HTMLElement).dataset.type;
+
+                return type ? this.setFilter(type) : undefined;


Please don't return anything here -- as it's currently set up, it's either returning "void" or "undefined".

Just calling setFilter should suffice.

ash-of-the-meadow · 2025-12-07T22:54:25Z

@@ -0,0 +1,142 @@
+import { describe, it, expect, beforeEach } from 'vitest';
+
+class MockConsoleManager {


please have your mock extend from the actual ConsoleManager. then use the base class methods, and verify that the state of the mocked console manager is correct

edit: not sure if "ConsoleManager" is one of our classes or not. If not, then this mock is fine as-is.

ash-of-the-meadow · 2025-12-07T22:55:37Z

+    windowControls.forEach(control => {
+        onClickSend(control.selector, control.channel);
+    });
+};


I love this class and the refactor you've done, great work!!

ash-of-the-meadow · 2025-12-07T22:58:35Z

+            const button = document.getElementById('minimizeBtn');
+            expect(button).toBeTruthy();
+
+            helperModule.onClickSend('#minimizeBtn', 'minimize-window');


Good work! This is how the other tests should be set up, too -- calling one of our actual functions.

ash-of-the-meadow · 2025-12-07T23:11:08Z

+
+    describe('Validation', () => {
+        it('should validate directory paths', () => {
+            const isValidPath = (path: string) => {


If possible, please test the actual function "isValidPath" (if it exists) instead of a local function

ash-of-the-meadow · 2025-12-07T23:11:28Z

+        });
+
+        it('should validate number ranges', () => {
+            const isValidNumber = (value: number, min: number, max: number) => {


ash-of-the-meadow · 2025-12-07T23:12:23Z

+            expect(safeParse('invalid json')).toBeNull();
+        });
+    });
+});


please update the local functions here to instead call the actual functions we want to test (when possible)

ash-of-the-meadow · 2025-12-07T23:15:03Z

+
+    describe('Progress Handling', () => {
+        it('should round progress to nearest integer', () => {
+            const roundProgress = (progress: number) => Math.round(progress);


please test the actual "roundProgress" function we use in the code instead of a local function in this test function

ash-of-the-meadow · 2025-12-07T23:21:31Z

+            );
+        });
+    });
+});


For future reference: Instead of using a local function, (e.g., 'handleUpdateDownloaded') we should change the actual "handleUpdateDownloaded" function to be unit-testable.

We might have to write something like a mock "download" that just returns the "successful" data that the function is expecting.

ash-of-the-meadow

Overall, please see if you can refactor the tests a bit so that more of our actual functions can be tested. You might have to change the functions we're testing themselves. No pressure, though -- any tests are better than no tests! Just see what you can do.

Also, please fix this:

Please don't return anything here -- as it's currently set up, it's either returning "void" or "undefined".

Just calling setFilter should suffice.

edit: Also forgot to mention, the vast majority of the code was excellent and left in a far more readable state than it began. Very good work!

ash-of-the-meadow

Working on re-reviewing this now

ash-of-the-meadow

Good work! Most of my issues were with the formatting, but that's because our formatter is messed up.

For vitest, some testing's better than no testing. I'd rather have darwin fixed than have perfect tests.

TuringProblem added 2 commits December 6, 2025 12:28

feat: added vitest & cleaned up code

3220365

feat: added vitest & cleaned up code

8ebcee6

Metsutan requested a review from ash-of-the-meadow December 6, 2025 22:56

Metsutan changed the base branch from main to staging December 6, 2025 22:58

ash-of-the-meadow reviewed Dec 7, 2025

View reviewed changes

TuringProblem commented Dec 7, 2025

View reviewed changes

ash-of-the-meadow reviewed Dec 7, 2025

View reviewed changes

ash-of-the-meadow requested changes Dec 7, 2025

View reviewed changes

ash-of-the-meadow reviewed Dec 18, 2025

View reviewed changes

ash-of-the-meadow approved these changes Dec 18, 2025

View reviewed changes

		@@ -0,0 +1,142 @@
		import { describe, it, expect, beforeEach } from 'vitest';

		class MockConsoleManager {

Uh oh!

Conversation

TuringProblem commented Dec 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ash-of-the-meadow Dec 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ash-of-the-meadow Dec 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ash-of-the-meadow left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ash-of-the-meadow left a comment

Choose a reason for hiding this comment

Uh oh!

ash-of-the-meadow left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ash-of-the-meadow Dec 7, 2025 •

edited

Loading

ash-of-the-meadow Dec 7, 2025 •

edited

Loading

ash-of-the-meadow left a comment •

edited

Loading