-
Notifications
You must be signed in to change notification settings - Fork 625
fix: fix macos full window #1254
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughOn macOS, tooltip overlay creation and display are suppressed while the parent window is fullscreen. Overlays are destroyed when entering fullscreen and recreated when exiting; initial pre-creation and show paths now return null if the target window is fullscreen. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer as Renderer (UI)
participant Presenter as WindowPresenter
participant Overlay as TooltipOverlay
participant System as macOS Window System
Renderer->>Presenter: requestShowTooltip(targetWindow, coords)
alt targetWindow is fullscreen (macOS)
Presenter-->>Renderer: return null / suppress show
note right of Presenter `#f6f8ff`: No overlay created or shown
else targetWindow not fullscreen
Presenter->>Overlay: getOrCreateTooltipOverlay(targetWindow)
alt overlay created or exists
Overlay->>Presenter: ready
Presenter->>Overlay: showTooltip(coords)
Overlay->>Renderer: rendered tooltip
end
end
System->>Presenter: windowDidEnterFullscreen(targetWindow)
Presenter->>Overlay: destroyOverlayIfExists(targetWindow)
note right of Overlay `#fff5e6`: overlay removed to avoid covering traffic lights
System->>Presenter: windowDidLeaveFullscreen(targetWindow)
Presenter->>Overlay: recreateOverlayIfNeeded(targetWindow)
note right of Overlay `#eafff0`: overlay reinitialized with macOS visibility flags
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ 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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/presenter/windowPresenter/index.ts (1)
1-1: Fix Prettier formatting before merge.The pipeline shows a Prettier formatting check failure. Please run the formatter to fix code style issues:
pnpm run formator
prettier --write src/main/presenter/windowPresenter/index.ts
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/presenter/windowPresenter/index.ts
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use English for logs and comments in TypeScript/JavaScript code
Files:
src/main/presenter/windowPresenter/index.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use TypeScript with strict type checking enabled
Use OxLint for linting JavaScript and TypeScript files; ensure lint-staged hooks and typecheck pass before commits
Files:
src/main/presenter/windowPresenter/index.ts
src/main/presenter/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
src/main/presenter/**/*.ts: Use EventBus to broadcast events from main to renderer viamainWindow.webContents.send()
Implement one presenter per functional domain in the main process
Files:
src/main/presenter/windowPresenter/index.ts
src/main/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
src/main/**/*.ts: Use EventBus fromsrc/main/eventbus.tsfor decoupled inter-process communication
Context isolation must be enabled with preload scripts for secure IPC communicationElectron main process code should reside in
src/main/, with presenters organized inpresenter/subdirectory (Window, Tab, Thread, Mcp, Config, LLMProvider), and app events managed viaeventbus.ts
Files:
src/main/presenter/windowPresenter/index.ts
**/*.{js,ts,tsx,jsx,vue,mjs,cjs}
📄 CodeRabbit inference engine (.cursor/rules/development-setup.mdc)
All logs and comments must be in English
Files:
src/main/presenter/windowPresenter/index.ts
**/*.{js,ts,tsx,jsx,mjs,cjs}
📄 CodeRabbit inference engine (.cursor/rules/development-setup.mdc)
Use OxLint as the linter
Files:
src/main/presenter/windowPresenter/index.ts
**/*.{js,ts,tsx,jsx,vue,json,mjs,cjs}
📄 CodeRabbit inference engine (.cursor/rules/development-setup.mdc)
Use Prettier as the code formatter
Files:
src/main/presenter/windowPresenter/index.ts
**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,vue}: Use camelCase for variable and function names; use PascalCase for types and classes; use SCREAMING_SNAKE_CASE for constants
Configure Prettier with single quotes, no semicolons, and line width of 100 characters. Runpnpm run formatafter completing features
Files:
src/main/presenter/windowPresenter/index.ts
🪛 GitHub Actions: PR Check
src/main/presenter/windowPresenter/index.ts
[error] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues in this file.
🔇 Additional comments (7)
src/main/presenter/windowPresenter/index.ts (7)
86-89: Good defensive check to suppress tooltips in macOS fullscreen.This early return prevents tooltip overlays from obscuring macOS traffic lights during fullscreen, which aligns with the PR objective of fixing fullscreen window behavior.
865-866: Proper cleanup when entering fullscreen.Destroying the tooltip overlay ensures it cannot interfere with macOS traffic lights or window management during fullscreen mode. This pairs well with the recreation logic on exit (line 884).
883-884: LGTM: Overlay recreation restores normal tooltip behavior.The overlay is recreated after exiting fullscreen, restoring normal tooltip functionality. Since
leave-full-screenis emitted after the window has exited fullscreen,isFullScreen()returns false and the overlay creation succeeds.
1005-1008: Good edge-case handling for windows created in fullscreen.The conditional pre-creation ensures overlay is not created if the window is already in fullscreen on macOS, maintaining consistency with the fullscreen suppression strategy.
1082-1083: Critical guard prevents overlay creation during macOS fullscreen.This central check ensures the overlay cannot be created while in fullscreen, preventing interference with traffic lights. All callers properly handle the null return value.
86-89: Comprehensive fix for macOS fullscreen tooltip interference.The changes implement a multi-layered approach to prevent tooltip overlays from interfering with macOS fullscreen behavior:
- Suppress tooltip display requests during fullscreen (line 86-89)
- Destroy overlay when entering fullscreen (line 865-866)
- Recreate overlay when exiting fullscreen (line 883-884)
- Prevent pre-creation if window starts in fullscreen (line 1005-1008)
- Guard overlay creation during fullscreen (line 1082-1083)
- Configure overlay to stay off fullscreen spaces (line 1119-1124)
The logic is consistent and should resolve the reported issue where windows cannot be reduced/shrunk from fullscreen on macOS.
Please manually test the following scenarios on macOS to ensure the fix works correctly and doesn't introduce regressions:
- Fullscreen behavior: Verify that the window can now be exited from fullscreen mode (clicking green traffic light, Escape key, gesture)
- Traffic lights accessibility: Ensure traffic light buttons remain accessible and responsive in fullscreen
- Tooltip functionality: Confirm tooltips work correctly in normal (non-fullscreen) window mode
- Tooltip after fullscreen exit: Verify tooltips work properly after exiting fullscreen
- Window created in fullscreen: Test creating a window that starts in fullscreen mode
As per coding guidelines, ensure all changes follow TypeScript strict type checking and EventBus patterns for IPC communication.
Also applies to: 865-866, 883-884, 1005-1008, 1082-1083, 1119-1124
1119-1124: macOS-specific configuration correctly prevents overlay from covering traffic lights.The
setVisibleOnAllWorkspaces(true, { visibleOnFullScreen: false })call ensures the overlay stays behind fullscreen spaces, protecting macOS traffic lights. The API and option are supported in Electron 37.10.3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/presenter/windowPresenter/index.ts (1)
883-884: Consider lazy recreation for better resource efficiency.The current implementation eagerly recreates the tooltip overlay when exiting fullscreen, even if it won't be shown immediately. While this ensures tooltip functionality is restored, a more resource-efficient approach would be to let the overlay be recreated lazily on the next
shell-tooltip:showrequest (which already callsgetOrCreateTooltipOverlay).However, the current approach is simpler and defensive, so this is an optional optimization.
🔎 Optional refactor for lazy recreation
- // Recreate tooltip overlay after exiting fullscreen for normal behavior - this.getOrCreateTooltipOverlay(shellWindow) + // Overlay will be recreated lazily on next tooltip show request
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/presenter/windowPresenter/index.ts
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use English for logs and comments in TypeScript/JavaScript code
Files:
src/main/presenter/windowPresenter/index.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use TypeScript with strict type checking enabled
Use OxLint for linting JavaScript and TypeScript files; ensure lint-staged hooks and typecheck pass before commits
Files:
src/main/presenter/windowPresenter/index.ts
src/main/presenter/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
src/main/presenter/**/*.ts: Use EventBus to broadcast events from main to renderer viamainWindow.webContents.send()
Implement one presenter per functional domain in the main process
Files:
src/main/presenter/windowPresenter/index.ts
src/main/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
src/main/**/*.ts: Use EventBus fromsrc/main/eventbus.tsfor decoupled inter-process communication
Context isolation must be enabled with preload scripts for secure IPC communicationElectron main process code should reside in
src/main/, with presenters organized inpresenter/subdirectory (Window, Tab, Thread, Mcp, Config, LLMProvider), and app events managed viaeventbus.ts
Files:
src/main/presenter/windowPresenter/index.ts
**/*.{js,ts,tsx,jsx,vue,mjs,cjs}
📄 CodeRabbit inference engine (.cursor/rules/development-setup.mdc)
All logs and comments must be in English
Files:
src/main/presenter/windowPresenter/index.ts
**/*.{js,ts,tsx,jsx,mjs,cjs}
📄 CodeRabbit inference engine (.cursor/rules/development-setup.mdc)
Use OxLint as the linter
Files:
src/main/presenter/windowPresenter/index.ts
**/*.{js,ts,tsx,jsx,vue,json,mjs,cjs}
📄 CodeRabbit inference engine (.cursor/rules/development-setup.mdc)
Use Prettier as the code formatter
Files:
src/main/presenter/windowPresenter/index.ts
**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,vue}: Use camelCase for variable and function names; use PascalCase for types and classes; use SCREAMING_SNAKE_CASE for constants
Configure Prettier with single quotes, no semicolons, and line width of 100 characters. Runpnpm run formatafter completing features
Files:
src/main/presenter/windowPresenter/index.ts
🔇 Additional comments (4)
src/main/presenter/windowPresenter/index.ts (4)
86-89: LGTM! Effective guard to preserve traffic light accessibility.The early return correctly prevents tooltip overlays from appearing in macOS fullscreen mode, ensuring system traffic light buttons remain accessible for users to exit fullscreen.
865-866: LGTM! Proper cleanup when entering fullscreen.Destroying the tooltip overlay when entering fullscreen is the correct approach to ensure it doesn't obstruct macOS system controls.
1005-1008: LGTM! Handles edge case of window created in fullscreen.The guard correctly prevents pre-creating the overlay when a window is already in fullscreen mode at load time, maintaining consistency with the fullscreen behavior.
1082-1083: LGTM! Central guard prevents overlay creation in fullscreen.This is the primary enforcement point that prevents tooltip overlay creation on macOS fullscreen. The null return is properly handled by all callers.
* fix: fix macos full window * fix: format
close #1243
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.