Improve menu ticker and portfolio UI#2
Closed
1254455745 wants to merge 6 commits into
Closed
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough此 PR 为 Ticker 增加可配置显示与布局选项、扩展持久化键与偏好迁移，重构 MenuBar 视图以支持可选图标/可选宽度与动画失效，更新 Popover 行为与 Renderer，并修正/现代化部分基础设施并发与内存访问模式。 变更Ticker 显示与布局可配置化
预期评审工作量🎯 4 (Complex) | ⏱️ ~60 minutes 彩诗
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
PanBar/Popover/PopoverController.swift (1)
49-57:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
didClose观察者未正确释放，存在回调泄漏风险。这里使用的是 block-based observer，但没有保存并移除返回 token；
removeObserver(self)不会移除该观察者。建议保存 token 并在deinit显式移除。建议修复
final class PopoverController { + private var didCloseObserver: NSObjectProtocol? @@ - NotificationCenter.default.addObserver( + didCloseObserver = NotificationCenter.default.addObserver( forName: NSPopover.didCloseNotification, object: popover, queue: .main ) { [weak self] _ in Task { `@MainActor` in self?.handlePopoverClosed() } } @@ deinit { if let m = eventMonitor { NSEvent.removeMonitor(m) } - NotificationCenter.default.removeObserver(self) + if let token = didCloseObserver { + NotificationCenter.default.removeObserver(token) + } }Also applies to: 60-63
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PanBar/Popover/PopoverController.swift` around lines 49 - 57, The block-based NotificationCenter observer for NSPopover.didCloseNotification is not stored or removed causing a leak; capture the returned observer token (e.g., a property like popoverDidCloseObserver: NSObjectProtocol?) when calling NotificationCenter.default.addObserver(forName:object:queue:using:) and call NotificationCenter.default.removeObserver(popoverDidCloseObserver!) (or nil-check) in deinit (and do the same for the other observer at lines 60-63), ensuring the closure still calls self?.handlePopoverClosed() via Task { `@MainActor` in … } while storing/removing the token to prevent callback retention.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@PanBar/Infrastructure/TickerPreferences.swift`:
- Around line 167-171: Init currently assigns directly to
menuBarWidth/scrollMenuBarWidth/carouselMenuBarWidth/compactMenuBarWidth which
bypasses their didSet clamp-and-persist logic; instead, read raw values from
repo into local Ints inside init(repo:), then pass those values through the same
clamping+persist path (either by calling a new private helper like
applyMenuBarWidths(menu:,scroll:,carousel:,compact:) or by invoking existing
setter methods you add such as setMenuBarWidth(_:), setScrollMenuBarWidth(_:),
setCarouselMenuBarWidth(_:), setCompactMenuBarWidth(_:)), so that the clamp
logic and repo.set(...) for SettingsRepository.Keys.ticker* are executed for
stored (possibly out-of-range) values.
In `@PanBar/MenuBar/TickerRenderer.swift`:
- Around line 33-35: The guard that returns an empty NSAttributedString when
items.isEmpty causes the status item to collapse; instead return a minimal
placeholder string so the menu bar item remains visible. In TickerRenderer (the
method that builds the attributed title where you currently check `guard
!items.isEmpty else { return NSAttributedString() }`), replace the empty return
with an NSAttributedString containing a single placeholder character (e.g. "—"
or a localized "no data" string) using the same attributes/style used for normal
items so sizing and accessibility remain consistent.
In `@PanBar/MenuBar/TickerView.swift`:
- Around line 43-49: The early return for empty attributed text causes
preferredTotalWidth to be ignored when content is briefly empty; reorder the
checks in the width calculation so that if preferredTotalWidth is non-nil it is
returned (max(40, preferredTotalWidth)) before the empty-content branch that
returns leadingTextX + 4; update the same logic in TickerView (the shown method
using attributed, preferredTotalWidth, leadingTextX, visibleTextWidth) and
mirror the fix in CarouselTickerView and CompactTickerView to prevent
menu-bar/anchor jitter.
---
Outside diff comments:
In `@PanBar/Popover/PopoverController.swift`:
- Around line 49-57: The block-based NotificationCenter observer for
NSPopover.didCloseNotification is not stored or removed causing a leak; capture
the returned observer token (e.g., a property like popoverDidCloseObserver:
NSObjectProtocol?) when calling
NotificationCenter.default.addObserver(forName:object:queue:using:) and call
NotificationCenter.default.removeObserver(popoverDidCloseObserver!) (or
nil-check) in deinit (and do the same for the other observer at lines 60-63),
ensuring the closure still calls self?.handlePopoverClosed() via Task {
`@MainActor` in … } while storing/removing the token to prevent callback
retention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 01278719-cc55-43eb-a9cc-1c277c85f332
📒 Files selected for processing (21)
PanBar/Data/Persistence/Repositories/SettingsRepository.swiftPanBar/Domain/Services/PortfolioService.swiftPanBar/Infrastructure/HotkeyBinding.swiftPanBar/Infrastructure/NotificationService.swiftPanBar/Infrastructure/QuoteRefresher.swiftPanBar/Infrastructure/TickerPreferences.swiftPanBar/Infrastructure/Updater.swiftPanBar/MenuBar/CarouselTickerView.swiftPanBar/MenuBar/CompactTickerView.swiftPanBar/MenuBar/MenuBarTickerView.swiftPanBar/MenuBar/MinimalTickerView.swiftPanBar/MenuBar/StatusItemController.swiftPanBar/MenuBar/TickerRenderer.swiftPanBar/MenuBar/TickerView.swiftPanBar/Popover/PopoverController.swiftPanBar/Popover/Views/HoldingsTab.swiftPanBar/Popover/Views/SummaryCards.swiftPanBar/Resources/Localizable.xcstringsPanBar/Settings/Panes/PortfolioPane.swiftPanBar/Settings/Panes/TickerPane.swiftproject.yml
Comment on lines
33
to
35
| guard !items.isEmpty else { | ||
| return NSAttributedString( | ||
| string: L("ticker.empty", comment: "no quotes"), | ||
| attributes: [ | ||
| .font: font, | ||
| .foregroundColor: NSColor.secondaryLabelColor | ||
| ] | ||
| ) | ||
| return NSAttributedString() | ||
| } |
There was a problem hiding this comment.
空数据直接返回空文本会导致菜单栏入口“消失”。
建议保留最小占位文本（例如 — 或本地化 no-data），避免无图标场景下状态栏项宽度过小、用户难以发现和点击。
建议修复
func render(items: [TickerItem]) -> NSAttributedString {
guard !items.isEmpty else {
- return NSAttributedString()
+ return NSAttributedString(
+ string: "—",
+ attributes: [
+ .font: font,
+ .foregroundColor: NSColor.white.withAlphaComponent(0.55)
+ ]
+ )
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| guard !items.isEmpty else { | |
| return NSAttributedString( | |
| string: L("ticker.empty", comment: "no quotes"), | |
| attributes: [ | |
| .font: font, | |
| .foregroundColor: NSColor.secondaryLabelColor | |
| ] | |
| ) | |
| return NSAttributedString() | |
| } | |
| guard !items.isEmpty else { | |
| return NSAttributedString( | |
| string: "—", | |
| attributes: [ | |
| .font: font, | |
| .foregroundColor: NSColor.white.withAlphaComponent(0.55) | |
| ] | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@PanBar/MenuBar/TickerRenderer.swift` around lines 33 - 35, The guard that
returns an empty NSAttributedString when items.isEmpty causes the status item to
collapse; instead return a minimal placeholder string so the menu bar item
remains visible. In TickerRenderer (the method that builds the attributed title
where you currently check `guard !items.isEmpty else { return
NSAttributedString() }`), replace the empty return with an NSAttributedString
containing a single placeholder character (e.g. "—" or a localized "no data"
string) using the same attributes/style used for normal items so sizing and
accessibility remain consistent.
Owner
|
@pullfrog 干活 |
Owner
|
@1254455745 拆一下pr 太多了review不过来 |
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.
Summary
Test
Summary by CodeRabbit
发行说明
New Features
Bug Fixes