raise variable browser window, if already created, otherwise create a… #537
Conversation
… new window. For #1775.
WalkthroughThe changes introduce enhanced window management capabilities in the Minsky Electron application. A new Changes
Sequence DiagramsequenceDiagram
participant User
participant WindowManager
participant ElectronService
participant ActiveWindows
User->>WindowManager: Request Window
WindowManager->>ActiveWindows: Check Existing Windows
alt Window with URL exists
WindowManager->>WindowManager: Raise Existing Window
else No Existing Window
WindowManager->>ElectronService: Create New Window
end
WindowManager->>User: Return Window
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
gui-js/libs/shared/src/lib/interfaces/ActiveWindow.ts (1)
10-10: Consider validating or documenting the newurlproperty.Adding a
urlproperty is a solid idea. However, determine whether any validation or additional documentation is needed, particularly if theurlcan be an empty string or some special format. This helps prevent confusion when referencing it elsewhere in the code.gui-js/apps/minsky-electron/src/app/app.ts (1)
152-152: Defaultingurlto an empty string is fine, but consider clarifying usage.Initialize
urlwith""indicates you may not load a specific route initially. If the app expects a meaningful URL, consider clarifying this in a doc comment or referencing existing usage in theWindowManagerto ensure consistency across the codebase.gui-js/apps/minsky-electron/src/app/managers/WindowManager.ts (1)
191-198: Reassess static usage and confirm code flow for existing windows.
At line 195, the direct call to
this.raiseWindow(url)once again usesthisin a static context. Though valid, consider referencing the class name directly (WindowManager.raiseWindow(...)) if it aligns better with your code style.This branch of code gracefully avoids creating a duplicate window when
raiseIfPresentis set. Ensure all call sites setraiseIfPresentappropriately, especially for windows that must be unique.🧰 Tools
🪛 Biome (1.9.4)
[error] 195-195: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
gui-js/apps/minsky-electron/src/app/app.ts(1 hunks)gui-js/apps/minsky-electron/src/app/managers/WindowManager.ts(3 hunks)gui-js/libs/shared/src/lib/interfaces/ActiveWindow.ts(1 hunks)gui-js/libs/shared/src/lib/interfaces/Interfaces.ts(1 hunks)gui-js/libs/ui-components/src/lib/wiring/variable/variable.component.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
gui-js/apps/minsky-electron/src/app/managers/WindowManager.ts
[error] 138-138: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 195-195: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (5)
gui-js/libs/shared/src/lib/interfaces/ActiveWindow.ts (1)
9-9: Use consistent formatting for interface properties.
Good job updating menu : Menu; to menu: Menu; for consistency. Ensure that similar formatting improvements are applied throughout this file to reduce friction for future contributors.
gui-js/libs/ui-components/src/lib/wiring/variable/variable.component.ts (1)
45-54: Verify presence of raiseIfPresent logic in the WindowManager.
Your addition of raiseIfPresent: true indicates you intend to raise an already open window. Confirm that the WindowManager checks both url and the raiseIfPresent flag as intended. This ensures that existing windows are not duplicated, and new windows are only created when needed.
gui-js/libs/shared/src/lib/interfaces/Interfaces.ts (1)
135-135: Excellent addition of the raiseIfPresent property.
Introducing raiseIfPresent?: boolean; helps manage window duplication. Be sure to handle undefined cases or default it to false to avoid unexpected behaviors.
gui-js/apps/minsky-electron/src/app/managers/WindowManager.ts (2)
134-145: Ensure consistent handling of multiple windows with the same URL.
This logic raises the first matching window by URL and stops searching. If multiple windows share the same URL, only the first match will be raised. Confirm this is the desired behavior or handle multiple matches if needed.
Additionally, note the static analysis suggestion regarding usage of this in a static context (line 138). In TypeScript, using this within a static method is valid, but some codebases prefer referencing the class name, e.g. WindowManager.activeWindows, to avoid confusion.
🧰 Tools
🪛 Biome (1.9.4)
[error] 138-138: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
245-245: Validate URL usage and fallback scenarios.
Storing the URL in childWindowDetails.url is crucial for future lookups or raising existing windows. Verify that the URL is always provided, and handle scenarios where it might be empty or invalid if the code design requires such fallback logic.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
gui-js/apps/minsky-electron/src/app/managers/WindowManager.ts (1)
134-145: Enhance the raiseWindow implementationWhile the functionality is correct, the implementation could be improved for better TypeScript practices and readability.
Consider this alternative implementation:
static raiseWindow(url: string): BrowserWindow { - let window=null; - for (let i of WindowManager.activeWindows) - if (i[1].url==url) { - window=i[1].context; - break; - } - if (window) window.show(); - return window; + const existingWindow = Array.from(WindowManager.activeWindows.values()) + .find(window => window.url === url)?.context; + + if (existingWindow) { + existingWindow.show(); + } + + return existingWindow || null; }This refactored version:
- Uses more idiomatic TypeScript/JavaScript array methods
- Improves variable naming for clarity
- Uses proper spacing and formatting
- Maintains better type safety
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
gui-js/apps/minsky-electron/src/app/managers/WindowManager.ts(11 hunks)gui-js/libs/shared/src/lib/interfaces/ActiveWindow.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- gui-js/libs/shared/src/lib/interfaces/ActiveWindow.ts
🔇 Additional comments (8)
gui-js/apps/minsky-electron/src/app/managers/WindowManager.ts (8)
35-35: LGTM: Improved static method access clarity
The changes from this to WindowManager make the static nature of these methods more explicit and align with TypeScript best practices.
Also applies to: 39-39
52-52: LGTM: Improved code formatting and static access
The changes maintain consistency in static method access and improve the readability of the renderFrame parameters through better formatting.
Also applies to: 62-69
79-82: LGTM: Consistent static method access in tab management
The changes maintain proper static method access while correctly handling tab switching and frame cleanup.
158-158: LGTM: Proper URL handling in popup window creation
The change correctly implements the window URL loading mechanism using the WindowManager's URL utility method.
191-197: LGTM: Proper implementation of window raising logic
The changes correctly implement the window raising functionality, preventing duplicate windows when requested.
245-245: LGTM: Proper window tracking and cleanup
The changes maintain consistent static access and properly handle window tracking and cleanup in the maps.
Also applies to: 249-249, 252-252, 257-257, 260-260, 270-270
284-294: LGTM: Consistent static access in layout handling
The changes maintain proper static method access while preserving the existing layout calculation logic.
Also applies to: 297-298
209-209: Verify modal window behavior
The change in parent window assignment could affect modal window behavior.
✅ Verification successful
Modal window behavior change is safe
Based on the codebase analysis, the change in parent window assignment is safe because:
- The default modal behavior is explicitly set to
truein theWindowManager.createWindowmethod:
const { width, height, minWidth, minHeight, title, modal = true, ... } = payload;- Most windows that need non-modal behavior explicitly set
modal: falsein their creation options, including:
- Menu-related windows
- Godley widget windows
- Plot widget windows
- Ravel widget windows
- Download windows
The parent window assignment change maintains the expected modal behavior - when modal is true, the window will be attached to the main window as its parent, and when modal is false, it won't have a parent window.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for modal window usage patterns
rg -l "modal:\s*true" --type ts
Length of output: 33
Script:
#!/bin/bash
# Search for modal window related patterns with broader context
rg "modal.*window" --type ts -B 2 -A 2
# Search for window creation patterns
rg "createWindow|new\s+BrowserWindow" --type ts -B 2 -A 2
# Search for modal property usage with different patterns
rg "modal:\s*(true|false)|modal\s*=\s*(true|false)" --type ts -B 2 -A 2
Length of output: 10881
… new window. For #1775.
This change is
Summary by CodeRabbit
New Features
urlin the main window details for future enhancements.urlparameter and a flag to raise existing windows.Bug Fixes
Documentation