-
Notifications
You must be signed in to change notification settings - Fork 625
fix: resolve floating button multi-display boundary detection issue #814
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
WalkthroughImplements multi-monitor aware clamping for the floating button’s DRAG_END handler by using Electron’s screen.getDisplayMatching(bounds) to derive the current display’s workArea and constrain the button position within that work area. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant FB as FloatingButtonPresenter
participant ES as Electron screen
participant OS as OS Window
U->>FB: Drag end (DRAG_END)
FB->>FB: Read current bounds
FB->>ES: getDisplayMatching(bounds)
ES-->>FB: Display (workArea)
FB->>FB: Compute clamped (targetX, targetY) within workArea
alt Position changed
FB->>OS: setBounds({ x: targetX, y: targetY })
note right of FB: Adjust only if out-of-bounds
else Already within bounds
FB->>FB: No-op
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
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/floatingButtonPresenter/index.ts (1)
73-85: Adopt structured logging with levels and context.console.log/error violate the guideline requiring structured logs with timestamp, level, codes, stack, and context (and avoiding sensitive data).
Minimal approach using electron-log:
-import { ipcMain, Menu, app, screen } from 'electron' +import { ipcMain, Menu, app, screen } from 'electron' +import log from 'electron-log/main' ... - console.log('FloatingButton is disabled, skipping window creation') + log.info('FloatingButton disabled; skipping window creation', { feature: 'floatingButton' }) ... - console.error('Failed to initialize FloatingButtonPresenter:', error) + log.error('Failed to initialize FloatingButtonPresenter', { + feature: 'floatingButton', + err: error instanceof Error ? { message: error.message, stack: error.stack } : error + })Apply consistently across handlers (INFO for lifecycle, DEBUG for drag positions if needed, ERROR on failures).
Also applies to: 88-90, 129-158, 159-166, 208-229, 231-297, 311-319, 358-387
🧹 Nitpick comments (2)
src/main/presenter/floatingButtonPresenter/index.ts (2)
21-23: Translate comments to English to comply with repo guidelines.Guideline: “Use English for all logs and comments” under src/**/*.{ts,tsx,vue}. Several inline comments are in Chinese.
Example edits:
- /** - * 初始化悬浮按钮功能 - */ + /** + * Initialize floating button feature + */ ... - /** - * 销毁悬浮按钮功能 - */ + /** + * Destroy floating button feature + */ ... - /** - * 启用悬浮按钮 - */ + /** + * Enable floating button + */ ... - /** - * 创建悬浮窗口 - */ + /** + * Create the floating window + */ ... - // 处理拖拽事件 + // Handle drag events ... - // 检查浮窗是否可见,如果可见则隐藏 + // If floating chat window is visible, hide it for dragIf the UI must remain Chinese, keep labels but translate only comments/logs.
Also applies to: 45-47, 70-72, 118-119, 167-176, 185-196
241-254: Use window center to select display with screen.getDisplayNearestPoint (optional)
Use the window’s center point to choose the display, reducing misclassification at edges.- const currentDisplay = screen.getDisplayMatching(bounds) + const centerPoint = { + x: bounds.x + Math.floor(bounds.width / 2), + y: bounds.y + Math.floor(bounds.height / 2) + } + const currentDisplay = screen.getDisplayNearestPoint(centerPoint)Electron ^37.3.0 supports screen.getDisplayNearestPoint(electronjs.org)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/main/presenter/floatingButtonPresenter/index.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development-setup.mdc)
**/*.{js,jsx,ts,tsx}: 使用 OxLint 进行代码检查
Log和注释使用英文书写
Files:
src/main/presenter/floatingButtonPresenter/index.ts
src/{main,renderer}/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/electron-best-practices.mdc)
src/{main,renderer}/**/*.ts: Use context isolation for improved security
Implement proper inter-process communication (IPC) patterns
Optimize application startup time with lazy loading
Implement proper error handling and logging for debugging
Files:
src/main/presenter/floatingButtonPresenter/index.ts
src/main/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/electron-best-practices.mdc)
Use Electron's built-in APIs for file system and native dialogs
From main to renderer, broadcast events via EventBus using mainWindow.webContents.send()
Files:
src/main/presenter/floatingButtonPresenter/index.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/error-logging.mdc)
**/*.{ts,tsx}: 始终使用 try-catch 处理可能的错误
提供有意义的错误信息
记录详细的错误日志
优雅降级处理
日志应包含时间戳、日志级别、错误代码、错误描述、堆栈跟踪(如适用)、相关上下文信息
日志级别应包括 ERROR、WARN、INFO、DEBUG
不要吞掉错误
提供用户友好的错误信息
实现错误重试机制
避免记录敏感信息
使用结构化日志
设置适当的日志级别Enable and adhere to strict TypeScript type checking
Files:
src/main/presenter/floatingButtonPresenter/index.ts
src/main/**/*.{ts,js,tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
主进程代码放在
src/main
Files:
src/main/presenter/floatingButtonPresenter/index.ts
src/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Use English for all logs and comments
Files:
src/main/presenter/floatingButtonPresenter/index.ts
src/main/presenter/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain one presenter per functional domain in src/main/presenter/
Files:
src/main/presenter/floatingButtonPresenter/index.ts
🧠 Learnings (1)
📚 Learning: 2025-07-21T01:45:54.229Z
Learnt from: CR
PR: ThinkInAIXYZ/deepchat#0
File: .cursor/rules/electron-best-practices.mdc:0-0
Timestamp: 2025-07-21T01:45:54.229Z
Learning: Applies to src/main/presenter/index.ts : The IPC messages from the main process to notify the view mainly rely on the EventBus index.ts to listen for events that need to be notified and then send them to the renderer through the mainWindow
Applied to files:
src/main/presenter/floatingButtonPresenter/index.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-check (x64)
🔇 Additional comments (3)
src/main/presenter/floatingButtonPresenter/index.ts (3)
3-3: Main-process-only import of screen is correct.Using Electron's screen in the main process aligns with best practices and is required for multi-display awareness.
241-254: Multi-display clamping is correct and fixes the out-of-bounds bug.Clamping to currentDisplay.workArea with min/max ensures the button stays fully visible across monitors, including negative virtual coordinates and taskbar/dock offsets.
321-338: Verify UI label localization policy.Menu labels are Chinese while logs/comments should be English. If the app targets multiple locales, route labels through your i18n layer; otherwise confirm Chinese UI is intentional.
resolve floating button multi-display boundary detection issue
Summary by CodeRabbit