Conversation
- Extract handleRecoveryAction, showRecoveryDialog, and handleLoadFailure as standalone functions instead of nested closures inside wireWindowRecovery, resolving Qlty's 'many returns' and 'high complexity' smells on wireWindowRecovery. - Bundle shared state (win, name, sampler, showing) into a RecoveryContext object to keep function signatures small after extraction. - Add packages/desktop/test/electron-mock.ts + bunfig.toml preload to enable testing Electron-main-process code with bun:test, since no such pattern existed in this package before. - Add windows.test.ts with 14 new tests covering all branches of the three extracted functions. Fixes CMU-17313Q#83
Export wireWindowRecovery and add tests for did-fail-provisional-load, render-process-gone, unresponsive, responsive, console-message, and preload-error, completing coverage alongside the existing did-fail-load tests.
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
2 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.
P1B: Starter Task: Refactoring PR
1. Issue
#83
packages/desktop/src/main/windows.ts
What do you think this file does?
I think it manages Electron BrowserWindow creation and lifecycle for the desktop app e.g. creating windows, handling titlebar/theme state, external URL navigation, and recovering the UI when a window's renderer crashes or fails to load
What is the scope of your refactoring within that file?
I refactored wireWindowRecovery, extracting its three nested closures (handle, show, failed) into standalone top-level functions: handleRecoveryAction, showRecoveryDialog, and handleLoadFailure.
Which Qlty‑reported issue did you address?
Function with many returns (count = 6) and Function with high complexity (count = 26) in wireWindowRecovery, at packages/desktop/src/main/windows.ts:345
2. Refactoring
How did the specific issue you chose impact the codebase’s maintainability?
Because handle, show, and failed were defined as closures inside wireWindowRecovery, all of their branching logic (if-statements, a while loop, a try/finally) counted toward the parent function's complexity score, making one function responsible for reading, understanding, and testing four distinct behaviors at once.
What changes did you make to resolve the issue?
I moved the 3 closures out to be standalone functions, and bundled the shared mutable state they depended on (win, name, sampler, and a showing flag) into a single RecoveryContext object, passed as one parameter instead of many individual ones, to also avoid triggering a "too many parameters" smell after extraction.
How do your changes improve maintainability? Did you consider alternatives?
Each extracted function is now independently readable, testable, and scored on its own much lower complexity, while wireWindowRecovery itself shrank.
I considered leaving the functions nested but simplifying their internal logic instead, but that wouldn't have addressed the root cause Qlty flagged (nested-function complexity rolling up into the parent), so extraction was the more direct fix.
3. Validation
How did you validate that the change is correct?
I re-ran qlty smells --no-snippets packages/desktop/src/main/windows.ts after the refactor and confirmed the "many returns" and "high complexity" smells on wireWindowRecovery no longer appear. I also ran bun run typecheck and oxlint on the file (both clean), and wrote 24 new tests in windows.test.ts: 14 covering every branch of the three extracted functions (handleRecoveryAction, showRecoveryDialog, handleLoadFailure), plus 10 more directly exercising wireWindowRecovery itself — confirming it correctly wires up and delegates to all 7 of its event listeners (did-fail-load, did-fail-provisional-load, render-process-gone, unresponsive, responsive, console-message, preload-error).
Note: Here I used a new Electron mock (bunfig.toml + test/electron-mock.ts) since this package had no prior pattern for testing Electron-main-process code.
-> The full desktop suite passes at 96/96 tests, up from 78 before my change, with zero regressions.
Attach a screenshot of the test coverage showing the lines were executed by the tests.
Attach a screenshot showing the tests that cover the change passing during CI
Attach a screenshot of
qlty smells --no-snippets <full/path/to/file.ts>showing fewer reported issues after the changes.