Fix windows not opening on Windows Machines#36
Conversation
WalkthroughThis update modifies window visibility handling in the main and onboarding processes, ensuring windows are shown immediately or only after they are ready. It adds extensive debug logging during app initialization, always enables debug prints, and adjusts minor UI details such as border radius documentation, padding, and sidebar hover width. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Main Process
participant BrowserWindow
participant OnboardingWindow
App->>BrowserWindow: Create (without show: false)
BrowserWindow-->>App: Emits "ready-to-show"
App->>BrowserWindow: Show and focus window
App->>OnboardingWindow: Create (with show: true)
Note right of OnboardingWindow: Window is immediately visible
Assessment against linked issues
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (4)
docs/references/window-border.md (1)
3-5: Enhance documentation clarity.
The bullet list correctly notes pixel values but lacks context about which elements use each radius/padding. Consider expanding points into descriptive sentences or grouping under subheadings (e.g.:
- Default window border radius: 4px
- setBorderRadius(): applies 8px
- Tab component padding: 8px in
main.tsx)
to improve readability and maintainability.src/main/modules/flags.ts (1)
26-26: Debug prints now always enabledYou've changed the
SHOW_DEBUG_PRINTSflag from a conditional value based on!app.isPackagedto a constanttrue. This means debug prints will now be shown in production builds as well.This change is reasonable for debugging the window opening issue on Windows machines, but you may want to consider adding a TODO comment to revert this change once the Windows issue is resolved, or implementing a more sophisticated logging mechanism for production.
- SHOW_DEBUG_PRINTS: true, + SHOW_DEBUG_PRINTS: true, // TODO: Revert to !app.isPackaged after Windows issue is fixedsrc/main/onboarding/main.ts (1)
48-56: Potentially redundant show callThe
onboarding.show()method still callsonboardingWindow.show()even though the window is now created withshow: true. This is harmless but redundant.Consider removing or refactoring this to be more explicit about the intent.
export const onboarding = { show: async () => { if (!onboardingWindow) { await createOnboardingWindow(); } if (!onboardingWindow) return; onboardingWindow.show(); // This is now redundant since the window is created with show:true onboardingWindow.focus(); },src/main/index.ts (1)
11-196: Consider conditional debug logging for production builds.With
SHOW_DEBUG_PRINTSnow always enabled (per the summary), these debug messages will appear in production builds. While helpful for diagnosing this issue, consider adding a condition to disable these logs in production once the issue is resolved.- import { debugPrint } from "@/modules/output"; + import { debugPrint, FLAGS } from "@/modules/output"; // Then in appropriate places: - debugPrint("INITIALIZATION", "message"); + if (!app.isPackaged || FLAGS.ENABLE_EXTENDED_DEBUGGING) { + debugPrint("INITIALIZATION", "message"); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
docs/references/window-border.md(1 hunks)src/main/browser/window.ts(1 hunks)src/main/index.ts(4 hunks)src/main/modules/flags.ts(1 hunks)src/main/modules/output.ts(1 hunks)src/main/onboarding/main.ts(1 hunks)src/main/settings/main.ts(0 hunks)src/renderer/src/components/browser-ui/browser-content.tsx(1 hunks)src/renderer/src/components/browser-ui/main.tsx(1 hunks)src/renderer/src/components/browser-ui/sidebar/hover-detector.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- src/main/settings/main.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/index.ts (2)
src/main/modules/output.ts (1)
debugPrint(22-28)src/main/onboarding/main.ts (1)
onboarding(47-83)
🔇 Additional comments (11)
src/renderer/src/components/browser-ui/sidebar/hover-detector.tsx (1)
30-30: Verify hover detection width on Windows.
Reducing the hover detector width fromw-2(8px) tow-1.5(6px) tightens the UI but could reduce hover-trigger reliability, particularly on Windows devices. Please validate this change on actual Windows machines and ensurew-1.5is recognized in your Tailwind configuration (JIT mode or custom spacing).src/renderer/src/components/browser-ui/main.tsx (1)
86-86: Approve padding adjustment and verify consistency with docs.
Changing the container padding fromp-2.5(10px) top-2(8px) aligns with the 8px padding specified indocs/references/window-border.md. This standardizes spacing across the UI. Please double-check that this reduced padding does not negatively affect content layouts in other screen sizes or states.src/renderer/src/components/browser-ui/browser-content.tsx (1)
30-31: Approve class reordering.
Extracting"rounded-lg"into its own argument in thecn()call does not alter rendered styles and improves readability by logically separating the border-radius utility from the rest of the class list.src/main/modules/output.ts (1)
5-5: Good addition of initialization debug areaAdding the
INITIALIZATIONdebug area will help track app startup, which is likely related to the window opening issue.This aligns well with the changes to make debug prints always visible, and will provide valuable diagnostic information.
src/main/browser/window.ts (2)
72-72: Window show behavior modificationYou've removed the explicit
show: falseoption from the BrowserWindow constructor. This allows the window to be immediately visible upon creation, rather than being initially hidden.This is likely a key fix for the Windows window opening issue, as it simplifies the window visibility management by removing the need to manually hide and show the window in succession.
119-122: Window visibility strategy changeThe window is now shown only when it's ready to display content, preventing flash of empty content while maintaining responsiveness.
This approach is cleaner and more reliable across platforms, especially on Windows where the previous hide/show sequence may have been causing issues.
src/main/onboarding/main.ts (1)
18-18: Window visibility approach made consistentSetting
show: truein the onboarding window options is consistent with the changes in the main window creation. This unified approach to window visibility management likely addresses the root cause of windows not opening on Windows machines.This is a good practice to maintain consistency across the codebase.
src/main/index.ts (4)
11-11: Good addition of debugPrint for troubleshooting.Adding the debugPrint import enables the detailed logging that follows, which is essential for diagnosing the window opening issues on Windows machines.
136-136: Well-placed debug logging helps diagnose window visibility issues.The strategically placed debug logs throughout the initialization process provide excellent visibility into the application startup flow, particularly around window creation and visibility. These logs will help identify exactly where the window display process might be failing on Windows machines.
I notice these logs align with the window visibility changes mentioned in the summary (changing windows to be shown immediately rather than waiting for 'ready-to-show'), which directly addresses the core issue of windows not appearing on Windows machines.
Also applies to: 148-148, 177-177, 182-182, 188-198
150-150: Comment formatting improvement.The comment is now properly formatted and aligned with other comments in the codebase.
134-141:Details
❓ Verification inconclusive
Verify single instance behavior still works properly.
The code no longer exits early on "electron-squirrel-startup" detection (according to the summary), but still exits if it couldn't get the single instance lock. Please verify that this change doesn't affect Windows installation or update processes.
🏁 Script executed:
#!/bin/bash # Check for any squirrel-related code that might have been removed rg -A 2 -B 2 "electron-squirrel-startup" --type tsLength of output: 50
🏁 Script executed:
#!/bin/bash # Thoroughly search for any remaining “electron-squirrel-startup” import or require in TS files rg -n "import ['\"]electron-squirrel-startup['\"]" --type ts rg -n "require\(['\"]electron-squirrel-startup['\"]\)" --type tsLength of output: 137
Verify Windows installer/update behavior
I searched the entire codebase and found no remaining imports or requires for
electron-squirrel-startup. Since the early exit on squirrel events appears fully removed, please manually confirm that Windows installation and auto-updates still run correctly, and that the single-instance lock logic (ininitializeApp) doesn’t interfere with those workflows.
1 participant
closes #34
Summary by CodeRabbit